From: Eric Anholt <eric@anholt.net>
To: Keith Packard <keithp@keithp.com>, mesa3d-dev@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 7/8] dri: add __DRIimageLoaderExtension and __DRIimageDriverExtension
Date: Tue, 05 Nov 2013 12:05:32 -0800 [thread overview]
Message-ID: <87zjpifwc3.fsf@eliezer.anholt.net> (raw)
In-Reply-To: <1383618208-21310-8-git-send-email-keithp@keithp.com>
[-- 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
next prev parent reply other threads:[~2013-11-05 20:05 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-05 2:23 [PATCH 0/8] Add DRIimage-based DRI3/Present loader Keith Packard
2013-11-05 2:23 ` [PATCH 1/8] drivers/dri/common: A few dri2 functions are not actually DRI2 specific Keith Packard
2013-11-05 2:23 ` [PATCH 2/8] dri/intel: Split out DRI2 buffer update code to separate function Keith Packard
2013-11-05 2:23 ` [PATCH 3/8] dri/intel: Add explicit size parameter to intel_region_alloc_for_fd Keith Packard
2013-11-05 22:23 ` Kristian Høgsberg
2013-11-06 0:52 ` Keith Packard
2013-11-07 5:17 ` Christopher James Halse Rogers
2013-11-07 5:42 ` Keith Packard
2013-11-05 2:23 ` [PATCH 4/8] Define __DRI_IMAGE_FORMAT_SARGB8 Keith Packard
2013-11-05 2:23 ` [PATCH 5/8] dri/common: Add functions mapping MESA_FORMAT_* <-> __DRI_IMAGE_FORMAT_* Keith Packard
2013-11-05 3:01 ` Jordan Justen
2013-11-05 4:11 ` Keith Packard
2013-11-05 22:53 ` Jordan Justen
2013-11-05 22:35 ` Kristian Høgsberg
2013-11-06 0:54 ` Keith Packard
2013-11-05 2:23 ` [PATCH 6/8] dri/i915, dri/i965: Use driGLFormatToImageFormat and driImageFormatToGLFormat Keith Packard
2013-11-05 22:37 ` [PATCH 6/8] dri/i915,dri/i965: " Kristian Høgsberg
2013-11-05 2:23 ` [PATCH 7/8] dri: add __DRIimageLoaderExtension and __DRIimageDriverExtension Keith Packard
2013-11-05 20:05 ` Eric Anholt [this message]
2013-11-05 23:47 ` Keith Packard
2013-11-05 22:59 ` Kristian Høgsberg
2013-11-06 0:59 ` Keith Packard
2013-11-06 2:48 ` Kristian Høgsberg
2013-11-06 6:25 ` Kristian Høgsberg
2013-11-06 14:55 ` Keith Packard
2013-11-06 16:17 ` Kristian Høgsberg
2013-11-06 18:09 ` Keith Packard
2013-11-06 19:06 ` Kristian Høgsberg
2013-11-06 19:29 ` Keith Packard
2013-11-05 2:23 ` [PATCH 8/8] Add DRI3+Present loader Keith Packard
2013-11-05 23:10 ` Eric Anholt
2013-11-06 2:32 ` Keith Packard
2013-11-05 16:40 ` [PATCH 0/8] Add DRIimage-based DRI3/Present loader Keith Packard
2013-11-05 20:04 ` Eric Anholt
2013-11-05 22:09 ` Kristian Høgsberg
2013-11-05 23:54 ` Keith Packard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zjpifwc3.fsf@eliezer.anholt.net \
--to=eric@anholt.net \
--cc=dri-devel@lists.freedesktop.org \
--cc=keithp@keithp.com \
--cc=mesa3d-dev@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.