Bug 5714

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.