Re: [PATCH 04/10] drm: Add Plane Gamma properties

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel