[PATCH] drm/i915: Sanitize prepare_pipes after valleyview_modeset_global_pipes()

[PATCH] drm/i915: Sanitize prepare_pipes after valleyview_modeset_global_pipes()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

valleyview_modeset_global_pipes() may add pipes that are getting fully
disabled to prepare_pipes bitmask. The rest of the code doesn't expect
this, so clear out any such pipes from the prepare_pipes bitmask.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f97e895..ddbef9c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9514,10 +9514,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 * mode set on this crtc.  For other crtcs we need to use the
 	 * adjusted_mode bits in the crtc directly.
 	 */
-	if (IS_VALLEYVIEW(dev))
+	if (IS_VALLEYVIEW(dev)) {
 		valleyview_modeset_global_pipes(dev, &prepare_pipes,
 						modeset_pipes, pipe_config);
 
+		/* may have added more to prepare_pipes than we should */
+		prepare_pipes &= ~disable_pipes;
+	}
+
 	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
 		intel_crtc_disable(&intel_crtc->base);
 
-- 
1.8.1.5

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

Re: [PATCH] drm/i915: Sanitize prepare_pipes after valleyview_modeset_global_pipes()

[Daniel Vetter]

On Tue, Nov 05, 2013 at 10:34:12PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> valleyview_modeset_global_pipes() may add pipes that are getting fully
> disabled to prepare_pipes bitmask. The rest of the code doesn't expect
> this, so clear out any such pipes from the prepare_pipes bitmask.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f97e895..ddbef9c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9514,10 +9514,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  	 * mode set on this crtc.  For other crtcs we need to use the
>  	 * adjusted_mode bits in the crtc directly.
>  	 */
> -	if (IS_VALLEYVIEW(dev))
> +	if (IS_VALLEYVIEW(dev)) {
>  		valleyview_modeset_global_pipes(dev, &prepare_pipes,
>  						modeset_pipes, pipe_config);
>  
> +		/* may have added more to prepare_pipes than we should */
> +		prepare_pipes &= ~disable_pipes;
> +	}

I'd have move the full "take disable_pipes out" block from affected_pipes.
Afacs there's no need to do it twice.
-Daniel
> +
>  	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
>  		intel_crtc_disable(&intel_crtc->base);
>  
> -- 
> 1.8.1.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Sanitize prepare_pipes after valleyview_modeset_global_pipes()

[Ville Syrjälä]

On Wed, Nov 06, 2013 at 08:33:08AM +0100, Daniel Vetter wrote:
> On Tue, Nov 05, 2013 at 10:34:12PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > valleyview_modeset_global_pipes() may add pipes that are getting fully
> > disabled to prepare_pipes bitmask. The rest of the code doesn't expect
> > this, so clear out any such pipes from the prepare_pipes bitmask.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f97e895..ddbef9c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9514,10 +9514,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> >  	 * mode set on this crtc.  For other crtcs we need to use the
> >  	 * adjusted_mode bits in the crtc directly.
> >  	 */
> > -	if (IS_VALLEYVIEW(dev))
> > +	if (IS_VALLEYVIEW(dev)) {
> >  		valleyview_modeset_global_pipes(dev, &prepare_pipes,
> >  						modeset_pipes, pipe_config);
> >  
> > +		/* may have added more to prepare_pipes than we should */
> > +		prepare_pipes &= ~disable_pipes;
> > +	}
> 
> I'd have move the full "take disable_pipes out" block from affected_pipes.
> Afacs there's no need to do it twice.

I think we'd need to keep the '*modeset_pipes &= ~(*disable_pipes)' part
in intel_modeset_affected_pipes(). Otherwise we might end up calling
intel_modeset_pipe_config() for a disabled pipe. For prepare_pipes, it
looks like we could move the masking to happen only once after
valleyview_modeset_global_pipes(). Not sure if spreading it around like
that makes sense.

> -Daniel
> > +
> >  	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
> >  		intel_crtc_disable(&intel_crtc->base);
> >  
> > -- 
> > 1.8.1.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Sanitize prepare_pipes after valleyview_modeset_global_pipes()

[Daniel Vetter]

On Wed, Nov 06, 2013 at 12:42:20PM +0200, Ville Syrjälä wrote:
> On Wed, Nov 06, 2013 at 08:33:08AM +0100, Daniel Vetter wrote:
> > On Tue, Nov 05, 2013 at 10:34:12PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > valleyview_modeset_global_pipes() may add pipes that are getting fully
> > > disabled to prepare_pipes bitmask. The rest of the code doesn't expect
> > > this, so clear out any such pipes from the prepare_pipes bitmask.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index f97e895..ddbef9c 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -9514,10 +9514,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> > >  	 * mode set on this crtc.  For other crtcs we need to use the
> > >  	 * adjusted_mode bits in the crtc directly.
> > >  	 */
> > > -	if (IS_VALLEYVIEW(dev))
> > > +	if (IS_VALLEYVIEW(dev)) {
> > >  		valleyview_modeset_global_pipes(dev, &prepare_pipes,
> > >  						modeset_pipes, pipe_config);
> > >  
> > > +		/* may have added more to prepare_pipes than we should */
> > > +		prepare_pipes &= ~disable_pipes;
> > > +	}
> > 
> > I'd have move the full "take disable_pipes out" block from affected_pipes.
> > Afacs there's no need to do it twice.
> 
> I think we'd need to keep the '*modeset_pipes &= ~(*disable_pipes)' part
> in intel_modeset_affected_pipes(). Otherwise we might end up calling
> intel_modeset_pipe_config() for a disabled pipe. For prepare_pipes, it
> looks like we could move the masking to happen only once after
> valleyview_modeset_global_pipes(). Not sure if spreading it around like
> that makes sense.

Hm, good point. I've merged this, we can beautify this code once the real
atomic modeset stuff has landed and we can actually do modesets on more
than one pipe.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch