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); }
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);
}
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Laurent Pinchart