On Thu, Feb 05, 2015 at 06:58:06PM +0000, Damien Lespiau wrote:
> 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.lesp...@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?
>
> So, this is what I tried first, but it was oopsing in:
>
> 0x1fd31 is in drm_atomic_get_crtc_state (drivers/gpu/drm/drm_atomic.c:205).
> 200 int ret, index;
> 201 struct drm_crtc_state *crtc_state;
> 202
> 203 index = drm_crtc_index(crtc);
> 204
> 205 if (state->crtc_states[index])
> 206 return state->crtc_states[index];
> 207
> 208 ret = drm_modeset_lock(&crtc->mutex, state->acquire_ctx);
> 209 if (ret)
>
> Presumably because we don't have a atomic modeset context setup in
> ->state when initially reading out the hw state.
>
> I went for the shortest way to make the warnings stop.
Ah, you're right. We're not in the middle of an atomic operation, so we
only have a plane_state and not a top-level drm_atomic_state to work
with.
I think what you have here should be fine.
> >
> > > + 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.
>
> I did mean to write:
> intel_crtc->base.primary->state->crtc = &intel_crtc->base;
Okay, that looks good to me.
I think the only thing we need to close on is Ville's concern about the
plane winding up with non-NULL fb and crtc pointers even though a crazy
firmware or bootloader somehow left the plane turned off. I'm not super
familiar with the fastboot stuff, but maybe our
get_initial_plane_config() calls need a test to ensure the plane is
actually turned on before they start trying to read out the FB base and
such?
Matt
>
> --
> Damien
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
