[PATCH 3/3] drm/atomic-helper: Don't call atomic_update_plane when it stays off

[PATCH 3/3] drm/atomic-helper: Don't call atomic_update_plane when it stays off

[Daniel Vetter]

It's a silly thing to do and surprises driver writers. Most likely
this did already blow up for exynos.

It's also a silly thing to change plane state when it's off, but fbdev
is silly (it does an unconditional modeset over all planes). And
userspace can be evil. So I think we need this.

With this check in the helpers we can remove the one in i915 code for
the same conditions (becuase ->crtc iff ->fb).

Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: dri-devel@lists.freedesktop.org
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c       | 3 ++-
 drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 41c38edade74..e1556143d811 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1216,7 +1216,8 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 		if (drm_atomic_plane_disabling(plane, old_plane_state) &&
 		    funcs->atomic_disable)
 			funcs->atomic_disable(plane, old_plane_state);
-		else
+		else if (plane->state->crtc ||
+			 drm_atomic_plane_disabling(plane, old_plane_state))
 			funcs->atomic_update(plane, old_plane_state);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 976b89156570..cb383a0fc392 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -172,10 +172,6 @@ static void intel_plane_atomic_update(struct drm_plane *plane,
 	struct intel_plane_state *intel_state =
 		to_intel_plane_state(plane->state);
 
-	/* Don't disable an already disabled plane */
-	if (!plane->state->fb && !old_state->fb)
-		return;
-
 	intel_plane->commit_plane(plane, intel_state);
 }
 
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/atomic-helper: Don't call atomic_update_plane when it stays off

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Friday 10 April 2015 16:22:39 Daniel Vetter wrote:
> It's a silly thing to do and surprises driver writers. Most likely
> this did already blow up for exynos.
> 
> It's also a silly thing to change plane state when it's off, but fbdev
> is silly (it does an unconditional modeset over all planes). And
> userspace can be evil. So I think we need this.
> 
> With this check in the helpers we can remove the one in i915 code for
> the same conditions (becuase ->crtc iff ->fb).
> 
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

(with the ongoing omapdrm atomic update conversion work)

> ---
>  drivers/gpu/drm/drm_atomic_helper.c       | 3 ++-
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ----
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index 41c38edade74..e1556143d811
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1216,7 +1216,8 @@ void drm_atomic_helper_commit_planes(struct drm_device
> *dev, if (drm_atomic_plane_disabling(plane, old_plane_state) &&
>  		    funcs->atomic_disable)
>  			funcs->atomic_disable(plane, old_plane_state);
> -		else
> +		else if (plane->state->crtc ||
> +			 drm_atomic_plane_disabling(plane, old_plane_state))
>  			funcs->atomic_update(plane, old_plane_state);

The test is so trivial that I wonder whether it makes sense to make 
atomic_disable() optional. Wouldn't it be easier to either make 
atomic_disable() mandatory, or to remove it completely ?

>  	}
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
> b/drivers/gpu/drm/i915/intel_atomic_plane.c index
> 976b89156570..cb383a0fc392 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -172,10 +172,6 @@ static void intel_plane_atomic_update(struct drm_plane
> *plane, struct intel_plane_state *intel_state =
>  		to_intel_plane_state(plane->state);
> 
> -	/* Don't disable an already disabled plane */
> -	if (!plane->state->fb && !old_state->fb)
> -		return;
> -
>  	intel_plane->commit_plane(plane, intel_state);
>  }

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/atomic-helper: Don't call atomic_update_plane when it stays off

[Daniel Vetter]

On Thu, Apr 16, 2015 at 12:46:53AM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Friday 10 April 2015 16:22:39 Daniel Vetter wrote:
> > It's a silly thing to do and surprises driver writers. Most likely
> > this did already blow up for exynos.
> > 
> > It's also a silly thing to change plane state when it's off, but fbdev
> > is silly (it does an unconditional modeset over all planes). And
> > userspace can be evil. So I think we need this.
> > 
> > With this check in the helpers we can remove the one in i915 code for
> > the same conditions (becuase ->crtc iff ->fb).
> > 
> > Cc: Gustavo Padovan <gustavo@padovan.org>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: Inki Dae <inki.dae@samsung.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> (with the ongoing omapdrm atomic update conversion work)
> 
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c       | 3 ++-
> >  drivers/gpu/drm/i915/intel_atomic_plane.c | 4 ----
> >  2 files changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c index 41c38edade74..e1556143d811
> > 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1216,7 +1216,8 @@ void drm_atomic_helper_commit_planes(struct drm_device
> > *dev, if (drm_atomic_plane_disabling(plane, old_plane_state) &&
> >  		    funcs->atomic_disable)
> >  			funcs->atomic_disable(plane, old_plane_state);
> > -		else
> > +		else if (plane->state->crtc ||
> > +			 drm_atomic_plane_disabling(plane, old_plane_state))
> >  			funcs->atomic_update(plane, old_plane_state);
> 
> The test is so trivial that I wonder whether it makes sense to make 
> atomic_disable() optional. Wouldn't it be easier to either make 
> atomic_disable() mandatory, or to remove it completely ?

Yeah it's fairly simple. I think it's more to give drivers some guidance
and to more closely resemble the legacy plane entry points which also had
the split between update and disable. Imo doesn't really hurt to keep this
around.

Merged to topic/drm-misc, thanks for the feedback.
-Daniel

> 
> >  	}
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > b/drivers/gpu/drm/i915/intel_atomic_plane.c index
> > 976b89156570..cb383a0fc392 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -172,10 +172,6 @@ static void intel_plane_atomic_update(struct drm_plane
> > *plane, struct intel_plane_state *intel_state =
> >  		to_intel_plane_state(plane->state);
> > 
> > -	/* Don't disable an already disabled plane */
> > -	if (!plane->state->fb && !old_state->fb)
> > -		return;
> > -
> >  	intel_plane->commit_plane(plane, intel_state);
> >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel