Nouveau - Jan 2017 - [PATCH 0/4] Allow ASYNC flip with atomic helpers.

On 2017-01-16 03:39 PM, Laurent Pinchart wrote:
> Hi Andrey,
>
> Thank you for the patches.
>
> On Monday 16 Jan 2017 10:44:54 Andrey Grodzovsky wrote:
>> This series is a folow-up on
>> https://patchwork.kernel.org/patch/9501787/
>>
>> The first patch makes changes to atomic helpers
>> to allow for drives with ASYNC flip support to use them.
>> Patches 2 and 3 are to use this in AMDGPU/DC and
>> patch 4 is possible cleanup in nouveau/kms who seems
>> to have the duplicate the helper as we did to support
>> ASYNC flips.
>
> I have my doubts regarding this. I'd much rather see userspace moving
to the
> atomic API instead of extending support for legacy APIs.
>
This change is not about introducing the async flag but cleaning up the 
legacy helpers to make sure drivers that currently use it through the 
legacy IOCTLs can benefit from the helpers and not have to roll their own.

If the problem is with the pflip_flags, wouldn't drivers still need that 
after moving userspace to the atomic IOCTL?

I don't disagree with you on having userspace move to atomic but I don't
expect to see all userspace drivers move to atomic in the next couple 
months. Why not clean this up in the meantime?

Harry
>> Andrey Grodzovsky (4):
>>   drm/atomic: Save flip flags in drm_plane_state
>>   drm/amdgpu: Remove flip_flag from amdgpu_crtc
>>   drm/amd/display: Switch to using atomic_helper for flip.
>>   drm/nouveau/kms/nv50: Switch to using atomic helper for flip.
>>
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h           |  1 -
>>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_types.c    | 92
-------------------
>>  drivers/gpu/drm/drm_atomic_helper.c                | 10 +--
>>  drivers/gpu/drm/nouveau/nv50_display.c             | 77
++----------------
>>  include/drm/drm_plane.h                            |  8 ++
>>  5 files changed, 22 insertions(+), 166 deletions(-)
>

Hi Harry,

On Monday 16 Jan 2017 16:13:39 Harry Wentland wrote:
> On 2017-01-16 03:39 PM, Laurent Pinchart wrote:
> > On Monday 16 Jan 2017 10:44:54 Andrey Grodzovsky wrote:
> >> This series is a folow-up on
> >> https://patchwork.kernel.org/patch/9501787/
> >> 
> >> The first patch makes changes to atomic helpers
> >> to allow for drives with ASYNC flip support to use them.
> >> Patches 2 and 3 are to use this in AMDGPU/DC and
> >> patch 4 is possible cleanup in nouveau/kms who seems
> >> to have the duplicate the helper as we did to support
> >> ASYNC flips.
> > 
> > I have my doubts regarding this. I'd much rather see userspace
moving to
> > the atomic API instead of extending support for legacy APIs.
> 
> This change is not about introducing the async flag but cleaning up the
> legacy helpers to make sure drivers that currently use it through the
> legacy IOCTLs can benefit from the helpers and not have to roll their own.
> 
> If the problem is with the pflip_flags, wouldn't drivers still need
that
> after moving userspace to the atomic IOCTL?
> 
> I don't disagree with you on having userspace move to atomic but I
don't
> expect to see all userspace drivers move to atomic in the next couple
> months. Why not clean this up in the meantime?
If this patch series was just about moving common driver code into the core, 
sure, but it goes beyond that. Or, actually, it needs to go beyond that, but 
doesn't yet. Removing the DRM_MODE_PAGE_FLIP_ASYNC test in patch 1/4 means 
that the DRM core will not reject async page flips anymore, for any driver 
that uses the helper. You thus need to either patch all drivers that use the 
helper to reject the flag, or implement the feature in the drivers (and 
preferably in the helpers then). The current version of this patch series will 
make all existing users of the helpers accept async page flips without 
actually implementing them.
> >> Andrey Grodzovsky (4):
> >>   drm/atomic: Save flip flags in drm_plane_state
> >>   drm/amdgpu: Remove flip_flag from amdgpu_crtc
> >>   drm/amd/display: Switch to using atomic_helper for flip.
> >>   drm/nouveau/kms/nv50: Switch to using atomic helper for flip.
> >>  
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h           |  1 -
> >>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_types.c    | 92
---------------
> >>  drivers/gpu/drm/drm_atomic_helper.c                | 10 +--
> >>  drivers/gpu/drm/nouveau/nv50_display.c             | 77
++-------------
> >>  include/drm/drm_plane.h                            |  8 ++
> >>  5 files changed, 22 insertions(+), 166 deletions(-)
-- 
Regards,

Laurent Pinchart

On Tue, Jan 17, 2017 at 12:14:24AM +0200, Laurent Pinchart
wrote:
> Hi Harry,
> 
> On Monday 16 Jan 2017 16:13:39 Harry Wentland wrote:
> > On 2017-01-16 03:39 PM, Laurent Pinchart wrote:
> > > On Monday 16 Jan 2017 10:44:54 Andrey Grodzovsky wrote:
> > >> This series is a folow-up on
> > >> https://patchwork.kernel.org/patch/9501787/
> > >> 
> > >> The first patch makes changes to atomic helpers
> > >> to allow for drives with ASYNC flip support to use them.
> > >> Patches 2 and 3 are to use this in AMDGPU/DC and
> > >> patch 4 is possible cleanup in nouveau/kms who seems
> > >> to have the duplicate the helper as we did to support
> > >> ASYNC flips.
> > > 
> > > I have my doubts regarding this. I'd much rather see
userspace moving to
> > > the atomic API instead of extending support for legacy APIs.
> > 
> > This change is not about introducing the async flag but cleaning up
the
> > legacy helpers to make sure drivers that currently use it through the
> > legacy IOCTLs can benefit from the helpers and not have to roll their
own.
> > 
> > If the problem is with the pflip_flags, wouldn't drivers still
need that
> > after moving userspace to the atomic IOCTL?
> > 
> > I don't disagree with you on having userspace move to atomic but I
don't
> > expect to see all userspace drivers move to atomic in the next couple
> > months. Why not clean this up in the meantime?
> 
> If this patch series was just about moving common driver code into the
core,
> sure, but it goes beyond that. Or, actually, it needs to go beyond that,
but
> doesn't yet. Removing the DRM_MODE_PAGE_FLIP_ASYNC test in patch 1/4
means
> that the DRM core will not reject async page flips anymore, for any driver 
> that uses the helper. You thus need to either patch all drivers that use
the
> helper to reject the flag, or implement the feature in the drivers (and 
> preferably in the helpers then). The current version of this patch series
will
> make all existing users of the helpers accept async page flips without 
> actually implementing them.
"How do we check for this driver feature" was one of the opens I
raised
here on irc, even before Andrey started typing :-) Definitely needs to be
addressed. What's imo optional is exposing async flips through the atomic
ioctl, since atm no atomic userspace seems to be in need of async flips
yet. And the semantics of multi-plane async flips are tricky to define
properly, so definitely want that userspace validation. That's why I think
just doing the compat stuff is ok.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch