On Mon, Sep 08, 2025 at 02:32:28PM +0100, Tvrtko Ursulin wrote:
>
> On 08/09/2025 14:15, Luiz Otavio Mello wrote:
> > Move legacy BKL struct_mutex from drm_device to drm_i915_private, which
> > is the last remaining user.
> >
> > Signed-off-by: Luiz Otavio Mello <[email protected]>
> > Reviewed-by: Rodrigo Vivi <[email protected]>
> > ---
> > drivers/gpu/drm/drm_drv.c | 2 --
> > drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 4 ++--
> > drivers/gpu/drm/i915/i915_driver.c | 2 ++
> > drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++
> > drivers/gpu/drm/i915/i915_irq.c | 4 ++--
> > include/drm/drm_device.h | 10 ----------
> > 6 files changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index cdd591b11488..ad3aee354ba3 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -694,7 +694,6 @@ static void drm_dev_init_release(struct drm_device
> > *dev, void *res)
> > mutex_destroy(&dev->master_mutex);
> > mutex_destroy(&dev->clientlist_mutex);
> > mutex_destroy(&dev->filelist_mutex);
> > - mutex_destroy(&dev->struct_mutex);
> > }
> > static int drm_dev_init(struct drm_device *dev,
> > @@ -735,7 +734,6 @@ static int drm_dev_init(struct drm_device *dev,
> > INIT_LIST_HEAD(&dev->vblank_event_list);
> > spin_lock_init(&dev->event_lock);
> > - mutex_init(&dev->struct_mutex);
> > mutex_init(&dev->filelist_mutex);
> > mutex_init(&dev->clientlist_mutex);
> > mutex_init(&dev->master_mutex);
> > 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 09a64f224c49..65ffcaeee4b9 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> > @@ -677,7 +677,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)
> > goto out_unlock;
> > @@ -695,7 +695,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_driver.c
> > b/drivers/gpu/drm/i915/i915_driver.c
> > index c6263c6d3384..d1559fd8ad3d 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -233,6 +233,7 @@ static int i915_driver_early_probe(struct
> > drm_i915_private *dev_priv)
> > intel_sbi_init(display);
> > vlv_iosf_sb_init(dev_priv);
> > + mutex_init(&dev_priv->struct_mutex);
> > mutex_init(&dev_priv->sb_lock);
> > i915_memcpy_init_early(dev_priv);
> > @@ -291,6 +292,7 @@ static void i915_driver_late_release(struct
> > drm_i915_private *dev_priv)
> > i915_workqueues_cleanup(dev_priv);
> > mutex_destroy(&dev_priv->sb_lock);
> > + mutex_destroy(&dev_priv->struct_mutex);
> > vlv_iosf_sb_fini(dev_priv);
> > intel_sbi_fini(display);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 4e4e89746aa6..15f66a7d496d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -222,6 +222,17 @@ struct drm_i915_private {
> > bool irqs_enabled;
> > + /*
> > + * Currently, struct_mutex is only used by the i915 driver as a
> > replacement
> > + * for BKL.
>
> It's not a replacement for the BKL (i915 does not use the "good old" BKL
> since who knows when), but just a mutex (ab)used by the GuC logging and IVB
> parity interrupt work.
>
> At least the comment should be improved, or even better, if not too much
> work, consider splitting into two separate mutexes. One could probably live
> in the i915->l3_parity and other in struct intel_guc_log.
>
> [comes back later]
>
> Never mind, I see that by the end of the series you get exactly there. I'd
> say series is a bit churny and typically you wouldn't be adding and changing
> so much just to remove it, but I can live with it in this instance.
Yeap, I had made same comment and got to the same conclusion.
Can I get your ack Tvrtko so I gen merge the gem/gt related patches in this
series through drm-intel-next? or do you prefer to take this to gt-next?
I have confirmed that this version is the same one that I had submitted to
CI and got positive results.
We just need to add the missing acks from Thomas in the both patches
touching drm files.
Thanks,
Rodrigo.
>
> Regards,
>
> Tvrtko
>
> > + *
> > + * 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;
> > +
> > /* VLV/CHV IOSF sideband */
> > struct {
> > struct mutex lock; /* protect sideband access */
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 191ed8bb1d9c..cdfb09464134 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -167,7 +167,7 @@ static void ivb_parity_work(struct work_struct *work)
> > * In order to prevent a get/put style interface, acquire struct mutex
> > * any time we access those registers.
> > */
> > - mutex_lock(&dev_priv->drm.struct_mutex);
> > + mutex_lock(&dev_priv->struct_mutex);
> > /* If we've screwed up tracking, just let the interrupt fire again */
> > if (drm_WARN_ON(&dev_priv->drm, !dev_priv->l3_parity.which_slice))
> > @@ -225,7 +225,7 @@ static void ivb_parity_work(struct work_struct *work)
> > gen5_gt_enable_irq(gt, GT_PARITY_ERROR(dev_priv));
> > spin_unlock_irq(gt->irq_lock);
> > - mutex_unlock(&dev_priv->drm.struct_mutex);
> > + mutex_unlock(&dev_priv->struct_mutex);
> > }
> > static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> > index a33aedd5e9ec..016df5529d46 100644
> > --- a/include/drm/drm_device.h
> > +++ b/include/drm/drm_device.h
> > @@ -188,16 +188,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.
> > - */
> > - struct mutex struct_mutex;
> > -
> > /**
> > * @master_mutex:
> > *
>
