Re: [PATCH v8 35/39] drm/tidss: Implement readout support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential
issues to consider:
- [High] drm/tidss: dispc_vp_init() clears DPIENABLE and dpi_output is
  unset on readout
- [High] drm/tidss: tidss_crtc_install_state() runs before
  tidss_irq_install() in probe
- [Medium] drm/tidss: missing drm_crtc_vblank_reset() in
  tidss_crtc_install_state()
- [Medium] drm/tidss: wrong channel ID comparison in
  dispc_ovr_get_plane()
- [Medium] drm/tidss: plane readout fails in
  tidss_plane_atomic_readout_state() when CRTC is disabled
- [Medium] drm/tidss: zpos and cropping mismatches in
  tidss_plane_atomic_readout_state()
- [Medium] drm/tidss: refcount imbalance and leak from
  tidss_crtc_install_state()
- [Medium] drm/tidss: bus_flags mismatch in dispc_vp_get_bus_flags()
  readout
--

commit 3df04a68595619e1ce0b980646624376e9262bc8
Author: Maxime Ripard <mripard@kernel.org>

drm/tidss: Implement readout support

This commit implements KMS hardware state readout callbacks for the tidss
CRTC, encoder bridge, plane, and DISPC layers.

> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index d2664bda53a99..38d7a5dfe4f3d 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
[ ... ]
> @@ -372,6 +376,108 @@ static struct drm_crtc_state *tidss_crtc_create_state(struct drm_crtc *crtc)
>  	return &tstate->base;
>  }
>  
> +static int tidss_crtc_readout_state(struct drm_crtc *crtc,
> +				    struct drm_atomic_sro_state *state,
> +				    struct drm_crtc_state *crtc_state)
> +{
> +	struct drm_device *ddev = crtc->dev;
> +	struct tidss_device *tidss = to_tidss(ddev);
> +	struct dispc_device *dispc = tidss->dispc;
> +	struct tidss_crtc_state *tstate = to_tidss_crtc_state(crtc_state);
> +	struct tidss_crtc *tcrtc =
> +		to_tidss_crtc(crtc);
> +	struct drm_display_mode mode = {};
> +	int ret;
> +
> +	tidss_runtime_get(tidss);
> +
> +	if (!dispc_vp_is_enabled(dispc, tcrtc->hw_videoport))
> +		goto out;

[Severity: High]
Does the first tidss_runtime_get() during boot readout disable active DPI
outputs on SoCs with has_vp_control_dpienable set (AM62L)?

During tidss_probe(), the device starts runtime-suspended, so the first
tidss_runtime_get() calls dispc_runtime_resume() -> dispc_initial_config() ->
dispc_vp_init():

drivers/gpu/drm/tidss/tidss_dispc.c:dispc_vp_init() {
	for (i = 0; i < dispc->feat->num_vps; i++) {
		VP_REG_FLD_MOD(dispc, i, DISPC_VP_CONFIG, 1,
			       DISPC_VP_CONFIG_GAMMAENABLE_MASK);

		if (dispc->feat->has_vp_control_dpienable) {
			/* Disable DPIENABLE for all VPs */
			VP_REG_FLD_MOD(dispc, i, DISPC_VP_CONTROL, 0,
				       DISPC_VP_CONTROL_DPIENABLE_MASK);
		}
	}
}

Because DISPC_VP_CONTROL_ENABLE_MASK stays set, dispc_vp_is_enabled() still
returns true and state readout succeeds without a full modeset, leaving
DISPC_VP_CONTROL_DPIENABLE_MASK cleared and the DPI display blank.

In addition, dispc->vp_data[hw_videoport].dpi_output is only populated in
dispc_vp_setup() during tidss_crtc_atomic_enable() or
tidss_crtc_atomic_flush(), so tidss_crtc_install_state() leaves it false on a
read-out CRTC and a subsequent tidss_crtc_atomic_disable() skips clearing
DISPC_VP_CONTROL_DPIENABLE_MASK in dispc_vp_unprepare().

[ ... ]

> +static void tidss_crtc_install_state(struct drm_crtc *crtc,
> +				     struct drm_crtc_state *crtc_state)
> +{
> +	struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
> +	struct drm_device *ddev = crtc->dev;
> +	struct tidss_device *tidss = to_tidss(ddev);
> +	int ret;
> +
> +	/*
> +	 * The video port wasn't enabled in hardware: nothing to take
> +	 * over for it, and enabling the clock and the vblank reporting
> +	 * would leak their reference for a CRTC that no disable will
> +	 * ever release.
> +	 */
> +	if (!crtc_state->active)
> +		return;

[Severity: Medium]
Should the inactive branch call drm_crtc_vblank_reset(crtc) before returning?

When drm_mode_config_create_initial_state() succeeds via state readout, it
skips the drm_mode_config_create_state() fallback that normally calls
drm_crtc_vblank_reset().  Returning early here leaves vblank->inmodeset at 0
after drm_vblank_init(), allowing userspace vblank ioctls such as
DRM_IOCTL_WAIT_VBLANK to enable vblank interrupts on a disabled video port
instead of failing with -EINVAL.

> +
> +	tidss_runtime_get(tidss);
> +
> +	ret = dispc_vp_enable_clk(tidss->dispc, tcrtc->hw_videoport);
> +	if (ret != 0) {
> +		tidss_runtime_put(tidss);
> +		return;
> +	}

[Severity: Medium]
If dispc_vp_enable_clk() fails here, or if tidss_probe() fails after
drm_mode_config_create_initial_state(), does this cause a clock and runtime
PM refcount imbalance?

When dispc_vp_enable_clk() fails, tidss_crtc_install_state() drops the
runtime PM reference and returns without enabling vp_clk, but crtc->state is
still installed with active = true, so a later tidss_crtc_atomic_disable()
unconditionally calls drm_crtc_vblank_off(), dispc_vp_disable_clk(), and
tidss_runtime_put(), underflowing both counts.  Conversely, if tidss_probe()
fails in platform_get_irq(), tidss_irq_install(), drm_dev_register(), or
aperture_remove_all_conflicting_devices(), the error path never unwinds the
vp_clk and runtime PM references acquired here for active CRTCs.

> +
> +	/* Turn vertical blanking interrupt reporting on. */\
> +	drm_crtc_vblank_on(crtc);
> +}

[Severity: High]
Does running tidss_crtc_install_state() before tidss_irq_install() in
tidss_probe() leave hardware interrupts unprogrammed while a read-out CRTC is
active?

In tidss_probe(), drm_mode_config_create_initial_state() runs before
tidss_irq_install():

drivers/gpu/drm/tidss/tidss_drv.c:tidss_probe() {
    ...
	ret = drm_mode_config_create_initial_state(ddev);
	if (ret) {
		dev_err(dev, "failed to create initial state: %d\n", ret);
		goto err_runtime_suspend;
	}

	irq = platform_get_irq(pdev, 0);
	if (irq < 0) {
		ret = irq;
		goto err_runtime_suspend;
	}
	tidss->irq = irq;

	ret = tidss_irq_install(ddev, irq);
    ...
}

When readout finds an active CRTC, tidss_crtc_install_state() keeps the
device runtime-active and calls drm_crtc_vblank_on(crtc).  tidss_irq_install()
then resets tidss->irq_mask = 0 (wiping any vblank IRQ bits set when
drm_vblank_offdelay == 0) and populates DSS_IRQ_VP_FRAME_DONE,
DSS_IRQ_VP_SYNC_LOST, and DSS_IRQ_PLANE_FIFO_UNDERFLOW in tidss->irq_mask
without writing the mask to hardware.  Because the device is already runtime
active, dispc_runtime_resume() -> tidss_irq_resume() does not run again, so
FRAME_DONE remains disabled in hardware and a subsequent
tidss_crtc_atomic_disable() times out waiting for framedone_completion.

> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 8720403af9ed3..c3117745965b7 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
[ ... ]
> @@ -1474,6 +1480,228 @@ int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
[ ... ]
> +u32 dispc_vp_get_bus_flags(struct dispc_device *dispc, u32 hw_videoport)
> +{
> +	u32 flags = 0;
> +	u32 val;
> +
> +	val = dispc_vp_read(dispc, hw_videoport, DISPC_VP_POL_FREQ);
> +	if (FIELD_GET(DISPC_VP_POL_FREQ_IPC_MASK, val))
> +		flags |= DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE;
> +	else
> +		flags |= DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE;
> +
> +	if (FIELD_GET(DISPC_VP_POL_FREQ_IEO_MASK, val))
> +		flags |= DRM_BUS_FLAG_DE_LOW;
> +	else
> +		flags |= DRM_BUS_FLAG_DE_HIGH;
> +
> +	if (FIELD_GET(DISPC_VP_POL_FREQ_RF_MASK, val))
> +		flags |= DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE;
> +	else
> +		flags |= DRM_BUS_FLAG_SYNC_DRIVE_NEGEDGE;
> +
> +	return flags;
> +}

[Severity: Medium]
Will dispc_vp_get_bus_flags() cause state comparison mismatches in
tidss_crtc_compare_state() and drm_atomic_helper_bridge_compare_state() when a
panel or bridge leaves some polarity flags unset?

In tidss_bridge_atomic_check(), tcrtc_state->bus_flags is copied directly
from the next bridge or connector display_info without normalizing unset
polarity bits, and dispc_vp_prepare() defaults unset bits in hardware to
POSEDGE / DE_HIGH / NEGEDGE (for example, rocktech_rk101ii01d_ct on AM654
OLDI only sets DRM_BUS_FLAG_DE_HIGH).  Because dispc_vp_get_bus_flags()
unconditionally sets all three polarity flags, the read-out bus_flags will
not match the committed bus_flags after a blocking commit.

> +
>  /* OVR */
>  static void dispc_k2g_ovr_set_plane(struct dispc_device *dispc,
>  				    u32 hw_plane, u32 hw_videoport,
> @@ -1547,6 +1775,94 @@ void dispc_ovr_enable_layer(struct dispc_device *dispc,
[ ... ]
> +int dispc_ovr_get_plane(struct dispc_device *dispc, u32 hw_plane,
> +			u32 *hw_videoport, int *x, int *y, u32 *layer)
> +{
> +	u32 vp, l;
> +
> +	switch (dispc->feat->subrev) {
[ ... ]
> +	case DISPC_AM625:
> +	case DISPC_AM62A7:
> +	case DISPC_AM62L:
> +	case DISPC_AM65X:
> +	case DISPC_J721E:
> +		for (vp = 0; vp < dispc->feat->num_vps; vp++) {
> +			for (l = 0; l < dispc->feat->num_vids; l++) {
> +				u32 attrs = dispc_ovr_read(dispc, vp,
> +							   DISPC_OVR_ATTRIBUTES(l));
> +
> +				if (!(attrs & DISPC_OVR_ATTRIBUTES_ENABLE_MASK))
> +					continue;
> +
> +				if (FIELD_GET(DISPC_OVR_ATTRIBUTES_CHANNELIN_MASK,
> +					      attrs) != hw_plane)
> +					continue;

[Severity: Medium]
Should DISPC_OVR_ATTRIBUTES_CHANNELIN_MASK be compared against
dispc->feat->vid_info[hw_plane].hw_id instead of hw_plane?

dispc_am65x_ovr_set_plane() and dispc_j721e_ovr_set_plane() program CHANNELIN
with dispc->feat->vid_info[hw_plane].hw_id.  On DISPC_AM62L, the single plane
has software index hw_plane = 0 but vid_info[0].hw_id = 1, so
dispc_ovr_get_plane() never matches the enabled layer and returns -ENODEV,
breaking both initial state readout and post-commit readout verification.

> diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
> index b0d5d6959b885..94824cff77f9d 100644
> --- a/drivers/gpu/drm/tidss/tidss_plane.c
> +++ b/drivers/gpu/drm/tidss/tidss_plane.c
[ ... ]
> @@ -176,11 +179,213 @@ static const struct drm_plane_helper_funcs tidss_primary_plane_helper_funcs = {
[ ... ]
> +static int tidss_plane_atomic_readout_state(struct drm_plane *plane,
> +					    struct drm_atomic_sro_state *state,
> +					    struct drm_plane_state *plane_state)
> +{
> +	struct drm_device *ddev = plane->dev;
> +	struct tidss_device *tidss = to_tidss(ddev);
> +	struct dispc_device *dispc = tidss->dispc;
> +	struct tidss_plane *tplane = to_tidss_plane(plane);
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_framebuffer *fb;
> +	struct drm_crtc *crtc;
> +	unsigned int in_w, in_h;
> +	int crtc_x, crtc_y;
> +	u32 normalized_zpos;
> +	int ret;
> +
> +	tidss_runtime_get(tidss);
> +
> +	if (!dispc_plane_is_enabled(dispc, tplane->hw_plane_id))
> +		goto out;
> +
> +	fb = tidss_plane_readout_fb(plane);
> +	if (IS_ERR(fb)) {
> +		ret = PTR_ERR(fb);
> +		goto err_runtime_pm;
> +	}
> +
> +	crtc = tidss_plane_readout_crtc(plane, &crtc_x, &crtc_y,
> +					&normalized_zpos);
> +	if (IS_ERR(crtc)) {
> +		drm_framebuffer_put(fb);
> +		ret = PTR_ERR(crtc);
> +		goto err_runtime_pm;
> +	}

[Severity: Medium]
Does plane state readout fail or report stale active planes when a CRTC is
disabled?

When a CRTC is disabled, tidss_crtc_atomic_disable() disables the OVR layers
and video port, while tidss_atomic_commit_tail() passes
DRM_PLANE_COMMIT_ACTIVE_ONLY to drm_atomic_helper_commit_planes(), skipping
tidss_plane_atomic_disable() on the newly inactive CRTC and leaving
DISPC_VID_ATTRIBUTES_ENABLE_MASK set in hardware.  When post-commit readout
runs:
- on K3 SoCs, dispc_ovr_get_plane() finds no enabled OVR layer and returns
  -ENODEV, which tidss_plane_atomic_readout_state() treats as a fatal error
  instead of an unrouted/inactive plane;
- on K2G, dispc_ovr_get_plane() unconditionally returns videoport 0 even when
  the video port is disabled, and tidss_plane_atomic_readout_state() never
  checks crtc_state->active before marking the plane visible and attaching it
  to the disabled CRTC.

> +
> +	plane_state->fb = fb;
> +	plane_state->crtc = crtc;
> +	plane_state->visible = true;
> +
> +	dispc_plane_get_picture_size(dispc, tplane->hw_plane_id, &in_w, &in_h);
> +	plane_state->src_w = in_w << 16;
> +	plane_state->src_h = in_h << 16;
> +
> +	dispc_plane_get_size(dispc, tplane->hw_plane_id, &plane_state->crtc_w,
> +			     &plane_state->crtc_h);
> +
> +	plane_state->crtc_x = crtc_x;
> +	plane_state->crtc_y = crtc_y;
> +
> +	/*
> +	 * The overlay layer index a plane is routed to is its
> +	 * normalized zpos, as that is what the atomic update uses as
> +	 * the layer index. The raw zpos property userspace set
> +	 * cannot be recovered from the hardware.
> +	 */
> +	plane_state->normalized_zpos = normalized_zpos;
> +	plane_state->zpos = normalized_zpos;
> +
> +	// TODO: Handle src_x/src_y, see DISPC_VID_BA_0
> +
> +	plane_state->src.x1 = 0;
> +	plane_state->src.x2 = plane_state->src_w;
> +	plane_state->src.y1 = 0;
> +	plane_state->src.y2 = plane_state->src_h;

[Severity: Medium]
Will using drm_atomic_helper_plane_compare_state() trigger false mismatch
warnings whenever zpos != normalized_zpos or a plane is cropped?

tidss_plane_create() initializes each plane's default zpos to its global
plane index (0, 1, 2, ...), whereas drm_atomic_normalize_zpos() normalizes
normalized_zpos per CRTC starting from 0.  Whenever plane 1 (such as the
primary plane on a second CRTC) or an overlay plane is enabled without all
lower-numbered planes on the same CRTC, expected->zpos differs from
normalized_zpos and STATE_CHECK_U32() on zpos in
drm_atomic_helper_plane_compare_state() fails.

Similarly, when a plane uses source cropping or non-zero src_x/src_y offsets,
tidss_plane_readout_fb() sets fb->width and fb->height from
DISPC_VID_PICTURE_SIZE (the cropped source dimensions) and
tidss_plane_atomic_readout_state() leaves src_x and src_y at 0, causing
drm_atomic_helper_plane_compare_state() to warn on fb->width, fb->height,
src_x, src_y, and src.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-drm-state-readout-v8-0-cbe78624e1bc@kernel.org?part=35