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