On 07/12/2020 17:31, Ville Syrjälä wrote:
> On Sat, Dec 05, 2020 at 12:35:25AM +0200, Laurent Pinchart wrote:
>> Hi Tomi,
>>
>> Thank you for the patch.
>>
>> On Thu, Dec 03, 2020 at 01:48:44PM +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@xxxxxx>
>>> ---
>>> drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++++++++---
>>> drivers/gpu/drm/drm_color_mgmt.c | 4 ++++
>>> include/drm/drm_crtc.h | 3 +++
>>> 3 files changed, 22 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>> index ba1507036f26..fe59c8ea42a9 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,
>>> @@ -3525,6 +3529,12 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>>> struct drm_color_lut *blob_data;
>>> int i, ret = 0;
>>> bool replaced;
>>> + bool use_degamma;
>>> +
>>> + if (!crtc->has_gamma_prop && !crtc->has_degamma_prop)
>>> + return -ENODEV;
>>> +
>>> + use_degamma = !crtc->has_gamma_prop;
>>>
>>> state = drm_atomic_state_alloc(crtc->dev);
>>> if (!state)
>>> @@ -3554,10 +3564,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,
>>> + 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);
>>> 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/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>> index ba839e5e357d..9e1f06047e3d 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;
>>> + bool has_degamma_prop;
>>
>> We could use a bitfield to save a bit of memory. Apart from that, the
>> patch looks good to me.
>
> Or we could just check if the crtc has the relevant prop or not.
That's what I did at first, but it requires iterating over the props (unless I missed a simple way
to just check it). Probably not noticeable (in performance) but just felt a bit ugly.
Tomi
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
