[PATCH v2 0/5] drm/omap: add color mgmt support

[PATCH v2 0/5] drm/omap: add color mgmt support

[Tomi Valkeinen]

Hi,

This is a resend of the v1, after adding Pekka's reviewed-by, rebasing
to latest drm-misc, and re-testing. The cover letter from v1:

This series is based on patches sent about a year ago:

https://lists.freedesktop.org/archives/dri-devel/2019-September/233966.html
https://lists.freedesktop.org/archives/dri-devel/2019-September/233967.html

I've fixed the minor issues reported, and according to the recent
discussions with Pekka and Daniel, I have changed how the gamma works.

After these patches omapdrm will expose DEGAMMA_LUT (and no GAMMA_LUT),
and the legacy gamma-set ioctl will use DEGAMMA_LUT underneath. And on
top of that, we have the CTM and plane color mgmt.

 Tomi

Jyri Sarha (2):
  drm/omap: Implement CTM property for CRTC using OVL managers CPR
    matrix
  drm/omap: Enable COLOR_ENCODING and COLOR_RANGE properties for planes

Tomi Valkeinen (3):
  drm: add legacy support for using degamma for gamma
  drm/omap: use degamma property for gamma table
  drm/omap: rearrange includes in omapdss.h

 drivers/gpu/drm/drm_atomic_helper.c   |  81 +++++++++++++++-----
 drivers/gpu/drm/omapdrm/dss/dispc.c   | 104 ++++++++++++++++----------
 drivers/gpu/drm/omapdrm/dss/omapdss.h |  12 ++-
 drivers/gpu/drm/omapdrm/omap_crtc.c   |  51 +++++++++++--
 drivers/gpu/drm/omapdrm/omap_plane.c  |  30 ++++++++
 include/drm/drm_atomic_helper.h       |   4 +
 6 files changed, 209 insertions(+), 73 deletions(-)

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

[PATCH v2 1/5] drm: add legacy support for using degamma for gamma

[Tomi Valkeinen]

We currently have drm_atomic_helper_legacy_gamma_set() helper which can
be used to handle legacy gamma-set ioctl.
drm_atomic_helper_legacy_gamma_set() sets GAMMA_LUT, and clears
CTM and DEGAMMA_LUT. This works fine on HW where we have either:

degamma -> ctm -> gamma -> out

or

ctm -> gamma -> out

However, if the HW has gamma table before ctm, the atomic property
should be DEGAMMA_LUT, and thus we have:

degamma -> ctm -> out

This is fine for userspace which sets gamma table using the properties,
as the userspace can check for the existence of gamma & degamma, but the
legacy gamma-set ioctl does not work.

This patch adds a new helper, drm_atomic_helper_legacy_degamma_set(),
which can be used instead of drm_atomic_helper_legacy_gamma_set() when
the DEGAMMA_LUT is the underlying property that needs to be set.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 81 ++++++++++++++++++++++-------
 include/drm/drm_atomic_helper.h     |  4 ++
 2 files changed, 65 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ddd0e3239150..23cbed541dc7 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3499,24 +3499,11 @@ int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc,
 }
 EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
 
-/**
- * drm_atomic_helper_legacy_gamma_set - set the legacy gamma correction table
- * @crtc: CRTC object
- * @red: red correction table
- * @green: green correction table
- * @blue: green correction table
- * @size: size of the tables
- * @ctx: lock acquire context
- *
- * Implements support for legacy gamma correction table for drivers
- * that support color management through the DEGAMMA_LUT/GAMMA_LUT
- * properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
- * how the atomic color management and gamma tables work.
- */
-int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
-				       u16 *red, u16 *green, u16 *blue,
-				       uint32_t size,
-				       struct drm_modeset_acquire_ctx *ctx)
+static int legacy_gamma_degamma_set(struct drm_crtc *crtc,
+				    u16 *red, u16 *green, u16 *blue,
+				    uint32_t size,
+				    struct drm_modeset_acquire_ctx *ctx,
+				    bool use_degamma)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_atomic_state *state;
@@ -3555,9 +3542,11 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
 	}
 
 	/* Reset DEGAMMA_LUT and CTM properties. */
-	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
+	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
+					      use_degamma ? blob : NULL);
 	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
-	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
+	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
+					      use_degamma ? NULL : blob);
 	crtc_state->color_mgmt_changed |= replaced;
 
 	ret = drm_atomic_commit(state);
@@ -3567,8 +3556,60 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
 	drm_property_blob_put(blob);
 	return ret;
 }
+
+/**
+ * drm_atomic_helper_legacy_gamma_set - set the legacy gamma correction table using gamma_lut
+ * @crtc: CRTC object
+ * @red: red correction table
+ * @green: green correction table
+ * @blue: green correction table
+ * @size: size of the tables
+ * @ctx: lock acquire context
+ *
+ * Implements support for legacy gamma correction table for drivers
+ * that support color management through the DEGAMMA_LUT/GAMMA_LUT
+ * properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
+ * how the atomic color management and gamma tables work.
+ *
+ * This function uses GAMMA_LUT property for the gamma table. This function
+ * can be used when the driver exposes either only GAMMA_LUT or both GAMMA_LUT
+ * and DEGAMMA_LUT.
+ */
+int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
+				       u16 *red, u16 *green, u16 *blue,
+				       uint32_t size,
+				       struct drm_modeset_acquire_ctx *ctx)
+{
+	return legacy_gamma_degamma_set(crtc, red, green, blue, size, ctx, false);
+}
 EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
 
+/**
+ * drm_atomic_helper_legacy_degamma_set - set the legacy gamma correction table using degamma_lut
+ * @crtc: CRTC object
+ * @red: red correction table
+ * @green: green correction table
+ * @blue: green correction table
+ * @size: size of the tables
+ * @ctx: lock acquire context
+ *
+ * Implements support for legacy gamma correction table for drivers
+ * that support color management through the DEGAMMA_LUT/GAMMA_LUT
+ * properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
+ * how the atomic color management and gamma tables work.
+ *
+ * This function uses DEGAMMA_LUT property for the gamma table. This function
+ * can be used when the driver exposes only DEGAMNMA_LUT.
+ */
+int drm_atomic_helper_legacy_degamma_set(struct drm_crtc *crtc,
+					 u16 *red, u16 *green, u16 *blue,
+					 uint32_t size,
+					 struct drm_modeset_acquire_ctx *ctx)
+{
+	return legacy_gamma_degamma_set(crtc, red, green, blue, size, ctx, true);
+}
+EXPORT_SYMBOL(drm_atomic_helper_legacy_degamma_set);
+
 /**
  * drm_atomic_helper_bridge_propagate_bus_fmt() - Propagate output format to
  *						  the input end of a bridge
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 85df04c8e62f..561c78680388 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -151,6 +151,10 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
 				       u16 *red, u16 *green, u16 *blue,
 				       uint32_t size,
 				       struct drm_modeset_acquire_ctx *ctx);
+int drm_atomic_helper_legacy_degamma_set(struct drm_crtc *crtc,
+					 u16 *red, u16 *green, u16 *blue,
+					 uint32_t size,
+					 struct drm_modeset_acquire_ctx *ctx);
 
 /**
  * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH v2 1/5] drm: add legacy support for using degamma for gamma

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Nov 03, 2020 at 10:03:06AM +0200, Tomi Valkeinen wrote:
> We currently have drm_atomic_helper_legacy_gamma_set() helper which can
> be used to handle legacy gamma-set ioctl.
> drm_atomic_helper_legacy_gamma_set() sets GAMMA_LUT, and clears
> CTM and DEGAMMA_LUT. This works fine on HW where we have either:
> 
> degamma -> ctm -> gamma -> out
> 
> or
> 
> ctm -> gamma -> out
> 
> However, if the HW has gamma table before ctm, the atomic property
> should be DEGAMMA_LUT, and thus we have:
> 
> degamma -> ctm -> out
> 
> This is fine for userspace which sets gamma table using the properties,
> as the userspace can check for the existence of gamma & degamma, but the
> legacy gamma-set ioctl does not work.
> 
> This patch adds a new helper, drm_atomic_helper_legacy_degamma_set(),
> which can be used instead of drm_atomic_helper_legacy_gamma_set() when
> the DEGAMMA_LUT is the underlying property that needs to be set.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 81 ++++++++++++++++++++++-------
>  include/drm/drm_atomic_helper.h     |  4 ++
>  2 files changed, 65 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ddd0e3239150..23cbed541dc7 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3499,24 +3499,11 @@ int drm_atomic_helper_page_flip_target(struct drm_crtc *crtc,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_page_flip_target);
>  
> -/**
> - * drm_atomic_helper_legacy_gamma_set - set the legacy gamma correction table
> - * @crtc: CRTC object
> - * @red: red correction table
> - * @green: green correction table
> - * @blue: green correction table
> - * @size: size of the tables
> - * @ctx: lock acquire context
> - *
> - * Implements support for legacy gamma correction table for drivers
> - * that support color management through the DEGAMMA_LUT/GAMMA_LUT
> - * properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
> - * how the atomic color management and gamma tables work.
> - */
> -int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> -				       u16 *red, u16 *green, u16 *blue,
> -				       uint32_t size,
> -				       struct drm_modeset_acquire_ctx *ctx)
> +static int legacy_gamma_degamma_set(struct drm_crtc *crtc,
> +				    u16 *red, u16 *green, u16 *blue,
> +				    uint32_t size,
> +				    struct drm_modeset_acquire_ctx *ctx,
> +				    bool use_degamma)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_atomic_state *state;
> @@ -3555,9 +3542,11 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>  	}
>  
>  	/* Reset DEGAMMA_LUT and CTM properties. */
> -	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut, NULL);
> +	replaced  = drm_property_replace_blob(&crtc_state->degamma_lut,
> +					      use_degamma ? blob : NULL);
>  	replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL);
> -	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob);
> +	replaced |= drm_property_replace_blob(&crtc_state->gamma_lut,
> +					      use_degamma ? NULL : blob);
>  	crtc_state->color_mgmt_changed |= replaced;
>  
>  	ret = drm_atomic_commit(state);
> @@ -3567,8 +3556,60 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>  	drm_property_blob_put(blob);
>  	return ret;
>  }
> +
> +/**
> + * drm_atomic_helper_legacy_gamma_set - set the legacy gamma correction table using gamma_lut
> + * @crtc: CRTC object
> + * @red: red correction table
> + * @green: green correction table
> + * @blue: green correction table
> + * @size: size of the tables
> + * @ctx: lock acquire context
> + *
> + * Implements support for legacy gamma correction table for drivers
> + * that support color management through the DEGAMMA_LUT/GAMMA_LUT
> + * properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
> + * how the atomic color management and gamma tables work.
> + *
> + * This function uses GAMMA_LUT property for the gamma table. This function

s/uses/uses the/
s/This function$/It/

Same below.

> + * can be used when the driver exposes either only GAMMA_LUT or both GAMMA_LUT
> + * and DEGAMMA_LUT.
> + */
> +int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> +				       u16 *red, u16 *green, u16 *blue,
> +				       uint32_t size,
> +				       struct drm_modeset_acquire_ctx *ctx)
> +{
> +	return legacy_gamma_degamma_set(crtc, red, green, blue, size, ctx, false);
> +}

I wonder, would it make sense to make this automatic by setting the
degamma LUT when only the DEGAMMA_LUT property exists, and the gamma LUT
otherwise ? Are there use cases for drm_atomic_helper_legacy_degamma_set
for drivers that support both gamma and degamma ?

>  EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
>  
> +/**
> + * drm_atomic_helper_legacy_degamma_set - set the legacy gamma correction table using degamma_lut
> + * @crtc: CRTC object
> + * @red: red correction table
> + * @green: green correction table
> + * @blue: green correction table
> + * @size: size of the tables
> + * @ctx: lock acquire context
> + *
> + * Implements support for legacy gamma correction table for drivers
> + * that support color management through the DEGAMMA_LUT/GAMMA_LUT
> + * properties. See drm_crtc_enable_color_mgmt() and the containing chapter for
> + * how the atomic color management and gamma tables work.
> + *
> + * This function uses DEGAMMA_LUT property for the gamma table. This function
> + * can be used when the driver exposes only DEGAMNMA_LUT.
> + */
> +int drm_atomic_helper_legacy_degamma_set(struct drm_crtc *crtc,
> +					 u16 *red, u16 *green, u16 *blue,
> +					 uint32_t size,
> +					 struct drm_modeset_acquire_ctx *ctx)
> +{
> +	return legacy_gamma_degamma_set(crtc, red, green, blue, size, ctx, true);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_legacy_degamma_set);
> +
>  /**
>   * drm_atomic_helper_bridge_propagate_bus_fmt() - Propagate output format to
>   *						  the input end of a bridge
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 85df04c8e62f..561c78680388 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -151,6 +151,10 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>  				       u16 *red, u16 *green, u16 *blue,
>  				       uint32_t size,
>  				       struct drm_modeset_acquire_ctx *ctx);
> +int drm_atomic_helper_legacy_degamma_set(struct drm_crtc *crtc,
> +					 u16 *red, u16 *green, u16 *blue,
> +					 uint32_t size,
> +					 struct drm_modeset_acquire_ctx *ctx);
>  
>  /**
>   * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/5] drm: add legacy support for using degamma for gamma

[Tomi Valkeinen]

On 30/11/2020 12:38, Laurent Pinchart wrote:

>> + * can be used when the driver exposes either only GAMMA_LUT or both GAMMA_LUT
>> + * and DEGAMMA_LUT.
>> + */
>> +int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>> +				       u16 *red, u16 *green, u16 *blue,
>> +				       uint32_t size,
>> +				       struct drm_modeset_acquire_ctx *ctx)
>> +{
>> +	return legacy_gamma_degamma_set(crtc, red, green, blue, size, ctx, false);
>> +}
> 
> I wonder, would it make sense to make this automatic by setting the
> degamma LUT when only the DEGAMMA_LUT property exists, and the gamma LUT
> otherwise ? Are there use cases for drm_atomic_helper_legacy_degamma_set
> for drivers that support both gamma and degamma ?

Yes, I think drm_atomic_helper_legacy_gamma_set() could do that.

But if you look at the second patch, the driver deals with crtc_state->degamma_lut. Having
.gamma_set = drm_atomic_helper_legacy_degamma_set makes it bit more explicit and clear what the
driver is doing.

That said, documenting what drm_atomic_helper_legacy_gamma_set does if there's only degamma should
also be clear enough, so... I don't have strong feelings either way =).

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/5] drm: add legacy support for using degamma for gamma

[Daniel Vetter]

On Mon, Nov 30, 2020 at 02:12:39PM +0200, Tomi Valkeinen wrote:
> On 30/11/2020 12:38, Laurent Pinchart wrote:
> 
> >> + * can be used when the driver exposes either only GAMMA_LUT or both GAMMA_LUT
> >> + * and DEGAMMA_LUT.
> >> + */
> >> +int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
> >> +				       u16 *red, u16 *green, u16 *blue,
> >> +				       uint32_t size,
> >> +				       struct drm_modeset_acquire_ctx *ctx)
> >> +{
> >> +	return legacy_gamma_degamma_set(crtc, red, green, blue, size, ctx, false);
> >> +}
> > 
> > I wonder, would it make sense to make this automatic by setting the
> > degamma LUT when only the DEGAMMA_LUT property exists, and the gamma LUT
> > otherwise ? Are there use cases for drm_atomic_helper_legacy_degamma_set
> > for drivers that support both gamma and degamma ?
> 
> Yes, I think drm_atomic_helper_legacy_gamma_set() could do that.
> 
> But if you look at the second patch, the driver deals with crtc_state->degamma_lut. Having
> .gamma_set = drm_atomic_helper_legacy_degamma_set makes it bit more explicit and clear what the
> driver is doing.
> 
> That said, documenting what drm_atomic_helper_legacy_gamma_set does if there's only degamma should
> also be clear enough, so... I don't have strong feelings either way =).

The thing is, the legacy helpers should be able to pull off what userspace
needs to do when it's using atomic anyway. Hard-coding information in the
kernel means we have a gap here. Hence imo legacy helpers doing the right
thing in all reasonable cases is imo better.

In many cases I think we should even go further, and ditch driver ability
to overwrite legacy helper hooks like this. I thought we'd need that
flexibility for legacy userspace being incompatible in awkward ways, but
wasn't ever really needed. Worse, many drivers forget to wire up the
compat hooks.

tldr, imo right thing to do here:
- move legacy gamma function from helpers into core code
- call it unconditionally for all atomic drivers (if there's no legacy
  drivers using the hook left then we can outright remove it)
- make sure it dtrt in all cases

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/5] drm: add legacy support for using degamma for gamma

[Tomi Valkeinen]

On 30/11/2020 16:10, Daniel Vetter wrote:

> The thing is, the legacy helpers should be able to pull off what userspace
> needs to do when it's using atomic anyway. Hard-coding information in the
> kernel means we have a gap here. Hence imo legacy helpers doing the right
> thing in all reasonable cases is imo better.
> 
> In many cases I think we should even go further, and ditch driver ability
> to overwrite legacy helper hooks like this. I thought we'd need that
> flexibility for legacy userspace being incompatible in awkward ways, but
> wasn't ever really needed. Worse, many drivers forget to wire up the
> compat hooks.
> 
> tldr, imo right thing to do here:
> - move legacy gamma function from helpers into core code
> - call it unconditionally for all atomic drivers (if there's no legacy
>   drivers using the hook left then we can outright remove it)
> - make sure it dtrt in all cases

There are atomic drivers which have their custom gamma_set function. I guess they don't support
atomic color mgmt, but do support (legacy) gamma.

We could make the core code call the gamma legacy helper automatically for atomic drivers that don't
have gamma_set defined but do have GAMMA_LUT or DEGAMMA_LUT. But the gamma_set function is called
also in a few places from drm_fb_helper.c, so this code wouldn't be fully inside drm_color_mgmt.c.

Or we could just change drm_atomic_helper_legacy_gamma_set() to do the right thing, depending on
GAMMA_LUT & DEGAMMA_LUT existence.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/5] drm: add legacy support for using degamma for gamma

[Daniel Vetter]

On Wed, Dec 2, 2020 at 12:52 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> On 30/11/2020 16:10, Daniel Vetter wrote:
>
> > The thing is, the legacy helpers should be able to pull off what userspace
> > needs to do when it's using atomic anyway. Hard-coding information in the
> > kernel means we have a gap here. Hence imo legacy helpers doing the right
> > thing in all reasonable cases is imo better.
> >
> > In many cases I think we should even go further, and ditch driver ability
> > to overwrite legacy helper hooks like this. I thought we'd need that
> > flexibility for legacy userspace being incompatible in awkward ways, but
> > wasn't ever really needed. Worse, many drivers forget to wire up the
> > compat hooks.
> >
> > tldr, imo right thing to do here:
> > - move legacy gamma function from helpers into core code
> > - call it unconditionally for all atomic drivers (if there's no legacy
> >   drivers using the hook left then we can outright remove it)
> > - make sure it dtrt in all cases
>
> There are atomic drivers which have their custom gamma_set function. I guess they don't support
> atomic color mgmt, but do support (legacy) gamma.

Hm yeah, but it's this kind of feature disparity which is why I think
we should at least try to unify more.

> We could make the core code call the gamma legacy helper automatically for atomic drivers that don't
> have gamma_set defined but do have GAMMA_LUT or DEGAMMA_LUT. But the gamma_set function is called
> also in a few places from drm_fb_helper.c, so this code wouldn't be fully inside drm_color_mgmt.c.
>
> Or we could just change drm_atomic_helper_legacy_gamma_set() to do the right thing, depending on
> GAMMA_LUT & DEGAMMA_LUT existence.

Yeah that would be at least better than pushing more decisions onto
drivers as hard-coding. I still think that maybe just automatically
calling the helper when either a GAMMA or DEGAMMA lut is set up would
be better.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/5] drm: add legacy support for using degamma for gamma

[Ville Syrjälä]

On Wed, Dec 02, 2020 at 01:38:42PM +0100, Daniel Vetter wrote:
> On Wed, Dec 2, 2020 at 12:52 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >
> > On 30/11/2020 16:10, Daniel Vetter wrote:
> >
> > > The thing is, the legacy helpers should be able to pull off what userspace
> > > needs to do when it's using atomic anyway. Hard-coding information in the
> > > kernel means we have a gap here. Hence imo legacy helpers doing the right
> > > thing in all reasonable cases is imo better.
> > >
> > > In many cases I think we should even go further, and ditch driver ability
> > > to overwrite legacy helper hooks like this. I thought we'd need that
> > > flexibility for legacy userspace being incompatible in awkward ways, but
> > > wasn't ever really needed. Worse, many drivers forget to wire up the
> > > compat hooks.
> > >
> > > tldr, imo right thing to do here:
> > > - move legacy gamma function from helpers into core code
> > > - call it unconditionally for all atomic drivers (if there's no legacy
> > >   drivers using the hook left then we can outright remove it)
> > > - make sure it dtrt in all cases
> >
> > There are atomic drivers which have their custom gamma_set function. I guess they don't support
> > atomic color mgmt, but do support (legacy) gamma.
> 
> Hm yeah, but it's this kind of feature disparity which is why I think
> we should at least try to unify more.
> 
> > We could make the core code call the gamma legacy helper automatically for atomic drivers that don't
> > have gamma_set defined but do have GAMMA_LUT or DEGAMMA_LUT. But the gamma_set function is called
> > also in a few places from drm_fb_helper.c, so this code wouldn't be fully inside drm_color_mgmt.c.
> >
> > Or we could just change drm_atomic_helper_legacy_gamma_set() to do the right thing, depending on
> > GAMMA_LUT & DEGAMMA_LUT existence.
> 
> Yeah that would be at least better than pushing more decisions onto
> drivers as hard-coding. I still think that maybe just automatically
> calling the helper when either a GAMMA or DEGAMMA lut is set up would
> be better.

BTW I have some gamma related stuff here
git://github.com/vsyrjala/linux.git fb_helper_c8_lut_4

which tries to fix some fb_helper gamma stuff, and I'm also
getting rid of the gamma_store stuff for the leacy uapi for
drivers which implement the fancier color management stuff.
In fact I just threw out the helper thing entirely and made
the core directly call the right stuff. Not sure if that
would be helpful, harmful or just meh here.

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

Re: [PATCH v2 1/5] drm: add legacy support for using degamma for gamma

[Tomi Valkeinen]

On 03/12/2020 14:31, Ville Syrjälä wrote:

> BTW I have some gamma related stuff here
> git://github.com/vsyrjala/linux.git fb_helper_c8_lut_4
> 
> which tries to fix some fb_helper gamma stuff, and I'm also
> getting rid of the gamma_store stuff for the leacy uapi for
> drivers which implement the fancier color management stuff.
> In fact I just threw out the helper thing entirely and made
> the core directly call the right stuff. Not sure if that
> would be helpful, harmful or just meh here.

I didn't check your branch yet, but I just sent "[PATCH 0/2] drm: fix and cleanup legacy gamma support".

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/5] drm/omap: use degamma property for gamma table

[Tomi Valkeinen]

omapdrm supports gamma via GAMMA_LUT property. However, the HW we have
is:

gamma -> ctm -> out

instead of what the model DRM framework uses:

ctm -> gamma -> out

As the following patches add CTM support for omapdrm, lets first fix the
gamma.

This patch changes the property from GAMMA_LUT to DEGAMMA_LUT, and uses
drm_atomic_helper_legacy_degamma_set for gamma_set helper. Thus we will
have:

degamma -> ctm -> out

and the legacy ioctl will continue working as before.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index d7442aa55f89..d40220b2f312 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -575,8 +575,8 @@ static int omap_crtc_atomic_check(struct drm_crtc *crtc,
 									  crtc);
 	struct drm_plane_state *pri_state;
 
