| Message ID | 20180215053300.70482-5-dcastagna@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018-02-15 12:32 AM, Daniele Castagna wrote: > From: "uma.shankar at intel.com (Uma Shankar)" <uma.shankar@intel.com> > > 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; >
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'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. On Thu, Feb 15, 2018 at 2:29 PM, Harry Wentland <harry.wentland@amd.com> wrote: > On 2018-02-15 12:32 AM, Daniele Castagna wrote: >> From: "uma.shankar at intel.com (Uma Shankar)" <uma.shankar@intel.com> >> >> 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; >>
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'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. 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@amd.com> wrote: > > On 2018-02-15 12:32 AM, Daniele Castagna wrote: > >> From: "uma.shankar at intel.com (Uma Shankar)" <uma.shankar@intel.com> > >> > >> 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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@amd.com> wrote: >>> On 2018-02-15 12:32 AM, Daniele Castagna wrote: >>>> From: "uma.shankar at intel.com (Uma Shankar)" <uma.shankar@intel.com> >>>> >>>> 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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
>-----Original Message----- >From: Harry Wentland [mailto:harry.wentland@amd.com] >Sent: Saturday, February 17, 2018 3:07 AM >To: Ville Syrjälä <ville.syrjala@linux.intel.com>; Daniele Castagna ><dcastagna@google.com> >Cc: Shankar, Uma <uma.shankar@intel.com>; dri-devel@lists.freedesktop.org >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@amd.com> wrote: >>>> On 2018-02-15 12:32 AM, Daniele Castagna wrote: >>>>> From: "uma.shankar at intel.com (Uma Shankar)" >>>>> <uma.shankar@intel.com> >>>>> >>>>> 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@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>
On Thu, Feb 15, 2018 at 12:32:54AM -0500, Daniele Castagna wrote: > From: "uma.shankar at intel.com (Uma Shankar)" <uma.shankar@intel.com> > > Add plane gamma as blob property and size as a > range property. > > (am from https://patchwork.kernel.org/patch/9971325/) > > Change-Id: I606cd40c9748b136fc2bf4750bea1da285add62d > Signed-off-by: Uma Shankar <uma.shankar at intel.com> Please also add kerneldoc for the properties itself, see https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#color-management-properties That means we need a bunch of rewording, now that we're adding planes. I also think a small diagram that shows the full pipeline (which probably no hw ever implements) would be really good. See the various inlined dot files we have in the KMS documentation already. Thanks, Daniel > --- > 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; > -- > 2.16.1.291.g4437f3f132-goog > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Feb 15, 2018 at 12:32:54AM -0500, Daniele Castagna wrote: > From: "uma.shankar at intel.com (Uma Shankar)" <uma.shankar@intel.com> > > Add plane gamma as blob property and size as a > range property. > > (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; Same comments here about drm_property location. > /** > * @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; Will 16-bit be sufficient here as well, or will we want to use the 32-bit variant that is required for degamma? Sean > + > struct drm_atomic_state *state; > > bool color_mgmt_changed : 1; > -- > 2.16.1.291.g4437f3f132-goog > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Feb 27, 2018 at 10:26:30AM -0500, Sean Paul wrote: > On Thu, Feb 15, 2018 at 12:32:54AM -0500, Daniele Castagna wrote: > > From: "uma.shankar at intel.com (Uma Shankar)" <uma.shankar@intel.com> > > > > Add plane gamma as blob property and size as a > > range property. > > > > (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; > > Same comments here about drm_property location. > > > /** > > * @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; > > Will 16-bit be sufficient here as well, or will we want to use the 32-bit variant > that is required for degamma? Someone proposing expanding the luts beying u0.16? I have been recently thinking about making the luts support values outside the [0.0,1.0) interval by stealing bits from the 'u16 reserved' have in drm_color_lut. At least modern intel hw can do up to [0.0,8.0) (and it also mirrors the values across the origin for negative inputs). Now, I don't have an immediate use for that but I think some xvYCC type of stuff with out of gamut values is one potential use case. There are potentially 5 bits per component to use, so s5.16 might be the thing I'd consider. That would cover the capabilities of intel hw. But if people are thinking that the .16 is not enough then I supposer we might have to consider a totally new property with higher precision lut entries, in which case extending the current thing doesn't make much sense. Another thing I've been sketching out in my head is some kind of enum property that actually describes the diffrent modes the luts/ctm support. This would allow the client to eg. select between modes with a proper LUT with lower precision vs. an interpolated curve with higher precision. At least on intel hw the crtc luts support several different modes. My current plan is that each mode would consist of a set of intervals, where each interval a small structure which describes how the hardware operates on input values in that range. Each set of intervals would be exposed as a blob, and the blobs would be exposed via an enum property. Such a blob enum property wouldn't allow the user to provide and arbitrary blob and instead the blob id must match one of the enum values.
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;