Re: [PATCH v9 04/16] drm/imx: dc-kms: Use driver specific .atomic_commit_tail callback

[Liu Ying <victor.liu@nxp.com>]

On Tue, Nov 03, 2026 at 06:39:15AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] drm/imx: dc: plane disable skipped on modeset in dc_drm_atomic_commit_tail()
> --
> 
> commit ff022f683849ca82589cd00028c8fb7c412423dc
> Author: Liu Ying <victor.liu@nxp.com>
> 
> drm/imx: dc-kms: Use driver specific .atomic_commit_tail callback
> 
> This commit introduces a custom atomic_commit_tail callback for the i.MX8qxp
> Display Controller KMS driver that passes DRM_PLANE_COMMIT_ACTIVE_ONLY and
> DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET to drm_atomic_helper_commit_planes()
> to avoid accessing plane registers while clocks are disabled.
> 
> > diff --git a/drivers/gpu/drm/imx/dc/dc-kms.c b/drivers/gpu/drm/imx/dc/dc-kms.c
> > index 0e2ce6a425487..9ffa15ac43a25 100644
> > --- a/drivers/gpu/drm/imx/dc/dc-kms.c
> > +++ b/drivers/gpu/drm/imx/dc/dc-kms.c
> [ ... ]
> > @@ -30,6 +31,29 @@ static const struct drm_mode_config_funcs dc_drm_mode_config_funcs = {
> >  	.atomic_commit = drm_atomic_helper_commit,
> >  };
> >  
> > +static void dc_drm_atomic_commit_tail(struct drm_atomic_commit *state)
> > +{
> > +	struct drm_device *dev = state->dev;
> > +
> > +	drm_atomic_helper_commit_modeset_disables(dev, state);
> > +
> > +	drm_atomic_helper_commit_planes(dev, state,
> > +					DRM_PLANE_COMMIT_ACTIVE_ONLY |
> > +					DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET);
> 
> [Severity: High]
> Could passing DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET (and
> DRM_PLANE_COMMIT_ACTIVE_ONLY) here leave the plane's FetchUnit source buffer
> and LayerBlend enabled in hardware after the old framebuffer is cleaned up?

No, because the plane's CRTC is being disabled, all hardware units backing
the plane will effectively stop working together with the disabled CRTC.

> 
> In drm_atomic_helper_commit_planes(), DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET
> skips calling funcs->atomic_disable() when the CRTC needs a modeset:
> 
>     if (disabling && funcs->atomic_disable) {
>         struct drm_crtc_state *crtc_state;
> 
>         crtc_state = old_plane_state->crtc->state;
> 
>         if (drm_atomic_crtc_needs_modeset(crtc_state) &&
>             no_disable)
>             continue;
> 
>         funcs->atomic_disable(plane, state);
>     }
> 
> That flag is meant for drivers where the CRTC atomic_disable callback also
> disables the CRTC's planes. However, in drivers/gpu/drm/imx/dc/dc-crtc.c,
> dc_crtc_atomic_disable() only disables FrameGen and calls pm_runtime_put() on
> the Display Engine and the shared Pixel Engine without disabling the planes:
> 
>     enable_irq(dc_crtc->irq_dec_seqcomplete);
>     dc_fg_disable(dc_crtc->fg);
>     DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
>     disable_irq(dc_crtc->irq_dec_seqcomplete);
> 
>     dc_fg_disable_clock(dc_crtc->fg);
> 
>     /* request pixel engine power-off as plane is off too */
>     pm_runtime_put(dc_drm->pe->dev);
> 
> And in drivers/gpu/drm/imx/dc/dc-plane.c, __dc_plane_atomic_disable() is what
> actually disables the FetchUnit source buffer and LayerBlend:
> 
>     /* disable fetchunit in shadow */
>     fu_ops = dc_fu_get_ops(dplane->fu);
>     fu_ops->disable_src_buf(dplane->fu, DC_FETCHUNIT_FRAC0);
> 
>     /* set ExtDst's source to ConstFrame */
>     dc_ed_pec_src_sel(dplane->ed, dc_cf_get_link_id(dplane->cf));
> 
> When dc_crtc_atomic_enable() runs with new_crtc_state->plane_mask == 0, it
> switches ed_cont's source selection to cf_cont, but it does not call
> fu_ops->disable_src_buf():

That's fine, because FetchUnit and LayerBlend won't work if ed_cont's source
selection is switched to cf_cont.

> 
>     /* show CRTC background if no plane is enabled */
>     if (new_crtc_state->plane_mask == 0) {
>         /* constframe in content stream shows black frame */
>         dc_cf_constantcolor_black(dc_crtc->cf_cont);
> 
>         cf_link = dc_cf_get_link_id(dc_crtc->cf_cont);
>         dc_ed_pec_src_sel(dc_crtc->ed_cont, cf_link);
>     }

[...]

-- 
Regards,
Liu Ying