-	if (crtc_state->color_mgmt_changed && crtc_state->gamma_lut) {
-		unsigned int length = crtc_state->gamma_lut->length /
+	if (crtc_state->color_mgmt_changed && crtc_state->degamma_lut) {
+		unsigned int length = crtc_state->degamma_lut->length /
 			sizeof(struct drm_color_lut);
 
 		if (length < 2)
@@ -617,10 +617,10 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
 		struct drm_color_lut *lut = NULL;
 		unsigned int length = 0;
 
-		if (crtc->state->gamma_lut) {
+		if (crtc->state->degamma_lut) {
 			lut = (struct drm_color_lut *)
-				crtc->state->gamma_lut->data;
-			length = crtc->state->gamma_lut->length /
+				crtc->state->degamma_lut->data;
+			length = crtc->state->degamma_lut->length /
 				sizeof(*lut);
 		}
 		priv->dispc_ops->mgr_set_gamma(priv->dispc, omap_crtc->channel,
@@ -741,7 +741,7 @@ static const struct drm_crtc_funcs omap_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
 	.destroy = omap_crtc_destroy,
 	.page_flip = drm_atomic_helper_page_flip,
-	.gamma_set = drm_atomic_helper_legacy_gamma_set,
+	.gamma_set = drm_atomic_helper_legacy_degamma_set,
 	.atomic_duplicate_state = omap_crtc_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 	.atomic_set_property = omap_crtc_atomic_set_property,
@@ -842,7 +842,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 	if (priv->dispc_ops->mgr_gamma_size(priv->dispc, channel)) {
 		unsigned int gamma_lut_size = 256;
 
-		drm_crtc_enable_color_mgmt(crtc, 0, false, gamma_lut_size);
+		drm_crtc_enable_color_mgmt(crtc, gamma_lut_size, false, 0);
 		drm_mode_crtc_set_gamma_size(crtc, gamma_lut_size);
 	}
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH v2 2/5] drm/omap: use degamma property for gamma table

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Nov 03, 2020 at 10:03:07AM +0200, Tomi Valkeinen wrote:
> omapdrm supports gamma via GAMMA_LUT property. However, the HW we have
> is:
> 
> gamma -> ctm -> out
> 
> instead of what the model DRM framework uses:
> 
> ctm -> gamma -> out
> 
> As the following patches add CTM support for omapdrm, lets first fix the
> gamma.
> 
> This patch changes the property from GAMMA_LUT to DEGAMMA_LUT, and uses
> drm_atomic_helper_legacy_degamma_set for gamma_set helper. Thus we will
> have:
> 
> degamma -> ctm -> out
> 
> and the legacy ioctl will continue working as before.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index d7442aa55f89..d40220b2f312 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -575,8 +575,8 @@ static int omap_crtc_atomic_check(struct drm_crtc *crtc,
>  									  crtc);
>  	struct drm_plane_state *pri_state;
>  
> -	if (crtc_state->color_mgmt_changed && crtc_state->gamma_lut) {
> -		unsigned int length = crtc_state->gamma_lut->length /
> +	if (crtc_state->color_mgmt_changed && crtc_state->degamma_lut) {
> +		unsigned int length = crtc_state->degamma_lut->length /
>  			sizeof(struct drm_color_lut);
>  
>  		if (length < 2)
> @@ -617,10 +617,10 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
>  		struct drm_color_lut *lut = NULL;
>  		unsigned int length = 0;
>  
> -		if (crtc->state->gamma_lut) {
> +		if (crtc->state->degamma_lut) {
>  			lut = (struct drm_color_lut *)
> -				crtc->state->gamma_lut->data;
> -			length = crtc->state->gamma_lut->length /
> +				crtc->state->degamma_lut->data;
> +			length = crtc->state->degamma_lut->length /
>  				sizeof(*lut);
>  		}
>  		priv->dispc_ops->mgr_set_gamma(priv->dispc, omap_crtc->channel,
> @@ -741,7 +741,7 @@ static const struct drm_crtc_funcs omap_crtc_funcs = {
>  	.set_config = drm_atomic_helper_set_config,
>  	.destroy = omap_crtc_destroy,
>  	.page_flip = drm_atomic_helper_page_flip,
> -	.gamma_set = drm_atomic_helper_legacy_gamma_set,
> +	.gamma_set = drm_atomic_helper_legacy_degamma_set,
>  	.atomic_duplicate_state = omap_crtc_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>  	.atomic_set_property = omap_crtc_atomic_set_property,
> @@ -842,7 +842,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>  	if (priv->dispc_ops->mgr_gamma_size(priv->dispc, channel)) {
>  		unsigned int gamma_lut_size = 256;
>  
> -		drm_crtc_enable_color_mgmt(crtc, 0, false, gamma_lut_size);
> +		drm_crtc_enable_color_mgmt(crtc, gamma_lut_size, false, 0);
>  		drm_mode_crtc_set_gamma_size(crtc, gamma_lut_size);
>  	}
>  
> 

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/5] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Tomi Valkeinen]

From: Jyri Sarha <jsarha@ti.com>

Implement CTM color management property for OMAP CRTC using DSS
overlay manager's Color Phase Rotation matrix. The CPR matrix does not
exactly match the CTM property documentation. On DSS the CPR matrix is
applied after gamma table look up. However, it seems stupid to add a
custom property just for that.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 39 +++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index d40220b2f312..b2c251a8b404 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -391,6 +391,33 @@ static void omap_crtc_manual_display_update(struct work_struct *data)
 	}
 }
 
+static s16 omap_crtc_s31_32_to_s2_8(s64 coef)
+{
+	u64 sign_bit = 1ULL << 63;
+	u64 cbits = (u64)coef;
+
+	s16 ret = clamp_val(((cbits & ~sign_bit) >> 24), 0, 0x1ff);
+
+	if (cbits & sign_bit)
+		ret = -ret;
+
+	return ret;
+}
+
+static void omap_crtc_cpr_coefs_from_ctm(const struct drm_color_ctm *ctm,
+					 struct omap_dss_cpr_coefs *cpr)
+{
+	cpr->rr = omap_crtc_s31_32_to_s2_8(ctm->matrix[0]);
+	cpr->rg = omap_crtc_s31_32_to_s2_8(ctm->matrix[1]);
+	cpr->rb = omap_crtc_s31_32_to_s2_8(ctm->matrix[2]);
+	cpr->gr = omap_crtc_s31_32_to_s2_8(ctm->matrix[3]);
+	cpr->gg = omap_crtc_s31_32_to_s2_8(ctm->matrix[4]);
+	cpr->gb = omap_crtc_s31_32_to_s2_8(ctm->matrix[5]);
+	cpr->br = omap_crtc_s31_32_to_s2_8(ctm->matrix[6]);
+	cpr->bg = omap_crtc_s31_32_to_s2_8(ctm->matrix[7]);
+	cpr->bb = omap_crtc_s31_32_to_s2_8(ctm->matrix[8]);
+}
+
 static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
 {
 	struct omap_drm_private *priv = crtc->dev->dev_private;
@@ -402,7 +429,15 @@ static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
 	info.default_color = 0x000000;
 	info.trans_enabled = false;
 	info.partial_alpha_enabled = false;
-	info.cpr_enable = false;
+
+	if (crtc->state->ctm) {
+		struct drm_color_ctm *ctm = crtc->state->ctm->data;
+
+		info.cpr_enable = true;
+		omap_crtc_cpr_coefs_from_ctm(ctm, &info.cpr_coefs);
+	} else {
+		info.cpr_enable = false;
+	}
 
 	priv->dispc_ops->mgr_setup(priv->dispc, omap_crtc->channel, &info);
 }
@@ -842,7 +877,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 	if (priv->dispc_ops->mgr_gamma_size(priv->dispc, channel)) {
 		unsigned int gamma_lut_size = 256;
 
-		drm_crtc_enable_color_mgmt(crtc, gamma_lut_size, false, 0);
+		drm_crtc_enable_color_mgmt(crtc, gamma_lut_size, true, 0);
 		drm_mode_crtc_set_gamma_size(crtc, gamma_lut_size);
 	}
 
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH v2 3/5] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Laurent Pinchart]

Hi Tomi and Jyri,

Thank you for the patch.

On Tue, Nov 03, 2020 at 10:03:08AM +0200, Tomi Valkeinen wrote:
> From: Jyri Sarha <jsarha@ti.com>
> 
> Implement CTM color management property for OMAP CRTC using DSS
> overlay manager's Color Phase Rotation matrix. The CPR matrix does not
> exactly match the CTM property documentation. On DSS the CPR matrix is
> applied after gamma table look up. However, it seems stupid to add a
> custom property just for that.

Should this be updated now that the driver has switched to using degamma
?

> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 39 +++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index d40220b2f312..b2c251a8b404 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -391,6 +391,33 @@ static void omap_crtc_manual_display_update(struct work_struct *data)
>  	}
>  }
>  
> +static s16 omap_crtc_s31_32_to_s2_8(s64 coef)
> +{
> +	u64 sign_bit = 1ULL << 63;
> +	u64 cbits = (u64)coef;
> +
> +	s16 ret = clamp_val(((cbits & ~sign_bit) >> 24), 0, 0x1ff);
> +
> +	if (cbits & sign_bit)
> +		ret = -ret;
> +
> +	return ret;
> +}
> +
> +static void omap_crtc_cpr_coefs_from_ctm(const struct drm_color_ctm *ctm,
> +					 struct omap_dss_cpr_coefs *cpr)
> +{
> +	cpr->rr = omap_crtc_s31_32_to_s2_8(ctm->matrix[0]);
> +	cpr->rg = omap_crtc_s31_32_to_s2_8(ctm->matrix[1]);
> +	cpr->rb = omap_crtc_s31_32_to_s2_8(ctm->matrix[2]);
> +	cpr->gr = omap_crtc_s31_32_to_s2_8(ctm->matrix[3]);
> +	cpr->gg = omap_crtc_s31_32_to_s2_8(ctm->matrix[4]);
> +	cpr->gb = omap_crtc_s31_32_to_s2_8(ctm->matrix[5]);
> +	cpr->br = omap_crtc_s31_32_to_s2_8(ctm->matrix[6]);
> +	cpr->bg = omap_crtc_s31_32_to_s2_8(ctm->matrix[7]);
> +	cpr->bb = omap_crtc_s31_32_to_s2_8(ctm->matrix[8]);
> +}
> +
>  static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
>  {
>  	struct omap_drm_private *priv = crtc->dev->dev_private;
> @@ -402,7 +429,15 @@ static void omap_crtc_write_crtc_properties(struct drm_crtc *crtc)
>  	info.default_color = 0x000000;
>  	info.trans_enabled = false;
>  	info.partial_alpha_enabled = false;
> -	info.cpr_enable = false;
> +
> +	if (crtc->state->ctm) {
> +		struct drm_color_ctm *ctm = crtc->state->ctm->data;
> +
> +		info.cpr_enable = true;
> +		omap_crtc_cpr_coefs_from_ctm(ctm, &info.cpr_coefs);
> +	} else {
> +		info.cpr_enable = false;
> +	}
>  
>  	priv->dispc_ops->mgr_setup(priv->dispc, omap_crtc->channel, &info);
>  }
> @@ -842,7 +877,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
>  	if (priv->dispc_ops->mgr_gamma_size(priv->dispc, channel)) {
>  		unsigned int gamma_lut_size = 256;
>  
> -		drm_crtc_enable_color_mgmt(crtc, gamma_lut_size, false, 0);
> +		drm_crtc_enable_color_mgmt(crtc, gamma_lut_size, true, 0);
>  		drm_mode_crtc_set_gamma_size(crtc, gamma_lut_size);
>  	}
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/5] drm/omap: Implement CTM property for CRTC using OVL managers CPR matrix

[Tomi Valkeinen]

On 30/11/2020 12:41, Laurent Pinchart wrote:
> Hi Tomi and Jyri,
> 
> Thank you for the patch.
> 
> On Tue, Nov 03, 2020 at 10:03:08AM +0200, Tomi Valkeinen wrote:
>> From: Jyri Sarha <jsarha@ti.com>
>>
>> Implement CTM color management property for OMAP CRTC using DSS
>> overlay manager's Color Phase Rotation matrix. The CPR matrix does not
>> exactly match the CTM property documentation. On DSS the CPR matrix is
>> applied after gamma table look up. However, it seems stupid to add a
>> custom property just for that.
> 
> Should this be updated now that the driver has switched to using degamma
> ?

Right, good catch. I think I can just drop everything after the first sentence.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/5] drm/omap: rearrange includes in omapdss.h

[Tomi Valkeinen]

Drop "uapi/" and rearrange alphabetically.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/omapdss.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index ab19d4af8de7..8e9a2019f173 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -7,13 +7,13 @@
 #ifndef __OMAP_DRM_DSS_H
 #define __OMAP_DRM_DSS_H
 
-#include <linux/list.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_mode.h>
 #include <linux/device.h>
 #include <linux/interrupt.h>
-#include <video/videomode.h>
+#include <linux/list.h>
 #include <linux/platform_data/omapdss.h>
-#include <uapi/drm/drm_mode.h>
-#include <drm/drm_crtc.h>
+#include <video/videomode.h>
 
 #define DISPC_IRQ_FRAMEDONE		(1 << 0)
 #define DISPC_IRQ_VSYNC			(1 << 1)
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH v2 4/5] drm/omap: rearrange includes in omapdss.h

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Tue, Nov 03, 2020 at 10:03:09AM +0200, Tomi Valkeinen wrote:
> Drop "uapi/" and rearrange alphabetically.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/omapdrm/dss/omapdss.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index ab19d4af8de7..8e9a2019f173 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -7,13 +7,13 @@
>  #ifndef __OMAP_DRM_DSS_H
>  #define __OMAP_DRM_DSS_H
>  
> -#include <linux/list.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_mode.h>
>  #include <linux/device.h>
>  #include <linux/interrupt.h>
> -#include <video/videomode.h>
> +#include <linux/list.h>
>  #include <linux/platform_data/omapdss.h>
> -#include <uapi/drm/drm_mode.h>
> -#include <drm/drm_crtc.h>
> +#include <video/videomode.h>
>  
>  #define DISPC_IRQ_FRAMEDONE		(1 << 0)
>  #define DISPC_IRQ_VSYNC			(1 << 1)

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/5] drm/omap: Enable COLOR_ENCODING and COLOR_RANGE properties for planes

