diff mbox series

drm/i915: Nuke INTEL_OUTPUT_FORMAT_INVALID

Message ID 20210205202322.27608-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Nuke INTEL_OUTPUT_FORMAT_INVALID | expand

Commit Message

Ville Syrjälä Feb. 5, 2021, 8:23 p.m. UTC
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(-)

Comments

José Roberto de Souza Feb. 17, 2021, 4:37 p.m. UTC | #1
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,
>  	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

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,
Ville Syrjälä Feb. 17, 2021, 10:10 p.m. UTC | #2
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.

> 
> 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,
>
José Roberto de Souza Feb. 18, 2021, 1:02 p.m. UTC | #3
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,
> > 
>
diff mbox series

Patch

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,
 	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;
 	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,