Re: [PATCH v8 39/39] fixup! drm/tidss: Implement readout support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/tidss: oldi: bus flags mismatch in
  tidss_oldi_bridge_readout_state()
- [Low] drm/tidss: oldi: unsquashed fixup commit and unused header
  includes
--

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

fixup! drm/tidss: Implement readout support

This commit adds atomic state and connector hardware readout callbacks for
the TI DSS OLDI bridge.

> diff --git a/drivers/gpu/drm/tidss/tidss_oldi.c b/drivers/gpu/drm/tidss/tidss_oldi.c
> index 82b059f852449..7dbfbb380205e 100644
> --- a/drivers/gpu/drm/tidss/tidss_oldi.c
> +++ b/drivers/gpu/drm/tidss/tidss_oldi.c
> @@ -5,6 +5,7 @@
>   * Aradhya Bhatia <a-bhatia1@ti.com>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/of.h>
>  #include <linux/of_graph.h>
> @@ -13,8 +14,12 @@
>  #include <linux/regmap.h>
>  
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_atomic_state_readout.h>
> +#include <drm/drm_atomic_state_readout_helper.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_connector.h>
>  #include <drm/drm_of.h>
> +#include <drm/drm_panel.h>

[Severity: Low]
This isn't a bug, but <linux/bitfield.h> and <drm/drm_panel.h> are not used
in tidss_oldi.c.  Also, this commit is still formatted as an unsquashed
"fixup!" patch with a placeholder commit message instead of being squashed
into commit "drm/tidss: Implement readout support" or given a full
changelog.

[ ... ]

> @@ -351,6 +356,135 @@ tidss_oldi_mode_valid(struct drm_bridge *bridge,
>  	return 0;
>  }

[ ... ]

> +static int
> +tidss_oldi_bridge_readout_state(struct drm_bridge *bridge,
> +				struct drm_atomic_sro_state *state,
> +				struct drm_bridge_state *bridge_state,
> +				struct drm_crtc_state *crtc_state,
> +				struct drm_connector_state *conn_state)
> +{

[ ... ]

> +	/*
> +	 * tidss_oldi_atomic_check() forces positive edge sampling on
> +	 * the input side, and the DE polarity is programmed from the
> +	 * negotiated flags: OLDI_DEPOL is set when the DE is active
> +	 * low.
> +	 */
> +	bridge_state->input_bus_cfg.flags =
> +		DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
> +		DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE |
> +		((cfg & OLDI_DEPOL) ? DRM_BUS_FLAG_DE_LOW :
> +				      DRM_BUS_FLAG_DE_HIGH);
> +
> +	bridge_state->output_bus_cfg.flags =
> +		(cfg & OLDI_DEPOL) ? DRM_BUS_FLAG_DE_LOW :
> +				     DRM_BUS_FLAG_DE_HIGH;
> +
> +	return 0;
> +}

[Severity: Medium]
Will input_bus_cfg.flags and output_bus_cfg.flags mismatch the committed
state during drm_atomic_helper_bridge_compare_state() when the downstream
panel or bridge sets other bus flags or omits DE polarity?

During atomic check, drm_atomic_bridge_propagate_bus_flags() copies the
downstream bridge's input_bus_cfg.flags (populated by panel_bridge from
connector->display_info.bus_flags) into both output_bus_cfg.flags and
input_bus_cfg.flags, and tidss_oldi_atomic_check() only adjusts the sampling
edge flags on input_bus_cfg.flags:

drivers/gpu/drm/tidss/tidss_oldi.c:tidss_oldi_atomic_check() {
	bridge_state->input_bus_cfg.flags &=
		~(DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
		  DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE);

	bridge_state->input_bus_cfg.flags |=
		DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
		DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE;
}

For LVDS panels driven by panel-lvds (which sets
DRM_BUS_FLAG_DATA_MSB_TO_LSB or DRM_BUS_FLAG_DATA_LSB_TO_MSB in
display_info.bus_flags) or panels that leave both DE polarity flags unset,
the committed bridge state retains those flags in input_bus_cfg.flags and
output_bus_cfg.flags, whereas tidss_oldi_bridge_readout_state() reconstructs
only DRM_BUS_FLAG_DE_LOW or DRM_BUS_FLAG_DE_HIGH from OLDI_DEPOL.  Every
blocking atomic modeset on those panels will therefore report a bridge state
readout mismatch.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-drm-state-readout-v8-0-cbe78624e1bc@kernel.org?part=39