drm: add capability DRM_CAP_ASYNC_UPDATE

Message ID	20181123215326.14274-1-helen.koike@collabora.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <dri-devel-bounces@lists.freedesktop.org> sender: koike) with ESMTPSA id 1EC7F263AE1 From: Helen Koike <helen.koike@collabora.com> To: David Airlie <airlied@linux.ie> Subject: [PATCH] drm: add capability DRM_CAP_ASYNC_UPDATE Date: Fri, 23 Nov 2018 19:53:26 -0200 Message-Id: <20181123215326.14274-1-helen.koike@collabora.com> MIME-Version: 1.0 Precedence: list Cc: dnicoara@chromium.org, =?utf-8?q?St=C3=A9phane_Marchesin?= <marcheu@google.com>, Sean Paul <seanpaul@google.com>, alexandros.frantzis@collabora.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, tomasz Figa <tfiga@chromium.org>, Gustavo Padovan <gustavo.padovan@collabora.com>, kernel@collabora.com Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" <dri-devel-bounces@lists.freedesktop.org>
Series	drm: add capability DRM_CAP_ASYNC_UPDATE \| expand drm: add capability DRM_CAP_ASYNC_UPDATE

Helen Mae Koike Fornazier Nov. 23, 2018, 9:53 p.m. UTC

Allow userspace to identify if the driver supports async update.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
[prepared for upstream]
Signed-off-by: Helen Koike <helen.koike@collabora.com>

---
Hi,

This patch introduces the ASYNC_UPDATE cap, which originated from the
discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
figure that async_update exists.

This was tested using a small program that exercises the uAPI for easy
sanity testing. The program was created by Alexandros and modified by
Enric to test the capability flag [2].

The test worked on a rockchip Ficus v1.1 board on top of mainline plus
the patch to update cursors asynchronously through atomic plus the patch
that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.

To test, just build the program and use the --atomic flag to use the cursor
plane with the ATOMIC_AMEND flag. E.g.

  drm_cursor --atomic

