Re: [PATCH RFC 9/9] drm/panel: novatek-nt35950: use the atomic state to select the mode

[Neil Armstrong <neil.armstrong@linaro.org>]

On 10/8/26 15:14, Dmitry Baryshkov wrote:
> The compression, scaler and display output settings of the NT35950
> depend on the mode being set. As the panel has no access to the
> configuration being committed, the driver saves the connector pointer in
> get_modes() and then dereferences connector->state->crtc->state from the
> prepare() callback. This is done without holding the modeset locks and
> it reads the current state of the objects rather than the state being
> committed. Moreover, if none of the panel modes match, the driver
> silently programs the panel for the first mode instead of rejecting the
> configuration.
> 
> Subclass the panel state to store the parameters of the selected mode.
> Look the mode up in atomic_check(), failing the commit if the panel
> doesn't support it, and use the stored parameters in atomic_prepare().
> Switch unprepare() to its atomic variant too, so that the driver uses a
> single set of callbacks.
> 
> Assisted-by: LLM
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>   drivers/gpu/drm/panel/panel-novatek-nt35950.c | 159 +++++++++++++++++++-------
>   1 file changed, 117 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> index 94aa6489d99f..f5161f11b1f5 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> @@ -11,12 +11,15 @@
>   #include <linux/of.h>
>   #include <linux/of_graph.h>
>   #include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
>   
> +#include <drm/drm_atomic.h>
>   #include <drm/drm_connector.h>
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_mipi_dsi.h>
>   #include <drm/drm_modes.h>
>   #include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
>   
>   #define MCS_CMD_MAUCCTR			0xf0 /* Manufacturer command enable */
>   #define MCS_PARAM_SCALER_FUNCTION	0x58 /* Scale-up function */
> @@ -51,16 +54,19 @@
>   
>   struct nt35950 {
>   	struct drm_panel panel;
> -	struct drm_connector *connector;
>   	struct mipi_dsi_device *dsi[2];
>   	struct regulator_bulk_data vregs[NT35950_VREG_MAX];
>   	struct gpio_desc *reset_gpio;
>   	const struct nt35950_panel_desc *desc;
>   
> -	int cur_mode;
>   	u8 last_page;
>   };
>   
> +struct nt35950_state {
> +	struct drm_panel_state base;
> +	const struct nt35950_panel_mode *mode_data;
> +};
> +
>   struct nt35950_panel_mode {
>   	const struct drm_display_mode mode;
>   
> @@ -88,6 +94,9 @@ static inline struct nt35950 *to_nt35950(struct drm_panel *panel)
>   	return container_of(panel, struct nt35950, panel);
>   }
>   
> +#define to_nt35950_state(panel_state) \
> +	container_of_const(panel_state, struct nt35950_state, base)
> +
>   static void nt35950_reset(struct nt35950 *nt)
>   {
>   	gpiod_set_value_cansleep(nt->reset_gpio, 1);
> @@ -212,61 +221,37 @@ static void nt35950_inject_black_image(struct mipi_dsi_multi_context *dsi_ctx)
>   
>   /*
>    * nt35950_set_dispout - Set Display Output register parameters
> - * @nt:    Main driver structure
>    * @dsi_ctx: context for mipi_dsi functions
> + * @mode_data: Parameters of the mode being set
>    */
>   static void nt35950_set_dispout(struct mipi_dsi_multi_context *dsi_ctx,
> -				struct nt35950 *nt)
> +				const struct nt35950_panel_mode *mode_data)
>   {
>   	u8 cmd_dispout[] = { MCS_PARAM_DISP_OUTPUT_CTRL, 0x00 };
> -	const struct nt35950_panel_mode *mode_data = nt->desc->mode_data;
>   
> -	if (mode_data[nt->cur_mode].is_video_mode)
> +	if (mode_data->is_video_mode)
>   		cmd_dispout[1] |= MCS_DISP_OUT_VIDEO_MODE;
> -	if (mode_data[nt->cur_mode].enable_sram)
> +	if (mode_data->enable_sram)
>   		cmd_dispout[1] |= MCS_DISP_OUT_SRAM_EN;
>   
>   	mipi_dsi_dcs_write_buffer_multi(dsi_ctx, cmd_dispout,
>   					ARRAY_SIZE(cmd_dispout));
>   }
>   
> -static int nt35950_get_current_mode(struct nt35950 *nt)
> -{
> -	struct drm_connector *connector = nt->connector;
> -	struct drm_crtc_state *crtc_state;
> -	int i;
> -
> -	/* Return the default (first) mode if no info available yet */
> -	if (!connector->state || !connector->state->crtc)
> -		return 0;
> -
> -	crtc_state = connector->state->crtc->state;
> -
> -	for (i = 0; i < nt->desc->num_modes; i++) {
> -		if (drm_mode_match(&crtc_state->mode,
> -				   &nt->desc->mode_data[i].mode,
> -				   DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_CLOCK))
> -			return i;
> -	}
> -
> -	return 0;
> -}
> -
> -static int nt35950_on(struct nt35950 *nt)
> +static int nt35950_on(struct nt35950 *nt,
> +		      const struct nt35950_panel_mode *mode_data)
>   {
> -	const struct nt35950_panel_mode *mode_data = nt->desc->mode_data;
>   	struct mipi_dsi_device *dsi = nt->dsi[0];
>   	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>   
> -	nt->cur_mode = nt35950_get_current_mode(nt);
>   	nt->dsi[0]->mode_flags |= MIPI_DSI_MODE_LPM;
>   	nt->dsi[1]->mode_flags |= MIPI_DSI_MODE_LPM;
>   
>   	nt35950_set_cmd2_page(&dsi_ctx, nt, 0);
> -	nt35950_set_data_compression(&dsi_ctx, nt, mode_data[nt->cur_mode].compression);
> -	nt35950_set_scale_mode(&dsi_ctx, mode_data[nt->cur_mode].scaler_mode);
> -	nt35950_set_scaler(&dsi_ctx, mode_data[nt->cur_mode].scaler_on);
> -	nt35950_set_dispout(&dsi_ctx, nt);
> +	nt35950_set_data_compression(&dsi_ctx, nt, mode_data->compression);
> +	nt35950_set_scale_mode(&dsi_ctx, mode_data->scaler_mode);
> +	nt35950_set_scaler(&dsi_ctx, mode_data->scaler_on);
> +	nt35950_set_dispout(&dsi_ctx, mode_data);
>   
>   	mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>   	mipi_dsi_dcs_set_tear_scanline_multi(&dsi_ctx, 0);
> @@ -354,11 +339,43 @@ static int nt35950_sharp_init_vregs(struct nt35950 *nt, struct device *dev)
>   	return 0;
>   }
>   
> -static int nt35950_prepare(struct drm_panel *panel)
> +static int nt35950_atomic_check(struct drm_panel *panel,
> +				struct drm_atomic_commit *commit)
> +{
> +	struct nt35950 *nt = to_nt35950(panel);
> +	struct drm_panel_state *panel_state;
> +	struct drm_crtc_state *crtc_state;
> +	int i;
> +
> +	panel_state = drm_atomic_get_new_panel_state(commit, panel);
> +	crtc_state = drm_atomic_get_new_crtc_state_for_panel(commit, panel);
> +	if (!panel_state || !crtc_state)
> +		return -EINVAL;
> +
> +	for (i = 0; i < nt->desc->num_modes; i++) {
> +		if (drm_mode_match(&crtc_state->mode,
> +				   &nt->desc->mode_data[i].mode,
> +				   DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_CLOCK)) {
> +			to_nt35950_state(panel_state)->mode_data =
> +				&nt->desc->mode_data[i];
> +			return 0;
> +		}
> +	}


This looks like a boilerplate which could be moved to the drm_panel if atomic_check is
not defined.

Neil

> +
> +	return -EINVAL;
> +}
> +
> +static int nt35950_atomic_prepare(struct drm_panel *panel,
> +				  struct drm_atomic_commit *commit)
>   {
>   	struct nt35950 *nt = to_nt35950(panel);
> +	struct drm_panel_state *panel_state;
>   	int ret;
>   
> +	panel_state = drm_atomic_get_new_panel_state(commit, panel);
> +	if (!panel_state || !to_nt35950_state(panel_state)->mode_data)
> +		return -EINVAL;
> +
>   	ret = regulator_enable(nt->vregs[0].consumer);
>   	if (ret)
>   		return ret;
> @@ -380,7 +397,7 @@ static int nt35950_prepare(struct drm_panel *panel)
>   
>   	nt35950_reset(nt);
>   
> -	ret = nt35950_on(nt);
> +	ret = nt35950_on(nt, to_nt35950_state(panel_state)->mode_data);
>   
>   end:
>   	if (ret < 0) {
> @@ -391,7 +408,8 @@ static int nt35950_prepare(struct drm_panel *panel)
>   	return 0;
>   }
>   
> -static int nt35950_unprepare(struct drm_panel *panel)
> +static int nt35950_atomic_unprepare(struct drm_panel *panel,
> +				    struct drm_atomic_commit *commit)
>   {
>   	struct nt35950 *nt = to_nt35950(panel);
>   
> @@ -429,15 +447,72 @@ static int nt35950_get_modes(struct drm_panel *panel,
>   	connector->display_info.bpc = 8;
>   	connector->display_info.height_mm = nt->desc->mode_data[0].mode.height_mm;
>   	connector->display_info.width_mm = nt->desc->mode_data[0].mode.width_mm;
> -	nt->connector = connector;
>   
>   	return nt->desc->num_modes;
>   }
>   
> +static struct drm_panel_state *
> +nt35950_atomic_create_state(struct drm_panel *panel)
> +{
> +	struct nt35950_state *state;
> +
> +	state = kzalloc_obj(*state);
> +	if (!state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	__drm_atomic_helper_panel_state_init(&state->base, panel);
> +
> +	return &state->base;
> +}
> +
> +static struct drm_panel_state *
> +nt35950_atomic_duplicate_state(struct drm_panel *panel)
> +{
> +	struct drm_panel_state *old_state = drm_panel_get_current_state(panel);
> +	struct nt35950_state *state;
> +
> +	if (!old_state)
> +		return NULL;
> +
> +	state = kzalloc_obj(*state);
> +	if (!state)
> +		return NULL;
> +
> +	__drm_atomic_helper_panel_duplicate_state(panel, &state->base);
> +	state->mode_data = to_nt35950_state(old_state)->mode_data;
> +
> +	return &state->base;
> +}
> +
> +static void nt35950_atomic_destroy_state(struct drm_panel *panel,
> +					 struct drm_panel_state *state)
> +{
> +	kfree(to_nt35950_state(state));
> +}
> +
> +static void nt35950_atomic_print_state(struct drm_printer *p,
> +				       const struct drm_panel_state *state)
> +{
> +	const struct nt35950_panel_mode *mode_data =
> +		to_nt35950_state(state)->mode_data;
> +
> +	if (!mode_data)
> +		return;
> +
> +	drm_printf_indent(p, 1, "compression: %02x\n", mode_data->compression);
> +	drm_printf_indent(p, 1, "scaler: %u, mode %02x\n",
> +			  mode_data->scaler_on, mode_data->scaler_mode);
> +}
> +
>   static const struct drm_panel_funcs nt35950_panel_funcs = {
> -	.prepare = nt35950_prepare,
> -	.unprepare = nt35950_unprepare,
> +	.atomic_check = nt35950_atomic_check,
> +	.atomic_prepare = nt35950_atomic_prepare,
> +	.atomic_unprepare = nt35950_atomic_unprepare,
>   	.get_modes = nt35950_get_modes,
> +	.atomic_create_state = nt35950_atomic_create_state,
> +	.atomic_duplicate_state = nt35950_atomic_duplicate_state,
> +	.atomic_destroy_state = nt35950_atomic_destroy_state,
> +	.atomic_print_state = nt35950_atomic_print_state,
>   };
>   
>   static int nt35950_probe(struct mipi_dsi_device *dsi)
>