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(-)
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
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->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 bridgediff --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
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
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
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
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
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.
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
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); }
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);@@ -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);
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); }
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);
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
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)
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)
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:
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;elsecsc = &coefs_yuv2rgb_bt601_lim;break;- case DRM_COLOR_YCBCR_BT709:
if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)csc = &coefs_yuv2rgb_bt709_full;elsecsc = &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);@@ -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:
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;elsecsc = &coefs_yuv2rgb_bt601_lim;break;- case DRM_COLOR_YCBCR_BT709:
if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)csc = &coefs_yuv2rgb_bt709_full;elsecsc = &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
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;elsecsc = &coefs_yuv2rgb_bt601_lim;break;- case DRM_COLOR_YCBCR_BT709:
if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)csc = &coefs_yuv2rgb_bt709_full;elsecsc = &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
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;elsecsc = &coefs_yuv2rgb_bt601_lim;break;- case DRM_COLOR_YCBCR_BT709:
if (color_range == DRM_COLOR_YCBCR_FULL_RANGE)csc = &coefs_yuv2rgb_bt709_full;elsecsc = &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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Laurent Pinchart -
Tomi Valkeinen -
Ville Syrjälä