[PATCH] drm/i915/icl: Update csc and gamma enable checks

[Matt Roper]

On Thu, Oct 03, 2024 at 02:10:31PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 03, 2024 at 02:33:41PM +0530, Sai Teja Pottumuttu wrote:
> > With ICL, we have a way to check if gamma and csc are enabled on
> > a pipe using bits in GAMMA_MODE and CSC_MODE. So, use them as well
> > along with the existing BOTTOM_COLOR checks.
> > 
> > BSpec: 7463, 7466
> > Signed-off-by: Sai Teja Pottumuttu <sai.teja.pottumuttu at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_color.c | 24 ++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> > index 50f41aeb3c28..1bf36898dc7e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -1076,6 +1076,26 @@ static void skl_get_config(struct intel_crtc_state *crtc_state)
> >  		crtc_state->csc_enable = true;
> >  }
> >  
> > +static void icl_get_config(struct intel_crtc_state *crtc_state)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > +	u32 bottom_color;
> > +
> > +	crtc_state->gamma_mode = hsw_read_gamma_mode(crtc);
> > +	crtc_state->csc_mode = ilk_read_csc_mode(crtc);
> > +
> > +	bottom_color = intel_de_read(i915, SKL_BOTTOM_COLOR(crtc->pipe));
> > +
> > +	if ((bottom_color & SKL_BOTTOM_COLOR_GAMMA_ENABLE) ||
> > +	    (crtc_state->gamma_mode & POST_CSC_GAMMA_ENABLE))
> > +		crtc_state->gamma_enable = true;
> > +
> > +	if ((bottom_color & SKL_BOTTOM_COLOR_CSC_ENABLE) ||
> > +	    (crtc_state->csc_mode & ICL_CSC_ENABLE))
> > +		crtc_state->csc_enable = true;
> 
> We don't use the old per-plane/bottom color way of enabling these.
> So this is not right.

I think that's the reason for the patch --- today we use
skl_get_config() which *only* checks the bottom color settings.  And
that approach is documented as being deprecated (although still
supported on current platforms).

If we're reading out pre-OS state programmed by the vbios, we probably
need to handle whichever approach it took, right?  Or are we sanitizing
this away to "off" somewhere that makes it okay to miss what was
programmed?


Matt

> 
> > +}
> > +
> >  static void skl_color_commit_arm(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > @@ -3811,7 +3831,7 @@ static const struct intel_color_funcs tgl_color_funcs = {
> >  	.read_luts = icl_read_luts,
> >  	.lut_equal = icl_lut_equal,
> >  	.read_csc = icl_read_csc,
> > -	.get_config = skl_get_config,
> > +	.get_config = icl_get_config,
> >  };
> >  
> >  static const struct intel_color_funcs icl_color_funcs = {
> > @@ -3823,7 +3843,7 @@ static const struct intel_color_funcs icl_color_funcs = {
> >  	.read_luts = icl_read_luts,
> >  	.lut_equal = icl_lut_equal,
> >  	.read_csc = icl_read_csc,
> > -	.get_config = skl_get_config,
> > +	.get_config = icl_get_config,
> >  };
> >  
> >  static const struct intel_color_funcs glk_color_funcs = {
> > -- 
> > 2.34.1
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation