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

[Chad Versace]

On 11/11/2013 02:20 PM, Kristian Høgsberg wrote:
> 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

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 at linux.intel.com>