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

[Kristian Høgsberg]

On Tue, Nov 12, 2013 at 12:15 PM, Eric Anholt <eric at anholt.net> wrote:
> Kristian Høgsberg <krh at 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 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
>> @@ -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 at 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