Re: [PATCH 7/8] dri: add __DRIimageLoaderExtension and __DRIimageDriverExtension

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1.1: Type: text/plain, Size: 12074 bytes --]

Keith Packard <keithp@keithp.com> writes:

> These provide an interface between the driver and the loader to allocate
> color buffers through the DRIimage extension interface rather than through a
> loader-specific extension (as is used by DRI2, for instance).
>
> The driver uses the loader 'getBuffers' interface to allocate color buffers.
>
> The loader uses the createNewScreen2, createNewDrawable, createNewContext,
> getAPIMask and createContextAttribs APIS (mostly shared with DRI2).
>
> This interface will work with the DRI3 loader, and should also work with GBM
> and other loaders so that drivers need not be customized for each new loader
> interface, as long as they provide this image interface.

Most of my review was going to be whining about yet another (broken)
copy of dri2CreateNewScreen2.  Sounds like you've fixed that.

> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  include/GL/internal/dri_interface.h           | 112 +++++++++++++++++++++++++
>  src/mesa/drivers/dri/common/dri_util.c        | 113 +++++++++++++++++++++++++
>  src/mesa/drivers/dri/common/dri_util.h        |   6 ++
>  src/mesa/drivers/dri/i915/intel_context.c     | 111 ++++++++++++++++++++++++-
>  src/mesa/drivers/dri/i915/intel_mipmap_tree.c |  33 ++++++++
>  src/mesa/drivers/dri/i915/intel_mipmap_tree.h |   8 ++
>  src/mesa/drivers/dri/i915/intel_screen.c      |   1 +
>  src/mesa/drivers/dri/i965/brw_context.c       | 114 ++++++++++++++++++++++++--
>  src/mesa/drivers/dri/i965/brw_context.h       |  16 ++--
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c |  61 ++++++++++++++
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |   8 ++
>  src/mesa/drivers/dri/i965/intel_screen.c      |   5 +-
>  12 files changed, 568 insertions(+), 20 deletions(-)
>
> diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
> index 907aeca..8fc1fa6 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -86,6 +86,10 @@ typedef struct __DRIdri2LoaderExtensionRec	__DRIdri2LoaderExtension;
>  typedef struct __DRI2flushExtensionRec	__DRI2flushExtension;
>  typedef struct __DRI2throttleExtensionRec	__DRI2throttleExtension;
>  
> +
> +typedef struct __DRIimageLoaderExtensionRec     __DRIimageLoaderExtension;
> +typedef struct __DRIimageDriverExtensionRec     __DRIimageDriverExtension;
> +
>  /*@}*/
>  
>  
> @@ -1288,4 +1292,112 @@ typedef struct __DRIDriverVtableExtensionRec {
>      const struct __DriverAPIRec *vtable;
>  } __DRIDriverVtableExtension;
>  
> +/**
> + * Image Loader extension. Drivers use this to allocate color buffers
> + */


> +
> +#define __DRI_DRIVER_EXTENSIONS "__driDriverExtensions"

This looks like rebase fail

> +#define __DRI_IMAGE_LOADER "DRI_IMAGE_LOADER"
> +#define __DRI_IMAGE_LOADER_VERSION 1
> +
> +struct __DRIimageLoaderExtensionRec {
> +    __DRIextension base;
> +
> +   /**
> +    * Allocate color buffers.
> +    *
> +    * \param driDrawable
> +    * \param width              Width of allocated buffers
> +    * \param height             Height of allocated buffers
> +    * \param format             one of __DRI_IMAGE_FORMAT_*
> +    * \param stamp              Address of variable to be updated when
> +    *                           getBuffers must be called again
> +    * \param loaderPrivate      The loaderPrivate for driDrawable
> +    * \param buffer_mask        Set of buffers to allocate
> +    * \param buffers            Returned buffers
> +    */
> +   int (*getBuffers)(__DRIdrawable *driDrawable,
> +                     int *width, int *height,
> +                     unsigned int format,
> +                     uint32_t *stamp,
> +                     void *loaderPrivate,
> +                     uint32_t buffer_mask,
> +                     struct __DRIimageList *buffers);
> +
> +    /**
> +     * Flush pending front-buffer rendering
> +     *
> +     * Any rendering that has been performed to the
> +     * fake front will be flushed to the front
> +     *
> +     * \param driDrawable    Drawable whose front-buffer is to be flushed
> +     * \param loaderPrivate  Loader's private data that was previously passed
> +     *                       into __DRIdri2ExtensionRec::createNewDrawable
> +     */
> +    void (*flushFrontBuffer)(__DRIdrawable *driDrawable, void *loaderPrivate);
> +};
> +
> +/**
> + * DRI extension.
> + */
> +
> +//struct gl_context;
> +//struct dd_function_table;

