Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Jyri Sarha]

On 05/02/17 11:33, Daniel Vetter wrote:
> On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:
>> Add standard properties to control YCbCr to RGB conversion in DRM
>> planes. The created properties are stored to drm_plane object to allow
>> different set of supported conversion modes for different planes on
>> the device. For simplicity the related property blobs for CSC matrix
>> and YCbCr preoffsets are also stored in the same place. The blob
>> contents are defined in the uapi/drm/drm_mode.h header.
>>
>> Signed-off-by: Jyri Sarha 
> 
> Just to make sure there's no surprises: We need the userspace for this
> too. -modesetting (for Xv maybe), some wayland compositor (weston, ozone,
> whatever you feel like) or drm_hwcomposer.
> 
> But yeah might be good to bikeshed the uabi first a bit more and get at
> least some agreement on that.
> 

In the first phase I have been using kms++ [1]. And when/if we have an
agreement about the API, I will push my patches there.

With X, wayland, and other compositors, we would in the end need some
video player supporting the different YCbCr encodings according to the
video stream. This sounds like a relatively big task, but surely I
volunteer to assist, when we get there.

Cheer,
Jyri

[1] https://github.com/tomba/kmsxx/

> Thanks, Daniel
>> ---
>>  drivers/gpu/drm/drm_atomic.c|  26 +++
>>  drivers/gpu/drm/drm_atomic_helper.c |   9 +++
>>  drivers/gpu/drm/drm_color_mgmt.c| 136 
>> +++-
>>  drivers/gpu/drm/drm_plane.c |  10 +++
>>  include/drm/drm_color_mgmt.h|  23 ++
>>  include/drm/drm_plane.h |  10 +++
>>  include/uapi/drm/drm_mode.h |  12 
>>  7 files changed, 223 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index f881319..d1512aa 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane 
>> *plane,
>>  {
>>  struct drm_device *dev = plane->dev;
>>  struct drm_mode_config *config = >mode_config;
>> +int ret;
>> +bool dummy;
>>  
>>  if (property == config->prop_fb_id) {
>>  struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
>> @@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane 
>> *plane,
>>  state->rotation = val;
>>  } else if (property == plane->zpos_property) {
>>  state->zpos = val;
>> +} else if (property == plane->ycbcr_to_rgb_mode_property) {
>> +state->ycbcr_to_rgb_mode = val;
>> +} else if (property == plane->ycbcr_to_rgb_csc_property) {
>> +ret = drm_atomic_replace_property_blob_from_id(dev,
>> +>ycbcr_to_rgb_csc,
>> +val,
>> +sizeof(struct drm_ycbcr_to_rgb_csc),
>> +);
>> +return ret;
>> +} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
>> +ret = drm_atomic_replace_property_blob_from_id(dev,
>> +>ycbcr_to_rgb_preoffset,
>> +val,
>> +sizeof(struct drm_ycbcr_to_rgb_preoffset),
>> +);
>> +return ret;
>>  } else if (plane->funcs->atomic_set_property) {
>>  return plane->funcs->atomic_set_property(plane, state,
>>  property, val);
>> @@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane 
>> *plane,
>>  *val = state->rotation;
>>  } else if (property == plane->zpos_property) {
>>  *val = state->zpos;
>> +} else if (property == plane->ycbcr_to_rgb_mode_property) {
>> +*val = state->ycbcr_to_rgb_mode;
>> +} else if (property == plane->ycbcr_to_rgb_csc_property) {
>> +*val = state->ycbcr_to_rgb_csc ?
>> +state->ycbcr_to_rgb_csc->base.id : 0;
>> +} else if (property == plane->ycbcr_to_rgb_preoffset_property) {
>> +*val = state->ycbcr_to_rgb_preoffset ?
>> +state->ycbcr_to_rgb_preoffset->base.id : 0;
>>  } else if (plane->funcs->atomic_get_property) {
>>  return plane->funcs->atomic_get_property(plane, state, 
>> property, val);
>>  } else {
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
>> b/drivers/gpu/drm/drm_atomic_helper.c
>> index c3994b4..89fd826 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
>> drm_plane *plane,
>>  {
>>  memcpy(state, plane->state, sizeof(*state));
>>  
>> +if (state->ycbcr_to_rgb_csc)
>> +drm_property_blob_get(state->ycbcr_to_rgb_csc);
>> +
>> +if (state->ycbcr_to_rgb_preoffset)
>> +

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Daniel Vetter]

On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:
> Add standard properties to control YCbCr to RGB conversion in DRM
> planes. The created properties are stored to drm_plane object to allow
> different set of supported conversion modes for different planes on
> the device. For simplicity the related property blobs for CSC matrix
> and YCbCr preoffsets are also stored in the same place. The blob
> contents are defined in the uapi/drm/drm_mode.h header.
> 
> Signed-off-by: Jyri Sarha 

Just to make sure there's no surprises: We need the userspace for this
too. -modesetting (for Xv maybe), some wayland compositor (weston, ozone,
whatever you feel like) or drm_hwcomposer.

But yeah might be good to bikeshed the uabi first a bit more and get at
least some agreement on that.

Thanks, Daniel
> ---
>  drivers/gpu/drm/drm_atomic.c|  26 +++
>  drivers/gpu/drm/drm_atomic_helper.c |   9 +++
>  drivers/gpu/drm/drm_color_mgmt.c| 136 
> +++-
>  drivers/gpu/drm/drm_plane.c |  10 +++
>  include/drm/drm_color_mgmt.h|  23 ++
>  include/drm/drm_plane.h |  10 +++
>  include/uapi/drm/drm_mode.h |  12 
>  7 files changed, 223 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f881319..d1512aa 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  {
>   struct drm_device *dev = plane->dev;
>   struct drm_mode_config *config = >mode_config;
> + int ret;
> + bool dummy;
>  
>   if (property == config->prop_fb_id) {
>   struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> @@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane 
> *plane,
>   state->rotation = val;
>   } else if (property == plane->zpos_property) {
>   state->zpos = val;
> + } else if (property == plane->ycbcr_to_rgb_mode_property) {
> + state->ycbcr_to_rgb_mode = val;
> + } else if (property == plane->ycbcr_to_rgb_csc_property) {
> + ret = drm_atomic_replace_property_blob_from_id(dev,
> + >ycbcr_to_rgb_csc,
> + val,
> + sizeof(struct drm_ycbcr_to_rgb_csc),
> + );
> + return ret;
> + } else if (property == plane->ycbcr_to_rgb_preoffset_property) {
> + ret = drm_atomic_replace_property_blob_from_id(dev,
> + >ycbcr_to_rgb_preoffset,
> + val,
> + sizeof(struct drm_ycbcr_to_rgb_preoffset),
> + );
> + return ret;
>   } else if (plane->funcs->atomic_set_property) {
>   return plane->funcs->atomic_set_property(plane, state,
>   property, val);
> @@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane 
> *plane,
>   *val = state->rotation;
>   } else if (property == plane->zpos_property) {
>   *val = state->zpos;
> + } else if (property == plane->ycbcr_to_rgb_mode_property) {
> + *val = state->ycbcr_to_rgb_mode;
> + } else if (property == plane->ycbcr_to_rgb_csc_property) {
> + *val = state->ycbcr_to_rgb_csc ?
> + state->ycbcr_to_rgb_csc->base.id : 0;
> + } else if (property == plane->ycbcr_to_rgb_preoffset_property) {
> + *val = state->ycbcr_to_rgb_preoffset ?
> + state->ycbcr_to_rgb_preoffset->base.id : 0;
>   } else if (plane->funcs->atomic_get_property) {
>   return plane->funcs->atomic_get_property(plane, state, 
> property, val);
>   } else {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index c3994b4..89fd826 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
> drm_plane *plane,
>  {
>   memcpy(state, plane->state, sizeof(*state));
>  
> + if (state->ycbcr_to_rgb_csc)
> + drm_property_blob_get(state->ycbcr_to_rgb_csc);
> +
> + if (state->ycbcr_to_rgb_preoffset)
> + drm_property_blob_get(state->ycbcr_to_rgb_preoffset);
> +
>   if (state->fb)
>   drm_framebuffer_get(state->fb);
>  
> @@ -3236,6 +3242,9 @@ struct drm_plane_state *
>   */
>  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  {
> + drm_property_blob_put(state->ycbcr_to_rgb_csc);
> + drm_property_blob_put(state->ycbcr_to_rgb_preoffset);
> +
>   if (state->fb)
>   drm_framebuffer_put(state->fb);
>  
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c 
> 

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 03:56:54PM +0300, Jyri Sarha wrote:
> On 04/24/17 19:55, Ville Syrjälä wrote:
> >> In fact we have plane specific YCbCr to RGB CSC (only preoffset
> >> possible), then (per crtc) gamma table, and finally a (per crtc) RGB to
> >> YCbCr CSC with optional post offset (so it can be used either as CSC or
> >> CTM).
> > So with that plane hw you could perhaps do:
> > - YCbCr->RGB if you input is not linear, but then you must
> >   blend using non-linear data
> > - colorspace conversion if your input is alredy linear
> > 
> > And with your crtc hw you could do:
> > - degamma + CTM
> > - gamma + RGB->YCbCr
> 
> 
> Just a generic question. Shouldn't - in an ideal HW - the degamma phase
> and the CTM be a plane specific property?
> 
> I mean, isn't the purpose of normalizing the non linear RGB to linear
> (and possibly converting the color space) to have same format for all
> plane data before blending and composing them together?
> 
> Of course it does not matter if all the planes use the same color space,
> which then should be converted to something else for the output.
> 
> ... or have I misunderstood something?

I think the pipeline we want to expose is

 planes:crtc:
 YCbCr->RGB -> degamma -> CTM -> (gamma) \
 YCbCr->RGB -> degamma -> CTM -> (gamma) -> (degamma) -> CTM -> gamma -> 
RGB->YCbCr
 ... /

I put the plane gamma and crtc degamma in parentheses because ideally
you perhaps wouldn't need them (since you want to blend with linear
data). But we have hardware which has them, and might be lacking some
of the other stages so they can actually be useful. Eg. if you don't
have plane gamma/degamma and you want to use the crtc CTM to change
the colorspace you would also use the crtc degamma.

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Sharma, Shashank]

Regards

Shashank


On 4/26/2017 6:26 PM, Jyri Sarha wrote:

On 04/24/17 19:55, Ville Syrjälä wrote:

In fact we have plane specific YCbCr to RGB CSC (only preoffset
possible), then (per crtc) gamma table, and finally a (per crtc) RGB to
YCbCr CSC with optional post offset (so it can be used either as CSC or
CTM).

So with that plane hw you could perhaps do:
- YCbCr->RGB if you input is not linear, but then you must
   blend using non-linear data
- colorspace conversion if your input is alredy linear

And with your crtc hw you could do:
- degamma + CTM
- gamma + RGB->YCbCr


Just a generic question. Shouldn't - in an ideal HW - the degamma phase
and the CTM be a plane specific property?

I mean, isn't the purpose of normalizing the non linear RGB to linear
(and possibly converting the color space) to have same format for all
plane data before blending and composing them together?

I think Ville's point is something else.
There are two types of color operations possible:
- color space matching so that you can blend the plane data accurately.
- color correction or transformation to enhance display, or change 
output from one format to other format.


Now, when you want to blend, you have to make sure that, you blend among 
the same, that means:

- all the framebuffers should be in one state, either linear or non-linear
- all the framebuffers should be in one color space (REC 709 or 601 or 
2020), else you have to do gamut mapping

- Gamut mapping is possible on Linear data only.

Else, if we try to blend one REC2020 buffer and one REC709 buffer, its 
like adding 2CM + 3MM and making = 5CM


So, we can use the plane level properties in such a way that, you should 
have similar data to the input of the blender (linear Or non-linear)
And then, once blending is done, you can use CRTC level operations for 
color correction and transformation
(Like RGB->YCBCR conversion using CRTC level CSC, for YCBCR420 kind of 
outputs)


Does it helps in explanation ?

- Shashank


Of course it does not matter if all the planes use the same color space,
which then should be converted to something else for the output.

... or have I misunderstood something?

Cheers,
Jyri


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Jyri Sarha]

On 04/24/17 19:55, Ville Syrjälä wrote:
>> In fact we have plane specific YCbCr to RGB CSC (only preoffset
>> possible), then (per crtc) gamma table, and finally a (per crtc) RGB to
>> YCbCr CSC with optional post offset (so it can be used either as CSC or
>> CTM).
> So with that plane hw you could perhaps do:
> - YCbCr->RGB if you input is not linear, but then you must
>   blend using non-linear data
> - colorspace conversion if your input is alredy linear
> 
> And with your crtc hw you could do:
> - degamma + CTM
> - gamma + RGB->YCbCr


Just a generic question. Shouldn't - in an ideal HW - the degamma phase
and the CTM be a plane specific property?

I mean, isn't the purpose of normalizing the non linear RGB to linear
(and possibly converting the color space) to have same format for all
plane data before blending and composing them together?

Of course it does not matter if all the planes use the same color space,
which then should be converted to something else for the output.

... or have I misunderstood something?

Cheers,
Jyri
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Mon, Apr 24, 2017 at 06:49:04PM +0300, Jyri Sarha wrote:
> On 04/24/17 18:13, Ville Syrjälä wrote:
> >> What about the uapi structs? In the patch there is an explicit struct
> >> naming each coefficient for what they are for in YCbCr to RGB
> >> conversion. Is this Ok, or should we go with a generic (CTM style)
> >> matrix, that could be used for RGB to YCbCr conversion too?
> > Not sure what we're talking about here, but like I said I think we
> > should stick to a fairly limited set of standard props and just have
> > each driver map the hardware resources to them as best they can.
> > 
> 
> Just about the implementation detail, if we should have a separate uapi
> struct for YCbCr to RGB CSC and RGB to YCbCr CSC. If we are going to use
> the same struct, then we could as well use the already existing CTM struct.
> 
> > If you just have csc+(de)gamma then I guess it might make sense
> > to just expose the YCbCr->RGB and degamma. If you have
> > degamma+csc+gamma then it might make sense to expose both
> > YCbCr->RGB, degamma, CTM, and gamma, and just refuse any
> > combination that can't be done. Eg. can't do YCbCr->RGB if
> > degamma is used, and YCbCr->RGB + CTM would require multiplying
> > the matrices together which you may or may not want to bother
> > with, I guess we could try to put some matrix math helpers into
> > the core to make such things less painful for drivers?
> 
> In fact we have plane specific YCbCr to RGB CSC (only preoffset
> possible), then (per crtc) gamma table, and finally a (per crtc) RGB to
> YCbCr CSC with optional post offset (so it can be used either as CSC or
> CTM).

So with that plane hw you could perhaps do:
- YCbCr->RGB if you input is not linear, but then you must
  blend using non-linear data
- colorspace conversion if your input is alredy linear

And with your crtc hw you could do:
- degamma + CTM
- gamma + RGB->YCbCr

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Jyri Sarha]

On 04/24/17 18:13, Ville Syrjälä wrote:
>> What about the uapi structs? In the patch there is an explicit struct
>> naming each coefficient for what they are for in YCbCr to RGB
>> conversion. Is this Ok, or should we go with a generic (CTM style)
>> matrix, that could be used for RGB to YCbCr conversion too?
> Not sure what we're talking about here, but like I said I think we
> should stick to a fairly limited set of standard props and just have
> each driver map the hardware resources to them as best they can.
> 

Just about the implementation detail, if we should have a separate uapi
struct for YCbCr to RGB CSC and RGB to YCbCr CSC. If we are going to use
the same struct, then we could as well use the already existing CTM struct.

> If you just have csc+(de)gamma then I guess it might make sense
> to just expose the YCbCr->RGB and degamma. If you have
> degamma+csc+gamma then it might make sense to expose both
> YCbCr->RGB, degamma, CTM, and gamma, and just refuse any
> combination that can't be done. Eg. can't do YCbCr->RGB if
> degamma is used, and YCbCr->RGB + CTM would require multiplying
> the matrices together which you may or may not want to bother
> with, I guess we could try to put some matrix math helpers into
> the core to make such things less painful for drivers?

In fact we have plane specific YCbCr to RGB CSC (only preoffset
possible), then (per crtc) gamma table, and finally a (per crtc) RGB to
YCbCr CSC with optional post offset (so it can be used either as CSC or
CTM).

Cheers,
Jyri
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Mon, Apr 24, 2017 at 04:21:39PM +0300, Jyri Sarha wrote:
> On 04/21/17 16:52, Ville Syrjälä wrote:
> > On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
> >> On 04/21/17 14:17, Ville Syrjälä wrote:
>  +static char *ycbcr_to_rgb_mode_name[] = {
>  +[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>  +"YCbCr BT.601 limited range TO RGB BT.601 full range",
>  +[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>  +"YCbCr BT.601 full range TO RGB BT.601 full range",
>  +[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>  +"YCbCr BT.709 limited range TO RGB BT.709 full range",
>  +[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>  +"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>  +[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>  +"YCbCr BT.601 limited range TO RGB BT.709 full range",
> >>> We probably don't want to conflate the YCbCr->RGB part with the colorspace
> >>> conversion because the YCbCr->RGB part should be performed on gamma 
> >>> encoded
> >>> data and the colorspace conversion on linear data. So we need a degamma
> >>> stage in between. At least that seemed to be the general concencus after
> >>> the last round of mails on this topic.
> >>>
> >>
> >> I do not really have the expertise to argue with that. I merely copied
> >> the idea from the mail thread I referred to in the cover letter.
> >> However, there are several display HWs out there that do not have all
> >> bolts and knobs to make the color-space conversion in exactly the ideal
> >> order, omap DSS being one of them.
> > 
> > Yeah. Intel hardware is in the same boat for the time being. On current
> > hw I think we can only really expose the YCbCr->RGB and degamma stages.
> > 
> > On some limited set of platforms we could expose a blob as well, and I
> > suppose it would then be possible to use it for color space conversion
> > if you ignore gamma and/or only deal with linear RGB data. But it's such
> > a limited subset of hardware for us that I don't think I'm interested
> > in exposing it.
> > 
> > In the future we should be getting a more fully fleged pipeline.
> > 
> 
> If going forward with these patches, then maybe it is better to stick
> with the enum options that we are sure of, e.g. BT.601, BT.709, and
> BT.2020 to full range RGB. It is easy enough to add more enum values in
> the future.
> 
> What is more important is the naming approach, whether we keep the
> conversion mode naming explicit about the output format, or just specify
> the output format implicitly to be always full range (non linear) RGB?
> 
> >>
> >>> After staring at the v4l docs on this stuff I kinda like their
> >>> "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> >>> little partial to calling the prop something like YCBCR_ENCODING. OTOH
> >>
> >> I guess this property should be called YCBCR_DECODING.
> > 
> > Only if you think of it as a verb.
> > 
> 
> No strong opinions here. I am fine with any of the names that have been
> suggested so far.
> 
> >>
> >>> if we want to expose the raw matrix as a blob then maybe calling it a
> >>> CSC might be better. Not sure there's much point in exposing it though.
> >>
> >> In my first version it was called just CSC, but then I wanted to make it
> >> explicit what this CSC was used for to avoid mixing the YCbCr decoding
> >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
> >> of HW that can do only one or the other, e.g. the offset calculations
> >> are supported only to one direction.
> > 
> > Are you planning to do RGB->YCbCr conversion in the plane as well? I
> > think we'll be only doing that at crtc/connector level.
> > 
> 
> No, but we need such a property to CRTC output, and there we certainly
> need to expose the CSC, because we do not have CTM (before gamma)
> matrix, and may have to use the output CSC in its place.

We're sort of in the same boat. We have just one matrix currently, but
the gamma can be either before, after, or both. So mixing CTM and YCbCr
output is a bit of a challenge, especially when gamma is involved.
Shashank has recently posted a patch set to do YCbCr output with i915.

So what we're going to be doing is using our matrix as the output CSC
or CTM as needed. And if the user asks to use both, well, then we get
to either multiply the matrices together (assuming user didn't want
some gamma in between) or we just refuse the entire thing. So I think
better to just expose the standard properties and map the hardware
resources to those dynamically. Otherwise there's going to be too many
ways to do things, and that just leads to confusion for userspace.

> 
> >>
> >>> I don't think most people are in the habit if cooking up new ways to
> >>> encode their pixel data.
> >>>
> >>
> >> In the embedded side I can imagine there could be some custom appliances
> >> where one 

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Jyri Sarha]

On 04/21/17 16:52, Ville Syrjälä wrote:
> On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
>> On 04/21/17 14:17, Ville Syrjälä wrote:
 +static char *ycbcr_to_rgb_mode_name[] = {
 +  [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
 +  "YCbCr BT.601 limited range TO RGB BT.601 full range",
 +  [DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
 +  "YCbCr BT.601 full range TO RGB BT.601 full range",
 +  [DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
 +  "YCbCr BT.709 limited range TO RGB BT.709 full range",
 +  [DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
 +  "YCbCr BT.2020 limited range TO RGB BT.2020 full range",
 +  [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
 +  "YCbCr BT.601 limited range TO RGB BT.709 full range",
>>> We probably don't want to conflate the YCbCr->RGB part with the colorspace
>>> conversion because the YCbCr->RGB part should be performed on gamma encoded
>>> data and the colorspace conversion on linear data. So we need a degamma
>>> stage in between. At least that seemed to be the general concencus after
>>> the last round of mails on this topic.
>>>
>>
>> I do not really have the expertise to argue with that. I merely copied
>> the idea from the mail thread I referred to in the cover letter.
>> However, there are several display HWs out there that do not have all
>> bolts and knobs to make the color-space conversion in exactly the ideal
>> order, omap DSS being one of them.
> 
> Yeah. Intel hardware is in the same boat for the time being. On current
> hw I think we can only really expose the YCbCr->RGB and degamma stages.
> 
> On some limited set of platforms we could expose a blob as well, and I
> suppose it would then be possible to use it for color space conversion
> if you ignore gamma and/or only deal with linear RGB data. But it's such
> a limited subset of hardware for us that I don't think I'm interested
> in exposing it.
> 
> In the future we should be getting a more fully fleged pipeline.
> 

If going forward with these patches, then maybe it is better to stick
with the enum options that we are sure of, e.g. BT.601, BT.709, and
BT.2020 to full range RGB. It is easy enough to add more enum values in
the future.

What is more important is the naming approach, whether we keep the
conversion mode naming explicit about the output format, or just specify
the output format implicitly to be always full range (non linear) RGB?

>>
>>> After staring at the v4l docs on this stuff I kinda like their
>>> "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
>>> little partial to calling the prop something like YCBCR_ENCODING. OTOH
>>
>> I guess this property should be called YCBCR_DECODING.
> 
> Only if you think of it as a verb.
> 

No strong opinions here. I am fine with any of the names that have been
suggested so far.

>>
>>> if we want to expose the raw matrix as a blob then maybe calling it a
>>> CSC might be better. Not sure there's much point in exposing it though.
>>
>> In my first version it was called just CSC, but then I wanted to make it
>> explicit what this CSC was used for to avoid mixing the YCbCr decoding
>> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
>> of HW that can do only one or the other, e.g. the offset calculations
>> are supported only to one direction.
> 
> Are you planning to do RGB->YCbCr conversion in the plane as well? I
> think we'll be only doing that at crtc/connector level.
> 

No, but we need such a property to CRTC output, and there we certainly
need to expose the CSC, because we do not have CTM (before gamma)
matrix, and may have to use the output CSC in its place.

>>
>>> I don't think most people are in the habit if cooking up new ways to
>>> encode their pixel data.
>>>
>>
>> In the embedded side I can imagine there could be some custom appliances
>> where one may want to do some custom thing with the CSC and not needing
>> a custom kernel for that could make a life easier... but then again I am
>> not really an expert in this area.
> 
> I would assume most customy things you'd do in the crtc (eg. color
> correction and whatnot). But could be that I just lack imagination.
> 

I am on thin ice here :), but surely there is no harm in specifying an
optional property for exposing the CSC as many display HWs provide a way
to set an arbitrary CSC.

What about the uapi structs? In the patch there is an explicit struct
naming each coefficient for what they are for in YCbCr to RGB
conversion. Is this Ok, or should we go with a generic (CTM style)
matrix, that could be used for RGB to YCbCr conversion too?

Best regards,
Jyri
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Brian Starkey]

On Fri, Apr 21, 2017 at 07:49:04PM +0300, Ville Syrjälä wrote:

On Fri, Apr 21, 2017 at 04:34:00PM +0100, Brian Starkey wrote:

On Fri, Apr 21, 2017 at 06:21:48PM +0300, Ville Syrjälä wrote:
>On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote:
>> Hi,
>>
>> Thanks for picking this up Jyri.
>>
>> On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote:
>> >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
>> >> On 04/21/17 14:17, Ville Syrjälä wrote:
>> >> >> +static char *ycbcr_to_rgb_mode_name[] = {
>> >> >> +[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>> >> >> +"YCbCr BT.601 limited range TO RGB BT.601 full range",
>> >> >> +[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>> >> >> +"YCbCr BT.601 full range TO RGB BT.601 full range",
>> >> >> +[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>> >> >> +"YCbCr BT.709 limited range TO RGB BT.709 full range",
>> >> >> +[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>> >> >> +"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>> >> >> +[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>> >> >> +"YCbCr BT.601 limited range TO RGB BT.709 full range",
>> >> > We probably don't want to conflate the YCbCr->RGB part with the 
colorspace
>> >> > conversion because the YCbCr->RGB part should be performed on gamma 
encoded
>> >> > data and the colorspace conversion on linear data. So we need a degamma
>> >> > stage in between. At least that seemed to be the general concencus after
>> >> > the last round of mails on this topic.
>> >> >
>>
>> I thought the conclusion of the last round was that on some hardware
>> this would be conflated as a conscious choice. If the HW doesn't have
>> the required degamma->csc->gamma stages, and the implementor/user
>> doesn't care about being a little incorrect, then it can all be
>> described in this single property.
>
>I was proposing the single prop approach initially, but now I think it
>might just lead to more confusion. So a dedicated property for each stage
>is the clearer design I think. We do lose potentially a bit of
>discoverability when not all combinations are supported, but we have
>that problem in many other places as well, so not a big deal I think.
>

Yeah you lose discoverability, but do you also lose the ability to do
the non-perfect, single-stage conversions?


Not sure why one vs. multiple props should matter here. Either you
accept the less than perfect pipeline or you don't. Whether you ask it
via one of multiple props doesn't seem all that different to me.



For HW that only has a matrix, is the driver expected to combine all
of the separated stages down into a single matrix? Or it wouldn't
expose the other properties, only a matrix, and userspace has to come
up with a blob that does the (approximate) right thing?


If you're not happy with exposing just one or the other, then I guess
you would expose both but indicate that there's no degamma in between
and userspace can then choose whether it's happy with that solution or
or not.



I might understand/explain better if we take a concrete example.

Let's assume I have a hardware pipeline with the following bits:

Input -> 3x3 + 3 matrix -> degamma LUT -> CRTC

My input is non-linear YCbCr BT.601 full-range, and I want linear RGB
BT.709, full-range to reach the CRTC.


Which properties would my driver expose, and which values should be
set on them?

My assumption /was/ that:

3x3 + 3 matrix:
   Exposed as
   YCBCR_TO_RGB_MODE = "YCbCr BT.601 full-range to RGB BT.709 full-range"

degamma LUT:
   Exposed as DEGAMMA, set by userspace to an inverse BT.601 gamma
   curve


Are you suggesting instead that:

3x3 + 3 matrix:
   Exposed as
   YCBCR_TO_RGB_MODE = "YCbCr TO RGB CSC full range preoffset"
   with YCBCR_TO_RGB_CSC = 

degamma LUT:
   Exposed as DEGAMMA, set by userspace to an inverse BT.601 gamma
   curve

Thanks,
Brian



>>
>> If there is more capable hardware with the additional stages, then
>> they should expose additional properties (in pipeline order):
>>
>> YCBCR_TO_RGB_*:
>>Does YCbCr->RGB conversion on non-linear YCbCr data,
>>only the enum values which don't have a CSC are exposed.
>
>Not sure what you mean but that last part.
>

Just that the enum list should only contain things that are:
YCbCr BT.601  -> RGB BT.601
YCbCr BT.709  -> RGB BT.709
YcbCr BT.2020 -> RGB BT.2020

and not a those including a color space change, e.g.:
YCbCr BT.601  -> RGB BT.709


Right. We probably shouldn't even have the "BT.whatever" on both sides
since it's not really a colorspace, just the encoding, and once you
decoded the YCbCr into RGB it's just RGB. We could actually just define
the enum values as "BT.601","BT.709" etc.



because an RGB BT.601 -> RGB BT.709 conversion can be performed
later with the RGB_TO_RGB/CTM/whatever property.

>>
>> DEGAMMA:
>>Does the non-linear to linear conversion on the RGB data output from
>>the 

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 04:34:00PM +0100, Brian Starkey wrote:
> On Fri, Apr 21, 2017 at 06:21:48PM +0300, Ville Syrjälä wrote:
> >On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote:
> >> Hi,
> >>
> >> Thanks for picking this up Jyri.
> >>
> >> On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote:
> >> >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
> >> >> On 04/21/17 14:17, Ville Syrjälä wrote:
> >> >> >> +static char *ycbcr_to_rgb_mode_name[] = {
> >> >> >> +[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> >> >> >> +"YCbCr BT.601 limited range TO RGB BT.601 full range",
> >> >> >> +[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> >> >> >> +"YCbCr BT.601 full range TO RGB BT.601 full range",
> >> >> >> +[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> >> >> >> +"YCbCr BT.709 limited range TO RGB BT.709 full range",
> >> >> >> +[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> >> >> >> +"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> >> >> >> +[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> >> >> >> +"YCbCr BT.601 limited range TO RGB BT.709 full range",
> >> >> > We probably don't want to conflate the YCbCr->RGB part with the 
> >> >> > colorspace
> >> >> > conversion because the YCbCr->RGB part should be performed on gamma 
> >> >> > encoded
> >> >> > data and the colorspace conversion on linear data. So we need a 
> >> >> > degamma
> >> >> > stage in between. At least that seemed to be the general concencus 
> >> >> > after
> >> >> > the last round of mails on this topic.
> >> >> >
> >>
> >> I thought the conclusion of the last round was that on some hardware
> >> this would be conflated as a conscious choice. If the HW doesn't have
> >> the required degamma->csc->gamma stages, and the implementor/user
> >> doesn't care about being a little incorrect, then it can all be
> >> described in this single property.
> >
> >I was proposing the single prop approach initially, but now I think it
> >might just lead to more confusion. So a dedicated property for each stage
> >is the clearer design I think. We do lose potentially a bit of
> >discoverability when not all combinations are supported, but we have
> >that problem in many other places as well, so not a big deal I think.
> >
> 
> Yeah you lose discoverability, but do you also lose the ability to do
> the non-perfect, single-stage conversions?

Not sure why one vs. multiple props should matter here. Either you
accept the less than perfect pipeline or you don't. Whether you ask it
via one of multiple props doesn't seem all that different to me.

> 
> For HW that only has a matrix, is the driver expected to combine all
> of the separated stages down into a single matrix? Or it wouldn't
> expose the other properties, only a matrix, and userspace has to come
> up with a blob that does the (approximate) right thing?

If you're not happy with exposing just one or the other, then I guess
you would expose both but indicate that there's no degamma in between
and userspace can then choose whether it's happy with that solution or
or not.

> 
> >>
> >> If there is more capable hardware with the additional stages, then
> >> they should expose additional properties (in pipeline order):
> >>
> >> YCBCR_TO_RGB_*:
> >>Does YCbCr->RGB conversion on non-linear YCbCr data,
> >>only the enum values which don't have a CSC are exposed.
> >
> >Not sure what you mean but that last part.
> >
> 
> Just that the enum list should only contain things that are:
> YCbCr BT.601  -> RGB BT.601
> YCbCr BT.709  -> RGB BT.709
> YcbCr BT.2020 -> RGB BT.2020
> 
> and not a those including a color space change, e.g.:
> YCbCr BT.601  -> RGB BT.709

Right. We probably shouldn't even have the "BT.whatever" on both sides
since it's not really a colorspace, just the encoding, and once you
decoded the YCbCr into RGB it's just RGB. We could actually just define
the enum values as "BT.601","BT.709" etc.

> 
> because an RGB BT.601 -> RGB BT.709 conversion can be performed
> later with the RGB_TO_RGB/CTM/whatever property.
> 
> >>
> >> DEGAMMA:
> >>Does the non-linear to linear conversion on the RGB data output from
> >>the YCBCR_TO_RGB stage.
> >>
> >> RGB_TO_RGB_*:
> >>Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC
> >>on the linear data.
> >
> >We may want to call this just CTM to match the matching crtc prop.
> >
> 
> Good call.
> 
> -Brian
> 
> >>
> >> GAMMA:
> >>Convert the CSC'd RGB data back in to non-linear for blending (if
> >>blending is to be done with non-linear data).
> >>
> >> Drivers can expose as many or as few of the above properties as their
> >> hardware supports.
> >>
> >> >>
> >> >> I do not really have the expertise to argue with that. I merely copied
> >> >> the idea from the mail thread I referred to in the cover letter.
> >> >> However, there are several display HWs out there that do not have all

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Brian Starkey]

On Fri, Apr 21, 2017 at 06:21:48PM +0300, Ville Syrjälä wrote:

On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote:

Hi,

Thanks for picking this up Jyri.

On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote:
>On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
>> On 04/21/17 14:17, Ville Syrjälä wrote:
>> >> +static char *ycbcr_to_rgb_mode_name[] = {
>> >> + [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>> >> + "YCbCr BT.601 limited range TO RGB BT.601 full range",
>> >> + [DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>> >> + "YCbCr BT.601 full range TO RGB BT.601 full range",
>> >> + [DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>> >> + "YCbCr BT.709 limited range TO RGB BT.709 full range",
>> >> + [DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>> >> + "YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>> >> + [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>> >> + "YCbCr BT.601 limited range TO RGB BT.709 full range",
>> > We probably don't want to conflate the YCbCr->RGB part with the colorspace
>> > conversion because the YCbCr->RGB part should be performed on gamma encoded
>> > data and the colorspace conversion on linear data. So we need a degamma
>> > stage in between. At least that seemed to be the general concencus after
>> > the last round of mails on this topic.
>> >

I thought the conclusion of the last round was that on some hardware
this would be conflated as a conscious choice. If the HW doesn't have
the required degamma->csc->gamma stages, and the implementor/user
doesn't care about being a little incorrect, then it can all be
described in this single property.


I was proposing the single prop approach initially, but now I think it
might just lead to more confusion. So a dedicated property for each stage
is the clearer design I think. We do lose potentially a bit of
discoverability when not all combinations are supported, but we have
that problem in many other places as well, so not a big deal I think.



Yeah you lose discoverability, but do you also lose the ability to do
the non-perfect, single-stage conversions?

For HW that only has a matrix, is the driver expected to combine all
of the separated stages down into a single matrix? Or it wouldn't
expose the other properties, only a matrix, and userspace has to come
up with a blob that does the (approximate) right thing?



If there is more capable hardware with the additional stages, then
they should expose additional properties (in pipeline order):

YCBCR_TO_RGB_*:
   Does YCbCr->RGB conversion on non-linear YCbCr data,
   only the enum values which don't have a CSC are exposed.


Not sure what you mean but that last part.



Just that the enum list should only contain things that are:
YCbCr BT.601  -> RGB BT.601
YCbCr BT.709  -> RGB BT.709
YcbCr BT.2020 -> RGB BT.2020

and not a those including a color space change, e.g.:
YCbCr BT.601  -> RGB BT.709

because an RGB BT.601 -> RGB BT.709 conversion can be performed
later with the RGB_TO_RGB/CTM/whatever property.



DEGAMMA:
   Does the non-linear to linear conversion on the RGB data output from
   the YCBCR_TO_RGB stage.

RGB_TO_RGB_*:
   Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC
   on the linear data.


We may want to call this just CTM to match the matching crtc prop.



Good call.

-Brian



GAMMA:
   Convert the CSC'd RGB data back in to non-linear for blending (if
   blending is to be done with non-linear data).

Drivers can expose as many or as few of the above properties as their
hardware supports.

>>
>> I do not really have the expertise to argue with that. I merely copied
>> the idea from the mail thread I referred to in the cover letter.
>> However, there are several display HWs out there that do not have all
>> bolts and knobs to make the color-space conversion in exactly the ideal
>> order, omap DSS being one of them.
>
>Yeah. Intel hardware is in the same boat for the time being. On current
>hw I think we can only really expose the YCbCr->RGB and degamma stages.
>
>On some limited set of platforms we could expose a blob as well, and I
>suppose it would then be possible to use it for color space conversion
>if you ignore gamma and/or only deal with linear RGB data. But it's such
>a limited subset of hardware for us that I don't think I'm interested
>in exposing it.
>

I'm not sure the custom blob is worth having either. It can easily be
added later if we decide we want it after all.

Thanks,
-Brian

>In the future we should be getting a more fully fleged pipeline.
>
>>
>> > After staring at the v4l docs on this stuff I kinda like their
>> > "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
>> > little partial to calling the prop something like YCBCR_ENCODING. OTOH
>>
>> I guess this property should be called YCBCR_DECODING.
>
>Only if you think of it as a verb.
>
>>
>> > if we want to expose the raw 

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote:
> Hi,
> 
> Thanks for picking this up Jyri.
> 
> On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote:
> >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
> >> On 04/21/17 14:17, Ville Syrjälä wrote:
> >> >> +static char *ycbcr_to_rgb_mode_name[] = {
> >> >> +   [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> >> >> +   "YCbCr BT.601 limited range TO RGB BT.601 full range",
> >> >> +   [DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> >> >> +   "YCbCr BT.601 full range TO RGB BT.601 full range",
> >> >> +   [DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> >> >> +   "YCbCr BT.709 limited range TO RGB BT.709 full range",
> >> >> +   [DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> >> >> +   "YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> >> >> +   [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> >> >> +   "YCbCr BT.601 limited range TO RGB BT.709 full range",
> >> > We probably don't want to conflate the YCbCr->RGB part with the 
> >> > colorspace
> >> > conversion because the YCbCr->RGB part should be performed on gamma 
> >> > encoded
> >> > data and the colorspace conversion on linear data. So we need a degamma
> >> > stage in between. At least that seemed to be the general concencus after
> >> > the last round of mails on this topic.
> >> >
> 
> I thought the conclusion of the last round was that on some hardware
> this would be conflated as a conscious choice. If the HW doesn't have
> the required degamma->csc->gamma stages, and the implementor/user
> doesn't care about being a little incorrect, then it can all be
> described in this single property.

I was proposing the single prop approach initially, but now I think it
might just lead to more confusion. So a dedicated property for each stage
is the clearer design I think. We do lose potentially a bit of
discoverability when not all combinations are supported, but we have
that problem in many other places as well, so not a big deal I think.

> 
> If there is more capable hardware with the additional stages, then
> they should expose additional properties (in pipeline order):
> 
> YCBCR_TO_RGB_*:
>Does YCbCr->RGB conversion on non-linear YCbCr data,
>only the enum values which don't have a CSC are exposed.

Not sure what you mean but that last part.

> 
> DEGAMMA:
>Does the non-linear to linear conversion on the RGB data output from
>the YCBCR_TO_RGB stage.
> 
> RGB_TO_RGB_*:
>Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC
>on the linear data.

We may want to call this just CTM to match the matching crtc prop.

> 
> GAMMA:
>Convert the CSC'd RGB data back in to non-linear for blending (if
>blending is to be done with non-linear data).
> 
> Drivers can expose as many or as few of the above properties as their
> hardware supports.
> 
> >>
> >> I do not really have the expertise to argue with that. I merely copied
> >> the idea from the mail thread I referred to in the cover letter.
> >> However, there are several display HWs out there that do not have all
> >> bolts and knobs to make the color-space conversion in exactly the ideal
> >> order, omap DSS being one of them.
> >
> >Yeah. Intel hardware is in the same boat for the time being. On current
> >hw I think we can only really expose the YCbCr->RGB and degamma stages.
> >
> >On some limited set of platforms we could expose a blob as well, and I
> >suppose it would then be possible to use it for color space conversion
> >if you ignore gamma and/or only deal with linear RGB data. But it's such
> >a limited subset of hardware for us that I don't think I'm interested
> >in exposing it.
> >
> 
> I'm not sure the custom blob is worth having either. It can easily be
> added later if we decide we want it after all.
> 
> Thanks,
> -Brian
> 
> >In the future we should be getting a more fully fleged pipeline.
> >
> >>
> >> > After staring at the v4l docs on this stuff I kinda like their
> >> > "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> >> > little partial to calling the prop something like YCBCR_ENCODING. OTOH
> >>
> >> I guess this property should be called YCBCR_DECODING.
> >
> >Only if you think of it as a verb.
> >
> >>
> >> > if we want to expose the raw matrix as a blob then maybe calling it a
> >> > CSC might be better. Not sure there's much point in exposing it though.
> >>
> >> In my first version it was called just CSC, but then I wanted to make it
> >> explicit what this CSC was used for to avoid mixing the YCbCr decoding
> >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
> >> of HW that can do only one or the other, e.g. the offset calculations
> >> are supported only to one direction.
> >
> >Are you planning to do RGB->YCbCr conversion in the plane as well? I
> >think we'll be only doing that at crtc/connector level.
> 

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Brian Starkey]

Hi,

Thanks for picking this up Jyri.

On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote:

On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:

On 04/21/17 14:17, Ville Syrjälä wrote:
>> +static char *ycbcr_to_rgb_mode_name[] = {
>> +  [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>> +  "YCbCr BT.601 limited range TO RGB BT.601 full range",
>> +  [DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>> +  "YCbCr BT.601 full range TO RGB BT.601 full range",
>> +  [DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>> +  "YCbCr BT.709 limited range TO RGB BT.709 full range",
>> +  [DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>> +  "YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>> +  [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>> +  "YCbCr BT.601 limited range TO RGB BT.709 full range",
> We probably don't want to conflate the YCbCr->RGB part with the colorspace
> conversion because the YCbCr->RGB part should be performed on gamma encoded
> data and the colorspace conversion on linear data. So we need a degamma
> stage in between. At least that seemed to be the general concencus after
> the last round of mails on this topic.
>


I thought the conclusion of the last round was that on some hardware
this would be conflated as a conscious choice. If the HW doesn't have
the required degamma->csc->gamma stages, and the implementor/user
doesn't care about being a little incorrect, then it can all be
described in this single property.

If there is more capable hardware with the additional stages, then
they should expose additional properties (in pipeline order):

YCBCR_TO_RGB_*:
  Does YCbCr->RGB conversion on non-linear YCbCr data,
  only the enum values which don't have a CSC are exposed.

DEGAMMA:
  Does the non-linear to linear conversion on the RGB data output from
  the YCBCR_TO_RGB stage.

RGB_TO_RGB_*:
  Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC
  on the linear data.

GAMMA:
  Convert the CSC'd RGB data back in to non-linear for blending (if
  blending is to be done with non-linear data).

Drivers can expose as many or as few of the above properties as their
hardware supports.



I do not really have the expertise to argue with that. I merely copied
the idea from the mail thread I referred to in the cover letter.
However, there are several display HWs out there that do not have all
bolts and knobs to make the color-space conversion in exactly the ideal
order, omap DSS being one of them.


Yeah. Intel hardware is in the same boat for the time being. On current
hw I think we can only really expose the YCbCr->RGB and degamma stages.

On some limited set of platforms we could expose a blob as well, and I
suppose it would then be possible to use it for color space conversion
if you ignore gamma and/or only deal with linear RGB data. But it's such
a limited subset of hardware for us that I don't think I'm interested
in exposing it.



I'm not sure the custom blob is worth having either. It can easily be
added later if we decide we want it after all.

Thanks,
-Brian


In the future we should be getting a more fully fleged pipeline.



> After staring at the v4l docs on this stuff I kinda like their
> "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> little partial to calling the prop something like YCBCR_ENCODING. OTOH

I guess this property should be called YCBCR_DECODING.


Only if you think of it as a verb.



> if we want to expose the raw matrix as a blob then maybe calling it a
> CSC might be better. Not sure there's much point in exposing it though.

In my first version it was called just CSC, but then I wanted to make it
explicit what this CSC was used for to avoid mixing the YCbCr decoding
matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
of HW that can do only one or the other, e.g. the offset calculations
are supported only to one direction.


Are you planning to do RGB->YCbCr conversion in the plane as well? I
think we'll be only doing that at crtc/connector level.



> I don't think most people are in the habit if cooking up new ways to
> encode their pixel data.
>

In the embedded side I can imagine there could be some custom appliances
where one may want to do some custom thing with the CSC and not needing
a custom kernel for that could make a life easier... but then again I am
not really an expert in this area.


I would assume most customy things you'd do in the crtc (eg. color
correction and whatnot). But could be that I just lack imagination.

--
Ville Syrjälä
Intel OTC

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
> On 04/21/17 14:17, Ville Syrjälä wrote:
> >> +static char *ycbcr_to_rgb_mode_name[] = {
> >> +  [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> >> +  "YCbCr BT.601 limited range TO RGB BT.601 full range",
> >> +  [DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> >> +  "YCbCr BT.601 full range TO RGB BT.601 full range",
> >> +  [DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> >> +  "YCbCr BT.709 limited range TO RGB BT.709 full range",
> >> +  [DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> >> +  "YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> >> +  [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> >> +  "YCbCr BT.601 limited range TO RGB BT.709 full range",
> > We probably don't want to conflate the YCbCr->RGB part with the colorspace
> > conversion because the YCbCr->RGB part should be performed on gamma encoded
> > data and the colorspace conversion on linear data. So we need a degamma
> > stage in between. At least that seemed to be the general concencus after
> > the last round of mails on this topic.
> > 
> 
> I do not really have the expertise to argue with that. I merely copied
> the idea from the mail thread I referred to in the cover letter.
> However, there are several display HWs out there that do not have all
> bolts and knobs to make the color-space conversion in exactly the ideal
> order, omap DSS being one of them.

Yeah. Intel hardware is in the same boat for the time being. On current
hw I think we can only really expose the YCbCr->RGB and degamma stages.

On some limited set of platforms we could expose a blob as well, and I
suppose it would then be possible to use it for color space conversion
if you ignore gamma and/or only deal with linear RGB data. But it's such
a limited subset of hardware for us that I don't think I'm interested
in exposing it.

In the future we should be getting a more fully fleged pipeline.

> 
> > After staring at the v4l docs on this stuff I kinda like their
> > "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> > little partial to calling the prop something like YCBCR_ENCODING. OTOH
> 
> I guess this property should be called YCBCR_DECODING.

Only if you think of it as a verb.

> 
> > if we want to expose the raw matrix as a blob then maybe calling it a
> > CSC might be better. Not sure there's much point in exposing it though.
> 
> In my first version it was called just CSC, but then I wanted to make it
> explicit what this CSC was used for to avoid mixing the YCbCr decoding
> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
> of HW that can do only one or the other, e.g. the offset calculations
> are supported only to one direction.

Are you planning to do RGB->YCbCr conversion in the plane as well? I
think we'll be only doing that at crtc/connector level.

> 
> > I don't think most people are in the habit if cooking up new ways to
> > encode their pixel data.
> > 
> 
> In the embedded side I can imagine there could be some custom appliances
> where one may want to do some custom thing with the CSC and not needing
> a custom kernel for that could make a life easier... but then again I am
> not really an expert in this area.

I would assume most customy things you'd do in the crtc (eg. color
correction and whatnot). But could be that I just lack imagination.

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Jyri Sarha]

On 04/21/17 14:17, Ville Syrjälä wrote:
>> +static char *ycbcr_to_rgb_mode_name[] = {
>> +[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>> +"YCbCr BT.601 limited range TO RGB BT.601 full range",
>> +[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>> +"YCbCr BT.601 full range TO RGB BT.601 full range",
>> +[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>> +"YCbCr BT.709 limited range TO RGB BT.709 full range",
>> +[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>> +"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>> +[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>> +"YCbCr BT.601 limited range TO RGB BT.709 full range",
> We probably don't want to conflate the YCbCr->RGB part with the colorspace
> conversion because the YCbCr->RGB part should be performed on gamma encoded
> data and the colorspace conversion on linear data. So we need a degamma
> stage in between. At least that seemed to be the general concencus after
> the last round of mails on this topic.
> 

I do not really have the expertise to argue with that. I merely copied
the idea from the mail thread I referred to in the cover letter.
However, there are several display HWs out there that do not have all
bolts and knobs to make the color-space conversion in exactly the ideal
order, omap DSS being one of them.

> After staring at the v4l docs on this stuff I kinda like their
> "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> little partial to calling the prop something like YCBCR_ENCODING. OTOH

I guess this property should be called YCBCR_DECODING.

> if we want to expose the raw matrix as a blob then maybe calling it a
> CSC might be better. Not sure there's much point in exposing it though.

In my first version it was called just CSC, but then I wanted to make it
explicit what this CSC was used for to avoid mixing the YCbCr decoding
matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
of HW that can do only one or the other, e.g. the offset calculations
are supported only to one direction.

> I don't think most people are in the habit if cooking up new ways to
> encode their pixel data.
> 

In the embedded side I can imagine there could be some custom appliances
where one may want to do some custom thing with the CSC and not needing
a custom kernel for that could make a life easier... but then again I am
not really an expert in this area.

Best regards,
Jyri
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 03:10:31PM +0300, Laurent Pinchart wrote:
> Hello,
> 
> CC'ing Hans Verkuil for his knowledge on colorspace.
> 
> On Friday 21 Apr 2017 14:17:56 Ville Syrjälä wrote:
> > On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:
> > > Add standard properties to control YCbCr to RGB conversion in DRM
> > > planes. The created properties are stored to drm_plane object to allow
> > > different set of supported conversion modes for different planes on
> > > the device. For simplicity the related property blobs for CSC matrix
> > > and YCbCr preoffsets are also stored in the same place. The blob
> > > contents are defined in the uapi/drm/drm_mode.h header.
> > > 
> > > Signed-off-by: Jyri Sarha 
> > > ---
> > > 
> > >  drivers/gpu/drm/drm_atomic.c|  26 +++
> > >  drivers/gpu/drm/drm_atomic_helper.c |   9 +++
> > >  drivers/gpu/drm/drm_color_mgmt.c| 136 ++-
> > >  drivers/gpu/drm/drm_plane.c |  10 +++
> > >  include/drm/drm_color_mgmt.h|  23 ++
> > >  include/drm/drm_plane.h |  10 +++
> > >  include/uapi/drm/drm_mode.h |  12 
> > >  7 files changed, 223 insertions(+), 3 deletions(-)
> 
> [snip]
> 
> > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> > > b/drivers/gpu/drm/drm_color_mgmt.c index cc23b9a..badaddd 100644
> > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > @@ -29,9 +29,14 @@
> > >  /**
> > >   * DOC: overview
> > >   *
> > > - * Color management or color space adjustments is supported through a set
> > > of 5
> > > - * properties on the _crtc object. They are set up by calling
> > > - * drm_crtc_enable_color_mgmt().
> > > + * Color management or color space adjustments in CRTCs is supported
> > > + * through a set of 5 properties on the _crtc object. They are set
> > > + * up by calling drm_crtc_enable_color_mgmt().
> > > + *
> > > + * Color space conversions from YCbCr to RGB color space in planes is
> > > + * supporter trough 3 optional properties in _plane object.
> > > + *
> > > + * The _crtc object's properties are:
> > >   *
> > >   * "DEGAMMA_LUT”:
> > >   *   Blob property to set the degamma lookup table (LUT) mapping 
> > > pixel data
> > > @@ -85,6 +90,28 @@
> > >   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
> > >   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp
> > >   with the
> > >   * "GAMMA_LUT" property above.
> > > 
> > > + *
> > > + * The _plane object's properties are:
> > > + *
> > > + * "YCBCR_TO_RGB_MODE"
> > > + *   Optional plane enum property to configure YCbCr to RGB
> > > + *   conversion. The possible modes include a number of standard
> > > + *   conversions and a possibility to select custom conversion
> > > + *   matrix and preoffset vector. The driver should select the
> > > + *   supported subset of of the modes.
> > > + *
> > > + * "YCBCR_TO_RGB_CSC"
> > > + *   Optional plane property blob to set YCbCr to RGB conversion
> > > + *   matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
> > > + *   defined in uapi/drm/drm_mode.h. Whether this property is
> > > + *   active dependent on YCBCR_TO_RGB_MODE property.
> > > + *
> > > + * "YCBCR_TO_RGB_PREOFFSET"
> > > + *   Optional plane property blob to configure YCbCr offset before
> > > + *   YCbCr to RGB CSC is applied. The blob contains struct
> > > + *   drm_ycbcr_to_rgb_preoffset which is defined in
> > > + *   uapi/drm/drm_mode.h. Whether this property is active dependent
> > > + *   on YCBCR_TO_RGB_MODE property.
> > >   */
> > >  
> > >  /**
> > > @@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> > >   drm_modeset_unlock_all(dev);
> > >   return ret;
> > >  }
> > > +
> > > +static char *ycbcr_to_rgb_mode_name[] = {
> > > + [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> > > + "YCbCr BT.601 limited range TO RGB BT.601 full range",
> > > + [DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> > > + "YCbCr BT.601 full range TO RGB BT.601 full range",
> > > + [DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> > > + "YCbCr BT.709 limited range TO RGB BT.709 full range",
> > > + [DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> > > + "YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> > > + [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> > > + "YCbCr BT.601 limited range TO RGB BT.709 full range",
> > 
> > We probably don't want to conflate the YCbCr->RGB part with the colorspace
> > conversion because the YCbCr->RGB part should be performed on gamma encoded
> > data and the colorspace conversion on linear data. So we need a degamma
> > stage in between. At least that seemed to be the general concencus after
> > the last round of mails on this topic.
> > 
> > After staring at the v4l docs on this stuff I kinda like their
> > "encoding" 

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Sharma, Shashank]

Regards

Shashank


On 4/21/2017 4:47 PM, Ville Syrjälä wrote:

On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:

Add standard properties to control YCbCr to RGB conversion in DRM
planes. The created properties are stored to drm_plane object to allow
different set of supported conversion modes for different planes on
the device. For simplicity the related property blobs for CSC matrix
and YCbCr preoffsets are also stored in the same place. The blob
contents are defined in the uapi/drm/drm_mode.h header.

Signed-off-by: Jyri Sarha 
---
  drivers/gpu/drm/drm_atomic.c|  26 +++
  drivers/gpu/drm/drm_atomic_helper.c |   9 +++
  drivers/gpu/drm/drm_color_mgmt.c| 136 +++-
  drivers/gpu/drm/drm_plane.c |  10 +++
  include/drm/drm_color_mgmt.h|  23 ++
  include/drm/drm_plane.h |  10 +++
  include/uapi/drm/drm_mode.h |  12 
  7 files changed, 223 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f881319..d1512aa 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
  {
struct drm_device *dev = plane->dev;
struct drm_mode_config *config = >mode_config;
+   int ret;
+   bool dummy;
  
  	if (property == config->prop_fb_id) {

struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
@@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
state->rotation = val;
} else if (property == plane->zpos_property) {
state->zpos = val;
+   } else if (property == plane->ycbcr_to_rgb_mode_property) {
+   state->ycbcr_to_rgb_mode = val;
+   } else if (property == plane->ycbcr_to_rgb_csc_property) {
+   ret = drm_atomic_replace_property_blob_from_id(dev,
+   >ycbcr_to_rgb_csc,
+   val,
+   sizeof(struct drm_ycbcr_to_rgb_csc),
+   );
+   return ret;
+   } else if (property == plane->ycbcr_to_rgb_preoffset_property) {
+   ret = drm_atomic_replace_property_blob_from_id(dev,
+   >ycbcr_to_rgb_preoffset,
+   val,
+   sizeof(struct drm_ycbcr_to_rgb_preoffset),
+   );
+   return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
*val = state->rotation;
} else if (property == plane->zpos_property) {
*val = state->zpos;
+   } else if (property == plane->ycbcr_to_rgb_mode_property) {
+   *val = state->ycbcr_to_rgb_mode;
+   } else if (property == plane->ycbcr_to_rgb_csc_property) {
+   *val = state->ycbcr_to_rgb_csc ?
+   state->ycbcr_to_rgb_csc->base.id : 0;
+   } else if (property == plane->ycbcr_to_rgb_preoffset_property) {
+   *val = state->ycbcr_to_rgb_preoffset ?
+   state->ycbcr_to_rgb_preoffset->base.id : 0;
} else if (plane->funcs->atomic_get_property) {
return plane->funcs->atomic_get_property(plane, state, 
property, val);
} else {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index c3994b4..89fd826 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
drm_plane *plane,
  {
memcpy(state, plane->state, sizeof(*state));
  
+	if (state->ycbcr_to_rgb_csc)

+   drm_property_blob_get(state->ycbcr_to_rgb_csc);
+
+   if (state->ycbcr_to_rgb_preoffset)
+   drm_property_blob_get(state->ycbcr_to_rgb_preoffset);
+
if (state->fb)
drm_framebuffer_get(state->fb);
  
@@ -3236,6 +3242,9 @@ struct drm_plane_state *

   */
  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
  {
+   drm_property_blob_put(state->ycbcr_to_rgb_csc);
+   drm_property_blob_put(state->ycbcr_to_rgb_preoffset);
+
if (state->fb)
drm_framebuffer_put(state->fb);
  
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c

index cc23b9a..badaddd 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -29,9 +29,14 @@
  /**
   * DOC: overview
   *
- * Color management or color space adjustments is supported through a set of 5
- * properties on the _crtc object. They are set up by calling
- * drm_crtc_enable_color_mgmt().

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Laurent Pinchart]

Hello,

CC'ing Hans Verkuil for his knowledge on colorspace.

On Friday 21 Apr 2017 14:17:56 Ville Syrjälä wrote:
> On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:
> > Add standard properties to control YCbCr to RGB conversion in DRM
> > planes. The created properties are stored to drm_plane object to allow
> > different set of supported conversion modes for different planes on
> > the device. For simplicity the related property blobs for CSC matrix
> > and YCbCr preoffsets are also stored in the same place. The blob
> > contents are defined in the uapi/drm/drm_mode.h header.
> > 
> > Signed-off-by: Jyri Sarha 
> > ---
> > 
> >  drivers/gpu/drm/drm_atomic.c|  26 +++
> >  drivers/gpu/drm/drm_atomic_helper.c |   9 +++
> >  drivers/gpu/drm/drm_color_mgmt.c| 136 ++-
> >  drivers/gpu/drm/drm_plane.c |  10 +++
> >  include/drm/drm_color_mgmt.h|  23 ++
> >  include/drm/drm_plane.h |  10 +++
> >  include/uapi/drm/drm_mode.h |  12 
> >  7 files changed, 223 insertions(+), 3 deletions(-)

[snip]

> > diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> > b/drivers/gpu/drm/drm_color_mgmt.c index cc23b9a..badaddd 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -29,9 +29,14 @@
> >  /**
> >   * DOC: overview
> >   *
> > - * Color management or color space adjustments is supported through a set
> > of 5
> > - * properties on the _crtc object. They are set up by calling
> > - * drm_crtc_enable_color_mgmt().
> > + * Color management or color space adjustments in CRTCs is supported
> > + * through a set of 5 properties on the _crtc object. They are set
> > + * up by calling drm_crtc_enable_color_mgmt().
> > + *
> > + * Color space conversions from YCbCr to RGB color space in planes is
> > + * supporter trough 3 optional properties in _plane object.
> > + *
> > + * The _crtc object's properties are:
> >   *
> >   * "DEGAMMA_LUT”:
> >   * Blob property to set the degamma lookup table (LUT) mapping pixel data
> > @@ -85,6 +90,28 @@
> >   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
> >   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp
> >   with the
> >   * "GAMMA_LUT" property above.
> > 
> > + *
> > + * The _plane object's properties are:
> > + *
> > + * "YCBCR_TO_RGB_MODE"
> > + * Optional plane enum property to configure YCbCr to RGB
> > + * conversion. The possible modes include a number of standard
> > + * conversions and a possibility to select custom conversion
> > + * matrix and preoffset vector. The driver should select the
> > + * supported subset of of the modes.
> > + *
> > + * "YCBCR_TO_RGB_CSC"
> > + * Optional plane property blob to set YCbCr to RGB conversion
> > + * matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
> > + * defined in uapi/drm/drm_mode.h. Whether this property is
> > + * active dependent on YCBCR_TO_RGB_MODE property.
> > + *
> > + * "YCBCR_TO_RGB_PREOFFSET"
> > + * Optional plane property blob to configure YCbCr offset before
> > + * YCbCr to RGB CSC is applied. The blob contains struct
> > + * drm_ycbcr_to_rgb_preoffset which is defined in
> > + * uapi/drm/drm_mode.h. Whether this property is active dependent
> > + * on YCBCR_TO_RGB_MODE property.
> >   */
> >  
> >  /**
> > @@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> > drm_modeset_unlock_all(dev);
> > return ret;
> >  }
> > +
> > +static char *ycbcr_to_rgb_mode_name[] = {
> > +   [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> > +   "YCbCr BT.601 limited range TO RGB BT.601 full range",
> > +   [DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> > +   "YCbCr BT.601 full range TO RGB BT.601 full range",
> > +   [DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> > +   "YCbCr BT.709 limited range TO RGB BT.709 full range",
> > +   [DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> > +   "YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> > +   [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> > +   "YCbCr BT.601 limited range TO RGB BT.709 full range",
> 
> We probably don't want to conflate the YCbCr->RGB part with the colorspace
> conversion because the YCbCr->RGB part should be performed on gamma encoded
> data and the colorspace conversion on linear data. So we need a degamma
> stage in between. At least that seemed to be the general concencus after
> the last round of mails on this topic.
> 
> After staring at the v4l docs on this stuff I kinda like their
> "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> little partial to calling the prop something like YCBCR_ENCODING.

For quite obvious reasons I agree with this partial reasoning :-) I would also 
copy how V4L2 splits color space information into a transfer function, an 
encoding and a quantization. If you group all 

Re: [PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:
> Add standard properties to control YCbCr to RGB conversion in DRM
> planes. The created properties are stored to drm_plane object to allow
> different set of supported conversion modes for different planes on
> the device. For simplicity the related property blobs for CSC matrix
> and YCbCr preoffsets are also stored in the same place. The blob
> contents are defined in the uapi/drm/drm_mode.h header.
> 
> Signed-off-by: Jyri Sarha 
> ---
>  drivers/gpu/drm/drm_atomic.c|  26 +++
>  drivers/gpu/drm/drm_atomic_helper.c |   9 +++
>  drivers/gpu/drm/drm_color_mgmt.c| 136 
> +++-
>  drivers/gpu/drm/drm_plane.c |  10 +++
>  include/drm/drm_color_mgmt.h|  23 ++
>  include/drm/drm_plane.h |  10 +++
>  include/uapi/drm/drm_mode.h |  12 
>  7 files changed, 223 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index f881319..d1512aa 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  {
>   struct drm_device *dev = plane->dev;
>   struct drm_mode_config *config = >mode_config;
> + int ret;
> + bool dummy;
>  
>   if (property == config->prop_fb_id) {
>   struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> @@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane 
> *plane,
>   state->rotation = val;
>   } else if (property == plane->zpos_property) {
>   state->zpos = val;
> + } else if (property == plane->ycbcr_to_rgb_mode_property) {
> + state->ycbcr_to_rgb_mode = val;
> + } else if (property == plane->ycbcr_to_rgb_csc_property) {
> + ret = drm_atomic_replace_property_blob_from_id(dev,
> + >ycbcr_to_rgb_csc,
> + val,
> + sizeof(struct drm_ycbcr_to_rgb_csc),
> + );
> + return ret;
> + } else if (property == plane->ycbcr_to_rgb_preoffset_property) {
> + ret = drm_atomic_replace_property_blob_from_id(dev,
> + >ycbcr_to_rgb_preoffset,
> + val,
> + sizeof(struct drm_ycbcr_to_rgb_preoffset),
> + );
> + return ret;
>   } else if (plane->funcs->atomic_set_property) {
>   return plane->funcs->atomic_set_property(plane, state,
>   property, val);
> @@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane 
> *plane,
>   *val = state->rotation;
>   } else if (property == plane->zpos_property) {
>   *val = state->zpos;
> + } else if (property == plane->ycbcr_to_rgb_mode_property) {
> + *val = state->ycbcr_to_rgb_mode;
> + } else if (property == plane->ycbcr_to_rgb_csc_property) {
> + *val = state->ycbcr_to_rgb_csc ?
> + state->ycbcr_to_rgb_csc->base.id : 0;
> + } else if (property == plane->ycbcr_to_rgb_preoffset_property) {
> + *val = state->ycbcr_to_rgb_preoffset ?
> + state->ycbcr_to_rgb_preoffset->base.id : 0;
>   } else if (plane->funcs->atomic_get_property) {
>   return plane->funcs->atomic_get_property(plane, state, 
> property, val);
>   } else {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index c3994b4..89fd826 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
> drm_plane *plane,
>  {
>   memcpy(state, plane->state, sizeof(*state));
>  
> + if (state->ycbcr_to_rgb_csc)
> + drm_property_blob_get(state->ycbcr_to_rgb_csc);
> +
> + if (state->ycbcr_to_rgb_preoffset)
> + drm_property_blob_get(state->ycbcr_to_rgb_preoffset);
> +
>   if (state->fb)
>   drm_framebuffer_get(state->fb);
>  
> @@ -3236,6 +3242,9 @@ struct drm_plane_state *
>   */
>  void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  {
> + drm_property_blob_put(state->ycbcr_to_rgb_csc);
> + drm_property_blob_put(state->ycbcr_to_rgb_preoffset);
> +
>   if (state->fb)
>   drm_framebuffer_put(state->fb);
>  
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c 
> b/drivers/gpu/drm/drm_color_mgmt.c
> index cc23b9a..badaddd 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -29,9 +29,14 @@
>  /**
>   * DOC: overview
>   *
> - * Color management or color space adjustments is supported through a set of 
> 5
> - * properties on the _crtc object. They are set 

[PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Jyri Sarha]

Add standard properties to control YCbCr to RGB conversion in DRM
planes. The created properties are stored to drm_plane object to allow
different set of supported conversion modes for different planes on
the device. For simplicity the related property blobs for CSC matrix
and YCbCr preoffsets are also stored in the same place. The blob
contents are defined in the uapi/drm/drm_mode.h header.

Signed-off-by: Jyri Sarha 
---
 drivers/gpu/drm/drm_atomic.c|  26 +++
 drivers/gpu/drm/drm_atomic_helper.c |   9 +++
 drivers/gpu/drm/drm_color_mgmt.c| 136 +++-
 drivers/gpu/drm/drm_plane.c |  10 +++
 include/drm/drm_color_mgmt.h|  23 ++
 include/drm/drm_plane.h |  10 +++
 include/uapi/drm/drm_mode.h |  12 
 7 files changed, 223 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f881319..d1512aa 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
 {
struct drm_device *dev = plane->dev;
struct drm_mode_config *config = >mode_config;
+   int ret;
+   bool dummy;
 
if (property == config->prop_fb_id) {
struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
@@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
state->rotation = val;
} else if (property == plane->zpos_property) {
state->zpos = val;
+   } else if (property == plane->ycbcr_to_rgb_mode_property) {
+   state->ycbcr_to_rgb_mode = val;
+   } else if (property == plane->ycbcr_to_rgb_csc_property) {
+   ret = drm_atomic_replace_property_blob_from_id(dev,
+   >ycbcr_to_rgb_csc,
+   val,
+   sizeof(struct drm_ycbcr_to_rgb_csc),
+   );
+   return ret;
+   } else if (property == plane->ycbcr_to_rgb_preoffset_property) {
+   ret = drm_atomic_replace_property_blob_from_id(dev,
+   >ycbcr_to_rgb_preoffset,
+   val,
+   sizeof(struct drm_ycbcr_to_rgb_preoffset),
+   );
+   return ret;
} else if (plane->funcs->atomic_set_property) {
return plane->funcs->atomic_set_property(plane, state,
property, val);
@@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
*val = state->rotation;
} else if (property == plane->zpos_property) {
*val = state->zpos;
+   } else if (property == plane->ycbcr_to_rgb_mode_property) {
+   *val = state->ycbcr_to_rgb_mode;
+   } else if (property == plane->ycbcr_to_rgb_csc_property) {
+   *val = state->ycbcr_to_rgb_csc ?
+   state->ycbcr_to_rgb_csc->base.id : 0;
+   } else if (property == plane->ycbcr_to_rgb_preoffset_property) {
+   *val = state->ycbcr_to_rgb_preoffset ?
+   state->ycbcr_to_rgb_preoffset->base.id : 0;
} else if (plane->funcs->atomic_get_property) {
return plane->funcs->atomic_get_property(plane, state, 
property, val);
} else {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index c3994b4..89fd826 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct 
drm_plane *plane,
 {
memcpy(state, plane->state, sizeof(*state));
 
+   if (state->ycbcr_to_rgb_csc)
+   drm_property_blob_get(state->ycbcr_to_rgb_csc);
+
+   if (state->ycbcr_to_rgb_preoffset)
+   drm_property_blob_get(state->ycbcr_to_rgb_preoffset);
+
if (state->fb)
drm_framebuffer_get(state->fb);
 
@@ -3236,6 +3242,9 @@ struct drm_plane_state *
  */
 void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 {
+   drm_property_blob_put(state->ycbcr_to_rgb_csc);
+   drm_property_blob_put(state->ycbcr_to_rgb_preoffset);
+
if (state->fb)
drm_framebuffer_put(state->fb);
 
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index cc23b9a..badaddd 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -29,9 +29,14 @@
 /**
  * DOC: overview
  *
- * Color management or color space adjustments is supported through a set of 5
- * properties on the _crtc object. They are set up by calling
- * drm_crtc_enable_color_mgmt().
+ * Color management or color space adjustments in CRTCs is supported
+ * through a set of 5 properties on the _crtc object. They are set
+