Re: [PATCH 1/7] drm/mediatek: fix atomic_state reference counting

[Bibby Hsieh]

On Tue, 2019-11-26 at 09:49 +0100, Daniel Vetter wrote:
> On Tue, Nov 26, 2019 at 02:29:26PM +0800, Bibby Hsieh wrote:
> > The DRM core takes care of all atomic state refcounting.
> > However, mediatek drm defers some work that accesses planes
> > and plane_states in drm_atomic_state, and must therefore
> > keep its own atomic state references until this work complete.
> > 
> > We take the atomic_state reference in atomic_fulsh() and ensure all the
> > information in atomic_state already was updated in hardware for
> > showing on screen and then schedules unreference_work to drop references
> > on atomic_state.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> > 
> > Signed-off-by: Bibby Hsieh <bibby.hsieh@xxxxxxxxxxxx>
> 
> This looks strange. For one you implement your own reference counting - if
> drivers have a need for drm_atomic_state_put_irq then I
> think we should implement this in the core code.
> 
> The other bit is that atomic commits are meant to simply wait for
> everything to finish - commit_tail doesn't hold locks, it's only ordered
> through drm_crtc_commit events (at least with the async implementation in
> the helpers), so you can just block there until your interrupt handler is
> done processing the commit. Depending how you want to do this you might
> want to wait before or after drm_atomic_helper_commit_hw_done().

OK, I will try to add a simple wait/completion before
drm_atomic_helper_commit_hw_done() until the commit was processed.

Thanks.

