[PATCH v3 00/39] drm/atomic: Get rid of existing states (not really)

[PATCH v3 00/39] drm/atomic: Get rid of existing states (not really)

[Maxime Ripard]

Hi,

Here's a series to get rid of the drm_atomic_helper_get_existing_*_state
accessors.

The initial intent was to remove the __drm_*_state->state pointer to
only rely on old and new states, but we still need it now to know which
of the two we need to free: if a state has not been committed (either
dropped or checked only), then we need to free the new one, if it has
been committed we need to free the old state. 

Thus, the state pointer is kept (and documented) only to point to the
state we should free eventually.

All users have been converted to the relevant old or new state
accessors.  

This was tested on tidss.

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
Changes in v3:
- Added an armada rework patch
- Added an ingenic fix
- Collected tags
- Rebased on latest drm-misc-next tag
- Link to v2: https://lore.kernel.org/r/20250902-drm-no-more-existing-state-v2-0-de98fc5f6d66@kernel.org

Changes in v2:
- Dropped the first and second patches
- Reworked the recipient list to be nicer with SMTPs
- Link to v1: https://lore.kernel.org/r/20250825-drm-no-more-existing-state-v1-0-f08ccd9f85c9@kernel.org

---
Maxime Ripard (39):
      drm/atomic: Convert drm_atomic_get_connector_state() to use new connector state
      drm/atomic: Remove unused drm_atomic_get_existing_connector_state()
      drm/atomic: Document __drm_connectors_state state pointer
      drm/atomic: Convert __drm_atomic_get_current_plane_state() to modern accessor
      drm/atomic: Convert drm_atomic_get_plane_state() to use new plane state
      drm/vkms: Convert vkms_crtc_atomic_check() to use new plane state
      drm/tilcdc: crtc: Use drm_atomic_helper_check_crtc_primary_plane()
      drm/atomic: Remove unused drm_atomic_get_existing_plane_state()
      drm/atomic: Document __drm_planes_state state pointer
      drm/atomic: Convert drm_atomic_get_crtc_state() to use new connector state
      drm/ingenic: ipu: Switch to drm_atomic_get_new_crtc_state()
      drm/arm/malidp: Switch to drm_atomic_get_new_crtc_state()
      drm/armada: Drop always true condition in atomic_check
      drm/armada: Switch to drm_atomic_get_new_crtc_state()
      drm/atmel-hlcdc: Switch to drm_atomic_get_new_crtc_state()
      drm/exynos: Switch to drm_atomic_get_new_crtc_state()
      drm/imx-dc: Switch to drm_atomic_get_new_crtc_state()
      drm/imx-dcss: Switch to drm_atomic_get_new_crtc_state()
      drm/imx-ipuv3: Switch to drm_atomic_get_new_crtc_state()
      drm/ingenic: Switch to drm_atomic_get_new_crtc_state()
      drm/kmb: Switch to drm_atomic_get_new_crtc_state()
      drm/logicvc: Switch to drm_atomic_get_new_crtc_state()
      drm/loongson: Switch to drm_atomic_get_new_crtc_state()
      drm/mediatek: Switch to drm_atomic_get_new_crtc_state()
      drm/msm/mdp5: Switch to drm_atomic_get_new_crtc_state()
      drm/omap: Switch to drm_atomic_get_new_crtc_state()
      drm/rockchip: Switch to drm_atomic_get_new_crtc_state()
      drm/sun4i: Switch to drm_atomic_get_new_crtc_state()
      drm/tegra: Switch to drm_atomic_get_new_crtc_state()
      drm/tilcdc: Switch to drm_atomic_get_new_crtc_state()
      drm/vboxvideo: Switch to drm_atomic_get_new_crtc_state()
      drm/vc4: Switch to drm_atomic_get_new_crtc_state()
      drm/atomic: Switch to drm_atomic_get_new_crtc_state()
      drm/framebuffer: Switch to drm_atomic_get_new_crtc_state()
      drm/atomic: Remove unused drm_atomic_get_existing_crtc_state()
      drm/atomic: Document __drm_crtcs_state state pointer
      drm/ingenic: crtc: Switch to ingenic_drm_get_new_priv_state()
      drm/atomic: Convert drm_atomic_get_private_obj_state() to use new plane state
      drm/atomic: Document __drm_private_objs_state state pointer

 drivers/gpu/drm/arm/malidp_planes.c             |   2 +-
 drivers/gpu/drm/armada/armada_plane.c           |   9 +-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c |   2 +-
 drivers/gpu/drm/drm_atomic.c                    |  21 ++--
 drivers/gpu/drm/drm_framebuffer.c               |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_plane.c       |   2 +-
 drivers/gpu/drm/imx/dc/dc-plane.c               |   2 +-
 drivers/gpu/drm/imx/dcss/dcss-plane.c           |   4 +-
 drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c         |   3 +-
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c       |   5 +-
 drivers/gpu/drm/ingenic/ingenic-ipu.c           |   4 +-
 drivers/gpu/drm/kmb/kmb_plane.c                 |   3 +-
 drivers/gpu/drm/logicvc/logicvc_layer.c         |   4 +-
 drivers/gpu/drm/loongson/lsdc_plane.c           |   2 +-
 drivers/gpu/drm/mediatek/mtk_plane.c            |   3 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c      |   7 +-
 drivers/gpu/drm/omapdrm/omap_plane.c            |   2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c     |   6 +-
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c    |   2 +-
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c          |   3 +-
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c          |   3 +-
 drivers/gpu/drm/tegra/dc.c                      |   2 +-
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c            |   9 +-
 drivers/gpu/drm/tilcdc/tilcdc_plane.c           |   3 +-
 drivers/gpu/drm/vboxvideo/vbox_mode.c           |   8 +-
 drivers/gpu/drm/vc4/vc4_plane.c                 |   6 +-
 drivers/gpu/drm/vkms/vkms_crtc.c                |   4 +-
 include/drm/drm_atomic.h                        | 144 ++++++++++++------------
 28 files changed, 126 insertions(+), 141 deletions(-)
---
base-commit: 2a1eea8fd601db4c52f0d14f8871663b7b052c91
change-id: 20250825-drm-no-more-existing-state-9b3252c1a33b

Best regards,
-- 
Maxime Ripard <mripard@kernel.org>

[PATCH v3 11/39] drm/ingenic: ipu: Switch to drm_atomic_get_new_crtc_state()

[Maxime Ripard]

The ingenic IPU atomic_set_property implementation uses the deprecated
drm_atomic_get_existing_crtc_state() helper.

This hook is called during the state building process, before
atomic_check, and thus before the states are swapped. The existing state
thus points to the new state, and we can use
drm_atomic_get_new_crtc_state() instead.

Reviewed-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-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-ipu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
index 26ebf424d63ec21ccee80221745c3e8bcc6b3d7f..2574a4b4d40a2c27cb212114117829d9f6ab3ddb 100644
--- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
+++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
@@ -703,11 +703,11 @@ ingenic_ipu_plane_atomic_set_property(struct drm_plane *plane,
 
 	mode_changed = val != ipu->sharpness;
 	ipu->sharpness = val;
 
 	if (state->crtc) {
-		crtc_state = drm_atomic_get_existing_crtc_state(state->state, state->crtc);
+		crtc_state = drm_atomic_get_new_crtc_state(state->state, state->crtc);
 		if (WARN_ON(!crtc_state))
 			return -EINVAL;
 
 		crtc_state->mode_changed |= mode_changed;
 	}

-- 
2.50.1

[PATCH v3 20/39] drm/ingenic: Switch to drm_atomic_get_new_crtc_state()

[Maxime Ripard]

The ingenic atomic_check implementation uses the deprecated
drm_atomic_get_existing_crtc_state() helper.

This hook is called as part of the global atomic_check, thus before the
states are swapped. The existing state thus points to the new state, and
we can use drm_atomic_get_new_crtc_state() instead.

Reviewed-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-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 | 3 +--
 drivers/gpu/drm/ingenic/ingenic-ipu.c     | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 9db1ceaed5188a4ef0897280dc72108eb3815b5f..05faed933e5619c796f2a4fa1906e0eaa029ac68 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -469,12 +469,11 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
 		return 0;
 
 	if (priv->soc_info->plane_f0_not_working && plane == &priv->f0)
 		return -EINVAL;
 
-	crtc_state = drm_atomic_get_existing_crtc_state(state,
-							crtc);
+	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	if (WARN_ON(!crtc_state))
 		return -EINVAL;
 
 	priv_state = ingenic_drm_get_priv_state(priv, state);
 	if (IS_ERR(priv_state))
diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
index 2574a4b4d40a2c27cb212114117829d9f6ab3ddb..32638a713241abbd4eaed09f0aaec2b790650cc9 100644
--- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
+++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
@@ -578,11 +578,11 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
 	struct ingenic_ipu_private_state *ipu_state;
 
 	if (!crtc)
 		return 0;
 
-	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
+	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 	if (WARN_ON(!crtc_state))
 		return -EINVAL;
 
 	ipu_state = ingenic_ipu_get_priv_state(ipu, state);
 	if (IS_ERR(ipu_state))

-- 
2.50.1

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

[Maxime Ripard]

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)))
 		return;
 
 	/* Set addresses of our DMA descriptor chains */
 	next_id = priv_state->use_palette ? HWDESC_PALETTE : 0;

-- 
2.50.1

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

[Paul Cercueil]

Hi Maxime,

Le mardi 09 septembre 2025 à 13:27 +0200, Maxime Ripard a écrit :
> 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>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> 
> ---
> 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..a1b641d63fc500dc169d0b0e22f
> 93168c343a242 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)))
>  		return;
>  
>  	/* Set addresses of our DMA descriptor chains */
>  	next_id = priv_state->use_palette ? HWDESC_PALETTE : 0;

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

[Ville Syrjälä]

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

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

[Paul Cercueil]

Hi Ville,

Le mardi 09 septembre 2025 à 16:52 +0300, Ville Syrjälä a écrit :
> 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..a1b641d63fc500dc169d0b0e2
> > 2f93168c343a242 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.

Good catch.

> 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...

Would it be just a matter of calling drm_atomic_get_private_obj_state()
in the crtc's .atomic_check() to make sure the object is created?

Cheers,
-Paul

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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3222 bytes --]

On Tue, Sep 09, 2025 at 04:45:27PM +0200, Paul Cercueil wrote:
> Hi Ville,
> 
> Le mardi 09 septembre 2025 à 16:52 +0300, Ville Syrjälä a écrit :
> > 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..a1b641d63fc500dc169d0b0e2
> > > 2f93168c343a242 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.
> 
> Good catch.

Yeah, thanks.

> > 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...
> 
> Would it be just a matter of calling drm_atomic_get_private_obj_state()
> in the crtc's .atomic_check() to make sure the object is created?

It's really not clear to me what that private object stores in the first
place. It looks like it's about toggling a bit in the crtc DMA
descriptors, but it's only set by planes?

Can you expand a bit on the hw design and why you're using a private
object to store that?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Ville Syrjälä]

On Tue, Sep 09, 2025 at 04:45:27PM +0200, Paul Cercueil wrote:
> Hi Ville,
> 
> Le mardi 09 septembre 2025 à 16:52 +0300, Ville Syrjälä a écrit :
> > 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..a1b641d63fc500dc169d0b0e2
> > > 2f93168c343a242 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.
> 
> Good catch.
> 
> > 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...
> 
> Would it be just a matter of calling drm_atomic_get_private_obj_state()
> in the crtc's .atomic_check() to make sure the object is created?

Looks like this thing only has the one crtc, so yeah, seems
fine to it uncoditionally there. With multiple crtcs the
private object locking would start to hinder parallelism.

-- 
Ville Syrjälä
Intel

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

[Paul Cercueil]

Hi Maxime,

Le mercredi 10 septembre 2025 à 13:26 +0200, Maxime Ripard a écrit :
> On Tue, Sep 09, 2025 at 04:45:27PM +0200, Paul Cercueil wrote:
> > Hi Ville,
> > 
> > Le mardi 09 septembre 2025 à 16:52 +0300, Ville Syrjälä a écrit :
> > > 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..a1b641d63fc500dc169d0
> > > > b0e2
> > > > 2f93168c343a242 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.
> > 
> > Good catch.
> 
> Yeah, thanks.
> 
> > > 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...
> > 
> > Would it be just a matter of calling
> > drm_atomic_get_private_obj_state()
> > in the crtc's .atomic_check() to make sure the object is created?
> 
> It's really not clear to me what that private object stores in the
> first
> place. It looks like it's about toggling a bit in the crtc DMA
> descriptors, but it's only set by planes?
> 
> Can you expand a bit on the hw design and why you're using a private
> object to store that?

The primary plane f0 supports paletted 8bpp, in which case the palette
needs to be DMA'd once per frame. The "use_palette" is set in the
plane's .atomic_check as it has access to the new frame's pixel format.
It is used in the plane's .atomic_update to configure the DMA chain of
the f0 plane to either transfer the frame data continuously (if non-
paletted) or alternate between transferring the frame data and the
palette. This field is also needed in the CRTC's .atomic_enable as
we're writing a different DMA descriptor chain address if the palette
is used (as the palette must be transferred before the frame).

Cheers,
-Paul