On 01/11/2023 16:56, Laurent Pinchart wrote:
Hi Tomi,
Thank you for the patch.
On Wed, Nov 01, 2023 at 11:17:47AM +0200, Tomi Valkeinen wrote:
tidss_crtc_atomic_flush() checks if the crtc is enabled, and if not,
returns immediately as there's no reason to do any register changes.
However, the code checks for 'crtc->state->enable', which does not
reflect the actual HW state. We should instead look at the
'crtc->state->active' flag.
This causes the tidss_crtc_atomic_flush() to proceed with the flush even
if the active state is false, which then causes us to hit the
WARN_ON(!crtc->state->event) check.
Fix this by checking the active flag, and while at it, fix the related
debug print which had "active" and "needs modeset" wrong way.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/tidss/tidss_crtc.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 5e5e466f35d1..4c7009a5d643 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -169,13 +169,12 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
struct tidss_device *tidss = to_tidss(ddev);
unsigned long flags;
- dev_dbg(ddev->dev,
- "%s: %s enabled %d, needs modeset %d, event %p\n", __func__,
- crtc->name, drm_atomic_crtc_needs_modeset(crtc->state),
- crtc->state->enable, crtc->state->event);
+ dev_dbg(ddev->dev, "%s: %s active %d, needs modeset %d, event %p\n",
+ __func__, crtc->name, crtc->state->active,
+ drm_atomic_crtc_needs_modeset(crtc->state), crtc->state->event);
While at it, how about this ?
Why not. The active part won't be needed if we use
DRM_PLANE_COMMIT_ACTIVE_ONLY, though.
dev_dbg(ddev->dev, "%s: %s is %sactive, %s modeset, event %p\n",
__func__, crtc->name, crtc->state->active ? "" : "not ",
drm_atomic_crtc_needs_modeset(crtc->state) ? "needs", "doesn't need",
crtc->state->event);
/* There is nothing to do if CRTC is not going to be enabled. */
- if (!crtc->state->enable)
+ if (!crtc->state->active)
I think the drm_atomic_helper_commit_planes() helper will handle this if
you pass it the DRM_PLANE_COMMIT_ACTIVE_ONLY flag.
I considered it, but it does a bit more, as it also affects the plane
updates. We specifically did not use DRM_PLANE_COMMIT_ACTIVE_ONLY as it
didn't work with DSS.
That said, I can't figure out what was the issue. It's possible the
issue was only on the older DSS HW, with omapdrm (tidss code was
originally somewhat based on omapdrm). I'm pretty sure the issue was
related to multi-display systems and the plane updates there. But I
don't have any multi-display board which uses tidss.
So, I'll keep this patch, but add another on top, which uses
DRM_PLANE_COMMIT_ACTIVE_ONLY. Then it'll be easy to revert the
DRM_PLANE_COMMIT_ACTIVE_ONLY one if needed, while still keeping this fix.
Tomi
