[PATCH 05/27] drm/hisilicon: Implement some semblance of vblank event handling

[Daniel Vetter]

On Wed, Jun 08, 2016 at 04:17:55PM +0200, Maarten Lankhorst wrote:
> Op 08-06-16 om 14:18 schreef Daniel Vetter:
> > atomic_flush seems to be the right place, but I'm not entirely sure
> > whether this will catch them all. It could be that when disabling the
> > crtc we'll miss the vblank.
> >
> > While at it nuke the dummy functions.
> >
> > v2: Be more robust and either arm, when the CRTC is on, or just send
> > the event out right away.
> >
> > Cc: Xinliang Liu <xinliang.liu at linaro.org>
> > Cc: Xinwei Kong <kong.kongxinwei at hisilicon.com>
> > Cc: Archit Taneja <architt at codeaurora.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > ---
> >  drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 20 ++++++++++++--------
> >  1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> > index fba6372d060e..ed76baad525f 100644
> > --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> > +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> > @@ -502,13 +502,6 @@ static void ade_crtc_disable(struct drm_crtc *crtc)
> >  	acrtc->enable = false;
> >  }
> >  
> > -static int ade_crtc_atomic_check(struct drm_crtc *crtc,
> > -				 struct drm_crtc_state *state)
> > -{
> > -	/* do nothing */
> > -	return 0;
> > -}
> > -
> >  static void ade_crtc_mode_set_nofb(struct drm_crtc *crtc)
> >  {
> >  	struct ade_crtc *acrtc = to_ade_crtc(crtc);
> > @@ -537,6 +530,7 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc,
> >  {
> >  	struct ade_crtc *acrtc = to_ade_crtc(crtc);
> >  	struct ade_hw_ctx *ctx = acrtc->ctx;
> > +	struct drm_pending_vblank_event *event = crtc->state->event;
> >  	void __iomem *base = ctx->base;
> >  
> >  	/* only crtc is enabled regs take effect */
> > @@ -545,12 +539,22 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc,
> >  		/* flush ade registers */
> >  		writel(ADE_ENABLE, base + ADE_EN);
> >  	}
> > +
> > +	if (event) {
> > +		crtc->state->event = NULL;
> ^I keep wondering why we set this to NULL every time. Nothing should depend on this right? duplicate_state sets this member to NULL..
> I'd rather have crtc_state be constified after commit. Other than that..

I added a WARN_ON in hw_done to make sure drivers have indeed consumed the
event. Given that lots of drivers don't bother I think that's good, since
I want to avoid getting flooded with bug reports along the lines of "my
atomic driver hangs" with these new helpers. WARN_ON(state->event) in
hw_done together with the commen is hopefully hint enough.
-Daniel

> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> 
>  ~Maarten

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch