Re: [PATCH 7/8] dri: add __DRIimageLoaderExtension and __DRIimageDriverExtension

[Keith Packard <keithp@keithp.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3123 bytes --]

Eric Anholt <eric@anholt.net> writes:

> Most of my review was going to be whining about yet another (broken)
> copy of dri2CreateNewScreen2.  Sounds like you've fixed that.

Yup, figured that out all on my own after I re-read the code -- the only
difference was that I need to look for the DRIimageLoader hooks, which I
now just do in the shared function.

>> +
>> +#define __DRI_DRIVER_EXTENSIONS "__driDriverExtensions"
>
> This looks like rebase fail

Removed.

>> +//struct gl_context;
>> +//struct dd_function_table;
>
> Looks like development leftovers.

Removed.

> Maybe append "Func" to the typedefs so they don't look like just another
> struct in the declarations?  And since they're supposed to be the same
> function pointers as in the __DRIswrastExtensionRec and
> __DRIdri2ExtensionRec, change them to this typedef, too?

I've moved the typedefs, renamed them and stuck that part of the patch
in the first patch of the series that renames the functions.

> It looks like getBuffers could just be two getBuffer calls, except for
> the updating of width and height.  Have you looked into doing things
> that way at all?

Yes, that was the way I started doing it. There are two reasons for
doing it in a single call:

 1) Pixmaps always need a front buffer, and the driver doesn't know
    which drawables are pixmaps.

 2) Deleting buffers when changing modes. I'd need to add another
    function to let the driver delete unused buffers.
  
Overall, I liked moving more buffer management logic to the loader and
out of the driver, so I followed the DRI2 API here.

> Style nit: we try and put braces around multi-line things like this,
> even if they are a single statement.

Fixed.

>> -bool
>> +GLboolean
>>  brwCreateContext(gl_api api,
>>  	         const struct gl_config *mesaVis,
>>  		 __DRIcontext *driContextPriv,
...
>>                                    __DRIdrawable *drawable);
>>  
>> -bool brwCreateContext(gl_api api,
>> -		      const struct gl_config *mesaVis,
>> -		      __DRIcontext *driContextPriv,
>> -                      unsigned major_version,
>> -                      unsigned minor_version,
>> -                      uint32_t flags,
>> -                      unsigned *error,
>> -		      void *sharedContextPrivate);
>> +GLboolean brwCreateContext(gl_api api,
>> +                           const struct gl_config *mesaVis,
>> +                           __DRIcontext *driContextPriv,
>> +                           unsigned major_version,
>> +                           unsigned minor_version,
>> +                           uint32_t flags,
>> +                           unsigned *error,
>> +                           void *sharedContextPrivate);
...
>
> Unrelated change?

Pulled out to a separate patch -- the return type for createContext is
GLboolean, not bool, and my compiler was whinging.

I've pushed these changes, along with a bunch of new comments in
dri3_glx.c to my dri3 branch. I can send the new series to the list if
that would be helpful.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel