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

[Matt Roper]

On Thu, Oct 03, 2024 at 07:12:41PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 03, 2024 at 09:05:40AM -0700, Matt Roper wrote:
> > On Thu, Oct 03, 2024 at 06:46:31PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 03, 2024 at 09:08:53PM +0530, Pottumuttu, Sai Teja wrote:
> > > > 
> > > > On 03-10-2024 20:28, Ville Syrjälä wrote:
> > > > > On Thu, Oct 03, 2024 at 05:32:56PM +0300, Ville Syrjälä wrote:
> > > > >> 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 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?
> > > > >> 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.
> > > > > Or maybe we don't need to do anything. The state checker will catch
> > > > > this problem with the current code as far as the pipe bottom color
> > > > > is concerned. It won't catch inconsistent plane programming though.
> > > > > But we also don't have such a consistency check for older platforms
> > > > > either.
> > > > 
> > > > So, are we essentially saying that we can completely remove BOTTOM_COLOR 
> > > > from here and just move to using GAMMA_MODE, CSC_MODE for setting 
> > > > gamma_enable and csc_enable?
> > > 
> > > No. The current code already works correctly, and pipe_csc_enable and 
> > > gamma_enable are not meant to be used on icl+.
> > 
> > Then does that mean icl_color_commit_arm() is wrong since we're
> > programming those registers there?
> 
> Not really. It just assumes those bits should never be set,
> which is true given how icl_color_check() operates.

Hmm, this is confusing since I see icl_gamma_mode() setting up
pipe-level gamma enable:

        if (crtc_state->hw.gamma_lut &&
            !crtc_state->c8_planes)
                gamma_mode |= POST_CSC_GAMMA_ENABLE;

and icl_csc_mode() setting up CSC enable:

        if (crtc_state->hw.ctm)
                csc_mode |= ICL_CSC_ENABLE;

and then those eventually get programmed to hardware in
icl_color_commit_arm.

But then our get_config function doesn't read these same bits and
instead only reads the bottom_color bit instead (which is mutually
exclusive with the pipe-level control, and which will likely be removed
from the hardware completely at some point since it's already listed as
deprecated).  What am I overlooking here?


Matt

> 
> -- 
> Ville Syrjälä
> Intel

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