This is a placeholder for an upcoming patch to remove the front buffer mapping code from libdri and push that burden into the drivers. Making the front buffer mapping just as a back, depth or stencil buffer. This helps the situations where the frontbuffer may move around in graphics memory. As it does now in the i810 driver with rotation.
Created attachment 4623 [details] [review] removal of front buffer mapping from libdri. This is the initial patch to remove the front buffer map code from libdri and bump it's major version to 6.0.0. more to follow....
Created attachment 4624 [details] [review] remove front buffer mapping from libGL An initial patch for comment on the front buffer removal from libGL.
Created attachment 4625 [details] [review] i810 DDX patch to map front buffer patch to support front buffer mapping in the i810 DDX driver.
Created attachment 4626 [details] [review] i810 3D driver patch for front buffer mapping Add front buffer mapping code to 3D driver and update FB offset for renderbuffers.
Note: the above patches haven't been tested yet, but are up for review and comments. The driver patches give the outline of what needs to happen. These are only patches for the older i810/i815 chips, as the i830 already does the front buffer mapping itself, although libdri/libGL are still doing their own. The other drivers will need updating of which I doubt I'll be able to test all. Comments appreciated or can anyone help fixup the other drivers ?? (ffb looks easy as that also does it's own front buffer mapping)
Oh, and no version bump checks have been done in the patches, but that should absolutely be done with all of this.
Created attachment 4627 [details] [review] update createnewscreen in common layer Forgot this one to update the DRI drivers common layer to handle the changes.
Hi Alan, I had a quick look through the patches and they look good to me. I'll need to update the aiglx loader too, but that should be simple enough, it looks like I get to delete some code :). If we're bumping the DRI interface in an incompatible way, I have a couple of nitpicks I'd like to get fixed. They're not serious enough to warrant a version bump on their own - I currently work around them - but if you're doing this change I'd like to piggyback them onto it. Let me whip up a patch. These changes are for Mesa HEAD, and will be release in the 6.6 series, right?
Created attachment 4868 [details] [review] Proposed DRI interface changes Here's a patch with the changes I'd like to make to the DRI interface. The patch is preliminary, it still crashes, but it should give an idea of the direction. Basically I have these gripes with the DRI interface: - The drawable hash in the DRI driver common code solves a client side x protocol problem, that of attaching and managing data to drawable IDs. I'd like to move this code into libGL. In the x server I can attach the __DRIdrawable structs as private data on the pDrawables and delete them when I get a DrawableGone callback, so it's not needed there. There's also a problem with __driGarbageCollectDrawables() - it free's the __DRIdrawable, assuming it's the one that's been _mesa_malloc()'ed in DoBindDrawable(), which makes __DRIscreen::createNewDrawable() unusable if you want to embed the __DRIdrawable struct in another struct. - Since the drawable hash is now out of the dri driver, we can't pass drawable IDs to the DRI driver in bindContext and unbindContext, we have to pass __DRIdrawable pointers. At this point __DRIscreen::getDrawable is no longer necessary. - The __DRIscreen::modes field looks like a hack to let the DRI driver know how to create the __DRIdrawable using the right config. Since the DRI driver no longer implicitly creates the __DRIdrawables, this field can go away and the loader (libGL or aiglx) can pass the right mode to createNewDrawable. - framebuffer.dev_priv is allocated by the libGL transport layers using Xcalloc() but freed by the DRI driver using _mesa_free(). This is broken, and furthermore, in the X server we don't have to copy, we can just pass in the dev_private returned from the DRI module. - Not in the patch: we could probably change the DRI methods to take a __DRIscreen instead of a screen index and thus eliminate the need for __DRIinterfaceMethods::getScreen. This would be in line with the change to pass in __DRIdrawable pointers instead of drawable IDs.
Another thing to consider when updating the interface: we may want a way to pass the client application name to the DRI driver for the purpose of parsing .drirc. The .drirc file can have different settings for different applications and for the dri driver to parse the section corresponding to the remote client, we need to be able to pass the client name. This will also require a new GLX extension, of course, but for now I'm just making a note of it here for the purpose of dri driver interface changes.
way too intrusive for 7.1 at this point. moving out to 7.2.
Well 7.1 is released, any plans on pushing this one?
I'm going to revisit this shortly, but Kristian - what state are things in for your commits ? Are they still valid ?
(In reply to comment #13) > I'm going to revisit this shortly, but Kristian - what state are things in for > your commits ? Are they still valid ? The patch posted has a couple of crashers, but I have a working version in my git repo, I'll attach it here.
Kristian - have you got new patches yet ?
Hi Alan, Didn't forget about this, have just been busy with releases here. I'll get to it. Kristian
Created attachment 7266 [details] [review] patch against head Overview of the changes to the DRI interface. The big change in this patch is moving the DRI drawable hash table out of the DRI driver and into the libGL loader. The DRI driver shouldn't worry about server side resources, that's the protocol codes job. When the DRI driver is loaded by AIGLX, we can register for a callback when a window is destroyed and don't need this garbage collection, so the hash table is just in the way there. Furthermore in GLX 1.3, GLX drawables created using either glXCreatePixmap or glXCreateWindow are managed by the client and the client are expected to destroy these drawables once it has finished using them. This means that the DRI driver should not track or garbage collect drawables that are created this way. Also, there is currently a bug where __DRIscreen::createNewDrawable always adds the created drawable to the hash, even . When the DRI driver tries to garbage collect that drawable, it ends up freeing structures it didn't allocate and crashes. Below is a few comments on the changes to dri_interface.h: @@ -170,16 +170,6 @@ struct __DRIinterfaceMethodsRec { * the wire protocol (e.g., EGL) will implement glorified no-op functions. */ /*@{*/ - /** - * Determine if the specified window ID still exists. - * - * \note - * Implementations may assume that the driver will only pass an ID into - * this function that actually corresponds to a window. On - * implementations where windows can only be destroyed by the DRI driver - * (e.g., EGL), this function is allowed to always return \c GL_TRUE. - */ - GLboolean (*windowExists)(__DRInativeDisplay *dpy, __DRIid draw); /** * Create the server-side portion of the GL context. Since the garbage collection is now done in the libGL loader code, the DRI driver no longer needs to call into the loader and query existance of the corresponding X windows. @@ -287,12 +277,6 @@ struct __DRIscreenRec { int renderType, const int *attrs); - /** - * Method to return a pointer to the DRI drawable data. - */ - __DRIdrawable *(*getDrawable)(__DRInativeDisplay *dpy, __DRIid draw, - void *drawablePrivate); - The loader is expected to explicitly create and track the __DRIdrawable's it needs, so the DRI driver doesn't need to provide this entrypoint. @@ -361,27 +345,24 @@ struct __DRIcontextRec { void *private; - /** - * Pointer to the mode used to create this context. - * - * \since Internal API version 20040317. - */ - const __GLcontextModes * mode; - The DRI interface function tables should not contain data fields like this. This info should be stored in __DRIcontextPrivate by driCreateNewContext, instead of requiring users of the DRI interface to manually set it after calling the context constructor. However with the changes to move implicit GLX drawable to the loader, the DRI driver no longer needs this field at all. /** * Method to bind a DRI drawable to a DRI graphics context. * * \since Internal API version 20050727. */ - GLboolean (*bindContext)(__DRInativeDisplay *dpy, int scrn, __DRIid draw, - __DRIid read, __DRIcontext *ctx); + GLboolean (*bindContext)(__DRInativeDisplay *dpy, int scrn, + __DRIdrawable *pdraw, + __DRIdrawable *pread, + __DRIcontext *ctx); We now pass in pointers to the previously created __DRIdrawables instead of XIDs. /** * Method to unbind a DRI drawable from a DRI graphics context. * * \since Internal API version 20050727. */ - GLboolean (*unbindContext)(__DRInativeDisplay *dpy, int scrn, __DRIid draw\ , - __DRIid read, __DRIcontext *ctx); + GLboolean (*unbindContext)(__DRInativeDisplay *dpy, int scrn, + __DRIdrawable *pdraw, + __DRIdrawable *pread, + __DRIcontext *ctx); As for bindContext. In fact we don't need the drawable and readable here, they should just be dropped.
moving out of 7.2 blockage
can we do much to alleviate the backwards compat for this? or should we just wait until 7.2 is released with a corresponding Mesa, and then dump the patches into the server master + Mesa? I'm sure for the server DRI patch we could just add a new entry point for drivers to call that takes a map framebuffer arg and make the old entry point call it with the map set, if 0 was an illegal handle then it would be quite simple to fix up the DRIGetDeviceInfo to fill out the struct or not on the null handle.. or we could add a new member to the DriScreenPrivPtr struct...
(In reply to comment #17) > Created an attachment (id=7266) [edit] > patch against head > > Overview of the changes to the DRI interface. > > The big change in this patch is moving the DRI drawable hash table out > of the DRI driver and into the libGL loader. The DRI driver shouldn't > worry about server side resources, that's the protocol codes job. > When the DRI driver is loaded by AIGLX, we can register for a callback > when a window is destroyed and don't need this garbage collection, so > the hash table is just in the way there. > > Furthermore in GLX 1.3, GLX drawables created using either > glXCreatePixmap or glXCreateWindow are managed by the client and the > client are expected to destroy these drawables once it has finished > using them. This means that the DRI driver should not track or > garbage collect drawables that are created this way. > > Also, there is currently a bug where __DRIscreen::createNewDrawable > always adds the created drawable to the hash, even . When the DRI driver > tries to garbage collect that drawable, it ends up freeing structures > it didn't allocate and crashes. I'd like to recommend a couple additional changes. - Remove the parameters to unbindContext that you noted were unnecessary. - Replace the dpy/screen tuple that is passed to many functions with a __DRIscreen pointer. It appears that the dpy is passed into the driver just so that the driver can pass it back to the loader. Instead store the dpy/screen in the loader private __DRIscreen data (screenConfigs). This would also eliminate all calls to getScreen. - Remove getScreen. Can we get this code, even as it is, committed? Maybe it would be best to have it on a branch?
I'll get this in tomorrow. I think it's lingered long enough now. And 7.2 is on it's own branch anyway. I'll pop it straight into the trunk.
(In reply to comment #20) > I'd like to recommend a couple additional changes. > > - Remove the parameters to unbindContext that you noted were unnecessary. > > - Replace the dpy/screen tuple that is passed to many functions with a > __DRIscreen pointer. It appears that the dpy is passed into the driver just so > that the driver can pass it back to the loader. Instead store the dpy/screen in > the loader private __DRIscreen data (screenConfigs). This would also eliminate > all calls to getScreen. > > - Remove getScreen. These changes all make a lot of sense, I've wanted to do something similar in a follow up patch. In fact, we shouldn't publish a new DRI interface version without these changes. It shouldn't be a lot of work and I may be able to get a patch done, but I'm temporarily not working on this stuff so I can't guarantee anything.
I'll put this stuff on a branch then, as the other DRI drivers have to play catch up too. I'll update this bug tomorrow.
I've started branches "frontbuffer-removal" in xorg and mesa to track these changes. I'm going to start a "frontbuffer-removal" in the i810 driver too. It'd be nice for other driver maintainers to create similar branches and fix up their DRI code for these changes.
Kristian, Eric has just committed some modifications to allow the drivers to not map the frontbuffer. But I'd rather get us all in-sync now before this gets too far out in the wild. What's the state of play with your code ? Eric's commit only happened an hour or so ago. Comments ?
(In reply to comment #25) > Kristian, > > Eric has just committed some modifications to allow the drivers to not map the > frontbuffer. That was actually my change and the way it's done, it doesn't break any ABIs and is pretty harmless as a small, incremental change. > But I'd rather get us all in-sync now before this gets too far out in the wild. > > What's the state of play with your code ? > > Eric's commit only happened an hour or so ago. Right now, Eric, Dave Airlied and I are working on getting ttm into the DDX driver and updating the X server / DRI driver interfaces to pass buffer object handles back and forth. At this point, the code that Dave and I are working with is mostly a prototype to see how ttm holds up in this use case and to figure out what kind of interfaces we need. The code is in our personal xserver, xf86-video-intel, mesa and drm trees, but it is mainly just a vehicle for understanding how things work. I think Eric is working towards a more production ready implementation, but I'm not sure how far along he is. I'm hoping to have something working soon, and then I'll try to write up the changes in drm and dri driver interfaces and send out an email. This mainly why the dri interface changes I've been working on have stalled - I wanted to make the new interface ttm proof.
ah great. Thanks for the info.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.