| Message ID | 1423156939-15705-6-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. Matt > update_state_fb(intel_crtc->base.primary); > obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); > break; > -- > 1.8.3.1 >
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@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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. > > > + 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;
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.lespiau@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
On Thu, Feb 05, 2015 at 11:12:20AM -0800, Matt Roper wrote: > 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? I was also looking at get_initial_plane_config() and was thinking the same, will add a patch on top.
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; + 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; update_state_fb(intel_crtc->base.primary); obj->frontbuffer_bits |= INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); break;
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@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)