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 <[email protected]> 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/[email protected]?part=2