[Tomi Valkeinen]

From: Jyri Sarha <jsarha@ti.com>

Adds support for COLOR_ENCODING and COLOR_RANGE properties to
omap_plane.c and dispc.c. The supported encodings and ranges are
presets are:

For COLOR_ENCODING:
- YCbCr BT.601 (default)
- YCbCr BT.709

For COLOR_RANGE:
- YCbCr limited range
- YCbCr full range (default)

Signed-off-by: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c   | 104 ++++++++++++++++----------
 drivers/gpu/drm/omapdrm/dss/omapdss.h |   4 +
 drivers/gpu/drm/omapdrm/omap_plane.c  |  30 ++++++++
 3 files changed, 97 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 48593932bddf..bf0c9d293077 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -874,50 +874,67 @@ static void dispc_ovl_write_color_conv_coef(struct dispc_device *dispc,
 #undef CVAL
 }
 
-static void dispc_wb_write_color_conv_coef(struct dispc_device *dispc,
-					   const struct csc_coef_rgb2yuv *ct)
-{
-	const enum omap_plane_id plane = OMAP_DSS_WB;
-
-#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
+/* YUV -> RGB, ITU-R BT.601, full range */
+static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {
+	256,   0,  358,		/* ry, rcb, rcr |1.000  0.000  1.402|*/
+	256, -88, -182,		/* gy, gcb, gcr |1.000 -0.344 -0.714|*/
+	256, 452,    0,		/* by, bcb, bcr |1.000  1.772  0.000|*/
+	true,			/* full range */
+};
 
-	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
-	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
-	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct->crg));
-	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct->cbr));
-	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
+/* YUV -> RGB, ITU-R BT.601, limited range */
+static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
+	298,    0,  409,	/* ry, rcb, rcr |1.164  0.000  1.596|*/
+	298, -100, -208,	/* gy, gcb, gcr |1.164 -0.392 -0.813|*/
+	298,  516,    0,	/* by, bcb, bcr |1.164  2.017  0.000|*/
+	false,			/* limited range */
+};
 
-	REG_FLD_MOD(dispc, DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
+/* YUV -> RGB, ITU-R BT.709, full range */
+static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_full = {
+	256,    0,  402,        /* ry, rcb, rcr |1.000  0.000  1.570|*/
+	256,  -48, -120,        /* gy, gcb, gcr |1.000 -0.187 -0.467|*/
+	256,  475,    0,        /* by, bcb, bcr |1.000  1.856  0.000|*/
+	true,                   /* full range */
+};
 
-#undef CVAL
-}
+/* YUV -> RGB, ITU-R BT.709, limited range */
+static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_lim = {
+	298,    0,  459,	/* ry, rcb, rcr |1.164  0.000  1.793|*/
+	298,  -55, -136,	/* gy, gcb, gcr |1.164 -0.213 -0.533|*/
+	298,  541,    0,	/* by, bcb, bcr |1.164  2.112  0.000|*/
+	false,			/* limited range */
+};
 
-static void dispc_setup_color_conv_coef(struct dispc_device *dispc)
+static int dispc_ovl_set_csc(struct dispc_device *dispc,
+			     enum omap_plane_id plane,
+			     enum drm_color_encoding color_encoding,
+			     enum drm_color_range color_range)
 {
-	int i;
-	int num_ovl = dispc_get_num_ovls(dispc);
-
-	/* YUV -> RGB, ITU-R BT.601, limited range */
-	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
-		298,    0,  409,	/* ry, rcb, rcr */
-		298, -100, -208,	/* gy, gcb, gcr */
-		298,  516,    0,	/* by, bcb, bcr */
-		false,			/* limited range */
-	};
+	const struct csc_coef_yuv2rgb *csc;
 
-	/* RGB -> YUV, ITU-R BT.601, limited range */
-	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
-		 66, 129,  25,		/* yr,   yg,  yb */
-		-38, -74, 112,		/* cbr, cbg, cbb */
-		112, -94, -18,		/* crr, crg, crb */
-		false,			/* limited range */
-	};
+	switch (color_encoding) {
+	case DRM_COLOR_YCBCR_BT601:
+		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+			csc = &coefs_yuv2rgb_bt601_full;
+		else
+			csc = &coefs_yuv2rgb_bt601_lim;
+		break;
+	case DRM_COLOR_YCBCR_BT709:
+		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
+			csc = &coefs_yuv2rgb_bt709_full;
+		else
+			csc = &coefs_yuv2rgb_bt709_lim;
+		break;
+	default:
+		DSSERR("Unsupported CSC mode %d for plane %d\n",
+		       color_encoding, plane);
+		return -EINVAL;
+	}
 
-	for (i = 1; i < num_ovl; i++)
-		dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
+	dispc_ovl_write_color_conv_coef(dispc, plane, csc);
 
-	if (dispc->feat->has_writeback)
-		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);
+	return 0;
 }
 
 static void dispc_ovl_set_ba0(struct dispc_device *dispc,
@@ -2598,7 +2615,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
 				  u8 pre_mult_alpha, u8 global_alpha,
 				  enum omap_dss_rotation_type rotation_type,
 				  bool replication, const struct videomode *vm,
-				  bool mem_to_mem)
+				  bool mem_to_mem,
+				  enum drm_color_encoding color_encoding,
+				  enum drm_color_range color_range)
 {
 	bool five_taps = true;
 	bool fieldmode = false;
@@ -2747,6 +2766,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
 				      fieldmode, fourcc, rotation);
 		dispc_ovl_set_output_size(dispc, plane, out_width, out_height);
 		dispc_ovl_set_vid_color_conv(dispc, plane, cconv);
+
+		if (plane != OMAP_DSS_WB)
+			dispc_ovl_set_csc(dispc, plane, color_encoding, color_range);
 	}
 
 	dispc_ovl_set_rotation_attrs(dispc, plane, rotation, rotation_type,
@@ -2783,7 +2805,8 @@ static int dispc_ovl_setup(struct dispc_device *dispc,
 		oi->screen_width, oi->pos_x, oi->pos_y, oi->width, oi->height,
 		oi->out_width, oi->out_height, oi->fourcc, oi->rotation,
 		oi->zorder, oi->pre_mult_alpha, oi->global_alpha,
-		oi->rotation_type, replication, vm, mem_to_mem);
+		oi->rotation_type, replication, vm, mem_to_mem,
+		oi->color_encoding, oi->color_range);
 
 	return r;
 }
@@ -2816,7 +2839,8 @@ static int dispc_wb_setup(struct dispc_device *dispc,
 		wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width,
 		wi->height, wi->fourcc, wi->rotation, zorder,
 		wi->pre_mult_alpha, global_alpha, wi->rotation_type,
-		replication, vm, mem_to_mem);
+		replication, vm, mem_to_mem, DRM_COLOR_YCBCR_BT601,
+		DRM_COLOR_YCBCR_LIMITED_RANGE);
 	if (r)
 		return r;
 
@@ -3927,8 +3951,6 @@ static void _omap_dispc_initial_config(struct dispc_device *dispc)
 	    dispc->feat->has_gamma_table)
 		REG_FLD_MOD(dispc, DISPC_CONFIG, 1, 9, 9);
 
-	dispc_setup_color_conv_coef(dispc);
-
 	dispc_set_loadmode(dispc, OMAP_DSS_LOAD_FRAME_ONLY);
 
 	dispc_init_fifos(dispc);
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index 8e9a2019f173..816424eb2d41 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -7,6 +7,7 @@
 #ifndef __OMAP_DRM_DSS_H
 #define __OMAP_DRM_DSS_H
 
+#include <drm/drm_color_mgmt.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_mode.h>
 #include <linux/device.h>
@@ -243,6 +244,9 @@ struct omap_overlay_info {
 	u8 global_alpha;
 	u8 pre_mult_alpha;
 	u8 zorder;
+
+	enum drm_color_encoding color_encoding;
+	enum drm_color_range color_range;
 };
 
 struct omap_overlay_manager_info {
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 73ec99819a3d..1f433fb5f207 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -59,6 +59,8 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 		info.pre_mult_alpha = 1;
 	else
 		info.pre_mult_alpha = 0;
+	info.color_encoding = state->color_encoding;
+	info.color_range = state->color_range;
 
 	/* update scanout: */
 	omap_framebuffer_update_scanout(state->fb, state, &info);
@@ -189,6 +191,8 @@ static void omap_plane_reset(struct drm_plane *plane)
 	 */
 	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
 			   ? 0 : omap_plane->id;
+	plane->state->color_encoding = DRM_COLOR_YCBCR_BT601;
+	plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE;
 }
 
 static int omap_plane_atomic_set_property(struct drm_plane *plane,
@@ -232,6 +236,23 @@ static const struct drm_plane_funcs omap_plane_funcs = {
 	.atomic_get_property = omap_plane_atomic_get_property,
 };
 
+static bool omap_plane_supports_yuv(struct drm_plane *plane)
+{
+	struct omap_drm_private *priv = plane->dev->dev_private;
+	struct omap_plane *omap_plane = to_omap_plane(plane);
+	const u32 *formats =
+		priv->dispc_ops->ovl_get_color_modes(priv->dispc, omap_plane->id);
+	u32 i;
+
+	for (i = 0; formats[i]; i++)
+		if (formats[i] == DRM_FORMAT_YUYV ||
+		    formats[i] == DRM_FORMAT_UYVY ||
+		    formats[i] == DRM_FORMAT_NV12)
+			return true;
+
+	return false;
+}
+
 static const char *plane_id_to_name[] = {
 	[OMAP_DSS_GFX] = "gfx",
 	[OMAP_DSS_VIDEO1] = "vid1",
@@ -293,6 +314,15 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 	drm_plane_create_blend_mode_property(plane, BIT(DRM_MODE_BLEND_PREMULTI) |
 					     BIT(DRM_MODE_BLEND_COVERAGE));
 
+	if (omap_plane_supports_yuv(plane))
+		drm_plane_create_color_properties(plane,
+						  BIT(DRM_COLOR_YCBCR_BT601) |
+						  BIT(DRM_COLOR_YCBCR_BT709),
+						  BIT(DRM_COLOR_YCBCR_FULL_RANGE) |
+						  BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
+						  DRM_COLOR_YCBCR_BT601,
+						  DRM_COLOR_YCBCR_FULL_RANGE);
+
 	return plane;
 
 error:
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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

Re: [PATCH v2 5/5] drm/omap: Enable COLOR_ENCODING and COLOR_RANGE properties for planes

[Laurent Pinchart]

Hi Tomi and Jyri,

Thank you for the patch.

On Tue, Nov 03, 2020 at 10:03:10AM +0200, Tomi Valkeinen wrote:
> From: Jyri Sarha <jsarha@ti.com>
> 
> Adds support for COLOR_ENCODING and COLOR_RANGE properties to
> omap_plane.c and dispc.c. The supported encodings and ranges are
> presets are:
> 
> For COLOR_ENCODING:
> - YCbCr BT.601 (default)
> - YCbCr BT.709
> 
> For COLOR_RANGE:
> - YCbCr limited range
> - YCbCr full range (default)
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 104 ++++++++++++++++----------
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |   4 +
>  drivers/gpu/drm/omapdrm/omap_plane.c  |  30 ++++++++
>  3 files changed, 97 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index 48593932bddf..bf0c9d293077 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -874,50 +874,67 @@ static void dispc_ovl_write_color_conv_coef(struct dispc_device *dispc,
>  #undef CVAL
>  }
>  
> -static void dispc_wb_write_color_conv_coef(struct dispc_device *dispc,
> -					   const struct csc_coef_rgb2yuv *ct)
> -{
> -	const enum omap_plane_id plane = OMAP_DSS_WB;
> -
> -#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
> +/* YUV -> RGB, ITU-R BT.601, full range */
> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {
> +	256,   0,  358,		/* ry, rcb, rcr |1.000  0.000  1.402|*/
> +	256, -88, -182,		/* gy, gcb, gcr |1.000 -0.344 -0.714|*/
> +	256, 452,    0,		/* by, bcb, bcr |1.000  1.772  0.000|*/
> +	true,			/* full range */
> +};
>  
> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct->crg));
> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct->cbr));
> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
> +/* YUV -> RGB, ITU-R BT.601, limited range */
> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
> +	298,    0,  409,	/* ry, rcb, rcr |1.164  0.000  1.596|*/
> +	298, -100, -208,	/* gy, gcb, gcr |1.164 -0.392 -0.813|*/
> +	298,  516,    0,	/* by, bcb, bcr |1.164  2.017  0.000|*/
> +	false,			/* limited range */
> +};
>  
> -	REG_FLD_MOD(dispc, DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
> +/* YUV -> RGB, ITU-R BT.709, full range */
> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_full = {
> +	256,    0,  402,        /* ry, rcb, rcr |1.000  0.000  1.570|*/
> +	256,  -48, -120,        /* gy, gcb, gcr |1.000 -0.187 -0.467|*/
> +	256,  475,    0,        /* by, bcb, bcr |1.000  1.856  0.000|*/
> +	true,                   /* full range */
> +};
>  
> -#undef CVAL
> -}
> +/* YUV -> RGB, ITU-R BT.709, limited range */
> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_lim = {
> +	298,    0,  459,	/* ry, rcb, rcr |1.164  0.000  1.793|*/
> +	298,  -55, -136,	/* gy, gcb, gcr |1.164 -0.213 -0.533|*/
> +	298,  541,    0,	/* by, bcb, bcr |1.164  2.112  0.000|*/
> +	false,			/* limited range */
> +};
>  
> -static void dispc_setup_color_conv_coef(struct dispc_device *dispc)
> +static int dispc_ovl_set_csc(struct dispc_device *dispc,
> +			     enum omap_plane_id plane,
> +			     enum drm_color_encoding color_encoding,
> +			     enum drm_color_range color_range)
>  {
> -	int i;
> -	int num_ovl = dispc_get_num_ovls(dispc);
> -
> -	/* YUV -> RGB, ITU-R BT.601, limited range */
> -	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
> -		298,    0,  409,	/* ry, rcb, rcr */
> -		298, -100, -208,	/* gy, gcb, gcr */
> -		298,  516,    0,	/* by, bcb, bcr */
> -		false,			/* limited range */
> -	};
> +	const struct csc_coef_yuv2rgb *csc;
>  
> -	/* RGB -> YUV, ITU-R BT.601, limited range */
> -	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
> -		 66, 129,  25,		/* yr,   yg,  yb */
> -		-38, -74, 112,		/* cbr, cbg, cbb */
> -		112, -94, -18,		/* crr, crg, crb */
> -		false,			/* limited range */
> -	};
> +	switch (color_encoding) {
> +	case DRM_COLOR_YCBCR_BT601:
> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> +			csc = &coefs_yuv2rgb_bt601_full;
> +		else
> +			csc = &coefs_yuv2rgb_bt601_lim;
> +		break;
> +	case DRM_COLOR_YCBCR_BT709:
> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> +			csc = &coefs_yuv2rgb_bt709_full;
> +		else
> +			csc = &coefs_yuv2rgb_bt709_lim;
> +		break;
> +	default:
> +		DSSERR("Unsupported CSC mode %d for plane %d\n",
> +		       color_encoding, plane);
> +		return -EINVAL;

Can this happen, given that the properties are created with only the
above four combinations being allowed ?

> +	}
>  
> -	for (i = 1; i < num_ovl; i++)
> -		dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
> +	dispc_ovl_write_color_conv_coef(dispc, plane, csc);
>  
> -	if (dispc->feat->has_writeback)
> -		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);

Unless I'm mistaken, the writeback plane doesn't have the CSC matrix
configured anymore. Is that intentional ?

