Re: [PATCH v7 32/34] drm/tidss: Implement readout support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm/tidss: tidss_crtc_install_state() enables inactive CRTCs and leaks PM
- [High] drm/tidss: missing drm_mode_config_create_initial_state() in tidss_probe()
- [High] drm/tidss: GEM-less readout fb fails prepare_fb() and zeroes DMA address
- [Medium] drm/tidss: tidss_crtc_compare_state() checks bus config on inactive CRTCs
- [Medium] drm/tidss: plane readout incomplete state and -EINVAL trigger WARN_ON on commit
- [Medium] drm/tidss: incomplete crtc_state->plane_mask readout on partial commits
--

commit 8f81eb167182a1d35a4ac3bf95240f371e285879
Author: Maxime Ripard <mripard@kernel.org>

drm/tidss: Implement readout support

This commit implements hardware state readout and state comparison callbacks
for CRTCs, bridges, and planes in the tidss driver using the DRM atomic state
readout helpers.

> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 3da49f77fa080..4409f8c9114bb 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
[ ... ]
> @@ -370,6 +374,90 @@ 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;
> +
> +	tstate->base.active = 1;
> +	tstate->base.enable = 1;
> +
> +	ret = dispc_vp_readout_mode(dispc, tcrtc->hw_videoport, &mode);
> +	if (ret)
> +		goto err_runtime_put;
> +
> +	ret = drm_atomic_set_mode_for_crtc(&tstate->base, &mode);
> +	if (WARN_ON(ret))
> +		goto err_runtime_put;
> +
> +	drm_mode_copy(&tstate->base.adjusted_mode, &mode);
> +
> +	tstate->bus_flags = dispc_vp_get_bus_flags(dispc, tcrtc->hw_videoport);
> +	ret = dispc_vp_get_bus_format(dispc, tcrtc->hw_videoport,
> +				      &tstate->bus_format);
> +	if (ret)
> +		goto err_runtime_put;
> +
> +	/*
> +	 * The active connectors and planes will be filled by their
> +	 * respective readout callbacks.
> +	 */

[Severity: Medium]
What happens to crtc_state->plane_mask (as well as connector_mask and
encoder_mask) during post-commit verification in
drm_atomic_sro_readout_and_compare() when a blocking commit includes the CRTC
but only a subset of its active planes?

When drm_atomic_sro_readout_and_compare(commit) calls
drm_atomic_helper_sro_build_state(dev, commit) with a non-NULL commit,
drm_atomic_helper_sro_readout_planes_state() only invokes
atomic_sro_readout_state() for planes present in that commit.

If two planes are active on a CRTC and userspace performs a blocking page
flip or property update on only one of them (without changing x/y or zpos, so
tidss_atomic_check() does not call drm_atomic_add_affected_planes()),
tidss_plane_atomic_check() adds the CRTC to the commit via
drm_atomic_get_crtc_state(), but the second active plane is not in the commit.

Because tidss_crtc_readout_state() leaves crtc_state->plane_mask at 0 and
relies on tidss_plane_atomic_readout_state() to OR in drm_plane_mask(plane),
won't the untouched plane's bit be missing from the readout CRTC state's
plane_mask, causing drm_atomic_helper_crtc_compare_state() to report a
plane_mask mismatch?

> +
> +out:
> +	tidss_runtime_put(tidss);
> +	return 0;
> +
> +err_runtime_put:
> +	tidss_runtime_put(tidss);
> +	return ret;
> +}
> +
> +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;
> +
> +	tidss_runtime_get(tidss);
> +
> +	ret = dispc_vp_enable_clk(tidss->dispc, tcrtc->hw_videoport);
> +	if (ret != 0)
> +		return;
> +
> +	/* Turn vertical blanking interrupt reporting on. */
> +	drm_crtc_vblank_on(crtc);
> +}

[Severity: High]
Should tidss_crtc_install_state() check whether crtc_state->active is set
before enabling runtime PM, the videoport clock, and vblank reporting?

When a videoport is disabled in hardware (!dispc_vp_is_enabled()),
tidss_crtc_readout_state() jumps to out and returns 0 with
crtc_state->active == false, and drm_atomic_helper_sro_readout_crtc_state()
stores that inactive state in state->crtcs[i].state.

