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

["Kasireddy, Vivek" <vivek.kasireddy@intel.com>]

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