[1] https://patchwork.freedesktop.org/patch/243088/
[2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability

Thanks
Helen


 drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
 include/uapi/drm/drm.h      |  1 +
 2 files changed, 12 insertions(+)

Ville Syrjälä Nov. 27, 2018, 1:34 p.m. UTC | #1

On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> Allow userspace to identify if the driver supports async update.

And what exactly is an "async update"?

I keep asking people to come up with the a better name for this, and to
document what it actually means. Recently I've been think we should
maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
avoid introducing yet another set of names for the same thing. We'd
still want to document things properly though.

> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> [prepared for upstream]
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> Hi,
> 
> This patch introduces the ASYNC_UPDATE cap, which originated from the
> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> figure that async_update exists.
> 
> This was tested using a small program that exercises the uAPI for easy
> sanity testing. The program was created by Alexandros and modified by
> Enric to test the capability flag [2].
> 
> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> the patch to update cursors asynchronously through atomic plus the patch
> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> 
> To test, just build the program and use the --atomic flag to use the cursor
> plane with the ATOMIC_AMEND flag. E.g.
> 
>   drm_cursor --atomic
> 
> [1] https://patchwork.freedesktop.org/patch/243088/
> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> 
> Thanks
> Helen
> 
> 
>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
>  include/uapi/drm/drm.h      |  1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 94bd872d56c4..4a7e0f874171 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -31,6 +31,7 @@
>  #include <drm/drm_ioctl.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_auth.h>
> +#include <drm/drm_modeset_helper_vtables.h>
>  #include "drm_legacy.h"
>  #include "drm_internal.h"
>  #include "drm_crtc_internal.h"
> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>  {
>  	struct drm_get_cap *req = data;
>  	struct drm_crtc *crtc;
> +	struct drm_plane *plane;
>  
>  	req->value = 0;
>  
> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>  	case DRM_CAP_CRTC_IN_VBLANK_EVENT:
>  		req->value = 1;
>  		break;
> +	case DRM_CAP_ASYNC_UPDATE:
> +		req->value = 1;
> +		list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> +			if (!plane->helper_private->atomic_async_update) {
> +				req->value = 0;
> +				break;
> +			}
> +		}
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 300f336633f2..ff01540cbb1d 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -649,6 +649,7 @@ struct drm_gem_open {
>  #define DRM_CAP_PAGE_FLIP_TARGET	0x11
>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT	0x12
>  #define DRM_CAP_SYNCOBJ		0x13
> +#define DRM_CAP_ASYNC_UPDATE		0x14
>  
>  /** DRM_IOCTL_GET_CAP ioctl argument type */
>  struct drm_get_cap {
> -- 
> 2.19.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Helen Mae Koike Fornazier Dec. 5, 2018, 4:12 p.m. UTC | #2

Hi Ville

On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
>> Allow userspace to identify if the driver supports async update.
> 
> And what exactly is an "async update"?

I agree we are lacking docs on this, I'll send in the next version as
soon as we agree on a name (please see below).

For reference:
https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html

> 
> I keep asking people to come up with the a better name for this, and to
> document what it actually means. Recently I've been think we should
> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> avoid introducing yet another set of names for the same thing. We'd
> still want to document things properly though.

Another name it was suggested was "atomic amend", this feature basically
allows userspace to complement an update previously sent (i.e. its in
the queue and wasn't commited yet), it allows adding a plane update to
the next commit. So what do you think in renaming it to "atomic amend"?
Or do you suggest another name? I am not familiar with vulkan terminology.


Thanks
Helen

> 
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> [prepared for upstream]
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>> Hi,
>>
>> This patch introduces the ASYNC_UPDATE cap, which originated from the
>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
>> figure that async_update exists.
>>
>> This was tested using a small program that exercises the uAPI for easy
>> sanity testing. The program was created by Alexandros and modified by
>> Enric to test the capability flag [2].
>>
>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
>> the patch to update cursors asynchronously through atomic plus the patch
>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
>>
>> To test, just build the program and use the --atomic flag to use the cursor
>> plane with the ATOMIC_AMEND flag. E.g.
>>
>>   drm_cursor --atomic
>>
>> [1] https://patchwork.freedesktop.org/patch/243088/
>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
>>
>> Thanks
>> Helen
>>
>>
>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
>>  include/uapi/drm/drm.h      |  1 +
>>  2 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index 94bd872d56c4..4a7e0f874171 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -31,6 +31,7 @@
>>  #include <drm/drm_ioctl.h>
>>  #include <drm/drmP.h>
>>  #include <drm/drm_auth.h>
>> +#include <drm/drm_modeset_helper_vtables.h>
>>  #include "drm_legacy.h"
>>  #include "drm_internal.h"
>>  #include "drm_crtc_internal.h"
>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>>  {
>>  	struct drm_get_cap *req = data;
>>  	struct drm_crtc *crtc;
>> +	struct drm_plane *plane;
>>  
>>  	req->value = 0;
>>  
>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>>  	case DRM_CAP_CRTC_IN_VBLANK_EVENT:
>>  		req->value = 1;
>>  		break;
>> +	case DRM_CAP_ASYNC_UPDATE:
>> +		req->value = 1;
>> +		list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
>> +			if (!plane->helper_private->atomic_async_update) {
>> +				req->value = 0;
>> +				break;
>> +			}
>> +		}
>> +		break;
>>  	default:
>>  		return -EINVAL;
>>  	}
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 300f336633f2..ff01540cbb1d 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -649,6 +649,7 @@ struct drm_gem_open {
>>  #define DRM_CAP_PAGE_FLIP_TARGET	0x11
>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT	0x12
>>  #define DRM_CAP_SYNCOBJ		0x13
>> +#define DRM_CAP_ASYNC_UPDATE		0x14
>>  
>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
>>  struct drm_get_cap {
>> -- 
>> 2.19.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Tomasz Figa Dec. 13, 2018, 4:02 a.m. UTC | #3

On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
>
> Hi Ville
>
> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> > On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> >> Allow userspace to identify if the driver supports async update.
> >
> > And what exactly is an "async update"?
>
> I agree we are lacking docs on this, I'll send in the next version as
> soon as we agree on a name (please see below).
>
> For reference:
> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
>
> >
> > I keep asking people to come up with the a better name for this, and to
> > document what it actually means. Recently I've been think we should
> > maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> > avoid introducing yet another set of names for the same thing. We'd
> > still want to document things properly though.
>
> Another name it was suggested was "atomic amend", this feature basically
> allows userspace to complement an update previously sent (i.e. its in
> the queue and wasn't commited yet), it allows adding a plane update to
> the next commit. So what do you think in renaming it to "atomic amend"?

Note that it doesn't seem to be what the code currently is doing. For
example, for cursor updates, it doesn't seem to be working on the
currently pending commit, but just directly issues an atomic async
update call to the planes. The code actually seems to fall back to a
normal sync commit, if there is an already pending commit touching the
same plane or including a modeset.

Best regards,
Tomasz

> Or do you suggest another name? I am not familiar with vulkan terminology.
>
>
> Thanks
> Helen
>
> >
> >>
> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> [prepared for upstream]
> >> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>
> >> ---
> >> Hi,
> >>
> >> This patch introduces the ASYNC_UPDATE cap, which originated from the
> >> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> >> figure that async_update exists.
> >>
> >> This was tested using a small program that exercises the uAPI for easy
> >> sanity testing. The program was created by Alexandros and modified by
> >> Enric to test the capability flag [2].
> >>
> >> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> >> the patch to update cursors asynchronously through atomic plus the patch
> >> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> >>
> >> To test, just build the program and use the --atomic flag to use the cursor
> >> plane with the ATOMIC_AMEND flag. E.g.
> >>
> >>   drm_cursor --atomic
> >>
> >> [1] https://patchwork.freedesktop.org/patch/243088/
> >> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> >>
> >> Thanks
> >> Helen
> >>
> >>
> >>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> >>  include/uapi/drm/drm.h      |  1 +
> >>  2 files changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> >> index 94bd872d56c4..4a7e0f874171 100644
> >> --- a/drivers/gpu/drm/drm_ioctl.c
> >> +++ b/drivers/gpu/drm/drm_ioctl.c
> >> @@ -31,6 +31,7 @@
> >>  #include <drm/drm_ioctl.h>
> >>  #include <drm/drmP.h>
> >>  #include <drm/drm_auth.h>
> >> +#include <drm/drm_modeset_helper_vtables.h>
> >>  #include "drm_legacy.h"
> >>  #include "drm_internal.h"
> >>  #include "drm_crtc_internal.h"
> >> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> >>  {
> >>      struct drm_get_cap *req = data;
> >>      struct drm_crtc *crtc;
> >> +    struct drm_plane *plane;
> >>
> >>      req->value = 0;
> >>
> >> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> >>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> >>              req->value = 1;
> >>              break;
> >> +    case DRM_CAP_ASYNC_UPDATE:
> >> +            req->value = 1;
> >> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> >> +                    if (!plane->helper_private->atomic_async_update) {
> >> +                            req->value = 0;
> >> +                            break;
> >> +                    }
> >> +            }
> >> +            break;
> >>      default:
> >>              return -EINVAL;
> >>      }
> >> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> >> index 300f336633f2..ff01540cbb1d 100644
> >> --- a/include/uapi/drm/drm.h
> >> +++ b/include/uapi/drm/drm.h
> >> @@ -649,6 +649,7 @@ struct drm_gem_open {
> >>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> >>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> >>  #define DRM_CAP_SYNCOBJ             0x13
> >> +#define DRM_CAP_ASYNC_UPDATE                0x14
> >>
> >>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> >>  struct drm_get_cap {
> >> --
> >> 2.19.1
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >

Helen Mae Koike Fornazier Dec. 14, 2018, 1:35 a.m. UTC | #4

Hi Tomasz,

On 12/13/18 2:02 AM, Tomasz Figa wrote:
> On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
>>
>> Hi Ville
>>
>> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
>>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
>>>> Allow userspace to identify if the driver supports async update.
>>>
>>> And what exactly is an "async update"?
>>
>> I agree we are lacking docs on this, I'll send in the next version as
>> soon as we agree on a name (please see below).
>>
>> For reference:
>> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
>>
>>>
>>> I keep asking people to come up with the a better name for this, and to
>>> document what it actually means. Recently I've been think we should
>>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
>>> avoid introducing yet another set of names for the same thing. We'd
>>> still want to document things properly though.
>>
>> Another name it was suggested was "atomic amend", this feature basically
>> allows userspace to complement an update previously sent (i.e. its in
>> the queue and wasn't commited yet), it allows adding a plane update to
>> the next commit. So what do you think in renaming it to "atomic amend"?
> 
> Note that it doesn't seem to be what the code currently is doing. For
> example, for cursor updates, it doesn't seem to be working on the
> currently pending commit, but just directly issues an atomic async
> update call to the planes. The code actually seems to fall back to a
> normal sync commit, if there is an already pending commit touching the
> same plane or including a modeset.

It should fail as discussed at:
https://patchwork.freedesktop.org/patch/243088/

There was the following code inside the drm_atomic_helper_async_check()
in the previous patch which would fallback to a normal commit if there
isn't any pending commit to amend:

+	if (!old_plane_state->commit)
+		return -EINVAL;

In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
this got removed, but which means that async update will be enabled
anyway. So the following code is wrong:

-	if (state->legacy_cursor_update)
+	if (state->async_update || state->legacy_cursor_update)
 		state->async_update = !drm_atomic_helper_async_check(dev, state);

Does it make sense? If yes I'll fix this in the next version of the
Atomic IOCTL patch (and also those two patches should be in the same
series, I'll send them together next time).

Thanks for pointing this out.

Please let me know if you still don't agree on the name "atomic amend",
or if I am missing something.

Helen

> 
> Best regards,
> Tomasz
> 
>> Or do you suggest another name? I am not familiar with vulkan terminology.
>>
>>
>> Thanks
>> Helen
>>
>>>
>>>>
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>> [prepared for upstream]
>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>
>>>> ---
>>>> Hi,
>>>>
>>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
>>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
>>>> figure that async_update exists.
>>>>
>>>> This was tested using a small program that exercises the uAPI for easy
>>>> sanity testing. The program was created by Alexandros and modified by
>>>> Enric to test the capability flag [2].
>>>>
>>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
>>>> the patch to update cursors asynchronously through atomic plus the patch
>>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
>>>>
>>>> To test, just build the program and use the --atomic flag to use the cursor
>>>> plane with the ATOMIC_AMEND flag. E.g.
>>>>
>>>>   drm_cursor --atomic
>>>>
>>>> [1] https://patchwork.freedesktop.org/patch/243088/
>>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
>>>>
>>>> Thanks
>>>> Helen
>>>>
>>>>
>>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
>>>>  include/uapi/drm/drm.h      |  1 +
>>>>  2 files changed, 12 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>>>> index 94bd872d56c4..4a7e0f874171 100644
>>>> --- a/drivers/gpu/drm/drm_ioctl.c
>>>> +++ b/drivers/gpu/drm/drm_ioctl.c
>>>> @@ -31,6 +31,7 @@
>>>>  #include <drm/drm_ioctl.h>
>>>>  #include <drm/drmP.h>
>>>>  #include <drm/drm_auth.h>
>>>> +#include <drm/drm_modeset_helper_vtables.h>
>>>>  #include "drm_legacy.h"
>>>>  #include "drm_internal.h"
>>>>  #include "drm_crtc_internal.h"
>>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>>>>  {
>>>>      struct drm_get_cap *req = data;
>>>>      struct drm_crtc *crtc;
>>>> +    struct drm_plane *plane;
>>>>
>>>>      req->value = 0;
>>>>
>>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
>>>>              req->value = 1;
>>>>              break;
>>>> +    case DRM_CAP_ASYNC_UPDATE:
>>>> +            req->value = 1;
>>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
>>>> +                    if (!plane->helper_private->atomic_async_update) {
>>>> +                            req->value = 0;
>>>> +                            break;
>>>> +                    }
>>>> +            }
>>>> +            break;
>>>>      default:
>>>>              return -EINVAL;
>>>>      }
>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>>> index 300f336633f2..ff01540cbb1d 100644
>>>> --- a/include/uapi/drm/drm.h
>>>> +++ b/include/uapi/drm/drm.h
>>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
>>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
>>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
>>>>  #define DRM_CAP_SYNCOBJ             0x13
>>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
>>>>
>>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
>>>>  struct drm_get_cap {
>>>> --
>>>> 2.19.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>

Tomasz Figa Dec. 20, 2018, 9:07 a.m. UTC | #5

Hi Helen,

On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
>
> Hi Tomasz,
>
> On 12/13/18 2:02 AM, Tomasz Figa wrote:
> > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> >>
> >> Hi Ville
> >>
> >> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> >>>> Allow userspace to identify if the driver supports async update.
> >>>
> >>> And what exactly is an "async update"?
> >>
> >> I agree we are lacking docs on this, I'll send in the next version as
> >> soon as we agree on a name (please see below).
> >>
> >> For reference:
> >> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> >>
> >>>
> >>> I keep asking people to come up with the a better name for this, and to
> >>> document what it actually means. Recently I've been think we should
> >>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> >>> avoid introducing yet another set of names for the same thing. We'd
> >>> still want to document things properly though.
> >>
> >> Another name it was suggested was "atomic amend", this feature basically
> >> allows userspace to complement an update previously sent (i.e. its in
> >> the queue and wasn't commited yet), it allows adding a plane update to
> >> the next commit. So what do you think in renaming it to "atomic amend"?
> >
> > Note that it doesn't seem to be what the code currently is doing. For
> > example, for cursor updates, it doesn't seem to be working on the
> > currently pending commit, but just directly issues an atomic async
> > update call to the planes. The code actually seems to fall back to a
> > normal sync commit, if there is an already pending commit touching the
> > same plane or including a modeset.
>
> It should fail as discussed at:
> https://patchwork.freedesktop.org/patch/243088/
>
> There was the following code inside the drm_atomic_helper_async_check()
> in the previous patch which would fallback to a normal commit if there
> isn't any pending commit to amend:
>
> +       if (!old_plane_state->commit)
> +               return -EINVAL;
>
> In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> this got removed, but which means that async update will be enabled
> anyway. So the following code is wrong:
>
> -       if (state->legacy_cursor_update)
> +       if (state->async_update || state->legacy_cursor_update)
>                 state->async_update = !drm_atomic_helper_async_check(dev, state);
>
> Does it make sense? If yes I'll fix this in the next version of the
> Atomic IOCTL patch (and also those two patches should be in the same
> series, I'll send them together next time).
>
> Thanks for pointing this out.
>
> Please let me know if you still don't agree on the name "atomic amend",
> or if I am missing something.

I'll defer it to the DRM maintainers. From Chrome OS perspective, we
need a way to commit the cursor plane asynchronously from other
commits any time the cursor changes its position or framebuffer. As
long as the new API allows that and the maintainers are fine with it,
I think I should be okay with it too.

Best regards,
Tomasz

>
> Helen
>
> >
> > Best regards,
> > Tomasz
> >
> >> Or do you suggest another name? I am not familiar with vulkan terminology.
> >>
> >>
> >> Thanks
> >> Helen
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >>>> [prepared for upstream]
> >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>>>
> >>>> ---
> >>>> Hi,
> >>>>
> >>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> >>>> figure that async_update exists.
> >>>>
> >>>> This was tested using a small program that exercises the uAPI for easy
> >>>> sanity testing. The program was created by Alexandros and modified by
> >>>> Enric to test the capability flag [2].
> >>>>
> >>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> >>>> the patch to update cursors asynchronously through atomic plus the patch
> >>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> >>>>
> >>>> To test, just build the program and use the --atomic flag to use the cursor
> >>>> plane with the ATOMIC_AMEND flag. E.g.
> >>>>
> >>>>   drm_cursor --atomic
> >>>>
> >>>> [1] https://patchwork.freedesktop.org/patch/243088/
> >>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> >>>>
> >>>> Thanks
> >>>> Helen
> >>>>
> >>>>
> >>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> >>>>  include/uapi/drm/drm.h      |  1 +
> >>>>  2 files changed, 12 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> >>>> index 94bd872d56c4..4a7e0f874171 100644
> >>>> --- a/drivers/gpu/drm/drm_ioctl.c
> >>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> >>>> @@ -31,6 +31,7 @@
> >>>>  #include <drm/drm_ioctl.h>
> >>>>  #include <drm/drmP.h>
> >>>>  #include <drm/drm_auth.h>
> >>>> +#include <drm/drm_modeset_helper_vtables.h>
> >>>>  #include "drm_legacy.h"
> >>>>  #include "drm_internal.h"
> >>>>  #include "drm_crtc_internal.h"
> >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> >>>>  {
> >>>>      struct drm_get_cap *req = data;
> >>>>      struct drm_crtc *crtc;
> >>>> +    struct drm_plane *plane;
> >>>>
> >>>>      req->value = 0;
> >>>>
> >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> >>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> >>>>              req->value = 1;
> >>>>              break;
> >>>> +    case DRM_CAP_ASYNC_UPDATE:
> >>>> +            req->value = 1;
> >>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> >>>> +                    if (!plane->helper_private->atomic_async_update) {
> >>>> +                            req->value = 0;
> >>>> +                            break;
> >>>> +                    }
> >>>> +            }
> >>>> +            break;
> >>>>      default:
> >>>>              return -EINVAL;
> >>>>      }
> >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> >>>> index 300f336633f2..ff01540cbb1d 100644
> >>>> --- a/include/uapi/drm/drm.h
> >>>> +++ b/include/uapi/drm/drm.h
> >>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> >>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> >>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> >>>>  #define DRM_CAP_SYNCOBJ             0x13
> >>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> >>>>
> >>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> >>>>  struct drm_get_cap {
> >>>> --
> >>>> 2.19.1
> >>>>
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> dri-devel@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>

Daniel Vetter Dec. 20, 2018, 10:47 a.m. UTC | #6

On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
>
> Hi Helen,
>
> On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
> >
> > Hi Tomasz,
> >
> > On 12/13/18 2:02 AM, Tomasz Figa wrote:
> > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> > >>
> > >> Hi Ville
> > >>
> > >> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> > >>>> Allow userspace to identify if the driver supports async update.
> > >>>
> > >>> And what exactly is an "async update"?
> > >>
> > >> I agree we are lacking docs on this, I'll send in the next version as
> > >> soon as we agree on a name (please see below).
> > >>
> > >> For reference:
> > >> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> > >>
> > >>>
> > >>> I keep asking people to come up with the a better name for this, and to
> > >>> document what it actually means. Recently I've been think we should
> > >>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> > >>> avoid introducing yet another set of names for the same thing. We'd
> > >>> still want to document things properly though.
> > >>
> > >> Another name it was suggested was "atomic amend", this feature basically
> > >> allows userspace to complement an update previously sent (i.e. its in
> > >> the queue and wasn't commited yet), it allows adding a plane update to
> > >> the next commit. So what do you think in renaming it to "atomic amend"?
> > >
> > > Note that it doesn't seem to be what the code currently is doing. For
> > > example, for cursor updates, it doesn't seem to be working on the
> > > currently pending commit, but just directly issues an atomic async
> > > update call to the planes. The code actually seems to fall back to a
> > > normal sync commit, if there is an already pending commit touching the
> > > same plane or including a modeset.
> >
> > It should fail as discussed at:
> > https://patchwork.freedesktop.org/patch/243088/
> >
> > There was the following code inside the drm_atomic_helper_async_check()
> > in the previous patch which would fallback to a normal commit if there
> > isn't any pending commit to amend:
> >
> > +       if (!old_plane_state->commit)
> > +               return -EINVAL;
> >
> > In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> > this got removed, but which means that async update will be enabled
> > anyway. So the following code is wrong:
> >
> > -       if (state->legacy_cursor_update)
> > +       if (state->async_update || state->legacy_cursor_update)
> >                 state->async_update = !drm_atomic_helper_async_check(dev, state);
> >
> > Does it make sense? If yes I'll fix this in the next version of the
> > Atomic IOCTL patch (and also those two patches should be in the same
> > series, I'll send them together next time).
> >
> > Thanks for pointing this out.
> >
> > Please let me know if you still don't agree on the name "atomic amend",
> > or if I am missing something.
>
> I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> need a way to commit the cursor plane asynchronously from other
> commits any time the cursor changes its position or framebuffer. As
> long as the new API allows that and the maintainers are fine with it,
> I think I should be okay with it too.

If this is just about the cursor, why is the current legacy cursor
ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
if we want to support having a normal atomic commit and a cursor
update in the same atomic ioctl, coming up with reasonable semantics
for that will be complicated.

Pointer to code that uses this would be better ofc.
-Daniel

> Best regards,
> Tomasz
>
> >
> > Helen
> >
> > >
> > > Best regards,
> > > Tomasz
> > >
> > >> Or do you suggest another name? I am not familiar with vulkan terminology.
> > >>
> > >>
> > >> Thanks
> > >> Helen
> > >>
> > >>>
> > >>>>
> > >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > >>>> [prepared for upstream]
> > >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > >>>>
> > >>>> ---
> > >>>> Hi,
> > >>>>
> > >>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> > >>>> figure that async_update exists.
> > >>>>
> > >>>> This was tested using a small program that exercises the uAPI for easy
> > >>>> sanity testing. The program was created by Alexandros and modified by
> > >>>> Enric to test the capability flag [2].
> > >>>>
> > >>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> > >>>> the patch to update cursors asynchronously through atomic plus the patch
> > >>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> > >>>>
> > >>>> To test, just build the program and use the --atomic flag to use the cursor
> > >>>> plane with the ATOMIC_AMEND flag. E.g.
> > >>>>
> > >>>>   drm_cursor --atomic
> > >>>>
> > >>>> [1] https://patchwork.freedesktop.org/patch/243088/
> > >>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> > >>>>
> > >>>> Thanks
> > >>>> Helen
> > >>>>
> > >>>>
> > >>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> > >>>>  include/uapi/drm/drm.h      |  1 +
> > >>>>  2 files changed, 12 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > >>>> index 94bd872d56c4..4a7e0f874171 100644
> > >>>> --- a/drivers/gpu/drm/drm_ioctl.c
> > >>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> > >>>> @@ -31,6 +31,7 @@
> > >>>>  #include <drm/drm_ioctl.h>
> > >>>>  #include <drm/drmP.h>
> > >>>>  #include <drm/drm_auth.h>
> > >>>> +#include <drm/drm_modeset_helper_vtables.h>
> > >>>>  #include "drm_legacy.h"
> > >>>>  #include "drm_internal.h"
> > >>>>  #include "drm_crtc_internal.h"
> > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > >>>>  {
> > >>>>      struct drm_get_cap *req = data;
> > >>>>      struct drm_crtc *crtc;
> > >>>> +    struct drm_plane *plane;
> > >>>>
> > >>>>      req->value = 0;
> > >>>>
> > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > >>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> > >>>>              req->value = 1;
> > >>>>              break;
> > >>>> +    case DRM_CAP_ASYNC_UPDATE:
> > >>>> +            req->value = 1;
> > >>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > >>>> +                    if (!plane->helper_private->atomic_async_update) {
> > >>>> +                            req->value = 0;
> > >>>> +                            break;
> > >>>> +                    }
> > >>>> +            }
> > >>>> +            break;
> > >>>>      default:
> > >>>>              return -EINVAL;
> > >>>>      }
> > >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > >>>> index 300f336633f2..ff01540cbb1d 100644
> > >>>> --- a/include/uapi/drm/drm.h
> > >>>> +++ b/include/uapi/drm/drm.h
> > >>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> > >>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> > >>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> > >>>>  #define DRM_CAP_SYNCOBJ             0x13
> > >>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> > >>>>
> > >>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> > >>>>  struct drm_get_cap {
> > >>>> --
> > >>>> 2.19.1
> > >>>>
> > >>>> _______________________________________________
> > >>>> dri-devel mailing list
> > >>>> dri-devel@lists.freedesktop.org
> > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Alex Deucher Dec. 20, 2018, 4:40 p.m. UTC | #7

+ Nicholas

On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > Hi Helen,
> >
> > On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On 12/13/18 2:02 AM, Tomasz Figa wrote:
> > > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > >>
> > > >> Hi Ville
> > > >>
> > > >> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> > > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> > > >>>> Allow userspace to identify if the driver supports async update.
> > > >>>
> > > >>> And what exactly is an "async update"?
> > > >>
> > > >> I agree we are lacking docs on this, I'll send in the next version as
> > > >> soon as we agree on a name (please see below).
> > > >>
> > > >> For reference:
> > > >> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> > > >>
> > > >>>
> > > >>> I keep asking people to come up with the a better name for this, and to
> > > >>> document what it actually means. Recently I've been think we should
> > > >>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> > > >>> avoid introducing yet another set of names for the same thing. We'd
> > > >>> still want to document things properly though.
> > > >>
> > > >> Another name it was suggested was "atomic amend", this feature basically
> > > >> allows userspace to complement an update previously sent (i.e. its in
> > > >> the queue and wasn't commited yet), it allows adding a plane update to
> > > >> the next commit. So what do you think in renaming it to "atomic amend"?
> > > >
> > > > Note that it doesn't seem to be what the code currently is doing. For
> > > > example, for cursor updates, it doesn't seem to be working on the
> > > > currently pending commit, but just directly issues an atomic async
> > > > update call to the planes. The code actually seems to fall back to a
> > > > normal sync commit, if there is an already pending commit touching the
> > > > same plane or including a modeset.
> > >
> > > It should fail as discussed at:
> > > https://patchwork.freedesktop.org/patch/243088/
> > >
> > > There was the following code inside the drm_atomic_helper_async_check()
> > > in the previous patch which would fallback to a normal commit if there
> > > isn't any pending commit to amend:
> > >
> > > +       if (!old_plane_state->commit)
> > > +               return -EINVAL;
> > >
> > > In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> > > this got removed, but which means that async update will be enabled
> > > anyway. So the following code is wrong:
> > >
> > > -       if (state->legacy_cursor_update)
> > > +       if (state->async_update || state->legacy_cursor_update)
> > >                 state->async_update = !drm_atomic_helper_async_check(dev, state);
> > >
> > > Does it make sense? If yes I'll fix this in the next version of the
> > > Atomic IOCTL patch (and also those two patches should be in the same
> > > series, I'll send them together next time).
> > >
> > > Thanks for pointing this out.
> > >
> > > Please let me know if you still don't agree on the name "atomic amend",
> > > or if I am missing something.
> >
> > I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> > need a way to commit the cursor plane asynchronously from other
> > commits any time the cursor changes its position or framebuffer. As
> > long as the new API allows that and the maintainers are fine with it,
> > I think I should be okay with it too.
>
> If this is just about the cursor, why is the current legacy cursor
> ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
> if we want to support having a normal atomic commit and a cursor
> update in the same atomic ioctl, coming up with reasonable semantics
> for that will be complicated.
>
> Pointer to code that uses this would be better ofc.

I haven't followed this thread too closely, but we ended up needing to
add a fast patch for cursor updates to amdgpu's atomic support to
avoid stuttering issues.  Other drivers may end up being affected by
this as well.  See:
https://bugs.freedesktop.org/show_bug.cgi?id=106175
Unfortunately, the fast path requires some hacks to handle the ref
counting properly on cursor fbs:
https://patchwork.freedesktop.org/patch/266138/
https://patchwork.freedesktop.org/patch/268298/
It looks like gamma may need similar treatment:
https://bugs.freedesktop.org/show_bug.cgi?id=108917

Alex


> -Daniel
>
> > Best regards,
> > Tomasz
> >
> > >
> > > Helen
> > >
> > > >
> > > > Best regards,
> > > > Tomasz
> > > >
> > > >> Or do you suggest another name? I am not familiar with vulkan terminology.
> > > >>
> > > >>
> > > >> Thanks
> > > >> Helen
> > > >>
> > > >>>
> > > >>>>
> > > >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > >>>> [prepared for upstream]
> > > >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > >>>>
> > > >>>> ---
> > > >>>> Hi,
> > > >>>>
> > > >>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> > > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> > > >>>> figure that async_update exists.
> > > >>>>
> > > >>>> This was tested using a small program that exercises the uAPI for easy
> > > >>>> sanity testing. The program was created by Alexandros and modified by
> > > >>>> Enric to test the capability flag [2].
> > > >>>>
> > > >>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> > > >>>> the patch to update cursors asynchronously through atomic plus the patch
> > > >>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> > > >>>>
> > > >>>> To test, just build the program and use the --atomic flag to use the cursor
> > > >>>> plane with the ATOMIC_AMEND flag. E.g.
> > > >>>>
> > > >>>>   drm_cursor --atomic
> > > >>>>
> > > >>>> [1] https://patchwork.freedesktop.org/patch/243088/
> > > >>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> > > >>>>
> > > >>>> Thanks
> > > >>>> Helen
> > > >>>>
> > > >>>>
> > > >>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> > > >>>>  include/uapi/drm/drm.h      |  1 +
> > > >>>>  2 files changed, 12 insertions(+)
> > > >>>>
> > > >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > >>>> index 94bd872d56c4..4a7e0f874171 100644
> > > >>>> --- a/drivers/gpu/drm/drm_ioctl.c
> > > >>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> > > >>>> @@ -31,6 +31,7 @@
> > > >>>>  #include <drm/drm_ioctl.h>
> > > >>>>  #include <drm/drmP.h>
> > > >>>>  #include <drm/drm_auth.h>
> > > >>>> +#include <drm/drm_modeset_helper_vtables.h>
> > > >>>>  #include "drm_legacy.h"
> > > >>>>  #include "drm_internal.h"
> > > >>>>  #include "drm_crtc_internal.h"
> > > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > >>>>  {
> > > >>>>      struct drm_get_cap *req = data;
> > > >>>>      struct drm_crtc *crtc;
> > > >>>> +    struct drm_plane *plane;
> > > >>>>
> > > >>>>      req->value = 0;
> > > >>>>
> > > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > >>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> > > >>>>              req->value = 1;
> > > >>>>              break;
> > > >>>> +    case DRM_CAP_ASYNC_UPDATE:
> > > >>>> +            req->value = 1;
> > > >>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > > >>>> +                    if (!plane->helper_private->atomic_async_update) {
> > > >>>> +                            req->value = 0;
> > > >>>> +                            break;
> > > >>>> +                    }
> > > >>>> +            }
> > > >>>> +            break;
> > > >>>>      default:
> > > >>>>              return -EINVAL;
> > > >>>>      }
> > > >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > >>>> index 300f336633f2..ff01540cbb1d 100644
> > > >>>> --- a/include/uapi/drm/drm.h
> > > >>>> +++ b/include/uapi/drm/drm.h
> > > >>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> > > >>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> > > >>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> > > >>>>  #define DRM_CAP_SYNCOBJ             0x13
> > > >>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> > > >>>>
> > > >>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> > > >>>>  struct drm_get_cap {
> > > >>>> --
> > > >>>> 2.19.1
> > > >>>>
> > > >>>> _______________________________________________
> > > >>>> dri-devel mailing list
> > > >>>> dri-devel@lists.freedesktop.org
> > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >>>
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter Dec. 20, 2018, 4:54 p.m. UTC | #8

On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> + Nicholas
>
> On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
> > >
> > > Hi Helen,
> > >
> > > On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > >
> > > > Hi Tomasz,
> > > >
> > > > On 12/13/18 2:02 AM, Tomasz Figa wrote:
> > > > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > > >>
> > > > >> Hi Ville
> > > > >>
> > > > >> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> > > > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> > > > >>>> Allow userspace to identify if the driver supports async update.
> > > > >>>
> > > > >>> And what exactly is an "async update"?
> > > > >>
> > > > >> I agree we are lacking docs on this, I'll send in the next version as
> > > > >> soon as we agree on a name (please see below).
> > > > >>
> > > > >> For reference:
> > > > >> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> > > > >>
> > > > >>>
> > > > >>> I keep asking people to come up with the a better name for this, and to
> > > > >>> document what it actually means. Recently I've been think we should
> > > > >>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> > > > >>> avoid introducing yet another set of names for the same thing. We'd
> > > > >>> still want to document things properly though.
> > > > >>
> > > > >> Another name it was suggested was "atomic amend", this feature basically
> > > > >> allows userspace to complement an update previously sent (i.e. its in
> > > > >> the queue and wasn't commited yet), it allows adding a plane update to
> > > > >> the next commit. So what do you think in renaming it to "atomic amend"?
> > > > >
> > > > > Note that it doesn't seem to be what the code currently is doing. For
> > > > > example, for cursor updates, it doesn't seem to be working on the
> > > > > currently pending commit, but just directly issues an atomic async
> > > > > update call to the planes. The code actually seems to fall back to a
> > > > > normal sync commit, if there is an already pending commit touching the
> > > > > same plane or including a modeset.
> > > >
> > > > It should fail as discussed at:
> > > > https://patchwork.freedesktop.org/patch/243088/
> > > >
> > > > There was the following code inside the drm_atomic_helper_async_check()
> > > > in the previous patch which would fallback to a normal commit if there
> > > > isn't any pending commit to amend:
> > > >
> > > > +       if (!old_plane_state->commit)
> > > > +               return -EINVAL;
> > > >
> > > > In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> > > > this got removed, but which means that async update will be enabled
> > > > anyway. So the following code is wrong:
> > > >
> > > > -       if (state->legacy_cursor_update)
> > > > +       if (state->async_update || state->legacy_cursor_update)
> > > >                 state->async_update = !drm_atomic_helper_async_check(dev, state);
> > > >
> > > > Does it make sense? If yes I'll fix this in the next version of the
> > > > Atomic IOCTL patch (and also those two patches should be in the same
> > > > series, I'll send them together next time).
> > > >
> > > > Thanks for pointing this out.
> > > >
> > > > Please let me know if you still don't agree on the name "atomic amend",
> > > > or if I am missing something.
> > >
> > > I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> > > need a way to commit the cursor plane asynchronously from other
> > > commits any time the cursor changes its position or framebuffer. As
> > > long as the new API allows that and the maintainers are fine with it,
> > > I think I should be okay with it too.
> >
> > If this is just about the cursor, why is the current legacy cursor
> > ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
> > if we want to support having a normal atomic commit and a cursor
> > update in the same atomic ioctl, coming up with reasonable semantics
> > for that will be complicated.
> >
> > Pointer to code that uses this would be better ofc.
>
> I haven't followed this thread too closely, but we ended up needing to
> add a fast patch for cursor updates to amdgpu's atomic support to
> avoid stuttering issues.  Other drivers may end up being affected by
> this as well.  See:
> https://bugs.freedesktop.org/show_bug.cgi?id=106175
> Unfortunately, the fast path requires some hacks to handle the ref
> counting properly on cursor fbs:
> https://patchwork.freedesktop.org/patch/266138/
> https://patchwork.freedesktop.org/patch/268298/
> It looks like gamma may need similar treatment:
> https://bugs.freedesktop.org/show_bug.cgi?id=108917

Can we get these patches cc'ed to dri-devel so that there's some
common solution? Everyone hacking legacy_cursor_update hacks on their
own doesn't really work well. Or would at least give some visibility
into what's all going on.

Not sure about the gamma thing since we had opposite bugs on i915
about gamma not being vsynced and tearing terribly. Cursor is special
since it tends to be too small to notice tearing.

Thanks, Daniel

> Alex
>
>
> > -Daniel
> >
> > > Best regards,
> > > Tomasz
> > >
> > > >
> > > > Helen
> > > >
> > > > >
> > > > > Best regards,
> > > > > Tomasz
> > > > >
> > > > >> Or do you suggest another name? I am not familiar with vulkan terminology.
> > > > >>
> > > > >>
> > > > >> Thanks
> > > > >> Helen
> > > > >>
> > > > >>>
> > > > >>>>
> > > > >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > >>>> [prepared for upstream]
> > > > >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > >>>>
> > > > >>>> ---
> > > > >>>> Hi,
> > > > >>>>
> > > > >>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> > > > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> > > > >>>> figure that async_update exists.
> > > > >>>>
> > > > >>>> This was tested using a small program that exercises the uAPI for easy
> > > > >>>> sanity testing. The program was created by Alexandros and modified by
> > > > >>>> Enric to test the capability flag [2].
> > > > >>>>
> > > > >>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> > > > >>>> the patch to update cursors asynchronously through atomic plus the patch
> > > > >>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> > > > >>>>
> > > > >>>> To test, just build the program and use the --atomic flag to use the cursor
> > > > >>>> plane with the ATOMIC_AMEND flag. E.g.
> > > > >>>>
> > > > >>>>   drm_cursor --atomic
> > > > >>>>
> > > > >>>> [1] https://patchwork.freedesktop.org/patch/243088/
> > > > >>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> > > > >>>>
> > > > >>>> Thanks
> > > > >>>> Helen
> > > > >>>>
> > > > >>>>
> > > > >>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> > > > >>>>  include/uapi/drm/drm.h      |  1 +
> > > > >>>>  2 files changed, 12 insertions(+)
> > > > >>>>
> > > > >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > > >>>> index 94bd872d56c4..4a7e0f874171 100644
> > > > >>>> --- a/drivers/gpu/drm/drm_ioctl.c
> > > > >>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> > > > >>>> @@ -31,6 +31,7 @@
> > > > >>>>  #include <drm/drm_ioctl.h>
> > > > >>>>  #include <drm/drmP.h>
> > > > >>>>  #include <drm/drm_auth.h>
> > > > >>>> +#include <drm/drm_modeset_helper_vtables.h>
> > > > >>>>  #include "drm_legacy.h"
> > > > >>>>  #include "drm_internal.h"
> > > > >>>>  #include "drm_crtc_internal.h"
> > > > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > >>>>  {
> > > > >>>>      struct drm_get_cap *req = data;
> > > > >>>>      struct drm_crtc *crtc;
> > > > >>>> +    struct drm_plane *plane;
> > > > >>>>
> > > > >>>>      req->value = 0;
> > > > >>>>
> > > > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > >>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> > > > >>>>              req->value = 1;
> > > > >>>>              break;
> > > > >>>> +    case DRM_CAP_ASYNC_UPDATE:
> > > > >>>> +            req->value = 1;
> > > > >>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > > > >>>> +                    if (!plane->helper_private->atomic_async_update) {
> > > > >>>> +                            req->value = 0;
> > > > >>>> +                            break;
> > > > >>>> +                    }
> > > > >>>> +            }
> > > > >>>> +            break;
> > > > >>>>      default:
> > > > >>>>              return -EINVAL;
> > > > >>>>      }
> > > > >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > > >>>> index 300f336633f2..ff01540cbb1d 100644
> > > > >>>> --- a/include/uapi/drm/drm.h
> > > > >>>> +++ b/include/uapi/drm/drm.h
> > > > >>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> > > > >>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> > > > >>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> > > > >>>>  #define DRM_CAP_SYNCOBJ             0x13
> > > > >>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> > > > >>>>
> > > > >>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> > > > >>>>  struct drm_get_cap {
> > > > >>>> --
> > > > >>>> 2.19.1
> > > > >>>>
> > > > >>>> _______________________________________________
> > > > >>>> dri-devel mailing list
> > > > >>>> dri-devel@lists.freedesktop.org
> > > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >>>
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> + Nicholas
>
> On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
> > >
> > > Hi Helen,
> > >
> > > On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > >
> > > > Hi Tomasz,
> > > >
> > > > On 12/13/18 2:02 AM, Tomasz Figa wrote:
> > > > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > > >>
> > > > >> Hi Ville
> > > > >>
> > > > >> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> > > > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> > > > >>>> Allow userspace to identify if the driver supports async update.
> > > > >>>
> > > > >>> And what exactly is an "async update"?
> > > > >>
> > > > >> I agree we are lacking docs on this, I'll send in the next version as
> > > > >> soon as we agree on a name (please see below).
> > > > >>
> > > > >> For reference:
> > > > >> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> > > > >>
> > > > >>>
> > > > >>> I keep asking people to come up with the a better name for this, and to
> > > > >>> document what it actually means. Recently I've been think we should
> > > > >>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> > > > >>> avoid introducing yet another set of names for the same thing. We'd
> > > > >>> still want to document things properly though.
> > > > >>
> > > > >> Another name it was suggested was "atomic amend", this feature basically
> > > > >> allows userspace to complement an update previously sent (i.e. its in
> > > > >> the queue and wasn't commited yet), it allows adding a plane update to
> > > > >> the next commit. So what do you think in renaming it to "atomic amend"?
> > > > >
> > > > > Note that it doesn't seem to be what the code currently is doing. For
> > > > > example, for cursor updates, it doesn't seem to be working on the
> > > > > currently pending commit, but just directly issues an atomic async
> > > > > update call to the planes. The code actually seems to fall back to a
> > > > > normal sync commit, if there is an already pending commit touching the
> > > > > same plane or including a modeset.
> > > >
> > > > It should fail as discussed at:
> > > > https://patchwork.freedesktop.org/patch/243088/
> > > >
> > > > There was the following code inside the drm_atomic_helper_async_check()
> > > > in the previous patch which would fallback to a normal commit if there
> > > > isn't any pending commit to amend:
> > > >
> > > > +       if (!old_plane_state->commit)
> > > > +               return -EINVAL;
> > > >
> > > > In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> > > > this got removed, but which means that async update will be enabled
> > > > anyway. So the following code is wrong:
> > > >
> > > > -       if (state->legacy_cursor_update)
> > > > +       if (state->async_update || state->legacy_cursor_update)
> > > >                 state->async_update = !drm_atomic_helper_async_check(dev, state);
> > > >
> > > > Does it make sense? If yes I'll fix this in the next version of the
> > > > Atomic IOCTL patch (and also those two patches should be in the same
> > > > series, I'll send them together next time).
> > > >
> > > > Thanks for pointing this out.
> > > >
> > > > Please let me know if you still don't agree on the name "atomic amend",
> > > > or if I am missing something.
> > >
> > > I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> > > need a way to commit the cursor plane asynchronously from other
> > > commits any time the cursor changes its position or framebuffer. As
> > > long as the new API allows that and the maintainers are fine with it,
> > > I think I should be okay with it too.
> >
> > If this is just about the cursor, why is the current legacy cursor
> > ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
> > if we want to support having a normal atomic commit and a cursor
> > update in the same atomic ioctl, coming up with reasonable semantics
> > for that will be complicated.
> >
> > Pointer to code that uses this would be better ofc.
>
> I haven't followed this thread too closely, but we ended up needing to
> add a fast patch for cursor updates to amdgpu's atomic support to
> avoid stuttering issues.  Other drivers may end up being affected by
> this as well.  See:
> https://bugs.freedesktop.org/show_bug.cgi?id=106175
> Unfortunately, the fast path requires some hacks to handle the ref
> counting properly on cursor fbs:
> https://patchwork.freedesktop.org/patch/266138/
> https://patchwork.freedesktop.org/patch/268298/
> It looks like gamma may need similar treatment:
> https://bugs.freedesktop.org/show_bug.cgi?id=108917
>
> Alex
>
>
> > -Daniel
> >
> > > Best regards,
> > > Tomasz
> > >
> > > >
> > > > Helen
> > > >
> > > > >
> > > > > Best regards,
> > > > > Tomasz
> > > > >
> > > > >> Or do you suggest another name? I am not familiar with vulkan terminology.
> > > > >>
> > > > >>
> > > > >> Thanks
> > > > >> Helen
> > > > >>
> > > > >>>
> > > > >>>>
> > > > >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > >>>> [prepared for upstream]
> > > > >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > >>>>
> > > > >>>> ---
> > > > >>>> Hi,
> > > > >>>>
> > > > >>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> > > > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> > > > >>>> figure that async_update exists.
> > > > >>>>
> > > > >>>> This was tested using a small program that exercises the uAPI for easy
> > > > >>>> sanity testing. The program was created by Alexandros and modified by
> > > > >>>> Enric to test the capability flag [2].
> > > > >>>>
> > > > >>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> > > > >>>> the patch to update cursors asynchronously through atomic plus the patch
> > > > >>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> > > > >>>>
> > > > >>>> To test, just build the program and use the --atomic flag to use the cursor
> > > > >>>> plane with the ATOMIC_AMEND flag. E.g.
> > > > >>>>
> > > > >>>>   drm_cursor --atomic
> > > > >>>>
> > > > >>>> [1] https://patchwork.freedesktop.org/patch/243088/
> > > > >>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> > > > >>>>
> > > > >>>> Thanks
> > > > >>>> Helen
> > > > >>>>
> > > > >>>>
> > > > >>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> > > > >>>>  include/uapi/drm/drm.h      |  1 +
> > > > >>>>  2 files changed, 12 insertions(+)
> > > > >>>>
> > > > >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > > >>>> index 94bd872d56c4..4a7e0f874171 100644
> > > > >>>> --- a/drivers/gpu/drm/drm_ioctl.c
> > > > >>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> > > > >>>> @@ -31,6 +31,7 @@
> > > > >>>>  #include <drm/drm_ioctl.h>
> > > > >>>>  #include <drm/drmP.h>
> > > > >>>>  #include <drm/drm_auth.h>
> > > > >>>> +#include <drm/drm_modeset_helper_vtables.h>
> > > > >>>>  #include "drm_legacy.h"
> > > > >>>>  #include "drm_internal.h"
> > > > >>>>  #include "drm_crtc_internal.h"
> > > > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > >>>>  {
> > > > >>>>      struct drm_get_cap *req = data;
> > > > >>>>      struct drm_crtc *crtc;
> > > > >>>> +    struct drm_plane *plane;
> > > > >>>>
> > > > >>>>      req->value = 0;
> > > > >>>>
> > > > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > >>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> > > > >>>>              req->value = 1;
> > > > >>>>              break;
> > > > >>>> +    case DRM_CAP_ASYNC_UPDATE:
> > > > >>>> +            req->value = 1;
> > > > >>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > > > >>>> +                    if (!plane->helper_private->atomic_async_update) {
> > > > >>>> +                            req->value = 0;
> > > > >>>> +                            break;
> > > > >>>> +                    }
> > > > >>>> +            }
> > > > >>>> +            break;
> > > > >>>>      default:
> > > > >>>>              return -EINVAL;
> > > > >>>>      }
> > > > >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > > >>>> index 300f336633f2..ff01540cbb1d 100644
> > > > >>>> --- a/include/uapi/drm/drm.h
> > > > >>>> +++ b/include/uapi/drm/drm.h
> > > > >>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> > > > >>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> > > > >>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> > > > >>>>  #define DRM_CAP_SYNCOBJ             0x13
> > > > >>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> > > > >>>>
> > > > >>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> > > > >>>>  struct drm_get_cap {
> > > > >>>> --
> > > > >>>> 2.19.1
> > > > >>>>
> > > > >>>> _______________________________________________
> > > > >>>> dri-devel mailing list
> > > > >>>> dri-devel@lists.freedesktop.org
> > > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >>>
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

Alex Deucher Dec. 20, 2018, 5:03 p.m. UTC | #9

+ Harry

On Thu, Dec 20, 2018 at 11:54 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > + Nicholas
> >
> > On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
> > > >
> > > > Hi Helen,
> > > >
> > > > On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > > >
> > > > > Hi Tomasz,
> > > > >
> > > > > On 12/13/18 2:02 AM, Tomasz Figa wrote:
> > > > > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > > > >>
> > > > > >> Hi Ville
> > > > > >>
> > > > > >> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> > > > > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> > > > > >>>> Allow userspace to identify if the driver supports async update.
> > > > > >>>
> > > > > >>> And what exactly is an "async update"?
> > > > > >>
> > > > > >> I agree we are lacking docs on this, I'll send in the next version as
> > > > > >> soon as we agree on a name (please see below).
> > > > > >>
> > > > > >> For reference:
> > > > > >> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> > > > > >>
> > > > > >>>
> > > > > >>> I keep asking people to come up with the a better name for this, and to
> > > > > >>> document what it actually means. Recently I've been think we should
> > > > > >>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> > > > > >>> avoid introducing yet another set of names for the same thing. We'd
> > > > > >>> still want to document things properly though.
> > > > > >>
> > > > > >> Another name it was suggested was "atomic amend", this feature basically
> > > > > >> allows userspace to complement an update previously sent (i.e. its in
> > > > > >> the queue and wasn't commited yet), it allows adding a plane update to
> > > > > >> the next commit. So what do you think in renaming it to "atomic amend"?
> > > > > >
> > > > > > Note that it doesn't seem to be what the code currently is doing. For
> > > > > > example, for cursor updates, it doesn't seem to be working on the
> > > > > > currently pending commit, but just directly issues an atomic async
> > > > > > update call to the planes. The code actually seems to fall back to a
> > > > > > normal sync commit, if there is an already pending commit touching the
> > > > > > same plane or including a modeset.
> > > > >
> > > > > It should fail as discussed at:
> > > > > https://patchwork.freedesktop.org/patch/243088/
> > > > >
> > > > > There was the following code inside the drm_atomic_helper_async_check()
> > > > > in the previous patch which would fallback to a normal commit if there
> > > > > isn't any pending commit to amend:
> > > > >
> > > > > +       if (!old_plane_state->commit)
> > > > > +               return -EINVAL;
> > > > >
> > > > > In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> > > > > this got removed, but which means that async update will be enabled
> > > > > anyway. So the following code is wrong:
> > > > >
> > > > > -       if (state->legacy_cursor_update)
> > > > > +       if (state->async_update || state->legacy_cursor_update)
> > > > >                 state->async_update = !drm_atomic_helper_async_check(dev, state);
> > > > >
> > > > > Does it make sense? If yes I'll fix this in the next version of the
> > > > > Atomic IOCTL patch (and also those two patches should be in the same
> > > > > series, I'll send them together next time).
> > > > >
> > > > > Thanks for pointing this out.
> > > > >
> > > > > Please let me know if you still don't agree on the name "atomic amend",
> > > > > or if I am missing something.
> > > >
> > > > I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> > > > need a way to commit the cursor plane asynchronously from other
> > > > commits any time the cursor changes its position or framebuffer. As
> > > > long as the new API allows that and the maintainers are fine with it,
> > > > I think I should be okay with it too.
> > >
> > > If this is just about the cursor, why is the current legacy cursor
> > > ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
> > > if we want to support having a normal atomic commit and a cursor
> > > update in the same atomic ioctl, coming up with reasonable semantics
> > > for that will be complicated.
> > >
> > > Pointer to code that uses this would be better ofc.
> >
> > I haven't followed this thread too closely, but we ended up needing to
> > add a fast patch for cursor updates to amdgpu's atomic support to
> > avoid stuttering issues.  Other drivers may end up being affected by
> > this as well.  See:
> > https://bugs.freedesktop.org/show_bug.cgi?id=106175
> > Unfortunately, the fast path requires some hacks to handle the ref
> > counting properly on cursor fbs:
> > https://patchwork.freedesktop.org/patch/266138/
> > https://patchwork.freedesktop.org/patch/268298/
> > It looks like gamma may need similar treatment:
> > https://bugs.freedesktop.org/show_bug.cgi?id=108917
>
> Can we get these patches cc'ed to dri-devel so that there's some
> common solution? Everyone hacking legacy_cursor_update hacks on their
> own doesn't really work well. Or would at least give some visibility
> into what's all going on.

Agreed.

>
> Not sure about the gamma thing since we had opposite bugs on i915
> about gamma not being vsynced and tearing terribly. Cursor is special
> since it tends to be too small to notice tearing.

Our cursor hw (and possibly gamma as well Nicholas?  Harry?) is double
buffered, so we can update it any time for the most part and the
changes won't take affect until the next vupdate period.

Alex

>
> Thanks, Daniel
>
> > Alex
> >
> >
> > > -Daniel
> > >
> > > > Best regards,
> > > > Tomasz
> > > >
> > > > >
> > > > > Helen
> > > > >
> > > > > >
> > > > > > Best regards,
> > > > > > Tomasz
> > > > > >
> > > > > >> Or do you suggest another name? I am not familiar with vulkan terminology.
> > > > > >>
> > > > > >>
> > > > > >> Thanks
> > > > > >> Helen
> > > > > >>
> > > > > >>>
> > > > > >>>>
> > > > > >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > >>>> [prepared for upstream]
> > > > > >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > > >>>>
> > > > > >>>> ---
> > > > > >>>> Hi,
> > > > > >>>>
> > > > > >>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> > > > > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> > > > > >>>> figure that async_update exists.
> > > > > >>>>
> > > > > >>>> This was tested using a small program that exercises the uAPI for easy
> > > > > >>>> sanity testing. The program was created by Alexandros and modified by
> > > > > >>>> Enric to test the capability flag [2].
> > > > > >>>>
> > > > > >>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> > > > > >>>> the patch to update cursors asynchronously through atomic plus the patch
> > > > > >>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> > > > > >>>>
> > > > > >>>> To test, just build the program and use the --atomic flag to use the cursor
> > > > > >>>> plane with the ATOMIC_AMEND flag. E.g.
> > > > > >>>>
> > > > > >>>>   drm_cursor --atomic
> > > > > >>>>
> > > > > >>>> [1] https://patchwork.freedesktop.org/patch/243088/
> > > > > >>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> > > > > >>>>
> > > > > >>>> Thanks
> > > > > >>>> Helen
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> > > > > >>>>  include/uapi/drm/drm.h      |  1 +
> > > > > >>>>  2 files changed, 12 insertions(+)
> > > > > >>>>
> > > > > >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > > > >>>> index 94bd872d56c4..4a7e0f874171 100644
> > > > > >>>> --- a/drivers/gpu/drm/drm_ioctl.c
> > > > > >>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> > > > > >>>> @@ -31,6 +31,7 @@
> > > > > >>>>  #include <drm/drm_ioctl.h>
> > > > > >>>>  #include <drm/drmP.h>
> > > > > >>>>  #include <drm/drm_auth.h>
> > > > > >>>> +#include <drm/drm_modeset_helper_vtables.h>
> > > > > >>>>  #include "drm_legacy.h"
> > > > > >>>>  #include "drm_internal.h"
> > > > > >>>>  #include "drm_crtc_internal.h"
> > > > > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > > >>>>  {
> > > > > >>>>      struct drm_get_cap *req = data;
> > > > > >>>>      struct drm_crtc *crtc;
> > > > > >>>> +    struct drm_plane *plane;
> > > > > >>>>
> > > > > >>>>      req->value = 0;
> > > > > >>>>
> > > > > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > > >>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> > > > > >>>>              req->value = 1;
> > > > > >>>>              break;
> > > > > >>>> +    case DRM_CAP_ASYNC_UPDATE:
> > > > > >>>> +            req->value = 1;
> > > > > >>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > > > > >>>> +                    if (!plane->helper_private->atomic_async_update) {
> > > > > >>>> +                            req->value = 0;
> > > > > >>>> +                            break;
> > > > > >>>> +                    }
> > > > > >>>> +            }
> > > > > >>>> +            break;
> > > > > >>>>      default:
> > > > > >>>>              return -EINVAL;
> > > > > >>>>      }
> > > > > >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > > > >>>> index 300f336633f2..ff01540cbb1d 100644
> > > > > >>>> --- a/include/uapi/drm/drm.h
> > > > > >>>> +++ b/include/uapi/drm/drm.h
> > > > > >>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> > > > > >>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> > > > > >>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> > > > > >>>>  #define DRM_CAP_SYNCOBJ             0x13
> > > > > >>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> > > > > >>>>
> > > > > >>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> > > > > >>>>  struct drm_get_cap {
> > > > > >>>> --
> > > > > >>>> 2.19.1
> > > > > >>>>
> > > > > >>>> _______________________________________________
> > > > > >>>> dri-devel mailing list
> > > > > >>>> dri-devel@lists.freedesktop.org
> > > > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > >>>
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > + Nicholas
> >
> > On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
> > > >
> > > > Hi Helen,
> > > >
> > > > On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > > >
> > > > > Hi Tomasz,
> > > > >
> > > > > On 12/13/18 2:02 AM, Tomasz Figa wrote:
> > > > > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > > > >>
> > > > > >> Hi Ville
> > > > > >>
> > > > > >> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> > > > > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> > > > > >>>> Allow userspace to identify if the driver supports async update.
> > > > > >>>
> > > > > >>> And what exactly is an "async update"?
> > > > > >>
> > > > > >> I agree we are lacking docs on this, I'll send in the next version as
> > > > > >> soon as we agree on a name (please see below).
> > > > > >>
> > > > > >> For reference:
> > > > > >> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> > > > > >>
> > > > > >>>
> > > > > >>> I keep asking people to come up with the a better name for this, and to
> > > > > >>> document what it actually means. Recently I've been think we should
> > > > > >>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> > > > > >>> avoid introducing yet another set of names for the same thing. We'd
> > > > > >>> still want to document things properly though.
> > > > > >>
> > > > > >> Another name it was suggested was "atomic amend", this feature basically
> > > > > >> allows userspace to complement an update previously sent (i.e. its in
> > > > > >> the queue and wasn't commited yet), it allows adding a plane update to
> > > > > >> the next commit. So what do you think in renaming it to "atomic amend"?
> > > > > >
> > > > > > Note that it doesn't seem to be what the code currently is doing. For
> > > > > > example, for cursor updates, it doesn't seem to be working on the
> > > > > > currently pending commit, but just directly issues an atomic async
> > > > > > update call to the planes. The code actually seems to fall back to a
> > > > > > normal sync commit, if there is an already pending commit touching the
> > > > > > same plane or including a modeset.
> > > > >
> > > > > It should fail as discussed at:
> > > > > https://patchwork.freedesktop.org/patch/243088/
> > > > >
> > > > > There was the following code inside the drm_atomic_helper_async_check()
> > > > > in the previous patch which would fallback to a normal commit if there
> > > > > isn't any pending commit to amend:
> > > > >
> > > > > +       if (!old_plane_state->commit)
> > > > > +               return -EINVAL;
> > > > >
> > > > > In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> > > > > this got removed, but which means that async update will be enabled
> > > > > anyway. So the following code is wrong:
> > > > >
> > > > > -       if (state->legacy_cursor_update)
> > > > > +       if (state->async_update || state->legacy_cursor_update)
> > > > >                 state->async_update = !drm_atomic_helper_async_check(dev, state);
> > > > >
> > > > > Does it make sense? If yes I'll fix this in the next version of the
> > > > > Atomic IOCTL patch (and also those two patches should be in the same
> > > > > series, I'll send them together next time).
> > > > >
> > > > > Thanks for pointing this out.
> > > > >
> > > > > Please let me know if you still don't agree on the name "atomic amend",
> > > > > or if I am missing something.
> > > >
> > > > I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> > > > need a way to commit the cursor plane asynchronously from other
> > > > commits any time the cursor changes its position or framebuffer. As
> > > > long as the new API allows that and the maintainers are fine with it,
> > > > I think I should be okay with it too.
> > >
> > > If this is just about the cursor, why is the current legacy cursor
> > > ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
> > > if we want to support having a normal atomic commit and a cursor
> > > update in the same atomic ioctl, coming up with reasonable semantics
> > > for that will be complicated.
> > >
> > > Pointer to code that uses this would be better ofc.
> >
> > I haven't followed this thread too closely, but we ended up needing to
> > add a fast patch for cursor updates to amdgpu's atomic support to
> > avoid stuttering issues.  Other drivers may end up being affected by
> > this as well.  See:
> > https://bugs.freedesktop.org/show_bug.cgi?id=106175
> > Unfortunately, the fast path requires some hacks to handle the ref
> > counting properly on cursor fbs:
> > https://patchwork.freedesktop.org/patch/266138/
> > https://patchwork.freedesktop.org/patch/268298/
> > It looks like gamma may need similar treatment:
> > https://bugs.freedesktop.org/show_bug.cgi?id=108917
> >
> > Alex
> >
> >
> > > -Daniel
> > >
> > > > Best regards,
> > > > Tomasz
> > > >
> > > > >
> > > > > Helen
> > > > >
> > > > > >
> > > > > > Best regards,
> > > > > > Tomasz
> > > > > >
> > > > > >> Or do you suggest another name? I am not familiar with vulkan terminology.
> > > > > >>
> > > > > >>
> > > > > >> Thanks
> > > > > >> Helen
> > > > > >>
> > > > > >>>
> > > > > >>>>
> > > > > >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > >>>> [prepared for upstream]
> > > > > >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > > >>>>
> > > > > >>>> ---
> > > > > >>>> Hi,
> > > > > >>>>
> > > > > >>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> > > > > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> > > > > >>>> figure that async_update exists.
> > > > > >>>>
> > > > > >>>> This was tested using a small program that exercises the uAPI for easy
> > > > > >>>> sanity testing. The program was created by Alexandros and modified by
> > > > > >>>> Enric to test the capability flag [2].
> > > > > >>>>
> > > > > >>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> > > > > >>>> the patch to update cursors asynchronously through atomic plus the patch
> > > > > >>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> > > > > >>>>
> > > > > >>>> To test, just build the program and use the --atomic flag to use the cursor
> > > > > >>>> plane with the ATOMIC_AMEND flag. E.g.
> > > > > >>>>
> > > > > >>>>   drm_cursor --atomic
> > > > > >>>>
> > > > > >>>> [1] https://patchwork.freedesktop.org/patch/243088/
> > > > > >>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> > > > > >>>>
> > > > > >>>> Thanks
> > > > > >>>> Helen
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> > > > > >>>>  include/uapi/drm/drm.h      |  1 +
> > > > > >>>>  2 files changed, 12 insertions(+)
> > > > > >>>>
> > > > > >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > > > >>>> index 94bd872d56c4..4a7e0f874171 100644
> > > > > >>>> --- a/drivers/gpu/drm/drm_ioctl.c
> > > > > >>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> > > > > >>>> @@ -31,6 +31,7 @@
> > > > > >>>>  #include <drm/drm_ioctl.h>
> > > > > >>>>  #include <drm/drmP.h>
> > > > > >>>>  #include <drm/drm_auth.h>
> > > > > >>>> +#include <drm/drm_modeset_helper_vtables.h>
> > > > > >>>>  #include "drm_legacy.h"
> > > > > >>>>  #include "drm_internal.h"
> > > > > >>>>  #include "drm_crtc_internal.h"
> > > > > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > > >>>>  {
> > > > > >>>>      struct drm_get_cap *req = data;
> > > > > >>>>      struct drm_crtc *crtc;
> > > > > >>>> +    struct drm_plane *plane;
> > > > > >>>>
> > > > > >>>>      req->value = 0;
> > > > > >>>>
> > > > > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > > >>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> > > > > >>>>              req->value = 1;
> > > > > >>>>              break;
> > > > > >>>> +    case DRM_CAP_ASYNC_UPDATE:
> > > > > >>>> +            req->value = 1;
> > > > > >>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > > > > >>>> +                    if (!plane->helper_private->atomic_async_update) {
> > > > > >>>> +                            req->value = 0;
> > > > > >>>> +                            break;
> > > > > >>>> +                    }
> > > > > >>>> +            }
> > > > > >>>> +            break;
> > > > > >>>>      default:
> > > > > >>>>              return -EINVAL;
> > > > > >>>>      }
> > > > > >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > > > >>>> index 300f336633f2..ff01540cbb1d 100644
> > > > > >>>> --- a/include/uapi/drm/drm.h
> > > > > >>>> +++ b/include/uapi/drm/drm.h
> > > > > >>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> > > > > >>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> > > > > >>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> > > > > >>>>  #define DRM_CAP_SYNCOBJ             0x13
> > > > > >>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> > > > > >>>>
> > > > > >>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> > > > > >>>>  struct drm_get_cap {
> > > > > >>>> --
> > > > > >>>> 2.19.1
> > > > > >>>>
> > > > > >>>> _______________________________________________
> > > > > >>>> dri-devel mailing list
> > > > > >>>> dri-devel@lists.freedesktop.org
> > > > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > >>>
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Daniel Vetter Dec. 20, 2018, 5:09 p.m. UTC | #10

On Thu, Dec 20, 2018 at 6:03 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> + Harry
>
> On Thu, Dec 20, 2018 at 11:54 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> > >
> > > + Nicholas
> > >
> > > On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
> > > > >
> > > > > Hi Helen,
> > > > >
> > > > > On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > > > >
> > > > > > Hi Tomasz,
> > > > > >
> > > > > > On 12/13/18 2:02 AM, Tomasz Figa wrote:
> > > > > > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > > > > >>
> > > > > > >> Hi Ville
> > > > > > >>
> > > > > > >> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> > > > > > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> > > > > > >>>> Allow userspace to identify if the driver supports async update.
> > > > > > >>>
> > > > > > >>> And what exactly is an "async update"?
> > > > > > >>
> > > > > > >> I agree we are lacking docs on this, I'll send in the next version as
> > > > > > >> soon as we agree on a name (please see below).
> > > > > > >>
> > > > > > >> For reference:
> > > > > > >> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> > > > > > >>
> > > > > > >>>
> > > > > > >>> I keep asking people to come up with the a better name for this, and to
> > > > > > >>> document what it actually means. Recently I've been think we should
> > > > > > >>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> > > > > > >>> avoid introducing yet another set of names for the same thing. We'd
> > > > > > >>> still want to document things properly though.
> > > > > > >>
> > > > > > >> Another name it was suggested was "atomic amend", this feature basically
> > > > > > >> allows userspace to complement an update previously sent (i.e. its in
> > > > > > >> the queue and wasn't commited yet), it allows adding a plane update to
> > > > > > >> the next commit. So what do you think in renaming it to "atomic amend"?
> > > > > > >
> > > > > > > Note that it doesn't seem to be what the code currently is doing. For
> > > > > > > example, for cursor updates, it doesn't seem to be working on the
> > > > > > > currently pending commit, but just directly issues an atomic async
> > > > > > > update call to the planes. The code actually seems to fall back to a
> > > > > > > normal sync commit, if there is an already pending commit touching the
> > > > > > > same plane or including a modeset.
> > > > > >
> > > > > > It should fail as discussed at:
> > > > > > https://patchwork.freedesktop.org/patch/243088/
> > > > > >
> > > > > > There was the following code inside the drm_atomic_helper_async_check()
> > > > > > in the previous patch which would fallback to a normal commit if there
> > > > > > isn't any pending commit to amend:
> > > > > >
> > > > > > +       if (!old_plane_state->commit)
> > > > > > +               return -EINVAL;
> > > > > >
> > > > > > In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> > > > > > this got removed, but which means that async update will be enabled
> > > > > > anyway. So the following code is wrong:
> > > > > >
> > > > > > -       if (state->legacy_cursor_update)
> > > > > > +       if (state->async_update || state->legacy_cursor_update)
> > > > > >                 state->async_update = !drm_atomic_helper_async_check(dev, state);
> > > > > >
> > > > > > Does it make sense? If yes I'll fix this in the next version of the
> > > > > > Atomic IOCTL patch (and also those two patches should be in the same
> > > > > > series, I'll send them together next time).
> > > > > >
> > > > > > Thanks for pointing this out.
> > > > > >
> > > > > > Please let me know if you still don't agree on the name "atomic amend",
> > > > > > or if I am missing something.
> > > > >
> > > > > I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> > > > > need a way to commit the cursor plane asynchronously from other
> > > > > commits any time the cursor changes its position or framebuffer. As
> > > > > long as the new API allows that and the maintainers are fine with it,
> > > > > I think I should be okay with it too.
> > > >
> > > > If this is just about the cursor, why is the current legacy cursor
> > > > ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
> > > > if we want to support having a normal atomic commit and a cursor
> > > > update in the same atomic ioctl, coming up with reasonable semantics
> > > > for that will be complicated.
> > > >
> > > > Pointer to code that uses this would be better ofc.
> > >
> > > I haven't followed this thread too closely, but we ended up needing to
> > > add a fast patch for cursor updates to amdgpu's atomic support to
> > > avoid stuttering issues.  Other drivers may end up being affected by
> > > this as well.  See:
> > > https://bugs.freedesktop.org/show_bug.cgi?id=106175
> > > Unfortunately, the fast path requires some hacks to handle the ref
> > > counting properly on cursor fbs:
> > > https://patchwork.freedesktop.org/patch/266138/
> > > https://patchwork.freedesktop.org/patch/268298/
> > > It looks like gamma may need similar treatment:
> > > https://bugs.freedesktop.org/show_bug.cgi?id=108917
> >
> > Can we get these patches cc'ed to dri-devel so that there's some
> > common solution? Everyone hacking legacy_cursor_update hacks on their
> > own doesn't really work well. Or would at least give some visibility
> > into what's all going on.
>
> Agreed.

Bit more detail: The legacy_cursor_update hacks all over is probably
the worst part of atomic, and everyone seems unhappy with it. Except
all efforts to address it fall short by a lot. I think Gustavo from
Collabora once had a patch series, but it only ever got merged
partially, and now we're back with a slightly different pick of color
it seems. Hence why I'm somewhat grumpy on this here.

> > Not sure about the gamma thing since we had opposite bugs on i915
> > about gamma not being vsynced and tearing terribly. Cursor is special
> > since it tends to be too small to notice tearing.
>
> Our cursor hw (and possibly gamma as well Nicholas?  Harry?) is double
> buffered, so we can update it any time for the most part and the
> changes won't take affect until the next vupdate period.

Hm, I guess we could make the gamma update optionally async, and let
drivers deal. The issue is that the current async helper stuff won't
cope with gamma updates (it's aimed at plane updates only, not at crtc
property updates). Or we get userspace to do proper atomic updates. Or
we do some faking in the kernel, e.g. waiting with the gamma update
until the next atomic update happens. But that kinda breaks
->atomic_check.
-Daniel

>
> Alex
>
> >
> > Thanks, Daniel
> >
> > > Alex
> > >
> > >
> > > > -Daniel
> > > >
> > > > > Best regards,
> > > > > Tomasz
> > > > >
> > > > > >
> > > > > > Helen
> > > > > >
> > > > > > >
> > > > > > > Best regards,
> > > > > > > Tomasz
> > > > > > >
> > > > > > >> Or do you suggest another name? I am not familiar with vulkan terminology.
> > > > > > >>
> > > > > > >>
> > > > > > >> Thanks
> > > > > > >> Helen
> > > > > > >>
> > > > > > >>>
> > > > > > >>>>
> > > > > > >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > >>>> [prepared for upstream]
> > > > > > >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > > > >>>>
> > > > > > >>>> ---
> > > > > > >>>> Hi,
> > > > > > >>>>
> > > > > > >>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> > > > > > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> > > > > > >>>> figure that async_update exists.
> > > > > > >>>>
> > > > > > >>>> This was tested using a small program that exercises the uAPI for easy
> > > > > > >>>> sanity testing. The program was created by Alexandros and modified by
> > > > > > >>>> Enric to test the capability flag [2].
> > > > > > >>>>
> > > > > > >>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> > > > > > >>>> the patch to update cursors asynchronously through atomic plus the patch
> > > > > > >>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> > > > > > >>>>
> > > > > > >>>> To test, just build the program and use the --atomic flag to use the cursor
> > > > > > >>>> plane with the ATOMIC_AMEND flag. E.g.
> > > > > > >>>>
> > > > > > >>>>   drm_cursor --atomic
> > > > > > >>>>
> > > > > > >>>> [1] https://patchwork.freedesktop.org/patch/243088/
> > > > > > >>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> > > > > > >>>>
> > > > > > >>>> Thanks
> > > > > > >>>> Helen
> > > > > > >>>>
> > > > > > >>>>
> > > > > > >>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> > > > > > >>>>  include/uapi/drm/drm.h      |  1 +
> > > > > > >>>>  2 files changed, 12 insertions(+)
> > > > > > >>>>
> > > > > > >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > > > > >>>> index 94bd872d56c4..4a7e0f874171 100644
> > > > > > >>>> --- a/drivers/gpu/drm/drm_ioctl.c
> > > > > > >>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> > > > > > >>>> @@ -31,6 +31,7 @@
> > > > > > >>>>  #include <drm/drm_ioctl.h>
> > > > > > >>>>  #include <drm/drmP.h>
> > > > > > >>>>  #include <drm/drm_auth.h>
> > > > > > >>>> +#include <drm/drm_modeset_helper_vtables.h>
> > > > > > >>>>  #include "drm_legacy.h"
> > > > > > >>>>  #include "drm_internal.h"
> > > > > > >>>>  #include "drm_crtc_internal.h"
> > > > > > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > > > >>>>  {
> > > > > > >>>>      struct drm_get_cap *req = data;
> > > > > > >>>>      struct drm_crtc *crtc;
> > > > > > >>>> +    struct drm_plane *plane;
> > > > > > >>>>
> > > > > > >>>>      req->value = 0;
> > > > > > >>>>
> > > > > > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > > > >>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> > > > > > >>>>              req->value = 1;
> > > > > > >>>>              break;
> > > > > > >>>> +    case DRM_CAP_ASYNC_UPDATE:
> > > > > > >>>> +            req->value = 1;
> > > > > > >>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > > > > > >>>> +                    if (!plane->helper_private->atomic_async_update) {
> > > > > > >>>> +                            req->value = 0;
> > > > > > >>>> +                            break;
> > > > > > >>>> +                    }
> > > > > > >>>> +            }
> > > > > > >>>> +            break;
> > > > > > >>>>      default:
> > > > > > >>>>              return -EINVAL;
> > > > > > >>>>      }
> > > > > > >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > > > > >>>> index 300f336633f2..ff01540cbb1d 100644
> > > > > > >>>> --- a/include/uapi/drm/drm.h
> > > > > > >>>> +++ b/include/uapi/drm/drm.h
> > > > > > >>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> > > > > > >>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> > > > > > >>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> > > > > > >>>>  #define DRM_CAP_SYNCOBJ             0x13
> > > > > > >>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> > > > > > >>>>
> > > > > > >>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> > > > > > >>>>  struct drm_get_cap {
> > > > > > >>>> --
> > > > > > >>>> 2.19.1
> > > > > > >>>>
> > > > > > >>>> _______________________________________________
> > > > > > >>>> dri-devel mailing list
> > > > > > >>>> dri-devel@lists.freedesktop.org
> > > > > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > > >>>
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> > On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> > >
> > > + Nicholas
> > >
> > > On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
> > > > >
> > > > > Hi Helen,
> > > > >
> > > > > On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > > > >
> > > > > > Hi Tomasz,
> > > > > >
> > > > > > On 12/13/18 2:02 AM, Tomasz Figa wrote:
> > > > > > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > > > > >>
> > > > > > >> Hi Ville
> > > > > > >>
> > > > > > >> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> > > > > > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> > > > > > >>>> Allow userspace to identify if the driver supports async update.
> > > > > > >>>
> > > > > > >>> And what exactly is an "async update"?
> > > > > > >>
> > > > > > >> I agree we are lacking docs on this, I'll send in the next version as
> > > > > > >> soon as we agree on a name (please see below).
> > > > > > >>
> > > > > > >> For reference:
> > > > > > >> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> > > > > > >>
> > > > > > >>>
> > > > > > >>> I keep asking people to come up with the a better name for this, and to
> > > > > > >>> document what it actually means. Recently I've been think we should
> > > > > > >>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> > > > > > >>> avoid introducing yet another set of names for the same thing. We'd
> > > > > > >>> still want to document things properly though.
> > > > > > >>
> > > > > > >> Another name it was suggested was "atomic amend", this feature basically
> > > > > > >> allows userspace to complement an update previously sent (i.e. its in
> > > > > > >> the queue and wasn't commited yet), it allows adding a plane update to
> > > > > > >> the next commit. So what do you think in renaming it to "atomic amend"?
> > > > > > >
> > > > > > > Note that it doesn't seem to be what the code currently is doing. For
> > > > > > > example, for cursor updates, it doesn't seem to be working on the
> > > > > > > currently pending commit, but just directly issues an atomic async
> > > > > > > update call to the planes. The code actually seems to fall back to a
> > > > > > > normal sync commit, if there is an already pending commit touching the
> > > > > > > same plane or including a modeset.
> > > > > >
> > > > > > It should fail as discussed at:
> > > > > > https://patchwork.freedesktop.org/patch/243088/
> > > > > >
> > > > > > There was the following code inside the drm_atomic_helper_async_check()
> > > > > > in the previous patch which would fallback to a normal commit if there
> > > > > > isn't any pending commit to amend:
> > > > > >
> > > > > > +       if (!old_plane_state->commit)
> > > > > > +               return -EINVAL;
> > > > > >
> > > > > > In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> > > > > > this got removed, but which means that async update will be enabled
> > > > > > anyway. So the following code is wrong:
> > > > > >
> > > > > > -       if (state->legacy_cursor_update)
> > > > > > +       if (state->async_update || state->legacy_cursor_update)
> > > > > >                 state->async_update = !drm_atomic_helper_async_check(dev, state);
> > > > > >
> > > > > > Does it make sense? If yes I'll fix this in the next version of the
> > > > > > Atomic IOCTL patch (and also those two patches should be in the same
> > > > > > series, I'll send them together next time).
> > > > > >
> > > > > > Thanks for pointing this out.
> > > > > >
> > > > > > Please let me know if you still don't agree on the name "atomic amend",
> > > > > > or if I am missing something.
> > > > >
> > > > > I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> > > > > need a way to commit the cursor plane asynchronously from other
> > > > > commits any time the cursor changes its position or framebuffer. As
> > > > > long as the new API allows that and the maintainers are fine with it,
> > > > > I think I should be okay with it too.
> > > >
> > > > If this is just about the cursor, why is the current legacy cursor
> > > > ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
> > > > if we want to support having a normal atomic commit and a cursor
> > > > update in the same atomic ioctl, coming up with reasonable semantics
> > > > for that will be complicated.
> > > >
> > > > Pointer to code that uses this would be better ofc.
> > >
> > > I haven't followed this thread too closely, but we ended up needing to
> > > add a fast patch for cursor updates to amdgpu's atomic support to
> > > avoid stuttering issues.  Other drivers may end up being affected by
> > > this as well.  See:
> > > https://bugs.freedesktop.org/show_bug.cgi?id=106175
> > > Unfortunately, the fast path requires some hacks to handle the ref
> > > counting properly on cursor fbs:
> > > https://patchwork.freedesktop.org/patch/266138/
> > > https://patchwork.freedesktop.org/patch/268298/
> > > It looks like gamma may need similar treatment:
> > > https://bugs.freedesktop.org/show_bug.cgi?id=108917
> > >
> > > Alex
> > >
> > >
> > > > -Daniel
> > > >
> > > > > Best regards,
> > > > > Tomasz
> > > > >
> > > > > >
> > > > > > Helen
> > > > > >
> > > > > > >
> > > > > > > Best regards,
> > > > > > > Tomasz
> > > > > > >
> > > > > > >> Or do you suggest another name? I am not familiar with vulkan terminology.
> > > > > > >>
> > > > > > >>
> > > > > > >> Thanks
> > > > > > >> Helen
> > > > > > >>
> > > > > > >>>
> > > > > > >>>>
> > > > > > >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > >>>> [prepared for upstream]
> > > > > > >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > > > >>>>
> > > > > > >>>> ---
> > > > > > >>>> Hi,
> > > > > > >>>>
> > > > > > >>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> > > > > > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> > > > > > >>>> figure that async_update exists.
> > > > > > >>>>
> > > > > > >>>> This was tested using a small program that exercises the uAPI for easy
> > > > > > >>>> sanity testing. The program was created by Alexandros and modified by
> > > > > > >>>> Enric to test the capability flag [2].
> > > > > > >>>>
> > > > > > >>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> > > > > > >>>> the patch to update cursors asynchronously through atomic plus the patch
> > > > > > >>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> > > > > > >>>>
> > > > > > >>>> To test, just build the program and use the --atomic flag to use the cursor
> > > > > > >>>> plane with the ATOMIC_AMEND flag. E.g.
> > > > > > >>>>
> > > > > > >>>>   drm_cursor --atomic
> > > > > > >>>>
> > > > > > >>>> [1] https://patchwork.freedesktop.org/patch/243088/
> > > > > > >>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> > > > > > >>>>
> > > > > > >>>> Thanks
> > > > > > >>>> Helen
> > > > > > >>>>
> > > > > > >>>>
> > > > > > >>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> > > > > > >>>>  include/uapi/drm/drm.h      |  1 +
> > > > > > >>>>  2 files changed, 12 insertions(+)
> > > > > > >>>>
> > > > > > >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > > > > >>>> index 94bd872d56c4..4a7e0f874171 100644
> > > > > > >>>> --- a/drivers/gpu/drm/drm_ioctl.c
> > > > > > >>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> > > > > > >>>> @@ -31,6 +31,7 @@
> > > > > > >>>>  #include <drm/drm_ioctl.h>
> > > > > > >>>>  #include <drm/drmP.h>
> > > > > > >>>>  #include <drm/drm_auth.h>
> > > > > > >>>> +#include <drm/drm_modeset_helper_vtables.h>
> > > > > > >>>>  #include "drm_legacy.h"
> > > > > > >>>>  #include "drm_internal.h"
> > > > > > >>>>  #include "drm_crtc_internal.h"
> > > > > > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > > > >>>>  {
> > > > > > >>>>      struct drm_get_cap *req = data;
> > > > > > >>>>      struct drm_crtc *crtc;
> > > > > > >>>> +    struct drm_plane *plane;
> > > > > > >>>>
> > > > > > >>>>      req->value = 0;
> > > > > > >>>>
> > > > > > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > > > >>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> > > > > > >>>>              req->value = 1;
> > > > > > >>>>              break;
> > > > > > >>>> +    case DRM_CAP_ASYNC_UPDATE:
> > > > > > >>>> +            req->value = 1;
> > > > > > >>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > > > > > >>>> +                    if (!plane->helper_private->atomic_async_update) {
> > > > > > >>>> +                            req->value = 0;
> > > > > > >>>> +                            break;
> > > > > > >>>> +                    }
> > > > > > >>>> +            }
> > > > > > >>>> +            break;
> > > > > > >>>>      default:
> > > > > > >>>>              return -EINVAL;
> > > > > > >>>>      }
> > > > > > >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > > > > >>>> index 300f336633f2..ff01540cbb1d 100644
> > > > > > >>>> --- a/include/uapi/drm/drm.h
> > > > > > >>>> +++ b/include/uapi/drm/drm.h
> > > > > > >>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> > > > > > >>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> > > > > > >>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> > > > > > >>>>  #define DRM_CAP_SYNCOBJ             0x13
> > > > > > >>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> > > > > > >>>>
> > > > > > >>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> > > > > > >>>>  struct drm_get_cap {
> > > > > > >>>> --
> > > > > > >>>> 2.19.1
> > > > > > >>>>
> > > > > > >>>> _______________________________________________
> > > > > > >>>> dri-devel mailing list
> > > > > > >>>> dri-devel@lists.freedesktop.org
> > > > > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > > >>>
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Michel Dänzer Dec. 20, 2018, 5:16 p.m. UTC | #11

On 2018-12-20 6:09 p.m., Daniel Vetter wrote:
> On Thu, Dec 20, 2018 at 6:03 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>> On Thu, Dec 20, 2018 at 11:54 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> 
>>> Not sure about the gamma thing since we had opposite bugs on i915
>>> about gamma not being vsynced and tearing terribly. Cursor is special
>>> since it tends to be too small to notice tearing.
>>
>> Our cursor hw (and possibly gamma as well Nicholas?  Harry?) is double
>> buffered, so we can update it any time for the most part and the
>> changes won't take affect until the next vupdate period.
> 
> Hm, I guess we could make the gamma update optionally async, and let
> drivers deal. The issue is that the current async helper stuff won't
> cope with gamma updates (it's aimed at plane updates only, not at crtc
> property updates). Or we get userspace to do proper atomic updates. Or
> we do some faking in the kernel, e.g. waiting with the gamma update
> until the next atomic update happens. But that kinda breaks
> ->atomic_check.

Would it be possible to merge gamma changes into a pending commit, if
there is one, and create a new commit otherwise?

Otherwise the atomic API just moves this same problem to be solved by
userspace.

Daniel Vetter Dec. 20, 2018, 5:24 p.m. UTC | #12

On Thu, Dec 20, 2018 at 6:16 PM Michel Dänzer <michel@daenzer.net> wrote:
>
> On 2018-12-20 6:09 p.m., Daniel Vetter wrote:
> > On Thu, Dec 20, 2018 at 6:03 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >> On Thu, Dec 20, 2018 at 11:54 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> >>> Not sure about the gamma thing since we had opposite bugs on i915
> >>> about gamma not being vsynced and tearing terribly. Cursor is special
> >>> since it tends to be too small to notice tearing.
> >>
> >> Our cursor hw (and possibly gamma as well Nicholas?  Harry?) is double
> >> buffered, so we can update it any time for the most part and the
> >> changes won't take affect until the next vupdate period.
> >
> > Hm, I guess we could make the gamma update optionally async, and let
> > drivers deal. The issue is that the current async helper stuff won't
> > cope with gamma updates (it's aimed at plane updates only, not at crtc
> > property updates). Or we get userspace to do proper atomic updates. Or
> > we do some faking in the kernel, e.g. waiting with the gamma update
> > until the next atomic update happens. But that kinda breaks
> > ->atomic_check.
>
> Would it be possible to merge gamma changes into a pending commit, if
> there is one, and create a new commit otherwise?
>
> Otherwise the atomic API just moves this same problem to be solved by
> userspace.

I guess possible, depending upon how much you want to type. The problem is:
- stopping the current commit before it gets too far
- without causing stutter in itself (we don't want a stream of cursor
updates to livelock pageflips)
- re-running atomic_check on the new combined commit (which I guess
means you somehow need a clean starting state again and apply both
updates, which currently isn't possible)
- push it out as a commit

Alternatively is some fudging in the backend like we do now have some
support for for async plane updates. We'd need the full trickery with
a dedicated async_update_check though, at least on i915 we need to (in
some cases) make sure the gamma ramp is monotonic and stuff like that.

Or we just give up on commone semantics on this and have legacy gamma
updates bypass atomic in dc somehow (which would be the unstructured
version of the above).
-Daniel

Kazlauskas, Nicholas Dec. 20, 2018, 5:38 p.m. UTC | #13

On 12/20/18 12:09 PM, Daniel Vetter wrote:
> On Thu, Dec 20, 2018 at 6:03 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>>
>> + Harry
>>
>> On Thu, Dec 20, 2018 at 11:54 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>>
>>> On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>>>>
>>>> + Nicholas
>>>>
>>>> On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>>
>>>>> On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
>>>>>>
>>>>>> Hi Helen,
>>>>>>
>>>>>> On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
>>>>>>>
>>>>>>> Hi Tomasz,
>>>>>>>
>>>>>>> On 12/13/18 2:02 AM, Tomasz Figa wrote:
>>>>>>>> On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
>>>>>>>>>
>>>>>>>>> Hi Ville
>>>>>>>>>
>>>>>>>>> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
>>>>>>>>>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
>>>>>>>>>>> Allow userspace to identify if the driver supports async update.
>>>>>>>>>>
>>>>>>>>>> And what exactly is an "async update"?
>>>>>>>>>
>>>>>>>>> I agree we are lacking docs on this, I'll send in the next version as
>>>>>>>>> soon as we agree on a name (please see below).
>>>>>>>>>
>>>>>>>>> For reference:
>>>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I keep asking people to come up with the a better name for this, and to
>>>>>>>>>> document what it actually means. Recently I've been think we should
>>>>>>>>>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
>>>>>>>>>> avoid introducing yet another set of names for the same thing. We'd
>>>>>>>>>> still want to document things properly though.
>>>>>>>>>
>>>>>>>>> Another name it was suggested was "atomic amend", this feature basically
>>>>>>>>> allows userspace to complement an update previously sent (i.e. its in
>>>>>>>>> the queue and wasn't commited yet), it allows adding a plane update to
>>>>>>>>> the next commit. So what do you think in renaming it to "atomic amend"?
>>>>>>>>
>>>>>>>> Note that it doesn't seem to be what the code currently is doing. For
>>>>>>>> example, for cursor updates, it doesn't seem to be working on the
>>>>>>>> currently pending commit, but just directly issues an atomic async
>>>>>>>> update call to the planes. The code actually seems to fall back to a
>>>>>>>> normal sync commit, if there is an already pending commit touching the
>>>>>>>> same plane or including a modeset.
>>>>>>>
>>>>>>> It should fail as discussed at:
>>>>>>> https://patchwork.freedesktop.org/patch/243088/
>>>>>>>
>>>>>>> There was the following code inside the drm_atomic_helper_async_check()
>>>>>>> in the previous patch which would fallback to a normal commit if there
>>>>>>> isn't any pending commit to amend:
>>>>>>>
>>>>>>> +       if (!old_plane_state->commit)
>>>>>>> +               return -EINVAL;
>>>>>>>
>>>>>>> In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
>>>>>>> this got removed, but which means that async update will be enabled
>>>>>>> anyway. So the following code is wrong:
>>>>>>>
>>>>>>> -       if (state->legacy_cursor_update)
>>>>>>> +       if (state->async_update || state->legacy_cursor_update)
>>>>>>>                  state->async_update = !drm_atomic_helper_async_check(dev, state);
>>>>>>>
>>>>>>> Does it make sense? If yes I'll fix this in the next version of the
>>>>>>> Atomic IOCTL patch (and also those two patches should be in the same
>>>>>>> series, I'll send them together next time).
>>>>>>>
>>>>>>> Thanks for pointing this out.
>>>>>>>
>>>>>>> Please let me know if you still don't agree on the name "atomic amend",
>>>>>>> or if I am missing something.
>>>>>>
>>>>>> I'll defer it to the DRM maintainers. From Chrome OS perspective, we
>>>>>> need a way to commit the cursor plane asynchronously from other
>>>>>> commits any time the cursor changes its position or framebuffer. As
>>>>>> long as the new API allows that and the maintainers are fine with it,
>>>>>> I think I should be okay with it too.
>>>>>
>>>>> If this is just about the cursor, why is the current legacy cursor
>>>>> ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
>>>>> if we want to support having a normal atomic commit and a cursor
>>>>> update in the same atomic ioctl, coming up with reasonable semantics
>>>>> for that will be complicated.
>>>>>
>>>>> Pointer to code that uses this would be better ofc.
>>>>
>>>> I haven't followed this thread too closely, but we ended up needing to
>>>> add a fast patch for cursor updates to amdgpu's atomic support to
>>>> avoid stuttering issues.  Other drivers may end up being affected by
>>>> this as well.  See:
>>>> https://bugs.freedesktop.org/show_bug.cgi?id=106175
>>>> Unfortunately, the fast path requires some hacks to handle the ref
>>>> counting properly on cursor fbs:
>>>> https://patchwork.freedesktop.org/patch/266138/
>>>> https://patchwork.freedesktop.org/patch/268298/
>>>> It looks like gamma may need similar treatment:
>>>> https://bugs.freedesktop.org/show_bug.cgi?id=108917
>>>
>>> Can we get these patches cc'ed to dri-devel so that there's some
>>> common solution? Everyone hacking legacy_cursor_update hacks on their
>>> own doesn't really work well. Or would at least give some visibility
>>> into what's all going on.
>>
>> Agreed.
> 
> Bit more detail: The legacy_cursor_update hacks all over is probably
> the worst part of atomic, and everyone seems unhappy with it. Except
> all efforts to address it fall short by a lot. I think Gustavo from
> Collabora once had a patch series, but it only ever got merged
> partially, and now we're back with a slightly different pick of color
> it seems. Hence why I'm somewhat grumpy on this here.

I was planning on submitting a patch that added the old->fb == new->fb 
check for the async check in drm helpers but haven't gotten around to it 
yet. Async update behavior always prepares the new plane fb and cleans 
up the new plane fb, so you end up with a sequence like the following:

- Fast update, fb1 prepare, fb1 cleanup
- Fast update, fb2 prepare, fb2 cleanup
- Slow update, fb1 prepare, fb2 cleanup
- Fast update, fb2 cleanup -> double cleanup, potential use after free

The only driver that still expects to be doing fb changes in their fast 
path is vc4 from what I can tell, but they'd likely run into an issue 
with interleaving fbs as well.

The fast path on i915 skips the async helpers and implements this a lot 
better (even though it ends up grabbing the struct mutex lock) since it 
always calls cleanup on the old plane state like the slow path does. It 
also swaps the old plane state with the new one.

But since async_update is in place it can't do that (the old plane state 
is the existing plane state). There's not a really good way of emulating 
i915 here without changing the whole concept of async update.

> 
>>> Not sure about the gamma thing since we had opposite bugs on i915
>>> about gamma not being vsynced and tearing terribly. Cursor is special
>>> since it tends to be too small to notice tearing.
>>
>> Our cursor hw (and possibly gamma as well Nicholas?  Harry?) is double
>> buffered, so we can update it any time for the most part and the
>> changes won't take affect until the next vupdate period.

I haven't really investigated too much into the gamma stuttering issue, 
but I think it's similar to the cursor update - a high volume of atomic 
updates that ends up skipping over a vblank or two.

> 
> Hm, I guess we could make the gamma update optionally async, and let
> drivers deal. The issue is that the current async helper stuff won't
> cope with gamma updates (it's aimed at plane updates only, not at crtc
> property updates). Or we get userspace to do proper atomic updates. Or
> we do some faking in the kernel, e.g. waiting with the gamma update
> until the next atomic update happens. But that kinda breaks
> ->atomic_check.
> -Daniel

The in-place update method can work for us for gamma updates I believe, 
but there's likely a lot more drivers would have to be careful about 
with in-place updates on the CRTC state.

A full state swap on the object itself would probably be best for 
tackling it this way. It's what I'd like to happen for the existing 
plane API at least (and which I think is fine since the plane is locked 
anyway).

Nicholas Kazlauskas

> 
>>
>> Alex
>>
>>>
>>> Thanks, Daniel
>>>
>>>> Alex
>>>>
>>>>
>>>>> -Daniel
>>>>>
>>>>>> Best regards,
>>>>>> Tomasz
>>>>>>
>>>>>>>
>>>>>>> Helen
>>>>>>>
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Tomasz
>>>>>>>>
>>>>>>>>> Or do you suggest another name? I am not familiar with vulkan terminology.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Helen
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>>>>>>>>> [prepared for upstream]
>>>>>>>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
>>>>>>>>>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
>>>>>>>>>>> figure that async_update exists.
>>>>>>>>>>>
>>>>>>>>>>> This was tested using a small program that exercises the uAPI for easy
>>>>>>>>>>> sanity testing. The program was created by Alexandros and modified by
>>>>>>>>>>> Enric to test the capability flag [2].
>>>>>>>>>>>
>>>>>>>>>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
>>>>>>>>>>> the patch to update cursors asynchronously through atomic plus the patch
>>>>>>>>>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
>>>>>>>>>>>
>>>>>>>>>>> To test, just build the program and use the --atomic flag to use the cursor
>>>>>>>>>>> plane with the ATOMIC_AMEND flag. E.g.
>>>>>>>>>>>
>>>>>>>>>>>    drm_cursor --atomic
>>>>>>>>>>>
>>>>>>>>>>> [1] https://patchwork.freedesktop.org/patch/243088/
>>>>>>>>>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> Helen
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>   drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
>>>>>>>>>>>   include/uapi/drm/drm.h      |  1 +
>>>>>>>>>>>   2 files changed, 12 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>>>>>>>>>>> index 94bd872d56c4..4a7e0f874171 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/drm_ioctl.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/drm_ioctl.c
>>>>>>>>>>> @@ -31,6 +31,7 @@
>>>>>>>>>>>   #include <drm/drm_ioctl.h>
>>>>>>>>>>>   #include <drm/drmP.h>
>>>>>>>>>>>   #include <drm/drm_auth.h>
>>>>>>>>>>> +#include <drm/drm_modeset_helper_vtables.h>
>>>>>>>>>>>   #include "drm_legacy.h"
>>>>>>>>>>>   #include "drm_internal.h"
>>>>>>>>>>>   #include "drm_crtc_internal.h"
>>>>>>>>>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>>>>>>>>>>>   {
>>>>>>>>>>>       struct drm_get_cap *req = data;
>>>>>>>>>>>       struct drm_crtc *crtc;
>>>>>>>>>>> +    struct drm_plane *plane;
>>>>>>>>>>>
>>>>>>>>>>>       req->value = 0;
>>>>>>>>>>>
>>>>>>>>>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>>>>>>>>>>>       case DRM_CAP_CRTC_IN_VBLANK_EVENT:
>>>>>>>>>>>               req->value = 1;
>>>>>>>>>>>               break;
>>>>>>>>>>> +    case DRM_CAP_ASYNC_UPDATE:
>>>>>>>>>>> +            req->value = 1;
>>>>>>>>>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
>>>>>>>>>>> +                    if (!plane->helper_private->atomic_async_update) {
>>>>>>>>>>> +                            req->value = 0;
>>>>>>>>>>> +                            break;
>>>>>>>>>>> +                    }
>>>>>>>>>>> +            }
>>>>>>>>>>> +            break;
>>>>>>>>>>>       default:
>>>>>>>>>>>               return -EINVAL;
>>>>>>>>>>>       }
>>>>>>>>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>>>>>>>>>> index 300f336633f2..ff01540cbb1d 100644
>>>>>>>>>>> --- a/include/uapi/drm/drm.h
>>>>>>>>>>> +++ b/include/uapi/drm/drm.h
>>>>>>>>>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
>>>>>>>>>>>   #define DRM_CAP_PAGE_FLIP_TARGET    0x11
>>>>>>>>>>>   #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
>>>>>>>>>>>   #define DRM_CAP_SYNCOBJ             0x13
>>>>>>>>>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
>>>>>>>>>>>
>>>>>>>>>>>   /** DRM_IOCTL_GET_CAP ioctl argument type */
>>>>>>>>>>>   struct drm_get_cap {
>>>>>>>>>>> --
>>>>>>>>>>> 2.19.1
>>>>>>>>>>>
>>>>>>>>>>> _______________________________________________
>>>>>>>>>>> dri-devel mailing list
>>>>>>>>>>> dri-devel@lists.freedesktop.org
>>>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>>>>>>
>>>>>> _______________________________________________
>>>>>> dri-devel mailing list
>>>>>> dri-devel@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Daniel Vetter
>>>>> Software Engineer, Intel Corporation
>>>>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>>
>>>
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>>
>>> On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>>>>
>>>> + Nicholas
>>>>
>>>> On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>>
>>>>> On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
>>>>>>
>>>>>> Hi Helen,
>>>>>>
>>>>>> On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
>>>>>>>
>>>>>>> Hi Tomasz,
>>>>>>>
>>>>>>> On 12/13/18 2:02 AM, Tomasz Figa wrote:
>>>>>>>> On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
>>>>>>>>>
>>>>>>>>> Hi Ville
>>>>>>>>>
>>>>>>>>> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
>>>>>>>>>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
>>>>>>>>>>> Allow userspace to identify if the driver supports async update.
>>>>>>>>>>
>>>>>>>>>> And what exactly is an "async update"?
>>>>>>>>>
>>>>>>>>> I agree we are lacking docs on this, I'll send in the next version as
>>>>>>>>> soon as we agree on a name (please see below).
>>>>>>>>>
>>>>>>>>> For reference:
>>>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I keep asking people to come up with the a better name for this, and to
>>>>>>>>>> document what it actually means. Recently I've been think we should
>>>>>>>>>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
>>>>>>>>>> avoid introducing yet another set of names for the same thing. We'd
>>>>>>>>>> still want to document things properly though.
>>>>>>>>>
>>>>>>>>> Another name it was suggested was "atomic amend", this feature basically
>>>>>>>>> allows userspace to complement an update previously sent (i.e. its in
>>>>>>>>> the queue and wasn't commited yet), it allows adding a plane update to
>>>>>>>>> the next commit. So what do you think in renaming it to "atomic amend"?
>>>>>>>>
>>>>>>>> Note that it doesn't seem to be what the code currently is doing. For
>>>>>>>> example, for cursor updates, it doesn't seem to be working on the
>>>>>>>> currently pending commit, but just directly issues an atomic async
>>>>>>>> update call to the planes. The code actually seems to fall back to a
>>>>>>>> normal sync commit, if there is an already pending commit touching the
>>>>>>>> same plane or including a modeset.
>>>>>>>
>>>>>>> It should fail as discussed at:
>>>>>>> https://patchwork.freedesktop.org/patch/243088/
>>>>>>>
>>>>>>> There was the following code inside the drm_atomic_helper_async_check()
>>>>>>> in the previous patch which would fallback to a normal commit if there
>>>>>>> isn't any pending commit to amend:
>>>>>>>
>>>>>>> +       if (!old_plane_state->commit)
>>>>>>> +               return -EINVAL;
>>>>>>>
>>>>>>> In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
>>>>>>> this got removed, but which means that async update will be enabled
>>>>>>> anyway. So the following code is wrong:
>>>>>>>
>>>>>>> -       if (state->legacy_cursor_update)
>>>>>>> +       if (state->async_update || state->legacy_cursor_update)
>>>>>>>                  state->async_update = !drm_atomic_helper_async_check(dev, state);
>>>>>>>
>>>>>>> Does it make sense? If yes I'll fix this in the next version of the
>>>>>>> Atomic IOCTL patch (and also those two patches should be in the same
>>>>>>> series, I'll send them together next time).
>>>>>>>
>>>>>>> Thanks for pointing this out.
>>>>>>>
>>>>>>> Please let me know if you still don't agree on the name "atomic amend",
>>>>>>> or if I am missing something.
>>>>>>
>>>>>> I'll defer it to the DRM maintainers. From Chrome OS perspective, we
>>>>>> need a way to commit the cursor plane asynchronously from other
>>>>>> commits any time the cursor changes its position or framebuffer. As
>>>>>> long as the new API allows that and the maintainers are fine with it,
>>>>>> I think I should be okay with it too.
>>>>>
>>>>> If this is just about the cursor, why is the current legacy cursor
>>>>> ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
>>>>> if we want to support having a normal atomic commit and a cursor
>>>>> update in the same atomic ioctl, coming up with reasonable semantics
>>>>> for that will be complicated.
>>>>>
>>>>> Pointer to code that uses this would be better ofc.
>>>>
>>>> I haven't followed this thread too closely, but we ended up needing to
>>>> add a fast patch for cursor updates to amdgpu's atomic support to
>>>> avoid stuttering issues.  Other drivers may end up being affected by
>>>> this as well.  See:
>>>> https://bugs.freedesktop.org/show_bug.cgi?id=106175
>>>> Unfortunately, the fast path requires some hacks to handle the ref
>>>> counting properly on cursor fbs:
>>>> https://patchwork.freedesktop.org/patch/266138/
>>>> https://patchwork.freedesktop.org/patch/268298/
>>>> It looks like gamma may need similar treatment:
>>>> https://bugs.freedesktop.org/show_bug.cgi?id=108917
>>>>
>>>> Alex
>>>>
>>>>
>>>>> -Daniel
>>>>>
>>>>>> Best regards,
>>>>>> Tomasz
>>>>>>
>>>>>>>
>>>>>>> Helen
>>>>>>>
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Tomasz
>>>>>>>>
>>>>>>>>> Or do you suggest another name? I am not familiar with vulkan terminology.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Helen
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>>>>>>>>> [prepared for upstream]
>>>>>>>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>>>>>>>>
>>>>>>>>>>> ---
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
>>>>>>>>>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
>>>>>>>>>>> figure that async_update exists.
>>>>>>>>>>>
>>>>>>>>>>> This was tested using a small program that exercises the uAPI for easy
>>>>>>>>>>> sanity testing. The program was created by Alexandros and modified by
>>>>>>>>>>> Enric to test the capability flag [2].
>>>>>>>>>>>
>>>>>>>>>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
>>>>>>>>>>> the patch to update cursors asynchronously through atomic plus the patch
>>>>>>>>>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
>>>>>>>>>>>
>>>>>>>>>>> To test, just build the program and use the --atomic flag to use the cursor
>>>>>>>>>>> plane with the ATOMIC_AMEND flag. E.g.
>>>>>>>>>>>
>>>>>>>>>>>    drm_cursor --atomic
>>>>>>>>>>>
>>>>>>>>>>> [1] https://patchwork.freedesktop.org/patch/243088/
>>>>>>>>>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> Helen
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>   drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
>>>>>>>>>>>   include/uapi/drm/drm.h      |  1 +
>>>>>>>>>>>   2 files changed, 12 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>>>>>>>>>>> index 94bd872d56c4..4a7e0f874171 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/drm_ioctl.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/drm_ioctl.c
>>>>>>>>>>> @@ -31,6 +31,7 @@
>>>>>>>>>>>   #include <drm/drm_ioctl.h>
>>>>>>>>>>>   #include <drm/drmP.h>
>>>>>>>>>>>   #include <drm/drm_auth.h>
>>>>>>>>>>> +#include <drm/drm_modeset_helper_vtables.h>
>>>>>>>>>>>   #include "drm_legacy.h"
>>>>>>>>>>>   #include "drm_internal.h"
>>>>>>>>>>>   #include "drm_crtc_internal.h"
>>>>>>>>>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>>>>>>>>>>>   {
>>>>>>>>>>>       struct drm_get_cap *req = data;
>>>>>>>>>>>       struct drm_crtc *crtc;
>>>>>>>>>>> +    struct drm_plane *plane;
>>>>>>>>>>>
>>>>>>>>>>>       req->value = 0;
>>>>>>>>>>>
>>>>>>>>>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>>>>>>>>>>>       case DRM_CAP_CRTC_IN_VBLANK_EVENT:
>>>>>>>>>>>               req->value = 1;
>>>>>>>>>>>               break;
>>>>>>>>>>> +    case DRM_CAP_ASYNC_UPDATE:
>>>>>>>>>>> +            req->value = 1;
>>>>>>>>>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
>>>>>>>>>>> +                    if (!plane->helper_private->atomic_async_update) {
>>>>>>>>>>> +                            req->value = 0;
>>>>>>>>>>> +                            break;
>>>>>>>>>>> +                    }
>>>>>>>>>>> +            }
>>>>>>>>>>> +            break;
>>>>>>>>>>>       default:
>>>>>>>>>>>               return -EINVAL;
>>>>>>>>>>>       }
>>>>>>>>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>>>>>>>>>> index 300f336633f2..ff01540cbb1d 100644
>>>>>>>>>>> --- a/include/uapi/drm/drm.h
>>>>>>>>>>> +++ b/include/uapi/drm/drm.h
>>>>>>>>>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
>>>>>>>>>>>   #define DRM_CAP_PAGE_FLIP_TARGET    0x11
>>>>>>>>>>>   #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
>>>>>>>>>>>   #define DRM_CAP_SYNCOBJ             0x13
>>>>>>>>>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
>>>>>>>>>>>
>>>>>>>>>>>   /** DRM_IOCTL_GET_CAP ioctl argument type */
>>>>>>>>>>>   struct drm_get_cap {
>>>>>>>>>>> --
>>>>>>>>>>> 2.19.1
>>>>>>>>>>>
>>>>>>>>>>> _______________________________________________
>>>>>>>>>>> dri-devel mailing list
>>>>>>>>>>> dri-devel@lists.freedesktop.org
>>>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>>>>>>
>>>>>> _______________________________________________
>>>>>> dri-devel mailing list
>>>>>> dri-devel@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Daniel Vetter
>>>>> Software Engineer, Intel Corporation
>>>>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>>
>>>
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> 
>

Daniel Vetter Dec. 20, 2018, 6:34 p.m. UTC | #14

On Thu, Dec 20, 2018 at 6:38 PM Kazlauskas, Nicholas
<Nicholas.Kazlauskas@amd.com> wrote:
>
> On 12/20/18 12:09 PM, Daniel Vetter wrote:
> > On Thu, Dec 20, 2018 at 6:03 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >>
> >> + Harry
> >>
> >> On Thu, Dec 20, 2018 at 11:54 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>>
> >>> On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >>>>
> >>>> + Nicholas
> >>>>
> >>>> On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>>>>
> >>>>> On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
> >>>>>>
> >>>>>> Hi Helen,
> >>>>>>
> >>>>>> On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
> >>>>>>>
> >>>>>>> Hi Tomasz,
> >>>>>>>
> >>>>>>> On 12/13/18 2:02 AM, Tomasz Figa wrote:
> >>>>>>>> On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> >>>>>>>>>
> >>>>>>>>> Hi Ville
> >>>>>>>>>
> >>>>>>>>> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> >>>>>>>>>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> >>>>>>>>>>> Allow userspace to identify if the driver supports async update.
> >>>>>>>>>>
> >>>>>>>>>> And what exactly is an "async update"?
> >>>>>>>>>
> >>>>>>>>> I agree we are lacking docs on this, I'll send in the next version as
> >>>>>>>>> soon as we agree on a name (please see below).
> >>>>>>>>>
> >>>>>>>>> For reference:
> >>>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> I keep asking people to come up with the a better name for this, and to
> >>>>>>>>>> document what it actually means. Recently I've been think we should
> >>>>>>>>>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> >>>>>>>>>> avoid introducing yet another set of names for the same thing. We'd
> >>>>>>>>>> still want to document things properly though.
> >>>>>>>>>
> >>>>>>>>> Another name it was suggested was "atomic amend", this feature basically
> >>>>>>>>> allows userspace to complement an update previously sent (i.e. its in
> >>>>>>>>> the queue and wasn't commited yet), it allows adding a plane update to
> >>>>>>>>> the next commit. So what do you think in renaming it to "atomic amend"?
> >>>>>>>>
> >>>>>>>> Note that it doesn't seem to be what the code currently is doing. For
> >>>>>>>> example, for cursor updates, it doesn't seem to be working on the
> >>>>>>>> currently pending commit, but just directly issues an atomic async
> >>>>>>>> update call to the planes. The code actually seems to fall back to a
> >>>>>>>> normal sync commit, if there is an already pending commit touching the
> >>>>>>>> same plane or including a modeset.
> >>>>>>>
> >>>>>>> It should fail as discussed at:
> >>>>>>> https://patchwork.freedesktop.org/patch/243088/
> >>>>>>>
> >>>>>>> There was the following code inside the drm_atomic_helper_async_check()
> >>>>>>> in the previous patch which would fallback to a normal commit if there
> >>>>>>> isn't any pending commit to amend:
> >>>>>>>
> >>>>>>> +       if (!old_plane_state->commit)
> >>>>>>> +               return -EINVAL;
> >>>>>>>
> >>>>>>> In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> >>>>>>> this got removed, but which means that async update will be enabled
> >>>>>>> anyway. So the following code is wrong:
> >>>>>>>
> >>>>>>> -       if (state->legacy_cursor_update)
> >>>>>>> +       if (state->async_update || state->legacy_cursor_update)
> >>>>>>>                  state->async_update = !drm_atomic_helper_async_check(dev, state);
> >>>>>>>
> >>>>>>> Does it make sense? If yes I'll fix this in the next version of the
> >>>>>>> Atomic IOCTL patch (and also those two patches should be in the same
> >>>>>>> series, I'll send them together next time).
> >>>>>>>
> >>>>>>> Thanks for pointing this out.
> >>>>>>>
> >>>>>>> Please let me know if you still don't agree on the name "atomic amend",
> >>>>>>> or if I am missing something.
> >>>>>>
> >>>>>> I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> >>>>>> need a way to commit the cursor plane asynchronously from other
> >>>>>> commits any time the cursor changes its position or framebuffer. As
> >>>>>> long as the new API allows that and the maintainers are fine with it,
> >>>>>> I think I should be okay with it too.
> >>>>>
> >>>>> If this is just about the cursor, why is the current legacy cursor
> >>>>> ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
> >>>>> if we want to support having a normal atomic commit and a cursor
> >>>>> update in the same atomic ioctl, coming up with reasonable semantics
> >>>>> for that will be complicated.
> >>>>>
> >>>>> Pointer to code that uses this would be better ofc.
> >>>>
> >>>> I haven't followed this thread too closely, but we ended up needing to
> >>>> add a fast patch for cursor updates to amdgpu's atomic support to
> >>>> avoid stuttering issues.  Other drivers may end up being affected by
> >>>> this as well.  See:
> >>>> https://bugs.freedesktop.org/show_bug.cgi?id=106175
> >>>> Unfortunately, the fast path requires some hacks to handle the ref
> >>>> counting properly on cursor fbs:
> >>>> https://patchwork.freedesktop.org/patch/266138/
> >>>> https://patchwork.freedesktop.org/patch/268298/
> >>>> It looks like gamma may need similar treatment:
> >>>> https://bugs.freedesktop.org/show_bug.cgi?id=108917
> >>>
> >>> Can we get these patches cc'ed to dri-devel so that there's some
> >>> common solution? Everyone hacking legacy_cursor_update hacks on their
> >>> own doesn't really work well. Or would at least give some visibility
> >>> into what's all going on.
> >>
> >> Agreed.
> >
> > Bit more detail: The legacy_cursor_update hacks all over is probably
> > the worst part of atomic, and everyone seems unhappy with it. Except
> > all efforts to address it fall short by a lot. I think Gustavo from
> > Collabora once had a patch series, but it only ever got merged
> > partially, and now we're back with a slightly different pick of color
> > it seems. Hence why I'm somewhat grumpy on this here.
>
> I was planning on submitting a patch that added the old->fb == new->fb
> check for the async check in drm helpers but haven't gotten around to it
> yet. Async update behavior always prepares the new plane fb and cleans
> up the new plane fb, so you end up with a sequence like the following:
>
> - Fast update, fb1 prepare, fb1 cleanup
> - Fast update, fb2 prepare, fb2 cleanup
> - Slow update, fb1 prepare, fb2 cleanup
> - Fast update, fb2 cleanup -> double cleanup, potential use after free

Hm yeah that's not going to work well.

> The only driver that still expects to be doing fb changes in their fast
> path is vc4 from what I can tell, but they'd likely run into an issue
> with interleaving fbs as well.
>
> The fast path on i915 skips the async helpers and implements this a lot
> better (even though it ends up grabbing the struct mutex lock) since it
> always calls cleanup on the old plane state like the slow path does. It
> also swaps the old plane state with the new one.
>
> But since async_update is in place it can't do that (the old plane state
> is the existing plane state). There's not a really good way of emulating
> i915 here without changing the whole concept of async update.

Might need to align the async update stuff with what i915 does then.
The idea of that was to make things easier for drivers, not even more
buggy.

> >>> Not sure about the gamma thing since we had opposite bugs on i915
> >>> about gamma not being vsynced and tearing terribly. Cursor is special
> >>> since it tends to be too small to notice tearing.
> >>
> >> Our cursor hw (and possibly gamma as well Nicholas?  Harry?) is double
> >> buffered, so we can update it any time for the most part and the
> >> changes won't take affect until the next vupdate period.
>
> I haven't really investigated too much into the gamma stuttering issue,
> but I think it's similar to the cursor update - a high volume of atomic
> updates that ends up skipping over a vblank or two.

Yeah, from the description that's exactly what's going on.

> > Hm, I guess we could make the gamma update optionally async, and let
> > drivers deal. The issue is that the current async helper stuff won't
> > cope with gamma updates (it's aimed at plane updates only, not at crtc
> > property updates). Or we get userspace to do proper atomic updates. Or
> > we do some faking in the kernel, e.g. waiting with the gamma update
> > until the next atomic update happens. But that kinda breaks
> > ->atomic_check.
> > -Daniel
>
> The in-place update method can work for us for gamma updates I believe,
> but there's likely a lot more drivers would have to be careful about
> with in-place updates on the CRTC state.
>
> A full state swap on the object itself would probably be best for
> tackling it this way. It's what I'd like to happen for the existing
> plane API at least (and which I think is fine since the plane is locked
> anyway).

Swapping it wreaks the synchronization of subsequent atomic updates.
They only check the preceeding state for anything to synchronize
against. If you swap it (instead of overwriting) things get really
complicated.

Or that's the vague memory I have of why we've done this, might be
lost in the past. As mentioned, the entire thing is lots of duct-tape
all around unfortunately :-/
-Daniel

>
> Nicholas Kazlauskas
>
> >
> >>
> >> Alex
> >>
> >>>
> >>> Thanks, Daniel
> >>>
> >>>> Alex
> >>>>
> >>>>
> >>>>> -Daniel
> >>>>>
> >>>>>> Best regards,
> >>>>>> Tomasz
> >>>>>>
> >>>>>>>
> >>>>>>> Helen
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Best regards,
> >>>>>>>> Tomasz
> >>>>>>>>
> >>>>>>>>> Or do you suggest another name? I am not familiar with vulkan terminology.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Thanks
> >>>>>>>>> Helen
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >>>>>>>>>>> [prepared for upstream]
> >>>>>>>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>>>>>>>>>>
> >>>>>>>>>>> ---
> >>>>>>>>>>> Hi,
> >>>>>>>>>>>
> >>>>>>>>>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> >>>>>>>>>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> >>>>>>>>>>> figure that async_update exists.
> >>>>>>>>>>>
> >>>>>>>>>>> This was tested using a small program that exercises the uAPI for easy
> >>>>>>>>>>> sanity testing. The program was created by Alexandros and modified by
> >>>>>>>>>>> Enric to test the capability flag [2].
> >>>>>>>>>>>
> >>>>>>>>>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> >>>>>>>>>>> the patch to update cursors asynchronously through atomic plus the patch
> >>>>>>>>>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> >>>>>>>>>>>
> >>>>>>>>>>> To test, just build the program and use the --atomic flag to use the cursor
> >>>>>>>>>>> plane with the ATOMIC_AMEND flag. E.g.
> >>>>>>>>>>>
> >>>>>>>>>>>    drm_cursor --atomic
> >>>>>>>>>>>
> >>>>>>>>>>> [1] https://patchwork.freedesktop.org/patch/243088/
> >>>>>>>>>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks
> >>>>>>>>>>> Helen
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>   drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> >>>>>>>>>>>   include/uapi/drm/drm.h      |  1 +
> >>>>>>>>>>>   2 files changed, 12 insertions(+)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> >>>>>>>>>>> index 94bd872d56c4..4a7e0f874171 100644
> >>>>>>>>>>> --- a/drivers/gpu/drm/drm_ioctl.c
> >>>>>>>>>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> >>>>>>>>>>> @@ -31,6 +31,7 @@
> >>>>>>>>>>>   #include <drm/drm_ioctl.h>
> >>>>>>>>>>>   #include <drm/drmP.h>
> >>>>>>>>>>>   #include <drm/drm_auth.h>
> >>>>>>>>>>> +#include <drm/drm_modeset_helper_vtables.h>
> >>>>>>>>>>>   #include "drm_legacy.h"
> >>>>>>>>>>>   #include "drm_internal.h"
> >>>>>>>>>>>   #include "drm_crtc_internal.h"
> >>>>>>>>>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> >>>>>>>>>>>   {
> >>>>>>>>>>>       struct drm_get_cap *req = data;
> >>>>>>>>>>>       struct drm_crtc *crtc;
> >>>>>>>>>>> +    struct drm_plane *plane;
> >>>>>>>>>>>
> >>>>>>>>>>>       req->value = 0;
> >>>>>>>>>>>
> >>>>>>>>>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> >>>>>>>>>>>       case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> >>>>>>>>>>>               req->value = 1;
> >>>>>>>>>>>               break;
> >>>>>>>>>>> +    case DRM_CAP_ASYNC_UPDATE:
> >>>>>>>>>>> +            req->value = 1;
> >>>>>>>>>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> >>>>>>>>>>> +                    if (!plane->helper_private->atomic_async_update) {
> >>>>>>>>>>> +                            req->value = 0;
> >>>>>>>>>>> +                            break;
> >>>>>>>>>>> +                    }
> >>>>>>>>>>> +            }
> >>>>>>>>>>> +            break;
> >>>>>>>>>>>       default:
> >>>>>>>>>>>               return -EINVAL;
> >>>>>>>>>>>       }
> >>>>>>>>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> >>>>>>>>>>> index 300f336633f2..ff01540cbb1d 100644
> >>>>>>>>>>> --- a/include/uapi/drm/drm.h
> >>>>>>>>>>> +++ b/include/uapi/drm/drm.h
> >>>>>>>>>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> >>>>>>>>>>>   #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> >>>>>>>>>>>   #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> >>>>>>>>>>>   #define DRM_CAP_SYNCOBJ             0x13
> >>>>>>>>>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> >>>>>>>>>>>
> >>>>>>>>>>>   /** DRM_IOCTL_GET_CAP ioctl argument type */
> >>>>>>>>>>>   struct drm_get_cap {
> >>>>>>>>>>> --
> >>>>>>>>>>> 2.19.1
> >>>>>>>>>>>
> >>>>>>>>>>> _______________________________________________
> >>>>>>>>>>> dri-devel mailing list
> >>>>>>>>>>> dri-devel@lists.freedesktop.org
> >>>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>>>>>>>>
> >>>>>> _______________________________________________
> >>>>>> dri-devel mailing list
> >>>>>> dri-devel@lists.freedesktop.org
> >>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>>>
> >>>>>
> >>>>>
> >>>>> --
> >>>>> Daniel Vetter
> >>>>> Software Engineer, Intel Corporation
> >>>>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >>>>> _______________________________________________
> >>>>> dri-devel mailing list
> >>>>> dri-devel@lists.freedesktop.org
> >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>
> >>>
> >>>
> >>> --
> >>> Daniel Vetter
> >>> Software Engineer, Intel Corporation
> >>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >>>
> >>> On Thu, Dec 20, 2018 at 5:40 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >>>>
> >>>> + Nicholas
> >>>>
> >>>> On Thu, Dec 20, 2018 at 5:47 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>>>>
> >>>>> On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
> >>>>>>
> >>>>>> Hi Helen,
> >>>>>>
> >>>>>> On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
> >>>>>>>
> >>>>>>> Hi Tomasz,
> >>>>>>>
> >>>>>>> On 12/13/18 2:02 AM, Tomasz Figa wrote:
> >>>>>>>> On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> >>>>>>>>>
> >>>>>>>>> Hi Ville
> >>>>>>>>>
> >>>>>>>>> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> >>>>>>>>>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> >>>>>>>>>>> Allow userspace to identify if the driver supports async update.
> >>>>>>>>>>
> >>>>>>>>>> And what exactly is an "async update"?
> >>>>>>>>>
> >>>>>>>>> I agree we are lacking docs on this, I'll send in the next version as
> >>>>>>>>> soon as we agree on a name (please see below).
> >>>>>>>>>
> >>>>>>>>> For reference:
> >>>>>>>>> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> I keep asking people to come up with the a better name for this, and to
> >>>>>>>>>> document what it actually means. Recently I've been think we should
> >>>>>>>>>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> >>>>>>>>>> avoid introducing yet another set of names for the same thing. We'd
> >>>>>>>>>> still want to document things properly though.
> >>>>>>>>>
> >>>>>>>>> Another name it was suggested was "atomic amend", this feature basically
> >>>>>>>>> allows userspace to complement an update previously sent (i.e. its in
> >>>>>>>>> the queue and wasn't commited yet), it allows adding a plane update to
> >>>>>>>>> the next commit. So what do you think in renaming it to "atomic amend"?
> >>>>>>>>
> >>>>>>>> Note that it doesn't seem to be what the code currently is doing. For
> >>>>>>>> example, for cursor updates, it doesn't seem to be working on the
> >>>>>>>> currently pending commit, but just directly issues an atomic async
> >>>>>>>> update call to the planes. The code actually seems to fall back to a
> >>>>>>>> normal sync commit, if there is an already pending commit touching the
> >>>>>>>> same plane or including a modeset.
> >>>>>>>
> >>>>>>> It should fail as discussed at:
> >>>>>>> https://patchwork.freedesktop.org/patch/243088/
> >>>>>>>
> >>>>>>> There was the following code inside the drm_atomic_helper_async_check()
> >>>>>>> in the previous patch which would fallback to a normal commit if there
> >>>>>>> isn't any pending commit to amend:
> >>>>>>>
> >>>>>>> +       if (!old_plane_state->commit)
> >>>>>>> +               return -EINVAL;
> >>>>>>>
> >>>>>>> In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> >>>>>>> this got removed, but which means that async update will be enabled
> >>>>>>> anyway. So the following code is wrong:
> >>>>>>>
> >>>>>>> -       if (state->legacy_cursor_update)
> >>>>>>> +       if (state->async_update || state->legacy_cursor_update)
> >>>>>>>                  state->async_update = !drm_atomic_helper_async_check(dev, state);
> >>>>>>>
> >>>>>>> Does it make sense? If yes I'll fix this in the next version of the
> >>>>>>> Atomic IOCTL patch (and also those two patches should be in the same
> >>>>>>> series, I'll send them together next time).
> >>>>>>>
> >>>>>>> Thanks for pointing this out.
> >>>>>>>
> >>>>>>> Please let me know if you still don't agree on the name "atomic amend",
> >>>>>>> or if I am missing something.
> >>>>>>
> >>>>>> I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> >>>>>> need a way to commit the cursor plane asynchronously from other
> >>>>>> commits any time the cursor changes its position or framebuffer. As
> >>>>>> long as the new API allows that and the maintainers are fine with it,
> >>>>>> I think I should be okay with it too.
> >>>>>
> >>>>> If this is just about the cursor, why is the current legacy cursor
> >>>>> ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
> >>>>> if we want to support having a normal atomic commit and a cursor
> >>>>> update in the same atomic ioctl, coming up with reasonable semantics
> >>>>> for that will be complicated.
> >>>>>
> >>>>> Pointer to code that uses this would be better ofc.
> >>>>
> >>>> I haven't followed this thread too closely, but we ended up needing to
> >>>> add a fast patch for cursor updates to amdgpu's atomic support to
> >>>> avoid stuttering issues.  Other drivers may end up being affected by
> >>>> this as well.  See:
> >>>> https://bugs.freedesktop.org/show_bug.cgi?id=106175
> >>>> Unfortunately, the fast path requires some hacks to handle the ref
> >>>> counting properly on cursor fbs:
> >>>> https://patchwork.freedesktop.org/patch/266138/
> >>>> https://patchwork.freedesktop.org/patch/268298/
> >>>> It looks like gamma may need similar treatment:
> >>>> https://bugs.freedesktop.org/show_bug.cgi?id=108917
> >>>>
> >>>> Alex
> >>>>
> >>>>
> >>>>> -Daniel
> >>>>>
> >>>>>> Best regards,
> >>>>>> Tomasz
> >>>>>>
> >>>>>>>
> >>>>>>> Helen
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Best regards,
> >>>>>>>> Tomasz
> >>>>>>>>
> >>>>>>>>> Or do you suggest another name? I am not familiar with vulkan terminology.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Thanks
> >>>>>>>>> Helen
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >>>>>>>>>>> [prepared for upstream]
> >>>>>>>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>>>>>>>>>>
> >>>>>>>>>>> ---
> >>>>>>>>>>> Hi,
> >>>>>>>>>>>
> >>>>>>>>>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> >>>>>>>>>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> >>>>>>>>>>> figure that async_update exists.
> >>>>>>>>>>>
> >>>>>>>>>>> This was tested using a small program that exercises the uAPI for easy
> >>>>>>>>>>> sanity testing. The program was created by Alexandros and modified by
> >>>>>>>>>>> Enric to test the capability flag [2].
> >>>>>>>>>>>
> >>>>>>>>>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> >>>>>>>>>>> the patch to update cursors asynchronously through atomic plus the patch
> >>>>>>>>>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> >>>>>>>>>>>
> >>>>>>>>>>> To test, just build the program and use the --atomic flag to use the cursor
> >>>>>>>>>>> plane with the ATOMIC_AMEND flag. E.g.
> >>>>>>>>>>>
> >>>>>>>>>>>    drm_cursor --atomic
> >>>>>>>>>>>
> >>>>>>>>>>> [1] https://patchwork.freedesktop.org/patch/243088/
> >>>>>>>>>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks
> >>>>>>>>>>> Helen
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>   drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> >>>>>>>>>>>   include/uapi/drm/drm.h      |  1 +
> >>>>>>>>>>>   2 files changed, 12 insertions(+)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> >>>>>>>>>>> index 94bd872d56c4..4a7e0f874171 100644
> >>>>>>>>>>> --- a/drivers/gpu/drm/drm_ioctl.c
> >>>>>>>>>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> >>>>>>>>>>> @@ -31,6 +31,7 @@
> >>>>>>>>>>>   #include <drm/drm_ioctl.h>
> >>>>>>>>>>>   #include <drm/drmP.h>
> >>>>>>>>>>>   #include <drm/drm_auth.h>
> >>>>>>>>>>> +#include <drm/drm_modeset_helper_vtables.h>
> >>>>>>>>>>>   #include "drm_legacy.h"
> >>>>>>>>>>>   #include "drm_internal.h"
> >>>>>>>>>>>   #include "drm_crtc_internal.h"
> >>>>>>>>>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> >>>>>>>>>>>   {
> >>>>>>>>>>>       struct drm_get_cap *req = data;
> >>>>>>>>>>>       struct drm_crtc *crtc;
> >>>>>>>>>>> +    struct drm_plane *plane;
> >>>>>>>>>>>
> >>>>>>>>>>>       req->value = 0;
> >>>>>>>>>>>
> >>>>>>>>>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> >>>>>>>>>>>       case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> >>>>>>>>>>>               req->value = 1;
> >>>>>>>>>>>               break;
> >>>>>>>>>>> +    case DRM_CAP_ASYNC_UPDATE:
> >>>>>>>>>>> +            req->value = 1;
> >>>>>>>>>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> >>>>>>>>>>> +                    if (!plane->helper_private->atomic_async_update) {
> >>>>>>>>>>> +                            req->value = 0;
> >>>>>>>>>>> +                            break;
> >>>>>>>>>>> +                    }
> >>>>>>>>>>> +            }
> >>>>>>>>>>> +            break;
> >>>>>>>>>>>       default:
> >>>>>>>>>>>               return -EINVAL;
> >>>>>>>>>>>       }
> >>>>>>>>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> >>>>>>>>>>> index 300f336633f2..ff01540cbb1d 100644
> >>>>>>>>>>> --- a/include/uapi/drm/drm.h
> >>>>>>>>>>> +++ b/include/uapi/drm/drm.h
> >>>>>>>>>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> >>>>>>>>>>>   #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> >>>>>>>>>>>   #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> >>>>>>>>>>>   #define DRM_CAP_SYNCOBJ             0x13
> >>>>>>>>>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> >>>>>>>>>>>
> >>>>>>>>>>>   /** DRM_IOCTL_GET_CAP ioctl argument type */
> >>>>>>>>>>>   struct drm_get_cap {
> >>>>>>>>>>> --
> >>>>>>>>>>> 2.19.1
> >>>>>>>>>>>
> >>>>>>>>>>> _______________________________________________
> >>>>>>>>>>> dri-devel mailing list
> >>>>>>>>>>> dri-devel@lists.freedesktop.org
> >>>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>>>>>>>>
> >>>>>> _______________________________________________
> >>>>>> dri-devel mailing list
> >>>>>> dri-devel@lists.freedesktop.org
> >>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>>>
> >>>>>
> >>>>>
> >>>>> --
> >>>>> Daniel Vetter
> >>>>> Software Engineer, Intel Corporation
> >>>>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >>>>> _______________________________________________
> >>>>> dri-devel mailing list
> >>>>> dri-devel@lists.freedesktop.org
> >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>
> >>>
> >>>
> >>> --
> >>> Daniel Vetter
> >>> Software Engineer, Intel Corporation
> >>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> >
> >
>

Tomasz Figa Dec. 21, 2018, 3:51 a.m. UTC | #15

On Thu, Dec 20, 2018 at 7:47 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > Hi Helen,
> >
> > On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On 12/13/18 2:02 AM, Tomasz Figa wrote:
> > > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > >>
> > > >> Hi Ville
> > > >>
> > > >> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> > > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> > > >>>> Allow userspace to identify if the driver supports async update.
> > > >>>
> > > >>> And what exactly is an "async update"?
> > > >>
> > > >> I agree we are lacking docs on this, I'll send in the next version as
> > > >> soon as we agree on a name (please see below).
> > > >>
> > > >> For reference:
> > > >> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> > > >>
> > > >>>
> > > >>> I keep asking people to come up with the a better name for this, and to
> > > >>> document what it actually means. Recently I've been think we should
> > > >>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> > > >>> avoid introducing yet another set of names for the same thing. We'd
> > > >>> still want to document things properly though.
> > > >>
> > > >> Another name it was suggested was "atomic amend", this feature basically
> > > >> allows userspace to complement an update previously sent (i.e. its in
> > > >> the queue and wasn't commited yet), it allows adding a plane update to
> > > >> the next commit. So what do you think in renaming it to "atomic amend"?
> > > >
> > > > Note that it doesn't seem to be what the code currently is doing. For
> > > > example, for cursor updates, it doesn't seem to be working on the
> > > > currently pending commit, but just directly issues an atomic async
> > > > update call to the planes. The code actually seems to fall back to a
> > > > normal sync commit, if there is an already pending commit touching the
> > > > same plane or including a modeset.
> > >
> > > It should fail as discussed at:
> > > https://patchwork.freedesktop.org/patch/243088/
> > >
> > > There was the following code inside the drm_atomic_helper_async_check()
> > > in the previous patch which would fallback to a normal commit if there
> > > isn't any pending commit to amend:
> > >
> > > +       if (!old_plane_state->commit)
> > > +               return -EINVAL;
> > >
> > > In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> > > this got removed, but which means that async update will be enabled
> > > anyway. So the following code is wrong:
> > >
> > > -       if (state->legacy_cursor_update)
> > > +       if (state->async_update || state->legacy_cursor_update)
> > >                 state->async_update = !drm_atomic_helper_async_check(dev, state);
> > >
> > > Does it make sense? If yes I'll fix this in the next version of the
> > > Atomic IOCTL patch (and also those two patches should be in the same
> > > series, I'll send them together next time).
> > >
> > > Thanks for pointing this out.
> > >
> > > Please let me know if you still don't agree on the name "atomic amend",
> > > or if I am missing something.
> >
> > I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> > need a way to commit the cursor plane asynchronously from other
> > commits any time the cursor changes its position or framebuffer. As
> > long as the new API allows that and the maintainers are fine with it,
> > I think I should be okay with it too.
>
> If this is just about the cursor, why is the current legacy cursor
> ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
> if we want to support having a normal atomic commit and a cursor
> update in the same atomic ioctl, coming up with reasonable semantics
> for that will be complicated.
>
> Pointer to code that uses this would be better ofc.

Sorry, let me clarify that I was mostly referring to the respective
kernel functionality here, not the userspace extension.

As far as I know, Chromium still uses the legacy cursor ioctls and we
have our drivers implement them directly, bypassing the atomic commit
mechanism. However, it sounds like this async commit could let us
remove the custom implementations from the drivers and have a common
helper for it, which would be a good step forward.

Daniel, Daniele, Sean, Stephane, could you clarify what are our needs
for this userspace interface?

> -Daniel
>
> > Best regards,
> > Tomasz
> >
> > >
> > > Helen
> > >
> > > >
> > > > Best regards,
> > > > Tomasz
> > > >
> > > >> Or do you suggest another name? I am not familiar with vulkan terminology.
> > > >>
> > > >>
> > > >> Thanks
> > > >> Helen
> > > >>
> > > >>>
> > > >>>>
> > > >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > >>>> [prepared for upstream]
> > > >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > >>>>
> > > >>>> ---
> > > >>>> Hi,
> > > >>>>
> > > >>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> > > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> > > >>>> figure that async_update exists.
> > > >>>>
> > > >>>> This was tested using a small program that exercises the uAPI for easy
> > > >>>> sanity testing. The program was created by Alexandros and modified by
> > > >>>> Enric to test the capability flag [2].
> > > >>>>
> > > >>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> > > >>>> the patch to update cursors asynchronously through atomic plus the patch
> > > >>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> > > >>>>
> > > >>>> To test, just build the program and use the --atomic flag to use the cursor
> > > >>>> plane with the ATOMIC_AMEND flag. E.g.
> > > >>>>
> > > >>>>   drm_cursor --atomic
> > > >>>>
> > > >>>> [1] https://patchwork.freedesktop.org/patch/243088/
> > > >>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> > > >>>>
> > > >>>> Thanks
> > > >>>> Helen
> > > >>>>
> > > >>>>
> > > >>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> > > >>>>  include/uapi/drm/drm.h      |  1 +
> > > >>>>  2 files changed, 12 insertions(+)
> > > >>>>
> > > >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > >>>> index 94bd872d56c4..4a7e0f874171 100644
> > > >>>> --- a/drivers/gpu/drm/drm_ioctl.c
> > > >>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> > > >>>> @@ -31,6 +31,7 @@
> > > >>>>  #include <drm/drm_ioctl.h>
> > > >>>>  #include <drm/drmP.h>
> > > >>>>  #include <drm/drm_auth.h>
> > > >>>> +#include <drm/drm_modeset_helper_vtables.h>
> > > >>>>  #include "drm_legacy.h"
> > > >>>>  #include "drm_internal.h"
> > > >>>>  #include "drm_crtc_internal.h"
> > > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > >>>>  {
> > > >>>>      struct drm_get_cap *req = data;
> > > >>>>      struct drm_crtc *crtc;
> > > >>>> +    struct drm_plane *plane;
> > > >>>>
> > > >>>>      req->value = 0;
> > > >>>>
> > > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > >>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> > > >>>>              req->value = 1;
> > > >>>>              break;
> > > >>>> +    case DRM_CAP_ASYNC_UPDATE:
> > > >>>> +            req->value = 1;
> > > >>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > > >>>> +                    if (!plane->helper_private->atomic_async_update) {
> > > >>>> +                            req->value = 0;
> > > >>>> +                            break;
> > > >>>> +                    }
> > > >>>> +            }
> > > >>>> +            break;
> > > >>>>      default:
> > > >>>>              return -EINVAL;
> > > >>>>      }
> > > >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > >>>> index 300f336633f2..ff01540cbb1d 100644
> > > >>>> --- a/include/uapi/drm/drm.h
> > > >>>> +++ b/include/uapi/drm/drm.h
> > > >>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> > > >>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> > > >>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> > > >>>>  #define DRM_CAP_SYNCOBJ             0x13
> > > >>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> > > >>>>
> > > >>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> > > >>>>  struct drm_get_cap {
> > > >>>> --
> > > >>>> 2.19.1
> > > >>>>
> > > >>>> _______________________________________________
> > > >>>> dri-devel mailing list
> > > >>>> dri-devel@lists.freedesktop.org
> > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >>>
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Michel Dänzer Dec. 21, 2018, 9:47 a.m. UTC | #16

On 2018-12-20 6:16 p.m., Michel Dänzer wrote:
> On 2018-12-20 6:09 p.m., Daniel Vetter wrote:
>> On Thu, Dec 20, 2018 at 6:03 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>>> On Thu, Dec 20, 2018 at 11:54 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>>>> Not sure about the gamma thing since we had opposite bugs on i915
>>>> about gamma not being vsynced and tearing terribly. Cursor is special
>>>> since it tends to be too small to notice tearing.
>>>
>>> Our cursor hw (and possibly gamma as well Nicholas?  Harry?) is double
>>> buffered, so we can update it any time for the most part and the
>>> changes won't take affect until the next vupdate period.
>>
>> Hm, I guess we could make the gamma update optionally async, and let
>> drivers deal. The issue is that the current async helper stuff won't
>> cope with gamma updates (it's aimed at plane updates only, not at crtc
>> property updates). Or we get userspace to do proper atomic updates. Or
>> we do some faking in the kernel, e.g. waiting with the gamma update
>> until the next atomic update happens. But that kinda breaks
>> ->atomic_check.
> 
> Would it be possible to merge gamma changes into a pending commit, if
> there is one, and create a new commit otherwise?
> 
> Otherwise the atomic API just moves this same problem to be solved by
> userspace.

Actually, I'm not sure this is even solvable in a Xorg driver. When it
gets called to update the gamma LUT:

1. If there's a pending atomic commit, it cannot amend that, can it?

2. It has no way of knowing when the next atomic commit would be
submitted e.g. for a page flip, so it kind of has to create its own
commit for the gamma update.

Michel Dänzer Dec. 21, 2018, 9:55 a.m. UTC | #17

On 2018-12-20 6:38 p.m., Kazlauskas, Nicholas wrote:
> On 12/20/18 12:09 PM, Daniel Vetter wrote:
>> On Thu, Dec 20, 2018 at 6:03 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>>> On Thu, Dec 20, 2018 at 11:54 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>
>>>> Not sure about the gamma thing since we had opposite bugs on i915
>>>> about gamma not being vsynced and tearing terribly. Cursor is special
>>>> since it tends to be too small to notice tearing.
>>>
>>> Our cursor hw (and possibly gamma as well Nicholas?  Harry?) is double
>>> buffered, so we can update it any time for the most part and the
>>> changes won't take affect until the next vupdate period.
> 
> I haven't really investigated too much into the gamma stuttering issue, 
> but I think it's similar to the cursor update - a high volume of atomic 
> updates that ends up skipping over a vblank or two.

FWIW, I don't think the use-cases described in
https://bugs.freedesktop.org/108917 (Night Light / RedShift) involve a
particularly high volume of gamma updates. I was able to reproduce the
stuttering with ~10 gamma changes per second, but I suspect even a
single one could cause a frame drop. I assume the issue is that gamma
updates are done as separate atomic commits, which can delay other
commits for page flips.

Daniel Vetter Dec. 21, 2018, 1:26 p.m. UTC | #18

On Fri, Dec 21, 2018 at 10:47:27AM +0100, Michel Dänzer wrote:
> On 2018-12-20 6:16 p.m., Michel Dänzer wrote:
> > On 2018-12-20 6:09 p.m., Daniel Vetter wrote:
> >> On Thu, Dec 20, 2018 at 6:03 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >>> On Thu, Dec 20, 2018 at 11:54 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >>>> Not sure about the gamma thing since we had opposite bugs on i915
> >>>> about gamma not being vsynced and tearing terribly. Cursor is special
> >>>> since it tends to be too small to notice tearing.
> >>>
> >>> Our cursor hw (and possibly gamma as well Nicholas?  Harry?) is double
> >>> buffered, so we can update it any time for the most part and the
> >>> changes won't take affect until the next vupdate period.
> >>
> >> Hm, I guess we could make the gamma update optionally async, and let
> >> drivers deal. The issue is that the current async helper stuff won't
> >> cope with gamma updates (it's aimed at plane updates only, not at crtc
> >> property updates). Or we get userspace to do proper atomic updates. Or
> >> we do some faking in the kernel, e.g. waiting with the gamma update
> >> until the next atomic update happens. But that kinda breaks
> >> ->atomic_check.
> > 
> > Would it be possible to merge gamma changes into a pending commit, if
> > there is one, and create a new commit otherwise?
> > 
> > Otherwise the atomic API just moves this same problem to be solved by
> > userspace.
> 
> Actually, I'm not sure this is even solvable in a Xorg driver. When it
> gets called to update the gamma LUT:
> 
> 1. If there's a pending atomic commit, it cannot amend that, can it?

Yup, but on the kernel side we kinda have the same problem.

> 2. It has no way of knowing when the next atomic commit would be
> submitted e.g. for a page flip, so it kind of has to create its own
> commit for the gamma update.

Block handler is what I had in mind for the fallback commit, if no other
commit happened meanwhile (which would need to include it).
-Daniel

Daniel Vetter Dec. 21, 2018, 1:42 p.m. UTC | #19

On Fri, Dec 21, 2018 at 12:51:05PM +0900, Tomasz Figa wrote:
> On Thu, Dec 20, 2018 at 7:47 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Dec 20, 2018 at 10:07 AM Tomasz Figa <tfiga@chromium.org> wrote:
> > >
> > > Hi Helen,
> > >
> > > On Fri, Dec 14, 2018 at 10:35 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > >
> > > > Hi Tomasz,
> > > >
> > > > On 12/13/18 2:02 AM, Tomasz Figa wrote:
> > > > > On Thu, Dec 6, 2018 at 1:12 AM Helen Koike <helen.koike@collabora.com> wrote:
> > > > >>
> > > > >> Hi Ville
> > > > >>
> > > > >> On 11/27/18 11:34 AM, Ville Syrjälä wrote:
> > > > >>> On Fri, Nov 23, 2018 at 07:53:26PM -0200, Helen Koike wrote:
> > > > >>>> Allow userspace to identify if the driver supports async update.
> > > > >>>
> > > > >>> And what exactly is an "async update"?
> > > > >>
> > > > >> I agree we are lacking docs on this, I'll send in the next version as
> > > > >> soon as we agree on a name (please see below).
> > > > >>
> > > > >> For reference:
> > > > >> https://lists.freedesktop.org/archives/dri-devel/2017-April/138586.html
> > > > >>
> > > > >>>
> > > > >>> I keep asking people to come up with the a better name for this, and to
> > > > >>> document what it actually means. Recently I've been think we should
> > > > >>> maybe just adopt the vulkan terminology (immediate/fifo/mailbox) to
> > > > >>> avoid introducing yet another set of names for the same thing. We'd
> > > > >>> still want to document things properly though.
> > > > >>
> > > > >> Another name it was suggested was "atomic amend", this feature basically
> > > > >> allows userspace to complement an update previously sent (i.e. its in
> > > > >> the queue and wasn't commited yet), it allows adding a plane update to
> > > > >> the next commit. So what do you think in renaming it to "atomic amend"?
> > > > >
> > > > > Note that it doesn't seem to be what the code currently is doing. For
> > > > > example, for cursor updates, it doesn't seem to be working on the
> > > > > currently pending commit, but just directly issues an atomic async
> > > > > update call to the planes. The code actually seems to fall back to a
> > > > > normal sync commit, if there is an already pending commit touching the
> > > > > same plane or including a modeset.
> > > >
> > > > It should fail as discussed at:
> > > > https://patchwork.freedesktop.org/patch/243088/
> > > >
> > > > There was the following code inside the drm_atomic_helper_async_check()
> > > > in the previous patch which would fallback to a normal commit if there
> > > > isn't any pending commit to amend:
> > > >
> > > > +       if (!old_plane_state->commit)
> > > > +               return -EINVAL;
> > > >
> > > > In the v2 of the patch https://patchwork.freedesktop.org/patch/263712/
> > > > this got removed, but which means that async update will be enabled
> > > > anyway. So the following code is wrong:
> > > >
> > > > -       if (state->legacy_cursor_update)
> > > > +       if (state->async_update || state->legacy_cursor_update)
> > > >                 state->async_update = !drm_atomic_helper_async_check(dev, state);
> > > >
> > > > Does it make sense? If yes I'll fix this in the next version of the
> > > > Atomic IOCTL patch (and also those two patches should be in the same
> > > > series, I'll send them together next time).
> > > >
> > > > Thanks for pointing this out.
> > > >
> > > > Please let me know if you still don't agree on the name "atomic amend",
> > > > or if I am missing something.
> > >
> > > I'll defer it to the DRM maintainers. From Chrome OS perspective, we
> > > need a way to commit the cursor plane asynchronously from other
> > > commits any time the cursor changes its position or framebuffer. As
> > > long as the new API allows that and the maintainers are fine with it,
> > > I think I should be okay with it too.
> >
> > If this is just about the cursor, why is the current legacy cursor
> > ioctl not good enough? It's 2 ioctls instead of one, but I'm not sure
> > if we want to support having a normal atomic commit and a cursor
> > update in the same atomic ioctl, coming up with reasonable semantics
> > for that will be complicated.
> >
> > Pointer to code that uses this would be better ofc.
> 
> Sorry, let me clarify that I was mostly referring to the respective
> kernel functionality here, not the userspace extension.
> 
> As far as I know, Chromium still uses the legacy cursor ioctls and we
> have our drivers implement them directly, bypassing the atomic commit
> mechanism. However, it sounds like this async commit could let us
> remove the custom implementations from the drivers and have a common
> helper for it, which would be a good step forward.

Yup, that was at least the motivation for merging it. Unfortunately that
upstream effort got stalled, and there's still a few custom
implementations, and not many drivers using the async plane update stuff
yet.

> Daniel, Daniele, Sean, Stephane, could you clarify what are our needs
> for this userspace interface?

At xdc17 we also discussed that something like this could be useful for
overlay planes (for e.g. video), to avoid video stuttering when the main
compositor is having a hard time. But I have no idea whether that idea
went anywhere.
-Daniel

> 
> > -Daniel
> >
> > > Best regards,
> > > Tomasz
> > >
> > > >
> > > > Helen
> > > >
> > > > >
> > > > > Best regards,
> > > > > Tomasz
> > > > >
> > > > >> Or do you suggest another name? I am not familiar with vulkan terminology.
> > > > >>
> > > > >>
> > > > >> Thanks
> > > > >> Helen
> > > > >>
> > > > >>>
> > > > >>>>
> > > > >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > >>>> [prepared for upstream]
> > > > >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > >>>>
> > > > >>>> ---
> > > > >>>> Hi,
> > > > >>>>
> > > > >>>> This patch introduces the ASYNC_UPDATE cap, which originated from the
> > > > >>>> discussion regarding DRM_MODE_ATOMIC_AMEND on [1], to allow user to
> > > > >>>> figure that async_update exists.
> > > > >>>>
> > > > >>>> This was tested using a small program that exercises the uAPI for easy
> > > > >>>> sanity testing. The program was created by Alexandros and modified by
> > > > >>>> Enric to test the capability flag [2].
> > > > >>>>
> > > > >>>> The test worked on a rockchip Ficus v1.1 board on top of mainline plus
> > > > >>>> the patch to update cursors asynchronously through atomic plus the patch
> > > > >>>> that introduces the ATOMIC_AMEND flag for the drm/rockchip driver.
> > > > >>>>
> > > > >>>> To test, just build the program and use the --atomic flag to use the cursor
> > > > >>>> plane with the ATOMIC_AMEND flag. E.g.
> > > > >>>>
> > > > >>>>   drm_cursor --atomic
> > > > >>>>
> > > > >>>> [1] https://patchwork.freedesktop.org/patch/243088/
> > > > >>>> [2] https://gitlab.collabora.com/eballetbo/drm-cursor/commits/async-capability
> > > > >>>>
> > > > >>>> Thanks
> > > > >>>> Helen
> > > > >>>>
> > > > >>>>
> > > > >>>>  drivers/gpu/drm/drm_ioctl.c | 11 +++++++++++
> > > > >>>>  include/uapi/drm/drm.h      |  1 +
> > > > >>>>  2 files changed, 12 insertions(+)
> > > > >>>>
> > > > >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > > >>>> index 94bd872d56c4..4a7e0f874171 100644
> > > > >>>> --- a/drivers/gpu/drm/drm_ioctl.c
> > > > >>>> +++ b/drivers/gpu/drm/drm_ioctl.c
> > > > >>>> @@ -31,6 +31,7 @@
> > > > >>>>  #include <drm/drm_ioctl.h>
> > > > >>>>  #include <drm/drmP.h>
> > > > >>>>  #include <drm/drm_auth.h>
> > > > >>>> +#include <drm/drm_modeset_helper_vtables.h>
> > > > >>>>  #include "drm_legacy.h"
> > > > >>>>  #include "drm_internal.h"
> > > > >>>>  #include "drm_crtc_internal.h"
> > > > >>>> @@ -229,6 +230,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > >>>>  {
> > > > >>>>      struct drm_get_cap *req = data;
> > > > >>>>      struct drm_crtc *crtc;
> > > > >>>> +    struct drm_plane *plane;
> > > > >>>>
> > > > >>>>      req->value = 0;
> > > > >>>>
> > > > >>>> @@ -292,6 +294,15 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> > > > >>>>      case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> > > > >>>>              req->value = 1;
> > > > >>>>              break;
> > > > >>>> +    case DRM_CAP_ASYNC_UPDATE:
> > > > >>>> +            req->value = 1;
> > > > >>>> +            list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> > > > >>>> +                    if (!plane->helper_private->atomic_async_update) {
> > > > >>>> +                            req->value = 0;
> > > > >>>> +                            break;
> > > > >>>> +                    }
> > > > >>>> +            }
> > > > >>>> +            break;
> > > > >>>>      default:
> > > > >>>>              return -EINVAL;
> > > > >>>>      }
> > > > >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > > >>>> index 300f336633f2..ff01540cbb1d 100644
> > > > >>>> --- a/include/uapi/drm/drm.h
> > > > >>>> +++ b/include/uapi/drm/drm.h
> > > > >>>> @@ -649,6 +649,7 @@ struct drm_gem_open {
> > > > >>>>  #define DRM_CAP_PAGE_FLIP_TARGET    0x11
> > > > >>>>  #define DRM_CAP_CRTC_IN_VBLANK_EVENT        0x12
> > > > >>>>  #define DRM_CAP_SYNCOBJ             0x13
> > > > >>>> +#define DRM_CAP_ASYNC_UPDATE                0x14
> > > > >>>>
> > > > >>>>  /** DRM_IOCTL_GET_CAP ioctl argument type */
> > > > >>>>  struct drm_get_cap {
> > > > >>>> --
> > > > >>>> 2.19.1
> > > > >>>>
> > > > >>>> _______________________________________________
> > > > >>>> dri-devel mailing list
> > > > >>>> dri-devel@lists.freedesktop.org
> > > > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >>>
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Michel Dänzer Dec. 21, 2018, 3:51 p.m. UTC | #20

On 2018-12-21 2:26 p.m., Daniel Vetter wrote:
> On Fri, Dec 21, 2018 at 10:47:27AM +0100, Michel Dänzer wrote:
>> On 2018-12-20 6:16 p.m., Michel Dänzer wrote:
>>> On 2018-12-20 6:09 p.m., Daniel Vetter wrote:
>>>> On Thu, Dec 20, 2018 at 6:03 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>>>>> On Thu, Dec 20, 2018 at 11:54 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>
>>>>>> Not sure about the gamma thing since we had opposite bugs on i915
>>>>>> about gamma not being vsynced and tearing terribly. Cursor is special
>>>>>> since it tends to be too small to notice tearing.
>>>>>
>>>>> Our cursor hw (and possibly gamma as well Nicholas?  Harry?) is double
>>>>> buffered, so we can update it any time for the most part and the
>>>>> changes won't take affect until the next vupdate period.
>>>>
>>>> Hm, I guess we could make the gamma update optionally async, and let
>>>> drivers deal. The issue is that the current async helper stuff won't
>>>> cope with gamma updates (it's aimed at plane updates only, not at crtc
>>>> property updates). Or we get userspace to do proper atomic updates. Or
>>>> we do some faking in the kernel, e.g. waiting with the gamma update
>>>> until the next atomic update happens. But that kinda breaks
>>>> ->atomic_check.
>>>
>>> Would it be possible to merge gamma changes into a pending commit, if
>>> there is one, and create a new commit otherwise?
>>>
>>> Otherwise the atomic API just moves this same problem to be solved by
>>> userspace.
>>
>> Actually, I'm not sure this is even solvable in a Xorg driver. When it
>> gets called to update the gamma LUT:
>>
>> 1. If there's a pending atomic commit, it cannot amend that, can it?
> 
> Yup, but on the kernel side we kinda have the same problem.

It's probably easier to solve in the kernel though; any solution
allowing userspace to amend commits would likely need similar changes in
the kernel anyway.


>> 2. It has no way of knowing when the next atomic commit would be
>> submitted e.g. for a page flip, so it kind of has to create its own
>> commit for the gamma update.
> 
> Block handler is what I had in mind for the fallback commit, if no other
> commit happened meanwhile (which would need to include it).

The BlockHandler runs more or less immediately after the gamma update,
after any other X11 requests submitted later by the same client and
flushed to the X server together. So this would still result in a commit
with only the gamma update most of the time. There might be a small
chance of combining with a flip with something like Night Light which is
done by the compositor itself (but still only if the compositor defers
the gamma update until just before it call SwapBuffers), basically 0
chance with something separate like RedShift.

drm: add capability DRM_CAP_ASYNC_UPDATE

Commit Message

Comments

Patch