Later, drm_atomic_sro_install_state() iterates over all CRTCs and invokes
atomic_sro_install_state() for every non-NULL crtc_state:

drivers/gpu/drm/drm_atomic_state_readout.c:drm_atomic_sro_install_state() {
    ...
		crtc_state = state->crtcs[i].state;
		if (!crtc_state)
			continue;

		if (crtc_funcs->atomic_sro_install_state)
			crtc_funcs->atomic_sro_install_state(crtc, crtc_state);
    ...
}

For an inactive CRTC, won't tidss_crtc_install_state() unconditionally call
tidss_runtime_get(), dispc_vp_enable_clk(), and drm_crtc_vblank_on(), leaking
both the runtime PM reference and the videoport clock enable count because
crtc->state->active is false (so tidss_crtc_atomic_disable() will not run to
release them)?

Also, if dispc_vp_enable_clk() returns a non-zero error in
tidss_crtc_install_state(), doesn't the early return leak the runtime PM
reference acquired by tidss_runtime_get(tidss)?

> +
> +static bool tidss_crtc_compare_state(struct drm_crtc *crtc,
> +				     struct drm_printer *p,
> +				     struct drm_crtc_state *expected,
> +				     struct drm_crtc_state *actual)
> +{
> +	struct tidss_crtc_state *t_expected = to_tidss_crtc_state(expected);
> +	struct tidss_crtc_state *t_actual = to_tidss_crtc_state(actual);
> +	bool ret = drm_atomic_helper_crtc_compare_state(crtc, p, expected, actual);
> +
> +	STATE_CHECK_U32_X(ret, p, crtc->name, t_expected, t_actual, bus_format);
> +	STATE_CHECK_U32_X(ret, p, crtc->name, t_expected, t_actual, bus_flags);
> +
> +	return ret;
> +}

[Severity: Medium]
Can tidss_crtc_compare_state() report false mismatches for bus_format and
bus_flags when an active CRTC is disabled?

When a CRTC is disabled in a blocking commit, tidss_crtc_duplicate_state()
copies the previous active state's non-zero bus_format and bus_flags into the
new state:

drivers/gpu/drm/tidss/tidss_crtc.c:tidss_crtc_duplicate_state() {
    ...
	state->bus_format = current_state->bus_format;
	state->bus_flags = current_state->bus_flags;
    ...
}

During post-commit readout in tidss_crtc_readout_state(),
!dispc_vp_is_enabled() is true and jumps to out before reading bus_flags or
bus_format, leaving them 0 in t_actual.

While drm_atomic_helper_crtc_compare_state() returns early when both
expected->active and actual->active are false:

drivers/gpu/drm/drm_atomic_state_readout_helper.c:drm_atomic_helper_crtc_compare_state() {
    ...
	/* Both inactive, mode and properties are meaningless. */
	if (!expected->active && !actual->active)
		return ret;
    ...
}

tidss_crtc_compare_state() still checks bus_format and bus_flags
unconditionally. Should those checks be skipped when the CRTC is inactive?

> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index 3b97c08c4f714..b0b69b8531111 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
[ ... ]
> @@ -133,6 +136,7 @@ static const struct drm_mode_config_funcs mode_config_funcs = {
>  	.fb_create = drm_gem_fb_create,
>  	.atomic_check = tidss_atomic_check,
>  	.atomic_commit = drm_atomic_helper_commit,
> +	.atomic_sro_readout_state = drm_atomic_helper_sro_readout_state,
>  	.hw_reset = tidss_hw_reset,
>  };

[Severity: High]
How are mode_config_funcs.atomic_sro_readout_state (and tidss_crtc_install_state)
and the fallback mode_config_funcs.hw_reset reached during probe?

In drivers/gpu/drm/drm_mode_config.c, atomic_sro_readout_state and hw_reset
are only invoked by drm_mode_config_create_initial_state():

drivers/gpu/drm/drm_mode_config.c:drm_mode_config_create_initial_state() {
    ...
	if (dev->mode_config.funcs->atomic_sro_readout_state) {
		ret = dev->mode_config.funcs->atomic_sro_readout_state(dev);
		if (!ret)
			goto out;
	}

	/* Reset the hardware first ... */
	if (dev->mode_config.funcs->hw_reset)
		dev->mode_config.funcs->hw_reset(dev);
    ...
}

