[Intel-gfx] [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Dave Airlie]

> +
> +#define fourcc_mod_code(vendor, val) \
> +   u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val & 
> 0x00ffL))

eh, yeah no,

/home/airlied/kernel/drm-2.6/drivers/gpu/drm/i915/intel_pm.c: In
function ‘skl_wm_method2’:
/home/airlied/kernel/drm-2.6/drivers/gpu/drm/i915/intel_pm.c:2631:
warning: integer constant is too large for ‘long’ type
/home/airlied/kernel/drm-2.6/drivers/gpu/drm/i915/intel_pm.c:2632:
warning: integer constant is too large for ‘long’ type

I think you meant ULL here.

Dave.

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Thierry Reding]

On Fri, Jan 30, 2015 at 05:08:23PM +0100, Daniel Vetter wrote:
> From: Rob Clark 
> 
> In DRM/KMS we are lacking a good way to deal with tiled/compressed
> formats.  Especially in the case of dmabuf/prime buffer sharing, where
> we cannot always rely on under-the-hood flags passed to driver specific
> gem-create ioctl to pass around these extra flags.
> 
> The proposal is to add a per-plane format modifier.  This allows to, if
> necessary, use different tiling patters for sub-sampled planes, etc.

"patterns". Alternatively perhaps "modes", which is how I've heard it
referred to most commonly.

> The format modifiers are added at the end of the ioctl struct, so for
> legacy userspace it will be zero padded.
> 
> v1: original
> v1.5: increase modifier to 64b
> 
> v2: Incorporate review comments from the big thread, plus a few more.
> 
> - Add a getcap so that userspace doesn't have to jump through hoops.
> - Allow modifiers only when a flag is set. That way drivers know when
>   they're dealing with old userspace and need to fish out e.g. tiling
>   from other information.
> - After rolling out checks for ->modifier to all drivers I've decided
>   that this is way too fragile and needs an explicit opt-in flag. So
>   do that instead.
> - Add a define (just for documentation really) for the "NONE"
>   modifier. Imo we don't need to add mask #defines since drivers
>   really should only do exact matches against values defined with
>   fourcc_mod_code.
> - Drop the Samsung tiling modifier on Rob's request since he's not yet
>   sure whether that one is accurate.
> 
> v3:
> - Also add a new ->modifier[] array to struct drm_framebuffer and fill
>   it in drm_helper_mode_fill_fb_struct. Requested by Tvrtko Uruslin.
> - Remove TODO in comment and add code comment that modifiers should be
>   properly documented, requested by Rob.
> 
> v4: Balance parens, spotted by Tvrtko.
> 
> Cc: Rob Clark 
> Cc: Tvrtko Ursulin 
> Cc: Laurent Pinchart 
> Cc: Daniel Stone 
> Cc: Ville Syrjälä 
> Cc: Michel Dänzer 
> Signed-off-by: Rob Clark  (v1.5)
> Reviewed-by: Rob Clark 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_crtc.c| 14 +-
>  drivers/gpu/drm/drm_crtc_helper.c |  1 +
>  drivers/gpu/drm/drm_ioctl.c   |  3 +++
>  include/drm/drm_crtc.h|  4 
>  include/uapi/drm/drm.h|  1 +
>  include/uapi/drm/drm_fourcc.h | 32 
>  include/uapi/drm/drm_mode.h   |  9 +
>  7 files changed, 63 insertions(+), 1 deletion(-)

Also as discussed on IRC, I think this would be better in a non-DRM
specific header so that we can have a central, cross-subsystem
authority.

> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 646ae5f39f42..622109677747 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -132,4 +132,36 @@
>  #define DRM_FORMAT_YUV444fourcc_code('Y', 'U', '2', '4') /* 
> non-subsampled Cb (1) and Cr (2) planes */
>  #define DRM_FORMAT_YVU444fourcc_code('Y', 'V', '2', '4') /* 
> non-subsampled Cr (1) and Cb (2) planes */
>  
> +

Possibly unintented extra blank line?

> +/*
> + * Format Modifiers:
> + *
> + * Format modifiers describe, typically, a re-ordering or modification
> + * of the data in a plane of an FB.  This can be used to express tiled/
> + * swizzled formats, or compression, or a combination of the two.
> + *
> + * The upper 8 bits of the format modifier are a vendor-id as assigned
> + * below.  The lower 56 bits are assigned as vendor sees fit.
> + */
> +
> +/* Vendor Ids: */
> +#define DRM_FORMAT_MOD_NONE   0
> +#define DRM_FORMAT_MOD_VENDOR_INTEL   0x01
> +#define DRM_FORMAT_MOD_VENDOR_AMD 0x02
> +#define DRM_FORMAT_MOD_VENDOR_NV  0x03


I think this should be NVIDIA for consistency with other naming in the
kernel, at least on Tegra.

Otherwise:

Reviewed-by: Thierry Reding 
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Vetter]

From: Rob Clark 

In DRM/KMS we are lacking a good way to deal with tiled/compressed
formats.  Especially in the case of dmabuf/prime buffer sharing, where
we cannot always rely on under-the-hood flags passed to driver specific
gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier.  This allows to, if
necessary, use different tiling patters for sub-sampled planes, etc.
The format modifiers are added at the end of the ioctl struct, so for
legacy userspace it will be zero padded.

v1: original
v1.5: increase modifier to 64b

v2: Incorporate review comments from the big thread, plus a few more.

- Add a getcap so that userspace doesn't have to jump through hoops.
- Allow modifiers only when a flag is set. That way drivers know when
  they're dealing with old userspace and need to fish out e.g. tiling
  from other information.
- After rolling out checks for ->modifier to all drivers I've decided
  that this is way too fragile and needs an explicit opt-in flag. So
  do that instead.
- Add a define (just for documentation really) for the "NONE"
  modifier. Imo we don't need to add mask #defines since drivers
  really should only do exact matches against values defined with
  fourcc_mod_code.
- Drop the Samsung tiling modifier on Rob's request since he's not yet
  sure whether that one is accurate.

v3:
- Also add a new ->modifier[] array to struct drm_framebuffer and fill
  it in drm_helper_mode_fill_fb_struct. Requested by Tvrtko Uruslin.
- Remove TODO in comment and add code comment that modifiers should be
  properly documented, requested by Rob.

v4: Balance parens, spotted by Tvrtko.

Cc: Rob Clark 
Cc: Tvrtko Ursulin 
Cc: Laurent Pinchart 
Cc: Daniel Stone 
Cc: Ville Syrjälä 
Cc: Michel Dänzer 
Signed-off-by: Rob Clark  (v1.5)
Reviewed-by: Rob Clark 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_crtc.c| 14 +-
 drivers/gpu/drm/drm_crtc_helper.c |  1 +
 drivers/gpu/drm/drm_ioctl.c   |  3 +++
 include/drm/drm_crtc.h|  4 
 include/uapi/drm/drm.h|  1 +
 include/uapi/drm/drm_fourcc.h | 32 
 include/uapi/drm/drm_mode.h   |  9 +
 7 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 419f9d915c78..8090e3d64f67 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3324,6 +3324,12 @@ static int framebuffer_check(const struct 
drm_mode_fb_cmd2 *r)
DRM_DEBUG_KMS("bad pitch %u for plane %d\n", 
r->pitches[i], i);
return -EINVAL;
}
+
+   if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
+   DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
+ r->modifier[i], i);
+   return -EINVAL;
+   }
}

return 0;
@@ -3337,7 +3343,7 @@ static struct drm_framebuffer 
*add_framebuffer_internal(struct drm_device *dev,
struct drm_framebuffer *fb;
int ret;

-   if (r->flags & ~DRM_MODE_FB_INTERLACED) {
+   if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
return ERR_PTR(-EINVAL);
}
@@ -3353,6 +3359,12 @@ static struct drm_framebuffer 
*add_framebuffer_internal(struct drm_device *dev,
return ERR_PTR(-EINVAL);
}

+   if (r->flags & DRM_MODE_FB_MODIFIERS &&
+   !dev->mode_config.allow_fb_modifiers) {
+   DRM_DEBUG_KMS("driver does not support fb modifiers\n");
+   return ERR_PTR(-EINVAL);
+   }
+
ret = framebuffer_check(r);
if (ret)
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
b/drivers/gpu/drm/drm_crtc_helper.c
index b1979e7bdc88..3053aab968f9 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -837,6 +837,7 @@ void drm_helper_mode_fill_fb_struct(struct drm_framebuffer 
*fb,
for (i = 0; i < 4; i++) {
fb->pitches[i] = mode_cmd->pitches[i];
fb->offsets[i] = mode_cmd->offsets[i];
+   fb->modifier[i] = mode_cmd->modifier[i];
}
drm_fb_get_bpp_depth(mode_cmd->pixel_format, >depth,
>bits_per_pixel);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 3785d66721f2..a6d773a61c2d 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -321,6 +321,9 @@ static int drm_getcap(struct drm_device *dev, void *data, 
struct drm_file *file_
else
req->value = 64;
break;
+   case DRM_CAP_ADDFB2_MODIFIERS:
+   req->value = dev->mode_config.allow_fb_modifiers;
+   break;

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Vetter]

On Fri, Jan 30, 2015 at 09:51:48AM -0500, Rob Clark wrote:
> On Fri, Jan 30, 2015 at 9:35 AM, Tvrtko Ursulin
>  wrote:
> >
> > On 01/30/2015 01:43 PM, Rob Clark wrote:
> >>
> >> On Fri, Jan 30, 2015 at 5:51 AM, Tvrtko Ursulin
> >>  wrote:
> 
>  +/*
>  + * Format Modifier tokens:
>  + *
>  + * When adding a new token please document the layout with a code
>  comment,
>  + * similar to the fourcc codes above. drm_fourcc.h is considered the
>  + * authoritative source for all of these.
>  + */
> >>>
> >>>
> >>>
> >>> On one side modifiers are supposed to be opaque, but then this suggest
> >>> they
> >>> are supposed to be added in this file and described. Is that right?
> >>
> >>
> >>
> >> correct..   opaque as in basically enum values.
> >>
> >> We do want a description of the format so when someone comes along and
> >> adds a new value, we have a chance of realizing that it is the same as
> >> an existing value, since there are cases where gpu's from different
> >> vendors can support (for example) the same tiling formats.
> >
> >
> > Opaque kind of suggests it is driver private and from that angle definitions
> > and descriptions wouldn't belong in the core uapi header. So I think that's
> > just confusing and I'd drop opaque from the description and just call it a
> > token.
> 
> hmm, to me, 'opaque' just meant 'do not try to interpret the bits'..
> but if that is confusing, I don't mind to just call it a token

Yeah could be misunderstood, luckily both the commit message and code
comments already just talk about tokens. So I think we're good.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Tvrtko Ursulin]

On 01/29/2015 05:01 PM, Daniel Vetter wrote:
> +#define fourcc_mod_code(vendor, val) \
> + u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val & 
> 0x00ffL)

Unbalanced parentheses.

Finding them as I go along, sorry! :)

Regards,

Tvrtko

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Tvrtko Ursulin]

On 01/30/2015 01:43 PM, Rob Clark wrote:
> On Fri, Jan 30, 2015 at 5:51 AM, Tvrtko Ursulin
>  wrote:
>>> +/*
>>> + * Format Modifier tokens:
>>> + *
>>> + * When adding a new token please document the layout with a code
>>> comment,
>>> + * similar to the fourcc codes above. drm_fourcc.h is considered the
>>> + * authoritative source for all of these.
>>> + */
>>
>>
>> On one side modifiers are supposed to be opaque, but then this suggest they
>> are supposed to be added in this file and described. Is that right?
>
>
> correct..   opaque as in basically enum values.
>
> We do want a description of the format so when someone comes along and
> adds a new value, we have a chance of realizing that it is the same as
> an existing value, since there are cases where gpu's from different
> vendors can support (for example) the same tiling formats.

Opaque kind of suggests it is driver private and from that angle 
definitions and descriptions wouldn't belong in the core uapi header. So 
I think that's just confusing and I'd drop opaque from the description 
and just call it a token.

(And another reason why I was suggesting to split the space for 
potential common and vendor/driver specific tokens.)

Regards,

Tvrtko

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Tvrtko Ursulin]

On 01/29/2015 05:01 PM, Daniel Vetter wrote:

[snip]

> +
> +/*
> + * Format Modifiers:
> + *
> + * Format modifiers describe, typically, a re-ordering or modification
> + * of the data in a plane of an FB.  This can be used to express tiled/
> + * swizzled formats, or compression, or a combination of the two.
> + *
> + * The upper 8 bits of the format modifier are a vendor-id as assigned
> + * below.  The lower 56 bits are assigned as vendor sees fit.
> + */
> +
> +/* Vendor Ids: */
> +#define DRM_FORMAT_MOD_NONE   0
> +#define DRM_FORMAT_MOD_VENDOR_INTEL   0x01
> +#define DRM_FORMAT_MOD_VENDOR_AMD 0x02
> +#define DRM_FORMAT_MOD_VENDOR_NV  0x03
> +#define DRM_FORMAT_MOD_VENDOR_SAMSUNG 0x04
> +#define DRM_FORMAT_MOD_VENDOR_QCOM0x05
> +/* add more to the end as needed */
> +
> +#define fourcc_mod_code(vendor, val) \
> + u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val & 
> 0x00ffL)
> +
> +/*
> + * Format Modifier tokens:
> + *
> + * When adding a new token please document the layout with a code comment,
> + * similar to the fourcc codes above. drm_fourcc.h is considered the
> + * authoritative source for all of these.
> + */

On one side modifiers are supposed to be opaque, but then this suggest 
they are supposed to be added in this file and described. Is that right?

Regards,

Tvrtko

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Rob Clark]

On Fri, Jan 30, 2015 at 9:35 AM, Tvrtko Ursulin
 wrote:
>
> On 01/30/2015 01:43 PM, Rob Clark wrote:
>>
>> On Fri, Jan 30, 2015 at 5:51 AM, Tvrtko Ursulin
>>  wrote:

 +/*
 + * Format Modifier tokens:
 + *
 + * When adding a new token please document the layout with a code
 comment,
 + * similar to the fourcc codes above. drm_fourcc.h is considered the
 + * authoritative source for all of these.
 + */
>>>
>>>
>>>
>>> On one side modifiers are supposed to be opaque, but then this suggest
>>> they
>>> are supposed to be added in this file and described. Is that right?
>>
>>
>>
>> correct..   opaque as in basically enum values.
>>
>> We do want a description of the format so when someone comes along and
>> adds a new value, we have a chance of realizing that it is the same as
>> an existing value, since there are cases where gpu's from different
>> vendors can support (for example) the same tiling formats.
>
>
> Opaque kind of suggests it is driver private and from that angle definitions
> and descriptions wouldn't belong in the core uapi header. So I think that's
> just confusing and I'd drop opaque from the description and just call it a
> token.

hmm, to me, 'opaque' just meant 'do not try to interpret the bits'..
but if that is confusing, I don't mind to just call it a token

> (And another reason why I was suggesting to split the space for potential
> common and vendor/driver specific tokens.)

(which works well until some vendor introduces some format, and later
another vendor adds support for it :-P)

BR,
-R

> Regards,
>
> Tvrtko

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Rob Clark]

On Fri, Jan 30, 2015 at 5:51 AM, Tvrtko Ursulin
 wrote:
>> +/*
>> + * Format Modifier tokens:
>> + *
>> + * When adding a new token please document the layout with a code
>> comment,
>> + * similar to the fourcc codes above. drm_fourcc.h is considered the
>> + * authoritative source for all of these.
>> + */
>
>
> On one side modifiers are supposed to be opaque, but then this suggest they
> are supposed to be added in this file and described. Is that right?


correct..   opaque as in basically enum values.

We do want a description of the format so when someone comes along and
adds a new value, we have a chance of realizing that it is the same as
an existing value, since there are cases where gpu's from different
vendors can support (for example) the same tiling formats.

BR,
-R

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Vetter]

From: Rob Clark 

In DRM/KMS we are lacking a good way to deal with tiled/compressed
formats.  Especially in the case of dmabuf/prime buffer sharing, where
we cannot always rely on under-the-hood flags passed to driver specific
gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier.  This allows to, if
necessary, use different tiling patters for sub-sampled planes, etc.
The format modifiers are added at the end of the ioctl struct, so for
legacy userspace it will be zero padded.

v1: original
v1.5: increase modifier to 64b

v2: Incorporate review comments from the big thread, plus a few more.

- Add a getcap so that userspace doesn't have to jump through hoops.
- Allow modifiers only when a flag is set. That way drivers know when
  they're dealing with old userspace and need to fish out e.g. tiling
  from other information.
- After rolling out checks for ->modifier to all drivers I've decided
  that this is way too fragile and needs an explicit opt-in flag. So
  do that instead.
