Re: [RFC v2 0/7] drm: asynchronous atomic plane update

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 27, 2017 at 03:36:50PM -0300, Gustavo Padovan wrote:
> 2017-04-27 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> 
> > On Thu, Apr 27, 2017 at 12:15:12PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > > 
> > > Hi,
> > > 
> > > Second take of Asynchronous Plane Updates over Atomic. Here I looked
> > > to msm, vc4 and i915 to identify a common pattern to create atomic helpers
> > > for async updates. So in patch 1 drm_atomic_async_check() and
> > > drm_atomic_helper_async_commit() are introduced along with driver's plane hooks:
> > > ->atomic_async_check() and ->atomic_async_commit().
> > > 
> > > For now we only support async update for one plane at a time. Also the async
> > > update can't modify the CRTC so no modesets are allowed.
> > > 
> > > Then the other patches add support for it in the drivers. I did virtio mostly
> > > for testing. i915 have been converted and I've been using it without any
> > > problem. IGT tests seems to be fine, but there are somewhat random failures
> > > with or without the async update changes. msm and vc4 are only compile-tested.
> > > So I think this needs more testing
> > > 
> > > I started IGT changes to test the Atomic IOCTL with the new flag:
> > > 
> > > https://git.collabora.com/cgit/user/padovan/intel-gpu-tools.git/
> > > 
> > > v2:
> > > 
> > > Apart from all comments on v1 one extra change I made was to remove the
> > > constraint of only updating the plane if the queued state didn't touch
> > > that plane. I believe it was a too cautious of a change, furthermore this
> > > constraint was affecting throughput negatively on i915.
> > 
> > So you're now allowing reordering the updates? As in update A is
> > scheduled before update B, but update B happens before update A.
> > That is not a good idea.
> 
> That is what already happens with legacy cursor updates. They jump ahead
> the scheduled update and apply the cursor update.

Well, that's clearly a bug then. They are supposed to be able to jump
ahead of other planes, but not themselves.

I think the real problem is having just one timeline for the entire
crtc. The proper solution would be to have a timeline for each plane.

> What we propose here
> is to do this over atomic when DRM_MODE_ATOMIC_ASYNC_UPDATE flag is set.

The cursor thing is a hack. Using it as a guideline for something else
is not a good idea IMO. Reordering, apart from being totally unexpected
would also cause all sorts of problems because the hardware state at
the time of the programming the hardware won't match what you checked
against in your async check function.

> Async PageFlips should use the same infrastructure in the future.

I don't quite see why you have to build a parallel infrastructure for
this stuff. If the driver would do things properly then it could just
as well do this stuff from the normal path as well. So I figured the
point of all this was just to unify the hacks to one place pretty much.
Expanding the hacks to some kind of big infrastrucure is not something
I'd do.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel