| Message ID | 20210913233529.3194401-4-vivek.kasireddy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add support for DRM_CAP_RELEASE_FENCE capability | expand |
On Mon, 13 Sep 2021 16:35:26 -0700 Vivek Kasireddy <vivek.kasireddy@intel.com> wrote: > If a driver supports this capability, it means that there would be an > additional signalling mechanism for a page flip completion in addition > to out_fence or DRM_MODE_PAGE_FLIP_EVENT. > > This capability may only be relevant for Virtual KMS drivers and is currently > used only by virtio-gpu. Also, it can provide a potential solution for: > https://gitlab.freedesktop.org/wayland/weston/-/issues/514 > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > --- > drivers/gpu/drm/drm_ioctl.c | 3 +++ > include/drm/drm_mode_config.h | 8 ++++++++ > include/uapi/drm/drm.h | 1 + > 3 files changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 8b8744dcf691..8a420844f8bc 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -302,6 +302,9 @@ 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_RELEASE_FENCE: > + req->value = dev->mode_config.release_fence; > + break; Hi Vivek, is this actually necessary? I would think that userspace figures out the existence of the release fence capability by seeing that the KMS property "RELEASE_FENCE_PTR" either exists or not. However, would we not need a client cap instead? If a KMS driver knows that userspace is aware of "RELEASE_FENCE_PTR" and will use it when necessary, then the KMS driver can send the pageflip completion without waiting for the host OS to signal the old buffer as free for re-use. If the KMS driver does not know that userspace can handle pageflip completing "too early", then it has no choice but to wait until the old buffer is really free before signalling pageflip completion. Wouldn't that make sense? Otherwise, this proposal sounds fine to me. Thanks, pq > default: > return -EINVAL; > } > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 12b964540069..944bebf359d7 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -935,6 +935,14 @@ struct drm_mode_config { > */ > bool normalize_zpos; > > + /** > + * @release_fence: > + * > + * If this option is set, it means there would be an additional signalling > + * mechanism for a page flip completion. > + */ > + bool release_fence; > + > /** > * @modifiers_property: Plane property to list support modifier/format > * combination. > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 3b810b53ba8b..8b8985f65581 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -767,6 +767,7 @@ struct drm_gem_open { > * Documentation/gpu/drm-mm.rst, section "DRM Sync Objects". > */ > #define DRM_CAP_SYNCOBJ_TIMELINE 0x14 > +#define DRM_CAP_RELEASE_FENCE 0x15 > > /* DRM_IOCTL_GET_CAP ioctl argument type */ > struct drm_get_cap {
Hi Pekka, Thank you for reviewing this patch. > On Mon, 13 Sep 2021 16:35:26 -0700 > Vivek Kasireddy <vivek.kasireddy@intel.com> wrote: > > > If a driver supports this capability, it means that there would be an > > additional signalling mechanism for a page flip completion in addition > > to out_fence or DRM_MODE_PAGE_FLIP_EVENT. > > > > This capability may only be relevant for Virtual KMS drivers and is currently > > used only by virtio-gpu. Also, it can provide a potential solution for: > > https://gitlab.freedesktop.org/wayland/weston/-/issues/514 > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > --- > > drivers/gpu/drm/drm_ioctl.c | 3 +++ > > include/drm/drm_mode_config.h | 8 ++++++++ > > include/uapi/drm/drm.h | 1 + > > 3 files changed, 12 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > > index 8b8744dcf691..8a420844f8bc 100644 > > --- a/drivers/gpu/drm/drm_ioctl.c > > +++ b/drivers/gpu/drm/drm_ioctl.c > > @@ -302,6 +302,9 @@ 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_RELEASE_FENCE: > > + req->value = dev->mode_config.release_fence; > > + break; > > Hi Vivek, > > is this actually necessary? > > I would think that userspace figures out the existence of the release > fence capability by seeing that the KMS property "RELEASE_FENCE_PTR" > either exists or not. [Vivek] Yeah, that makes sense. However, in order for the userspace to not see this property, we'd have to prevent drm core from exposing it; which means we need to check dev->mode_config.release_fence before attaching the property to the crtc. > > However, would we not need a client cap instead? > > If a KMS driver knows that userspace is aware of "RELEASE_FENCE_PTR" > and will use it when necessary, then the KMS driver can send the > pageflip completion without waiting for the host OS to signal the old > buffer as free for re-use. [Vivek] Right, the KMS driver can just look at whether the release_fence was added by the userspace (in the atomic commit) to determine whether it needs to wait for the old fb. > > If the KMS driver does not know that userspace can handle pageflip > completing "too early", then it has no choice but to wait until the old > buffer is really free before signalling pageflip completion. > > Wouldn't that make sense? [Vivek] Yes; DRM_CAP_RELEASE_FENCE may not be necessary to implement the behavior you suggest which makes sense. > > > Otherwise, this proposal sounds fine to me. [Vivek] Did you get a chance to review the Weston MR: https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/668 Could you please take a look? Thanks, Vivek > > > Thanks, > pq > > > > default: > > return -EINVAL; > > } > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > > index 12b964540069..944bebf359d7 100644 > > --- a/include/drm/drm_mode_config.h > > +++ b/include/drm/drm_mode_config.h > > @@ -935,6 +935,14 @@ struct drm_mode_config { > > */ > > bool normalize_zpos; > > > > + /** > > + * @release_fence: > > + * > > + * If this option is set, it means there would be an additional signalling > > + * mechanism for a page flip completion. > > + */ > > + bool release_fence; > > + > > /** > > * @modifiers_property: Plane property to list support modifier/format > > * combination. > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > index 3b810b53ba8b..8b8985f65581 100644 > > --- a/include/uapi/drm/drm.h > > +++ b/include/uapi/drm/drm.h > > @@ -767,6 +767,7 @@ struct drm_gem_open { > > * Documentation/gpu/drm-mm.rst, section "DRM Sync Objects". > > */ > > #define DRM_CAP_SYNCOBJ_TIMELINE 0x14 > > +#define DRM_CAP_RELEASE_FENCE 0x15 > > > > /* DRM_IOCTL_GET_CAP ioctl argument type */ > > struct drm_get_cap {
On Fri, 15 Oct 2021 03:37:01 +0000 "Kasireddy, Vivek" <vivek.kasireddy@intel.com> wrote: > Hi Pekka, > Thank you for reviewing this patch. > Hi Vivek! > > On Mon, 13 Sep 2021 16:35:26 -0700 > > Vivek Kasireddy <vivek.kasireddy@intel.com> wrote: > > > > > If a driver supports this capability, it means that there would be an > > > additional signalling mechanism for a page flip completion in addition > > > to out_fence or DRM_MODE_PAGE_FLIP_EVENT. > > > > > > This capability may only be relevant for Virtual KMS drivers and is currently > > > used only by virtio-gpu. Also, it can provide a potential solution for: > > > https://gitlab.freedesktop.org/wayland/weston/-/issues/514 > > > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > > --- > > > drivers/gpu/drm/drm_ioctl.c | 3 +++ > > > include/drm/drm_mode_config.h | 8 ++++++++ > > > include/uapi/drm/drm.h | 1 + > > > 3 files changed, 12 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > > > index 8b8744dcf691..8a420844f8bc 100644 > > > --- a/drivers/gpu/drm/drm_ioctl.c > > > +++ b/drivers/gpu/drm/drm_ioctl.c > > > @@ -302,6 +302,9 @@ 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_RELEASE_FENCE: > > > + req->value = dev->mode_config.release_fence; > > > + break; > > > > Hi Vivek, > > > > is this actually necessary? > > > > I would think that userspace figures out the existence of the release > > fence capability by seeing that the KMS property "RELEASE_FENCE_PTR" > > either exists or not. > [Vivek] Yeah, that makes sense. However, in order for the userspace to not see > this property, we'd have to prevent drm core from exposing it; which means we > need to check dev->mode_config.release_fence before attaching the property > to the crtc. Kernel implementation details, I don't bother with those personally. ;-) Sounds right. > > > > However, would we not need a client cap instead? > > > > If a KMS driver knows that userspace is aware of "RELEASE_FENCE_PTR" > > and will use it when necessary, then the KMS driver can send the > > pageflip completion without waiting for the host OS to signal the old > > buffer as free for re-use. > [Vivek] Right, the KMS driver can just look at whether the release_fence was > added by the userspace (in the atomic commit) to determine whether it needs > to wait for the old fb. You could do it that way, but is it a good idea? I'm not sure. > > If the KMS driver does not know that userspace can handle pageflip > > completing "too early", then it has no choice but to wait until the old > > buffer is really free before signalling pageflip completion. > > > > Wouldn't that make sense? > [Vivek] Yes; DRM_CAP_RELEASE_FENCE may not be necessary to > implement the behavior you suggest which makes sense. > > > > > > > Otherwise, this proposal sounds fine to me. > [Vivek] Did you get a chance to review the Weston MR: > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/668 > > Could you please take a look? Unfortunately I cannot promise any timely feedback on that, I try to concentrate on CM&HDR. However, I'm not the only Weston reviewer, I hope. Thanks, pq > > Thanks, > Vivek > > > > > > > Thanks, > > pq > > > > > > > default: > > > return -EINVAL; > > > } > > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > > > index 12b964540069..944bebf359d7 100644 > > > --- a/include/drm/drm_mode_config.h > > > +++ b/include/drm/drm_mode_config.h > > > @@ -935,6 +935,14 @@ struct drm_mode_config { > > > */ > > > bool normalize_zpos; > > > > > > + /** > > > + * @release_fence: > > > + * > > > + * If this option is set, it means there would be an additional signalling > > > + * mechanism for a page flip completion. > > > + */ > > > + bool release_fence; > > > + > > > /** > > > * @modifiers_property: Plane property to list support modifier/format > > > * combination. > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > > index 3b810b53ba8b..8b8985f65581 100644 > > > --- a/include/uapi/drm/drm.h > > > +++ b/include/uapi/drm/drm.h > > > @@ -767,6 +767,7 @@ struct drm_gem_open { > > > * Documentation/gpu/drm-mm.rst, section "DRM Sync Objects". > > > */ > > > #define DRM_CAP_SYNCOBJ_TIMELINE 0x14 > > > +#define DRM_CAP_RELEASE_FENCE 0x15 > > > > > > /* DRM_IOCTL_GET_CAP ioctl argument type */ > > > struct drm_get_cap { >
Hi Pekka, > > Hi Vivek! > > > > On Mon, 13 Sep 2021 16:35:26 -0700 > > > Vivek Kasireddy <vivek.kasireddy@intel.com> wrote: > > > > > > > If a driver supports this capability, it means that there would be an > > > > additional signalling mechanism for a page flip completion in addition > > > > to out_fence or DRM_MODE_PAGE_FLIP_EVENT. > > > > > > > > This capability may only be relevant for Virtual KMS drivers and is currently > > > > used only by virtio-gpu. Also, it can provide a potential solution for: > > > > https://gitlab.freedesktop.org/wayland/weston/-/issues/514 > > > > > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > > > --- > > > > drivers/gpu/drm/drm_ioctl.c | 3 +++ > > > > include/drm/drm_mode_config.h | 8 ++++++++ > > > > include/uapi/drm/drm.h | 1 + > > > > 3 files changed, 12 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > > > > index 8b8744dcf691..8a420844f8bc 100644 > > > > --- a/drivers/gpu/drm/drm_ioctl.c > > > > +++ b/drivers/gpu/drm/drm_ioctl.c > > > > @@ -302,6 +302,9 @@ 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_RELEASE_FENCE: > > > > + req->value = dev->mode_config.release_fence; > > > > + break; > > > > > > Hi Vivek, > > > > > > is this actually necessary? > > > > > > I would think that userspace figures out the existence of the release > > > fence capability by seeing that the KMS property "RELEASE_FENCE_PTR" > > > either exists or not. > > [Vivek] Yeah, that makes sense. However, in order for the userspace to not see > > this property, we'd have to prevent drm core from exposing it; which means we > > need to check dev->mode_config.release_fence before attaching the property > > to the crtc. > > Kernel implementation details, I don't bother with those personally. ;-) > > Sounds right. > > > > > > > However, would we not need a client cap instead? > > > > > > If a KMS driver knows that userspace is aware of "RELEASE_FENCE_PTR" > > > and will use it when necessary, then the KMS driver can send the > > > pageflip completion without waiting for the host OS to signal the old > > > buffer as free for re-use. > > [Vivek] Right, the KMS driver can just look at whether the release_fence was > > added by the userspace (in the atomic commit) to determine whether it needs > > to wait for the old fb. > > You could do it that way, but is it a good idea? I'm not sure. > > > > If the KMS driver does not know that userspace can handle pageflip > > > completing "too early", then it has no choice but to wait until the old > > > buffer is really free before signalling pageflip completion. > > > > > > Wouldn't that make sense? > > [Vivek] Yes; DRM_CAP_RELEASE_FENCE may not be necessary to > > implement the behavior you suggest which makes sense. > > > > > > > > > > > Otherwise, this proposal sounds fine to me. > > [Vivek] Did you get a chance to review the Weston MR: > > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/668 > > > > Could you please take a look? > > Unfortunately I cannot promise any timely feedback on that, I try to > concentrate on CM&HDR. However, I'm not the only Weston reviewer, I > hope. [Kasireddy, Vivek] I was going to say it's a small patch to review but, ok np, I'll ping Simon or Michel or Daniel. Thanks, Vivek > > > Thanks, > pq > > > > > Thanks, > > Vivek > > > > > > > > > > > Thanks, > > > pq > > > > > > > > > > default: > > > > return -EINVAL; > > > > } > > > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > > > > index 12b964540069..944bebf359d7 100644 > > > > --- a/include/drm/drm_mode_config.h > > > > +++ b/include/drm/drm_mode_config.h > > > > @@ -935,6 +935,14 @@ struct drm_mode_config { > > > > */ > > > > bool normalize_zpos; > > > > > > > > + /** > > > > + * @release_fence: > > > > + * > > > > + * If this option is set, it means there would be an additional signalling > > > > + * mechanism for a page flip completion. > > > > + */ > > > > + bool release_fence; > > > > + > > > > /** > > > > * @modifiers_property: Plane property to list support modifier/format > > > > * combination. > > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > > > index 3b810b53ba8b..8b8985f65581 100644 > > > > --- a/include/uapi/drm/drm.h > > > > +++ b/include/uapi/drm/drm.h > > > > @@ -767,6 +767,7 @@ struct drm_gem_open { > > > > * Documentation/gpu/drm-mm.rst, section "DRM Sync Objects". > > > > */ > > > > #define DRM_CAP_SYNCOBJ_TIMELINE 0x14 > > > > +#define DRM_CAP_RELEASE_FENCE 0x15 > > > > > > > > /* DRM_IOCTL_GET_CAP ioctl argument type */ > > > > struct drm_get_cap { > >
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8b8744dcf691..8a420844f8bc 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -302,6 +302,9 @@ 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_RELEASE_FENCE: + req->value = dev->mode_config.release_fence; + break; default: return -EINVAL; } diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 12b964540069..944bebf359d7 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -935,6 +935,14 @@ struct drm_mode_config { */ bool normalize_zpos; + /** + * @release_fence: + * + * If this option is set, it means there would be an additional signalling + * mechanism for a page flip completion. + */ + bool release_fence; + /** * @modifiers_property: Plane property to list support modifier/format * combination. diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3b810b53ba8b..8b8985f65581 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -767,6 +767,7 @@ struct drm_gem_open { * Documentation/gpu/drm-mm.rst, section "DRM Sync Objects". */ #define DRM_CAP_SYNCOBJ_TIMELINE 0x14 +#define DRM_CAP_RELEASE_FENCE 0x15 /* DRM_IOCTL_GET_CAP ioctl argument type */ struct drm_get_cap {
If a driver supports this capability, it means that there would be an additional signalling mechanism for a page flip completion in addition to out_fence or DRM_MODE_PAGE_FLIP_EVENT. This capability may only be relevant for Virtual KMS drivers and is currently used only by virtio-gpu. Also, it can provide a potential solution for: https://gitlab.freedesktop.org/wayland/weston/-/issues/514 Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> --- drivers/gpu/drm/drm_ioctl.c | 3 +++ include/drm/drm_mode_config.h | 8 ++++++++ include/uapi/drm/drm.h | 1 + 3 files changed, 12 insertions(+)