[PATCH v2 3/4] drm/imx: call drm_atomic_helper_commit_hw_done after drm_atomic_helper_wait_for_vblanks

[Daniel Vetter]

On Tue, Feb 28, 2017 at 10:59:54AM +0100, Philipp Zabel wrote:
> On Mon, 2017-02-27 at 17:25 +0100, Daniel Vetter wrote:
> > On Mon, Feb 27, 2017 at 02:14:57PM +0100, Philipp Zabel wrote:
> > > Disabling planes will consist of two steps as of the following patch.
> > > First, the DP is asked to stop at the next vblank, and then, after the
> > > vblank the associated IDMAC channel is idle and can be disabled.
> > > To avoid further commits being awoken out of their wait for dependencies
> > > too early, we should report commit_hw_done only after wait_for_vblanks.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel at pengutronix.de>
> > > Reviewed-by: Lucas Stach <l.stach at pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/imx/imx-drm-core.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > > index f562cb7964b08..1ed120c181724 100644
> > > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > > @@ -169,10 +169,10 @@ static void imx_drm_atomic_commit_tail(struct drm_atomic_state *state)
> > >  
> > >  	drm_atomic_helper_commit_modeset_enables(dev, state);
> > >  
> > > -	drm_atomic_helper_commit_hw_done(state);
> > > -
> > >  	drm_atomic_helper_wait_for_vblanks(dev, state);
> > >  
> > > +	drm_atomic_helper_commit_hw_done(state);
> > 
> > That gives you a pretty good chance of halfing your refresh rate for async
> > flips. At least if you're not super careful about things. Did you
> > double-check this?
> > -Daniel
> 
> I'm not used to the terminology, does async flips just mean
> DRM_MODE_PAGE_FLIP_ASYNC? We can't support that at all, since the
> hardware can't flip during hblank. We are not allowed to touch or even
> look at the DMA channel parameter memory during the whole frame,
> unfortunately.

sorry, I mixed up async with nonblocking. I meant nonblocking.

> With sync flips during vblank, which is the only thing we can support,
> the only place waiting for the hw_done completion is
> drm_atomic_helper_wait_for_dependencies, which goes on to wait for the
> same commit's flip_done completion right afterwards. That is completed
> by the same vblank IRQ that wait_for_vblanks is also waiting for.
> 
> So to be completely correct, we could report hw_done before
> wait_for_vblanks only if there are no plane disables pending, and
> afterwards otherwise, but I don't think this should make a difference
> right now.

if it works, then it's all fine. Just wanted to raise here that this went
wrong before ... The issue is that the _next_ update only waits for
hw_done, not for the entire previous commit work item to complete. So
there's a very real chance that the next nonblocking commit gets delayed,
since you reduce the amount of interleaving they support. It mostly works,
except when you get it wrong, or when you're unlucky about races.

I think a better fix here would be to put the vblank wait into the disable
function of your DP port, to make sure you only incur the vblank delay for
the case that needs it, and not for the usual case of a stream of screen
updates. hw_done means "I stopped touching the hw, the next thread can
jump in", not "the hw is done touching stuff".

Oh and if this is all unclear in the docs, pls submit a patch to clarify
things more.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch