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

[Fernando Ramos <greenfoo@u92.eu>]

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.

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.

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?

Please let me know what you think.

Thanks!