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. 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
