diff mbox series

[3/3] drm/i915/dp: Disable Port sync mode correctly on teardown

Message ID 20191211211425.17821-4-manasi.d.navare@intel.com (mailing list archive)
State New, archived
Headers show
Series i915 fixes to handle hotplug cases on 8K tiled monitor | expand

Commit Message

Navare, Manasi Dec. 11, 2019, 9:14 p.m. UTC 
While clearing the Ports ync mode enable and master select bits
we need to make sure that we perform a RMW for disable else
it sets the other bits casuing unwanted sideeffects.

Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Fixes: 51528afe7c5e ("drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence")
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Matt Roper Dec. 13, 2019, 3:14 a.m. UTC | #1 
On Wed, Dec 11, 2019 at 01:14:25PM -0800, Manasi Navare wrote:
> While clearing the Ports ync mode enable and master select bits
> we need to make sure that we perform a RMW for disable else
> it sets the other bits casuing unwanted sideeffects.
> 
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Fixes: 51528afe7c5e ("drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c0a2dab3fe67..3fccda0f1f36 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4599,7 +4599,8 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
>  		      transcoder_name(old_crtc_state->cpu_transcoder));
>  
>  	reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
> -	trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
> +	trans_ddi_func_ctl2_val = I915_READ(reg);
> +	trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE |
>  				    PORT_SYNC_MODE_MASTER_SELECT_MASK);
>  	I915_WRITE(reg, trans_ddi_func_ctl2_val);
>  }
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Dec. 13, 2019, 8:06 p.m. UTC | #2 
On Wed, Dec 11, 2019 at 01:14:25PM -0800, Manasi Navare wrote:
> While clearing the Ports ync mode enable and master select bits
> we need to make sure that we perform a RMW for disable else
> it sets the other bits casuing unwanted sideeffects.
> 
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Fixes: 51528afe7c5e ("drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence")
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c0a2dab3fe67..3fccda0f1f36 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4599,7 +4599,8 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
>  		      transcoder_name(old_crtc_state->cpu_transcoder));
>  
>  	reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
> -	trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
> +	trans_ddi_func_ctl2_val = I915_READ(reg);
> +	trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE |
>  				    PORT_SYNC_MODE_MASTER_SELECT_MASK);
>  	I915_WRITE(reg, trans_ddi_func_ctl2_val);

I915_WRITE(TRANS_DDI_FUNC_CTL2, 0);

>  }
> -- 
> 2.19.1
Navare, Manasi Dec. 13, 2019, 8:40 p.m. UTC | #3 
On Fri, Dec 13, 2019 at 10:06:37PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 01:14:25PM -0800, Manasi Navare wrote:
> > While clearing the Ports ync mode enable and master select bits
> > we need to make sure that we perform a RMW for disable else
> > it sets the other bits casuing unwanted sideeffects.
> > 
> > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Fixes: 51528afe7c5e ("drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence")
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index c0a2dab3fe67..3fccda0f1f36 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4599,7 +4599,8 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
> >  		      transcoder_name(old_crtc_state->cpu_transcoder));
> >  
> >  	reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
> > -	trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
> > +	trans_ddi_func_ctl2_val = I915_READ(reg);
> > +	trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE |
> >  				    PORT_SYNC_MODE_MASTER_SELECT_MASK);
> >  	I915_WRITE(reg, trans_ddi_func_ctl2_val);
> 
> I915_WRITE(TRANS_DDI_FUNC_CTL2, 0);

So not even consider the other values that might have been set in this reg?
You would prefer setting this to 0 directly?
Right now i do see that no other bits are set, but things can change when we
start using DSI port sync mode or genlock mode etc.

Manasi

> 
> >  }
> > -- 
> > 2.19.1
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjälä Dec. 13, 2019, 8:49 p.m. UTC | #4 
On Fri, Dec 13, 2019 at 12:40:13PM -0800, Manasi Navare wrote:
> On Fri, Dec 13, 2019 at 10:06:37PM +0200, Ville Syrjälä wrote:
> > On Wed, Dec 11, 2019 at 01:14:25PM -0800, Manasi Navare wrote:
> > > While clearing the Ports ync mode enable and master select bits
> > > we need to make sure that we perform a RMW for disable else
> > > it sets the other bits casuing unwanted sideeffects.
> > > 
> > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/issues/5
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Fixes: 51528afe7c5e ("drm/i915/display/icl: Disable transcoder port sync as part of crtc_disable() sequence")
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index c0a2dab3fe67..3fccda0f1f36 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -4599,7 +4599,8 @@ static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
> > >  		      transcoder_name(old_crtc_state->cpu_transcoder));
> > >  
> > >  	reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
> > > -	trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
> > > +	trans_ddi_func_ctl2_val = I915_READ(reg);
> > > +	trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE |
> > >  				    PORT_SYNC_MODE_MASTER_SELECT_MASK);
> > >  	I915_WRITE(reg, trans_ddi_func_ctl2_val);
> > 
> > I915_WRITE(TRANS_DDI_FUNC_CTL2, 0);
> 
> So not even consider the other values that might have been set in this reg?
> You would prefer setting this to 0 directly?
> Right now i do see that no other bits are set, but things can change when we
> start using DSI port sync mode or genlock mode etc.

No point in making it more complicated than it has to be.
In fact after the series I just posted we should probably just
suck this whole thing into intel_ddi_{enable,disable}_transcoder_func()
since on older platforms these bits live in TRANS_DDI_FUNC_CTL itself.

> 
> Manasi
> 
> > 
> > >  }
> > > -- 
> > > 2.19.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index c0a2dab3fe67..3fccda0f1f36 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -4599,7 +4599,8 @@  static void icl_disable_transcoder_port_sync(const struct intel_crtc_state *old_
 		      transcoder_name(old_crtc_state->cpu_transcoder));
 
 	reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
-	trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
+	trans_ddi_func_ctl2_val = I915_READ(reg);
+	trans_ddi_func_ctl2_val &= ~(PORT_SYNC_MODE_ENABLE |
 				    PORT_SYNC_MODE_MASTER_SELECT_MASK);
 	I915_WRITE(reg, trans_ddi_func_ctl2_val);
 }