- Add a define (just for documentation really) for the "NONE"
  modifier. Imo we don't need to add mask #defines since drivers
  really should only do exact matches against values defined with
  fourcc_mod_code.
- Drop the Samsung tiling modifier on Rob's request since he's not yet
  sure whether that one is accurate.

v3:
- Also add a new ->modifier[] array to struct drm_framebuffer and fill
  it in drm_helper_mode_fill_fb_struct. Requested by Tvrkto Uruslin.
- Remove TODO in comment and add code comment that modifiers should be
  properly documented, requested by Rob.

Cc: Rob Clark 
Cc: Tvrtko Ursulin 
Cc: Laurent Pinchart 
Cc: Daniel Stone 
Cc: Ville Syrjälä 
Cc: Michel Dänzer 
Signed-off-by: Rob Clark  (v1.5)
Reviewed-by: Rob Clark 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_crtc.c| 14 +-
 drivers/gpu/drm/drm_crtc_helper.c |  1 +
 drivers/gpu/drm/drm_ioctl.c   |  3 +++
 include/drm/drm_crtc.h|  4 
 include/uapi/drm/drm.h|  1 +
 include/uapi/drm/drm_fourcc.h | 32 
 include/uapi/drm/drm_mode.h   |  9 +
 7 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 419f9d915c78..8090e3d64f67 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3324,6 +3324,12 @@ static int framebuffer_check(const struct 
drm_mode_fb_cmd2 *r)
DRM_DEBUG_KMS("bad pitch %u for plane %d\n", 
r->pitches[i], i);
return -EINVAL;
}
+
+   if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
+   DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
+ r->modifier[i], i);
+   return -EINVAL;
+   }
}

return 0;
@@ -3337,7 +3343,7 @@ static struct drm_framebuffer 
*add_framebuffer_internal(struct drm_device *dev,
struct drm_framebuffer *fb;
int ret;

-   if (r->flags & ~DRM_MODE_FB_INTERLACED) {
+   if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
return ERR_PTR(-EINVAL);
}
@@ -3353,6 +3359,12 @@ static struct drm_framebuffer 
*add_framebuffer_internal(struct drm_device *dev,
return ERR_PTR(-EINVAL);
}

+   if (r->flags & DRM_MODE_FB_MODIFIERS &&
+   !dev->mode_config.allow_fb_modifiers) {
+   DRM_DEBUG_KMS("driver does not support fb modifiers\n");
+   return ERR_PTR(-EINVAL);
+   }
+
ret = framebuffer_check(r);
if (ret)
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/drm_crtc_helper.c 
b/drivers/gpu/drm/drm_crtc_helper.c
index b1979e7bdc88..3053aab968f9 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -837,6 +837,7 @@ void drm_helper_mode_fill_fb_struct(struct drm_framebuffer 
*fb,
for (i = 0; i < 4; i++) {
fb->pitches[i] = mode_cmd->pitches[i];
fb->offsets[i] = mode_cmd->offsets[i];
+   fb->modifier[i] = mode_cmd->modifier[i];
}
drm_fb_get_bpp_depth(mode_cmd->pixel_format, >depth,
>bits_per_pixel);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 3785d66721f2..a6d773a61c2d 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -321,6 +321,9 @@ static int drm_getcap(struct drm_device *dev, void *data, 
struct drm_file *file_
else
req->value = 64;
break;
+   case DRM_CAP_ADDFB2_MODIFIERS:
+   req->value = dev->mode_config.allow_fb_modifiers;
+   break;
default:
return -EINVAL;

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Daniel Vetter]

On Thu, Jan 29, 2015 at 12:55:48PM +, Tvrtko Ursulin wrote:
> 
> On 01/29/2015 11:57 AM, Daniel Vetter wrote:
> >On Thu, Jan 29, 2015 at 11:43:07AM +, Tvrtko Ursulin wrote:
> >>
> >>On 01/29/2015 11:30 AM, Daniel Vetter wrote:
> >>>On Wed, Jan 28, 2015 at 05:57:56PM +, Tvrtko Ursulin wrote:
> 
> On 01/28/2015 05:37 PM, Daniel Vetter wrote:
> >From: Rob Clark 
> >
> >In DRM/KMS we are lacking a good way to deal with tiled/compressed
> >formats.  Especially in the case of dmabuf/prime buffer sharing, where
> >we cannot always rely on under-the-hood flags passed to driver specific
> >gem-create ioctl to pass around these extra flags.
> >
> >The proposal is to add a per-plane format modifier.  This allows to, if
> >necessary, use different tiling patters for sub-sampled planes, etc.
> >The format modifiers are added at the end of the ioctl struct, so for
> >legacy userspace it will be zero padded.
> >
> >TODO how best to deal with assignment of modifier token values?  The
> >rough idea was to namespace things with an 8bit vendor-id, and then
> >beyond that it is treated as an opaque value.  But that was a relatively
> >arbitrary choice.  There are cases where same tiling pattern and/or
> >compression is supported by various different vendors.  So we should
> >standardize to use the vendor-id and value of the first one who
> >documents the format?
> 
> Maybe:
>   __u64 modifier[4];
>   __u64 vendor_modifier[4];
> >>>
> >>>Seems rendundant since the modifier added in this patch is already vendor
> >>>specific. Or what exactly are you trying to accomplish here?
> >>
> >>I am trying to avoid packet-in-a-packet (bitmasks) mumbo-jumbo and vendor id
> >>on the head followed by maybe standardized or maybe vendor specific tag.
> >>Feels funny. Would it not be simpler to put a struct in there?
> >
> >The u64 modifier is just an opaque thing, with 8 bit to identifier the
> >vendor (for easier number management really) and the low 56 bits can be
> >whatever we want them. On i915 I think we should just enumerate all the
> >various tiling modes we have. And if the modifiers aren't set we use the
> >tiling mode of the underlying gem bo. We already have code in place to
> >guarantee that the underlying bo's tiling can't change as long as there's
> >a kms fb around, which means all code which checks for tiling can switch
> >over to these flags.
> >
> >struct won't work since by definition this is vendor-specific, and every
> >vendor is slightly insane in a different way.
> 
> Well 8 + 56 bits is a "struct" already and not fully opaque. Are the bits
> expensive? :) That was first part of my point.
> 
> Secondly, "vendor who registers first" part of discussion is what came to my
> attention as well.
> 
> With this, a hypothetical standard format would be under a vendor id which
> looks funny to me. While you could split standard formats/modifiers and
> vendor specific modifiers.

If some standard body actually manages to pull this off we can always add
a new enum value for KHRONOS or MICROSOFT or ISO or whatever it ends up
being. The split really is just to make number assignements less
conflit-y.

> I don't care really, it was just an idea, but I don't get this arguments why
> it is, not only not better, but worse than a bitfield. :)

Just from your struct without any explanation what your idea was (namely
modifiers which are standardized across vendors it seems) it looked like
both a plain modifier and a vendor_modifier was possible. Which didn't
make a lot of sense to me, so I asked.

Going with an opaque u64 has the benefits that it matches kms property
values. So if we ever extend this and allow generic properties for
framebuffers it'll still fit. A struct is more painful. The idea of fb
properties was one of the longer-term ideas tossed around in the v1
thread.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Daniel Vetter]

On Thu, Jan 29, 2015 at 11:43:07AM +, Tvrtko Ursulin wrote:
> 
> On 01/29/2015 11:30 AM, Daniel Vetter wrote:
> >On Wed, Jan 28, 2015 at 05:57:56PM +, Tvrtko Ursulin wrote:
> >>
> >>On 01/28/2015 05:37 PM, Daniel Vetter wrote:
> >>>From: Rob Clark 
> >>>
> >>>In DRM/KMS we are lacking a good way to deal with tiled/compressed
> >>>formats.  Especially in the case of dmabuf/prime buffer sharing, where
> >>>we cannot always rely on under-the-hood flags passed to driver specific
> >>>gem-create ioctl to pass around these extra flags.
> >>>
> >>>The proposal is to add a per-plane format modifier.  This allows to, if
> >>>necessary, use different tiling patters for sub-sampled planes, etc.
> >>>The format modifiers are added at the end of the ioctl struct, so for
> >>>legacy userspace it will be zero padded.
> >>>
> >>>TODO how best to deal with assignment of modifier token values?  The
> >>>rough idea was to namespace things with an 8bit vendor-id, and then
> >>>beyond that it is treated as an opaque value.  But that was a relatively
> >>>arbitrary choice.  There are cases where same tiling pattern and/or
> >>>compression is supported by various different vendors.  So we should
> >>>standardize to use the vendor-id and value of the first one who
> >>>documents the format?
> >>
> >>Maybe:
> >>__u64 modifier[4];
> >>__u64 vendor_modifier[4];
> >
> >Seems rendundant since the modifier added in this patch is already vendor
> >specific. Or what exactly are you trying to accomplish here?
> 
> I am trying to avoid packet-in-a-packet (bitmasks) mumbo-jumbo and vendor id
> on the head followed by maybe standardized or maybe vendor specific tag.
> Feels funny. Would it not be simpler to put a struct in there?

The u64 modifier is just an opaque thing, with 8 bit to identifier the
vendor (for easier number management really) and the low 56 bits can be
whatever we want them. On i915 I think we should just enumerate all the
various tiling modes we have. And if the modifiers aren't set we use the
tiling mode of the underlying gem bo. We already have code in place to
guarantee that the underlying bo's tiling can't change as long as there's
a kms fb around, which means all code which checks for tiling can switch
over to these flags.

struct won't work since by definition this is vendor-specific, and every
vendor is slightly insane in a different way.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Tvrtko Ursulin]

On 01/29/2015 11:57 AM, Daniel Vetter wrote:
> On Thu, Jan 29, 2015 at 11:43:07AM +, Tvrtko Ursulin wrote:
>>
>> On 01/29/2015 11:30 AM, Daniel Vetter wrote:
>>> On Wed, Jan 28, 2015 at 05:57:56PM +, Tvrtko Ursulin wrote:

 On 01/28/2015 05:37 PM, Daniel Vetter wrote:
> From: Rob Clark 
>
> In DRM/KMS we are lacking a good way to deal with tiled/compressed
> formats.  Especially in the case of dmabuf/prime buffer sharing, where
> we cannot always rely on under-the-hood flags passed to driver specific
> gem-create ioctl to pass around these extra flags.
>
> The proposal is to add a per-plane format modifier.  This allows to, if
> necessary, use different tiling patters for sub-sampled planes, etc.
> The format modifiers are added at the end of the ioctl struct, so for
> legacy userspace it will be zero padded.
>
> TODO how best to deal with assignment of modifier token values?  The
> rough idea was to namespace things with an 8bit vendor-id, and then
> beyond that it is treated as an opaque value.  But that was a relatively
> arbitrary choice.  There are cases where same tiling pattern and/or
> compression is supported by various different vendors.  So we should
> standardize to use the vendor-id and value of the first one who
> documents the format?

 Maybe:
__u64 modifier[4];
__u64 vendor_modifier[4];
>>>
>>> Seems rendundant since the modifier added in this patch is already vendor
>>> specific. Or what exactly are you trying to accomplish here?
>>
>> I am trying to avoid packet-in-a-packet (bitmasks) mumbo-jumbo and vendor id
>> on the head followed by maybe standardized or maybe vendor specific tag.
>> Feels funny. Would it not be simpler to put a struct in there?
>
> The u64 modifier is just an opaque thing, with 8 bit to identifier the
> vendor (for easier number management really) and the low 56 bits can be
> whatever we want them. On i915 I think we should just enumerate all the
> various tiling modes we have. And if the modifiers aren't set we use the
> tiling mode of the underlying gem bo. We already have code in place to
> guarantee that the underlying bo's tiling can't change as long as there's
> a kms fb around, which means all code which checks for tiling can switch
> over to these flags.
>
> struct won't work since by definition this is vendor-specific, and every
> vendor is slightly insane in a different way.

Well 8 + 56 bits is a "struct" already and not fully opaque. Are the 
bits expensive? :) That was first part of my point.

Secondly, "vendor who registers first" part of discussion is what came 
to my attention as well.

With this, a hypothetical standard format would be under a vendor id 
which looks funny to me. While you could split standard 
formats/modifiers and vendor specific modifiers.

I don't care really, it was just an idea, but I don't get this arguments 
why it is, not only not better, but worse than a bitfield. :)

Regards,

Tvrtko

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Daniel Vetter]

On Wed, Jan 28, 2015 at 05:57:56PM +, Tvrtko Ursulin wrote:
> 
> On 01/28/2015 05:37 PM, Daniel Vetter wrote:
> >From: Rob Clark 
> >
> >In DRM/KMS we are lacking a good way to deal with tiled/compressed
> >formats.  Especially in the case of dmabuf/prime buffer sharing, where
> >we cannot always rely on under-the-hood flags passed to driver specific
> >gem-create ioctl to pass around these extra flags.
> >
> >The proposal is to add a per-plane format modifier.  This allows to, if
> >necessary, use different tiling patters for sub-sampled planes, etc.
> >The format modifiers are added at the end of the ioctl struct, so for
> >legacy userspace it will be zero padded.
> >
> >TODO how best to deal with assignment of modifier token values?  The
> >rough idea was to namespace things with an 8bit vendor-id, and then
> >beyond that it is treated as an opaque value.  But that was a relatively
> >arbitrary choice.  There are cases where same tiling pattern and/or
> >compression is supported by various different vendors.  So we should
> >standardize to use the vendor-id and value of the first one who
> >documents the format?
> 
> Maybe:
>   __u64 modifier[4];
>   __u64 vendor_modifier[4];

Seems rendundant since the modifier added in this patch is already vendor
specific. Or what exactly are you trying to accomplish here?
-Daniel

> 
> ?
> 
> Regards,
> 
> Tvrtko

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Daniel Vetter]

On Wed, Jan 28, 2015 at 01:46:53PM -0500, Rob Clark wrote:
> On Wed, Jan 28, 2015 at 12:37 PM, Daniel Vetter  
> wrote:
> > From: Rob Clark 
> >
> > In DRM/KMS we are lacking a good way to deal with tiled/compressed
> > formats.  Especially in the case of dmabuf/prime buffer sharing, where
> > we cannot always rely on under-the-hood flags passed to driver specific
> > gem-create ioctl to pass around these extra flags.
> >
> > The proposal is to add a per-plane format modifier.  This allows to, if
> > necessary, use different tiling patters for sub-sampled planes, etc.
> > The format modifiers are added at the end of the ioctl struct, so for
> > legacy userspace it will be zero padded.
> >
> > TODO how best to deal with assignment of modifier token values?  The
> > rough idea was to namespace things with an 8bit vendor-id, and then
> > beyond that it is treated as an opaque value.  But that was a relatively
> > arbitrary choice.  There are cases where same tiling pattern and/or
> > compression is supported by various different vendors.  So we should
> > standardize to use the vendor-id and value of the first one who
> > documents the format?
> 
> iirc, I think we decided that drm_fourcc.h in drm-next is a sufficient
> authority for coordinating assignment of modifier token values, so we
> could probably drop this TODO.  Perhaps it wouldn't hurt to document
> somewhere that, as with fourcc/format values, new additions are
> expected to come with some description of the format?

Oh right forgotten to drop the TODO. I'll add a comment to the header
file.
> 
> >
> > v1: original
> > v1.5: increase modifier to 64b
> >
> > v2: Incorporate review comments from the big thread, plus a few more.
> >
> > - Add a getcap so that userspace doesn't have to jump through hoops.
> > - Allow modifiers only when a flag is set. That way drivers know when
> >   they're dealing with old userspace and need to fish out e.g. tiling
> >   from other information.
> > - After rolling out checks for ->modifier to all drivers I've decided
> >   that this is way too fragile and needs an explicit opt-in flag. So
> >   do that instead.
> > - Add a define (just for documentation really) for the "NONE"
> >   modifier. Imo we don't need to add mask #defines since drivers
> >   really should only do exact matches against values defined with
> >   fourcc_mod_code.
> > - Drop the Samsung tiling modifier on Rob's request since he's not yet
> >   sure whether that one is accurate.
> >
> > Cc: Rob Clark 
> > Cc: Tvrtko Ursulin 
> > Cc: Laurent Pinchart 
> > Cc: Daniel Stone 
> > Cc: Ville Syrjälä 
> > Cc: Michel Dänzer 
> > Signed-off-by: Rob Clark  (v1.5)
> > Signed-off-by: Daniel Vetter 
> >
> 
> you forgot to change subject line to (v2).. but other than that, looks good

Ah, I generally don't keep a patch revision in the subject and forgot to
update it ;-)

> 
> Reviewed-by: Rob Clark 
> 
> > --
> >
> > I've picked this up since we want to push out some fancy new tiling
> > modes soonish. No defines yet, but Tvrkto is working on the i915 parts
> > of this.
> >
> > I think the only part I haven't done from the discussion is exposing a
> > list of supported modifiers. Not sure that's really useful though
> > since all this is highly hw specific.
> >
> > And a note to driver writes: They need to check or the flag and in its
> > absence make a reasonable choice about the internal layet (e.g. for
> > i915 consult the tiling mode of the underlying bo).
> > -Daniel
> > ---
> >  drivers/gpu/drm/drm_crtc.c| 14 +-
> >  drivers/gpu/drm/drm_ioctl.c   |  3 +++
> >  include/drm/drm_crtc.h|  3 +++
> >  include/uapi/drm/drm.h|  1 +
> >  include/uapi/drm/drm_fourcc.h | 26 ++
> >  include/uapi/drm/drm_mode.h   |  9 +
> >  6 files changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 419f9d915c78..8090e3d64f67 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -3324,6 +3324,12 @@ static int framebuffer_check(const struct 
> > drm_mode_fb_cmd2 *r)
> > DRM_DEBUG_KMS("bad pitch %u for plane %d\n", 
> > r->pitches[i], i);
> > return -EINVAL;
> > }
> > +
> > +   if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
> > +   DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> > + r->modifier[i], i);
> > +   return -EINVAL;
> > +   }
> > }
> >
> > return 0;
> > @@ -3337,7 +3343,7 @@ static struct drm_framebuffer 
> > *add_framebuffer_internal(struct drm_device *dev,
> > struct drm_framebuffer *fb;
> > int ret;
> >
> > -   if (r->flags & ~DRM_MODE_FB_INTERLACED) {
> > +   if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
> > DRM_DEBUG_KMS("bad framebuffer 

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Tvrtko Ursulin]

On 01/29/2015 11:30 AM, Daniel Vetter wrote:
> On Wed, Jan 28, 2015 at 05:57:56PM +, Tvrtko Ursulin wrote:
>>
>> On 01/28/2015 05:37 PM, Daniel Vetter wrote:
>>> From: Rob Clark 
>>>
>>> In DRM/KMS we are lacking a good way to deal with tiled/compressed
>>> formats.  Especially in the case of dmabuf/prime buffer sharing, where
>>> we cannot always rely on under-the-hood flags passed to driver specific
>>> gem-create ioctl to pass around these extra flags.
>>>
>>> The proposal is to add a per-plane format modifier.  This allows to, if
>>> necessary, use different tiling patters for sub-sampled planes, etc.
>>> The format modifiers are added at the end of the ioctl struct, so for
>>> legacy userspace it will be zero padded.
>>>
>>> TODO how best to deal with assignment of modifier token values?  The
>>> rough idea was to namespace things with an 8bit vendor-id, and then
>>> beyond that it is treated as an opaque value.  But that was a relatively
>>> arbitrary choice.  There are cases where same tiling pattern and/or
>>> compression is supported by various different vendors.  So we should
>>> standardize to use the vendor-id and value of the first one who
>>> documents the format?
>>
>> Maybe:
>>  __u64 modifier[4];
>>  __u64 vendor_modifier[4];
>
> Seems rendundant since the modifier added in this patch is already vendor
> specific. Or what exactly are you trying to accomplish here?

I am trying to avoid packet-in-a-packet (bitmasks) mumbo-jumbo and 
vendor id on the head followed by maybe standardized or maybe vendor 
specific tag. Feels funny. Would it not be simpler to put a struct in there?

But I was not following this from the start so maybe I am missing 
something..

Regards,

Tvrtko

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Rob Clark]

On Thu, Jan 29, 2015 at 7:55 AM, Tvrtko Ursulin
 wrote:
>
> On 01/29/2015 11:57 AM, Daniel Vetter wrote:
>>
>> On Thu, Jan 29, 2015 at 11:43:07AM +, Tvrtko Ursulin wrote:
>>>
>>>
>>> On 01/29/2015 11:30 AM, Daniel Vetter wrote:

 On Wed, Jan 28, 2015 at 05:57:56PM +, Tvrtko Ursulin wrote:
>
>
> On 01/28/2015 05:37 PM, Daniel Vetter wrote:
>>
>> From: Rob Clark 
>>
>> In DRM/KMS we are lacking a good way to deal with tiled/compressed
>> formats.  Especially in the case of dmabuf/prime buffer sharing, where
>> we cannot always rely on under-the-hood flags passed to driver
>> specific
>> gem-create ioctl to pass around these extra flags.
>>
>> The proposal is to add a per-plane format modifier.  This allows to,
>> if
>> necessary, use different tiling patters for sub-sampled planes, etc.
>> The format modifiers are added at the end of the ioctl struct, so for
>> legacy userspace it will be zero padded.
>>
>> TODO how best to deal with assignment of modifier token values?  The
>> rough idea was to namespace things with an 8bit vendor-id, and then
>> beyond that it is treated as an opaque value.  But that was a
>> relatively
>> arbitrary choice.  There are cases where same tiling pattern and/or
>> compression is supported by various different vendors.  So we should
>> standardize to use the vendor-id and value of the first one who
>> documents the format?
>
>
> Maybe:
> __u64 modifier[4];
> __u64 vendor_modifier[4];


 Seems rendundant since the modifier added in this patch is already
 vendor
 specific. Or what exactly are you trying to accomplish here?
>>>
>>>
>>> I am trying to avoid packet-in-a-packet (bitmasks) mumbo-jumbo and vendor
>>> id
>>> on the head followed by maybe standardized or maybe vendor specific tag.
>>> Feels funny. Would it not be simpler to put a struct in there?
>>
>>
>> The u64 modifier is just an opaque thing, with 8 bit to identifier the
>> vendor (for easier number management really) and the low 56 bits can be
>> whatever we want them. On i915 I think we should just enumerate all the
>> various tiling modes we have. And if the modifiers aren't set we use the
>> tiling mode of the underlying gem bo. We already have code in place to
>> guarantee that the underlying bo's tiling can't change as long as there's
>> a kms fb around, which means all code which checks for tiling can switch
>> over to these flags.
>>
>> struct won't work since by definition this is vendor-specific, and every
>> vendor is slightly insane in a different way.
>
>
> Well 8 + 56 bits is a "struct" already and not fully opaque. Are the bits
> expensive? :) That was first part of my point.

tbh, we could decide to do something different from 8+56b later if
needed..  nothing should really *depend* on the 8+56, since it is
intended to be an opaque token.  The 8+56 was just intended to make it
easier to merge values coming from different driver trees with less
conflicts.

> Secondly, "vendor who registers first" part of discussion is what came to my
> attention as well.
>
> With this, a hypothetical standard format would be under a vendor id which
> looks funny to me. While you could split standard formats/modifiers and
> vendor specific modifiers.

maybe we should s/vendor/driver/

> I don't care really, it was just an idea, but I don't get this arguments why
> it is, not only not better, but worse than a bitfield. :)

I guess it gets into bikeshed territory a bit, but I've tried to avoid
giving userspace the temptation to assume it is much more than an
opaque value.  The 8+56 thing was mainly just intended for logistical
convenience ;-)

BR,
-R


> Regards,
>
> Tvrtko

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Daniel Vetter]

From: Rob Clark 

In DRM/KMS we are lacking a good way to deal with tiled/compressed
formats.  Especially in the case of dmabuf/prime buffer sharing, where
we cannot always rely on under-the-hood flags passed to driver specific
gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier.  This allows to, if
necessary, use different tiling patters for sub-sampled planes, etc.
The format modifiers are added at the end of the ioctl struct, so for
legacy userspace it will be zero padded.

TODO how best to deal with assignment of modifier token values?  The
rough idea was to namespace things with an 8bit vendor-id, and then
beyond that it is treated as an opaque value.  But that was a relatively
arbitrary choice.  There are cases where same tiling pattern and/or
compression is supported by various different vendors.  So we should
standardize to use the vendor-id and value of the first one who
documents the format?

v1: original
v1.5: increase modifier to 64b

v2: Incorporate review comments from the big thread, plus a few more.

- Add a getcap so that userspace doesn't have to jump through hoops.
- Allow modifiers only when a flag is set. That way drivers know when
  they're dealing with old userspace and need to fish out e.g. tiling
  from other information.
- After rolling out checks for ->modifier to all drivers I've decided
  that this is way too fragile and needs an explicit opt-in flag. So
  do that instead.
- Add a define (just for documentation really) for the "NONE"
  modifier. Imo we don't need to add mask #defines since drivers
  really should only do exact matches against values defined with
  fourcc_mod_code.
- Drop the Samsung tiling modifier on Rob's request since he's not yet
  sure whether that one is accurate.

Cc: Rob Clark 
Cc: Tvrtko Ursulin 
Cc: Laurent Pinchart 
Cc: Daniel Stone 
Cc: Ville Syrjälä 
Cc: Michel Dänzer 
Signed-off-by: Rob Clark  (v1.5)
Signed-off-by: Daniel Vetter 

--

I've picked this up since we want to push out some fancy new tiling
modes soonish. No defines yet, but Tvrkto is working on the i915 parts
of this.

I think the only part I haven't done from the discussion is exposing a
list of supported modifiers. Not sure that's really useful though
since all this is highly hw specific.

And a note to driver writes: They need to check or the flag and in its
absence make a reasonable choice about the internal layet (e.g. for
i915 consult the tiling mode of the underlying bo).
-Daniel
---
 drivers/gpu/drm/drm_crtc.c| 14 +-
 drivers/gpu/drm/drm_ioctl.c   |  3 +++
 include/drm/drm_crtc.h|  3 +++
 include/uapi/drm/drm.h|  1 +
 include/uapi/drm/drm_fourcc.h | 26 ++
 include/uapi/drm/drm_mode.h   |  9 +
 6 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 419f9d915c78..8090e3d64f67 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3324,6 +3324,12 @@ static int framebuffer_check(const struct 
drm_mode_fb_cmd2 *r)
DRM_DEBUG_KMS("bad pitch %u for plane %d\n", 
r->pitches[i], i);
return -EINVAL;
}
+
+   if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
+   DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
+ r->modifier[i], i);
+   return -EINVAL;
+   }
}

return 0;
@@ -3337,7 +3343,7 @@ static struct drm_framebuffer 
*add_framebuffer_internal(struct drm_device *dev,
struct drm_framebuffer *fb;
int ret;

-   if (r->flags & ~DRM_MODE_FB_INTERLACED) {
+   if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
return ERR_PTR(-EINVAL);
}
@@ -3353,6 +3359,12 @@ static struct drm_framebuffer 
*add_framebuffer_internal(struct drm_device *dev,
return ERR_PTR(-EINVAL);
}

+   if (r->flags & DRM_MODE_FB_MODIFIERS &&
+   !dev->mode_config.allow_fb_modifiers) {
+   DRM_DEBUG_KMS("driver does not support fb modifiers\n");
+   return ERR_PTR(-EINVAL);
+   }
+
ret = framebuffer_check(r);
if (ret)
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 3785d66721f2..a6d773a61c2d 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -321,6 +321,9 @@ static int drm_getcap(struct drm_device *dev, void *data, 
struct drm_file *file_
else
req->value = 64;
break;
+   case DRM_CAP_ADDFB2_MODIFIERS:
+   req->value = dev->mode_config.allow_fb_modifiers;
+   break;
default:
return -EINVAL;
  

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Tvrtko Ursulin]

On 01/28/2015 05:37 PM, Daniel Vetter wrote:
> From: Rob Clark 
>
> In DRM/KMS we are lacking a good way to deal with tiled/compressed
> formats.  Especially in the case of dmabuf/prime buffer sharing, where
> we cannot always rely on under-the-hood flags passed to driver specific
> gem-create ioctl to pass around these extra flags.
>
> The proposal is to add a per-plane format modifier.  This allows to, if
> necessary, use different tiling patters for sub-sampled planes, etc.
> The format modifiers are added at the end of the ioctl struct, so for
> legacy userspace it will be zero padded.
>
> TODO how best to deal with assignment of modifier token values?  The
> rough idea was to namespace things with an 8bit vendor-id, and then
> beyond that it is treated as an opaque value.  But that was a relatively
> arbitrary choice.  There are cases where same tiling pattern and/or
> compression is supported by various different vendors.  So we should
> standardize to use the vendor-id and value of the first one who
> documents the format?

Maybe:
__u64 modifier[4];
__u64 vendor_modifier[4];

?

Regards,

Tvrtko

[PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Rob Clark]

On Wed, Jan 28, 2015 at 12:37 PM, Daniel Vetter  
wrote:
> From: Rob Clark 
>
> In DRM/KMS we are lacking a good way to deal with tiled/compressed
> formats.  Especially in the case of dmabuf/prime buffer sharing, where
> we cannot always rely on under-the-hood flags passed to driver specific
> gem-create ioctl to pass around these extra flags.
>
> The proposal is to add a per-plane format modifier.  This allows to, if
> necessary, use different tiling patters for sub-sampled planes, etc.
> The format modifiers are added at the end of the ioctl struct, so for
> legacy userspace it will be zero padded.
>
> TODO how best to deal with assignment of modifier token values?  The
> rough idea was to namespace things with an 8bit vendor-id, and then
> beyond that it is treated as an opaque value.  But that was a relatively
> arbitrary choice.  There are cases where same tiling pattern and/or
> compression is supported by various different vendors.  So we should
> standardize to use the vendor-id and value of the first one who
> documents the format?

iirc, I think we decided that drm_fourcc.h in drm-next is a sufficient
authority for coordinating assignment of modifier token values, so we
could probably drop this TODO.  Perhaps it wouldn't hurt to document
somewhere that, as with fourcc/format values, new additions are
expected to come with some description of the format?

>
> v1: original
> v1.5: increase modifier to 64b
>
> v2: Incorporate review comments from the big thread, plus a few more.
>
> - Add a getcap so that userspace doesn't have to jump through hoops.
> - Allow modifiers only when a flag is set. That way drivers know when
>   they're dealing with old userspace and need to fish out e.g. tiling
>   from other information.
> - After rolling out checks for ->modifier to all drivers I've decided
>   that this is way too fragile and needs an explicit opt-in flag. So
>   do that instead.
> - Add a define (just for documentation really) for the "NONE"
>   modifier. Imo we don't need to add mask #defines since drivers
>   really should only do exact matches against values defined with
>   fourcc_mod_code.
> - Drop the Samsung tiling modifier on Rob's request since he's not yet
>   sure whether that one is accurate.
>
> Cc: Rob Clark 
> Cc: Tvrtko Ursulin 
> Cc: Laurent Pinchart 
> Cc: Daniel Stone 
> Cc: Ville Syrjälä 
> Cc: Michel Dänzer 
> Signed-off-by: Rob Clark  (v1.5)
> Signed-off-by: Daniel Vetter 
>

you forgot to change subject line to (v2).. but other than that, looks good

Reviewed-by: Rob Clark 

> --
>
> I've picked this up since we want to push out some fancy new tiling
> modes soonish. No defines yet, but Tvrkto is working on the i915 parts
> of this.
>
> I think the only part I haven't done from the discussion is exposing a
> list of supported modifiers. Not sure that's really useful though
> since all this is highly hw specific.
>
> And a note to driver writes: They need to check or the flag and in its
> absence make a reasonable choice about the internal layet (e.g. for
> i915 consult the tiling mode of the underlying bo).
> -Daniel
> ---
>  drivers/gpu/drm/drm_crtc.c| 14 +-
>  drivers/gpu/drm/drm_ioctl.c   |  3 +++
>  include/drm/drm_crtc.h|  3 +++
>  include/uapi/drm/drm.h|  1 +
>  include/uapi/drm/drm_fourcc.h | 26 ++
>  include/uapi/drm/drm_mode.h   |  9 +
>  6 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 419f9d915c78..8090e3d64f67 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3324,6 +3324,12 @@ static int framebuffer_check(const struct 
> drm_mode_fb_cmd2 *r)
> DRM_DEBUG_KMS("bad pitch %u for plane %d\n", 
> r->pitches[i], i);
> return -EINVAL;
> }
> +
> +   if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
> +   DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> + r->modifier[i], i);
> +   return -EINVAL;
> +   }
> }
>
> return 0;
> @@ -3337,7 +3343,7 @@ static struct drm_framebuffer 
> *add_framebuffer_internal(struct drm_device *dev,
> struct drm_framebuffer *fb;
> int ret;
>
> -   if (r->flags & ~DRM_MODE_FB_INTERLACED) {
> +   if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
> DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
> return ERR_PTR(-EINVAL);
> }
> @@ -3353,6 +3359,12 @@ static struct drm_framebuffer 
> *add_framebuffer_internal(struct drm_device *dev,
> return ERR_PTR(-EINVAL);
> }
>
> +   if (r->flags & DRM_MODE_FB_MODIFIERS &&
> +   !dev->mode_config.allow_fb_modifiers) {
> +   DRM_DEBUG_KMS("driver does not support fb modifiers\n");
> +   

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Laurent Pinchart]

Hi Rob,

On Thursday 18 December 2014 19:55:24 Rob Clark wrote:
> On Thu, Dec 18, 2014 at 4:22 PM, Daniel Vetter  wrote:
> >> TODO move definition of tokens to drm_fourcc.h?
> > 
> > Seems orthogonal imo. Another todo is to add checking to all
> > drivers to reject it if it's not 0 with -EINVAL. Otherwise we have yet
> > another case of an ioctl with fields that can't actually be used
> > everywhere.
>  
>  Could we please add the check in core code instead of drivers ?
> >>> 
> >>> Nope since then no driver could ever use that extension. Defeats the
> >>> point ;-)
> >> 
> >> Except if we follow the proposal of adding a flag to tell whether a
> >> driver supports the extension ;-)
> > 
> > I'm not a terrible big fan of driver flags, mostly because I've seen too
> > much of the horrible stuff in dri1. Imo much better to pass everything to
> > drivers and help them out with helpers if needed. I might be going
> > overboard a bit with my bias against driver flags ;-)
> 
> I ventured a little ways down the thought path of adding list of
> supported modifier tokens per plane.. and then doing more complete
> checks in the core.  But then the question is, what about cases where
> some tiling format is only supported for UV but not Y, etc.. it
> quickly gets ugly.

