[Intel-gfx] [PATCH 5/5] drm/i915: Fix atomic state when reusing the firmware fb

[Ville Syrjälä]

On Thu, Feb 05, 2015 at 10:10:26AM -0800, Matt Roper wrote:
> On Thu, Feb 05, 2015 at 05:22:19PM +0000, Damien Lespiau wrote:
> > Right now, we get a warning when taking over the firmware fb:
> > 
> >   [drm:drm_atomic_plane_check] FB set but no CRTC
> > 
> > with the following backtrace:
> > 
> >   [<ffffffffa010339d>] drm_atomic_check_only+0x35d/0x510 [drm]
> >   [<ffffffffa0103567>] drm_atomic_commit+0x17/0x60 [drm]
> >   [<ffffffffa00a6ccd>] drm_atomic_helper_plane_set_property+0x8d/0xd0 [drm_kms_helper]
> >   [<ffffffffa00f1fed>] drm_mode_plane_set_obj_prop+0x2d/0x90 [drm]
> >   [<ffffffffa00a8a1b>] restore_fbdev_mode+0x6b/0xf0 [drm_kms_helper]
> >   [<ffffffffa00aa969>] drm_fb_helper_restore_fbdev_mode_unlocked+0x29/0x80 [drm_kms_helper]
> >   [<ffffffffa00aa9e2>] drm_fb_helper_set_par+0x22/0x50 [drm_kms_helper]
> >   [<ffffffffa050a71a>] intel_fbdev_set_par+0x1a/0x60 [i915]
> >   [<ffffffff813ad444>] fbcon_init+0x4f4/0x580
> > 
> > That's because we update the plane state with the fb from the firmware, but we
> > never associate the plane to that CRTC.
> > 
> > We don't quite have the full DRM take over from HW state just yet, so
> > fake enough of the plane atomic state to pass the checks.
> > 
> > Signed-off-by: Damien Lespiau <damien.lespiau at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0d07535..eed1c0c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2439,8 +2439,12 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
> >  		return;
> >  
> >  	if (intel_alloc_plane_obj(intel_crtc, plane_config)) {
> > -		intel_crtc->base.primary->fb = &plane_config->fb->base;
> > -		update_state_fb(intel_crtc->base.primary);
> > +		struct drm_plane *primary = intel_crtc->base.primary;
> > +
> > +		primary->fb = &plane_config->fb->base;
> > +		primary->state->crtc = &intel_crtc->base;
> 
> Would
> 
>         drm_atomic_set_crtc_for_plane(primary->state, &intel_crtc->base)
> 
> be better here so that the various bitmasks get updated?
> 
> > +		update_state_fb(primary);
> > +
> >  		return;
> >  	}
> >  
> > @@ -2469,6 +2473,7 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
> >  
> >  			drm_framebuffer_reference(c->primary->fb);
> >  			intel_crtc->base.primary->fb = c->primary->fb;
> > +			c->state->crtc = &intel_crtc->base;
> 
> I'm not sure I understand this line.  c->state is the crtc state for the
> other CRTC we're going to share with with (and has a backpointer of
> c->state->crtc = c).  Was this supposed to be something like
> 
>         drm_atomic_set_crtc_for_plane(intel_crtc->base.primary->state,
>                                       &intel_crtc->base)
> 
> instead?  I'm not sure if updating the other crtc (that we're sharing
> with) was a mistake here.

plane->crtc != NULL indicates that the plane is actually enabled. But
that might not be true here actually. We know the pipe is enabled, but
we haven't actually checked the primary plane state. If some silly BIOS
boots with the plane disabled on some pipe we might get confused here.
Or did we add some check to sanitize to turn off the entire pipe if
the primary plane isn't enabled? There was some problem in this area as
I recall, but can't remember if anything came of it.

> 
> 
> Matt
> 
> >  			update_state_fb(intel_crtc->base.primary);
> >  			obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> >  			break;
> > -- 
> > 1.8.3.1
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC