[RFC v1 3/6] drm: Add a capability flag to support additional flip completion signalling

[Kasireddy, Vivek]

Hi Pekka,
Thank you for reviewing this patch.
 
> On Mon, 13 Sep 2021 16:35:26 -0700
> Vivek Kasireddy <vivek.kasireddy at 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 at 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 {