Looks like development leftovers.

> +typedef __DRIscreen *
> +(*__DRIcreateNewScreen2)(int screen, int fd,
> +                         const __DRIextension **extensions,
> +                         const __DRIextension **driver_extensions,
> +                         const __DRIconfig ***driver_configs,
> +                         void *loaderPrivate);
> +
> +typedef __DRIdrawable *
> +(*__DRIcreateNewDrawable)(__DRIscreen *screen,
> +                          const __DRIconfig *config,
> +                          void *loaderPrivate);
> +
> +typedef __DRIcontext *
> +(*__DRIcreateNewContext)(__DRIscreen *screen,
> +                         const __DRIconfig *config,
> +                         __DRIcontext *shared,
> +                         void *loaderPrivate);
> +
> +typedef __DRIcontext *
> +(*__DRIcreateContextAttribs)(__DRIscreen *screen,
> +                             int api,
> +                             const __DRIconfig *config,
> +                             __DRIcontext *shared,
> +                             unsigned num_attribs,
> +                             const uint32_t *attribs,
> +                             unsigned *error,
> +                             void *loaderPrivate);
> +typedef unsigned int
> +(*__DRIgetAPIMask)(__DRIscreen *screen);

Maybe append "Func" to the typedefs so they don't look like just another
struct in the declarations?  And since they're supposed to be the same
function pointers as in the __DRIswrastExtensionRec and
__DRIdri2ExtensionRec, change them to this typedef, too?


> +static void
> +intel_update_image_buffers(struct intel_context *intel, __DRIdrawable *drawable)
> +{
> +   struct gl_framebuffer *fb = drawable->driverPrivate;
> +   __DRIscreen *screen = intel->intelScreen->driScrnPriv;
> +   struct intel_renderbuffer *front_rb;
> +   struct intel_renderbuffer *back_rb;
> +   struct __DRIimageList images;
> +   unsigned int format;
> +   uint32_t buffer_mask = 0;
> +
> +   front_rb = intel_get_renderbuffer(fb, BUFFER_FRONT_LEFT);
> +   back_rb = intel_get_renderbuffer(fb, BUFFER_BACK_LEFT);
> +
> +   if (back_rb)
> +      format = intel_rb_format(back_rb);
> +   else if (front_rb)
> +      format = intel_rb_format(front_rb);
> +   else
> +      return;
> +
> +   if ((intel->is_front_buffer_rendering || intel->is_front_buffer_reading || !back_rb) && front_rb)
> +      buffer_mask |= __DRI_IMAGE_BUFFER_FRONT;
> +
> +   if (back_rb)
> +      buffer_mask |= __DRI_IMAGE_BUFFER_BACK;
> +
> +   (*screen->image.loader->getBuffers) (drawable,
> +                                        &drawable->w,
> +                                        &drawable->h,
> +                                        driGLFormatToImageFormat(format),
> +                                        &drawable->dri2.stamp,
> +                                        drawable->loaderPrivate,
> +                                        buffer_mask,
> +                                        &images);
> +
> +   if (images.front) {
> +      assert(front_rb);
> +      intel_update_image_buffer(intel,
> +                                drawable,
> +                                front_rb,
> +                                images.front,
> +                                __DRI_IMAGE_BUFFER_FRONT);
> +   }
> +   if (images.back)
> +      intel_update_image_buffer(intel,
> +                                drawable,
> +                                back_rb,
> +                                images.back,
> +                                __DRI_IMAGE_BUFFER_BACK);
> +}

