Re: [PATCH 4/4] drm/sun4i: make sure we don't have a commit pending

[Daniel Vetter]

On Thu, Jul 20, 2017 at 11:53:39AM +0200, Maxime Ripard wrote:
> Hi Daniel,
> 
> On Tue, Jul 18, 2017 at 09:35:03AM +0200, Daniel Vetter wrote:
> > On Tue, Jul 18, 2017 at 9:07 AM, Maxime Ripard
> > <maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> > > On Mon, Jul 17, 2017 at 02:57:19PM +0800, Chen-Yu Tsai wrote:
> > >> On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard
> > >> <maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> > >> > On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote:
> > >> >> Hi,
> > >> >>
> > >> >> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard
> > >> >> <maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> > >> >> > In the earlier display engine designs, any register access while a commit
> > >> >> > is pending is forbidden.
> > >> >> >
> > >> >> > One of the symptoms is that reading a register will return another, random,
> > >> >> > register value which can lead to register corruptions if we ever do a
> > >> >> > read/modify/write cycle.
> > >> >>
> > >> >> Alternatively, if changes to the backend (layers) are guaranteed to happen
> > >> >> while the CRTC is disabled (which seems to be the case after looking at
> > >> >> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we
> > >> >> could just turn on register auto-commit all the time and not deal with
> > >> >> this.
> > >> >
> > >> > As far as I understand, it will only be the case if we need a new
> > >> > modeset or we changed the active CRTC or connectors. But if you change
> > >> > only the format, buffers or properties it won't be the case, and we'll
> > >> > need to commit.
> > >>
> > >> So in other words, if someone were to use it for actual compositing and
> > >> moved the upper composited layer around, we would need commit support to be
> > >> safe.
> > >>
> > >> Sounds more or less like something a video player would do.
> > >
> > > Not only that. A change of buffer will happen every frame or so, and
> > > we can change the format whenever we want too (even if it's usually
> > > going to be in sync with a new buffer). Changing a property can happen
> > > any time too (like zpos for example).
> > 
> > You can upgrade any property change to an atomic modeset by e.g.
> > setting connector->mode_changed (and then making sure to call
> > check_modeset() helper again perhaps). This is for cases where your hw
> > can't handle a property change within 1 vblank. The default is just
> > the solution for most common hw.
> > 
> > The other way round works too, you can clear these flags in your
> > atomic_check callbacks. But that requires a bit more care (to make
> > sure you never clear it when there's something else also changing that
> > still needs a full modeset sequence to commit to hw).
> 
> Hmm, that's good to know, but that would imply disabling the CRTC each
> time we change even a small property, with all the visual artifacts it
> might imply, right?

This isn't black&white, you only need to set this when needed. Of course
that means you need to have code (and hopefully testcases) to make sure
you only set it when needed. And userspace can ask the driver whether a
given change requires a full modeset or not and then decided whether it
wants to do that switch or not.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch