[PATCH] drm/i915: Sanitize cursors on driver load

[PATCH] drm/i915: Sanitize cursors on driver load

[Chris Wilson]

Extremely unlikely to ever be required, but BIOSes do like to attack in
unexpected ways.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a943ea7..5583e9b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11817,6 +11817,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	/* Adjust the state of the output pipe according to whether we
 	 * have active connectors/encoders. */
 	intel_crtc_update_dpms(&crtc->base);
+	intel_crtc_update_cursor(crtc,
+				 intel_crtc->active && intel_crtc->cursor_bo);
 
 	if (crtc->active != crtc->base.enabled) {
 		struct intel_encoder *encoder;
-- 
1.7.9.5

Re: [PATCH] drm/i915: Sanitize cursors on driver load

[Daniel Vetter]

On Fri, May 16, 2014 at 05:36:59PM +0100, Chris Wilson wrote:
> Extremely unlikely to ever be required, but BIOSes do like to attack in
> unexpected ways.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a943ea7..5583e9b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11817,6 +11817,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  	/* Adjust the state of the output pipe according to whether we
>  	 * have active connectors/encoders. */
>  	intel_crtc_update_dpms(&crtc->base);
> +	intel_crtc_update_cursor(crtc,
> +				 intel_crtc->active && intel_crtc->cursor_bo);

Should we do this for sprite planes, too? That way it would be nice fodder
for Matt to clean up later on ;-)
-Daniel
>  
>  	if (crtc->active != crtc->base.enabled) {
>  		struct intel_encoder *encoder;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Sanitize cursors on driver load

[Chris Wilson]

On Fri, May 16, 2014 at 06:53:52PM +0200, Daniel Vetter wrote:
> On Fri, May 16, 2014 at 05:36:59PM +0100, Chris Wilson wrote:
> > Extremely unlikely to ever be required, but BIOSes do like to attack in
> > unexpected ways.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a943ea7..5583e9b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11817,6 +11817,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> >  	/* Adjust the state of the output pipe according to whether we
> >  	 * have active connectors/encoders. */
> >  	intel_crtc_update_dpms(&crtc->base);
> > +	intel_crtc_update_cursor(crtc,
> > +				 intel_crtc->active && intel_crtc->cursor_bo);
> 
> Should we do this for sprite planes, too? That way it would be nice fodder
> for Matt to clean up later on ;-)

* pokes Ville ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Sanitize cursors on driver load

[Ville Syrjälä]

On Fri, May 16, 2014 at 06:02:29PM +0100, Chris Wilson wrote:
> On Fri, May 16, 2014 at 06:53:52PM +0200, Daniel Vetter wrote:
> > On Fri, May 16, 2014 at 05:36:59PM +0100, Chris Wilson wrote:
> > > Extremely unlikely to ever be required, but BIOSes do like to attack in
> > > unexpected ways.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index a943ea7..5583e9b 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11817,6 +11817,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> > >  	/* Adjust the state of the output pipe according to whether we
> > >  	 * have active connectors/encoders. */
> > >  	intel_crtc_update_dpms(&crtc->base);
> > > +	intel_crtc_update_cursor(crtc,
> > > +				 intel_crtc->active && intel_crtc->cursor_bo);
> > 
> > Should we do this for sprite planes, too? That way it would be nice fodder
> > for Matt to clean up later on ;-)
> 
> * pokes Ville ;-)

The problem I see here is that we would end up restoring twice, first in
sanitize while we're still using whatever mode we get handed, and later
when we restore the mode we actually want. So if we start restoring in
sanitize based on the state userspace requested before we suspended,
we might end up in some weird plane flicker land.

Just forcing all the extra planes off in sanitize should be OK. Or
we do the restore only when force_restore==false, which means driver
init and so we can't even have any user space state to worry about.
So really the two options should both result in just turning all
the extra planes off.

-- 
Ville Syrjälä
Intel OTC