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

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

On Thu, Oct 03, 2024 at 07:22:37AM -0700, Matt Roper wrote:
> 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@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?

I think we're not doing anything. I suppose some kind of
assert_legacy_color_stuff_is_off() thing somewhere could be a
decent addition.

-- 
Ville Syrjälä
Intel