On Wed, Sep 17, 2025 at 04:46:18PM +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. > > We still need to make sure the private state will be part of the global > state by the time atomic_enable runs, so we still need to call > ingenic_drm_get_priv_state() in atomic_check. We can then call > ingenic_drm_get_new_priv_state() in atomic_enable, which is a wrapper > around drm_atomic_get_new_private_obj_state(). > > Reported-by: Dmitry Baryshkov <[email protected]> > Suggested-by: Ville Syrjälä <[email protected]> > Signed-off-by: Maxime Ripard <[email protected]>
Seems fine by me. Reviewed-by: Ville Syrjälä <[email protected]> > --- > To: Paul Cercueil <[email protected]> > Cc: [email protected] > --- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index > 05faed933e5619c796f2a4fa1906e0eaa029ac68..d3213fbf22be14b177fc1b7100c5b721d5f17924 > 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > @@ -245,12 +245,12 @@ 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); > - if (WARN_ON(IS_ERR(priv_state))) > + priv_state = ingenic_drm_get_new_priv_state(priv, state); > + if (WARN_ON(!priv_state)) > return; > > /* Set addresses of our DMA descriptor chains */ > next_id = priv_state->use_palette ? HWDESC_PALETTE : 0; > regmap_write(priv->map, JZ_REG_LCD_DA0, dma_hwdesc_addr(priv, next_id)); > @@ -338,17 +338,23 @@ static int ingenic_drm_crtc_atomic_check(struct > drm_crtc *crtc, > { > struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, > crtc); > struct ingenic_drm *priv = drm_crtc_get_priv(crtc); > struct drm_plane_state *f1_state, *f0_state, *ipu_state = NULL; > + struct ingenic_drm_private_state *priv_state; > > if (crtc_state->gamma_lut && > drm_color_lut_size(crtc_state->gamma_lut) != > ARRAY_SIZE(priv->dma_hwdescs->palette)) { > dev_dbg(priv->dev, "Invalid palette size\n"); > return -EINVAL; > } > > + /* We will need the state in atomic_enable, so let's make sure it's > part of the state */ > + priv_state = ingenic_drm_get_priv_state(priv, state); > + if (IS_ERR(priv_state)) > + return PTR_ERR(priv_state); > + > if (drm_atomic_crtc_needs_modeset(crtc_state) && > priv->soc_info->has_osd) { > f1_state = drm_atomic_get_plane_state(crtc_state->state, > &priv->f1); > if (IS_ERR(f1_state)) > return PTR_ERR(f1_state); > > -- > 2.50.1 -- Ville Syrjälä Intel
