[Intel-gfx] [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode

[Shankar, Uma]

>-----Original Message-----
>From: Syrjala, Ville
>Sent: Tuesday, March 19, 2019 10:29 PM
>To: Lankhorst, Maarten <maarten.lankhorst at intel.com>
>Cc: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org;
>Sharma, Shashank <shashank.sharma at intel.com>; Roper, Matthew D
><matthew.d.roper at intel.com>
>Subject: Re: [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode
>
>On Tue, Mar 19, 2019 at 10:46:27AM +0200, Lankhorst, Maarten wrote:
>> tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
>> > Multi Segment Gamma Mode is added in Gen11+ platforms.
>> > Added a property interface to enable that.
>> >
>> > Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>> >  drivers/gpu/drm/i915/intel_color.c | 23 +++++++++++++++++++++++
>> >  include/uapi/drm/i915_drm.h        | 14 ++++++++++++++
>> >  3 files changed, 38 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> > b/drivers/gpu/drm/i915/i915_drv.h index 02231ae..f20d418 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -1736,6 +1736,7 @@ struct drm_i915_private {
>> >  	struct drm_property *force_audio_property;
>> >
>> >  	struct drm_property *gamma_mode_property;
>> > +	struct drm_property *multi_segment_gamma_mode_property;
>>
>> Seems to me both properties should be part of drm core?

Sure Maarten, we can move gamma_mode property to drm.

>>
>> >  	/* hda/i915 audio component */
>> >  	struct i915_audio_component *audio_component; diff --git
>> > a/drivers/gpu/drm/i915/intel_color.c
>> > b/drivers/gpu/drm/i915/intel_color.c
>> > index 9d43d19..399d63d 100644
>> > --- a/drivers/gpu/drm/i915/intel_color.c
>> > +++ b/drivers/gpu/drm/i915/intel_color.c
>> > @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
>> > struct intel_crtc_state *crtc_state
>> >  	drm_object_attach_property(&crtc->base.base, prop, 0);  }
>> >
>> > +void
>> > +intel_attach_multi_segment_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->multi_segment_gamma_mode_property;
>> > +	if (!prop) {
>> > +		prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
>> > +					   "Multi-segment Gamma",
>> > 0);
>> > +		if (!prop)
>> > +			return;
>> > +
>> > +		dev_priv->multi_segment_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.
>> > @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
>> >  					   INTEL_INFO(dev_priv)-
>> > >color.gamma_lut_size);
>> >
>> >  	intel_attach_gamma_mode_property(crtc);
>> > +
>> > +	if (INTEL_GEN(dev_priv) >= 11)
>> > +		intel_attach_multi_segment_gamma_mode_property(crtc)
>> > ;
>> >  }
>> > diff --git a/include/uapi/drm/i915_drm.h
>> > b/include/uapi/drm/i915_drm.h index aa2d4c7..8f1974e 100644
>> > --- a/include/uapi/drm/i915_drm.h
>> > +++ b/include/uapi/drm/i915_drm.h
>> > @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
>> >  	__u8 data[];
>> >  };
>> >
>> > +/*
>> > + * Structure for muti segmented gamma lut  */ struct
>> > +multi_segment_gamma_lut {
>> > +	/* Number of Lut Segments */
>> > +	__u8 segment_cnt;
>> > +	/* Precison of LUT entries in bits */
>> > +	__u8 precision_bits;
>> > +	/* Pointer having number of LUT elements in each segment */
>> > +	__u32 *segment_lut_cnt_ptr;
>> > +	/* Pointer to store exact lut values for each segment */
>> > +	__u32 *segment_lut_ptr;
>> > +};
>> >
>> And perhaps a variation of this as description for all gamma mode
>> types.
>
>This is my old idea how to represent fancier LUTs:
>https://github.com/vsyrjala/linux/commit/1aab7625dca77b831e05e32af17904c2130
>0ff95
>https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb687bb0ae5a
>a58f
>
>Each distinct segment of the curve would be just described by a fixed size range
>descriptor, and the entire blob would be made up of however many of those are
>needed.
>
>That way we don't even need any multi-segment uapi for setting up the LUT. The blob
>for that would just contain as many entries as the LUT needs in that specific mode.

Hi Ville,
This also sounds good.  What is your suggestion on this. Any recommendation on how to take this
forward.

Thanks & Regards,
Uma Shankar

>--
>Ville Syrjälä
>Intel