subject:"\[Mesa\-dev\] \[PATCH 1\/3\] dri\: Add an image creation with modifiers"

Re: [Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers

2017-03-16 Thread Emil Velikov

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

2017-03-13 Thread Ben Widawsky


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

2017-03-13 Thread Ben Widawsky


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

Re: [Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers

2017-03-10 Thread Emil Velikov

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

2017-03-09 Thread Jason Ekstrand

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

[Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers

2017-03-09 Thread Ben Widawsky

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

Re: [Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers

Re: [Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers

Re: [Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers

Re: [Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers

Re: [Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers

[Mesa-dev] [PATCH 1/3] dri: Add an image creation with modifiers

6 matches

Site Navigation

Mail list logo

Footer information