> +	return 0;
>  }
>  
>  static void dispc_ovl_set_ba0(struct dispc_device *dispc,
> @@ -2598,7 +2615,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
>  				  u8 pre_mult_alpha, u8 global_alpha,
>  				  enum omap_dss_rotation_type rotation_type,
>  				  bool replication, const struct videomode *vm,
> -				  bool mem_to_mem)
> +				  bool mem_to_mem,
> +				  enum drm_color_encoding color_encoding,
> +				  enum drm_color_range color_range)
>  {
>  	bool five_taps = true;
>  	bool fieldmode = false;
> @@ -2747,6 +2766,9 @@ static int dispc_ovl_setup_common(struct dispc_device *dispc,
>  				      fieldmode, fourcc, rotation);
>  		dispc_ovl_set_output_size(dispc, plane, out_width, out_height);
>  		dispc_ovl_set_vid_color_conv(dispc, plane, cconv);
> +
> +		if (plane != OMAP_DSS_WB)
> +			dispc_ovl_set_csc(dispc, plane, color_encoding, color_range);
>  	}
>  
>  	dispc_ovl_set_rotation_attrs(dispc, plane, rotation, rotation_type,
> @@ -2783,7 +2805,8 @@ static int dispc_ovl_setup(struct dispc_device *dispc,
>  		oi->screen_width, oi->pos_x, oi->pos_y, oi->width, oi->height,
>  		oi->out_width, oi->out_height, oi->fourcc, oi->rotation,
>  		oi->zorder, oi->pre_mult_alpha, oi->global_alpha,
> -		oi->rotation_type, replication, vm, mem_to_mem);
> +		oi->rotation_type, replication, vm, mem_to_mem,
> +		oi->color_encoding, oi->color_range);
>  
>  	return r;
>  }
> @@ -2816,7 +2839,8 @@ static int dispc_wb_setup(struct dispc_device *dispc,
>  		wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width,
>  		wi->height, wi->fourcc, wi->rotation, zorder,
>  		wi->pre_mult_alpha, global_alpha, wi->rotation_type,
> -		replication, vm, mem_to_mem);
> +		replication, vm, mem_to_mem, DRM_COLOR_YCBCR_BT601,
> +		DRM_COLOR_YCBCR_LIMITED_RANGE);
>  	if (r)
>  		return r;
>  
> @@ -3927,8 +3951,6 @@ static void _omap_dispc_initial_config(struct dispc_device *dispc)
>  	    dispc->feat->has_gamma_table)
>  		REG_FLD_MOD(dispc, DISPC_CONFIG, 1, 9, 9);
>  
> -	dispc_setup_color_conv_coef(dispc);
> -
>  	dispc_set_loadmode(dispc, OMAP_DSS_LOAD_FRAME_ONLY);
>  
>  	dispc_init_fifos(dispc);
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index 8e9a2019f173..816424eb2d41 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -7,6 +7,7 @@
>  #ifndef __OMAP_DRM_DSS_H
>  #define __OMAP_DRM_DSS_H
>  
> +#include <drm/drm_color_mgmt.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_mode.h>
>  #include <linux/device.h>
> @@ -243,6 +244,9 @@ struct omap_overlay_info {
>  	u8 global_alpha;
>  	u8 pre_mult_alpha;
>  	u8 zorder;
> +
> +	enum drm_color_encoding color_encoding;
> +	enum drm_color_range color_range;
>  };
>  
>  struct omap_overlay_manager_info {
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 73ec99819a3d..1f433fb5f207 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -59,6 +59,8 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
>  		info.pre_mult_alpha = 1;
>  	else
>  		info.pre_mult_alpha = 0;
> +	info.color_encoding = state->color_encoding;
> +	info.color_range = state->color_range;
>  
>  	/* update scanout: */
>  	omap_framebuffer_update_scanout(state->fb, state, &info);
> @@ -189,6 +191,8 @@ static void omap_plane_reset(struct drm_plane *plane)
>  	 */
>  	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
>  			   ? 0 : omap_plane->id;
> +	plane->state->color_encoding = DRM_COLOR_YCBCR_BT601;
> +	plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE;
>  }
>  
>  static int omap_plane_atomic_set_property(struct drm_plane *plane,
> @@ -232,6 +236,23 @@ static const struct drm_plane_funcs omap_plane_funcs = {
>  	.atomic_get_property = omap_plane_atomic_get_property,
>  };
>  
> +static bool omap_plane_supports_yuv(struct drm_plane *plane)
> +{
> +	struct omap_drm_private *priv = plane->dev->dev_private;
> +	struct omap_plane *omap_plane = to_omap_plane(plane);
> +	const u32 *formats =
> +		priv->dispc_ops->ovl_get_color_modes(priv->dispc, omap_plane->id);
> +	u32 i;
> +
> +	for (i = 0; formats[i]; i++)
> +		if (formats[i] == DRM_FORMAT_YUYV ||
> +		    formats[i] == DRM_FORMAT_UYVY ||
> +		    formats[i] == DRM_FORMAT_NV12)
> +			return true;
> +
> +	return false;
> +}
> +
>  static const char *plane_id_to_name[] = {
>  	[OMAP_DSS_GFX] = "gfx",
>  	[OMAP_DSS_VIDEO1] = "vid1",
> @@ -293,6 +314,15 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
>  	drm_plane_create_blend_mode_property(plane, BIT(DRM_MODE_BLEND_PREMULTI) |
>  					     BIT(DRM_MODE_BLEND_COVERAGE));
>  
> +	if (omap_plane_supports_yuv(plane))
> +		drm_plane_create_color_properties(plane,
> +						  BIT(DRM_COLOR_YCBCR_BT601) |
> +						  BIT(DRM_COLOR_YCBCR_BT709),
> +						  BIT(DRM_COLOR_YCBCR_FULL_RANGE) |
> +						  BIT(DRM_COLOR_YCBCR_LIMITED_RANGE),
> +						  DRM_COLOR_YCBCR_BT601,
> +						  DRM_COLOR_YCBCR_FULL_RANGE);
> +
>  	return plane;
>  
>  error:

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/5] drm/omap: Enable COLOR_ENCODING and COLOR_RANGE properties for planes

[Tomi Valkeinen]

On 30/11/2020 12:50, Laurent Pinchart wrote:
> Hi Tomi and Jyri,
> 
> Thank you for the patch.
> 
> On Tue, Nov 03, 2020 at 10:03:10AM +0200, Tomi Valkeinen wrote:
>> From: Jyri Sarha <jsarha@ti.com>
>>
>> Adds support for COLOR_ENCODING and COLOR_RANGE properties to
>> omap_plane.c and dispc.c. The supported encodings and ranges are
>> presets are:
>>
>> For COLOR_ENCODING:
>> - YCbCr BT.601 (default)
>> - YCbCr BT.709
>>
>> For COLOR_RANGE:
>> - YCbCr limited range
>> - YCbCr full range (default)
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 104 ++++++++++++++++----------
>>  drivers/gpu/drm/omapdrm/dss/omapdss.h |   4 +
>>  drivers/gpu/drm/omapdrm/omap_plane.c  |  30 ++++++++
>>  3 files changed, 97 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
>> index 48593932bddf..bf0c9d293077 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
>> @@ -874,50 +874,67 @@ static void dispc_ovl_write_color_conv_coef(struct dispc_device *dispc,
>>  #undef CVAL
>>  }
>>  
>> -static void dispc_wb_write_color_conv_coef(struct dispc_device *dispc,
>> -					   const struct csc_coef_rgb2yuv *ct)
>> -{
>> -	const enum omap_plane_id plane = OMAP_DSS_WB;
>> -
>> -#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
>> +/* YUV -> RGB, ITU-R BT.601, full range */
>> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {
>> +	256,   0,  358,		/* ry, rcb, rcr |1.000  0.000  1.402|*/
>> +	256, -88, -182,		/* gy, gcb, gcr |1.000 -0.344 -0.714|*/
>> +	256, 452,    0,		/* by, bcb, bcr |1.000  1.772  0.000|*/
>> +	true,			/* full range */
>> +};
>>  
>> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
>> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
>> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct->crg));
>> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct->cbr));
>> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
>> +/* YUV -> RGB, ITU-R BT.601, limited range */
>> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
>> +	298,    0,  409,	/* ry, rcb, rcr |1.164  0.000  1.596|*/
>> +	298, -100, -208,	/* gy, gcb, gcr |1.164 -0.392 -0.813|*/
>> +	298,  516,    0,	/* by, bcb, bcr |1.164  2.017  0.000|*/
>> +	false,			/* limited range */
>> +};
>>  
>> -	REG_FLD_MOD(dispc, DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
>> +/* YUV -> RGB, ITU-R BT.709, full range */
>> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_full = {
>> +	256,    0,  402,        /* ry, rcb, rcr |1.000  0.000  1.570|*/
>> +	256,  -48, -120,        /* gy, gcb, gcr |1.000 -0.187 -0.467|*/
>> +	256,  475,    0,        /* by, bcb, bcr |1.000  1.856  0.000|*/
>> +	true,                   /* full range */
>> +};
>>  
>> -#undef CVAL
>> -}
>> +/* YUV -> RGB, ITU-R BT.709, limited range */
>> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_lim = {
>> +	298,    0,  459,	/* ry, rcb, rcr |1.164  0.000  1.793|*/
>> +	298,  -55, -136,	/* gy, gcb, gcr |1.164 -0.213 -0.533|*/
>> +	298,  541,    0,	/* by, bcb, bcr |1.164  2.112  0.000|*/
>> +	false,			/* limited range */
>> +};
>>  
>> -static void dispc_setup_color_conv_coef(struct dispc_device *dispc)
>> +static int dispc_ovl_set_csc(struct dispc_device *dispc,
>> +			     enum omap_plane_id plane,
>> +			     enum drm_color_encoding color_encoding,
>> +			     enum drm_color_range color_range)
>>  {
>> -	int i;
>> -	int num_ovl = dispc_get_num_ovls(dispc);
>> -
>> -	/* YUV -> RGB, ITU-R BT.601, limited range */
>> -	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
>> -		298,    0,  409,	/* ry, rcb, rcr */
>> -		298, -100, -208,	/* gy, gcb, gcr */
>> -		298,  516,    0,	/* by, bcb, bcr */
>> -		false,			/* limited range */
>> -	};
>> +	const struct csc_coef_yuv2rgb *csc;
>>  
>> -	/* RGB -> YUV, ITU-R BT.601, limited range */
>> -	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
>> -		 66, 129,  25,		/* yr,   yg,  yb */
>> -		-38, -74, 112,		/* cbr, cbg, cbb */
>> -		112, -94, -18,		/* crr, crg, crb */
>> -		false,			/* limited range */
>> -	};
>> +	switch (color_encoding) {
>> +	case DRM_COLOR_YCBCR_BT601:
>> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
>> +			csc = &coefs_yuv2rgb_bt601_full;
>> +		else
>> +			csc = &coefs_yuv2rgb_bt601_lim;
>> +		break;
>> +	case DRM_COLOR_YCBCR_BT709:
>> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
>> +			csc = &coefs_yuv2rgb_bt709_full;
>> +		else
>> +			csc = &coefs_yuv2rgb_bt709_lim;
>> +		break;
>> +	default:
>> +		DSSERR("Unsupported CSC mode %d for plane %d\n",
>> +		       color_encoding, plane);
>> +		return -EINVAL;
> 
> Can this happen, given that the properties are created with only the
> above four combinations being allowed ?

No, it shouldn't happen. Are you just asking, or are you suggesting that we shouldn't check if
color_encoding is valid here?

I don't usually check parameters which we can expect to be correct, but with we use switch/if with
the parameter, we have to deal with the "else" case too.

>> +	}
>>  
>> -	for (i = 1; i < num_ovl; i++)
>> -		dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
>> +	dispc_ovl_write_color_conv_coef(dispc, plane, csc);
>>  
>> -	if (dispc->feat->has_writeback)
>> -		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);
> 
> Unless I'm mistaken, the writeback plane doesn't have the CSC matrix
> configured anymore. Is that intentional ?

It's intentional. I should add it to the description.

The driver doesn't support writeback, even if we have bits and pieces of writeback code in the
dispc.c. I've been hoping to add WB support, so I haven't just removed them. However, here I didn't
want to add new code for WB that I can't test, so I decided to just drop the WB case.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/5] drm/omap: Enable COLOR_ENCODING and COLOR_RANGE properties for planes

[Laurent Pinchart]

Hi Tomi,

On Mon, Nov 30, 2020 at 01:53:25PM +0200, Tomi Valkeinen wrote:
> On 30/11/2020 12:50, Laurent Pinchart wrote:
> > On Tue, Nov 03, 2020 at 10:03:10AM +0200, Tomi Valkeinen wrote:
> >> From: Jyri Sarha <jsarha@ti.com>
> >>
> >> Adds support for COLOR_ENCODING and COLOR_RANGE properties to
> >> omap_plane.c and dispc.c. The supported encodings and ranges are
> >> presets are:
> >>
> >> For COLOR_ENCODING:
> >> - YCbCr BT.601 (default)
> >> - YCbCr BT.709
> >>
> >> For COLOR_RANGE:
> >> - YCbCr limited range
> >> - YCbCr full range (default)
> >>
> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 104 ++++++++++++++++----------
> >>  drivers/gpu/drm/omapdrm/dss/omapdss.h |   4 +
> >>  drivers/gpu/drm/omapdrm/omap_plane.c  |  30 ++++++++
> >>  3 files changed, 97 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> index 48593932bddf..bf0c9d293077 100644
> >> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >> @@ -874,50 +874,67 @@ static void dispc_ovl_write_color_conv_coef(struct dispc_device *dispc,
> >>  #undef CVAL
> >>  }
> >>  
> >> -static void dispc_wb_write_color_conv_coef(struct dispc_device *dispc,
> >> -					   const struct csc_coef_rgb2yuv *ct)
> >> -{
> >> -	const enum omap_plane_id plane = OMAP_DSS_WB;
> >> -
> >> -#define CVAL(x, y) (FLD_VAL(x, 26, 16) | FLD_VAL(y, 10, 0))
> >> +/* YUV -> RGB, ITU-R BT.601, full range */
> >> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_full = {
> >> +	256,   0,  358,		/* ry, rcb, rcr |1.000  0.000  1.402|*/
> >> +	256, -88, -182,		/* gy, gcb, gcr |1.000 -0.344 -0.714|*/
> >> +	256, 452,    0,		/* by, bcb, bcr |1.000  1.772  0.000|*/
> >> +	true,			/* full range */
> >> +};
> >>  
> >> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 0), CVAL(ct->yg,  ct->yr));
> >> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 1), CVAL(ct->crr, ct->yb));
> >> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 2), CVAL(ct->crb, ct->crg));
> >> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 3), CVAL(ct->cbg, ct->cbr));
> >> -	dispc_write_reg(dispc, DISPC_OVL_CONV_COEF(plane, 4), CVAL(0, ct->cbb));
> >> +/* YUV -> RGB, ITU-R BT.601, limited range */
> >> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
> >> +	298,    0,  409,	/* ry, rcb, rcr |1.164  0.000  1.596|*/
> >> +	298, -100, -208,	/* gy, gcb, gcr |1.164 -0.392 -0.813|*/
> >> +	298,  516,    0,	/* by, bcb, bcr |1.164  2.017  0.000|*/
> >> +	false,			/* limited range */
> >> +};
> >>  
> >> -	REG_FLD_MOD(dispc, DISPC_OVL_ATTRIBUTES(plane), ct->full_range, 11, 11);
> >> +/* YUV -> RGB, ITU-R BT.709, full range */
> >> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_full = {
> >> +	256,    0,  402,        /* ry, rcb, rcr |1.000  0.000  1.570|*/
> >> +	256,  -48, -120,        /* gy, gcb, gcr |1.000 -0.187 -0.467|*/
> >> +	256,  475,    0,        /* by, bcb, bcr |1.000  1.856  0.000|*/
> >> +	true,                   /* full range */
> >> +};
> >>  
> >> -#undef CVAL
> >> -}
> >> +/* YUV -> RGB, ITU-R BT.709, limited range */
> >> +static const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt709_lim = {
> >> +	298,    0,  459,	/* ry, rcb, rcr |1.164  0.000  1.793|*/
> >> +	298,  -55, -136,	/* gy, gcb, gcr |1.164 -0.213 -0.533|*/
> >> +	298,  541,    0,	/* by, bcb, bcr |1.164  2.112  0.000|*/
> >> +	false,			/* limited range */
> >> +};
> >>  
> >> -static void dispc_setup_color_conv_coef(struct dispc_device *dispc)
> >> +static int dispc_ovl_set_csc(struct dispc_device *dispc,
> >> +			     enum omap_plane_id plane,
> >> +			     enum drm_color_encoding color_encoding,
> >> +			     enum drm_color_range color_range)
> >>  {
> >> -	int i;
> >> -	int num_ovl = dispc_get_num_ovls(dispc);
> >> -
> >> -	/* YUV -> RGB, ITU-R BT.601, limited range */
> >> -	const struct csc_coef_yuv2rgb coefs_yuv2rgb_bt601_lim = {
> >> -		298,    0,  409,	/* ry, rcb, rcr */
> >> -		298, -100, -208,	/* gy, gcb, gcr */
> >> -		298,  516,    0,	/* by, bcb, bcr */
> >> -		false,			/* limited range */
> >> -	};
> >> +	const struct csc_coef_yuv2rgb *csc;
> >>  
> >> -	/* RGB -> YUV, ITU-R BT.601, limited range */
> >> -	const struct csc_coef_rgb2yuv coefs_rgb2yuv_bt601_lim = {
> >> -		 66, 129,  25,		/* yr,   yg,  yb */
> >> -		-38, -74, 112,		/* cbr, cbg, cbb */
> >> -		112, -94, -18,		/* crr, crg, crb */
> >> -		false,			/* limited range */
> >> -	};
> >> +	switch (color_encoding) {
> >> +	case DRM_COLOR_YCBCR_BT601:
> >> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> >> +			csc = &coefs_yuv2rgb_bt601_full;
> >> +		else
> >> +			csc = &coefs_yuv2rgb_bt601_lim;
> >> +		break;
> >> +	case DRM_COLOR_YCBCR_BT709:
> >> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
> >> +			csc = &coefs_yuv2rgb_bt709_full;
> >> +		else
> >> +			csc = &coefs_yuv2rgb_bt709_lim;
> >> +		break;
> >> +	default:
> >> +		DSSERR("Unsupported CSC mode %d for plane %d\n",
> >> +		       color_encoding, plane);
> >> +		return -EINVAL;
> > 
> > Can this happen, given that the properties are created with only the
> > above four combinations being allowed ?
> 
> No, it shouldn't happen. Are you just asking, or are you suggesting that we shouldn't check if
> color_encoding is valid here?
> 
> I don't usually check parameters which we can expect to be correct, but with we use switch/if with
> the parameter, we have to deal with the "else" case too.

I use a default in that case, especially if it can avoid turning the
function from void to int.

> >> +	}
> >>  
> >> -	for (i = 1; i < num_ovl; i++)
> >> -		dispc_ovl_write_color_conv_coef(dispc, i, &coefs_yuv2rgb_bt601_lim);
> >> +	dispc_ovl_write_color_conv_coef(dispc, plane, csc);
> >>  
> >> -	if (dispc->feat->has_writeback)
> >> -		dispc_wb_write_color_conv_coef(dispc, &coefs_rgb2yuv_bt601_lim);
> > 
> > Unless I'm mistaken, the writeback plane doesn't have the CSC matrix
> > configured anymore. Is that intentional ?
> 
> It's intentional. I should add it to the description.
> 
> The driver doesn't support writeback, even if we have bits and pieces of writeback code in the
> dispc.c. I've been hoping to add WB support, so I haven't just removed them. However, here I didn't
> want to add new code for WB that I can't test, so I decided to just drop the WB case.

Sounds fair.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/5] drm/omap: Enable COLOR_ENCODING and COLOR_RANGE properties for planes

[Tomi Valkeinen]

On 30/11/2020 16:19, Laurent Pinchart wrote:

>>>> +	switch (color_encoding) {
>>>> +	case DRM_COLOR_YCBCR_BT601:
>>>> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
>>>> +			csc = &coefs_yuv2rgb_bt601_full;
>>>> +		else
>>>> +			csc = &coefs_yuv2rgb_bt601_lim;
>>>> +		break;
>>>> +	case DRM_COLOR_YCBCR_BT709:
>>>> +		if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)
>>>> +			csc = &coefs_yuv2rgb_bt709_full;
>>>> +		else
>>>> +			csc = &coefs_yuv2rgb_bt709_lim;
>>>> +		break;
>>>> +	default:
>>>> +		DSSERR("Unsupported CSC mode %d for plane %d\n",
>>>> +		       color_encoding, plane);
>>>> +		return -EINVAL;
>>>
>>> Can this happen, given that the properties are created with only the
>>> above four combinations being allowed ?
>>
>> No, it shouldn't happen. Are you just asking, or are you suggesting that we shouldn't check if
>> color_encoding is valid here?
>>
>> I don't usually check parameters which we can expect to be correct, but with we use switch/if with
>> the parameter, we have to deal with the "else" case too.
> 
> I use a default in that case, especially if it can avoid turning the
> function from void to int.

Yes, that's true. I'll do the change.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel