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

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

On Tue, Sep 09, 2025 at 01:27:56PM +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.
> 
> The ingenic driver has a wrapper around that helper with
> ingenic_drm_get_new_priv_state(), so let's use that instead.
> 
> 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>
> 
> ---
> To: Paul Cercueil <paul@crapouillou.net>
> Cc: linux-mips@vger.kernel.org
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 05faed933e5619c796f2a4fa1906e0eaa029ac68..a1b641d63fc500dc169d0b0e22f93168c343a242 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -245,11 +245,11 @@ 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);
> +	priv_state = ingenic_drm_get_new_priv_state(priv, state);
>  	if (WARN_ON(IS_ERR(priv_state)))

get_new_state() will never return an error pointer. It's either
a valid pointer or NULL.

To me it looks like this could potentially be NULL here as the
get_pvi_state() call is done from the plane .atomic_check()
whereas this gets called for the crtc. So if the plane is
disabled there might not be any private state included in the
commit.

Not sure how this driver/hardware is supposed to work so not
sure what the proper fix for that is...

>  		return;
>  
>  	/* Set addresses of our DMA descriptor chains */
>  	next_id = priv_state->use_palette ? HWDESC_PALETTE : 0;
> 
> -- 
> 2.50.1

-- 
Ville Syrjälä
Intel