[PATCH 10/10] drm/tidss: Fix atomic_flush check
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Nov 1 14:56:58 UTC 2023
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 at 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;
>
> /*
--
Regards,
Laurent Pinchart
More information about the dri-devel
mailing list