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 <[email protected]> > > > Suggested-by: Ville Syrjälä <[email protected]> > > > Signed-off-by: Maxime Ripard <[email protected]> > > > > > > --- > > > To: Paul Cercueil <[email protected]> > > > Cc: [email protected] > > > --- > > > 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
