[RFC] drm: atomic-rmfb semantics

[Daniel Vetter]

On Sat, Apr 01, 2017 at 03:11:36PM -0400, Rob Clark wrote:
> We possibly missed the boat on redefining rmfb semantics for atomic
> userspace to something more sane, unless perhaps the few existing atomic
> userspaces (CrOS?) could confirm that this change won't cause problems
> (in which case we could just call this a bug-fix, drop the cap, and
> delete some code?).
> 
> The old semantics of rmfb shutting down the display is kind of pointless
> in an atomic world, and it is more awkward for userspace that creates
> and destroys the fb every frame, since they need to defer the rmfb.
> Since we have better ways for userspace to shut down the display pipe
> and the legacy behaviour of rmfb is awkward, provide a way for atomic
> userspace to simply make rmfb an unref.
> 
> Signed-off-by: Rob Clark <robdclark at gmail.com>

We can't change existing semantics, because we reuse atomic paths for
legacy clients. But I think this here is real good. Please come up with
the userspace side in any of the atomic compositors we have, and this is
good to merge.

Well, basic igt testcases would be lovely, too. We have some to exercise
rmfb behaviour, so should be easy to adapt them.
-Daniel

> ---
>  drivers/gpu/drm/drm_framebuffer.c | 2 +-
>  drivers/gpu/drm/drm_ioctl.c       | 9 +++++++++
>  include/drm/drm_file.h            | 8 ++++++++
>  include/uapi/drm/drm.h            | 7 +++++++
>  4 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index e4909ae..c5127dd0 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -383,7 +383,7 @@ int drm_mode_rmfb(struct drm_device *dev,
>  	 * so run this in a separate stack as there's no way to correctly
>  	 * handle this after the fb is already removed from the lookup table.
>  	 */
> -	if (drm_framebuffer_read_refcount(fb) > 1) {
> +	if (drm_framebuffer_read_refcount(fb) > 1 && !file_priv->atomic_rmfb) {
>  		struct drm_mode_rmfb_work arg;
>  
>  		INIT_WORK_ONSTACK(&arg.work, drm_mode_rmfb_work_fn);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index a7c61c2..b42348f 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -318,6 +318,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  		file_priv->atomic = req->value;
>  		file_priv->universal_planes = req->value;
>  		break;
> +	case DRM_CLIENT_CAP_ATOMIC_RMFB:
> +		if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> +			return -EINVAL;
> +		if (req->value > 1)
> +			return -EINVAL;
> +		file_priv->atomic = req->value;
> +		file_priv->universal_planes = req->value;
> +		file_priv->atomic_rmfb = req->value;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 5dd27ae..2a41c29 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -181,6 +181,14 @@ struct drm_file {
>  	unsigned atomic:1;
>  
>  	/**
> +	 * @atomic_rmfb:
> +	 *
> +	 * True if client wants new semantics for rmfb, ie. simply dropping
> +	 * refcnt without tearing down the display.
> +	 */
> +	unsigned atomic_rmfb:1;
> +
> +	/**
>  	 * @is_master:
>  	 *
>  	 * This client is the creator of @master. Protected by struct
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b2c5284..4063cc8 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -678,6 +678,13 @@ struct drm_get_cap {
>   */
>  #define DRM_CLIENT_CAP_ATOMIC	3
>  
> +/**
> + * DRM_CLIENT_CAP_ATOMIC_RMFB
> + *
> + * If set to 1, the DRM core will not shutdown display pipe on rmfb ioctl.
> + */
> +#define DRM_CLIENT_CAP_ATOMIC_RMFB	4
> +
>  /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
>  struct drm_set_client_cap {
>  	__u64 capability;
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch