[PATCH] drm/i915/vlv: don't try to enable a crtc without fb

[PATCH] drm/i915/vlv: don't try to enable a crtc without fb

[Rob Clark]

On valleyview, all enabled pipes get added to the prepare mask.  However
if you did state readout to find that the crtc is enabled, but failed to
map stolen mem and wrap it in a bo (and fb), you could end up in a
scenario where crtc->primary->fb is still null on first modeset.  So
lets check for this rather than exploding.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
As with http://lists.freedesktop.org/archives/dri-devel/2014-September/069341.html
the root cause is issues with stolen region.  But it would be nice if
i915 was a bit less fragile when things don't go as planned.

This could have the side effect that, at first modeset, the crtc's which
were enabled by BIOS but not yet set (by fbcon or userspace) get disabled.
This is a somewhat lesser inconvenience compared to opps'ing under
console_lock.

 drivers/gpu/drm/i915/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d8324c6..9f85033 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10895,6 +10895,9 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
+		if (!intel_crtc->base.primary->fb)
+			continue;
+
 		update_scanline_offset(intel_crtc);
 
 		dev_priv->display.crtc_enable(&intel_crtc->base);
-- 
1.9.3

Re: [PATCH] drm/i915/vlv: don't try to enable a crtc without fb

[Ville Syrjälä]

On Fri, Oct 17, 2014 at 06:53:34PM -0400, Rob Clark wrote:
> On valleyview, all enabled pipes get added to the prepare mask.  However
> if you did state readout to find that the crtc is enabled, but failed to
> map stolen mem and wrap it in a bo (and fb), you could end up in a
> scenario where crtc->primary->fb is still null on first modeset.  So
> lets check for this rather than exploding.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> As with http://lists.freedesktop.org/archives/dri-devel/2014-September/069341.html
> the root cause is issues with stolen region.  But it would be nice if
> i915 was a bit less fragile when things don't go as planned.
> 
> This could have the side effect that, at first modeset, the crtc's which
> were enabled by BIOS but not yet set (by fbcon or userspace) get disabled.
> This is a somewhat lesser inconvenience compared to opps'ing under
> console_lock.
> 
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d8324c6..9f85033 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10895,6 +10895,9 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> +		if (!intel_crtc->base.primary->fb)
> +			continue;
> +

I think the right fix would be to turn off the primary plane during
the sanitize phase. Although our primary plane code isn't quite up to
the task yet since we'd still try to turn it back on during
.crtc_enable(). The code is somewhat in flux currently and hopefully
soon we'd be able to handle that case correctly.

In the meantime we could potentially disable the whole crtc in
intel_sanitize_crtc() or we could stick a hack into 
intel_enable_primary_hw_plane() so that it won't try to enable the plane
w/o an fb (with a comment stating that it's a temporary kludge).

>  		update_scanline_offset(intel_crtc);
>  
>  		dev_priv->display.crtc_enable(&intel_crtc->base);
> -- 
> 1.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915/vlv: don't try to enable a crtc without fb

[Daniel Vetter]

On Sat, Oct 18, 2014 at 03:56:38PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 17, 2014 at 06:53:34PM -0400, Rob Clark wrote:
> > On valleyview, all enabled pipes get added to the prepare mask.  However
> > if you did state readout to find that the crtc is enabled, but failed to
> > map stolen mem and wrap it in a bo (and fb), you could end up in a
> > scenario where crtc->primary->fb is still null on first modeset.  So
> > lets check for this rather than exploding.
> > 
> > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > ---
> > As with http://lists.freedesktop.org/archives/dri-devel/2014-September/069341.html
> > the root cause is issues with stolen region.  But it would be nice if
> > i915 was a bit less fragile when things don't go as planned.
> > 
> > This could have the side effect that, at first modeset, the crtc's which
> > were enabled by BIOS but not yet set (by fbcon or userspace) get disabled.
> > This is a somewhat lesser inconvenience compared to opps'ing under
> > console_lock.
> > 
> >  drivers/gpu/drm/i915/intel_display.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index d8324c6..9f85033 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10895,6 +10895,9 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> >  
> >  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> >  	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> > +		if (!intel_crtc->base.primary->fb)
> > +			continue;
> > +
> 
> I think the right fix would be to turn off the primary plane during
> the sanitize phase. Although our primary plane code isn't quite up to
> the task yet since we'd still try to turn it back on during
> .crtc_enable(). The code is somewhat in flux currently and hopefully
> soon we'd be able to handle that case correctly.
> 
> In the meantime we could potentially disable the whole crtc in
> intel_sanitize_crtc() or we could stick a hack into 
> intel_enable_primary_hw_plane() so that it won't try to enable the plane
> w/o an fb (with a comment stating that it's a temporary kludge).

Yeah, I think whacking a temporary hack into sanitize_crtc is the better
option. Long term we really want what Ville said and just disable the
primary plane. But until we've fully untangled that mess in prep for
atomic modesets we can't do that just yet.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch