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

[Daniel Vetter <daniel@ffwll.ch>]

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