Re: [PATCH v2 1/2] drm: add legacy support for using degamma for gamma
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
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/2] drm: add legacy support for using degamma for gamma
On Wed, Dec 9, 2020 at 12:17 PM Tomi Valkeinen 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/2] drm: add legacy support for using degamma for gamma
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
>
> Reviewed-by: Laurent Pinchart
>
> > ---
> > 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)
> > +
Re: [PATCH v2 1/2] drm: add legacy support for using degamma for gamma
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
Reviewed-by: Laurent Pinchart
> ---
> 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