>From my experience with V4L2 expressing such constraints in a way that would 
be both simple and comprehensive isn't possible. We should aim for the common 
case, and I agree that finding out what the common case is would require 
implementing the feature first.

However, it would be pretty easy to flag whether a driver supports this new 
API at all. That could be used to zero the extra fields.

> I think for now better to let the driver do this (with a must_be_all_zeros()
> helper for what I expect will be the common case initially).  If common
> patterns emerge, then we refactor out a better helper..

-- 
Regards,

Laurent Pinchart

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Laurent Pinchart]

Hi Daniel,

On Monday 15 December 2014 08:33:10 Daniel Vetter wrote:
> On Fri, Dec 12, 2014 at 10:56:53PM +0200, Laurent Pinchart wrote:
> > On Wednesday 10 December 2014 18:30:10 Daniel Vetter wrote:
> > > On Wed, Dec 10, 2014 at 12:17:51PM -0500, Rob Clark wrote:
> > > > In DRM/KMS we are lacking a good way to deal with tiled/compressed
> > > > formats.  Especially in the case of dmabuf/prime buffer sharing, where
> > > > we cannot always rely on under-the-hood flags passed to driver
> > > > specific gem-create ioctl to pass around these extra flags.
> > > > 
> > > > The proposal is to add a per-plane format modifier.  This allows to,
> > > > if necessary, use different tiling patters for sub-sampled planes,
> > > > etc. The format modifiers are added at the end of the ioctl struct, so
> > > > for legacy userspace it will be zero padded.
> > > > 
> > > > TODO how best to deal with assignment of modifier token values?  The
> > > > rough idea was to namespace things with an 8bit vendor-id, and then
> > > > beyond that it is treated as an opaque value.  But that was a
> > > > relatively arbitrary choice.  There are cases where same tiling
> > > > pattern and/or compression is supported by various different vendors. 
> > > > So we should standardize to use the vendor-id and value of the first
> > > > one who documents the format?
> > > 
> > > 8bits should be enough, will take a while until we have more than 250
> > > gpu drivers in the linux kernel ;-) I'm leaning a bit towards using
> > > 64bits though to make sure that there's enough space in the bitfiel to
> > > encode substrides and stuff like that, in case anyone needs it. For
> > > vendor ids I'd just go with first come and starting at 1 (i.e. rename
> > > yours). That way we make it clear that until a patch is merged upstream
> > > the id isn't reserved yet. drm-next should be sufficient as registry
> > > imo.
> > > 
> > > > TODO move definition of tokens to drm_fourcc.h?
> > > 
> > > Seems orthogonal imo. Another todo is to add checking to all drivers to
> > > reject it if it's not 0 with -EINVAL. Otherwise we have yet another case
> > > of an ioctl with fields that can't actually be used everywhere.
> > 
> > Could we please add the check in core code instead of drivers ?
> 
> Nope since then no driver could ever use that extension. Defeats the point
> ;-)

Except if we follow the proposal of adding a flag to tell whether a driver 
supports the extension ;-)

-- 
Regards,

Laurent Pinchart

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Vetter]

On Thu, Dec 18, 2014 at 10:54:14PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Monday 15 December 2014 08:33:10 Daniel Vetter wrote:
> > On Fri, Dec 12, 2014 at 10:56:53PM +0200, Laurent Pinchart wrote:
> > > On Wednesday 10 December 2014 18:30:10 Daniel Vetter wrote:
> > > > On Wed, Dec 10, 2014 at 12:17:51PM -0500, Rob Clark wrote:
> > > > > In DRM/KMS we are lacking a good way to deal with tiled/compressed
> > > > > formats.  Especially in the case of dmabuf/prime buffer sharing, where
> > > > > we cannot always rely on under-the-hood flags passed to driver
> > > > > specific gem-create ioctl to pass around these extra flags.
> > > > > 
> > > > > The proposal is to add a per-plane format modifier.  This allows to,
> > > > > if necessary, use different tiling patters for sub-sampled planes,
> > > > > etc. The format modifiers are added at the end of the ioctl struct, so
> > > > > for legacy userspace it will be zero padded.
> > > > > 
> > > > > TODO how best to deal with assignment of modifier token values?  The
> > > > > rough idea was to namespace things with an 8bit vendor-id, and then
> > > > > beyond that it is treated as an opaque value.  But that was a
> > > > > relatively arbitrary choice.  There are cases where same tiling
> > > > > pattern and/or compression is supported by various different vendors. 
> > > > > So we should standardize to use the vendor-id and value of the first
> > > > > one who documents the format?
> > > > 
> > > > 8bits should be enough, will take a while until we have more than 250
> > > > gpu drivers in the linux kernel ;-) I'm leaning a bit towards using
> > > > 64bits though to make sure that there's enough space in the bitfiel to
> > > > encode substrides and stuff like that, in case anyone needs it. For
> > > > vendor ids I'd just go with first come and starting at 1 (i.e. rename
> > > > yours). That way we make it clear that until a patch is merged upstream
> > > > the id isn't reserved yet. drm-next should be sufficient as registry
> > > > imo.
> > > > 
> > > > > TODO move definition of tokens to drm_fourcc.h?
> > > > 
> > > > Seems orthogonal imo. Another todo is to add checking to all drivers to
> > > > reject it if it's not 0 with -EINVAL. Otherwise we have yet another case
> > > > of an ioctl with fields that can't actually be used everywhere.
> > > 
> > > Could we please add the check in core code instead of drivers ?
> > 
> > Nope since then no driver could ever use that extension. Defeats the point
> > ;-)
> 
> Except if we follow the proposal of adding a flag to tell whether a driver 
> supports the extension ;-)

I'm not a terrible big fan of driver flags, mostly because I've seen too
much of the horrible stuff in dri1. Imo much better to pass everything to
drivers and help them out with helpers if needed. I might be going
overboard a bit with my bias against driver flags ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Rob Clark]

On Thu, Dec 18, 2014 at 4:22 PM, Daniel Vetter  wrote:
>> > > > > TODO move definition of tokens to drm_fourcc.h?
>> > > >
>> > > > Seems orthogonal imo. Another todo is to add checking to all drivers to
>> > > > reject it if it's not 0 with -EINVAL. Otherwise we have yet another 
>> > > > case
>> > > > of an ioctl with fields that can't actually be used everywhere.
>> > >
>> > > Could we please add the check in core code instead of drivers ?
>> >
>> > Nope since then no driver could ever use that extension. Defeats the point
>> > ;-)
>>
>> Except if we follow the proposal of adding a flag to tell whether a driver
>> supports the extension ;-)
>
> I'm not a terrible big fan of driver flags, mostly because I've seen too
> much of the horrible stuff in dri1. Imo much better to pass everything to
> drivers and help them out with helpers if needed. I might be going
> overboard a bit with my bias against driver flags ;-)

I ventured a little ways down the thought path of adding list of
supported modifier tokens per plane.. and then doing more complete
checks in the core.  But then the question is, what about cases where
some tiling format is only supported for UV but not Y, etc.. it
quickly gets ugly.  I think for now better to let the driver do this
(with a must_be_all_zeros() helper for what I expect will be the
common case initially).  If common patterns emerge, then we refactor
out a better helper..

BR,
-R

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Ville Syrjälä]

On Mon, Dec 15, 2014 at 10:19:47PM +, Daniel Stone wrote:
> Hi,
> 
> On 12 December 2014 at 18:22, Ville Syrjälä  linux.intel.com>
> wrote:
> >
> > On Fri, Dec 12, 2014 at 06:05:49PM +, Daniel Stone wrote:
> > > On 12 December 2014 at 18:00, Ville Syrjälä <
> > ville.syrjala at linux.intel.com>
> > > wrote:
> > > > Well anyone who is serious about quality ought to handle that stuff.
> > > > Or at least make sure both GL/whatever and planes ignore the hints in
> > > > the same way. So if you GL implementation is lax then you anyway need
> > > > to have some driver/hardware specific knowledge to know which way to go
> > > > when using the plane path to get matching output.
> > > >
> > >
> > > Anyone who's serious about quality and is also using GL for video, is not
> > > serious about quality. Or accurate timing.
> >
> > You're too hung up on the "GL" there. It doesn't actually matter what
> > you use to render the video when not using the display hardware. The
> > same problem remains.
> 
> 
> Yes, the problem being that sometimes you want to force the hardware to do
> exactly what you want and literally nothing else, and sometimes you don't
> care.
> 
> I posit that a hint-with-optional-force interface is best, because:
>   - if your hint isn't supported, what do you do? fall back to software?
>   - the number of people who care enough to force it is vanishingly small,
> which is partly shown by how:
>   - it matches GL semantics
>   - not a great deal of hardware supports them
> 
> 
> > > > I was more thinking of some global "I want exactly what I said" kind
> > > > of knob. Maybe as a client cap type of thingy.
> > >
> > > I like the idea of keeping it local to the chroma-siting/range hints,
> > > because it makes it far more clear exactly what it affects.
> >
> > You're not thinking wide enough. We would need to add similar hints
> > to pretty much every property.
> 
> 
> Which other properties do we have right now that drivers treat as optional
> hints?

All the src/dst coordinates for one.

> If the answer involves hypothetical new properties, what are they?

Future ones might involve various color adjustment controls, at least if
we want to attempt to make them hardware agnostic.

Color keying properties might be have the same problem. I realize that
treating color keying properties as hints doesn't really make much sense,
but I'm not sure we have any sane alternative since the specific values
you need to plug into the color keying register is rather hardware
specific, and eg. with dst color keying it often depends on the pixel
format of some other plane.

-- 
Ville Syrjälä
Intel OTC

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Michel Dänzer]

On 12.12.2014 20:27, Daniel Stone wrote:
> On 10 December 2014 at 17:17, Rob Clark  > wrote:
> 
> In DRM/KMS we are lacking a good way to deal with tiled/compressed
> formats.  Especially in the case of dmabuf/prime buffer sharing, where
> we cannot always rely on under-the-hood flags passed to driver specific
> gem-create ioctl to pass around these extra flags.
> 
> The proposal is to add a per-plane format modifier.  This allows to, if
> necessary, use different tiling patters for sub-sampled planes, etc.
> The format modifiers are added at the end of the ioctl struct, so for
> legacy userspace it will be zero padded.
> 
> 
> Cool, thanks a lot for looking at this! My one comment is that we could
> maybe go even further: keep the current modifier as a strict
> pixel-layout modifier (e.g. tiled, compressed - anything that affects
> how you actually determine pixel location), and then support an extra
> (perhaps non-vendor-namespaced) argument for optional pixel
> _interpretation_ modifiers, e.g. the hints in
> EGL_EXT_image_dma_buf_import. V4L2 is starting to properly attack things
> like chroma siting, and being able to specify narrow/wide YUV range is
> pretty important for STB/DTV in particular. And they're actually
> starting to move to KMS, too ...
> 
> It might be useful to make the interpretation modifiers bitmaskable, so
> they can be combined (e.g. wide-range/unclamped YUV | whatever chroma
> siting), but I can't think of a usecase for combining multiple layout
> modifiers (e.g. this tiling | that compression).

I might be misunderstanding what you're referring to, but FWIW: With AMD
GPUs, the compression format and tiling parameters can be chosen
(mostly) independently.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Rob Clark]

On Tue, Dec 16, 2014 at 3:01 AM, Daniel Stone  wrote:
> Hi,
>
> On 16 December 2014 at 03:56, Michel Dänzer  wrote:
>>
>> On 12.12.2014 20:27, Daniel Stone wrote:
>> > It might be useful to make the interpretation modifiers bitmaskable, so
>> > they can be combined (e.g. wide-range/unclamped YUV | whatever chroma
>> > siting), but I can't think of a usecase for combining multiple layout
>> > modifiers (e.g. this tiling | that compression).
>>
>> I might be misunderstanding what you're referring to, but FWIW: With AMD
>> GPUs, the compression format and tiling parameters can be chosen
>> (mostly) independently.
>
>
> Should've been a little more clear. Definitely each vendor will have their
> own combination of modes, but I mostly meant that I don't really see a use
> for VENDOR_AMD(compression_1) | VENDOR_OTHER(tiling_N), or VENDOR_ARM(afbc)
> | VENDOR_SAMSUNG(tiling_12x64). Each vendor's space would be free to use the
> 24 sub-bits as a bitmask, but I don't think there's a need for combining
> tokens from multiple vendor spaces.
>

fwiw, I bumped it up to 64b (but haven't resent yet).. so now there
are 56bits to divide as vendor sees fit between tiling/compression..
56bits ought to be enough for anyone, right? :-P

BR,
-R

> Cheers,
> Daniel

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Stone]

Hi,

On 16 December 2014 at 03:56, Michel Dänzer  wrote:
>
> On 12.12.2014 20:27, Daniel Stone wrote:
> > It might be useful to make the interpretation modifiers bitmaskable, so
> > they can be combined (e.g. wide-range/unclamped YUV | whatever chroma
> > siting), but I can't think of a usecase for combining multiple layout
> > modifiers (e.g. this tiling | that compression).
>
> I might be misunderstanding what you're referring to, but FWIW: With AMD
> GPUs, the compression format and tiling parameters can be chosen
> (mostly) independently.


Should've been a little more clear. Definitely each vendor will have their
own combination of modes, but I mostly meant that I don't really see a use
for VENDOR_AMD(compression_1) | VENDOR_OTHER(tiling_N), or VENDOR_ARM(afbc)
| VENDOR_SAMSUNG(tiling_12x64). Each vendor's space would be free to use
the 24 sub-bits as a bitmask, but I don't think there's a need for
combining tokens from multiple vendor spaces.

Cheers,
Daniel
-- next part --
An HTML attachment was scrubbed...
URL: 

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Stone]

Hi,

On 12 December 2014 at 18:22, Ville Syrjälä 
wrote:
>
> On Fri, Dec 12, 2014 at 06:05:49PM +, Daniel Stone wrote:
> > On 12 December 2014 at 18:00, Ville Syrjälä <
> ville.syrjala at linux.intel.com>
> > wrote:
> > > Well anyone who is serious about quality ought to handle that stuff.
> > > Or at least make sure both GL/whatever and planes ignore the hints in
> > > the same way. So if you GL implementation is lax then you anyway need
> > > to have some driver/hardware specific knowledge to know which way to go
> > > when using the plane path to get matching output.
> > >
> >
> > Anyone who's serious about quality and is also using GL for video, is not
> > serious about quality. Or accurate timing.
>
> You're too hung up on the "GL" there. It doesn't actually matter what
> you use to render the video when not using the display hardware. The
> same problem remains.


Yes, the problem being that sometimes you want to force the hardware to do
exactly what you want and literally nothing else, and sometimes you don't
care.

I posit that a hint-with-optional-force interface is best, because:
  - if your hint isn't supported, what do you do? fall back to software?
  - the number of people who care enough to force it is vanishingly small,
which is partly shown by how:
  - it matches GL semantics
  - not a great deal of hardware supports them


> > > I was more thinking of some global "I want exactly what I said" kind
> > > of knob. Maybe as a client cap type of thingy.
> >
> > I like the idea of keeping it local to the chroma-siting/range hints,
> > because it makes it far more clear exactly what it affects.
>
> You're not thinking wide enough. We would need to add similar hints
> to pretty much every property.


Which other properties do we have right now that drivers treat as optional
hints? If the answer involves hypothetical new properties, what are they?

Cheers,
Daniel
-- next part --
An HTML attachment was scrubbed...
URL: 

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Vetter]

