| Message ID | 20170401191136.20401-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Apr 1, 2017 at 3:11 PM, Rob Clark <robdclark@gmail.com> 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> > --- > 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) { oh, ignore the fact that this patch as-is would leak.. RFC cobbled together after a red-eye flight, the fact that it compiles is an accomplishment ;-) (but I still want to hear what folks think of the idea) BR, -R > 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 >
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. 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 >
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 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
On Sun, Apr 02, 2017 at 09:13:34AM -0400, Rob Clark wrote: > 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. We had a big discussion on irc, and my tldr is "no". The same way the implicit cleanup on rmfb is confusing, the implicit cleanup on close is confusing, and in most cases not what we want. We also don't clean up any other kms state, like background color, blending, gamma tables, color spaces or anything else, all of which can make your kms state entirely useless for a compositor which doesn't know. We've discussed this at great lengths about a year ago, still all relevant: http://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html Note that if all you ever run is the same compositor (and no fbdev or anything) this isn't a problem, as long as you teach your compositor to clean up it's own mess on restart. We also bikesheded the name a bit, and figured instead of a cap it's better to just have a unref_fb ioctl. And there's also no depency to atomic. -Daniel > > 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
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;
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> --- 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(-)