Bibby
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 11 +++-
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c  | 79 +++++++++++++++++++++++++
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.h  |  9 +++
> >  3 files changed, 97 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 29d0582e90e9..68b92adc96bb 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -7,7 +7,7 @@
> >  #include <linux/pm_runtime.h>
> >  
> >  #include <asm/barrier.h>
> > -
> > +#include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_plane_helper.h>
> >  #include <drm/drm_probe_helper.h>
> > @@ -47,6 +47,7 @@ struct mtk_drm_crtc {
> >  	struct mtk_disp_mutex		*mutex;
> >  	unsigned int			ddp_comp_nr;
> >  	struct mtk_ddp_comp		**ddp_comp;
> > +	struct drm_crtc_state		*old_crtc_state;
> >  };
> >  
> >  struct mtk_crtc_state {
> > @@ -362,6 +363,7 @@ static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc)
> >  static void mtk_crtc_ddp_config(struct drm_crtc *crtc)
> >  {
> >  	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> > +	struct drm_atomic_state *atomic_state = mtk_crtc->old_crtc_state->state;
> >  	struct mtk_crtc_state *state = to_mtk_crtc_state(mtk_crtc->base.state);
> >  	struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
> >  	unsigned int i;
> > @@ -399,6 +401,7 @@ static void mtk_crtc_ddp_config(struct drm_crtc *crtc)
> >  			plane_state->pending.config = false;
> >  		}
> >  		mtk_crtc->pending_planes = false;
> > +		mtk_atomic_state_put_queue(atomic_state);
> >  	}
> >  }
> >  
> > @@ -494,6 +497,7 @@ static void mtk_drm_crtc_atomic_begin(struct drm_crtc *crtc,
> >  static void mtk_drm_crtc_atomic_flush(struct drm_crtc *crtc,
> >  				      struct drm_crtc_state *old_crtc_state)
> >  {
> > +	struct drm_atomic_state *old_atomic_state = old_crtc_state->state;
> >  	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> >  	struct mtk_drm_private *priv = crtc->dev->dev_private;
> >  	unsigned int pending_planes = 0;
> > @@ -512,8 +516,11 @@ static void mtk_drm_crtc_atomic_flush(struct drm_crtc *crtc,
> >  			pending_planes |= BIT(i);
> >  		}
> >  	}
> > -	if (pending_planes)
> > +	if (pending_planes) {
> >  		mtk_crtc->pending_planes = true;
> > +		drm_atomic_state_get(old_atomic_state);
> > +		mtk_crtc->old_crtc_state = old_crtc_state;
> > +	}
> >  	if (crtc->state->color_mgmt_changed)
> >  		for (i = 0; i < mtk_crtc->ddp_comp_nr; i++)
> >  			mtk_ddp_gamma_set(mtk_crtc->ddp_comp[i], crtc->state);
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > index 6588dc6dd5e3..6c68283b6124 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > @@ -115,10 +115,85 @@ static int mtk_atomic_commit(struct drm_device *drm,
> >  	return 0;
> >  }
> >  
> > +struct mtk_atomic_state {
> > +	struct drm_atomic_state base;
> > +	struct list_head list;
> > +};
> > +
> > +static inline struct mtk_atomic_state *to_mtk_state(struct drm_atomic_state *s)
> > +{
> > +	return container_of(s, struct mtk_atomic_state, base);
> > +}
> > +
> > +void mtk_atomic_state_put_queue(struct drm_atomic_state *state)
> > +{
> > +	struct drm_device *drm = state->dev;
> > +	struct mtk_drm_private *mtk_drm = drm->dev_private;
> > +	struct mtk_atomic_state *mtk_state = to_mtk_state(state);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&mtk_drm->unreference.lock, flags);
> > +	list_add_tail(&mtk_state->list, &mtk_drm->unreference.list);
> > +	spin_unlock_irqrestore(&mtk_drm->unreference.lock, flags);
> > +
> > +	schedule_work(&mtk_drm->unreference.work);
> > +}
> > +
> > +static void mtk_unreference_work(struct work_struct *work)
> > +{
> > +	struct mtk_drm_private *mtk_drm = container_of(work,
> > +			struct mtk_drm_private, unreference.work);
> > +	unsigned long flags;
> > +	struct mtk_atomic_state *state, *tmp;
> > +
> > +	/*
> > +	 * framebuffers cannot be unreferenced in atomic context.
> > +	 * Therefore, only hold the spinlock when iterating unreference_list,
> > +	 * and drop it when doing the unreference.
> > +	 */
> > +	spin_lock_irqsave(&mtk_drm->unreference.lock, flags);
> > +	list_for_each_entry_safe(state, tmp, &mtk_drm->unreference.list, list) {
> > +		list_del(&state->list);
> > +		spin_unlock_irqrestore(&mtk_drm->unreference.lock, flags);
> > +		drm_atomic_state_put(&state->base);
> > +		spin_lock_irqsave(&mtk_drm->unreference.lock, flags);
> > +	}
> > +	spin_unlock_irqrestore(&mtk_drm->unreference.lock, flags);
> > +}
> > +
> > +static struct drm_atomic_state *
> > +		mtk_drm_atomic_state_alloc(struct drm_device *dev)
> > +{
> > +	struct mtk_atomic_state *mtk_state;
> > +
> > +	mtk_state = kzalloc(sizeof(*mtk_state), GFP_KERNEL);
> > +	if (!mtk_state)
> > +		return NULL;
> > +
> > +	if (drm_atomic_state_init(dev, &mtk_state->base) < 0) {
> > +		kfree(mtk_state);
> > +		return NULL;
> > +	}
> > +
> > +	INIT_LIST_HEAD(&mtk_state->list);
> > +
> > +	return &mtk_state->base;
> > +}
> > +
> > +static void mtk_drm_atomic_state_free(struct drm_atomic_state *state)
> > +{
> > +	struct mtk_atomic_state *mtk_state = to_mtk_state(state);
> > +
> > +	drm_atomic_state_default_release(state);
> > +	kfree(mtk_state);
> > +}
> > +
> >  static const struct drm_mode_config_funcs mtk_drm_mode_config_funcs = {
> >  	.fb_create = mtk_drm_mode_fb_create,
> >  	.atomic_check = drm_atomic_helper_check,
> >  	.atomic_commit = mtk_atomic_commit,
> > +	.atomic_state_alloc = mtk_drm_atomic_state_alloc,
> > +	.atomic_state_free = mtk_drm_atomic_state_free
> >  };
> >  
> >  static const enum mtk_ddp_comp_id mt2701_mtk_ddp_main[] = {
> > @@ -337,6 +412,10 @@ static int mtk_drm_kms_init(struct drm_device *drm)
> >  	drm_kms_helper_poll_init(drm);
> >  	drm_mode_config_reset(drm);
> >  
> > +	INIT_WORK(&private->unreference.work, mtk_unreference_work);
> > +	INIT_LIST_HEAD(&private->unreference.list);
> > +	spin_lock_init(&private->unreference.lock);
> > +
> >  	return 0;
> >  
> >  err_unset_dma_parms:
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > index b6a82728d563..c37d835cf949 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > @@ -55,6 +55,13 @@ struct mtk_drm_private {
> >  
> >  	struct drm_atomic_state *suspend_state;
> >  
> > +	struct {
> > +		struct work_struct	work;
> > +		struct list_head	list;
> > +		/* lock for unreference list */
> > +		spinlock_t		lock;
> > +	} unreference;
> > +
> >  	bool dma_parms_allocated;
> >  };
> >  
> > @@ -66,4 +73,6 @@ extern struct platform_driver mtk_dpi_driver;
> >  extern struct platform_driver mtk_dsi_driver;
> >  extern struct platform_driver mtk_mipi_tx_driver;
> >  
> > +void mtk_atomic_state_put_queue(struct drm_atomic_state *state);
> > +
> >  #endif /* MTK_DRM_DRV_H */
> > -- 
> > 2.18.0
>