It looks like getBuffers could just be two getBuffer calls, except for
the updating of width and height.  Have you looked into doing things
that way at all?

> @@ -549,7 +549,7 @@ brw_process_driconf_options(struct brw_context *brw)
>        driQueryOptionb(options, "disable_glsl_line_continuations");
>  }
>  
> -bool
> +GLboolean
>  brwCreateContext(gl_api api,
>  	         const struct gl_config *mesaVis,
>  		 __DRIcontext *driContextPriv,

Unrelated change?

> +static void
> +intel_update_image_buffers(struct brw_context *brw, __DRIdrawable *drawable)
> +{
> +   struct gl_framebuffer *fb = drawable->driverPrivate;
> +   __DRIscreen *screen = brw->intelScreen->driScrnPriv;
> +   struct intel_renderbuffer *front_rb;
> +   struct intel_renderbuffer *back_rb;
> +   struct __DRIimageList images;
> +   unsigned int format;
> +   uint32_t buffer_mask = 0;
> +
> +   front_rb = intel_get_renderbuffer(fb, BUFFER_FRONT_LEFT);
> +   back_rb = intel_get_renderbuffer(fb, BUFFER_BACK_LEFT);
> +
> +   if (back_rb)
> +      format = intel_rb_format(back_rb);
> +   else if (front_rb)
> +      format = intel_rb_format(front_rb);
> +   else
> +      return;
> +
> +   if ((brw->is_front_buffer_rendering || brw->is_front_buffer_reading || !back_rb) && front_rb)
> +      buffer_mask |= __DRI_IMAGE_BUFFER_FRONT;
> +
> +   if (back_rb)
> +      buffer_mask |= __DRI_IMAGE_BUFFER_BACK;
> +
> +   (*screen->image.loader->getBuffers) (drawable,
> +                                        &drawable->w,
> +                                        &drawable->h,
> +                                        driGLFormatToImageFormat(format),
> +                                        &drawable->dri2.stamp,
> +                                        drawable->loaderPrivate,
> +                                        buffer_mask,
> +                                        &images);
> +
> +   if (images.front) {
> +      assert(front_rb);
> +      intel_update_image_buffer(brw,
> +                                drawable,
> +                                front_rb,
> +                                images.front,
> +                                __DRI_IMAGE_BUFFER_FRONT);
> +   }
> +   if (images.back)
> +      intel_update_image_buffer(brw,
> +                                drawable,
> +                                back_rb,
> +                                images.back,
> +                                __DRI_IMAGE_BUFFER_BACK);
> +}

Style nit: we try and put braces around multi-line things like this,
even if they are a single statement.

> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index bec4d6b..1ecbfb7 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -1477,14 +1477,14 @@ void intel_prepare_render(struct brw_context *brw);
>  void intel_resolve_for_dri2_flush(struct brw_context *brw,
>                                    __DRIdrawable *drawable);
>  
> -bool brwCreateContext(gl_api api,
> -		      const struct gl_config *mesaVis,
> -		      __DRIcontext *driContextPriv,
> -                      unsigned major_version,
> -                      unsigned minor_version,
> -                      uint32_t flags,
> -                      unsigned *error,
> -		      void *sharedContextPrivate);
> +GLboolean brwCreateContext(gl_api api,
> +                           const struct gl_config *mesaVis,
> +                           __DRIcontext *driContextPriv,
> +                           unsigned major_version,
> +                           unsigned minor_version,
> +                           uint32_t flags,
> +                           unsigned *error,
> +                           void *sharedContextPrivate);

Unrelated change.


[-- Attachment #1.2: Type: application/pgp-signature, Size: 835 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel