| Message ID | 20201208135759.451772-2-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: fix and cleanup legacy gamma support | expand |
Hi Tomi, Thank you for the patch. On Tue, Dec 08, 2020 at 03:57:58PM +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 fixes the issue by changing > drm_atomic_helper_legacy_gamma_set() so that GAMMA_LUT will be used if > it exists, and DEGAMMA_LUT will be used as a fallback. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++++++--- > drivers/gpu/drm/drm_color_mgmt.c | 4 ++++ > drivers/gpu/drm/drm_fb_helper.c | 8 ++++++-- > include/drm/drm_crtc.h | 3 +++ > 4 files changed, 25 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index ba1507036f26..117b186fe646 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -3512,6 +3512,10 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip_target); > * 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 the GAMMA_LUT or DEGAMMA_LUT property for the gamma table. > + * GAMMA_LUT property is used if it exists, and DEGAMMA_LUT property is used as > + * a fallback. > */ > int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, > u16 *red, u16 *green, u16 *blue, > @@ -3526,6 +3530,9 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, > int i, ret = 0; > bool replaced; > > + if (!crtc->has_gamma_prop && !crtc->has_degamma_prop) > + return -ENODEV; > + > state = drm_atomic_state_alloc(crtc->dev); > if (!state) > return -ENOMEM; > @@ -3554,10 +3561,12 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, > goto fail; > } > > - /* Reset DEGAMMA_LUT and CTM properties. */ > - replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); > + /* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */ > + replaced = drm_property_replace_blob(&crtc_state->degamma_lut, > + crtc->has_gamma_prop ? NULL : blob); > 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, > + crtc->has_gamma_prop ? blob : NULL); > crtc_state->color_mgmt_changed |= replaced; > > ret = drm_atomic_commit(state); > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c > index 3bcabc2f6e0e..956e59d5f6a7 100644 > --- a/drivers/gpu/drm/drm_color_mgmt.c > +++ b/drivers/gpu/drm/drm_color_mgmt.c > @@ -176,6 +176,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > degamma_lut_size); > } > > + crtc->has_degamma_prop = !!degamma_lut_size; > + > if (has_ctm) > drm_object_attach_property(&crtc->base, > config->ctm_property, 0); > @@ -187,6 +189,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > config->gamma_lut_size_property, > gamma_lut_size); > } > + > + crtc->has_gamma_prop = !!gamma_lut_size; > } > EXPORT_SYMBOL(drm_crtc_enable_color_mgmt); > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 25edf670867c..b0906ef97617 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1001,6 +1001,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) > drm_client_for_each_modeset(modeset, &fb_helper->client) { > crtc = modeset->crtc; > > + if (!crtc->has_gamma_prop && !crtc->has_degamma_prop) > + continue; > + > if (!gamma_lut) > gamma_lut = setcmap_new_gamma_lut(crtc, cmap); > if (IS_ERR(gamma_lut)) { > @@ -1015,11 +1018,12 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) > goto out_state; > } > > + /* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */ > replaced = drm_property_replace_blob(&crtc_state->degamma_lut, > - NULL); > + crtc->has_gamma_prop ? NULL : gamma_lut); > replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); > replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, > - gamma_lut); > + crtc->has_gamma_prop ? gamma_lut : NULL); > crtc_state->color_mgmt_changed |= replaced; > } > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index ba839e5e357d..4d9e217e5040 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1084,6 +1084,9 @@ struct drm_crtc { > */ > uint16_t *gamma_store; > > + bool has_gamma_prop : 1; > + bool has_degamma_prop : 1; > + > /** @helper_private: mid-layer private data */ > const struct drm_crtc_helper_funcs *helper_private; >
On Tue, Dec 08, 2020 at 05:55:11PM +0200, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Tue, Dec 08, 2020 at 03:57:58PM +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 fixes the issue by changing > > drm_atomic_helper_legacy_gamma_set() so that GAMMA_LUT will be used if > > it exists, and DEGAMMA_LUT will be used as a fallback. > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++++++--- > > drivers/gpu/drm/drm_color_mgmt.c | 4 ++++ > > drivers/gpu/drm/drm_fb_helper.c | 8 ++++++-- > > include/drm/drm_crtc.h | 3 +++ > > 4 files changed, 25 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index ba1507036f26..117b186fe646 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -3512,6 +3512,10 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip_target); > > * 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 the GAMMA_LUT or DEGAMMA_LUT property for the gamma table. > > + * GAMMA_LUT property is used if it exists, and DEGAMMA_LUT property is used as > > + * a fallback. > > */ > > int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, > > u16 *red, u16 *green, u16 *blue, > > @@ -3526,6 +3530,9 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, > > int i, ret = 0; > > bool replaced; > > > > + if (!crtc->has_gamma_prop && !crtc->has_degamma_prop) > > + return -ENODEV; > > + > > state = drm_atomic_state_alloc(crtc->dev); > > if (!state) > > return -ENOMEM; > > @@ -3554,10 +3561,12 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, > > goto fail; > > } > > > > - /* Reset DEGAMMA_LUT and CTM properties. */ > > - replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); > > + /* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */ > > + replaced = drm_property_replace_blob(&crtc_state->degamma_lut, > > + crtc->has_gamma_prop ? NULL : blob); > > 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, > > + crtc->has_gamma_prop ? blob : NULL); > > crtc_state->color_mgmt_changed |= replaced; > > > > ret = drm_atomic_commit(state); > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c > > index 3bcabc2f6e0e..956e59d5f6a7 100644 > > --- a/drivers/gpu/drm/drm_color_mgmt.c > > +++ b/drivers/gpu/drm/drm_color_mgmt.c > > @@ -176,6 +176,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > > degamma_lut_size); > > } > > > > + crtc->has_degamma_prop = !!degamma_lut_size; > > + > > if (has_ctm) > > drm_object_attach_property(&crtc->base, > > config->ctm_property, 0); > > @@ -187,6 +189,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, > > config->gamma_lut_size_property, > > gamma_lut_size); > > } > > + > > + crtc->has_gamma_prop = !!gamma_lut_size; > > } > > EXPORT_SYMBOL(drm_crtc_enable_color_mgmt); > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > index 25edf670867c..b0906ef97617 100644 > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -1001,6 +1001,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) > > drm_client_for_each_modeset(modeset, &fb_helper->client) { > > crtc = modeset->crtc; > > > > + if (!crtc->has_gamma_prop && !crtc->has_degamma_prop) > > + continue; > > + > > if (!gamma_lut) > > gamma_lut = setcmap_new_gamma_lut(crtc, cmap); > > if (IS_ERR(gamma_lut)) { > > @@ -1015,11 +1018,12 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) > > goto out_state; > > } > > > > + /* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */ > > replaced = drm_property_replace_blob(&crtc_state->degamma_lut, > > - NULL); > > + crtc->has_gamma_prop ? NULL : gamma_lut); > > replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); > > replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, > > - gamma_lut); > > + crtc->has_gamma_prop ? gamma_lut : NULL); > > crtc_state->color_mgmt_changed |= replaced; > > } > > > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > > index ba839e5e357d..4d9e217e5040 100644 > > --- a/include/drm/drm_crtc.h > > +++ b/include/drm/drm_crtc.h > > @@ -1084,6 +1084,9 @@ struct drm_crtc { > > */ > > uint16_t *gamma_store; > > > > + bool has_gamma_prop : 1; > > + bool has_degamma_prop : 1; I'm a bit behind on patches, but in case this got missed please remove this and replace with the (obj, prop) lookup function thing or something like that. Makes sure everything stays in sync, plus like I said atomic uses this a ton. So not a problem here. -Daniel > > + > > /** @helper_private: mid-layer private data */ > > const struct drm_crtc_helper_funcs *helper_private; > > > > -- > Regards, > > Laurent Pinchart
Hi Daniel, On 09/12/2020 02:51, Daniel Vetter wrote: >>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >>> index ba839e5e357d..4d9e217e5040 100644 >>> --- a/include/drm/drm_crtc.h >>> +++ b/include/drm/drm_crtc.h >>> @@ -1084,6 +1084,9 @@ struct drm_crtc { >>> */ >>> uint16_t *gamma_store; >>> >>> + bool has_gamma_prop : 1; >>> + bool has_degamma_prop : 1; > > I'm a bit behind on patches, but in case this got missed please remove > this and replace with the (obj, prop) lookup function thing or something > like that. Makes sure everything stays in sync, plus like I said atomic > uses this a ton. So not a problem here. The drm_mode_obj_find_prop_id() is in core drm.ko, and not exported, but I need it also from drm_kms_helper.ko. drm_mode_obj_find_prop_id() is declared in drm_crtc_internal.h. Are those functions supposed to be not exported from drm.ko? So, is it fine to just export drm_mode_obj_find_prop_id? If I export, should I move it to drm_mode_object.h? Tomi
On Wed, Dec 9, 2020 at 12:17 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > Hi Daniel, > > On 09/12/2020 02:51, Daniel Vetter wrote: > > >>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > >>> index ba839e5e357d..4d9e217e5040 100644 > >>> --- a/include/drm/drm_crtc.h > >>> +++ b/include/drm/drm_crtc.h > >>> @@ -1084,6 +1084,9 @@ struct drm_crtc { > >>> */ > >>> uint16_t *gamma_store; > >>> > >>> + bool has_gamma_prop : 1; > >>> + bool has_degamma_prop : 1; > > > > I'm a bit behind on patches, but in case this got missed please remove > > this and replace with the (obj, prop) lookup function thing or something > > like that. Makes sure everything stays in sync, plus like I said atomic > > uses this a ton. So not a problem here. > > The drm_mode_obj_find_prop_id() is in core drm.ko, and not exported, but I need it also from > drm_kms_helper.ko. > > drm_mode_obj_find_prop_id() is declared in drm_crtc_internal.h. Are those functions supposed to be > not exported from drm.ko? > > So, is it fine to just export drm_mode_obj_find_prop_id? If I export, should I move it to > drm_mode_object.h? It's not exported to drivers so they don't play games in their own ioctls or do something else than the atomic set/get_property hooks. I guess we could export, but I think here the better solution is to lift the legacy gamma compat function to core code status, since I think that's the direction we want to go anyway. Bit more churn since the name changes. -Daniel
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ba1507036f26..117b186fe646 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3512,6 +3512,10 @@ EXPORT_SYMBOL(drm_atomic_helper_page_flip_target); * 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 the GAMMA_LUT or DEGAMMA_LUT property for the gamma table. + * GAMMA_LUT property is used if it exists, and DEGAMMA_LUT property is used as + * a fallback. */ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green, u16 *blue, @@ -3526,6 +3530,9 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, int i, ret = 0; bool replaced; + if (!crtc->has_gamma_prop && !crtc->has_degamma_prop) + return -ENODEV; + state = drm_atomic_state_alloc(crtc->dev); if (!state) return -ENOMEM; @@ -3554,10 +3561,12 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc, goto fail; } - /* Reset DEGAMMA_LUT and CTM properties. */ - replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); + /* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */ + replaced = drm_property_replace_blob(&crtc_state->degamma_lut, + crtc->has_gamma_prop ? NULL : blob); 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, + crtc->has_gamma_prop ? blob : NULL); crtc_state->color_mgmt_changed |= replaced; ret = drm_atomic_commit(state); diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 3bcabc2f6e0e..956e59d5f6a7 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -176,6 +176,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, degamma_lut_size); } + crtc->has_degamma_prop = !!degamma_lut_size; + if (has_ctm) drm_object_attach_property(&crtc->base, config->ctm_property, 0); @@ -187,6 +189,8 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc, config->gamma_lut_size_property, gamma_lut_size); } + + crtc->has_gamma_prop = !!gamma_lut_size; } EXPORT_SYMBOL(drm_crtc_enable_color_mgmt); diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 25edf670867c..b0906ef97617 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1001,6 +1001,9 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) drm_client_for_each_modeset(modeset, &fb_helper->client) { crtc = modeset->crtc; + if (!crtc->has_gamma_prop && !crtc->has_degamma_prop) + continue; + if (!gamma_lut) gamma_lut = setcmap_new_gamma_lut(crtc, cmap); if (IS_ERR(gamma_lut)) { @@ -1015,11 +1018,12 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) goto out_state; } + /* Set GAMMA/DEGAMMA_LUT and reset DEGAMMA/GAMMA_LUT and CTM */ replaced = drm_property_replace_blob(&crtc_state->degamma_lut, - NULL); + crtc->has_gamma_prop ? NULL : gamma_lut); replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, - gamma_lut); + crtc->has_gamma_prop ? gamma_lut : NULL); crtc_state->color_mgmt_changed |= replaced; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index ba839e5e357d..4d9e217e5040 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1084,6 +1084,9 @@ struct drm_crtc { */ uint16_t *gamma_store; + bool has_gamma_prop : 1; + bool has_degamma_prop : 1; + /** @helper_private: mid-layer private data */ const struct drm_crtc_helper_funcs *helper_private;
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 fixes the issue by changing drm_atomic_helper_legacy_gamma_set() so that GAMMA_LUT will be used if it exists, and DEGAMMA_LUT will be used as a fallback. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/drm_atomic_helper.c | 15 ++++++++++++--- drivers/gpu/drm/drm_color_mgmt.c | 4 ++++ drivers/gpu/drm/drm_fb_helper.c | 8 ++++++-- include/drm/drm_crtc.h | 3 +++ 4 files changed, 25 insertions(+), 5 deletions(-)