On Sun, Apr 2, 2017 at 8:46 AM, Daniel Vetter daniel@ffwll.ch wrote:
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@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.
So, since this is done in rmfb ioctl fxn, I don't think it effects internal use. It is really just making rmfb ioctl into an unref.
But, I'm thinking we should leave the fb on the file_priv->fbs list so it gets cleaned up (and legacy behaviour of shutting down pipe is preserved) at lastclose time..
And a small related change in drm_framebuffer_free() to make sure it is removed from file_priv->fbs list.
BR, -R
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 structdiff --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