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