[RFC] drm: atomic-rmfb semantics

[Rob Clark]

On Sun, Apr 2, 2017 at 8:46 AM, Daniel Vetter <daniel at 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 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.

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