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

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

On Wed, Mar 20, 2019 at 05:03:16PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Syrjala, Ville
> >Sent: Tuesday, March 19, 2019 10:29 PM
> >To: Lankhorst, Maarten <maarten.lankhorst@intel.com>
> >Cc: Shankar, Uma <uma.shankar@intel.com>; intel-gfx@lists.freedesktop.org;
> >Sharma, Shashank <shashank.sharma@intel.com>; Roper, Matthew D
> ><matthew.d.roper@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@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.

I think my design should be more or less feasible to use for
userspace. I didn't actually get as far as to write userspace
code for it, but my idea for x11 was something along these lines:

1) find a non-interpolated gamma mode with num_entries >= 1<<screen bpc
2) if none was found pick an interpolated gamma mode with
   input/output bpc >= screen bpc
3) fall back to whatever is left

That approach should work for i915 + intel ddx at least. In theory it
should be find for generic userspace too, but maybe it would need
a few extra tweaks.

Also we need fp16 to actually be able to test the interpolated 
modes fully. We now have that for icl, but I still need to post
my "fp16 for everything gen4+" followup series. And after that
we need to glue it all together in igt.

Anyways, I've not worked on this branch myself in a while because
I want to get the current gamma support fixed/cleaned up first.
I have at least one more series after the current one gets
reviewed. And after that we still have the current limited
range/yuv vs. ctm vs. degamma/gamma bugs to sort out.

So yeah, I'd prefer to make the current stuff sensible before
spending time adding fancier stuff on top. But in the meantime
if you want to pick up where I left off with this gamma mode
I'd be fine with that.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx