[Mesa-dev,1/3] dri: Add helpers for implementing allocBuffer/releaseBuffer with __DRIimage

Submitted by Kristian H. Kristensen on Nov. 11, 2013, 9:22 p.m.

Details

Message ID 1384204927-22184-1-git-send-email-krh@bitplanet.net
State New
Headers show

Not browsing as part of any series.

Commit Message

Kristian H. Kristensen Nov. 11, 2013, 9:22 p.m.
Drivers that only call getBuffers to request color buffers can use these
generic, __DRIimage based helpers to implement the allocBuffer and
releaseBuffer functions of __DRIdri2Extension.

For the intel dri driver, this consolidates window system color buffer
allocation in intel_create_image().

Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
---
 src/mesa/drivers/dri/common/dri_util.c   | 75 ++++++++++++++++++++++++++++++++
 src/mesa/drivers/dri/common/dri_util.h   | 10 +++++
 src/mesa/drivers/dri/i915/intel_screen.c | 56 +-----------------------
 src/mesa/drivers/dri/i965/intel_screen.c | 55 +----------------------
 4 files changed, 89 insertions(+), 107 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/common/dri_util.c b/src/mesa/drivers/dri/common/dri_util.c
index 86cf24c..a7328e4 100644
--- a/src/mesa/drivers/dri/common/dri_util.c
+++ b/src/mesa/drivers/dri/common/dri_util.c
@@ -115,6 +115,7 @@  driCreateNewScreen2(int scrn, int fd,
 {
     static const __DRIextension *emptyExtensionList[] = { NULL };
     __DRIscreen *psp;
+    int i;
 
     psp = calloc(1, sizeof(*psp));
     if (!psp)
@@ -161,6 +162,11 @@  driCreateNewScreen2(int scrn, int fd,
 	return NULL;
     }
 
+    for (i = 0; psp->extensions[i]; i++) {
+       if (strcmp(psp->extensions[i]->name, __DRI_IMAGE) == 0)
+          psp->image_extension = (__DRIimageExtension *) psp->extensions[i];
+    }
+
     int gl_version_override = _mesa_get_gl_version_override();
     if (gl_version_override >= 31) {
        psp->max_gl_core_version = MAX2(psp->max_gl_core_version,
@@ -862,6 +868,75 @@  driImageFormatToGLFormat(uint32_t image_format)
    }
 }
 
+struct dri_image_buffer {
+   __DRIbuffer base;
+   __DRIimage *image;
+};
+
+__DRIbuffer *
+driAllocateBuffer(__DRIscreen *screen,
+                  unsigned attachment, unsigned format,
+                  int width, int height)
+{
+   struct dri_image_buffer *buffer;
+   __DRIimageExtension *image = screen->image_extension;
+   int dri_format, name, stride;
+
+   assert(attachment == __DRI_BUFFER_FRONT_LEFT ||
+          attachment == __DRI_BUFFER_BACK_LEFT);
+
+   /* We just need a __DRI_IMAGE_FORMAT code that has a cpp that matches
+    * format / 8.  The image format code is stored in the __DRIimage, but the
+    * __DRIbuffer we create from the image, only stores the cpp. */
+
+   switch (format) {
+   case 32:
+      dri_format = __DRI_IMAGE_FORMAT_XRGB8888;
+      break;
+   case 16:
+      dri_format = __DRI_IMAGE_FORMAT_RGB565;
+      break;
+   default:
+      return NULL;
+   }
+
+   buffer = calloc(1, sizeof *buffer);
+   if (buffer == NULL)
+      return NULL;
+
+   buffer->image = image->createImage(screen,
+                                      width, height, dri_format,
+                                      __DRI_IMAGE_USE_SHARE |
+                                      __DRI_IMAGE_USE_SCANOUT,
+                                      buffer);
+
+   if (buffer->image == NULL) {
+	   free(buffer);
+	   return NULL;
+   }
+
+   image->queryImage(buffer->image, __DRI_IMAGE_ATTRIB_NAME, &name);
+   image->queryImage(buffer->image, __DRI_IMAGE_ATTRIB_STRIDE, &stride);
+
+   buffer->base.attachment = attachment;
+   buffer->base.name = name;
+   buffer->base.pitch = stride;
+   buffer->base.cpp = format / 8;
+
+   return &buffer->base;
+}
+
+void
+driReleaseBuffer(__DRIscreen *screen, __DRIbuffer *_buffer)
+{
+   struct dri_image_buffer *buffer = (struct dri_image_buffer *) _buffer;
+   __DRIimageExtension *image = screen->image_extension;
+
+   image->destroyImage(buffer->image);
+   free(buffer);
+}
+
+
 /** Image driver interface */
 const __DRIimageDriverExtension driImageDriverExtension = {
     .base = { __DRI_IMAGE_DRIVER, __DRI_IMAGE_DRIVER_VERSION },
diff --git a/src/mesa/drivers/dri/common/dri_util.h b/src/mesa/drivers/dri/common/dri_util.h
index 79a8564..240213d 100644
--- a/src/mesa/drivers/dri/common/dri_util.h
+++ b/src/mesa/drivers/dri/common/dri_util.h
@@ -165,6 +165,7 @@  struct __DRIscreenRec {
     int max_gl_es2_version;
 
     const __DRIextension **extensions;
+   __DRIimageExtension *image_extension;
 
     const __DRIswrastLoaderExtension *swrast_loader;
 
@@ -291,4 +292,13 @@  driUpdateFramebufferSize(struct gl_context *ctx, const __DRIdrawable *dPriv);
 
 extern const __DRIimageDriverExtension driImageDriverExtension;
 
+extern __DRIbuffer *
+driAllocateBuffer(__DRIscreen *screen,
+                  unsigned attachment, unsigned format,
+                  int width, int height);
+
+extern void
+driReleaseBuffer(__DRIscreen *screen, __DRIbuffer *_buffer);
+
+
 #endif /* _DRI_UTIL_H_ */
diff --git a/src/mesa/drivers/dri/i915/intel_screen.c b/src/mesa/drivers/dri/i915/intel_screen.c
index 2c309ed..a143652 100644
--- a/src/mesa/drivers/dri/i915/intel_screen.c
+++ b/src/mesa/drivers/dri/i915/intel_screen.c
@@ -1185,58 +1185,6 @@  __DRIconfig **intelInitScreen2(__DRIscreen *psp)
    return (const __DRIconfig**) intel_screen_make_configs(psp);
 }
 
-struct intel_buffer {
-   __DRIbuffer base;
-   struct intel_region *region;
-};
-
-static __DRIbuffer *
-intelAllocateBuffer(__DRIscreen *screen,
-		    unsigned attachment, unsigned format,
-		    int width, int height)
-{
-   struct intel_buffer *intelBuffer;
-   struct intel_screen *intelScreen = screen->driverPrivate;
-
-   assert(attachment == __DRI_BUFFER_FRONT_LEFT ||
-          attachment == __DRI_BUFFER_BACK_LEFT);
-
-   intelBuffer = calloc(1, sizeof *intelBuffer);
-   if (intelBuffer == NULL)
-      return NULL;
-
-   /* The front and back buffers are color buffers, which are X tiled. */
-   intelBuffer->region = intel_region_alloc(intelScreen,
-                                            I915_TILING_X,
-                                            format / 8,
-                                            width,
-                                            height,
-                                            true);
-   
-   if (intelBuffer->region == NULL) {
-	   free(intelBuffer);
-	   return NULL;
-   }
-   
-   intel_region_flink(intelBuffer->region, &intelBuffer->base.name);
-
-   intelBuffer->base.attachment = attachment;
-   intelBuffer->base.cpp = intelBuffer->region->cpp;
-   intelBuffer->base.pitch = intelBuffer->region->pitch;
-
-   return &intelBuffer->base;
-}
-
-static void
-intelReleaseBuffer(__DRIscreen *screen, __DRIbuffer *buffer)
-{
-   struct intel_buffer *intelBuffer = (struct intel_buffer *) buffer;
-
-   intel_region_release(&intelBuffer->region);
-   free(intelBuffer);
-}
-
-
 static const struct __DriverAPIRec i915_driver_api = {
    .InitScreen		 = intelInitScreen2,
    .DestroyScreen	 = intelDestroyScreen,
@@ -1246,8 +1194,8 @@  static const struct __DriverAPIRec i915_driver_api = {
    .DestroyBuffer	 = intelDestroyBuffer,
    .MakeCurrent		 = intelMakeCurrent,
    .UnbindContext	 = intelUnbindContext,
-   .AllocateBuffer       = intelAllocateBuffer,
-   .ReleaseBuffer        = intelReleaseBuffer
+   .AllocateBuffer       = driAllocateBuffer,
+   .ReleaseBuffer        = driReleaseBuffer
 };
 
 static const struct __DRIDriverVtableExtensionRec i915_vtable = {
diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
index e39d654..92b2eac 100644
--- a/src/mesa/drivers/dri/i965/intel_screen.c
+++ b/src/mesa/drivers/dri/i965/intel_screen.c
@@ -1302,57 +1302,6 @@  __DRIconfig **intelInitScreen2(__DRIscreen *psp)
    return (const __DRIconfig**) intel_screen_make_configs(psp);
 }
 
-struct intel_buffer {
-   __DRIbuffer base;
-   struct intel_region *region;
-};
-
-static __DRIbuffer *
-intelAllocateBuffer(__DRIscreen *screen,
-		    unsigned attachment, unsigned format,
-		    int width, int height)
-{
-   struct intel_buffer *intelBuffer;
-   struct intel_screen *intelScreen = screen->driverPrivate;
-
-   assert(attachment == __DRI_BUFFER_FRONT_LEFT ||
-          attachment == __DRI_BUFFER_BACK_LEFT);
-
-   intelBuffer = calloc(1, sizeof *intelBuffer);
-   if (intelBuffer == NULL)
-      return NULL;
-
-   /* The front and back buffers are color buffers, which are X tiled. */
-   intelBuffer->region = intel_region_alloc(intelScreen,
-                                            I915_TILING_X,
-                                            format / 8,
-                                            width,
-                                            height,
-                                            true);
-   
-   if (intelBuffer->region == NULL) {
-	   free(intelBuffer);
-	   return NULL;
-   }
-   
-   intel_region_flink(intelBuffer->region, &intelBuffer->base.name);
-
-   intelBuffer->base.attachment = attachment;
-   intelBuffer->base.cpp = intelBuffer->region->cpp;
-   intelBuffer->base.pitch = intelBuffer->region->pitch;
-
-   return &intelBuffer->base;
-}
-
-static void
-intelReleaseBuffer(__DRIscreen *screen, __DRIbuffer *buffer)
-{
-   struct intel_buffer *intelBuffer = (struct intel_buffer *) buffer;
-
-   intel_region_release(&intelBuffer->region);
-   free(intelBuffer);
-}
-
 static const struct __DriverAPIRec brw_driver_api = {
    .InitScreen		 = intelInitScreen2,
    .DestroyScreen	 = intelDestroyScreen,
@@ -1362,8 +1311,8 @@  static const struct __DriverAPIRec brw_driver_api = {
    .DestroyBuffer	 = intelDestroyBuffer,
    .MakeCurrent		 = intelMakeCurrent,
    .UnbindContext	 = intelUnbindContext,
-   .AllocateBuffer       = intelAllocateBuffer,
-   .ReleaseBuffer        = intelReleaseBuffer
+   .AllocateBuffer       = driAllocateBuffer,
+   .ReleaseBuffer        = driReleaseBuffer
 };
 
 static const struct __DRIDriverVtableExtensionRec brw_vtable = {

Comments

On 11/11/2013 01:22 PM, Kristian Høgsberg wrote:
> Drivers that only call getBuffers to request color buffers can use these
> generic, __DRIimage based helpers to implement the allocBuffer and
> releaseBuffer functions of __DRIdri2Extension.
>
> For the intel dri driver, this consolidates window system color buffer
> allocation in intel_create_image().
>
> Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
> ---
>   src/mesa/drivers/dri/common/dri_util.c   | 75 ++++++++++++++++++++++++++++++++
>   src/mesa/drivers/dri/common/dri_util.h   | 10 +++++
>   src/mesa/drivers/dri/i915/intel_screen.c | 56 +-----------------------
>   src/mesa/drivers/dri/i965/intel_screen.c | 55 +----------------------
>   4 files changed, 89 insertions(+), 107 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/common/dri_util.c b/src/mesa/drivers/dri/common/dri_util.c
> index 86cf24c..a7328e4 100644
> --- a/src/mesa/drivers/dri/common/dri_util.c
> +++ b/src/mesa/drivers/dri/common/dri_util.c


> +__DRIbuffer *
> +driAllocateBuffer(__DRIscreen *screen,
> +                  unsigned attachment, unsigned format,
> +                  int width, int height)
> +{
> +   struct dri_image_buffer *buffer;
> +   __DRIimageExtension *image = screen->image_extension;
> +   int dri_format, name, stride;
> +
> +   assert(attachment == __DRI_BUFFER_FRONT_LEFT ||
> +          attachment == __DRI_BUFFER_BACK_LEFT);
> +
> +   /* We just need a __DRI_IMAGE_FORMAT code that has a cpp that matches
> +    * format / 8.  The image format code is stored in the __DRIimage, but the
> +    * __DRIbuffer we create from the image, only stores the cpp. */
> +
> +   switch (format) {
> +   case 32:
> +      dri_format = __DRI_IMAGE_FORMAT_XRGB8888;
> +      break;
> +   case 16:
> +      dri_format = __DRI_IMAGE_FORMAT_RGB565;
> +      break;
> +   default:
> +      return NULL;
> +   }
> +
> +   buffer = calloc(1, sizeof *buffer);
> +   if (buffer == NULL)
> +      return NULL;
> +
> +   buffer->image = image->createImage(screen,
> +                                      width, height, dri_format,
> +                                      __DRI_IMAGE_USE_SHARE |
> +                                      __DRI_IMAGE_USE_SCANOUT,
> +                                      buffer);

It's incorrect to specify __DRI_IMAGE_USE_SCANOUT regardless of
attachment type. GBM, Wayland, and Android use driAllocateBuffer
to allocate more than just the scanout buffer. They use it to
allocate auxillary buffers too. If i965 were to respect the
caching restrictions implied by __DRI_IMAGE_USE_SCANOUT, its
use for aux buffers would hurt performance. (As far as I can
tell, though, intel_create_image() wrongly ignores __DRI_IMAGE_USE_SCANOUT).

Instead, I think you should set the USE_SCANOUT bit if and only if
attachment is one of __DRI_BUFFER_(BACK|FRONT)_(LEFT|RIGHT).

> +
> +   if (buffer->image == NULL) {
> +	   free(buffer);
> +	   return NULL;
> +   }
> +
> +   image->queryImage(buffer->image, __DRI_IMAGE_ATTRIB_NAME, &name);
> +   image->queryImage(buffer->image, __DRI_IMAGE_ATTRIB_STRIDE, &stride);
> +
> +   buffer->base.attachment = attachment;
> +   buffer->base.name = name;
> +   buffer->base.pitch = stride;
> +   buffer->base.cpp = format / 8;
> +
> +   return &buffer->base;
> +}
On Mon, Nov 11, 2013 at 1:57 PM, Chad Versace
<chad.versace@linux.intel.com> wrote:
> On 11/11/2013 01:22 PM, Kristian Høgsberg wrote:
>>
>> Drivers that only call getBuffers to request color buffers can use these
>> generic, __DRIimage based helpers to implement the allocBuffer and
>> releaseBuffer functions of __DRIdri2Extension.
>>
>> For the intel dri driver, this consolidates window system color buffer
>> allocation in intel_create_image().
>>
>> Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
>> ---
>>   src/mesa/drivers/dri/common/dri_util.c   | 75
>> ++++++++++++++++++++++++++++++++
>>   src/mesa/drivers/dri/common/dri_util.h   | 10 +++++
>>   src/mesa/drivers/dri/i915/intel_screen.c | 56 +-----------------------
>>   src/mesa/drivers/dri/i965/intel_screen.c | 55 +----------------------
>>   4 files changed, 89 insertions(+), 107 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/common/dri_util.c
>> b/src/mesa/drivers/dri/common/dri_util.c
>> index 86cf24c..a7328e4 100644
>> --- a/src/mesa/drivers/dri/common/dri_util.c
>> +++ b/src/mesa/drivers/dri/common/dri_util.c
>
>
>
>> +__DRIbuffer *
>> +driAllocateBuffer(__DRIscreen *screen,
>> +                  unsigned attachment, unsigned format,
>> +                  int width, int height)
>> +{
>> +   struct dri_image_buffer *buffer;
>> +   __DRIimageExtension *image = screen->image_extension;
>> +   int dri_format, name, stride;
>> +
>> +   assert(attachment == __DRI_BUFFER_FRONT_LEFT ||
>> +          attachment == __DRI_BUFFER_BACK_LEFT);
>> +
>> +   /* We just need a __DRI_IMAGE_FORMAT code that has a cpp that matches
>> +    * format / 8.  The image format code is stored in the __DRIimage, but
>> the
>> +    * __DRIbuffer we create from the image, only stores the cpp. */
>> +
>> +   switch (format) {
>> +   case 32:
>> +      dri_format = __DRI_IMAGE_FORMAT_XRGB8888;
>> +      break;
>> +   case 16:
>> +      dri_format = __DRI_IMAGE_FORMAT_RGB565;
>> +      break;
>> +   default:
>> +      return NULL;
>> +   }
>> +
>> +   buffer = calloc(1, sizeof *buffer);
>> +   if (buffer == NULL)
>> +      return NULL;
>> +
>> +   buffer->image = image->createImage(screen,
>> +                                      width, height, dri_format,
>> +                                      __DRI_IMAGE_USE_SHARE |
>> +                                      __DRI_IMAGE_USE_SCANOUT,
>> +                                      buffer);
>
>
> It's incorrect to specify __DRI_IMAGE_USE_SCANOUT regardless of
> attachment type. GBM, Wayland, and Android use driAllocateBuffer
> to allocate more than just the scanout buffer. They use it to
> allocate auxillary buffers too. If i965 were to respect the
> caching restrictions implied by __DRI_IMAGE_USE_SCANOUT, its
> use for aux buffers would hurt performance. (As far as I can
> tell, though, intel_create_image() wrongly ignores __DRI_IMAGE_USE_SCANOUT).
>
> Instead, I think you should set the USE_SCANOUT bit if and only if
> attachment is one of __DRI_BUFFER_(BACK|FRONT)_(LEFT|RIGHT).

The commit message says:

"Drivers that only call getBuffers to request color buffers can use these
generic, __DRIimage based helpers to implement the allocBuffer and
releaseBuffer functions of __DRIdri2Extension."

which is true for the Intel DRI driver.  The DRI2 interface allows the
driver to ask for auxillary buffers, but since we stopped supporting
multiple processes rendering to the same X window, our driver no
longer does that.  This is under driver control and thus if a driver
knows it will never ask for aux buffers, it can use these helpers.

Kristian
>
>> +
>> +   if (buffer->image == NULL) {
>> +          free(buffer);
>> +          return NULL;
>> +   }
>> +
>> +   image->queryImage(buffer->image, __DRI_IMAGE_ATTRIB_NAME, &name);
>> +   image->queryImage(buffer->image, __DRI_IMAGE_ATTRIB_STRIDE, &stride);
>> +
>> +   buffer->base.attachment = attachment;
>> +   buffer->base.name = name;
>> +   buffer->base.pitch = stride;
>> +   buffer->base.cpp = format / 8;
>> +
>> +   return &buffer->base;
>> +}
>
>
On 11/11/2013 02:20 PM, Kristian Høgsberg wrote:
> On Mon, Nov 11, 2013 at 1:57 PM, Chad Versace
> <chad.versace@linux.intel.com> wrote:
>> On 11/11/2013 01:22 PM, Kristian Høgsberg wrote:
>>>
>>> Drivers that only call getBuffers to request color buffers can use these
>>> generic, __DRIimage based helpers to implement the allocBuffer and
>>> releaseBuffer functions of __DRIdri2Extension.
>>>
>>> For the intel dri driver, this consolidates window system color buffer
>>> allocation in intel_create_image().
>>>
>>> Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
>>> ---
>>>    src/mesa/drivers/dri/common/dri_util.c   | 75
>>> ++++++++++++++++++++++++++++++++
>>>    src/mesa/drivers/dri/common/dri_util.h   | 10 +++++
>>>    src/mesa/drivers/dri/i915/intel_screen.c | 56 +-----------------------
>>>    src/mesa/drivers/dri/i965/intel_screen.c | 55 +----------------------
>>>    4 files changed, 89 insertions(+), 107 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/common/dri_util.c
>>> b/src/mesa/drivers/dri/common/dri_util.c
>>> index 86cf24c..a7328e4 100644
>>> --- a/src/mesa/drivers/dri/common/dri_util.c
>>> +++ b/src/mesa/drivers/dri/common/dri_util.c
>>
>>
>>
>>> +__DRIbuffer *
>>> +driAllocateBuffer(__DRIscreen *screen,
>>> +                  unsigned attachment, unsigned format,
>>> +                  int width, int height)
>>> +{
>>> +   struct dri_image_buffer *buffer;
>>> +   __DRIimageExtension *image = screen->image_extension;
>>> +   int dri_format, name, stride;
>>> +
>>> +   assert(attachment == __DRI_BUFFER_FRONT_LEFT ||
>>> +          attachment == __DRI_BUFFER_BACK_LEFT);
>>> +
>>> +   /* We just need a __DRI_IMAGE_FORMAT code that has a cpp that matches
>>> +    * format / 8.  The image format code is stored in the __DRIimage, but
>>> the
>>> +    * __DRIbuffer we create from the image, only stores the cpp. */
>>> +
>>> +   switch (format) {
>>> +   case 32:
>>> +      dri_format = __DRI_IMAGE_FORMAT_XRGB8888;
>>> +      break;
>>> +   case 16:
>>> +      dri_format = __DRI_IMAGE_FORMAT_RGB565;
>>> +      break;
>>> +   default:
>>> +      return NULL;
>>> +   }
>>> +
>>> +   buffer = calloc(1, sizeof *buffer);
>>> +   if (buffer == NULL)
>>> +      return NULL;
>>> +
>>> +   buffer->image = image->createImage(screen,
>>> +                                      width, height, dri_format,
>>> +                                      __DRI_IMAGE_USE_SHARE |
>>> +                                      __DRI_IMAGE_USE_SCANOUT,
>>> +                                      buffer);
>>
>>
>> It's incorrect to specify __DRI_IMAGE_USE_SCANOUT regardless of
>> attachment type. GBM, Wayland, and Android use driAllocateBuffer
>> to allocate more than just the scanout buffer. They use it to
>> allocate auxillary buffers too. If i965 were to respect the
>> caching restrictions implied by __DRI_IMAGE_USE_SCANOUT, its
>> use for aux buffers would hurt performance. (As far as I can
>> tell, though, intel_create_image() wrongly ignores __DRI_IMAGE_USE_SCANOUT).
>>
>> Instead, I think you should set the USE_SCANOUT bit if and only if
>> attachment is one of __DRI_BUFFER_(BACK|FRONT)_(LEFT|RIGHT).
>
> The commit message says:
>
> "Drivers that only call getBuffers to request color buffers can use these
> generic, __DRIimage based helpers to implement the allocBuffer and
> releaseBuffer functions of __DRIdri2Extension."
>
> which is true for the Intel DRI driver.  The DRI2 interface allows the
> driver to ask for auxillary buffers, but since we stopped supporting
> multiple processes rendering to the same X window, our driver no
> longer does that.  This is under driver control and thus if a driver
> knows it will never ask for aux buffers, it can use these helpers.
>
> Kristian

Ah, thanks for correcting me. I also solved the questions I had regarding
patch 3, so this series is
Reviewed-by: Chad Versace <chad.versace@linux.intel.com>
Kristian Høgsberg <krh@bitplanet.net> writes:

> Drivers that only call getBuffers to request color buffers can use these
> generic, __DRIimage based helpers to implement the allocBuffer and
> releaseBuffer functions of __DRIdri2Extension.
>
> For the intel dri driver, this consolidates window system color buffer
> allocation in intel_create_image().
>
> Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
> ---
>  src/mesa/drivers/dri/common/dri_util.c   | 75 ++++++++++++++++++++++++++++++++
>  src/mesa/drivers/dri/common/dri_util.h   | 10 +++++
>  src/mesa/drivers/dri/i915/intel_screen.c | 56 +-----------------------
>  src/mesa/drivers/dri/i965/intel_screen.c | 55 +----------------------
>  4 files changed, 89 insertions(+), 107 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/common/dri_util.c b/src/mesa/drivers/dri/common/dri_util.c
> index 86cf24c..a7328e4 100644
> --- a/src/mesa/drivers/dri/common/dri_util.c
> +++ b/src/mesa/drivers/dri/common/dri_util.c
> @@ -115,6 +115,7 @@ driCreateNewScreen2(int scrn, int fd,
>  {
>      static const __DRIextension *emptyExtensionList[] = { NULL };
>      __DRIscreen *psp;
> +    int i;
>  
>      psp = calloc(1, sizeof(*psp));
>      if (!psp)
> @@ -161,6 +162,11 @@ driCreateNewScreen2(int scrn, int fd,
>  	return NULL;
>      }
>  
> +    for (i = 0; psp->extensions[i]; i++) {
> +       if (strcmp(psp->extensions[i]->name, __DRI_IMAGE) == 0)
> +          psp->image_extension = (__DRIimageExtension *) psp->extensions[i];
> +    }
> +
>      int gl_version_override = _mesa_get_gl_version_override();
>      if (gl_version_override >= 31) {
>         psp->max_gl_core_version = MAX2(psp->max_gl_core_version,
> @@ -862,6 +868,75 @@ driImageFormatToGLFormat(uint32_t image_format)
>     }
>  }
>  
> +struct dri_image_buffer {
> +   __DRIbuffer base;
> +   __DRIimage *image;
> +};
> +
> +__DRIbuffer *
> +driAllocateBuffer(__DRIscreen *screen,
> +                  unsigned attachment, unsigned format,
> +                  int width, int height)
> +{
> +   struct dri_image_buffer *buffer;
> +   __DRIimageExtension *image = screen->image_extension;
> +   int dri_format, name, stride;
> +
> +   assert(attachment == __DRI_BUFFER_FRONT_LEFT ||
> +          attachment == __DRI_BUFFER_BACK_LEFT);
> +
> +   /* We just need a __DRI_IMAGE_FORMAT code that has a cpp that matches
> +    * format / 8.  The image format code is stored in the __DRIimage, but the
> +    * __DRIbuffer we create from the image, only stores the cpp. */

s/, only/ only/

> +
> +   switch (format) {
> +   case 32:
> +      dri_format = __DRI_IMAGE_FORMAT_XRGB8888;
> +      break;
> +   case 16:
> +      dri_format = __DRI_IMAGE_FORMAT_RGB565;
> +      break;
> +   default:
> +      return NULL;
> +   }
> +
> +   buffer = calloc(1, sizeof *buffer);
> +   if (buffer == NULL)
> +      return NULL;
> +
> +   buffer->image = image->createImage(screen,
> +                                      width, height, dri_format,
> +                                      __DRI_IMAGE_USE_SHARE |
> +                                      __DRI_IMAGE_USE_SCANOUT,
> +                                      buffer);

Chad's right about scanout here.

Other than that, this series is:

Reviewed-by: Eric Anholt <eric@anholt.net>

I don't have a strong opinion on the stable nomination for the later two
patches.  It feels wrong to me to cherry-pick feature-ish work after the
branchpoint, but I'm not the maintainer of the code in question so it's
not my business (and I understand how desirable not generating names for
all your buffers is, from a security perspective).
On Tue, Nov 12, 2013 at 12:15 PM, Eric Anholt <eric@anholt.net> wrote:
> Kristian Høgsberg <krh@bitplanet.net> writes:
>
>> Drivers that only call getBuffers to request color buffers can use these
>> generic, __DRIimage based helpers to implement the allocBuffer and
>> releaseBuffer functions of __DRIdri2Extension.
>>
>> For the intel dri driver, this consolidates window system color buffer
>> allocation in intel_create_image().
>>
>> Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
>> ---
>>  src/mesa/drivers/dri/common/dri_util.c   | 75 ++++++++++++++++++++++++++++++++
>>  src/mesa/drivers/dri/common/dri_util.h   | 10 +++++
>>  src/mesa/drivers/dri/i915/intel_screen.c | 56 +-----------------------
>>  src/mesa/drivers/dri/i965/intel_screen.c | 55 +----------------------
>>  4 files changed, 89 insertions(+), 107 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/common/dri_util.c b/src/mesa/drivers/dri/common/dri_util.c
>> index 86cf24c..a7328e4 100644
>> --- a/src/mesa/drivers/dri/common/dri_util.c
>> +++ b/src/mesa/drivers/dri/common/dri_util.c
>> @@ -115,6 +115,7 @@ driCreateNewScreen2(int scrn, int fd,
>>  {
>>      static const __DRIextension *emptyExtensionList[] = { NULL };
>>      __DRIscreen *psp;
>> +    int i;
>>
>>      psp = calloc(1, sizeof(*psp));
>>      if (!psp)
>> @@ -161,6 +162,11 @@ driCreateNewScreen2(int scrn, int fd,
>>       return NULL;
>>      }
>>
>> +    for (i = 0; psp->extensions[i]; i++) {
>> +       if (strcmp(psp->extensions[i]->name, __DRI_IMAGE) == 0)
>> +          psp->image_extension = (__DRIimageExtension *) psp->extensions[i];
>> +    }
>> +
>>      int gl_version_override = _mesa_get_gl_version_override();
>>      if (gl_version_override >= 31) {
>>         psp->max_gl_core_version = MAX2(psp->max_gl_core_version,
>> @@ -862,6 +868,75 @@ driImageFormatToGLFormat(uint32_t image_format)
>>     }
>>  }
>>
>> +struct dri_image_buffer {
>> +   __DRIbuffer base;
>> +   __DRIimage *image;
>> +};
>> +
>> +__DRIbuffer *
>> +driAllocateBuffer(__DRIscreen *screen,
>> +                  unsigned attachment, unsigned format,
>> +                  int width, int height)
>> +{
>> +   struct dri_image_buffer *buffer;
>> +   __DRIimageExtension *image = screen->image_extension;
>> +   int dri_format, name, stride;
>> +
>> +   assert(attachment == __DRI_BUFFER_FRONT_LEFT ||
>> +          attachment == __DRI_BUFFER_BACK_LEFT);
>> +
>> +   /* We just need a __DRI_IMAGE_FORMAT code that has a cpp that matches
>> +    * format / 8.  The image format code is stored in the __DRIimage, but the
>> +    * __DRIbuffer we create from the image, only stores the cpp. */
>
> s/, only/ only/
>
>> +
>> +   switch (format) {
>> +   case 32:
>> +      dri_format = __DRI_IMAGE_FORMAT_XRGB8888;
>> +      break;
>> +   case 16:
>> +      dri_format = __DRI_IMAGE_FORMAT_RGB565;
>> +      break;
>> +   default:
>> +      return NULL;
>> +   }
>> +
>> +   buffer = calloc(1, sizeof *buffer);
>> +   if (buffer == NULL)
>> +      return NULL;
>> +
>> +   buffer->image = image->createImage(screen,
>> +                                      width, height, dri_format,
>> +                                      __DRI_IMAGE_USE_SHARE |
>> +                                      __DRI_IMAGE_USE_SCANOUT,
>> +                                      buffer);
>
> Chad's right about scanout here.

These helpers are only for drivers that no longer use DRI2 back
buffers.  This __DRIimage helper can only allocate color buffers and
they have to be usable for scanout.

There is no impact on cacheability or tiling of aux buffers, since
this function will never allocate those.  A given driver knows whether
or not it will call out to DRI2 getBuffersWithFormat to ask for aux
buffers or not.  If it allocates its own aux buffers and supports the
__DRIimage extension, it can plug in these helpers and avoid
implementing allocateBuffer/releaseBuffer.  If the driver still relies
on DRI2 getBufferWithFormat for aux buffers, it must implement its own
allocBuffer/releaseBuffer pair.

> Other than that, this series is:
>
> Reviewed-by: Eric Anholt <eric@anholt.net>
>
> I don't have a strong opinion on the stable nomination for the later two
> patches.  It feels wrong to me to cherry-pick feature-ish work after the
> branchpoint, but I'm not the maintainer of the code in question so it's
> not my business (and I understand how desirable not generating names for
> all your buffers is, from a security perspective).

I would also prefer not to pick these feature-ish patches back to the
10 branch, but there are a few caveats: the patches depend on the
__DRIimageLoaderExtension, but as we branched for 10 just as those
patches landed there wasn't any time to get these in before branching.
 We're also still very close the the branch point, so we'll still have
some time between pushing the patches and the release. Finally, with
DRI3 seemingly impossible to build, the GBM and Wayland EGL platforms
are the best ways to test the new DRI driver changes we're shipping in
10.

Kristian
Kristian Høgsberg <krh@bitplanet.net> writes:

> On Tue, Nov 12, 2013 at 12:15 PM, Eric Anholt <eric@anholt.net> wrote:
>> Kristian Høgsberg <krh@bitplanet.net> writes:
>>
>>> Drivers that only call getBuffers to request color buffers can use these
>>> generic, __DRIimage based helpers to implement the allocBuffer and
>>> releaseBuffer functions of __DRIdri2Extension.
>>>
>>> For the intel dri driver, this consolidates window system color buffer
>>> allocation in intel_create_image().
>>>
>>> Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
>>> ---
>>>  src/mesa/drivers/dri/common/dri_util.c   | 75 ++++++++++++++++++++++++++++++++
>>>  src/mesa/drivers/dri/common/dri_util.h   | 10 +++++
>>>  src/mesa/drivers/dri/i915/intel_screen.c | 56 +-----------------------
>>>  src/mesa/drivers/dri/i965/intel_screen.c | 55 +----------------------
>>>  4 files changed, 89 insertions(+), 107 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/common/dri_util.c b/src/mesa/drivers/dri/common/dri_util.c
>>> index 86cf24c..a7328e4 100644
>>> --- a/src/mesa/drivers/dri/common/dri_util.c
>>> +++ b/src/mesa/drivers/dri/common/dri_util.c
>>> @@ -115,6 +115,7 @@ driCreateNewScreen2(int scrn, int fd,
>>>  {
>>>      static const __DRIextension *emptyExtensionList[] = { NULL };
>>>      __DRIscreen *psp;
>>> +    int i;
>>>
>>>      psp = calloc(1, sizeof(*psp));
>>>      if (!psp)
>>> @@ -161,6 +162,11 @@ driCreateNewScreen2(int scrn, int fd,
>>>       return NULL;
>>>      }
>>>
>>> +    for (i = 0; psp->extensions[i]; i++) {
>>> +       if (strcmp(psp->extensions[i]->name, __DRI_IMAGE) == 0)
>>> +          psp->image_extension = (__DRIimageExtension *) psp->extensions[i];
>>> +    }
>>> +
>>>      int gl_version_override = _mesa_get_gl_version_override();
>>>      if (gl_version_override >= 31) {
>>>         psp->max_gl_core_version = MAX2(psp->max_gl_core_version,
>>> @@ -862,6 +868,75 @@ driImageFormatToGLFormat(uint32_t image_format)
>>>     }
>>>  }
>>>
>>> +struct dri_image_buffer {
>>> +   __DRIbuffer base;
>>> +   __DRIimage *image;
>>> +};
>>> +
>>> +__DRIbuffer *
>>> +driAllocateBuffer(__DRIscreen *screen,
>>> +                  unsigned attachment, unsigned format,
>>> +                  int width, int height)
>>> +{
>>> +   struct dri_image_buffer *buffer;
>>> +   __DRIimageExtension *image = screen->image_extension;
>>> +   int dri_format, name, stride;
>>> +
>>> +   assert(attachment == __DRI_BUFFER_FRONT_LEFT ||
>>> +          attachment == __DRI_BUFFER_BACK_LEFT);
>>> +
>>> +   /* We just need a __DRI_IMAGE_FORMAT code that has a cpp that matches
>>> +    * format / 8.  The image format code is stored in the __DRIimage, but the
>>> +    * __DRIbuffer we create from the image, only stores the cpp. */
>>
>> s/, only/ only/
>>
>>> +
>>> +   switch (format) {
>>> +   case 32:
>>> +      dri_format = __DRI_IMAGE_FORMAT_XRGB8888;
>>> +      break;
>>> +   case 16:
>>> +      dri_format = __DRI_IMAGE_FORMAT_RGB565;
>>> +      break;
>>> +   default:
>>> +      return NULL;
>>> +   }
>>> +
>>> +   buffer = calloc(1, sizeof *buffer);
>>> +   if (buffer == NULL)
>>> +      return NULL;
>>> +
>>> +   buffer->image = image->createImage(screen,
>>> +                                      width, height, dri_format,
>>> +                                      __DRI_IMAGE_USE_SHARE |
>>> +                                      __DRI_IMAGE_USE_SCANOUT,
>>> +                                      buffer);
>>
>> Chad's right about scanout here.
>
> These helpers are only for drivers that no longer use DRI2 back
> buffers.  This __DRIimage helper can only allocate color buffers and
> they have to be usable for scanout.
>
> There is no impact on cacheability or tiling of aux buffers, since
> this function will never allocate those.  A given driver knows whether
> or not it will call out to DRI2 getBuffersWithFormat to ask for aux
> buffers or not.  If it allocates its own aux buffers and supports the
> __DRIimage extension, it can plug in these helpers and avoid
> implementing allocateBuffer/releaseBuffer.  If the driver still relies
> on DRI2 getBufferWithFormat for aux buffers, it must implement its own
> allocBuffer/releaseBuffer pair.

You've convinced me.