| Message ID | 20231101-tidss-probe-v1-10-45149e0f9415@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tidss: Probe related fixes and cleanups | expand |
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@ideasonboard.com> > --- > 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 ? 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. > return; > > /*
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@ideasonboard.com> >> --- >> 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
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. Candidate for stable? Add a Fixes tag? Francesco
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); /* There is nothing to do if CRTC is not going to be enabled. */ - if (!crtc->state->enable) + if (!crtc->state->active) return; /*
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@ideasonboard.com> --- drivers/gpu/drm/tidss/tidss_crtc.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)