However, tidss_probe() in drivers/gpu/drm/tidss/tidss_drv.c still calls
drm_mode_config_reset(ddev):

drivers/gpu/drm/tidss/tidss_drv.c:tidss_probe() {
    ...
	drm_kms_helper_poll_init(ddev);

	drm_mode_config_reset(ddev);

	ret = drm_dev_register(ddev, 0);
    ...
}

Because drm_mode_config_reset() only allocates blank software states without
calling atomic_sro_readout_state or hw_reset, isn't initial state readout
(along with the hardware reset moved into tidss_hw_reset()) never executed at
probe time?

Furthermore, if tidss_probe() is updated to call
drm_mode_config_create_initial_state(ddev) after pm_runtime_enable(dev), won't
dispc_reset_hw() fail when readout falls back to hw_reset because
pm_runtime_set_active(dev) returns -EAGAIN when runtime PM is already enabled?

> diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
> index b0d5d6959b885..ebdb63f8e8f17 100644
> --- a/drivers/gpu/drm/tidss/tidss_plane.c
> +++ b/drivers/gpu/drm/tidss/tidss_plane.c
[ ... ]
> @@ -176,11 +179,186 @@ static const struct drm_plane_helper_funcs tidss_primary_plane_helper_funcs = {
>  	.get_scanout_buffer = drm_fb_dma_get_scanout_buffer,
>  };
>  
> +static const struct drm_framebuffer_funcs tidss_plane_readout_fb_funcs = {
> +	.destroy	= drm_gem_fb_destroy,
> +};
> +
> +static struct drm_framebuffer *tidss_plane_readout_fb(struct drm_plane *plane)
> +{
> +	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);
> +	const struct drm_format_info *info;
> +	struct drm_framebuffer *fb;
> +	int ret;
> +
> +	fb = kzalloc_obj(*fb);
> +	if (!fb)
> +		return ERR_PTR(-ENOMEM);
> +
> +	fb->dev = plane->dev;
> +
> +	info = dispc_plane_get_current_format(dispc, tplane->hw_plane_id);
> +	if (IS_ERR(info)) {
> +		ret = PTR_ERR(info);
> +		goto err_free_fb;
> +	}
> +
> +	// TODO: Figure out YUV and multiplanar formats, along with
> +	// the color encoding and range
> +	if (info->is_yuv) {
> +		ret = -EINVAL;
> +		goto err_free_fb;
> +	}
> +
> +	fb->format = info;
> +
> +	dispc_plane_get_picture_size(dispc, tplane->hw_plane_id, &fb->width,
> +				     &fb->height);
> +
> +	/*
> +	 * A pixel increment of 1 means that the DMA engine fetches every
> +	 * pixel of the source buffer. Any other value means that
> +	 * horizontal decimation is enabled, and the buffer geometry
> +	 * cannot be reconstructed from the registers.
> +	 */
> +	if (dispc_plane_get_pixel_inc(dispc, tplane->hw_plane_id) != 1) {
> +		ret = -EINVAL;
> +		goto err_free_fb;
> +	}
> +
> +	/*
> +	 * The row increment register holds the extra bytes between the
> +	 * end of a row of the picture and the beginning of the next one,
> +	 * which includes any padding the firmware might have programmed.
> +	 */
> +	fb->pitches[0] = fb->width * (drm_format_info_bpp(info, 0) / 8) +
> +			 dispc_plane_get_row_inc(dispc, tplane->hw_plane_id) - 1;
> +
> +	/*
> +	 * TODO: The DMA base address has the offsets baked in, and there
> +	 * is no backing buffer to recover them from.
> +	 */
> +	fb->offsets[0] = 0;
> +
> +	ret = drm_framebuffer_init(plane->dev, fb, &tidss_plane_readout_fb_funcs);
> +	if (ret) {
> +		kfree(fb);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return fb;
> +
> +err_free_fb:
> +	kfree(fb);
> +	return ERR_PTR(ret);
> +}

