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

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

On Thu, Mar 28, 2019 at 07:00:30PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, March 22, 2019 7:23 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, Maarten
> ><maarten.lankhorst@intel.com>; intel-gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma
> >Mode
> >
> >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/1aab7625dca77b831e05e32af179
> >> >04c2130
> >> >0ff95
> >> >https://github.com/vsyrjala/linux/commit/74ffa5d441702c53830f6d71bb68
> >> >7bb0ae5a
> >> >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.
> 
> Hi Ville,
> Thanks for sharing these details and also the design what you have is really nice.
> 
> I was trying to build on top of it and trying to check the interface before floating the next
> version. There seems to be a limitation in creating a property with both BLOB and ENUM
> flags.
> 
> /* Only one legacy type at a time please */
>         if (legacy_type && !is_power_of_2(legacy_type))
>                 return false;
> 
> Any suggestion on how to deal with this limitation.

I think we should just define a new BLOB_ENUM type. Trying to mix two
old types like this could end in explosions with existing userspace so
safer to make it a totally distinct type.

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