Re: [Intel-gfx] [PATCH] drm/i915: Nuke INTEL_OUTPUT_FORMAT_INVALID

["Souza, Jose" <jose.souza@intel.com>]

On Thu, 2021-02-18 at 00:10 +0200, Ville Syrjälä wrote:
> On Wed, Feb 17, 2021 at 04:37:20PM +0000, Souza, Jose wrote:
> > On Fri, 2021-02-05 at 22:23 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > We tend to use output_format!=RGB as a shorthand for YCbCr, but
> > > this fails if we have a disabled crtc where output_format==INVALID.
> > > We're now getting some fail from intel_color_check() when we have:
> > >  hw.enable==false
> > >  hw.ctm!=NULL
> > >  output_format==INVALID
> > > 
> > > Let's avoid that by throwing INTEL_OUTPUT_FORMAT_INVALID to the
> > > dumpster, and thus everything defaults to RGB when the crtc
> > > is disabled.
> > > 
> > > This does beg the deeper question of how much of the state
> > > should we in fact be validating when hw/uapi.enable==false.
> > > And should we even be doing the uapi->hw copy when
> > > uapi.enable==false? So far I've not been able to come up with
> > > satisfactory answers for myself, so I'm putting it off for the
> > > moment.
> > > 
> > > Cc: Lee Shawn C <shawn.c.lee@intel.com>
> > > Fixes: 0aa5c3835c8a ("drm/i915: support two CSC module on gen11 and later")
> > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2964
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_crtc.c          | 1 -
> > >  drivers/gpu/drm/i915/display/intel_display.c       | 3 +--
> > >  drivers/gpu/drm/i915/display/intel_display_types.h | 1 -
> > >  3 files changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > index 57b0a3ebe908..8e77ca7ddf11 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > @@ -109,7 +109,6 @@ void intel_crtc_state_reset(struct intel_crtc_state *crtc_state,
> 
> void intel_crtc_state_reset(struct intel_crtc_state *crtc_state,
>                             struct intel_crtc *crtc)
> {
>         memset(crtc_state, 0, sizeof(*crtc_state));
> ...
> 
> > >  	crtc_state->cpu_transcoder = INVALID_TRANSCODER;
> > >  	crtc_state->master_transcoder = INVALID_TRANSCODER;
> > >  	crtc_state->hsw_workaround_pipe = INVALID_PIPE;
> > > -	crtc_state->output_format = INTEL_OUTPUT_FORMAT_INVALID;
> > 
> > Missing set output_format to INTEL_OUTPUT_FORMAT_RGB, kmalloc() don't set memory allocated to zero and INTEL_OUTPUT_FORMAT_INVALID was the index 0 and
> > we were setting it during intel_crtc_state_reset() so we should now set it to INTEL_OUTPUT_FORMAT_RGB.
> > https://www.kernel.org/doc/htmldocs/kernel-api/mm.html
> 
> ie. we zero out the whole thing. The reason why the explicit assignment
> was here I suppose is that I had assumed INTEL_OUTPUT_FORMAT_INVALID==-1,
> which is the case for INVALID_TRANSCODER/PIPE/etc.

Ah okay, missed the memset().

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> > 
> > With that fixed:
> > 
> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> > 
> > >  	crtc_state->scaler_state.scaler_id = -1;
> > >  	crtc_state->mst_master_transcoder = INVALID_TRANSCODER;
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 92c14f3f0abf..46d0093187f8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -10220,7 +10220,6 @@ static void snprintf_output_types(char *buf, size_t len,
> > >  }
> > >  
> > > 
> > > 
> > > 
> > >  static const char * const output_format_str[] = {
> > > -	[INTEL_OUTPUT_FORMAT_INVALID] = "Invalid",
> > >  	[INTEL_OUTPUT_FORMAT_RGB] = "RGB",
> > >  	[INTEL_OUTPUT_FORMAT_YCBCR420] = "YCBCR4:2:0",
> > >  	[INTEL_OUTPUT_FORMAT_YCBCR444] = "YCBCR4:4:4",
> > > @@ -10229,7 +10228,7 @@ static const char * const output_format_str[] = {
> > >  static const char *output_formats(enum intel_output_format format)
> > >  {
> > >  	if (format >= ARRAY_SIZE(output_format_str))
> > > -		format = INTEL_OUTPUT_FORMAT_INVALID;
> > > +		return "invalid";
> > >  	return output_format_str[format];
> > >  }
> > >  
> > > 
> > > 
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 307ff4b771f4..b3ac39fea6f0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -830,7 +830,6 @@ struct intel_crtc_wm_state {
> > >  };
> > >  
> > > 
> > > 
> > > 
> > >  enum intel_output_format {
> > > -	INTEL_OUTPUT_FORMAT_INVALID,
> > >  	INTEL_OUTPUT_FORMAT_RGB,
> > >  	INTEL_OUTPUT_FORMAT_YCBCR420,
> > >  	INTEL_OUTPUT_FORMAT_YCBCR444,
> > 
> 

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