On Tue, 2017-02-28 at 12:45 +0100, Daniel Vetter wrote: > 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@xxxxxxxxxxxxxx> > > > > Reviewed-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx> > > > > --- > > > > 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. Oh, ok. > > 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 appreciate the hint. We should remove the wait_for_vblanks and cleanup_planes calls from the commit_tail completely if no plane is currently being disabled, as there is no cleanup_fb for CMA based planes anyway. > 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. Sorry for being unclear, DP in this context doesn't mean DisplayPort, it's just a part of the display pipeline between DMA controller and display controller that handles colorspace conversion and compositing. It just happens to be called "display processor". > 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. Yes, that much is clear. We really have to tell the overlay plane to please stop at the next vblank, then wait for the vblank, and only then touch the stopped hardware to finalize disabling the plane. regards Philipp _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