[Severity: High]
What happens on a subsequent atomic commit if a plane still references the
framebuffer created by tidss_plane_readout_fb() (for example, when updating
plane properties or when tidss_atomic_check() adds affected planes on the
CRTC)?

Because tidss_plane_readout_fb() allocates fb with fb->obj[0] == NULL and
neither tidss_plane_helper_funcs nor tidss_primary_plane_helper_funcs sets a
custom .prepare_fb callback, drm_atomic_helper_prepare_planes() defaults to
drm_gem_plane_helper_prepare_fb() on this DRIVER_GEM driver. Won't
drm_gem_fb_get_obj(state->fb, 0) trigger drm_WARN_ON_ONCE(dev, !fb->obj[plane])
and fail the commit with -EINVAL?

In addition, if dispc_plane_setup() is called while state->fb is a GEM-less
readout framebuffer:

drivers/gpu/drm/tidss/tidss_dispc.c:dispc_plane_setup() {
    ...
	dma_addr_t dma_addr = drm_fb_dma_get_gem_addr(state->fb, state, 0);
    ...
	dispc_vid_write(dispc, hw_plane, DISPC_VID_BA_0, dma_addr & 0xffffffff);
	dispc_vid_write(dispc, hw_plane, DISPC_VID_BA_EXT_0, (u64)dma_addr >> 32);
    ...
}

drm_fb_dma_get_gem_addr() returns 0 when fb->obj[0] is NULL, which would
overwrite DISPC_VID_BA_0 and DISPC_VID_BA_EXT_0 with 0x0.

> +
> +static struct drm_crtc *tidss_plane_readout_crtc(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +
> +	/* TODO: Add support for multiple CRTCs */
> +	if (dev->num_crtcs != 1)
> +		return ERR_PTR(-EINVAL);
> +
> +	return list_first_entry(&dev->mode_config.crtc_list, struct drm_crtc, head);
> +}
> +
> +static int tidss_plane_atomic_readout_state(struct drm_plane *plane,
> +					    struct drm_atomic_sro_state *state,
> +					    struct drm_plane_state *plane_state)
> +{
[ ... ]
> +	dispc_plane_get_size(dispc, tplane->hw_plane_id, &plane_state->crtc_w,
> +			     &plane_state->crtc_h);
> +
> +	// TODO: Handle crtc_x/crtc_x/src_x/src_y
> +	// crtc_x/crtc_y are handled by DISPC_OVR_ATTRIBUTES / OVR1_DSS_ATTRIBUTES
> +
> +	// TODO: Handle zpos, see DISPC_OVR_ATTRIBUTES / OVR1_DSS_ATTRIBUTES
> +
> +	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;
> +	plane_state->dst.x1 = 0;
> +	plane_state->dst.x2 = plane_state->crtc_w;
> +	plane_state->dst.y1 = 0;
> +	plane_state->dst.y2 = plane_state->crtc_h;

[Severity: Medium]
Won't the unimplemented cases in tidss_plane_readout_fb(),
tidss_plane_readout_crtc(), and tidss_plane_atomic_readout_state() trigger
kernel warnings and state mismatch errors during normal runtime commits?

When all bridges and connectors in the pipeline implement readout callbacks,
drm_atomic_sro_device_can_readout() returns true and every blocking atomic
commit runs drm_atomic_sro_readout_and_compare() in commit_tail().

If userspace commits a YUV plane (info->is_yuv), enables horizontal
decimation (dispc_plane_get_pixel_inc() != 1), or runs on a multi-CRTC SoC
(dev->num_crtcs != 1), tidss_plane_atomic_readout_state() returns -EINVAL,
which triggers drm_WARN_ON(dev, ret) in
drm_atomic_helper_sro_readout_plane_state() and WARN_ON(IS_ERR(readout_state))
in drm_atomic_sro_readout_and_compare().

Similarly, if a blocking commit positions a plane at non-zero crtc_x/crtc_y or
src_x/src_y, or uses a non-zero zpos/normalized_zpos, leaving those fields at
0 in tidss_plane_atomic_readout_state() will cause
drm_atomic_helper_plane_compare_state() to log state mismatch errors on every
such commit.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-drm-state-readout-v7-0-25e5163d75c3@kernel.org?part=32