On Fri, Dec 12, 2014 at 12:05:41PM -0500, Rob Clark wrote:
> On Fri, Dec 12, 2014 at 11:14 AM, Ville Syrjälä
>  wrote:
> > On Fri, Dec 12, 2014 at 11:00:29AM -0500, Rob Clark wrote:
> >> On Fri, Dec 12, 2014 at 10:30 AM, Ville Syrjälä
> >>  wrote:
> >> >>
> >> >> Having them separated is still kinda nice though, for the same 
> >> >> rationale as
> >> >> the EGLImage import extension having them as hints. If your hardware
> >> >> doesn't support the tiling/compression format you use, then you're 
> >> >> going to
> >> >> be showing absolute garbage. But if it doesn't support your exact
> >> >> chroma-siting or YUV range request, it'll still be totally viewable, 
> >> >> just
> >> >> not entirely perfect. So I don't see the logic in failing these.
> >> >
> >> > Well, it will look nasty when switching between GL and display
> >> > composition the GL path does the right thing an display path doesn't/
> >> > And we already have that problem with the fuzzy alignment/scaling
> >> > restriction stuff. So I think we will want some kind of strict flag
> >> > somewhere to allow the user to specify that they'd rather fail the whole
> >> > thing and fall back to GL rather than annoy the user.
> >>
> >>
> >> another argument in favor of plane properties, I think.  This way
> >> userspace can query what is actually possibly and we don't implicitly
> >> give userspace the idea that display hw can handle something that it
> >> doesn't..
> >
> > Well, we don't have properties to describe a lot of the limitations. I'm
> > not sure we want to add tons of read-only properties for that. And as
> > stated, sometimes the limitations depend on other properties/pixel
> > format/etc. so seems rather hard to describe in a sane way that would
> > actually be useful to userspace.
> 
> sorry, wasn't quite what I meant..  What I meant was that YUV range
> and siting properties would probably be enum properties, so userspace
> could see which enum values are supported.
> 
> r/o props could be a way to deal w/ some limits.  Other limits, it
> could just be a matter of expressing the correct range as we convert
> things to properties for atomic.

There's still the problem that yuv setting might not work on all formats,
maybe it only works on the planar ones.

> > One idea that came up again just yesterday would be to have the kernel
> > assign the planes on behalf of userspace. But that would then mean we
> > need some kind of virtual plane layer on top so that the virtual plane
> > state gets tracked correctly, or userspace would need to pass in the
> > entire state for every display update. Also soon it may start to look
> > like we're implementing some kind of compositor in the kernel. Another
> > other approach might be to implement this plane assignment stuff in
> > libdrm and duplicate some hw specific knowledge there.
> 
> I kinda lean towards userspace.  I don't want to preclude the case of
> a smart userspace (which has some driver specific userspace piece)..
> could be an interesting idea to have something in libdrm.

I expect this to happen sooner or later, probably a descendant of hwc (with
all the assumtpions about single-crtc fixed and maybe some provisions to
allow flips faster than vblanks).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Vetter]

On Fri, Dec 12, 2014 at 10:56:21AM -0500, Rob Clark wrote:
> On Fri, Dec 12, 2014 at 10:11 AM, Daniel Stone  
> wrote:
> > Hi,
> >
> > On 12 December 2014 at 14:56, Ville Syrjälä  > linux.intel.com>
> > wrote:
> >>
> >> On Fri, Dec 12, 2014 at 08:50:18AM -0500, Rob Clark wrote:
> >>
> >> > It does make me briefly think of just letting us set properties on fb
> >>
> >> > objects :-P (but maybe that is a bit overkill)
> >>
> >> Yeah I had the same idea at some point. But then I decided that we could
> >> just have these as properties on the plane.
> >
> >
> > Mm, it does seem a bit weird. Yes, they are relative to how the plane
> > interprets things, but then again so is the format surely. Not to mention
> > another thing to go wrong, if someone forgets to set and/or clear it when
> > changing the plane. Keeping it in the fb eliminates that possibility.
> >
> 
> yeah.. logically it seems nicer for them to be prop's on fb's.  The
> drawback is having to invent some bit of infrastructure to support
> that.  Avoidance of inheriting someone else's plane prop's might be
> enough justification to invent that infrastructure.  But fb prop's
> don't really help w/ the whole not-all-planes-are-the-same thing..

The nice thing with fbs currently is that all the metadata is invariant.
Imo we should keep that for any prop extension, since it massively
simplifies things for everyone.

But I'm not sure that's so important since we already have (and probably
always will have) a mess between plane and fb props. E.g. the rotation
thing actually affects the pte ordering on skl. So would be nice to have
as an invariant fb prop, but since it's now on the plane we can't change
it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Vetter]

On Fri, Dec 12, 2014 at 10:56:53PM +0200, Laurent Pinchart wrote:
> On Wednesday 10 December 2014 18:30:10 Daniel Vetter wrote:
> > On Wed, Dec 10, 2014 at 12:17:51PM -0500, Rob Clark wrote:
> > > In DRM/KMS we are lacking a good way to deal with tiled/compressed
> > > formats.  Especially in the case of dmabuf/prime buffer sharing, where
> > > we cannot always rely on under-the-hood flags passed to driver specific
> > > gem-create ioctl to pass around these extra flags.
> > > 
> > > The proposal is to add a per-plane format modifier.  This allows to, if
> > > necessary, use different tiling patters for sub-sampled planes, etc.
> > > The format modifiers are added at the end of the ioctl struct, so for
> > > legacy userspace it will be zero padded.
> > > 
> > > TODO how best to deal with assignment of modifier token values?  The
> > > rough idea was to namespace things with an 8bit vendor-id, and then
> > > beyond that it is treated as an opaque value.  But that was a relatively
> > > arbitrary choice.  There are cases where same tiling pattern and/or
> > > compression is supported by various different vendors.  So we should
> > > standardize to use the vendor-id and value of the first one who
> > > documents the format?
> > 
> > 8bits should be enough, will take a while until we have more than 250 gpu
> > drivers in the linux kernel ;-) I'm leaning a bit towards using 64bits
> > though to make sure that there's enough space in the bitfiel to encode
> > substrides and stuff like that, in case anyone needs it. For vendor ids
> > I'd just go with first come and starting at 1 (i.e. rename yours). That
> > way we make it clear that until a patch is merged upstream the id isn't
> > reserved yet. drm-next should be sufficient as registry imo.
> > 
> > > TODO move definition of tokens to drm_fourcc.h?
> > 
> > Seems orthogonal imo. Another todo is to add checking to all drivers to
> > reject it if it's not 0 with -EINVAL. Otherwise we have yet another case
> > of an ioctl with fields that can't actually be used everywhere.
> 
> Could we please add the check in core code instead of drivers ?

Nope since then no driver could ever use that extension. Defeats the point
;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Ville Syrjälä]

On Fri, Dec 12, 2014 at 04:33:38PM -0500, Rob Clark wrote:
> On Fri, Dec 12, 2014 at 3:57 PM, Laurent Pinchart
>  wrote:
> > Hi Rob,
> >
> > On Wednesday 10 December 2014 12:17:51 Rob Clark wrote:
> >> In DRM/KMS we are lacking a good way to deal with tiled/compressed
> >> formats.  Especially in the case of dmabuf/prime buffer sharing, where
> >> we cannot always rely on under-the-hood flags passed to driver specific
> >> gem-create ioctl to pass around these extra flags.
> >>
> >> The proposal is to add a per-plane format modifier.  This allows to, if
> >> necessary, use different tiling patters for sub-sampled planes, etc.
> >> The format modifiers are added at the end of the ioctl struct, so for
> >> legacy userspace it will be zero padded.
> >
> > But it will change the size of the structure, and thus the ioctl value. You
> > can't extend existing structures used in ioctls I'm afraid.
> 
> Actually, that is why it will work.  Old userspace passes smaller
> size, drm_ioctl() zero pads the difference..
> 
> The issue is (potentially) in the other direction (new userspace, old
> kernel) since the old kernel will ignore the new fields.  But that can
> be sorted w/ a cap/query

Or by using up one flag in the ioctl to specify whether the new fields
are valid or not.

> 
> BR,
> -R
> 
> 
> > By the way, is thus calls for an addfb3, I would add reserved fields at the
> > end of the structure to make future extensions possible without a new ioctl.
> >
> >> TODO how best to deal with assignment of modifier token values?  The
> >> rough idea was to namespace things with an 8bit vendor-id, and then
> >> beyond that it is treated as an opaque value.  But that was a relatively
> >> arbitrary choice.  There are cases where same tiling pattern and/or
> >> compression is supported by various different vendors.  So we should
> >> standardize to use the vendor-id and value of the first one who
> >> documents the format?
> >>
> >> TODO move definition of tokens to drm_fourcc.h?
> >>
> >> Signed-off-by: Rob Clark 
> >> ---
> >>  include/uapi/drm/drm_mode.h | 30 ++
> >>  1 file changed, 30 insertions(+)
> >>
> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> index aae71cb..c43648c 100644
> >> --- a/include/uapi/drm/drm_mode.h
> >> +++ b/include/uapi/drm/drm_mode.h
> >> @@ -330,6 +330,28 @@ struct drm_mode_fb_cmd {
> >>
> >>  #define DRM_MODE_FB_INTERLACED   (1<<0) /* for interlaced 
> >> framebuffers */
> >>
> >> +/*
> >> + * Format Modifiers:
> >> + *
> >> + * Format modifiers describe, typically, a re-ordering or modification
> >> + * of the data in a plane of an FB.  This can be used to express tiled/
> >> + * swizzled formats, or compression, or a combination of the two.
> >> + *
> >> + * The upper 8 bits of the format modifier are a vendor-id as assigned
> >> + * below.  The lower 24 bits are assigned as vendor sees fit.
> >> + */
> >> +
> >> +/* Vendor Ids: */
> >> +#define DRM_FOURCC_MOD_VENDOR_SAMSUNG 0x03
> >> +/* ... more */
> >> +
> >> +#define DRM_FOURCC_MOD(vendor, name) (((DRM_FOURCC_MOD_VENDOR_## vendor) 
> >> <<
> >> 24) | val)
> >> +
> >> +/* Modifier values: */
> >> +/* 64x32 macroblock, ie NV12MT: */
> >> +#define DRM_FOURCC_MOD_SAMSUNG_64_32_TILE DRM_FOURCC_MOD(SAMSUNG, 123)
> >> +/* ... more */
> >> +
> >>  struct drm_mode_fb_cmd2 {
> >>   __u32 fb_id;
> >>   __u32 width, height;
> >> @@ -349,10 +371,18 @@ struct drm_mode_fb_cmd2 {
> >>* So it would consist of Y as offsets[0] and UV as
> >>* offsets[1].  Note that offsets[0] will generally
> >>* be 0 (but this is not required).
> >> +  *
> >> +  * To accommodate tiled, compressed, etc formats, a per-plane
> >> +  * modifier can be specified.  The default value of zero
> >> +  * indicates "native" format as specified by the fourcc.
> >> +  * Vendor specific modifier token.  This allows, for example,
> >> +  * different tiling/swizzling pattern on different planes.
> >> +  * See discussion above of DRM_FOURCC_MOD_xxx.
> >>*/
> >>   __u32 handles[4];
> >>   __u32 pitches[4]; /* pitch for each plane */
> >>   __u32 offsets[4]; /* offset of each plane */
> >> + __u32 modifier[4]; /* ie, tiling, compressed (per plane) */
> >>  };
> >>
> >>  #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Laurent Pinchart]

Hi Rob,

On Wednesday 10 December 2014 12:17:51 Rob Clark wrote:
> In DRM/KMS we are lacking a good way to deal with tiled/compressed
> formats.  Especially in the case of dmabuf/prime buffer sharing, where
> we cannot always rely on under-the-hood flags passed to driver specific
> gem-create ioctl to pass around these extra flags.
> 
> The proposal is to add a per-plane format modifier.  This allows to, if
> necessary, use different tiling patters for sub-sampled planes, etc.
> The format modifiers are added at the end of the ioctl struct, so for
> legacy userspace it will be zero padded.

But it will change the size of the structure, and thus the ioctl value. You 
can't extend existing structures used in ioctls I'm afraid.

By the way, is thus calls for an addfb3, I would add reserved fields at the 
end of the structure to make future extensions possible without a new ioctl.

> TODO how best to deal with assignment of modifier token values?  The
> rough idea was to namespace things with an 8bit vendor-id, and then
> beyond that it is treated as an opaque value.  But that was a relatively
> arbitrary choice.  There are cases where same tiling pattern and/or
> compression is supported by various different vendors.  So we should
> standardize to use the vendor-id and value of the first one who
> documents the format?
> 
> TODO move definition of tokens to drm_fourcc.h?
> 
> Signed-off-by: Rob Clark 
> ---
>  include/uapi/drm/drm_mode.h | 30 ++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index aae71cb..c43648c 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -330,6 +330,28 @@ struct drm_mode_fb_cmd {
> 
>  #define DRM_MODE_FB_INTERLACED   (1<<0) /* for interlaced framebuffers */
> 
> +/*
> + * Format Modifiers:
> + *
> + * Format modifiers describe, typically, a re-ordering or modification
> + * of the data in a plane of an FB.  This can be used to express tiled/
> + * swizzled formats, or compression, or a combination of the two.
> + *
> + * The upper 8 bits of the format modifier are a vendor-id as assigned
> + * below.  The lower 24 bits are assigned as vendor sees fit.
> + */
> +
> +/* Vendor Ids: */
> +#define DRM_FOURCC_MOD_VENDOR_SAMSUNG 0x03
> +/* ... more */
> +
> +#define DRM_FOURCC_MOD(vendor, name) (((DRM_FOURCC_MOD_VENDOR_## vendor) <<
> 24) | val)
> +
> +/* Modifier values: */
> +/* 64x32 macroblock, ie NV12MT: */
> +#define DRM_FOURCC_MOD_SAMSUNG_64_32_TILE DRM_FOURCC_MOD(SAMSUNG, 123)
> +/* ... more */
> +
>  struct drm_mode_fb_cmd2 {
>   __u32 fb_id;
>   __u32 width, height;
> @@ -349,10 +371,18 @@ struct drm_mode_fb_cmd2 {
>* So it would consist of Y as offsets[0] and UV as
>* offsets[1].  Note that offsets[0] will generally
>* be 0 (but this is not required).
> +  *
> +  * To accommodate tiled, compressed, etc formats, a per-plane
> +  * modifier can be specified.  The default value of zero
> +  * indicates "native" format as specified by the fourcc.
> +  * Vendor specific modifier token.  This allows, for example,
> +  * different tiling/swizzling pattern on different planes.
> +  * See discussion above of DRM_FOURCC_MOD_xxx.
>*/
>   __u32 handles[4];
>   __u32 pitches[4]; /* pitch for each plane */
>   __u32 offsets[4]; /* offset of each plane */
> + __u32 modifier[4]; /* ie, tiling, compressed (per plane) */
>  };
> 
>  #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01

-- 
Regards,

Laurent Pinchart

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Laurent Pinchart]

On Wednesday 10 December 2014 18:30:10 Daniel Vetter wrote:
> On Wed, Dec 10, 2014 at 12:17:51PM -0500, Rob Clark wrote:
> > In DRM/KMS we are lacking a good way to deal with tiled/compressed
> > formats.  Especially in the case of dmabuf/prime buffer sharing, where
> > we cannot always rely on under-the-hood flags passed to driver specific
> > gem-create ioctl to pass around these extra flags.
> > 
> > The proposal is to add a per-plane format modifier.  This allows to, if
> > necessary, use different tiling patters for sub-sampled planes, etc.
> > The format modifiers are added at the end of the ioctl struct, so for
> > legacy userspace it will be zero padded.
> > 
> > TODO how best to deal with assignment of modifier token values?  The
> > rough idea was to namespace things with an 8bit vendor-id, and then
> > beyond that it is treated as an opaque value.  But that was a relatively
> > arbitrary choice.  There are cases where same tiling pattern and/or
> > compression is supported by various different vendors.  So we should
> > standardize to use the vendor-id and value of the first one who
> > documents the format?
> 
> 8bits should be enough, will take a while until we have more than 250 gpu
> drivers in the linux kernel ;-) I'm leaning a bit towards using 64bits
> though to make sure that there's enough space in the bitfiel to encode
> substrides and stuff like that, in case anyone needs it. For vendor ids
> I'd just go with first come and starting at 1 (i.e. rename yours). That
> way we make it clear that until a patch is merged upstream the id isn't
> reserved yet. drm-next should be sufficient as registry imo.
> 
> > TODO move definition of tokens to drm_fourcc.h?
> 
> Seems orthogonal imo. Another todo is to add checking to all drivers to
> reject it if it's not 0 with -EINVAL. Otherwise we have yet another case
> of an ioctl with fields that can't actually be used everywhere.

Could we please add the check in core code instead of drivers ?

> But yeah I like this and in i915 we definitely need this. skl adds a bunch
> of framebuffer layouts where we need to spec tiling in more detail.
> 
> Overall I like.
> 
> > Signed-off-by: Rob Clark 
> > ---
> > 
> >  include/uapi/drm/drm_mode.h | 30 ++
> >  1 file changed, 30 insertions(+)

