[PATCH 1/7] drm/mediatek: fix atomic_state reference counting
Bibby Hsieh
bibby.hsieh at mediatek.com
Wed Nov 27 01:41:52 UTC 2019
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 at mediatek.com>
>
> 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
>
More information about the dri-devel
mailing list