Re: [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible

Ville Syrjälä
2021-10-04T08:01:15Z

On Sat, Oct 02, 2021 at 07:28:02PM +0200, Fernando Ramos wrote:
> On 21/10/02 09:13AM, Fernando Ramos wrote:
> > On 21/10/02 05:30AM, Ville Syrjälä wrote:
> > > On Sat, Oct 02, 2021 at 01:05:47AM +0300, Ville Syrjälä wrote:
> > > > On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote:
> > > > > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> > > > > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > > > > > >
> > > > > > > Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along
> > > > > > > with the necessary drm-tip conflict resolutions).
> > > > > >
> > > > > > Ugh. Did anyone actually review the locking changes this does?
> > > > > > I shot the previous i915 stuff down because the commit messages
> > > > > > did not address any of it.
> > > > >
> > > > > I reviewed the set on 9/17, I didn't see your feedback on that thread.
> > > >
> > > > It was much earlir than that.
> > > > https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html
>
> Sorry, I'm new to this and it did not occur to me to search for similar patches
> in the mailing list archives in case there were additional comments that applied
> to my change set.
>
> In case I had done that I would have found that, as you mentioned, you had
> already raised two issues back in June:
>
> On Tue, Jun 29, 2021, Ville Syrjälä wrote:
> >
> > That looks wrong. You're using a private ctx here, but still
> > passing dev->mode_config.acquire_ctx to the lower level stuff.
> >
> > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be
> > equivalent to drm_modeset_{lock,unlock}_all() when it comes to
> > mode_config.mutex. So would need a proper review whether we
> > actually need that lock or not.
>
> The first one was pointing out the same error I would later repeat in my patch
> series (ups).
>
> After further inspection of the code it looks to me that changing this:
>
> intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
>
> ...into this:
>
> intel_modeset_setup_hw_state(dev, &ctx);
>
> ...would be enough.

Yes.

>
> Why? The only difference between the old drm_modeset_{lock,unlock}_all()
> functions and the new DRM_MODESET_LOCK_ALL_{BEGIN,END}() macros is that the
> former use a global context stored in dev->mode_config.acquire_ctx while the
> latter depend on a user provided one (typically in the stack).
>
> In the old (working) code the global context structure is freed in
> drm_modeset_unlock_all() thus we are sure no one is holding a reference to it at
> that point. This means that as long as no one accesses the global
> dev->mode_config.acquire_ctx context in the block that runs between lock/BEGIN
> and unlock/END, the code should be equivalent before and after my changes.
>
> In fact, now that my patch series removes the drm_modeset_{lock,unlock}_all()
> functions, the acquire_ctx field of the drm_mode_config structure should be
> deleted:
>
> /**
> * @acquire_ctx:
> *
> * Global implicit acquire context used by atomic drivers for legacy
> * IOCTLs. Deprecated, since implicit locking contexts make it
> * impossible to use driver-private &struct drm_modeset_lock. Users of
> * this must hold @mutex.
> */
> struct drm_modeset_acquire_ctx *acquire_ctx;
>
> If I had done that (ie. removing this field) I would have detected the problem
> when compiling.
>
> There is another place (in the amdgpu driver) where this field is still being
> referenced, but before I investigate that I would like to know if you agree that
> this is a good path to follow.

Yeah, removing the mode_config.acquire_ctx is a good idea if it's
no longer needed.

>
> Regarding the second issue you raised...
>
> > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be
> > equivalent to drm_modeset_{lock,unlock}_all() when it comes to
> > mode_config.mutex. So would need a proper review whether we
> > actually need that lock or not.
>
> ...the only difference regarding mode_config.mutex I see is that in the new
> macros the mutex is locked only under this condition:
>
> if (!drm_drv_uses_atomic_modeset(dev))
>
> ...which seems reasonable, right? Is this what you were referring to or is it
> something else?

In order to eliminate the lock one first has to determine what that lock
might be protecting here, and then prove that such protection is not
actually needed.

--
Ville Syrjälä
Intel