On Sun, Apr 2, 2017 at 8:46 AM, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxx>
>
> 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 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
