diff mbox series

[01/10] drm/i915: Grab intel_display from the encoder to avoid potential oopsies

Message ID 20241107161123.16269-2-ville.syrjala@linux.intel.com (mailing list archive)
State New
Headers show
Series drm/i915: Potential boot oops fix and some cleanups | expand

Commit Message

Ville Syrjala Nov. 7, 2024, 4:11 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Grab the intel_display from 'encoder' rather than 'state'
in the encoder hooks to avoid the massive footgun that is
intel_sanitize_encoder(), which passes NULL as the 'state'
argument to encoder .disable() and .post_disable().

TODO: figure out how to actually fix intel_sanitize_encoder()...

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_crt.c | 10 +++++-----
 drivers/gpu/drm/i915/display/intel_tv.c  |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Jani Nikula Nov. 8, 2024, 1 p.m. UTC | #1 
On Thu, 07 Nov 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Grab the intel_display from 'encoder' rather than 'state'
> in the encoder hooks to avoid the massive footgun that is
> intel_sanitize_encoder(), which passes NULL as the 'state'
> argument to encoder .disable() and .post_disable().
>
> TODO: figure out how to actually fix intel_sanitize_encoder()...
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Whoopsies.

Do we need a Fixes: tag? I presume the changes are headed to v6.13 and
without the tag this one isn't.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/display/intel_crt.c | 10 +++++-----
>  drivers/gpu/drm/i915/display/intel_tv.c  |  4 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
> index 74c1983fe07e..1be55bdb48b9 100644
> --- a/drivers/gpu/drm/i915/display/intel_crt.c
> +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> @@ -244,7 +244,7 @@ static void hsw_disable_crt(struct intel_atomic_state *state,
>  			    const struct intel_crtc_state *old_crtc_state,
>  			    const struct drm_connector_state *old_conn_state)
>  {
> -	struct intel_display *display = to_intel_display(state);
> +	struct intel_display *display = to_intel_display(encoder);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  
>  	drm_WARN_ON(display->drm, !old_crtc_state->has_pch_encoder);
> @@ -257,7 +257,7 @@ static void hsw_post_disable_crt(struct intel_atomic_state *state,
>  				 const struct intel_crtc_state *old_crtc_state,
>  				 const struct drm_connector_state *old_conn_state)
>  {
> -	struct intel_display *display = to_intel_display(state);
> +	struct intel_display *display = to_intel_display(encoder);
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  
> @@ -287,7 +287,7 @@ static void hsw_pre_pll_enable_crt(struct intel_atomic_state *state,
>  				   const struct intel_crtc_state *crtc_state,
>  				   const struct drm_connector_state *conn_state)
>  {
> -	struct intel_display *display = to_intel_display(state);
> +	struct intel_display *display = to_intel_display(encoder);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  
>  	drm_WARN_ON(display->drm, !crtc_state->has_pch_encoder);
> @@ -300,7 +300,7 @@ static void hsw_pre_enable_crt(struct intel_atomic_state *state,
>  			       const struct intel_crtc_state *crtc_state,
>  			       const struct drm_connector_state *conn_state)
>  {
> -	struct intel_display *display = to_intel_display(state);
> +	struct intel_display *display = to_intel_display(encoder);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	enum pipe pipe = crtc->pipe;
> @@ -319,7 +319,7 @@ static void hsw_enable_crt(struct intel_atomic_state *state,
>  			   const struct intel_crtc_state *crtc_state,
>  			   const struct drm_connector_state *conn_state)
>  {
> -	struct intel_display *display = to_intel_display(state);
> +	struct intel_display *display = to_intel_display(encoder);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	enum pipe pipe = crtc->pipe;
> diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c
> index bfd16054ca05..27c530218ee6 100644
> --- a/drivers/gpu/drm/i915/display/intel_tv.c
> +++ b/drivers/gpu/drm/i915/display/intel_tv.c
> @@ -929,7 +929,7 @@ intel_enable_tv(struct intel_atomic_state *state,
>  		const struct intel_crtc_state *pipe_config,
>  		const struct drm_connector_state *conn_state)
>  {
> -	struct intel_display *display = to_intel_display(state);
> +	struct intel_display *display = to_intel_display(encoder);
>  
>  	/* Prevents vblank waits from timing out in intel_tv_detect_type() */
>  	intel_crtc_wait_for_next_vblank(to_intel_crtc(pipe_config->uapi.crtc));
> @@ -943,7 +943,7 @@ intel_disable_tv(struct intel_atomic_state *state,
>  		 const struct intel_crtc_state *old_crtc_state,
>  		 const struct drm_connector_state *old_conn_state)
>  {
> -	struct intel_display *display = to_intel_display(state);
> +	struct intel_display *display = to_intel_display(encoder);
>  
>  	intel_de_rmw(display, TV_CTL, TV_ENC_ENABLE, 0);
>  }
Ville Syrjala Nov. 11, 2024, 10:03 p.m. UTC | #2 
On Fri, Nov 08, 2024 at 03:00:21PM +0200, Jani Nikula wrote:
> On Thu, 07 Nov 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Grab the intel_display from 'encoder' rather than 'state'
> > in the encoder hooks to avoid the massive footgun that is
> > intel_sanitize_encoder(), which passes NULL as the 'state'
> > argument to encoder .disable() and .post_disable().
> >
> > TODO: figure out how to actually fix intel_sanitize_encoder()...
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Whoopsies.
> 
> Do we need a Fixes: tag? I presume the changes are headed to v6.13 and
> without the tag this one isn't.

Slapped these on:
Fixes: 40eb34c3f491 ("drm/i915/crt: convert to struct intel_display")
Fixes: ab0b0eb5c85c ("drm/i915/tv: convert to struct intel_display")

> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Thanks. Pushed the lot.
 
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_crt.c | 10 +++++-----
> >  drivers/gpu/drm/i915/display/intel_tv.c  |  4 ++--
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
> > index 74c1983fe07e..1be55bdb48b9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> > @@ -244,7 +244,7 @@ static void hsw_disable_crt(struct intel_atomic_state *state,
> >  			    const struct intel_crtc_state *old_crtc_state,
> >  			    const struct drm_connector_state *old_conn_state)
> >  {
> > -	struct intel_display *display = to_intel_display(state);
> > +	struct intel_display *display = to_intel_display(encoder);
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  
> >  	drm_WARN_ON(display->drm, !old_crtc_state->has_pch_encoder);
> > @@ -257,7 +257,7 @@ static void hsw_post_disable_crt(struct intel_atomic_state *state,
> >  				 const struct intel_crtc_state *old_crtc_state,
> >  				 const struct drm_connector_state *old_conn_state)
> >  {
> > -	struct intel_display *display = to_intel_display(state);
> > +	struct intel_display *display = to_intel_display(encoder);
> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  
> > @@ -287,7 +287,7 @@ static void hsw_pre_pll_enable_crt(struct intel_atomic_state *state,
> >  				   const struct intel_crtc_state *crtc_state,
> >  				   const struct drm_connector_state *conn_state)
> >  {
> > -	struct intel_display *display = to_intel_display(state);
> > +	struct intel_display *display = to_intel_display(encoder);
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  
> >  	drm_WARN_ON(display->drm, !crtc_state->has_pch_encoder);
> > @@ -300,7 +300,7 @@ static void hsw_pre_enable_crt(struct intel_atomic_state *state,
> >  			       const struct intel_crtc_state *crtc_state,
> >  			       const struct drm_connector_state *conn_state)
> >  {
> > -	struct intel_display *display = to_intel_display(state);
> > +	struct intel_display *display = to_intel_display(encoder);
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  	enum pipe pipe = crtc->pipe;
> > @@ -319,7 +319,7 @@ static void hsw_enable_crt(struct intel_atomic_state *state,
> >  			   const struct intel_crtc_state *crtc_state,
> >  			   const struct drm_connector_state *conn_state)
> >  {
> > -	struct intel_display *display = to_intel_display(state);
> > +	struct intel_display *display = to_intel_display(encoder);
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >  	enum pipe pipe = crtc->pipe;
> > diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c
> > index bfd16054ca05..27c530218ee6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tv.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tv.c
> > @@ -929,7 +929,7 @@ intel_enable_tv(struct intel_atomic_state *state,
> >  		const struct intel_crtc_state *pipe_config,
> >  		const struct drm_connector_state *conn_state)
> >  {
> > -	struct intel_display *display = to_intel_display(state);
> > +	struct intel_display *display = to_intel_display(encoder);
> >  
> >  	/* Prevents vblank waits from timing out in intel_tv_detect_type() */
> >  	intel_crtc_wait_for_next_vblank(to_intel_crtc(pipe_config->uapi.crtc));
> > @@ -943,7 +943,7 @@ intel_disable_tv(struct intel_atomic_state *state,
> >  		 const struct intel_crtc_state *old_crtc_state,
> >  		 const struct drm_connector_state *old_conn_state)
> >  {
> > -	struct intel_display *display = to_intel_display(state);
> > +	struct intel_display *display = to_intel_display(encoder);
> >  
> >  	intel_de_rmw(display, TV_CTL, TV_ENC_ENABLE, 0);
> >  }
> 
> -- 
> Jani Nikula, Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
index 74c1983fe07e..1be55bdb48b9 100644
--- a/drivers/gpu/drm/i915/display/intel_crt.c
+++ b/drivers/gpu/drm/i915/display/intel_crt.c
@@ -244,7 +244,7 @@  static void hsw_disable_crt(struct intel_atomic_state *state,
 			    const struct intel_crtc_state *old_crtc_state,
 			    const struct drm_connector_state *old_conn_state)
 {
-	struct intel_display *display = to_intel_display(state);
+	struct intel_display *display = to_intel_display(encoder);
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 
 	drm_WARN_ON(display->drm, !old_crtc_state->has_pch_encoder);
@@ -257,7 +257,7 @@  static void hsw_post_disable_crt(struct intel_atomic_state *state,
 				 const struct intel_crtc_state *old_crtc_state,
 				 const struct drm_connector_state *old_conn_state)
 {
-	struct intel_display *display = to_intel_display(state);
+	struct intel_display *display = to_intel_display(encoder);
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 
@@ -287,7 +287,7 @@  static void hsw_pre_pll_enable_crt(struct intel_atomic_state *state,
 				   const struct intel_crtc_state *crtc_state,
 				   const struct drm_connector_state *conn_state)
 {
-	struct intel_display *display = to_intel_display(state);
+	struct intel_display *display = to_intel_display(encoder);
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 
 	drm_WARN_ON(display->drm, !crtc_state->has_pch_encoder);
@@ -300,7 +300,7 @@  static void hsw_pre_enable_crt(struct intel_atomic_state *state,
 			       const struct intel_crtc_state *crtc_state,
 			       const struct drm_connector_state *conn_state)
 {
-	struct intel_display *display = to_intel_display(state);
+	struct intel_display *display = to_intel_display(encoder);
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	enum pipe pipe = crtc->pipe;
@@ -319,7 +319,7 @@  static void hsw_enable_crt(struct intel_atomic_state *state,
 			   const struct intel_crtc_state *crtc_state,
 			   const struct drm_connector_state *conn_state)
 {
-	struct intel_display *display = to_intel_display(state);
+	struct intel_display *display = to_intel_display(encoder);
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	enum pipe pipe = crtc->pipe;
diff --git a/drivers/gpu/drm/i915/display/intel_tv.c b/drivers/gpu/drm/i915/display/intel_tv.c
index bfd16054ca05..27c530218ee6 100644
--- a/drivers/gpu/drm/i915/display/intel_tv.c
+++ b/drivers/gpu/drm/i915/display/intel_tv.c
@@ -929,7 +929,7 @@  intel_enable_tv(struct intel_atomic_state *state,
 		const struct intel_crtc_state *pipe_config,
 		const struct drm_connector_state *conn_state)
 {
-	struct intel_display *display = to_intel_display(state);
+	struct intel_display *display = to_intel_display(encoder);
 
 	/* Prevents vblank waits from timing out in intel_tv_detect_type() */
 	intel_crtc_wait_for_next_vblank(to_intel_crtc(pipe_config->uapi.crtc));
@@ -943,7 +943,7 @@  intel_disable_tv(struct intel_atomic_state *state,
 		 const struct intel_crtc_state *old_crtc_state,
 		 const struct drm_connector_state *old_conn_state)
 {
-	struct intel_display *display = to_intel_display(state);
+	struct intel_display *display = to_intel_display(encoder);
 
 	intel_de_rmw(display, TV_CTL, TV_ENC_ENABLE, 0);
 }