Re: [PATCH v3 2/7] drm/tegra: rgb: Parameterize configuration based on bus flags

[sashiko-bot@kernel.org]

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

commit 62888cceae72e28c0425a5560b48d0bf56dccc9e
Author: Svyatoslav Ryhel <clamor95@gmail.com>

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

This commit parameterizes the Tegra RGB encoder configuration based on bus
flags from the bridge state, including pixel clock polarity, display enable
polarity, and data alignment.

> 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()?

Looking at include/drm/drm_bridge.h, drm_bridge_get_current_state() asserts
that bridge->base.lock is held:

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);
}

When userspace submits a non-blocking atomic modeset commit
(DRM_MODE_ATOMIC_NONBLOCK | DRM_MODE_ATOMIC_ALLOW_MODESET),
drm_atomic_helper_commit() swaps the state and schedules commit_work on
system_dfl_wq, after which drm_mode_atomic_ioctl() drops all modeset locks.

The worker thread then executes without holding bridge->base.lock:

commit_work() ->
  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()

Won't calling drm_bridge_get_current_state(output->bridge) here trigger a
lockdep warning when CONFIG_PROVE_LOCKING is enabled and read
bridge->base.state without synchronization against concurrent atomic state
swaps?

Should this encoder implement .atomic_enable and retrieve the bridge state via
drm_atomic_get_new_bridge_state(state, output->bridge), or cache bus_flags
during .atomic_check instead?

[ ... ]

>
>  	tegra_dc_commit(rgb->dc);

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