| Message ID | 20201103080310.164453-2-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: add color mgmt support | expand |
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
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
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