Move the conversion from drm_format to vs_format to atomic_check, which
is before the point of no return and can properly bail out.

Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
Changes in v2:
- Add non-NULL check for drm_plane_state->fb.

 drivers/gpu/drm/verisilicon/vs_plane.h        |  2 ++
 .../gpu/drm/verisilicon/vs_primary_plane.c    | 21 ++++++++++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/verisilicon/vs_plane.h b/drivers/gpu/drm/verisilicon/vs_plane.h
index 12848a72af576..df4b248b52954 100644
--- a/drivers/gpu/drm/verisilicon/vs_plane.h
+++ b/drivers/gpu/drm/verisilicon/vs_plane.h
@@ -65,6 +65,8 @@ struct vs_format {
 
 struct vs_plane_state {
 	struct drm_plane_state base;
+
+	struct vs_format format;
 };
 
 static inline struct vs_plane_state *state_to_vs_plane_state(struct drm_plane_state *state)
diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
index bad0bc5e3242d..ca41c8053ae12 100644
--- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
+++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
@@ -25,12 +25,20 @@ static int vs_primary_plane_atomic_check(struct drm_plane *plane,
 {
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
+	struct vs_plane_state *new_vs_plane_state = state_to_vs_plane_state(new_plane_state);
 	struct drm_crtc *crtc = new_plane_state->crtc;
+	struct drm_framebuffer *fb = new_plane_state->fb;
 	struct drm_crtc_state *crtc_state;
+	int ret;
 
-	if (!crtc)
+	if (!crtc || !fb)
 		return 0;
 
+	ret = drm_format_to_vs_format(fb->format->format,
+				      &new_vs_plane_state->format);
+	if (ret)
+		return ret;
+
 	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	if (WARN_ON(!crtc_state))
 		return -EINVAL;
@@ -88,11 +96,11 @@ static void vs_primary_plane_atomic_update(struct drm_plane *plane,
 {
 	struct drm_plane_state *state = drm_atomic_get_new_plane_state(atomic_state,
 								       plane);
+	struct vs_plane_state *vs_state = state_to_vs_plane_state(state);
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_crtc *crtc = state->crtc;
 	struct vs_dc *dc;
 	struct vs_crtc *vcrtc;
-	struct vs_format fmt;
 	unsigned int output;
 	dma_addr_t dma_addr;
 
@@ -105,16 +113,15 @@ static void vs_primary_plane_atomic_update(struct drm_plane *plane,
 	output = vcrtc->id;
 	dc = vcrtc->dc;
 
-	drm_format_to_vs_format(state->fb->format->format, &fmt);
-
 	regmap_update_bits(dc->regs, VSDC_FB_CONFIG(output),
 			   VSDC_FB_CONFIG_FMT_MASK,
-			   VSDC_FB_CONFIG_FMT(fmt.color));
+			   VSDC_FB_CONFIG_FMT(vs_state->format.color));
 	regmap_update_bits(dc->regs, VSDC_FB_CONFIG(output),
 			   VSDC_FB_CONFIG_SWIZZLE_MASK,
-			   VSDC_FB_CONFIG_SWIZZLE(fmt.swizzle));
+			   VSDC_FB_CONFIG_SWIZZLE(vs_state->format.swizzle));
 	regmap_assign_bits(dc->regs, VSDC_FB_CONFIG(output),
-			   VSDC_FB_CONFIG_UV_SWIZZLE_EN, fmt.uv_swizzle);
+			   VSDC_FB_CONFIG_UV_SWIZZLE_EN,
+			   vs_state->format.uv_swizzle);
 
 	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
 
-- 
2.52.0

Hi

Am 05.03.26 um 09:09 schrieb Icenowy Zheng:
> Move the conversion from drm_format to vs_format to atomic_check, which
> is before the point of no return and can properly bail out.
>
> Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> ---
> Changes in v2:
> - Add non-NULL check for drm_plane_state->fb.
>
>   drivers/gpu/drm/verisilicon/vs_plane.h        |  2 ++
>   .../gpu/drm/verisilicon/vs_primary_plane.c    | 21 ++++++++++++-------
>   2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/verisilicon/vs_plane.h b/drivers/gpu/drm/verisilicon/vs_plane.h
> index 12848a72af576..df4b248b52954 100644
> --- a/drivers/gpu/drm/verisilicon/vs_plane.h
> +++ b/drivers/gpu/drm/verisilicon/vs_plane.h
> @@ -65,6 +65,8 @@ struct vs_format {
>   
>   struct vs_plane_state {
>   	struct drm_plane_state base;
> +
> +	struct vs_format format;

In case you implement my comment about kzalloc vs kmemdup, don't forget 
to assign it in the plane-duplicate helper.

>   };
>   
>   static inline struct vs_plane_state *state_to_vs_plane_state(struct drm_plane_state *state)
> diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> index bad0bc5e3242d..ca41c8053ae12 100644
> --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c
> @@ -25,12 +25,20 @@ static int vs_primary_plane_atomic_check(struct drm_plane *plane,
>   {
>   	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
>   										 plane);
> +	struct vs_plane_state *new_vs_plane_state = state_to_vs_plane_state(new_plane_state);
>   	struct drm_crtc *crtc = new_plane_state->crtc;
> +	struct drm_framebuffer *fb = new_plane_state->fb;
>   	struct drm_crtc_state *crtc_state;
> +	int ret;
>   
> -	if (!crtc)
> +	if (!crtc || !fb)
>   		return 0;

I see that even the original code was not correct.

You have to call drm_atomic_helper_check_plane_state() in any case. If 
there's no CRTC, use NULL for the CRTC state. See [1] for an example.

>   
> +	ret = drm_format_to_vs_format(fb->format->format,
> +				      &new_vs_plane_state->format);
> +	if (ret)
> +		return ret;
> +

That should also only happen after 
drm_atomic_helper_check_plane_state().  You can again take the code 
around [1] for an example.

[1] 
https://elixir.bootlin.com/linux/v7.0-rc1/source/drivers/gpu/drm/ast/ast_mode.c#L504


I think it makes sense to warn once if the conversion fails. After all, 
the driver should only export DRM formats that it can program to hardware.

   if (drm_WARN_ON_ONCE(plane->dev, ret))
     return ret;


>   	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>   	if (WARN_ON(!crtc_state))
>   		return -EINVAL;
> @@ -88,11 +96,11 @@ static void vs_primary_plane_atomic_update(struct drm_plane *plane,
>   {
>   	struct drm_plane_state *state = drm_atomic_get_new_plane_state(atomic_state,
>   								       plane);
> +	struct vs_plane_state *vs_state = state_to_vs_plane_state(state);
>   	struct drm_framebuffer *fb = state->fb;
>   	struct drm_crtc *crtc = state->crtc;
>   	struct vs_dc *dc;
>   	struct vs_crtc *vcrtc;
> -	struct vs_format fmt;
>   	unsigned int output;
>   	dma_addr_t dma_addr;
>   
> @@ -105,16 +113,15 @@ static void vs_primary_plane_atomic_update(struct drm_plane *plane,
>   	output = vcrtc->id;
>   	dc = vcrtc->dc;
>   
> -	drm_format_to_vs_format(state->fb->format->format, &fmt);

Removing another possible failure from the atomic commit is really nice.

Best regards
Thomas

> -
>   	regmap_update_bits(dc->regs, VSDC_FB_CONFIG(output),
>   			   VSDC_FB_CONFIG_FMT_MASK,
> -			   VSDC_FB_CONFIG_FMT(fmt.color));
> +			   VSDC_FB_CONFIG_FMT(vs_state->format.color));
>   	regmap_update_bits(dc->regs, VSDC_FB_CONFIG(output),
>   			   VSDC_FB_CONFIG_SWIZZLE_MASK,
> -			   VSDC_FB_CONFIG_SWIZZLE(fmt.swizzle));
> +			   VSDC_FB_CONFIG_SWIZZLE(vs_state->format.swizzle));
>   	regmap_assign_bits(dc->regs, VSDC_FB_CONFIG(output),
> -			   VSDC_FB_CONFIG_UV_SWIZZLE_EN, fmt.uv_swizzle);
> +			   VSDC_FB_CONFIG_UV_SWIZZLE_EN,
> +			   vs_state->format.uv_swizzle);
>   
>   	dma_addr = vs_fb_get_dma_addr(fb, &state->src);
>   

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)