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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4753 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]