On Fri, Jul 18, 2025 at 01:34:13PM -0300, Luiz Otavio Mello wrote:
Oi Luiz! :)
First of all thank you so much for the patch. A few comments below:
> Includes the missing file drm_device.h, which was unintentionally
> omitted in v1.
This version comment should be below, not in the beginning of the
commit message.
Most of Linux subsystems prefers version mentions to added to a place
that doesn't even appears in the merged commit later.
for that you git --format-patch
then open the file and add the revision mention below the '---' that
goes below the sign-off block.
Here in drm we are fine with the version history in the commit message
itself, but it needs to be below. between the commit message and your
sign-off.
>
> i915 is the only remaining user of struct_mutex lock.
>
> Move it from drm_device to drm_i915_private so it is only used within
> the i915 driver.
>
> Also update intel_guc_log.c to use the new location of struct_mutex.
something like this and here:
v2: Includes the missing file drm_device.h, which was unintentionally
omitted in v1.
>
> Signed-off-by: Luiz Otavio Mello <luiz.me...@estudante.ufscar.br>
Cc: John Harrison <john.c.harri...@intel.com>
Let me also cc John here since this is locking thing around GuC...
more below...
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 4 ++--
> drivers/gpu/drm/i915/i915_drv.h | 8 ++++++++
> include/drm/drm_device.h | 10 ----------
> 3 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> index e8a04e476c57..7135fdb0ebb4 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> @@ -678,7 +678,7 @@ int intel_guc_log_set_level(struct intel_guc_log *log,
> u32 level)
> if (level < GUC_LOG_LEVEL_DISABLED || level > GUC_LOG_LEVEL_MAX)
> return -EINVAL;
>
> - mutex_lock(&i915->drm.struct_mutex);
> + mutex_lock(&i915->struct_mutex);
>
> if (log->level == level)
it looks to me that this lock here is trying to protect the log->level
against races on the unlikely event of multiple callers.
so, to me the best place to have this locker is inside struct intel_guc_log.
> goto out_unlock;
> @@ -696,7 +696,7 @@ int intel_guc_log_set_level(struct intel_guc_log *log,
> u32 level)
> log->level = level;
>
> out_unlock:
> - mutex_unlock(&i915->drm.struct_mutex);
> + mutex_unlock(&i915->struct_mutex);
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d0e1980dcba2..c585569b6036 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -224,6 +224,14 @@ struct drm_i915_private {
>
> bool irqs_enabled;
>
> + /*
> + * Currently, struct_mutex is only used by the i915 driver as a
> replacement
> + * for BLK.
> + *
> + * For this reason, it is no longer part of struct drm_device.
> + */
> + struct mutex struct_mutex;
> +
> /* LPT/WPT IOSF sideband protection */
> struct mutex sbi_lock;
>
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index e2f894f1b90a..c374c58fb975 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -177,16 +177,6 @@ struct drm_device {
> /** @unique: Unique name of the device */
> char *unique;
>
> - /**
> - * @struct_mutex:
> - *
> - * Lock for others (not &drm_minor.master and &drm_file.is_master)
> - *
> - * TODO: This lock used to be the BKL of the DRM subsystem. Move the
> - * lock into i915, which is the only remaining user.
> - */
not so fast! :)
You also need to remove the other user:
drivers/gpu/drm/i915/i915_irq.c: mutex_lock(&dev_priv->drm.struct_mutex);
drivers/gpu/drm/i915/i915_irq.c:
mutex_unlock(&dev_priv->drm.struct_mutex);
Honestly, for this one here I would go with a separate patch that simply removes
this at all. I don't see that protecting anything.
But if someone insists we need to keep that, then we need to create a new
specific
lock likely:
dev_priv->l3_parity.lock
But again, I fail to see why we would need that... we are in a work queue that
is not going to be paralelized with itself.
> - struct mutex struct_mutex;
> -
> /**
> * @master_mutex:
> *
> --
> 2.50.1
>
And this is not all, you also need to remove the init and destroy for the old
one
and create new init and destroy for the new one... or use the drm managed locks
instead.
Also, please take a look to all code comments mentioning struct_mutex. Those
probably deserves some clean-up as well.
I'd say that this is a series of patches:
patch 1 - Remove struct_mutex usage in the l3parity wq
patch 2 - Create a specific guc log level lock
patch 3 - Clean up on struct_mutex outdated comments
patch 4 - Kill drm.struct_mutex for good
or something like that.
Obrigado,
Rodrigo.
