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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/tegra: rgb: NULL pointer deref in tegra_rgb_encoder_enable()
- [High] drm/tegra: rgb: pixel clock polarity regression for legacy panels
--

commit 54b0f3191ac9412b6aca8d1f4028b3159a42b6a2
Author: Svyatoslav Ryhel <clamor95@gmail.com>

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

This commit parameterizes the RGB display configuration based on bus flags
passed from the bridge or panel. It updates pixel clock polarity, display
enable polarity, and data alignment according to the input bus configuration.

> --- a/drivers/gpu/drm/tegra/rgb.c
> +++ b/drivers/gpu/drm/tegra/rgb.c
> @@ -104,14 +105,19 @@ 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);
> +	bus_flags = bridge_state->input_bus_cfg.flags;

[Severity: High]
Does this introduce a NULL pointer dereference for non-atomic bridges?

Looking at drm_bridge_get_current_state(), it can return NULL if the
attached bridge does not support atomic state:

static inline struct drm_bridge_state *
drm_bridge_get_current_state(struct drm_bridge *bridge)
{
	...
	if (!bridge->base.state)
		return NULL;

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

Since bridge_state is unconditionally dereferenced to read
input_bus_cfg.flags, will this crash the display pipeline during
tegra_rgb_encoder_enable() for any devices using non-atomic bridges?

[ ... ]
> -	/* configure H- and V-sync signal polarities */
> +	/* configure H- and V-sync and pixel clock signal polarities */
>  	value = tegra_dc_readl(rgb->dc, DC_COM_PIN_OUTPUT_POLARITY(1));
>  
>  	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> @@ -124,11 +130,31 @@ static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)
>  	else
>  		value &= ~LVS_OUTPUT_POLARITY_LOW;
>  
> +	if (bus_flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE)
> +		value |= LSC0_OUTPUT_POLARITY_LOW;
> +	else
> +		value &= ~LSC0_OUTPUT_POLARITY_LOW;

[Severity: High]
Can this cause a regression for legacy panels by inverting the pixel clock?

The previous code preserved the initial state of the
LSC0_OUTPUT_POLARITY_LOW bit (which is initialized just prior to this block
via tegra_dc_write_regs(..., rgb_enable, ...)).

By forcefully clearing LSC0_OUTPUT_POLARITY_LOW when the
DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE flag is absent, doesn't this break
existing hardware relying on older device trees that do not explicitly
specify this bus flag?

> +
>  	tegra_dc_writel(rgb->dc, value, DC_COM_PIN_OUTPUT_POLARITY(1));

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