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

[Ville Syrjälä]

On Thu, May 11, 2017 at 04:29:56PM -0300, Gustavo Padovan wrote:
> 2017-05-09 Ville Syrjälä :
> 
> > On Thu, Apr 27, 2017 at 12:15:12PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan 
> > > 
> > > 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/
> > 
> > BTW I also realized recently that this is probably not going to work
> > w.r.t. per-crtc out fences. I know we agrees earlier that the
> > "return -1" trick would work, but now that I think about it again,
> > I don't think it actually will work when combined with non-blocking
> > commits since we can't decide whether to return -1 or a fence
> > until the commit has actually been performed.
> 
> What we agreed wasn't that the 1st request was going to return the

I presume you meant "was"

> out-fence and the subsequent requests modifying that request would
> return -1. How does that change with non-blocking?

With non-blocking the commit happens after the ioctl has returned to
userspace, so it's too late to return the -1.

I suppose one option would be to avoid in fences altogether. So we'd
do the synchronization in userspace, and then do a blocking commit
without in fences to get the out fence. But that would open the
thing up to more scheduling latencies and whatnot since userspace
would have to be involved more.

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

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

[Gustavo Padovan]

2017-05-09 Ville Syrjälä :

> On Thu, Apr 27, 2017 at 12:15:12PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > 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/
> 
> BTW I also realized recently that this is probably not going to work
> w.r.t. per-crtc out fences. I know we agrees earlier that the
> "return -1" trick would work, but now that I think about it again,
> I don't think it actually will work when combined with non-blocking
> commits since we can't decide whether to return -1 or a fence
> until the commit has actually been performed.

What we agreed wasn't that the 1st request was going to return the
out-fence and the subsequent requests modifying that request would
return -1. How does that change with non-blocking?

Gustavo
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Thu, Apr 27, 2017 at 12:15:12PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> 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/

BTW I also realized recently that this is probably not going to work
w.r.t. per-crtc out fences. I know we agrees earlier that the
"return -1" trick would work, but now that I think about it again,
I don't think it actually will work when combined with non-blocking
commits since we can't decide whether to return -1 or a fence
until the commit has actually been performed.

> 
> 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.
> 
> TODO
> 
>  - improve i-g-t tests where needed
>  - support async page flips (that can be done after uptreaming this part)
>  - figure out what to do for hw that do not update the plane until the next
>  vblank. Maybe wait and see what they do and them extract helpers?
> 
> Comments are welcome!
> 
> Gustavo Padovan (7):
>   drm/atomic: initial support for asynchronous plane update
>   drm/virtio: support async cursor updates
>   drm/i915: update cursors asynchronously through atomic
>   drm/i915: remove intel_cursor_plane_funcs
>   drm/msm: update cursors asynchronously through atomic
>   drm/msm: remove mdp5_cursor_plane_funcs
>   drm/vc4: update cursors asynchronously through atomic
> 
>  drivers/gpu/drm/drm_atomic.c  |  50 ++
>  drivers/gpu/drm/drm_atomic_helper.c   |  48 +
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  52 ++
>  drivers/gpu/drm/i915/intel_display.c  | 158 -
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 161 
> ++
>  drivers/gpu/drm/vc4/vc4_plane.c   |  94 +
>  drivers/gpu/drm/virtio/virtgpu_plane.c|  42 
>  include/drm/drm_atomic.h  |   2 +
>  include/drm/drm_atomic_helper.h   |   2 +
>  include/drm/drm_modeset_helper_vtables.h  |  45 +
>  include/uapi/drm/drm_mode.h   |   4 +-
>  11 files changed, 343 insertions(+), 315 deletions(-)
> 
> -- 
> 2.9.3
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[Gustavo Padovan]

2017-04-28 Ville Syrjälä :

> On Thu, Apr 27, 2017 at 03:36:50PM -0300, Gustavo Padovan wrote:
> > 2017-04-27 Ville Syrjälä :
> > 
> > > On Thu, Apr 27, 2017 at 12:15:12PM -0300, Gustavo Padovan wrote:
> > > > From: Gustavo Padovan 
> > > > 
> > > > 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.

Right, maybe I didn't checked this correctly. Removing that was a
mistake, I blame my lack of knowledge of such a big subsystem :)
I'll bring that code back.

