[Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers

[Ben Widawsky]

On 17-03-10 10:28:42, Emil Velikov wrote:
>Hi Ben,
>
>Mostly pointing out a few things that look strange, pardon if some
>seem too pedantic.
>
>On 10 March 2017 at 01:48, Ben Widawsky <ben at bwidawsk.net> wrote:
>
>> ---
>>  include/GL/internal/dri_interface.h      | 27 ++++++++++++++++++++++++++-
>Split the Infra from the i965 implementation ?
>

Sure. Doesn't matter to me.

>>  src/gallium/state_trackers/dri/dri2.c    |  1 +
>Not needed.
>

Yeah, whoops.

>>  src/mesa/drivers/dri/i965/intel_screen.c | 32 +++++++++++++++++++++++++++++++-
>Patch should come as/after __DRI_IMAGE_ATTRIB_MODIFIER_* are honoured.
>Or modifiers and modifiers count in general.
>

Assuming this also means split the infra from implementation, sure.

>> --- a/src/gallium/state_trackers/dri/dri2.c
>> +++ b/src/gallium/state_trackers/dri/dri2.c
>> @@ -1413,6 +1413,7 @@ static __DRIimageExtension dri2ImageExtension = {
>>      .getCapabilities              = dri2_get_capabilities,
>>      .mapImage                     = dri2_map_image,
>>      .unmapImage                   = dri2_unmap_image,
>> +    .createImageWithModifiers     = NULL,
>>  };
>>
>Not needed - drop ?
>
>

Gone.

>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
>> index 21786eb54a..3452572874 100644
>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>> @@ -510,9 +510,11 @@ intel_destroy_image(__DRIimage *image)
>>  }
>>
>>  static __DRIimage *
>> -intel_create_image(__DRIscreen *dri_screen,
>> +__intel_create_image(__DRIscreen *dri_screen,
>Don't think there's (m)any functions that start with __ in 965.
>Perhaps append "_common" ?
>
>

Sure.

>>     int cpp;
>>     unsigned long pitch;
>>
>> +   /* Callers of this may specify a modifier, or a dri usage, but not both. The
>> +    * newer modifier interface deprecates the older usage flags newer modifier
>> +    * interface deprecates the older usage flags.
>> +    */
>> +   assert(!(use && count));
>> +
>What would happen if we don't have either old (use) or new (modifiers) ?
>Shouldn't this be XOR ?
>

Not having either is fine. It's like the previous case of supporting use flags
but passing in none, which is allowed.

>
>> @@ -840,6 +869,7 @@ static const __DRIimageExtension intelImageExtension = {
>Afaict, nothing in this series bumps the version to 14.
>So you either missed a patch or we have a bug somewhere ?
>

Well, it bumps the version in dri_interface.h. Formerly, before splitting things
up, this patch had the i965 version bumped too, but that's gone now. I'm happy
to reword this however you'd like.

>-Emil