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
