[Mesa-dev] [PATCH 08/27] dri: Add an image creation with modifiers

[Ben Widawsky]

On 16-12-02 18:01:02, Eric Engestrom wrote:
>On Thursday, 2016-12-01 14:09:49 -0800, Ben Widawsky wrote:
>> From: Ben Widawsky <ben at bwidawsk.net>
>>
>> Modifiers will be obtains or guessed by the client and passed in during
>> image creation/import.
>>
>> This requires bumping the DRIimage version.
>>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> ---
>>  include/GL/internal/dri_interface.h      | 28 +++++++++++++++++++++++++++-
>>  src/gallium/state_trackers/dri/dri2.c    |  1 +
>>  src/mesa/drivers/dri/i965/intel_screen.c | 26 +++++++++++++++++++++++++-
>>  3 files changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
>> index d0b1bc6..657e158 100644
>> --- a/include/GL/internal/dri_interface.h
>> +++ b/include/GL/internal/dri_interface.h
>> @@ -1094,7 +1094,7 @@ struct __DRIdri2ExtensionRec {
>>   * extensions.
>>   */
>>  #define __DRI_IMAGE "DRI_IMAGE"
>> -#define __DRI_IMAGE_VERSION 13
>> +#define __DRI_IMAGE_VERSION 14
>>
>>  /**
>>   * These formats correspond to the similarly named MESA_FORMAT_*
>> @@ -1209,6 +1209,8 @@ struct __DRIdri2ExtensionRec {
>>  #define __DRI_IMAGE_ATTRIB_NUM_PLANES   0x2009 /* available in versions 11 */
>>
>>  #define __DRI_IMAGE_ATTRIB_OFFSET 0x200A /* available in versions 13 */
>> +#define __DRI_IMAGE_ATTRIB_MODIFIER_LOWER 0x200B /* available in versions 14 */
>> +#define __DRI_IMAGE_ATTRIB_MODIFIER_UPPER 0x200C /* available in versions 14 */
>>
>>  enum __DRIYUVColorSpace {
>>     __DRI_YUV_COLOR_SPACE_UNDEFINED = 0,
>> @@ -1420,6 +1422,30 @@ struct __DRIimageExtensionRec {
>>      */
>>     void (*unmapImage)(__DRIcontext *context, __DRIimage *image, void *data);
>>
>> +
>> +   /**
>> +    * Creates an image with implementations favorite modifiers.
>> +    *
>> +    * This acts like createImage except there is a list of modifiers passed in
>> +    * which the implementation may selectively use to create the DRIimage. The
>> +    * result should be the implementation selects one modifier (perhaps it would
>> +    * hold on to a few and later pick).
>> +    *
>> +    * The created image should be destroyed with destroyImage().
>> +    *
>> +    * Returns the new DRIimage. The chosen modifier can be obtained later on
>> +    * through some API visible functionality if required.
>> +    *
>> +    * \sa __DRIimageRec::createImage
>> +    *
>> +    * \since 14
>> +    */
>> +   __DRIimage *(*createImageWithModifiers)(__DRIscreen *screen,
>> +                                           int width, int height, int format,
>> +                                           unsigned int use,
>> +                                           const uint64_t *modifiers,
>> +                                           const unsigned int modifier_count,
>> +                                           void *loaderPrivate);
>>  };
>>
>>
>> diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
>> index 9ec069b..c9fbe84 100644
>> --- a/src/gallium/state_trackers/dri/dri2.c
>> +++ b/src/gallium/state_trackers/dri/dri2.c
>> @@ -1409,6 +1409,7 @@ static __DRIimageExtension dri2ImageExtension = {
>>      .getCapabilities              = dri2_get_capabilities,
>>      .mapImage                     = dri2_map_image,
>>      .unmapImage                   = dri2_unmap_image,
>> +    .createImageWithModifiers     = NULL,
>>  };
>>
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
>> index 5808bde..b5bb4a0 100644
>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>> @@ -538,9 +538,11 @@ intel_destroy_image(__DRIimage *image)
>>  }
>>
>>  static __DRIimage *
>> -intel_create_image(__DRIscreen *dri_screen,
>> +__intel_create_image(__DRIscreen *dri_screen,
>>  		   int width, int height, int format,
>>  		   unsigned int use,
>> +                   const uint64_t *modifiers,
>> +                   unsigned count,
>>  		   void *loaderPrivate)
>>  {
>>     __DRIimage *image;
>> @@ -578,6 +580,27 @@ intel_create_image(__DRIscreen *dri_screen,
>>     return image;
>>  }
>>
>> +static __DRIimage *
>> +intel_create_image(__DRIscreen *dri_screen,
>> +		   int width, int height, int format,
>> +		   unsigned int use,
>> +		   void *loaderPrivate)
>> +{
>> +   return __intel_create_image(dri_screen, width, height, format, use, NULL, 0, loaderPrivate);
>> +}
>> +
>> +static __DRIimage *
>> +intel_create_image_with_modifiers(__DRIscreen *dri_screen,
>> +                                  int width, int height, int format,
>> +                                  unsigned int use,
>> +                                  const uint64_t *modifiers,
>> +                                  const unsigned count,
>> +                                  void *loaderPrivate)
>> +{
>> +   return __intel_create_image(dri_screen, width, height, format, use, NULL, 0,
>
>I think you meant to use `modifiers` and `count` here :P
>
>If you really want to leave them out (because they're not used yet?),
>you might want to add a note in the commit message.
>(I know, you replace this code in the next patch, but I think commits
>should also make sense on their own. Otherwise, why have two commits?)
>

I'll update the commit message. There is now an assert in this patch which
prevents usage + count being used simultaneously (a request by Kristian).

>Aside from this and my other nit-picks, the series is
>Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
>up to patch #11 (the rest is too intel-specific for me, I'm afraid).
>
>Cheers,
>  Eric
>

Done and done. Please see my response about the plane signedness. I'll send out
v2 shortly.

>> +                               loaderPrivate);
>> +}
>> +
>>  static GLboolean
>>  intel_query_image(__DRIimage *image, int attrib, int *value)
>>  {
>> @@ -870,6 +893,7 @@ static const __DRIimageExtension intelImageExtension = {
>>      .getCapabilities                    = NULL,
>>      .mapImage                           = NULL,
>>      .unmapImage                         = NULL,
>> +    .createImageWithModifiers           = intel_create_image_with_modifiers,
>>  };
>>
>>  static int
>> --
>> 2.10.2
>>