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

[Maxime Ripard <mripard@kernel.org>]

[-- 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 --]