> 
> 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.

The issue I wanted to solve in the first place was to create a way to
update cursors over the atomic ioctl as fast (or relatively fast) as
the legacy update. Then discussing this with Daniel Vetter he suggested
to solve a bigger problem: add generic async plane update that would
benefit cursors, async PageFlips (vc4 already has it) and VR (but I
don't really know all the needs for VR).

Gustavo


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Thu, Apr 27, 2017 at 03:36:50PM -0300, Gustavo Padovan wrote:
> 2017-04-27 Ville Syrjälä :
> 
> > On Thu, Apr 27, 2017 at 12:15:12PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan 
> > > 
> > > 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

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

[Gustavo Padovan]

2017-04-27 Ville Syrjälä :

> On Thu, Apr 27, 2017 at 12:15:12PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > 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. What we propose here
is to do this over atomic when DRM_MODE_ATOMIC_ASYNC_UPDATE flag is set.
Async PageFlips should use the same infrastructure in the future.

Gustavo
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Thu, Apr 27, 2017 at 12:15:12PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> 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.

> 
> TODO
> 
>  - improve i-g-t tests where needed
>  - support async page flips (that can be done after uptreaming this part)
>  - figure out what to do for hw that do not update the plane until the next
>  vblank. Maybe wait and see what they do and them extract helpers?
> 
> Comments are welcome!
> 
> Gustavo Padovan (7):
>   drm/atomic: initial support for asynchronous plane update
>   drm/virtio: support async cursor updates
>   drm/i915: update cursors asynchronously through atomic
>   drm/i915: remove intel_cursor_plane_funcs
>   drm/msm: update cursors asynchronously through atomic
>   drm/msm: remove mdp5_cursor_plane_funcs
>   drm/vc4: update cursors asynchronously through atomic
> 
>  drivers/gpu/drm/drm_atomic.c  |  50 ++
>  drivers/gpu/drm/drm_atomic_helper.c   |  48 +
>  drivers/gpu/drm/i915/intel_atomic_plane.c |  52 ++
>  drivers/gpu/drm/i915/intel_display.c  | 158 -
>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 161 
> ++
>  drivers/gpu/drm/vc4/vc4_plane.c   |  94 +
>  drivers/gpu/drm/virtio/virtgpu_plane.c|  42 
>  include/drm/drm_atomic.h  |   2 +
>  include/drm/drm_atomic_helper.h   |   2 +
>  include/drm/drm_modeset_helper_vtables.h  |  45 +
>  include/uapi/drm/drm_mode.h   |   4 +-
>  11 files changed, 343 insertions(+), 315 deletions(-)
> 
> -- 
> 2.9.3
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[Gustavo Padovan]

From: Gustavo Padovan 

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.

TODO

 - improve i-g-t tests where needed
 - support async page flips (that can be done after uptreaming this part)
 - figure out what to do for hw that do not update the plane until the next
 vblank. Maybe wait and see what they do and them extract helpers?

Comments are welcome!

Gustavo Padovan (7):
  drm/atomic: initial support for asynchronous plane update
  drm/virtio: support async cursor updates
  drm/i915: update cursors asynchronously through atomic
  drm/i915: remove intel_cursor_plane_funcs
  drm/msm: update cursors asynchronously through atomic
  drm/msm: remove mdp5_cursor_plane_funcs
  drm/vc4: update cursors asynchronously through atomic

 drivers/gpu/drm/drm_atomic.c  |  50 ++
 drivers/gpu/drm/drm_atomic_helper.c   |  48 +
 drivers/gpu/drm/i915/intel_atomic_plane.c |  52 ++
 drivers/gpu/drm/i915/intel_display.c  | 158 -
 drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 161 ++
 drivers/gpu/drm/vc4/vc4_plane.c   |  94 +
 drivers/gpu/drm/virtio/virtgpu_plane.c|  42 
 include/drm/drm_atomic.h  |   2 +
 include/drm/drm_atomic_helper.h   |   2 +
 include/drm/drm_modeset_helper_vtables.h  |  45 +
 include/uapi/drm/drm_mode.h   |   4 +-
 11 files changed, 343 insertions(+), 315 deletions(-)

-- 
2.9.3

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel