Re: [Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers
Hi Ben, On 13 March 2017 at 21:47, Ben Widawsky wrote: > On 17-03-10 10:28:42, Emil Velikov wrote: >> >> Hi Ben, >> >> Mostly pointing out a few things that look strange, pardon if some >> seem too pedantic. >> >> On 10 March 2017 at 01:48, Ben Widawsky wrote: >> >>> --- >>> include/GL/internal/dri_interface.h | 27 >>> ++- >> >> Split the Infra from the i965 implementation ? >> > > Sure. Doesn't matter to me. > Just a big thank you for the respin on the series - it looks great ! Small note in case people poke you that this "broke" their builds - if make is fine while make install fails they're hitting a libtool bug^Wfeature. Workarounds include - moving the system libgbm, building in clean chroot or using slibtool. >>> @@ -840,6 +869,7 @@ static const __DRIimageExtension intelImageExtension >>> = { >> >> Afaict, nothing in this series bumps the version to 14. >> So you either missed a patch or we have a bug somewhere ? >> > > Well, it bumps the version in dri_interface.h. Formerly, before splitting > things > up, this patch had the i965 version bumped too, but that's gone now. I'm > happy > to reword this however you'd like. > Since i965 does not fully implement the v14 you're correct in not bumping the version in the i965 driver. Note: the version in dri_interface.h is the one that drivers/loaders can implement, not the one that they do. Or in other words - no driver/loader should use the __DRI..._VERSION defines. Thanks Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers
On 17-03-10 10:28:42, Emil Velikov wrote: Hi Ben, Mostly pointing out a few things that look strange, pardon if some seem too pedantic. On 10 March 2017 at 01:48, Ben Widawsky wrote: --- include/GL/internal/dri_interface.h | 27 ++- Split the Infra from the i965 implementation ? Sure. Doesn't matter to me. src/gallium/state_trackers/dri/dri2.c| 1 + Not needed. Yeah, whoops. src/mesa/drivers/dri/i965/intel_screen.c | 32 +++- Patch should come as/after __DRI_IMAGE_ATTRIB_MODIFIER_* are honoured. Or modifiers and modifiers count in general. Assuming this also means split the infra from implementation, sure. --- a/src/gallium/state_trackers/dri/dri2.c +++ b/src/gallium/state_trackers/dri/dri2.c @@ -1413,6 +1413,7 @@ static __DRIimageExtension dri2ImageExtension = { .getCapabilities = dri2_get_capabilities, .mapImage = dri2_map_image, .unmapImage = dri2_unmap_image, +.createImageWithModifiers = NULL, }; Not needed - drop ? Gone. diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c index 21786eb54a..3452572874 100644 --- a/src/mesa/drivers/dri/i965/intel_screen.c +++ b/src/mesa/drivers/dri/i965/intel_screen.c @@ -510,9 +510,11 @@ intel_destroy_image(__DRIimage *image) } static __DRIimage * -intel_create_image(__DRIscreen *dri_screen, +__intel_create_image(__DRIscreen *dri_screen, Don't think there's (m)any functions that start with __ in 965. Perhaps append "_common" ? Sure. int cpp; unsigned long pitch; + /* Callers of this may specify a modifier, or a dri usage, but not both. The +* newer modifier interface deprecates the older usage flags newer modifier +* interface deprecates the older usage flags. +*/ + assert(!(use && count)); + What would happen if we don't have either old (use) or new (modifiers) ? Shouldn't this be XOR ? Not having either is fine. It's like the previous case of supporting use flags but passing in none, which is allowed. @@ -840,6 +869,7 @@ static const __DRIimageExtension intelImageExtension = { Afaict, nothing in this series bumps the version to 14. So you either missed a patch or we have a bug somewhere ? Well, it bumps the version in dri_interface.h. Formerly, before splitting things up, this patch had the i965 version bumped too, but that's gone now. I'm happy to reword this however you'd like. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers
On 17-03-09 18:38:15, Jason Ekstrand wrote: On Thu, Mar 9, 2017 at 5:48 PM, Ben Widawsky wrote: Modifiers will be obtains or guessed by the client and passed in during "obtained" Got it. image creation/import. This requires bumping the DRIimage version. As of this patch, the modifiers aren't plumbed all the way down, this patch simply makes sure the interface level stuff is correct. v2: Don't allow usage + modifiers v3: Make NAND actually NAND. Bug introduced in v2. (Jason) Cc: Kristian Høgsberg Cc: Jason Ekstrand Signed-off-by: Ben Widawsky Reviewed-by: Eric Engestrom (v1) Acked-by: Daniel Stone --- include/GL/internal/dri_interface.h | 27 ++- src/gallium/state_trackers/dri/dri2.c| 1 + src/mesa/drivers/dri/i965/intel_screen.c | 32 +++- 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h index 598d111f33..53fac6fc3c 100644 --- a/include/GL/internal/dri_interface.h +++ b/include/GL/internal/dri_interface.h @@ -1136,7 +1136,7 @@ struct __DRIdri2ExtensionRec { * extensions. */ #define __DRI_IMAGE "DRI_IMAGE" -#define __DRI_IMAGE_VERSION 13 +#define __DRI_IMAGE_VERSION 14 /** * These formats correspond to the similarly named MESA_FORMAT_* @@ -1257,6 +1257,8 @@ struct __DRIdri2ExtensionRec { #define __DRI_IMAGE_ATTRIB_NUM_PLANES 0x2009 /* available in versions 11 */ #define __DRI_IMAGE_ATTRIB_OFFSET 0x200A /* available in versions 13 */ +#define __DRI_IMAGE_ATTRIB_MODIFIER_LOWER 0x200B /* available in versions 14 */ +#define __DRI_IMAGE_ATTRIB_MODIFIER_UPPER 0x200C /* available in versions 14 */ enum __DRIYUVColorSpace { __DRI_YUV_COLOR_SPACE_UNDEFINED = 0, @@ -1468,6 +1470,29 @@ struct __DRIimageExtensionRec { */ void (*unmapImage)(__DRIcontext *context, __DRIimage *image, void *data); + + /** +* Creates an image with implementation's favorite modifiers. +* +* This acts like createImage except there is a list of modifiers passed in +* which the implementation may selectively use to create the DRIimage. The +* result should be the implementation selects one modifier (perhaps it would +* hold on to a few and later pick). +* +* The created image should be destroyed with destroyImage(). +* +* Returns the new DRIimage. The chosen modifier can be obtained later on +* and passed back to things like the kernel's AddFB2 interface. +* +* \sa __DRIimageRec::createImage +* +* \since 14 +*/ + __DRIimage *(*createImageWithModifiers)(__DRIscreen *screen, + int width, int height, int format, + const uint64_t *modifiers, + const unsigned int modifier_count, + void *loaderPrivate); }; diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c index b50e096443..12e466c6f1 100644 --- a/src/gallium/state_trackers/dri/dri2.c +++ b/src/gallium/state_trackers/dri/dri2.c @@ -1413,6 +1413,7 @@ static __DRIimageExtension dri2ImageExtension = { .getCapabilities = dri2_get_capabilities, .mapImage = dri2_map_image, .unmapImage = dri2_unmap_image, +.createImageWithModifiers = NULL, }; diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c index 21786eb54a..3452572874 100644 --- a/src/mesa/drivers/dri/i965/intel_screen.c +++ b/src/mesa/drivers/dri/i965/intel_screen.c @@ -510,9 +510,11 @@ intel_destroy_image(__DRIimage *image) } static __DRIimage * -intel_create_image(__DRIscreen *dri_screen, +__intel_create_image(__DRIscreen *dri_screen, int width, int height, int format, unsigned int use, + const uint64_t *modifiers, + unsigned count, void *loaderPrivate) { __DRIimage *image; @@ -521,6 +523,12 @@ intel_create_image(__DRIscreen *dri_screen, int cpp; unsigned long pitch; + /* Callers of this may specify a modifier, or a dri usage, but not both. The +* newer modifier interface deprecates the older usage flags newer modifier +* interface deprecates the older usage flags. +*/ + assert(!(use && count)); + tiling = I915_TILING_X; if (use & __DRI_IMAGE_USE_CURSOR) { if (width != 64 || height != 64) @@ -550,6 +558,27 @@ intel_create_image(__DRIscreen *dri_screen, return image; } +static __DRIimage * +intel_create_image(__DRIscreen *dri_screen, + int width, int height, int format, + unsigned int use, + void *loaderPrivate) +{ + return __intel_create_image(dri_screen, width, height, format, use, NULL, 0, + loaderPrivate); +} + +static __DRIi
Re: [Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers
Hi Ben, Mostly pointing out a few things that look strange, pardon if some seem too pedantic. On 10 March 2017 at 01:48, Ben Widawsky wrote: > --- > include/GL/internal/dri_interface.h | 27 ++- Split the Infra from the i965 implementation ? > src/gallium/state_trackers/dri/dri2.c| 1 + Not needed. > src/mesa/drivers/dri/i965/intel_screen.c | 32 > +++- Patch should come as/after __DRI_IMAGE_ATTRIB_MODIFIER_* are honoured. Or modifiers and modifiers count in general. > --- a/src/gallium/state_trackers/dri/dri2.c > +++ b/src/gallium/state_trackers/dri/dri2.c > @@ -1413,6 +1413,7 @@ static __DRIimageExtension dri2ImageExtension = { > .getCapabilities = dri2_get_capabilities, > .mapImage = dri2_map_image, > .unmapImage = dri2_unmap_image, > +.createImageWithModifiers = NULL, > }; > Not needed - drop ? > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c > b/src/mesa/drivers/dri/i965/intel_screen.c > index 21786eb54a..3452572874 100644 > --- a/src/mesa/drivers/dri/i965/intel_screen.c > +++ b/src/mesa/drivers/dri/i965/intel_screen.c > @@ -510,9 +510,11 @@ intel_destroy_image(__DRIimage *image) > } > > static __DRIimage * > -intel_create_image(__DRIscreen *dri_screen, > +__intel_create_image(__DRIscreen *dri_screen, Don't think there's (m)any functions that start with __ in 965. Perhaps append "_common" ? > int cpp; > unsigned long pitch; > > + /* Callers of this may specify a modifier, or a dri usage, but not both. > The > +* newer modifier interface deprecates the older usage flags newer > modifier > +* interface deprecates the older usage flags. > +*/ > + assert(!(use && count)); > + What would happen if we don't have either old (use) or new (modifiers) ? Shouldn't this be XOR ? > @@ -840,6 +869,7 @@ static const __DRIimageExtension intelImageExtension = { Afaict, nothing in this series bumps the version to 14. So you either missed a patch or we have a bug somewhere ? -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers
On Thu, Mar 9, 2017 at 5:48 PM, Ben Widawsky wrote: > Modifiers will be obtains or guessed by the client and passed in during > "obtained" > image creation/import. > > This requires bumping the DRIimage version. > > As of this patch, the modifiers aren't plumbed all the way down, this > patch simply makes sure the interface level stuff is correct. > > v2: Don't allow usage + modifiers > > v3: Make NAND actually NAND. Bug introduced in v2. (Jason) > > Cc: Kristian Høgsberg > Cc: Jason Ekstrand > Signed-off-by: Ben Widawsky > Reviewed-by: Eric Engestrom (v1) > Acked-by: Daniel Stone > --- > include/GL/internal/dri_interface.h | 27 ++- > src/gallium/state_trackers/dri/dri2.c| 1 + > src/mesa/drivers/dri/i965/intel_screen.c | 32 > +++- > 3 files changed, 58 insertions(+), 2 deletions(-) > > diff --git a/include/GL/internal/dri_interface.h > b/include/GL/internal/dri_interface.h > index 598d111f33..53fac6fc3c 100644 > --- a/include/GL/internal/dri_interface.h > +++ b/include/GL/internal/dri_interface.h > @@ -1136,7 +1136,7 @@ struct __DRIdri2ExtensionRec { > * extensions. > */ > #define __DRI_IMAGE "DRI_IMAGE" > -#define __DRI_IMAGE_VERSION 13 > +#define __DRI_IMAGE_VERSION 14 > > /** > * These formats correspond to the similarly named MESA_FORMAT_* > @@ -1257,6 +1257,8 @@ struct __DRIdri2ExtensionRec { > #define __DRI_IMAGE_ATTRIB_NUM_PLANES 0x2009 /* available in versions > 11 */ > > #define __DRI_IMAGE_ATTRIB_OFFSET 0x200A /* available in versions 13 */ > +#define __DRI_IMAGE_ATTRIB_MODIFIER_LOWER 0x200B /* available in > versions 14 */ > +#define __DRI_IMAGE_ATTRIB_MODIFIER_UPPER 0x200C /* available in > versions 14 */ > > enum __DRIYUVColorSpace { > __DRI_YUV_COLOR_SPACE_UNDEFINED = 0, > @@ -1468,6 +1470,29 @@ struct __DRIimageExtensionRec { > */ > void (*unmapImage)(__DRIcontext *context, __DRIimage *image, void > *data); > > + > + /** > +* Creates an image with implementation's favorite modifiers. > +* > +* This acts like createImage except there is a list of modifiers > passed in > +* which the implementation may selectively use to create the > DRIimage. The > +* result should be the implementation selects one modifier (perhaps > it would > +* hold on to a few and later pick). > +* > +* The created image should be destroyed with destroyImage(). > +* > +* Returns the new DRIimage. The chosen modifier can be obtained later > on > +* and passed back to things like the kernel's AddFB2 interface. > +* > +* \sa __DRIimageRec::createImage > +* > +* \since 14 > +*/ > + __DRIimage *(*createImageWithModifiers)(__DRIscreen *screen, > + int width, int height, int > format, > + const uint64_t *modifiers, > + const unsigned int > modifier_count, > + void *loaderPrivate); > }; > > > diff --git a/src/gallium/state_trackers/dri/dri2.c > b/src/gallium/state_trackers/dri/dri2.c > index b50e096443..12e466c6f1 100644 > --- a/src/gallium/state_trackers/dri/dri2.c > +++ b/src/gallium/state_trackers/dri/dri2.c > @@ -1413,6 +1413,7 @@ static __DRIimageExtension dri2ImageExtension = { > .getCapabilities = dri2_get_capabilities, > .mapImage = dri2_map_image, > .unmapImage = dri2_unmap_image, > +.createImageWithModifiers = NULL, > }; > > > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c > b/src/mesa/drivers/dri/i965/intel_screen.c > index 21786eb54a..3452572874 100644 > --- a/src/mesa/drivers/dri/i965/intel_screen.c > +++ b/src/mesa/drivers/dri/i965/intel_screen.c > @@ -510,9 +510,11 @@ intel_destroy_image(__DRIimage *image) > } > > static __DRIimage * > -intel_create_image(__DRIscreen *dri_screen, > +__intel_create_image(__DRIscreen *dri_screen, >int width, int height, int format, >unsigned int use, > + const uint64_t *modifiers, > + unsigned count, >void *loaderPrivate) > { > __DRIimage *image; > @@ -521,6 +523,12 @@ intel_create_image(__DRIscreen *dri_screen, > int cpp; > unsigned long pitch; > > + /* Callers of this may specify a modifier, or a dri usage, but not > both. The > +* newer modifier interface deprecates the older usage flags newer > modifier > +* interface deprecates the older usage flags. > +*/ > + assert(!(use && count)); > + > tiling = I915_TILING_X; > if (use & __DRI_IMAGE_USE_CURSOR) { >if (width != 64 || height != 64) > @@ -550,6 +558,27 @@ intel_create_image(__DRIscreen *dri_screen, > return image; > } > > +static __DRIimage * > +intel_create_image(__DRIscreen *dri_screen, > + int width, int height, int format, > + uns
[Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers
Modifiers will be obtains or guessed by the client and passed in during image creation/import. This requires bumping the DRIimage version. As of this patch, the modifiers aren't plumbed all the way down, this patch simply makes sure the interface level stuff is correct. v2: Don't allow usage + modifiers v3: Make NAND actually NAND. Bug introduced in v2. (Jason) Cc: Kristian Høgsberg Cc: Jason Ekstrand Signed-off-by: Ben Widawsky Reviewed-by: Eric Engestrom (v1) Acked-by: Daniel Stone --- include/GL/internal/dri_interface.h | 27 ++- src/gallium/state_trackers/dri/dri2.c| 1 + src/mesa/drivers/dri/i965/intel_screen.c | 32 +++- 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h index 598d111f33..53fac6fc3c 100644 --- a/include/GL/internal/dri_interface.h +++ b/include/GL/internal/dri_interface.h @@ -1136,7 +1136,7 @@ struct __DRIdri2ExtensionRec { * extensions. */ #define __DRI_IMAGE "DRI_IMAGE" -#define __DRI_IMAGE_VERSION 13 +#define __DRI_IMAGE_VERSION 14 /** * These formats correspond to the similarly named MESA_FORMAT_* @@ -1257,6 +1257,8 @@ struct __DRIdri2ExtensionRec { #define __DRI_IMAGE_ATTRIB_NUM_PLANES 0x2009 /* available in versions 11 */ #define __DRI_IMAGE_ATTRIB_OFFSET 0x200A /* available in versions 13 */ +#define __DRI_IMAGE_ATTRIB_MODIFIER_LOWER 0x200B /* available in versions 14 */ +#define __DRI_IMAGE_ATTRIB_MODIFIER_UPPER 0x200C /* available in versions 14 */ enum __DRIYUVColorSpace { __DRI_YUV_COLOR_SPACE_UNDEFINED = 0, @@ -1468,6 +1470,29 @@ struct __DRIimageExtensionRec { */ void (*unmapImage)(__DRIcontext *context, __DRIimage *image, void *data); + + /** +* Creates an image with implementation's favorite modifiers. +* +* This acts like createImage except there is a list of modifiers passed in +* which the implementation may selectively use to create the DRIimage. The +* result should be the implementation selects one modifier (perhaps it would +* hold on to a few and later pick). +* +* The created image should be destroyed with destroyImage(). +* +* Returns the new DRIimage. The chosen modifier can be obtained later on +* and passed back to things like the kernel's AddFB2 interface. +* +* \sa __DRIimageRec::createImage +* +* \since 14 +*/ + __DRIimage *(*createImageWithModifiers)(__DRIscreen *screen, + int width, int height, int format, + const uint64_t *modifiers, + const unsigned int modifier_count, + void *loaderPrivate); }; diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c index b50e096443..12e466c6f1 100644 --- a/src/gallium/state_trackers/dri/dri2.c +++ b/src/gallium/state_trackers/dri/dri2.c @@ -1413,6 +1413,7 @@ static __DRIimageExtension dri2ImageExtension = { .getCapabilities = dri2_get_capabilities, .mapImage = dri2_map_image, .unmapImage = dri2_unmap_image, +.createImageWithModifiers = NULL, }; diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c index 21786eb54a..3452572874 100644 --- a/src/mesa/drivers/dri/i965/intel_screen.c +++ b/src/mesa/drivers/dri/i965/intel_screen.c @@ -510,9 +510,11 @@ intel_destroy_image(__DRIimage *image) } static __DRIimage * -intel_create_image(__DRIscreen *dri_screen, +__intel_create_image(__DRIscreen *dri_screen, int width, int height, int format, unsigned int use, + const uint64_t *modifiers, + unsigned count, void *loaderPrivate) { __DRIimage *image; @@ -521,6 +523,12 @@ intel_create_image(__DRIscreen *dri_screen, int cpp; unsigned long pitch; + /* Callers of this may specify a modifier, or a dri usage, but not both. The +* newer modifier interface deprecates the older usage flags newer modifier +* interface deprecates the older usage flags. +*/ + assert(!(use && count)); + tiling = I915_TILING_X; if (use & __DRI_IMAGE_USE_CURSOR) { if (width != 64 || height != 64) @@ -550,6 +558,27 @@ intel_create_image(__DRIscreen *dri_screen, return image; } +static __DRIimage * +intel_create_image(__DRIscreen *dri_screen, + int width, int height, int format, + unsigned int use, + void *loaderPrivate) +{ + return __intel_create_image(dri_screen, width, height, format, use, NULL, 0, + loaderPrivate); +} + +static __DRIimage * +intel_create_image_with_modifiers(__DRIscreen *dri_screen, + int width, i