[03/10] drm/i915: Use the atomic state in intel_update_primary_planes.
diff mbox

Message ID 1441894085-25662-4-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst Sept. 10, 2015, 2:07 p.m. UTC
This function was still using the legacy state, convert it to atomic.
While we're at it, fix the FIXME too and disable the primary plane.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

Comments

Daniel Vetter Sept. 10, 2015, 3:32 p.m. UTC | #1
On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote:
> This function was still using the legacy state, convert it to atomic.
> While we're at it, fix the FIXME too and disable the primary plane.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 33200403a5db..b68aa95c5460 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
>  
>  static void intel_update_primary_planes(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc;
>  
>  	for_each_crtc(dev, crtc) {
> -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +		struct intel_plane *plane = to_intel_plane(crtc->primary);
> +		struct intel_plane_state *plane_state;
>  
> -		drm_modeset_lock(&crtc->mutex, NULL);
> -		/*
> -		 * FIXME: Once we have proper support for primary planes (and
> -		 * disabling them without disabling the entire crtc) allow again
> -		 * a NULL crtc->primary->fb.
> -		 */

We probably should have a new FIXME here to ditch this all once we use
atomic for legacy page_flips (since the only reason we have this is when
CS flips are lost in the reset).
-Daniel

> -		if (intel_crtc->active && crtc->primary->fb)
> -			dev_priv->display.update_primary_plane(crtc,
> -							       crtc->primary->fb,
> -							       crtc->x,
> -							       crtc->y);
> -		drm_modeset_unlock(&crtc->mutex);
> +		drm_modeset_lock_crtc(crtc, &plane->base);
> +
> +		plane_state = to_intel_plane_state(plane->base.state);
> +
> +		if (plane_state->base.fb)
> +			plane->commit_plane(&plane->base, plane_state);
> +		else if (crtc->state->active)
> +			plane->disable_plane(&plane->base, crtc);
> +
> +		drm_modeset_unlock_crtc(crtc);
>  	}
>  }
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Sept. 10, 2015, 3:43 p.m. UTC | #2
On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote:
> This function was still using the legacy state, convert it to atomic.
> While we're at it, fix the FIXME too and disable the primary plane.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 33200403a5db..b68aa95c5460 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
>  
>  static void intel_update_primary_planes(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *crtc;
>  
>  	for_each_crtc(dev, crtc) {
> -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +		struct intel_plane *plane = to_intel_plane(crtc->primary);
> +		struct intel_plane_state *plane_state;
>  
> -		drm_modeset_lock(&crtc->mutex, NULL);
> -		/*
> -		 * FIXME: Once we have proper support for primary planes (and
> -		 * disabling them without disabling the entire crtc) allow again
> -		 * a NULL crtc->primary->fb.
> -		 */
> -		if (intel_crtc->active && crtc->primary->fb)
> -			dev_priv->display.update_primary_plane(crtc,
> -							       crtc->primary->fb,
> -							       crtc->x,
> -							       crtc->y);
> -		drm_modeset_unlock(&crtc->mutex);
> +		drm_modeset_lock_crtc(crtc, &plane->base);
> +
> +		plane_state = to_intel_plane_state(plane->base.state);
> +
> +		if (plane_state->base.fb)
> +			plane->commit_plane(&plane->base, plane_state);
> +		else if (crtc->state->active)
> +			plane->disable_plane(&plane->base, crtc);

That doesn't make sense. There's no way to disable a plane with a page
flip, so there's simply nothing to do here if the fb is NULL. If we can
trust the plane state to be sane we should just check 'visible' here and
commit the plane in that case.

> +
> +		drm_modeset_unlock_crtc(crtc);
>  	}
>  }
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Sept. 10, 2015, 4:31 p.m. UTC | #3
On Thu, Sep 10, 2015 at 06:43:26PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote:
> > This function was still using the legacy state, convert it to atomic.
> > While we're at it, fix the FIXME too and disable the primary plane.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
> >  1 file changed, 12 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 33200403a5db..b68aa95c5460 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
> >  
> >  static void intel_update_primary_planes(struct drm_device *dev)
> >  {
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc *crtc;
> >  
> >  	for_each_crtc(dev, crtc) {
> > -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +		struct intel_plane *plane = to_intel_plane(crtc->primary);
> > +		struct intel_plane_state *plane_state;
> >  
> > -		drm_modeset_lock(&crtc->mutex, NULL);
> > -		/*
> > -		 * FIXME: Once we have proper support for primary planes (and
> > -		 * disabling them without disabling the entire crtc) allow again
> > -		 * a NULL crtc->primary->fb.
> > -		 */
> > -		if (intel_crtc->active && crtc->primary->fb)
> > -			dev_priv->display.update_primary_plane(crtc,
> > -							       crtc->primary->fb,
> > -							       crtc->x,
> > -							       crtc->y);
> > -		drm_modeset_unlock(&crtc->mutex);
> > +		drm_modeset_lock_crtc(crtc, &plane->base);
> > +
> > +		plane_state = to_intel_plane_state(plane->base.state);
> > +
> > +		if (plane_state->base.fb)
> > +			plane->commit_plane(&plane->base, plane_state);
> > +		else if (crtc->state->active)
> > +			plane->disable_plane(&plane->base, crtc);
> 
> That doesn't make sense. There's no way to disable a plane with a page
> flip, so there's simply nothing to do here if the fb is NULL. If we can
> trust the plane state to be sane we should just check 'visible' here and
> commit the plane in that case.

Right missed that and squash up a fixup that Ville acked on irc. Also
added the FIXME comment too.
-Daniel

> 
> > +
> > +		drm_modeset_unlock_crtc(crtc);
> >  	}
> >  }
> >  
> > -- 
> > 2.1.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Sept. 10, 2015, 4:34 p.m. UTC | #4
On Thu, Sep 10, 2015 at 06:31:02PM +0200, Daniel Vetter wrote:
> On Thu, Sep 10, 2015 at 06:43:26PM +0300, Ville Syrjälä wrote:
> > On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote:
> > > This function was still using the legacy state, convert it to atomic.
> > > While we're at it, fix the FIXME too and disable the primary plane.
> > > 
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
> > >  1 file changed, 12 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 33200403a5db..b68aa95c5460 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
> > >  
> > >  static void intel_update_primary_planes(struct drm_device *dev)
> > >  {
> > > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	struct drm_crtc *crtc;
> > >  
> > >  	for_each_crtc(dev, crtc) {
> > > -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +		struct intel_plane *plane = to_intel_plane(crtc->primary);
> > > +		struct intel_plane_state *plane_state;
> > >  
> > > -		drm_modeset_lock(&crtc->mutex, NULL);
> > > -		/*
> > > -		 * FIXME: Once we have proper support for primary planes (and
> > > -		 * disabling them without disabling the entire crtc) allow again
> > > -		 * a NULL crtc->primary->fb.
> > > -		 */
> > > -		if (intel_crtc->active && crtc->primary->fb)
> > > -			dev_priv->display.update_primary_plane(crtc,
> > > -							       crtc->primary->fb,
> > > -							       crtc->x,
> > > -							       crtc->y);
> > > -		drm_modeset_unlock(&crtc->mutex);
> > > +		drm_modeset_lock_crtc(crtc, &plane->base);
> > > +
> > > +		plane_state = to_intel_plane_state(plane->base.state);
> > > +
> > > +		if (plane_state->base.fb)
> > > +			plane->commit_plane(&plane->base, plane_state);
> > > +		else if (crtc->state->active)
> > > +			plane->disable_plane(&plane->base, crtc);
> > 
> > That doesn't make sense. There's no way to disable a plane with a page
> > flip, so there's simply nothing to do here if the fb is NULL. If we can
> > trust the plane state to be sane we should just check 'visible' here and
> > commit the plane in that case.
> 
> Right missed that and squash up a fixup that Ville acked on irc. Also
> added the FIXME comment too.

Actually I take it back. We just lost the ->active check entirely so now
we're commiting a plane on a disabled pipe potentially.

> -Daniel
> 
> > 
> > > +
> > > +		drm_modeset_unlock_crtc(crtc);
> > >  	}
> > >  }
> > >  
> > > -- 
> > > 2.1.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter Sept. 14, 2015, 9:02 a.m. UTC | #5
On Thu, Sep 10, 2015 at 07:34:05PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 10, 2015 at 06:31:02PM +0200, Daniel Vetter wrote:
> > On Thu, Sep 10, 2015 at 06:43:26PM +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote:
> > > > This function was still using the legacy state, convert it to atomic.
> > > > While we're at it, fix the FIXME too and disable the primary plane.
> > > > 
> > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++--------------
> > > >  1 file changed, 12 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 33200403a5db..b68aa95c5460 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev)
> > > >  
> > > >  static void intel_update_primary_planes(struct drm_device *dev)
> > > >  {
> > > > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > > >  	struct drm_crtc *crtc;
> > > >  
> > > >  	for_each_crtc(dev, crtc) {
> > > > -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > +		struct intel_plane *plane = to_intel_plane(crtc->primary);
> > > > +		struct intel_plane_state *plane_state;
> > > >  
> > > > -		drm_modeset_lock(&crtc->mutex, NULL);
> > > > -		/*
> > > > -		 * FIXME: Once we have proper support for primary planes (and
> > > > -		 * disabling them without disabling the entire crtc) allow again
> > > > -		 * a NULL crtc->primary->fb.
> > > > -		 */
> > > > -		if (intel_crtc->active && crtc->primary->fb)
> > > > -			dev_priv->display.update_primary_plane(crtc,
> > > > -							       crtc->primary->fb,
> > > > -							       crtc->x,
> > > > -							       crtc->y);
> > > > -		drm_modeset_unlock(&crtc->mutex);
> > > > +		drm_modeset_lock_crtc(crtc, &plane->base);
> > > > +
> > > > +		plane_state = to_intel_plane_state(plane->base.state);
> > > > +
> > > > +		if (plane_state->base.fb)
> > > > +			plane->commit_plane(&plane->base, plane_state);
> > > > +		else if (crtc->state->active)
> > > > +			plane->disable_plane(&plane->base, crtc);
> > > 
> > > That doesn't make sense. There's no way to disable a plane with a page
> > > flip, so there's simply nothing to do here if the fb is NULL. If we can
> > > trust the plane state to be sane we should just check 'visible' here and
> > > commit the plane in that case.
> > 
> > Right missed that and squash up a fixup that Ville acked on irc. Also
> > added the FIXME comment too.
> 
> Actually I take it back. We just lost the ->active check entirely so now
> we're commiting a plane on a disabled pipe potentially.

Blergh and I tagged the pile already :( Maarten, can you pls send out the
fixup as a separate patch asap?

Thanks, Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 33200403a5db..b68aa95c5460 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3138,24 +3138,22 @@  static void intel_complete_page_flips(struct drm_device *dev)
 
 static void intel_update_primary_planes(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 
 	for_each_crtc(dev, crtc) {
-		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		struct intel_plane *plane = to_intel_plane(crtc->primary);
+		struct intel_plane_state *plane_state;
 
-		drm_modeset_lock(&crtc->mutex, NULL);
-		/*
-		 * FIXME: Once we have proper support for primary planes (and
-		 * disabling them without disabling the entire crtc) allow again
-		 * a NULL crtc->primary->fb.
-		 */
-		if (intel_crtc->active && crtc->primary->fb)
-			dev_priv->display.update_primary_plane(crtc,
-							       crtc->primary->fb,
-							       crtc->x,
-							       crtc->y);
-		drm_modeset_unlock(&crtc->mutex);
+		drm_modeset_lock_crtc(crtc, &plane->base);
+
+		plane_state = to_intel_plane_state(plane->base.state);
+
+		if (plane_state->base.fb)
+			plane->commit_plane(&plane->base, plane_state);
+		else if (crtc->state->active)
+			plane->disable_plane(&plane->base, crtc);
+
+		drm_modeset_unlock_crtc(crtc);
 	}
 }