Re: [PATCH 3/7] drm/i915: Fix fbdev sprite disable code

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, May 24, 2013 at 11:35:07AM +0200, Daniel Vetter wrote:
> On Wed, May 08, 2013 at 12:55:18PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > plane->enabled is never set, so this code didn't do anything.
> > 
> > If we end up doing a full modeset, sprites already get disabled. However
> > if we end up doing a simple set_base, we still need to turn off the sprites
> > manually.
> > 
> > And do the same for cursors, since we only want to see the primary plane
> > for fbdev.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >  drivers/gpu/drm/i915/intel_fb.c      |  8 ++------
> >  3 files changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index cfe2803..cef0bff 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9928,3 +9928,17 @@ intel_display_print_error_state(struct seq_file *m,
> >  	}
> >  }
> >  #endif
> > +
> > +void intel_disable_cursors_and_sprites(struct drm_device *dev)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_plane *plane;
> > +
> > +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > +		intel_crtc_dpms_overlay(to_intel_crtc(crtc), false);
> > +		intel_crtc_update_cursor(crtc, false);
> > +	}
> > +
> > +	list_for_each_entry(plane, &dev->mode_config.plane_list, head)
> > +		intel_plane_disable(plane);
> 
> Somehow my tree doesn't have an intel_plane_disable and while hunting that
> one down I've started to think a bit more ...

Yeah, that sucker was in the modeset sequence patch set. I should've
pulled it out into a separate patch.

> Why exactly do we need special driver callbacks for all this? Strictly
> speaking fbcon is just another kms client, if it needs to dig around in
> driver details that just means our interfaces are not good enough. I know
> that there's some historical baggage left around, but for disabling
> cursors and planes I don't see any need at all:
> 
> Can't we just loop over all crtcs and disable every cursor and then loop
> over all sprites and also shut them all down when restoring the fbdev
> mode? All without any new driver callbacks?

Some driver might want to use hardware cursor for fb_cursor, also some
drivers want to use planes for fbdev scanout, which as you know is
something I want for i915 as well in the future.

Also there's the whole state mismanagement. We don't track enough of
the state in the core to restore stuff properly, so if we disable
stuff, we can't re-enable it when switching back to the client using
sprites/planes. We could move the plane coordinate tracking to the
core easily enough (I even had a patch for that in some old atomic
branch). I haven't looked at out current cursor stuff closely enough
to figure out if we can do the same there, but currently the cursor
state is tracked purely inside the driver.

So for the short term I think it's easier to go with the new hook,
and let each driver manage their planes/cursors.

> 
> It might flicker like mad since we can't do it atomically, but then again
> this would give us a neat in-kernel user for the atomic modeset stuff
> right away ...

When we get to the atomic stuff, I'd like to keep a full state per
client, and then it'd be very easy to just blast the new state in when
swithing between clients.

> 
> Cheers, Daniel
> 
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index cd1297e..a4e866a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -789,5 +789,6 @@ extern bool intel_set_cpu_fifo_underrun_reporting(struct drm_device *dev,
> >  extern bool intel_set_pch_fifo_underrun_reporting(struct drm_device *dev,
> >  						 enum transcoder pch_transcoder,
> >  						 bool enable);
> > +extern void intel_disable_cursors_and_sprites(struct drm_device *dev);
> >  
> >  #endif /* __INTEL_DRV_H__ */
> > diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> > index a04481f..57a082c 100644
> > --- a/drivers/gpu/drm/i915/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/intel_fb.c
> > @@ -281,8 +281,6 @@ void intel_fb_restore_mode(struct drm_device *dev)
> >  {
> >  	int ret;
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > -	struct drm_mode_config *config = &dev->mode_config;
> > -	struct drm_plane *plane;
> >  
> >  	if (INTEL_INFO(dev)->num_pipes == 0)
> >  		return;
> > @@ -293,10 +291,8 @@ void intel_fb_restore_mode(struct drm_device *dev)
> >  	if (ret)
> >  		DRM_DEBUG("failed to restore crtc mode\n");
> >  
> > -	/* Be sure to shut off any planes that may be active */
> > -	list_for_each_entry(plane, &config->plane_list, head)
> > -		if (plane->enabled)
> > -			plane->funcs->disable_plane(plane);
> > +	/* in case we ended up doing just set_base above */
> > +	intel_disable_cursors_and_sprites(dev);
> >  
> >  	drm_modeset_unlock_all(dev);
> >  }
> > -- 
> > 1.8.1.5
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC