Re: [PATCH 2/6] drm/i915: Don't use split gamma when we don't have to

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

On Fri, Mar 29, 2019 at 12:47:02PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 28, 2019 at 05:16:03PM -0700, Matt Roper wrote:
> > On Thu, Mar 28, 2019 at 11:05:01PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Using the split gamma mode when we don't have to has the annoying
> > > requirement of loading a linear LUT to the unused half. Instead
> > > let's make life simpler by switching to the 10bit gamma mode
> > > and duplicating each entry.
> > > 
> > > This also allows us to load the software gamma LUT into the
> > > hardware degamma LUT, thus removing some of the buggy
> > > configurations we currently allow (YCbCr/limited range RGB
> > > + gamma LUT). We do still have other configurations that are
> > > also buggy, but those will need more complicated fixes
> > > or they just need to be rejected. Sadly GLK doesn't have
> > > this flexibility anymore and the degamma and gamma LUTs
> > > are very different so no help there.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h    |   1 +
> > >  drivers/gpu/drm/i915/intel_color.c | 159 +++++++++++++++--------------
> > >  2 files changed, 86 insertions(+), 74 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index c866379a521b..eb7e93354cfe 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -10127,6 +10127,7 @@ enum skl_power_gate {
> > >  #define   PAL_PREC_SPLIT_MODE		(1 << 31)
> > >  #define   PAL_PREC_AUTO_INCREMENT	(1 << 15)
> > >  #define   PAL_PREC_INDEX_VALUE_MASK	(0x3ff << 0)
> > > +#define   PAL_PREC_INDEX_VALUE(x)	((x) << 0)
> > >  #define _PAL_PREC_DATA_A	0x4A404
> > >  #define _PAL_PREC_DATA_B	0x4AC04
> > >  #define _PAL_PREC_DATA_C	0x4B404
> > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > > index d7c38a2bbd8f..ed4bd9bd15f5 100644
> > > --- a/drivers/gpu/drm/i915/intel_color.c
> > > +++ b/drivers/gpu/drm/i915/intel_color.c
> > > @@ -466,72 +466,32 @@ static void skl_color_commit(const struct intel_crtc_state *crtc_state)
> > >  		ilk_load_csc_matrix(crtc_state);
> > >  }
> > >  
> > > -static void bdw_load_degamma_lut(const struct intel_crtc_state *crtc_state)
> > > +static void bdw_load_lut_10(struct intel_crtc *crtc,
> > > +			    const struct drm_property_blob *blob,
> > > +			    u32 prec_index, bool duplicate)
> > >  {
> > > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > -	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> > > -	u32 i, lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > > +	const struct drm_color_lut *lut = blob->data;
> > > +	int i, lut_size = drm_color_lut_size(blob);
> > >  	enum pipe pipe = crtc->pipe;
> > >  
> > > -	I915_WRITE(PREC_PAL_INDEX(pipe),
> > > -		   PAL_PREC_SPLIT_MODE | PAL_PREC_AUTO_INCREMENT);
> > > -
> > > -	if (degamma_lut) {
> > > -		const struct drm_color_lut *lut = degamma_lut->data;
> > > +	I915_WRITE(PREC_PAL_INDEX(pipe), prec_index |
> > > +		   PAL_PREC_AUTO_INCREMENT);
> > >  
> > > -		for (i = 0; i < lut_size; i++)
> > > -			I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(&lut[i]));
> > > -	} else {
> > > +	/*
> > > +	 * We advertize the split gamma sizes. When not using split
> > > +	 * gamma we just duplicate each entry.
> > > +	 *
> > > +	 * TODO: expose the full LUT to userspace
> > 
> > Any reason not to just do this immediately?  Throwing away half the
> > table entries if we decide we need split mode doesn't seem any harder
> > than duplicating the entries when we decide we don't.  The color
> > management kerneldoc already explicitly recommends this approach for
> > hardware that can support multiple gamma modes, so I don't think we need
> > any new ABI to handle it.
> 
> Hmm. I guess that apporach could be doable. It might be a bit annoying
> for userspace though if it expects a direct color visual. But at least
> for X we won't use degamma/ctm anyway so seems like it should work out
> just fine.

As usual this needs a bit of care when picking the LUT entries we
use. I think I'll send that as a followup.

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