-- 
Regards,

Laurent Pinchart

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Ville Syrjälä]

On Fri, Dec 12, 2014 at 06:05:49PM +, Daniel Stone wrote:
> Hi,
> 
> On 12 December 2014 at 18:00, Ville Syrjälä  linux.intel.com>
> wrote:
> >
> > On Fri, Dec 12, 2014 at 05:11:50PM +, Daniel Stone wrote:
> > > If you're doing it through GL, you've already lost. Either you're doing
> > > some magic behind the user's back to bind multi-planar dmabuf-EGLImages
> > to
> > > TEXTURE_2D with RGB sampling, or you're binding to TEXTURE_EXTERNAL_OES,
> > > which forces you to use linear/nearest filtering. Even if you do use
> > > TEXTURE_2D binding, the EGLImage import spec does exactly the same as
> > > what's suggested here, and treats them as hints, which the implementation
> > > can use or ignore. So far I don't know of any implementation which
> > doesn't
> > > ignore them.
> >
> > Well anyone who is serious about quality ought to handle that stuff.
> > Or at least make sure both GL/whatever and planes ignore the hints in
> > the same way. So if you GL implementation is lax then you anyway need
> > to have some driver/hardware specific knowledge to know which way to go
> > when using the plane path to get matching output.
> >
> 
> Anyone who's serious about quality and is also using GL for video, is not
> serious about quality. Or accurate timing.

You're too hung up on the "GL" there. It doesn't actually matter what
you use to render the video when not using the display hardware. The
same problem remains.

> 
> 
> > > > But for some simpler cases like Xv it would seem perfectly OK to use
> > the
> > > > less strict rules. Well, unless someone implements Xv in a way that can
> > > > also transparently switch between display planes and GL/software
> > rendering.
> > >
> > > Well sure, if you absolutely want to ensure it works, you're going to
> > need
> > > some kind of query. Maybe, if the range/chroma-siting ones were part of a
> > > bitmask, you could steal the top bit to mark that the hints are actually
> > > requirements, and to fail if you can't respect the hints.
> >
> > I was more thinking of some global "I want exactly what I said" kind
> > of knob. Maybe as a client cap type of thingy.
> >
> 
> I like the idea of keeping it local to the chroma-siting/range hints,
> because it makes it far more clear exactly what it affects.

You're not thinking wide enough. We would need to add similar hints
to pretty much every property.

-- 
Ville Syrjälä
Intel OTC

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Ville Syrjälä]

On Fri, Dec 12, 2014 at 05:11:50PM +, Daniel Stone wrote:
> Hi,
> 
> On 12 December 2014 at 15:30, Ville Syrjälä  linux.intel.com>
> wrote:
> >
> > On Fri, Dec 12, 2014 at 03:11:02PM +, Daniel Stone wrote:
> > > On 12 December 2014 at 14:56, Ville Syrjälä <
> > ville.syrjala at linux.intel.com>
> > > wrote:
> > > Having them separated is still kinda nice though, for the same rationale
> > as
> > > the EGLImage import extension having them as hints. If your hardware
> > > doesn't support the tiling/compression format you use, then you're going
> > to
> > > be showing absolute garbage. But if it doesn't support your exact
> > > chroma-siting or YUV range request, it'll still be totally viewable, just
> > > not entirely perfect. So I don't see the logic in failing these.
> >
> > Well, it will look nasty when switching between GL and display
> > composition the GL path does the right thing an display path doesn't/
> > And we already have that problem with the fuzzy alignment/scaling
> > restriction stuff. So I think we will want some kind of strict flag
> > somewhere to allow the user to specify that they'd rather fail the whole
> > thing and fall back to GL rather than annoy the user.
> >
> 
> If you're doing it through GL, you've already lost. Either you're doing
> some magic behind the user's back to bind multi-planar dmabuf-EGLImages to
> TEXTURE_2D with RGB sampling, or you're binding to TEXTURE_EXTERNAL_OES,
> which forces you to use linear/nearest filtering. Even if you do use
> TEXTURE_2D binding, the EGLImage import spec does exactly the same as
> what's suggested here, and treats them as hints, which the implementation
> can use or ignore. So far I don't know of any implementation which doesn't
> ignore them.

Well anyone who is serious about quality ought to handle that stuff.
Or at least make sure both GL/whatever and planes ignore the hints in
the same way. So if you GL implementation is lax then you anyway need
to have some driver/hardware specific knowledge to know which way to go
when using the plane path to get matching output.

> 
> FWIW, i965 completely disallows multi-planar EGLImage imports in the first
> place. Mali as shipped on ChromeOS forces you to use TEXTURE_EXTERNAL_OES.
> S ...

Without the strict flag you probably need to patch the kernel too then
to make sure the planes ignore the hint the same way as your GL
implementation. Or vice versa.

> > But for some simpler cases like Xv it would seem perfectly OK to use the
> > less strict rules. Well, unless someone implements Xv in a way that can
> > also transparently switch between display planes and GL/software rendering.
> 
> 
> Well sure, if you absolutely want to ensure it works, you're going to need
> some kind of query. Maybe, if the range/chroma-siting ones were part of a
> bitmask, you could steal the top bit to mark that the hints are actually
> requirements, and to fail if you can't respect the hints.

I was more thinking of some global "I want exactly what I said" kind
of knob. Maybe as a client cap type of thingy.

Another idea I had was to have such a flag in the atomic ioctl. But
it seems impossible to handle that in a sane way unless you require
the caller to specify the entire state every time the ioctl is
called with the flag set.

-- 
Ville Syrjälä
Intel OTC

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Ville Syrjälä]

On Fri, Dec 12, 2014 at 11:00:29AM -0500, Rob Clark wrote:
> On Fri, Dec 12, 2014 at 10:30 AM, Ville Syrjälä
>  wrote:
> >>
> >> Having them separated is still kinda nice though, for the same rationale as
> >> the EGLImage import extension having them as hints. If your hardware
> >> doesn't support the tiling/compression format you use, then you're going to
> >> be showing absolute garbage. But if it doesn't support your exact
> >> chroma-siting or YUV range request, it'll still be totally viewable, just
> >> not entirely perfect. So I don't see the logic in failing these.
> >
> > Well, it will look nasty when switching between GL and display
> > composition the GL path does the right thing an display path doesn't/
> > And we already have that problem with the fuzzy alignment/scaling
> > restriction stuff. So I think we will want some kind of strict flag
> > somewhere to allow the user to specify that they'd rather fail the whole
> > thing and fall back to GL rather than annoy the user.
> 
> 
> another argument in favor of plane properties, I think.  This way
> userspace can query what is actually possibly and we don't implicitly
> give userspace the idea that display hw can handle something that it
> doesn't..

Well, we don't have properties to describe a lot of the limitations. I'm
not sure we want to add tons of read-only properties for that. And as
stated, sometimes the limitations depend on other properties/pixel
format/etc. so seems rather hard to describe in a sane way that would
actually be useful to userspace.

One idea that came up again just yesterday would be to have the kernel
assign the planes on behalf of userspace. But that would then mean we
need some kind of virtual plane layer on top so that the virtual plane
state gets tracked correctly, or userspace would need to pass in the
entire state for every display update. Also soon it may start to look
like we're implementing some kind of compositor in the kernel. Another
other approach might be to implement this plane assignment stuff in
libdrm and duplicate some hw specific knowledge there.

-- 
Ville Syrjälä
Intel OTC

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Stone]

Hi,

On 12 December 2014 at 18:00, Ville Syrjälä 
wrote:
>
> On Fri, Dec 12, 2014 at 05:11:50PM +, Daniel Stone wrote:
> > If you're doing it through GL, you've already lost. Either you're doing
> > some magic behind the user's back to bind multi-planar dmabuf-EGLImages
> to
> > TEXTURE_2D with RGB sampling, or you're binding to TEXTURE_EXTERNAL_OES,
> > which forces you to use linear/nearest filtering. Even if you do use
> > TEXTURE_2D binding, the EGLImage import spec does exactly the same as
> > what's suggested here, and treats them as hints, which the implementation
> > can use or ignore. So far I don't know of any implementation which
> doesn't
> > ignore them.
>
> Well anyone who is serious about quality ought to handle that stuff.
> Or at least make sure both GL/whatever and planes ignore the hints in
> the same way. So if you GL implementation is lax then you anyway need
> to have some driver/hardware specific knowledge to know which way to go
> when using the plane path to get matching output.
>

Anyone who's serious about quality and is also using GL for video, is not
serious about quality. Or accurate timing.


> > > But for some simpler cases like Xv it would seem perfectly OK to use
> the
> > > less strict rules. Well, unless someone implements Xv in a way that can
> > > also transparently switch between display planes and GL/software
> rendering.
> >
> > Well sure, if you absolutely want to ensure it works, you're going to
> need
> > some kind of query. Maybe, if the range/chroma-siting ones were part of a
> > bitmask, you could steal the top bit to mark that the hints are actually
> > requirements, and to fail if you can't respect the hints.
>
> I was more thinking of some global "I want exactly what I said" kind
> of knob. Maybe as a client cap type of thingy.
>

I like the idea of keeping it local to the chroma-siting/range hints,
because it makes it far more clear exactly what it affects.

Cheers,
Daniel
-- next part --
An HTML attachment was scrubbed...
URL: 

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Ville Syrjälä]

On Fri, Dec 12, 2014 at 03:11:02PM +, Daniel Stone wrote:
> Hi,
> 
> On 12 December 2014 at 14:56, Ville Syrjälä  linux.intel.com>
> wrote:
> >
> > On Fri, Dec 12, 2014 at 08:50:18AM -0500, Rob Clark wrote:
> >
> > It does make me briefly think of just letting us set properties on fb
> >
> > objects :-P (but maybe that is a bit overkill)
> >
> > Yeah I had the same idea at some point. But then I decided that we could
> > just have these as properties on the plane.
> 
> 
> Mm, it does seem a bit weird. Yes, they are relative to how the plane
> interprets things, but then again so is the format surely. Not to mention
> another thing to go wrong, if someone forgets to set and/or clear it when
> changing the plane. Keeping it in the fb eliminates that possibility.
> 
> 
> > > I suppose the semi-custom plane property approach is a bit easier to
> > > extend-as-we-go, and we already have a mechanism so userspace can
> > > query about what is actually supported.  But this does feel a bit more
> > > like attributes of the fb.  I'm interested if anyone has particularly
> > > good arguments one way or another.
> >
> > I guess we could have just specified offset/size/stride as part of the
> > fb and let pixel format and such as properties. That would be a fairly
> > natural line IMO since it would be enough data to do a blit, but not
> > enough to actually interpret the pixel data. But we already went beyond
> > that with pixel formats. So I'm not sure how far we want to go.
> >
> > Also all this chroma siting and colorspace stuff definitely runs into
> > hardware specific limitations so having some way to tell userspace what
> > is possible would be nice as you said. Properties seem a decent match for
> > that.
> 
> 
> Yeah, that's a good idea actually, especially since different planes do
> have different capabilities.
> 
> 
> > > > It might be useful to make the interpretation modifiers bitmaskable,
> > so they
> > > > can be combined (e.g. wide-range/unclamped YUV | whatever chroma
> > siting),
> > > > but I can't think of a usecase for combining multiple layout modifiers
> > (e.g.
> > > > this tiling | that compression).
> > >
> > > Yeah, I think the vendor-range part of the token, the vendor would
> > > probably want to define as a bitmask or set of bitfields so that they
> > > could have things like tiled+compressed
> > >
> > > (otoh, if you try to organize it too nicely now, eventually enough hw
> > > generations in the future that scheme will break down.. so maybe a big
> > > switch of #define cases is better than trying to interpret the
> > > modifier token)
> >
> 
> Having them separated is still kinda nice though, for the same rationale as
> the EGLImage import extension having them as hints. If your hardware
> doesn't support the tiling/compression format you use, then you're going to
> be showing absolute garbage. But if it doesn't support your exact
> chroma-siting or YUV range request, it'll still be totally viewable, just
> not entirely perfect. So I don't see the logic in failing these.

Well, it will look nasty when switching between GL and display
composition the GL path does the right thing an display path doesn't/
And we already have that problem with the fuzzy alignment/scaling
restriction stuff. So I think we will want some kind of strict flag
somewhere to allow the user to specify that they'd rather fail the whole
thing and fall back to GL rather than annoy the user.

But for some simpler cases like Xv it would seem perfectly OK to use the
less strict rules. Well, unless someone implements Xv in a way that can
also transparently switch between display planes and GL/software rendering.

> 
> 
> > > >> TODO how best to deal with assignment of modifier token values?  The
> > > >> rough idea was to namespace things with an 8bit vendor-id, and then
> > > >> beyond that it is treated as an opaque value.  But that was a
> > relatively
> > > >> arbitrary choice.  There are cases where same tiling pattern and/or
> > > >> compression is supported by various different vendors.  So we should
> > > >> standardize to use the vendor-id and value of the first one who
> > > >> documents the format?
> > > >
> > > >
> > > > Yeah, I'd second all of danvet's comments here, as well as adding a new
> > > > ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much
> > easier
> > > > for generic userspace.
> > >
> > > I've locally made a few tweaks (64b and move some stuff to
> > drm_fourcc.h)..
> > >
> > > I was kicking around the idea of letting plane specify an array of
> > > supported format modifiers, and adding this to getplane ioctl, as an
> > > alternative to a cap.  That plus wiring up some checking to disallow
> > > addfb2 for a format + modifiers not supported by at least one plane.
> > > Although some hw could only support certain tiling patterns for
> > > certain layers of an fb (ie. luma vs chroma).  So I may scrap that
> > > idea and just go back to cap.
> >
> > Indeed the format 

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Stone]

Hi,

On 12 December 2014 at 15:30, Ville Syrjälä 
wrote:
>
> On Fri, Dec 12, 2014 at 03:11:02PM +, Daniel Stone wrote:
> > On 12 December 2014 at 14:56, Ville Syrjälä <
> ville.syrjala at linux.intel.com>
> > wrote:
> > Having them separated is still kinda nice though, for the same rationale
> as
> > the EGLImage import extension having them as hints. If your hardware
> > doesn't support the tiling/compression format you use, then you're going
> to
> > be showing absolute garbage. But if it doesn't support your exact
> > chroma-siting or YUV range request, it'll still be totally viewable, just
> > not entirely perfect. So I don't see the logic in failing these.
>
> Well, it will look nasty when switching between GL and display
> composition the GL path does the right thing an display path doesn't/
> And we already have that problem with the fuzzy alignment/scaling
> restriction stuff. So I think we will want some kind of strict flag
> somewhere to allow the user to specify that they'd rather fail the whole
> thing and fall back to GL rather than annoy the user.
>

If you're doing it through GL, you've already lost. Either you're doing
some magic behind the user's back to bind multi-planar dmabuf-EGLImages to
TEXTURE_2D with RGB sampling, or you're binding to TEXTURE_EXTERNAL_OES,
which forces you to use linear/nearest filtering. Even if you do use
TEXTURE_2D binding, the EGLImage import spec does exactly the same as
what's suggested here, and treats them as hints, which the implementation
can use or ignore. So far I don't know of any implementation which doesn't
ignore them.

FWIW, i965 completely disallows multi-planar EGLImage imports in the first
place. Mali as shipped on ChromeOS forces you to use TEXTURE_EXTERNAL_OES.
S ...


> But for some simpler cases like Xv it would seem perfectly OK to use the
> less strict rules. Well, unless someone implements Xv in a way that can
> also transparently switch between display planes and GL/software rendering.


Well sure, if you absolutely want to ensure it works, you're going to need
some kind of query. Maybe, if the range/chroma-siting ones were part of a
bitmask, you could steal the top bit to mark that the hints are actually
requirements, and to fail if you can't respect the hints.


> > Well, you don't have to solve literally everything at once. Just having a
> > list of formats which could possibly be supported if you did the right
> > thing, would be a hell of a lot better than punting to userspace, which
> > either a) has to have hardware-specific knowledge in every component
> > (compositor, media library, etc etc), or b) brute-force it. The lack of
> any
> > format query in EGLImage dmabuf import is a serious, serious, serious,
> pain
> > when trying to do generic userspace (e.g. compositor feeds GStreamer a
> list
> > of formats which are supported by the hardware). I get that there are
> > combinations that could fail, but that's true of everything. At least
> > narrowing down the problem space a bit is an enormous help.
>
> We alredy have a list of supported formats. The problem is when specific
> formats impose additonal constraints (eg. more restricted scaling factor
> limits).


Where's the list of supported formats in this proposal? It just adds a
modifier: there's no way to determine which modifiers are supported by a
specific plane, which is what I really need to know. Right now, the only
way is just brute-forcing your way through every single combination until
you find one which succeeds.

Like I said, I completely get that there are going to be
specific/weird/arbitrary restrictions. There already are, such as scaling
factors, maximum one non-primary plane per scanline, global bandwidth
limits, etc etc. Those are not something KMS has ever attempted to solve,
and I'm not suggesting that the modifier mechanism attempts to solve them.

Cheers,
Daniel
-- next part --
An HTML attachment was scrubbed...
URL: 

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Ville Syrjälä]

On Fri, Dec 12, 2014 at 08:50:18AM -0500, Rob Clark wrote:
> On Fri, Dec 12, 2014 at 6:27 AM, Daniel Stone  wrote:
> > Hey,
> >
> > On 10 December 2014 at 17:17, Rob Clark  wrote:
> >>
> >> In DRM/KMS we are lacking a good way to deal with tiled/compressed
> >> formats.  Especially in the case of dmabuf/prime buffer sharing, where
> >> we cannot always rely on under-the-hood flags passed to driver specific
> >> gem-create ioctl to pass around these extra flags.
> >>
> >> The proposal is to add a per-plane format modifier.  This allows to, if
> >> necessary, use different tiling patters for sub-sampled planes, etc.
> >> The format modifiers are added at the end of the ioctl struct, so for
> >> legacy userspace it will be zero padded.
> >
> >
> > Cool, thanks a lot for looking at this! My one comment is that we could
> > maybe go even further: keep the current modifier as a strict pixel-layout
> > modifier (e.g. tiled, compressed - anything that affects how you actually
> > determine pixel location), and then support an extra (perhaps
> > non-vendor-namespaced) argument for optional pixel _interpretation_
> > modifiers, e.g. the hints in EGL_EXT_image_dma_buf_import. V4L2 is starting
> > to properly attack things like chroma siting, and being able to specify
> > narrow/wide YUV range is pretty important for STB/DTV in particular. And
> > they're actually starting to move to KMS, too ...
> 
> Up until now I had sort of thought of things like YUV range as plane
> properties which could be updated (potentially on flip as part of
> atomic ioctl).  But they are also additional metadata about how to
> properly interpret the pixel data contained in the buffer.
> 
> I guess chroma siting and YUV range would at least be one value that
> applies across all the bos/planes of the fb, rather than per plane?

There is the DV case where the chroma is sampled at different points for
Cb and Cr. So we could in theory specify chroma siting per-plane. But it
seems to me that it'd be enough to have it for the entire fb. I had some
ideas posted years ago. Here's [1] one at least.

[1] http://lists.freedesktop.org/archives/dri-devel/2011-November/016379.html

> 
> It does make me briefly think of just letting us set properties on fb
> objects :-P (but maybe that is a bit overkill)

Yeah I had the same idea at some point. But then I decided that we could
just have these as properties on the plane.

> 
> I suppose the semi-custom plane property approach is a bit easier to
> extend-as-we-go, and we already have a mechanism so userspace can
> query about what is actually supported.  But this does feel a bit more
> like attributes of the fb.  I'm interested if anyone has particularly
> good arguments one way or another.

I guess we could have just specified offset/size/stride as part of the
fb and let pixel format and such as properties. That would be a fairly
natural line IMO since it would be enough data to do a blit, but not
enough to actually interpret the pixel data. But we already went beyond
that with pixel formats. So I'm not sure how far we want to go.

Also all this chroma siting and colorspace stuff definitely runs into
hardware specific limitations so having some way to tell userspace what
is possible would be nice as you said. Properties seem a decent match for
that.

> 
> > It might be useful to make the interpretation modifiers bitmaskable, so they
> > can be combined (e.g. wide-range/unclamped YUV | whatever chroma siting),
> > but I can't think of a usecase for combining multiple layout modifiers (e.g.
> > this tiling | that compression).
> >
> 
> Yeah, I think the vendor-range part of the token, the vendor would
> probably want to define as a bitmask or set of bitfields so that they
> could have things like tiled+compressed
> 
> (otoh, if you try to organize it too nicely now, eventually enough hw
> generations in the future that scheme will break down.. so maybe a big
> switch of #define cases is better than trying to interpret the
> modifier token)
> 
> >>
> >> TODO how best to deal with assignment of modifier token values?  The
> >> rough idea was to namespace things with an 8bit vendor-id, and then
> >> beyond that it is treated as an opaque value.  But that was a relatively
> >> arbitrary choice.  There are cases where same tiling pattern and/or
> >> compression is supported by various different vendors.  So we should
> >> standardize to use the vendor-id and value of the first one who
> >> documents the format?
> >
> >
> > Yeah, I'd second all of danvet's comments here, as well as adding a new
> > ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much easier
> > for generic userspace.
> 
> I've locally made a few tweaks (64b and move some stuff to drm_fourcc.h)..
> 
> I was kicking around the idea of letting plane specify an array of
> supported format modifiers, and adding this to getplane ioctl, as an
> alternative to a cap.  That plus wiring up some checking to disallow
> addfb2 for a format + 

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Rob Clark]

On Fri, Dec 12, 2014 at 3:57 PM, Laurent Pinchart
 wrote:
> Hi Rob,
>
> On Wednesday 10 December 2014 12:17:51 Rob Clark wrote:
>> In DRM/KMS we are lacking a good way to deal with tiled/compressed
>> formats.  Especially in the case of dmabuf/prime buffer sharing, where
>> we cannot always rely on under-the-hood flags passed to driver specific
>> gem-create ioctl to pass around these extra flags.
>>
>> The proposal is to add a per-plane format modifier.  This allows to, if
>> necessary, use different tiling patters for sub-sampled planes, etc.
>> The format modifiers are added at the end of the ioctl struct, so for
>> legacy userspace it will be zero padded.
>
> But it will change the size of the structure, and thus the ioctl value. You
> can't extend existing structures used in ioctls I'm afraid.

Actually, that is why it will work.  Old userspace passes smaller
size, drm_ioctl() zero pads the difference..

The issue is (potentially) in the other direction (new userspace, old
kernel) since the old kernel will ignore the new fields.  But that can
be sorted w/ a cap/query

BR,
-R


> By the way, is thus calls for an addfb3, I would add reserved fields at the
> end of the structure to make future extensions possible without a new ioctl.
>
>> TODO how best to deal with assignment of modifier token values?  The
>> rough idea was to namespace things with an 8bit vendor-id, and then
>> beyond that it is treated as an opaque value.  But that was a relatively
>> arbitrary choice.  There are cases where same tiling pattern and/or
>> compression is supported by various different vendors.  So we should
>> standardize to use the vendor-id and value of the first one who
>> documents the format?
>>
>> TODO move definition of tokens to drm_fourcc.h?
>>
>> Signed-off-by: Rob Clark 
>> ---
>>  include/uapi/drm/drm_mode.h | 30 ++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index aae71cb..c43648c 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -330,6 +330,28 @@ struct drm_mode_fb_cmd {
>>
>>  #define DRM_MODE_FB_INTERLACED   (1<<0) /* for interlaced framebuffers 
>> */
>>
>> +/*
>> + * Format Modifiers:
>> + *
>> + * Format modifiers describe, typically, a re-ordering or modification
>> + * of the data in a plane of an FB.  This can be used to express tiled/
>> + * swizzled formats, or compression, or a combination of the two.
>> + *
>> + * The upper 8 bits of the format modifier are a vendor-id as assigned
>> + * below.  The lower 24 bits are assigned as vendor sees fit.
>> + */
>> +
>> +/* Vendor Ids: */
>> +#define DRM_FOURCC_MOD_VENDOR_SAMSUNG 0x03
>> +/* ... more */
>> +
>> +#define DRM_FOURCC_MOD(vendor, name) (((DRM_FOURCC_MOD_VENDOR_## vendor) <<
>> 24) | val)
>> +
>> +/* Modifier values: */
>> +/* 64x32 macroblock, ie NV12MT: */
>> +#define DRM_FOURCC_MOD_SAMSUNG_64_32_TILE DRM_FOURCC_MOD(SAMSUNG, 123)
>> +/* ... more */
>> +
>>  struct drm_mode_fb_cmd2 {
>>   __u32 fb_id;
>>   __u32 width, height;
>> @@ -349,10 +371,18 @@ struct drm_mode_fb_cmd2 {
>>* So it would consist of Y as offsets[0] and UV as
>>* offsets[1].  Note that offsets[0] will generally
>>* be 0 (but this is not required).
>> +  *
>> +  * To accommodate tiled, compressed, etc formats, a per-plane
>> +  * modifier can be specified.  The default value of zero
>> +  * indicates "native" format as specified by the fourcc.
>> +  * Vendor specific modifier token.  This allows, for example,
>> +  * different tiling/swizzling pattern on different planes.
>> +  * See discussion above of DRM_FOURCC_MOD_xxx.
>>*/
>>   __u32 handles[4];
>>   __u32 pitches[4]; /* pitch for each plane */
>>   __u32 offsets[4]; /* offset of each plane */
>> + __u32 modifier[4]; /* ie, tiling, compressed (per plane) */
>>  };
>>
>>  #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01
>
> --
> Regards,
>
> Laurent Pinchart
>

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Stone]

Hi,

On 12 December 2014 at 14:56, Ville Syrjälä 
wrote:
>
> On Fri, Dec 12, 2014 at 08:50:18AM -0500, Rob Clark wrote:
>
> It does make me briefly think of just letting us set properties on fb
>
> objects :-P (but maybe that is a bit overkill)
>
> Yeah I had the same idea at some point. But then I decided that we could
> just have these as properties on the plane.


Mm, it does seem a bit weird. Yes, they are relative to how the plane
interprets things, but then again so is the format surely. Not to mention
another thing to go wrong, if someone forgets to set and/or clear it when
changing the plane. Keeping it in the fb eliminates that possibility.


> > I suppose the semi-custom plane property approach is a bit easier to
> > extend-as-we-go, and we already have a mechanism so userspace can
> > query about what is actually supported.  But this does feel a bit more
> > like attributes of the fb.  I'm interested if anyone has particularly
> > good arguments one way or another.
>
> I guess we could have just specified offset/size/stride as part of the
> fb and let pixel format and such as properties. That would be a fairly
> natural line IMO since it would be enough data to do a blit, but not
> enough to actually interpret the pixel data. But we already went beyond
> that with pixel formats. So I'm not sure how far we want to go.
>
> Also all this chroma siting and colorspace stuff definitely runs into
> hardware specific limitations so having some way to tell userspace what
> is possible would be nice as you said. Properties seem a decent match for
> that.


Yeah, that's a good idea actually, especially since different planes do
have different capabilities.


> > > It might be useful to make the interpretation modifiers bitmaskable,
> so they
> > > can be combined (e.g. wide-range/unclamped YUV | whatever chroma
> siting),
> > > but I can't think of a usecase for combining multiple layout modifiers
> (e.g.
> > > this tiling | that compression).
> >
> > Yeah, I think the vendor-range part of the token, the vendor would
> > probably want to define as a bitmask or set of bitfields so that they
> > could have things like tiled+compressed
> >
> > (otoh, if you try to organize it too nicely now, eventually enough hw
> > generations in the future that scheme will break down.. so maybe a big
> > switch of #define cases is better than trying to interpret the
> > modifier token)
>

Having them separated is still kinda nice though, for the same rationale as
the EGLImage import extension having them as hints. If your hardware
doesn't support the tiling/compression format you use, then you're going to
be showing absolute garbage. But if it doesn't support your exact
chroma-siting or YUV range request, it'll still be totally viewable, just
not entirely perfect. So I don't see the logic in failing these.


> > >> TODO how best to deal with assignment of modifier token values?  The
> > >> rough idea was to namespace things with an 8bit vendor-id, and then
> > >> beyond that it is treated as an opaque value.  But that was a
> relatively
> > >> arbitrary choice.  There are cases where same tiling pattern and/or
> > >> compression is supported by various different vendors.  So we should
> > >> standardize to use the vendor-id and value of the first one who
> > >> documents the format?
> > >
> > >
> > > Yeah, I'd second all of danvet's comments here, as well as adding a new
> > > ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much
> easier
> > > for generic userspace.
> >
> > I've locally made a few tweaks (64b and move some stuff to
> drm_fourcc.h)..
> >
> > I was kicking around the idea of letting plane specify an array of
> > supported format modifiers, and adding this to getplane ioctl, as an
> > alternative to a cap.  That plus wiring up some checking to disallow
> > addfb2 for a format + modifiers not supported by at least one plane.
> > Although some hw could only support certain tiling patterns for
> > certain layers of an fb (ie. luma vs chroma).  So I may scrap that
> > idea and just go back to cap.
>
> Indeed the format specific limitations are problem. Properties can't
> handle that. We'd need to have some kind of caps for each plane+format
> combination if we want to deal with that. But I don't think we can
> still make it handle all the hw limitations, so I'm not sure it's worth
> going down this path.


Well, you don't have to solve literally everything at once. Just having a
list of formats which could possibly be supported if you did the right
thing, would be a hell of a lot better than punting to userspace, which
either a) has to have hardware-specific knowledge in every component
(compositor, media library, etc etc), or b) brute-force it. The lack of any
format query in EGLImage dmabuf import is a serious, serious, serious, pain
when trying to do generic userspace (e.g. compositor feeds GStreamer a list
of formats which are supported by the hardware). I get that there are
combinations 

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Rob Clark]

On Fri, Dec 12, 2014 at 11:14 AM, Ville Syrjälä
 wrote:
> On Fri, Dec 12, 2014 at 11:00:29AM -0500, Rob Clark wrote:
>> On Fri, Dec 12, 2014 at 10:30 AM, Ville Syrjälä
>>  wrote:
>> >>
>> >> Having them separated is still kinda nice though, for the same rationale 
>> >> as
>> >> the EGLImage import extension having them as hints. If your hardware
>> >> doesn't support the tiling/compression format you use, then you're going 
>> >> to
>> >> be showing absolute garbage. But if it doesn't support your exact
>> >> chroma-siting or YUV range request, it'll still be totally viewable, just
>> >> not entirely perfect. So I don't see the logic in failing these.
>> >
>> > Well, it will look nasty when switching between GL and display
>> > composition the GL path does the right thing an display path doesn't/
>> > And we already have that problem with the fuzzy alignment/scaling
>> > restriction stuff. So I think we will want some kind of strict flag
>> > somewhere to allow the user to specify that they'd rather fail the whole
>> > thing and fall back to GL rather than annoy the user.
>>
>>
>> another argument in favor of plane properties, I think.  This way
>> userspace can query what is actually possibly and we don't implicitly
>> give userspace the idea that display hw can handle something that it
>> doesn't..
>
> Well, we don't have properties to describe a lot of the limitations. I'm
> not sure we want to add tons of read-only properties for that. And as
> stated, sometimes the limitations depend on other properties/pixel
> format/etc. so seems rather hard to describe in a sane way that would
> actually be useful to userspace.

sorry, wasn't quite what I meant..  What I meant was that YUV range
and siting properties would probably be enum properties, so userspace
could see which enum values are supported.

r/o props could be a way to deal w/ some limits.  Other limits, it
could just be a matter of expressing the correct range as we convert
things to properties for atomic.

> One idea that came up again just yesterday would be to have the kernel
> assign the planes on behalf of userspace. But that would then mean we
> need some kind of virtual plane layer on top so that the virtual plane
> state gets tracked correctly, or userspace would need to pass in the
> entire state for every display update. Also soon it may start to look
> like we're implementing some kind of compositor in the kernel. Another
> other approach might be to implement this plane assignment stuff in
> libdrm and duplicate some hw specific knowledge there.

I kinda lean towards userspace.  I don't want to preclude the case of
a smart userspace (which has some driver specific userspace piece)..
could be an interesting idea to have something in libdrm.

BR,
-R

> --
> Ville Syrjälä
> Intel OTC

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Stone]

Hey,

On 10 December 2014 at 17:17, Rob Clark  wrote:
>
> In DRM/KMS we are lacking a good way to deal with tiled/compressed
> formats.  Especially in the case of dmabuf/prime buffer sharing, where
> we cannot always rely on under-the-hood flags passed to driver specific
> gem-create ioctl to pass around these extra flags.
>
> The proposal is to add a per-plane format modifier.  This allows to, if
> necessary, use different tiling patters for sub-sampled planes, etc.
> The format modifiers are added at the end of the ioctl struct, so for
> legacy userspace it will be zero padded.
>

Cool, thanks a lot for looking at this! My one comment is that we could
maybe go even further: keep the current modifier as a strict pixel-layout
modifier (e.g. tiled, compressed - anything that affects how you actually
determine pixel location), and then support an extra (perhaps
non-vendor-namespaced) argument for optional pixel _interpretation_
modifiers, e.g. the hints in EGL_EXT_image_dma_buf_import. V4L2 is starting
to properly attack things like chroma siting, and being able to specify
narrow/wide YUV range is pretty important for STB/DTV in particular. And
they're actually starting to move to KMS, too ...

