Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers

[Daniel Vetter]

On Thu, Apr 30, 2020 at 5:38 PM Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
>
> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, 28 Apr 2020, Michal Orzel <michalorzel.eng@xxxxxxxxx> wrote:
> > > As suggested by the TODO list for the kernel DRM subsystem, replace
> > > the deprecated functions that take/drop modeset locks with new helpers.
> > >
> > > Signed-off-by: Michal Orzel <michalorzel.eng@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/drm_mode_object.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> > > index 35c2719..901b078 100644
> > > --- a/drivers/gpu/drm/drm_mode_object.c
> > > +++ b/drivers/gpu/drm/drm_mode_object.c
> > > @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > >  {
> > >       struct drm_mode_obj_get_properties *arg = data;
> > >       struct drm_mode_object *obj;
> > > +     struct drm_modeset_acquire_ctx ctx;
> > >       int ret = 0;
> > >
> > >       if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > >               return -EOPNOTSUPP;
> > >
> > > -     drm_modeset_lock_all(dev);
> > > +     DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> >
> > I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and
> > DRM_MODESET_LOCK_ALL_END macros. :(
> >
> > Currently only six users... but there are ~60 calls to
> > drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder
> > if this will come back and haunt us.
> >
>
> What's the alternative? Seems like the options without the macros is
> to use incorrect scope or have a bunch of retry/backoff cargo-cult
> everywhere (and hope the copy source is done correctly).

Yeah Sean & me had a bunch of bikesheds and this is the least worst
option we could come up with. You can't make it a function because of
the control flow. You don't want to open code this because it's tricky
to get right, if all you want is to just grab all locks. But it is
magic hidden behind a macro, which occasionally ends up hurting.
-Daniel

> Sean
>
> > BR,
> > Jani.
> >
> >
> > >
> > >       obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type);
> > >       if (!obj) {
> > > @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > >  out_unref:
> > >       drm_mode_object_put(obj);
> > >  out:
> > > -     drm_modeset_unlock_all(dev);
> > > +     DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > >       return ret;
> > >  }
> > >
> > > @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > >  {
> > >       struct drm_device *dev = prop->dev;
> > >       struct drm_mode_object *ref;
> > > +     struct drm_modeset_acquire_ctx ctx;
> > >       int ret = -EINVAL;
> > >
> > >       if (!drm_property_change_valid_get(prop, prop_value, &ref))
> > >               return -EINVAL;
> > >
> > > -     drm_modeset_lock_all(dev);
> > > +     DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
> > >       switch (obj->type) {
> > >       case DRM_MODE_OBJECT_CONNECTOR:
> > >               ret = drm_connector_set_obj_prop(obj, prop, prop_value);
> > > @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj,
> > >               break;
> > >       }
> > >       drm_property_change_valid_put(prop, ref);
> > > -     drm_modeset_unlock_all(dev);
> > > +     DRM_MODESET_LOCK_ALL_END(ctx, ret);
> > >
> > >       return ret;
> > >  }
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch