>-----Original Message----- >From: Harry Wentland [mailto:harry.wentland@xxxxxxx] >Sent: Saturday, February 17, 2018 3:07 AM >To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; Daniele Castagna ><dcastagna@xxxxxxxxxx> >Cc: Shankar, Uma <uma.shankar@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx >Subject: Re: [PATCH 04/10] drm: Add Plane Gamma properties > >On 2018-02-16 03:10 PM, Ville Syrjälä wrote: >> On Thu, Feb 15, 2018 at 02:45:29PM -0500, Daniele Castagna wrote: >>> rk3399 has the option of setting per-plane gamma. >>> The YUV2YUV registers are per-plane. Each plane YUV2YUV pipeline has >>> at least 3 optional stages, two of those being degamma and gamma, and >>> they can be configured per-plane. >>> > >I don't mind having a per-plane gamma, especially since rk3399 has it in HW. It >just seemed a bit curious to me and I'd rather avoid properties that would never >be used by any driver. > >>> I'm not sure about Intel HW. I think they might have something >>> similar planned, considering the original patch was from uma.shankar. >>> CCing directly in case he knows more. >> >> IIRC some of out upcoming stuff will have a pipeline like >> 'csc->degamma->csc->gamma->blender'. I don't really know what the >> point of the post csc gamma is though. Normally you would not want to >> reapply any gamma prior to blending. >> >> The only use case I can think of would be if you don't have a gamma >> lut in the crtc for post blend gamma. In that case I suppose you might >> consider doing the gamma before blending and accepting the slightly >> incorrect blending results. But at least on our hardware we always >> have a gamma lut in the crtc as well. > >That's what I was thinking of as the only possible use-case as well. > The primary use case of the post CSC plane gamma LUT will be to do tone mapping in case of HDR data. Again the accuracy of it will depend on the number of LUT samples and precision. It could be used, if for some strange use case we want non-linear blending. I believe Android was using non-linear blending by design (not sure if it changed in recent variants). Also as Ville mentioned, if we lack gamma at pipe level on certain hardware, this may be required. But tone mapping is the primary use case. Regards, Uma Shankar >Harry > >> >> So yeah, I don't really have any reason why we'd need to actually to >> expose the per-plane gamma. Some "crazy" artistic use case perhaps? >> >> I'm totally fine not exposing it until someone comes up with an actual >> use for it. >> >> Does rk3399 have a dedicated csc for yuv->rgb before degamma? Or are >> you supposed to use the same csc for that as you'd use for ctm? In >> that case I can understand why the hw would have a gamm lut on each >> side of the csc. But it would also means that you can't do yuv->rgb >> and ctm at the same time unless you're fine with doing it wrong. >> >>> >>> On Thu, Feb 15, 2018 at 2:29 PM, Harry Wentland ><harry.wentland@xxxxxxx> wrote: >>>> On 2018-02-15 12:32 AM, Daniele Castagna wrote: >>>>> From: "uma.shankar at intel.com (Uma Shankar)" >>>>> <uma.shankar@xxxxxxxxx> >>>>> >>>>> Add plane gamma as blob property and size as a range property. >>>>> >>>> >>>> Plane degamma & CTM make sense to me but I'm not sure why gamma >would be on a per-plane basis. That said, HW sometimes has these implemented >in odd ways. Do we know of any HW that will currently or in the future need per- >plane gamma or are we just trying to cover all potentialities? >>>> >>>> Harry >>>> >>>>> (am from https://patchwork.kernel.org/patch/9971325/) >>>>> >>>>> Change-Id: I606cd40c9748b136fc2bf4750bea1da285add62d >>>>> Signed-off-by: Uma Shankar <uma.shankar at intel.com> >>>>> --- >>>>> drivers/gpu/drm/drm_atomic.c | 8 ++++++++ >>>>> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++ >>>>> drivers/gpu/drm/drm_mode_config.c | 14 ++++++++++++++ >>>>> include/drm/drm_mode_config.h | 11 +++++++++++ >>>>> include/drm/drm_plane.h | 9 +++++++++ >>>>> 5 files changed, 46 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_atomic.c >>>>> b/drivers/gpu/drm/drm_atomic.c index d4b8c6cc84128..f634f6450f415 >>>>> 100644 >>>>> --- a/drivers/gpu/drm/drm_atomic.c >>>>> +++ b/drivers/gpu/drm/drm_atomic.c >>>>> @@ -778,6 +778,12 @@ static int drm_atomic_plane_set_property(struct >drm_plane *plane, >>>>> &replaced); >>>>> state->color_mgmt_changed |= replaced; >>>>> return ret; >>>>> + } else if (property == config->plane_gamma_lut_property) { >>>>> + ret = drm_atomic_replace_property_blob_from_id(dev, >>>>> + &state->gamma_lut, >>>>> + val, -1, &replaced); >>>>> + state->color_mgmt_changed |= replaced; >>>>> + return ret; >>>>> } else { >>>>> return -EINVAL; >>>>> } >>>>> @@ -841,6 +847,8 @@ drm_atomic_plane_get_property(struct drm_plane >*plane, >>>>> state->degamma_lut->base.id : 0; >>>>> } else if (property == config->plane_ctm_property) { >>>>> *val = (state->ctm) ? state->ctm->base.id : 0; >>>>> + } else if (property == config->plane_gamma_lut_property) { >>>>> + *val = (state->gamma_lut) ? state->gamma_lut->base.id >>>>> + : 0; >>>>> } else { >>>>> return -EINVAL; >>>>> } >>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c >>>>> b/drivers/gpu/drm/drm_atomic_helper.c >>>>> index 17e137a529a0e..97dbb36153d14 100644 >>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>>> @@ -3495,6 +3495,9 @@ void >__drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, >>>>> drm_property_reference_blob(state->degamma_lut); >>>>> if (state->ctm) >>>>> drm_property_reference_blob(state->ctm); >>>>> + if (state->gamma_lut) >>>>> + drm_property_reference_blob(state->gamma_lut); >>>>> + >>>>> state->color_mgmt_changed = false; } >>>>> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); >>>>> @@ -3543,6 +3546,7 @@ void >>>>> __drm_atomic_helper_plane_destroy_state(struct drm_plane_state >>>>> *state) >>>>> >>>>> drm_property_unreference_blob(state->degamma_lut); >>>>> drm_property_unreference_blob(state->ctm); >>>>> + drm_property_unreference_blob(state->gamma_lut); >>>>> } >>>>> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c >>>>> b/drivers/gpu/drm/drm_mode_config.c >>>>> index c8763977413e7..bc6f8e51c7464 100644 >>>>> --- a/drivers/gpu/drm/drm_mode_config.c >>>>> +++ b/drivers/gpu/drm/drm_mode_config.c >>>>> @@ -368,6 +368,20 @@ static int >drm_mode_create_standard_properties(struct drm_device *dev) >>>>> return -ENOMEM; >>>>> dev->mode_config.plane_ctm_property = prop; >>>>> >>>>> + prop = drm_property_create(dev, >>>>> + DRM_MODE_PROP_BLOB, >>>>> + "PLANE_GAMMA_LUT", 0); >>>>> + if (!prop) >>>>> + return -ENOMEM; >>>>> + dev->mode_config.plane_gamma_lut_property = prop; >>>>> + >>>>> + prop = drm_property_create_range(dev, >>>>> + DRM_MODE_PROP_IMMUTABLE, >>>>> + "PLANE_GAMMA_LUT_SIZE", 0, UINT_MAX); >>>>> + if (!prop) >>>>> + return -ENOMEM; >>>>> + dev->mode_config.plane_gamma_lut_size_property = prop; >>>>> + >>>>> return 0; >>>>> } >>>>> >>>>> diff --git a/include/drm/drm_mode_config.h >>>>> b/include/drm/drm_mode_config.h index ad7235ced531b..3ca3eb3c98950 >>>>> 100644 >>>>> --- a/include/drm/drm_mode_config.h >>>>> +++ b/include/drm/drm_mode_config.h >>>>> @@ -740,6 +740,17 @@ struct drm_mode_config { >>>>> * degamma LUT. >>>>> */ >>>>> struct drm_property *plane_ctm_property; >>>>> + /** >>>>> + * @plane_gamma_lut_property: Optional Plane property to set the LUT >>>>> + * used to convert the colors, after the CTM matrix, to the common >>>>> + * gamma space chosen for blending. >>>>> + */ >>>>> + struct drm_property *plane_gamma_lut_property; >>>>> + /** >>>>> + * @plane_gamma_lut_size_property: Optional Plane property for the >size >>>>> + * of the gamma LUT as supported by the driver (read-only). >>>>> + */ >>>>> + struct drm_property *plane_gamma_lut_size_property; >>>>> /** >>>>> * @ctm_property: Optional CRTC property to set the >>>>> * matrix used to convert colors after the lookup in the diff >>>>> --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index >>>>> 21aecc9c91a09..acabb85009a14 100644 >>>>> --- a/include/drm/drm_plane.h >>>>> +++ b/include/drm/drm_plane.h >>>>> @@ -147,6 +147,15 @@ struct drm_plane_state { >>>>> */ >>>>> struct drm_property_blob *ctm; >>>>> >>>>> + /** >>>>> + * @gamma_lut: >>>>> + * >>>>> + * Lookup table for converting pixel data after the color conversion >>>>> + * matrix @ctm. See drm_plane_enable_color_mgmt(). The blob (if not >>>>> + * NULL) is an array of &struct drm_color_lut. >>>>> + */ >>>>> + struct drm_property_blob *gamma_lut; >>>>> + >>>>> struct drm_atomic_state *state; >>>>> >>>>> bool color_mgmt_changed : 1; >>>>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@xxxxxxxxxxxxxxxxxxxxx >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
