| Message ID | 1465388359-8070-5-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linaro.org> > Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> > Cc: Archit Taneja <architt@codeaurora.org> > Signed-off-by: Daniel Vetter <daniel.vetter@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.. Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> ~Maarten
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@linaro.org> > > Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> > > Cc: Archit Taneja <architt@codeaurora.org> > > Signed-off-by: Daniel Vetter <daniel.vetter@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@linux.intel.com> > > ~Maarten
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; + + spin_lock_irq(&crtc->dev->event_lock); + if (drm_crtc_vblank_get(crtc) == 0) + drm_crtc_arm_vblank_event(crtc, event); + else + drm_crtc_send_vblank_event(crtc, event); + spin_unlock_irq(&crtc->dev->event_lock); + } } static const struct drm_crtc_helper_funcs ade_crtc_helper_funcs = { .enable = ade_crtc_enable, .disable = ade_crtc_disable, - .atomic_check = ade_crtc_atomic_check, .mode_set_nofb = ade_crtc_mode_set_nofb, .atomic_begin = ade_crtc_atomic_begin, .atomic_flush = ade_crtc_atomic_flush,
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@linaro.org> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com> Cc: Archit Taneja <architt@codeaurora.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)