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

[Kristian Høgsberg]

On Mon, Nov 11, 2013 at 1:57 PM, Chad Versace
<chad.versace at 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 at 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;
>> +}
>
>