[Intel-gfx] [RFC v1 1/7] drm/i915: Add gamma mode property

Thu Mar 21 21:19:42 UTC 2019

On Tue, Mar 19, 2019 at 02:00:12PM +0530, Uma Shankar wrote:
> Gen platforms support multiple gamma modes, currently
> it's hard coded to operate only in 1 specific mode.
> This patch adds a property to make gamma mode programmable.
> User can select which mode should be used for a particular
> usecase or scenario.
> 
> Signed-off-by: Uma Shankar <uma.shankar at intel.com>

I haven't reviewed the series in depth, but I'm a bit confused on how
userspace would approach using this property.  This seems to be exposing
hardware implementation details that I wouldn't expect userspace to need
to worry about (plus I don't think any of the property values here
convey any specific meaning to someone who hasn't read the Intel
bspec/PRM).  E.g., why does userspace care about "split gamma" when the
driver takes care of the programming details and userspace never sees
the actual way the registers are laid out and written?

My understanding is that what really matters is how many table entries
there are for userspace to fill in, what input range(s) they cover, and
how the bits of each table entry are interpreted.  I think we'd want to
handle this in a vendor-agnostic way in the DRM core if possible; most
of the display servers that get used these days are cross-platform and
probably won't want to add Intel-specific logic (or platform-specific
logic if we wind up with a different set of options on future Intel
platforms).

> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  2 ++
>  drivers/gpu/drm/i915/intel_color.c | 46 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h   |  3 +++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c65c2e6..02231ae 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1735,6 +1735,8 @@ struct drm_i915_private {
>  	struct drm_property *broadcast_rgb_property;
>  	struct drm_property *force_audio_property;
>  
> +	struct drm_property *gamma_mode_property;
> +
>  	/* hda/i915 audio component */
>  	struct i915_audio_component *audio_component;
>  	bool audio_component_registered;
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 467fd1a..9d43d19 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -92,6 +92,19 @@
>  	0x0800, 0x0100, 0x0800,
>  };
>  
> +#define LEGACY_PALETTE_MODE_8BIT		BIT(0)
> +#define PRECISION_PALETTE_MODE_10BIT		BIT(1)
> +#define INTERPOLATED_GAMMA_MODE_12BIT		BIT(2)
> +#define MULTI_SEGMENTED_GAMMA_MODE_12BIT	BIT(3)
> +#define SPLIT_GAMMA_MODE_12BIT			BIT(4)
> +
> +#define INTEL_GAMMA_MODE_MASK (\
> +		LEGACY_PALETTE_MODE_8BIT | \
> +		PRECISION_PALETTE_MODE_10BIT | \
> +		INTERPOLATED_GAMMA_MODE_12BIT | \
> +		MULTI_SEGMENTED_GAMMA_MODE_12BIT | \
> +		BIT_SPLIT_GAMMA_MODE_12BIT)

Is the "BIT_" prefix on this last one a typo?  I assume this was
supposed to just be the SPLIT_GAMMA_MODE_12BIT defined above?

> +
>  static bool lut_is_legacy(const struct drm_property_blob *lut)
>  {
>  	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
> @@ -105,6 +118,37 @@ static bool crtc_state_is_legacy_gamma(const struct intel_crtc_state *crtc_state
>  		lut_is_legacy(crtc_state->base.gamma_lut);
>  }
>  
> +static const struct drm_prop_enum_list gamma_mode_supported[] = {
> +	{ LEGACY_PALETTE_MODE_8BIT, "8 Bit Legacy Palette Mode" },
> +	{ PRECISION_PALETTE_MODE_10BIT, "10 Bit Precision Palette Mode" },
> +	{ INTERPOLATED_GAMMA_MODE_12BIT, "12 Bit Interploated Gamma Mode" },
> +	{ MULTI_SEGMENTED_GAMMA_MODE_12BIT,
> +			"12 Bit Multi Segmented Gamma Mode" },
> +	{ SPLIT_GAMMA_MODE_12BIT, "12 Bit Split Gamma Mode" },
> +};
> +
> +void
> +intel_attach_gamma_mode_property(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_property *prop;
> +
> +	prop = dev_priv->gamma_mode_property;
> +	if (!prop) {
> +		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
> +						"Gamma Mode",
> +						gamma_mode_supported,
> +						ARRAY_SIZE(gamma_mode_supported));

If we do expose hardware-specific gamma modes like this, then I think
we'd want to create this property with a platform-specific list of
modes so that userspace doesn't even have the options for modes that
aren't supported on the platform they're running on.

> +		if (!prop)
> +			return;
> +
> +		dev_priv->gamma_mode_property = prop;
> +	}
> +
> +	drm_object_attach_property(&crtc->base.base, prop, 0);
> +}
> +
>  /*
>   * When using limited range, multiply the matrix given by userspace by
>   * the matrix that we would use for the limited range.
> @@ -907,4 +951,6 @@ void intel_color_init(struct intel_crtc *crtc)
>  					   INTEL_INFO(dev_priv)->color.degamma_lut_size,
>  					   true,
>  					   INTEL_INFO(dev_priv)->color.gamma_lut_size);
> +
> +	intel_attach_gamma_mode_property(crtc);

It looks like we're exposing the property to userspace in this patch,
but we don't finish wiring up the functionality until later patches in
the series; that's going to make things confusing if someone bisects
over this range of patches.  It would be best to hold off on exposing
new interfaces like this to userspace until the end of the
implementation when they're fully functional.

Matt

>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d9f188e..fd84fe9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1034,6 +1034,9 @@ struct intel_crtc_state {
>  	u8 nv12_planes;
>  	u8 c8_planes;
>  
> +	/* Gamma mode type programmed on the pipe */
> +	u32 gamma_mode_type;
> +
>  	/* bitmask of planes that will be updated during the commit */
>  	u8 update_planes;
>  
> -- 
> 1.9.1
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795