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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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