Re: [PATCH v4 37/39] drm/ingenic: crtc: Switch to ingenic_drm_get_new_priv_state()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 17, 2025 at 04:46:18PM +0200, Maxime Ripard wrote:
> The ingenic CRTC atomic_enable() implementation will indirectly call
> drm_atomic_get_private_obj_state() through ingenic_drm_get_priv_state().
> 
> drm_atomic_get_private_obj_state() will either return the new state for
> the object in the global state if it exists, or will allocate a new one
> and add it to the global state.
> 
> atomic_enable() however isn't allowed to modify the global state. So
> what the implementation should use is the
> drm_atomic_get_new_private_obj_state() helper to get the new state for
> the CRTC, without performing an extra allocation.
> 
> We still need to make sure the private state will be part of the global
> state by the time atomic_enable runs, so we still need to call
> ingenic_drm_get_priv_state() in atomic_check. We can then call
> ingenic_drm_get_new_priv_state() in atomic_enable, which is a wrapper
> around drm_atomic_get_new_private_obj_state().
> 
> Reported-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Seems fine by me.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
> To: Paul Cercueil <paul@crapouillou.net>
> Cc: linux-mips@vger.kernel.org
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 05faed933e5619c796f2a4fa1906e0eaa029ac68..d3213fbf22be14b177fc1b7100c5b721d5f17924 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -245,12 +245,12 @@ static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
>  {
>  	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>  	struct ingenic_drm_private_state *priv_state;
>  	unsigned int next_id;
>  
> -	priv_state = ingenic_drm_get_priv_state(priv, state);
> -	if (WARN_ON(IS_ERR(priv_state)))
> +	priv_state = ingenic_drm_get_new_priv_state(priv, state);
> +	if (WARN_ON(!priv_state))
>  		return;
>  
>  	/* Set addresses of our DMA descriptor chains */
>  	next_id = priv_state->use_palette ? HWDESC_PALETTE : 0;
>  	regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, next_id));
> @@ -338,17 +338,23 @@ static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
>  {
>  	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
>  									  crtc);
>  	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>  	struct drm_plane_state *f1_state, *f0_state, *ipu_state = NULL;
> +	struct ingenic_drm_private_state *priv_state;
>  
>  	if (crtc_state->gamma_lut &&
>  	    drm_color_lut_size(crtc_state->gamma_lut) != ARRAY_SIZE(priv->dma_hwdescs->palette)) {
>  		dev_dbg(priv->dev, "Invalid palette size\n");
>  		return -EINVAL;
>  	}
>  
> +	/* We will need the state in atomic_enable, so let's make sure it's part of the state */
> +	priv_state = ingenic_drm_get_priv_state(priv, state);
> +	if (IS_ERR(priv_state))
> +		return PTR_ERR(priv_state);
> +
>  	if (drm_atomic_crtc_needs_modeset(crtc_state) && priv->soc_info->has_osd) {
>  		f1_state = drm_atomic_get_plane_state(crtc_state->state,
>  						      &priv->f1);
>  		if (IS_ERR(f1_state))
>  			return PTR_ERR(f1_state);
> 
> -- 
> 2.50.1

-- 
Ville Syrjälä
Intel