It might be useful to make the interpretation modifiers bitmaskable, so
they can be combined (e.g. wide-range/unclamped YUV | whatever chroma
siting), but I can't think of a usecase for combining multiple layout
modifiers (e.g. this tiling | that compression).


> TODO how best to deal with assignment of modifier token values?  The
> rough idea was to namespace things with an 8bit vendor-id, and then
> beyond that it is treated as an opaque value.  But that was a relatively
> arbitrary choice.  There are cases where same tiling pattern and/or
> compression is supported by various different vendors.  So we should
> standardize to use the vendor-id and value of the first one who
> documents the format?


Yeah, I'd second all of danvet's comments here, as well as adding a new
ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much
easier for generic userspace.

Cheers,
Daniel
-- next part --
An HTML attachment was scrubbed...
URL: 

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Rob Clark]

On Fri, Dec 12, 2014 at 10:30 AM, Ville Syrjälä
 wrote:
>>
>> Having them separated is still kinda nice though, for the same rationale as
>> the EGLImage import extension having them as hints. If your hardware
>> doesn't support the tiling/compression format you use, then you're going to
>> be showing absolute garbage. But if it doesn't support your exact
>> chroma-siting or YUV range request, it'll still be totally viewable, just
>> not entirely perfect. So I don't see the logic in failing these.
>
> Well, it will look nasty when switching between GL and display
> composition the GL path does the right thing an display path doesn't/
> And we already have that problem with the fuzzy alignment/scaling
> restriction stuff. So I think we will want some kind of strict flag
> somewhere to allow the user to specify that they'd rather fail the whole
> thing and fall back to GL rather than annoy the user.


another argument in favor of plane properties, I think.  This way
userspace can query what is actually possibly and we don't implicitly
give userspace the idea that display hw can handle something that it
doesn't..

BR,
-R

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Rob Clark]

On Fri, Dec 12, 2014 at 10:11 AM, Daniel Stone  wrote:
> Hi,
>
> On 12 December 2014 at 14:56, Ville Syrjälä  linux.intel.com>
> wrote:
>>
>> On Fri, Dec 12, 2014 at 08:50:18AM -0500, Rob Clark wrote:
>>
>> > It does make me briefly think of just letting us set properties on fb
>>
>> > objects :-P (but maybe that is a bit overkill)
>>
>> Yeah I had the same idea at some point. But then I decided that we could
>> just have these as properties on the plane.
>
>
> Mm, it does seem a bit weird. Yes, they are relative to how the plane
> interprets things, but then again so is the format surely. Not to mention
> another thing to go wrong, if someone forgets to set and/or clear it when
> changing the plane. Keeping it in the fb eliminates that possibility.
>

yeah.. logically it seems nicer for them to be prop's on fb's.  The
drawback is having to invent some bit of infrastructure to support
that.  Avoidance of inheriting someone else's plane prop's might be
enough justification to invent that infrastructure.  But fb prop's
don't really help w/ the whole not-all-planes-are-the-same thing..

>>
>> > I suppose the semi-custom plane property approach is a bit easier to
>> > extend-as-we-go, and we already have a mechanism so userspace can
>> > query about what is actually supported.  But this does feel a bit more
>> > like attributes of the fb.  I'm interested if anyone has particularly
>> > good arguments one way or another.
>>
>> I guess we could have just specified offset/size/stride as part of the
>> fb and let pixel format and such as properties. That would be a fairly
>> natural line IMO since it would be enough data to do a blit, but not
>> enough to actually interpret the pixel data. But we already went beyond
>> that with pixel formats. So I'm not sure how far we want to go.
>>
>> Also all this chroma siting and colorspace stuff definitely runs into
>> hardware specific limitations so having some way to tell userspace what
>> is possible would be nice as you said. Properties seem a decent match for
>> that.
>
>
> Yeah, that's a good idea actually, especially since different planes do have
> different capabilities.
>
>>
>> > > It might be useful to make the interpretation modifiers bitmaskable,
>> > > so they
>> > > can be combined (e.g. wide-range/unclamped YUV | whatever chroma
>> > > siting),
>> > > but I can't think of a usecase for combining multiple layout modifiers
>> > > (e.g.
>> > > this tiling | that compression).
>> >
>> > Yeah, I think the vendor-range part of the token, the vendor would
>> > probably want to define as a bitmask or set of bitfields so that they
>> > could have things like tiled+compressed
>> >
>> > (otoh, if you try to organize it too nicely now, eventually enough hw
>> > generations in the future that scheme will break down.. so maybe a big
>> > switch of #define cases is better than trying to interpret the
>> > modifier token)
>
>
> Having them separated is still kinda nice though, for the same rationale as
> the EGLImage import extension having them as hints. If your hardware doesn't
> support the tiling/compression format you use, then you're going to be
> showing absolute garbage. But if it doesn't support your exact chroma-siting
> or YUV range request, it'll still be totally viewable, just not entirely
> perfect. So I don't see the logic in failing these.

oh, sorry, I was just referring to the 'modifier token' stuff..
chroma-siting and YUV range are common enough that I think they should
be something separate from the per-plane 'modifer token'

>>
>> > >> TODO how best to deal with assignment of modifier token values?  The
>> > >> rough idea was to namespace things with an 8bit vendor-id, and then
>> > >> beyond that it is treated as an opaque value.  But that was a
>> > >> relatively
>> > >> arbitrary choice.  There are cases where same tiling pattern and/or
>> > >> compression is supported by various different vendors.  So we should
>> > >> standardize to use the vendor-id and value of the first one who
>> > >> documents the format?
>> > >
>> > >
>> > > Yeah, I'd second all of danvet's comments here, as well as adding a
>> > > new
>> > > ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much
>> > > easier
>> > > for generic userspace.
>> >
>> > I've locally made a few tweaks (64b and move some stuff to
>> > drm_fourcc.h)..
>> >
>> > I was kicking around the idea of letting plane specify an array of
>> > supported format modifiers, and adding this to getplane ioctl, as an
>> > alternative to a cap.  That plus wiring up some checking to disallow
>> > addfb2 for a format + modifiers not supported by at least one plane.
>> > Although some hw could only support certain tiling patterns for
>> > certain layers of an fb (ie. luma vs chroma).  So I may scrap that
>> > idea and just go back to cap.
>>
>> Indeed the format specific limitations are problem. Properties can't
>> handle that. We'd need to have some kind of caps for each plane+format
>> combination if 

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Rob Clark]

On Fri, Dec 12, 2014 at 6:27 AM, Daniel Stone  wrote:
> Hey,
>
> On 10 December 2014 at 17:17, Rob Clark  wrote:
>>
>> In DRM/KMS we are lacking a good way to deal with tiled/compressed
>> formats.  Especially in the case of dmabuf/prime buffer sharing, where
>> we cannot always rely on under-the-hood flags passed to driver specific
>> gem-create ioctl to pass around these extra flags.
>>
>> The proposal is to add a per-plane format modifier.  This allows to, if
>> necessary, use different tiling patters for sub-sampled planes, etc.
>> The format modifiers are added at the end of the ioctl struct, so for
>> legacy userspace it will be zero padded.
>
>
> Cool, thanks a lot for looking at this! My one comment is that we could
> maybe go even further: keep the current modifier as a strict pixel-layout
> modifier (e.g. tiled, compressed - anything that affects how you actually
> determine pixel location), and then support an extra (perhaps
> non-vendor-namespaced) argument for optional pixel _interpretation_
> modifiers, e.g. the hints in EGL_EXT_image_dma_buf_import. V4L2 is starting
> to properly attack things like chroma siting, and being able to specify
> narrow/wide YUV range is pretty important for STB/DTV in particular. And
> they're actually starting to move to KMS, too ...

Up until now I had sort of thought of things like YUV range as plane
properties which could be updated (potentially on flip as part of
atomic ioctl).  But they are also additional metadata about how to
properly interpret the pixel data contained in the buffer.

I guess chroma siting and YUV range would at least be one value that
applies across all the bos/planes of the fb, rather than per plane?

It does make me briefly think of just letting us set properties on fb
objects :-P (but maybe that is a bit overkill)

I suppose the semi-custom plane property approach is a bit easier to
extend-as-we-go, and we already have a mechanism so userspace can
query about what is actually supported.  But this does feel a bit more
like attributes of the fb.  I'm interested if anyone has particularly
good arguments one way or another.

> It might be useful to make the interpretation modifiers bitmaskable, so they
> can be combined (e.g. wide-range/unclamped YUV | whatever chroma siting),
> but I can't think of a usecase for combining multiple layout modifiers (e.g.
> this tiling | that compression).
>

Yeah, I think the vendor-range part of the token, the vendor would
probably want to define as a bitmask or set of bitfields so that they
could have things like tiled+compressed

(otoh, if you try to organize it too nicely now, eventually enough hw
generations in the future that scheme will break down.. so maybe a big
switch of #define cases is better than trying to interpret the
modifier token)

>>
>> TODO how best to deal with assignment of modifier token values?  The
>> rough idea was to namespace things with an 8bit vendor-id, and then
>> beyond that it is treated as an opaque value.  But that was a relatively
>> arbitrary choice.  There are cases where same tiling pattern and/or
>> compression is supported by various different vendors.  So we should
>> standardize to use the vendor-id and value of the first one who
>> documents the format?
>
>
> Yeah, I'd second all of danvet's comments here, as well as adding a new
> ADDFB2_MODIFIERS cap + query for supported modifiers. Makes life much easier
> for generic userspace.

I've locally made a few tweaks (64b and move some stuff to drm_fourcc.h)..

I was kicking around the idea of letting plane specify an array of
supported format modifiers, and adding this to getplane ioctl, as an
alternative to a cap.  That plus wiring up some checking to disallow
addfb2 for a format + modifiers not supported by at least one plane.
Although some hw could only support certain tiling patterns for
certain layers of an fb (ie. luma vs chroma).  So I may scrap that
idea and just go back to cap.

BR,
-R

>
> Cheers,
> Daniel

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Daniel Vetter]

On Wed, Dec 10, 2014 at 12:17:51PM -0500, Rob Clark wrote:
> In DRM/KMS we are lacking a good way to deal with tiled/compressed
> formats.  Especially in the case of dmabuf/prime buffer sharing, where
> we cannot always rely on under-the-hood flags passed to driver specific
> gem-create ioctl to pass around these extra flags.
> 
> The proposal is to add a per-plane format modifier.  This allows to, if
> necessary, use different tiling patters for sub-sampled planes, etc.
> The format modifiers are added at the end of the ioctl struct, so for
> legacy userspace it will be zero padded.
> 
> TODO how best to deal with assignment of modifier token values?  The
> rough idea was to namespace things with an 8bit vendor-id, and then
> beyond that it is treated as an opaque value.  But that was a relatively
> arbitrary choice.  There are cases where same tiling pattern and/or
> compression is supported by various different vendors.  So we should
> standardize to use the vendor-id and value of the first one who
> documents the format?

8bits should be enough, will take a while until we have more than 250 gpu
drivers in the linux kernel ;-) I'm leaning a bit towards using 64bits
though to make sure that there's enough space in the bitfiel to encode
substrides and stuff like that, in case anyone needs it. For vendor ids
I'd just go with first come and starting at 1 (i.e. rename yours). That
way we make it clear that until a patch is merged upstream the id isn't
reserved yet. drm-next should be sufficient as registry imo.

> TODO move definition of tokens to drm_fourcc.h?

Seems orthogonal imo. Another todo is to add checking to all drivers to
reject it if it's not 0 with -EINVAL. Otherwise we have yet another case
of an ioctl with fields that can't actually be used everywhere.

But yeah I like this and in i915 we definitely need this. skl adds a bunch
of framebuffer layouts where we need to spec tiling in more detail.

Overall I like.

> Signed-off-by: Rob Clark 
> ---
>  include/uapi/drm/drm_mode.h | 30 ++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index aae71cb..c43648c 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -330,6 +330,28 @@ struct drm_mode_fb_cmd {
>  
>  #define DRM_MODE_FB_INTERLACED   (1<<0) /* for interlaced framebuffers */
>  
> +/*
> + * Format Modifiers:
> + *
> + * Format modifiers describe, typically, a re-ordering or modification
> + * of the data in a plane of an FB.  This can be used to express tiled/
> + * swizzled formats, or compression, or a combination of the two.
> + *
> + * The upper 8 bits of the format modifier are a vendor-id as assigned
> + * below.  The lower 24 bits are assigned as vendor sees fit.
> + */
> +
> +/* Vendor Ids: */
> +#define DRM_FOURCC_MOD_VENDOR_SAMSUNG 0x03
> +/* ... more */
> +
> +#define DRM_FOURCC_MOD(vendor, name) (((DRM_FOURCC_MOD_VENDOR_## vendor) << 
> 24) | val)

Maybe a mask for the lower 24 bits?
-Daniel

> +
> +/* Modifier values: */
> +/* 64x32 macroblock, ie NV12MT: */
> +#define DRM_FOURCC_MOD_SAMSUNG_64_32_TILE DRM_FOURCC_MOD(SAMSUNG, 123)
> +/* ... more */
> +
>  struct drm_mode_fb_cmd2 {
>   __u32 fb_id;
>   __u32 width, height;
> @@ -349,10 +371,18 @@ struct drm_mode_fb_cmd2 {
>* So it would consist of Y as offsets[0] and UV as
>* offsets[1].  Note that offsets[0] will generally
>* be 0 (but this is not required).
> +  *
> +  * To accommodate tiled, compressed, etc formats, a per-plane
> +  * modifier can be specified.  The default value of zero
> +  * indicates "native" format as specified by the fourcc.
> +  * Vendor specific modifier token.  This allows, for example,
> +  * different tiling/swizzling pattern on different planes.
> +  * See discussion above of DRM_FOURCC_MOD_xxx.
>*/
>   __u32 handles[4];
>   __u32 pitches[4]; /* pitch for each plane */
>   __u32 offsets[4]; /* offset of each plane */
> + __u32 modifier[4]; /* ie, tiling, compressed (per plane) */
>  };
>  
>  #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RFC] drm: add support for tiled/compressed/etc modifier in addfb2

[Rob Clark]

In DRM/KMS we are lacking a good way to deal with tiled/compressed
formats.  Especially in the case of dmabuf/prime buffer sharing, where
we cannot always rely on under-the-hood flags passed to driver specific
gem-create ioctl to pass around these extra flags.

The proposal is to add a per-plane format modifier.  This allows to, if
necessary, use different tiling patters for sub-sampled planes, etc.
The format modifiers are added at the end of the ioctl struct, so for
legacy userspace it will be zero padded.

TODO how best to deal with assignment of modifier token values?  The
rough idea was to namespace things with an 8bit vendor-id, and then
beyond that it is treated as an opaque value.  But that was a relatively
arbitrary choice.  There are cases where same tiling pattern and/or
compression is supported by various different vendors.  So we should
standardize to use the vendor-id and value of the first one who
documents the format?

TODO move definition of tokens to drm_fourcc.h?

Signed-off-by: Rob Clark 
---
 include/uapi/drm/drm_mode.h | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index aae71cb..c43648c 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -330,6 +330,28 @@ struct drm_mode_fb_cmd {

 #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */

+/*
+ * Format Modifiers:
+ *
+ * Format modifiers describe, typically, a re-ordering or modification
+ * of the data in a plane of an FB.  This can be used to express tiled/
+ * swizzled formats, or compression, or a combination of the two.
+ *
+ * The upper 8 bits of the format modifier are a vendor-id as assigned
+ * below.  The lower 24 bits are assigned as vendor sees fit.
+ */
+
+/* Vendor Ids: */
+#define DRM_FOURCC_MOD_VENDOR_SAMSUNG 0x03
+/* ... more */
+
+#define DRM_FOURCC_MOD(vendor, name) (((DRM_FOURCC_MOD_VENDOR_## vendor) << 
24) | val)
+
+/* Modifier values: */
+/* 64x32 macroblock, ie NV12MT: */
+#define DRM_FOURCC_MOD_SAMSUNG_64_32_TILE DRM_FOURCC_MOD(SAMSUNG, 123)
+/* ... more */
+
 struct drm_mode_fb_cmd2 {
__u32 fb_id;
__u32 width, height;
@@ -349,10 +371,18 @@ struct drm_mode_fb_cmd2 {
 * So it would consist of Y as offsets[0] and UV as
 * offsets[1].  Note that offsets[0] will generally
 * be 0 (but this is not required).
+*
+* To accommodate tiled, compressed, etc formats, a per-plane
+* modifier can be specified.  The default value of zero
+* indicates "native" format as specified by the fourcc.
+* Vendor specific modifier token.  This allows, for example,
+* different tiling/swizzling pattern on different planes.
+* See discussion above of DRM_FOURCC_MOD_xxx.
 */
__u32 handles[4];
__u32 pitches[4]; /* pitch for each plane */
__u32 offsets[4]; /* offset of each plane */
+   __u32 modifier[4]; /* ie, tiling, compressed (per plane) */
 };

 #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01
-- 
2.1.0