Re: [PATCH v4 6/7] dt-bindings: display: tegra: Document 8-bit CPU parallel interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/tegra: unlocked drm_bridge_get_current_state() call in tegra_rgb_encoder_enable()
--

commit 743a0ce150c28976e990ee80b4eaf7239b83fcc3
Author: Svyatoslav Ryhel <clamor95@gmail.com>

drm/tegra: rgb: Parameterize configuration based on bus flags

This commit updates tegra_rgb_encoder_enable() to read bus flags from the
bridge state and configure pixel clock polarity, display enable polarity,
and data alignment accordingly.

> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> index b6d09a8d8e5ea..da9e37fe36689 100644
> --- a/drivers/gpu/drm/tegra/rgb.c
> +++ b/drivers/gpu/drm/tegra/rgb.c

[ ... ]

> @@ -104,14 +105,24 @@ static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
>  	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
>  	struct tegra_output *output = encoder_to_output(encoder);
>  	struct tegra_rgb *rgb = to_rgb(output);
> -	u32 value;
> +	struct drm_bridge_state *bridge_state;
> +	u32 bus_flags, value;
> +
> +	/* Get but flags from the bridge state. */
> +	bridge_state = drm_bridge_get_current_state(output->bridge);
> +	if (bridge_state)
> +		bus_flags = bridge_state->input_bus_cfg.flags;
> +	else
> +		bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> +			    DRM_BUS_FLAG_DE_HIGH |
> +			    DRM_BUS_FLAG_DATA_MSB_TO_LSB;

[Severity: High]
Is it safe to call drm_bridge_get_current_state() from
tegra_rgb_encoder_enable() during a non-blocking atomic commit?

In include/drm/drm_bridge.h, drm_bridge_get_current_state() asserts that
bridge->base.lock is held before reading bridge->base.state:

static inline struct drm_bridge_state *
drm_bridge_get_current_state(struct drm_bridge *bridge)
{
	if (!bridge)
		return NULL;

	drm_modeset_lock_assert_held(&bridge->base.lock);

	if (!bridge->base.state)
		return NULL;

	return drm_priv_to_bridge_state(bridge->base.state);
}

During a non-blocking atomic modeset commit in
drivers/gpu/drm/drm_atomic_helper.c, drm_atomic_helper_commit() queues
state->commit_work to run commit_tail() asynchronously on system_dfl_wq,
and drm_mode_atomic_ioctl() drops all modeset locks before the worker runs:

drm_mode_atomic_ioctl() ->
  drm_atomic_nonblocking_commit() ->
    drm_atomic_helper_commit()
      queue_work(system_dfl_wq, &state->commit_work);
  drm_modeset_drop_locks()

When the worker executes commit_tail() -> tegra_atomic_commit_tail() ->
drm_atomic_helper_commit_tail_rpm() ->
drm_atomic_helper_commit_modeset_enables() ->
drm_atomic_helper_commit_encoder_bridge_enable() ->
tegra_rgb_encoder_enable(), no drm_modeset_lock is held. Calling
drm_bridge_get_current_state(output->bridge) here will trigger the
drm_modeset_lock_assert_held(&bridge->base.lock) lockdep warning and perform
an unlocked read of bridge->base.state that can race with concurrent atomic
commits swapping or freeing the state.

In addition, if an attached bridge does not implement
.atomic_duplicate_state, drm_atomic_add_encoder_bridges() skips acquiring
bridge->base.lock even during blocking modeset commits, while
drm_bridge_attach() unconditionally initializes bridge->base.lock via
drm_atomic_private_obj_init().

Would it be better to switch tegra_rgb_encoder_helper_funcs to use the
.atomic_enable callback and look up the state from the drm_atomic_state via
drm_atomic_get_new_bridge_state(state, output->bridge)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022132259.240717-1-clamor95@gmail.com?part=6