[Intel-gfx] [PATCH 09/39] drm/i915: move and group gmbus members under display.gmbus

[Jani Nikula]

On Tue, 16 Aug 2022, "Murthy, Arun R" <arun.r.murthy at intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani <jani.nikula at intel.com>
>> Sent: Friday, August 12, 2022 12:26 PM
>> To: Murthy, Arun R <arun.r.murthy at intel.com>; intel-
>> gfx at lists.freedesktop.org
>> Cc: De Marchi, Lucas <lucas.demarchi at intel.com>
>> Subject: RE: [Intel-gfx] [PATCH 09/39] drm/i915: move and group gmbus
>> members under display.gmbus
>>
>> On Fri, 12 Aug 2022, "Murthy, Arun R" <arun.r.murthy at intel.com> wrote:
>> >> -----Original Message-----
>> >> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf
>> >> Of Jani Nikula
>> >> Sent: Thursday, August 11, 2022 8:37 PM
>> >> To: intel-gfx at lists.freedesktop.org
>> >> Cc: Nikula, Jani <jani.nikula at intel.com>; De Marchi, Lucas
>> >> <lucas.demarchi at intel.com>
>> >> Subject: [Intel-gfx] [PATCH 09/39] drm/i915: move and group gmbus
>> >> members under display.gmbus
>> >>
>> >> Move display related members under drm_i915_private display sub-
>> struct.
>> >>
>> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/display/intel_cdclk.c    |  6 +--
>> >>  .../gpu/drm/i915/display/intel_display_core.h | 23 ++++++++++
>> >>  drivers/gpu/drm/i915/display/intel_dp_aux.c   |  2 +-
>> >>  drivers/gpu/drm/i915/display/intel_gmbus.c    | 46 +++++++++----------
>> >>  drivers/gpu/drm/i915/i915_drv.h               | 16 -------
>> >>  drivers/gpu/drm/i915/i915_irq.c               |  4 +-
>> >>  drivers/gpu/drm/i915/i915_reg.h               | 14 +++---
>> >>  7 files changed, 59 insertions(+), 52 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >> b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >> index 6095f5800a2e..ea40c75c2986 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >> @@ -2098,12 +2098,12 @@ static void intel_set_cdclk(struct
>> >> drm_i915_private *dev_priv,
>> >>        * functions use cdclk. Not all platforms/ports do,
>> >>        * but we'll lock them all for simplicity.
>> >>        */
>> >> -     mutex_lock(&dev_priv->gmbus_mutex);
>> >> +     mutex_lock(&dev_priv->display.gmbus.mutex);
>> >>       for_each_intel_dp(&dev_priv->drm, encoder) {
>> >>               struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>> >>
>> >>               mutex_lock_nest_lock(&intel_dp->aux.hw_mutex,
>> >> -                                  &dev_priv->gmbus_mutex);
>> >> +                                  &dev_priv->display.gmbus.mutex);
>> >>       }
>> >>
>> >>       intel_cdclk_set_cdclk(dev_priv, cdclk_config, pipe); @@ -2113,7
>> >> +2113,7 @@ static void intel_set_cdclk(struct drm_i915_private
>> >> +*dev_priv,
>> >>
>> >>               mutex_unlock(&intel_dp->aux.hw_mutex);
>> >>       }
>> >> -     mutex_unlock(&dev_priv->gmbus_mutex);
>> >> +     mutex_unlock(&dev_priv->display.gmbus.mutex);
>> >>
>> >>       for_each_intel_encoder_with_psr(&dev_priv->drm, encoder) {
>> >>               struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>> >> diff -- git a/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> b/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> index 306584c038c9..fe19d4f9a9ab 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> @@ -6,7 +6,11 @@
>> >>  #ifndef __INTEL_DISPLAY_CORE_H__
>> >>  #define __INTEL_DISPLAY_CORE_H__
>> >>
>> >> +#include <linux/mutex.h>
>> >>  #include <linux/types.h>
>> >> +#include <linux/wait.h>
>> >> +
>> >> +#include "intel_gmbus.h"
>> >>
>> >>  struct drm_i915_private;
>> >>  struct intel_atomic_state;
>> >> @@ -78,6 +82,25 @@ struct intel_display {
>> >>               /* Display internal color functions */
>> >>               const struct intel_color_funcs *color;
>> >>       } funcs;
>> >> +
>> >> +     /* Grouping using anonymous structs. Keep sorted. */
>> >> +     struct {
>> >> +             /*
>> >> +              * Base address of where the gmbus and gpio blocks are
>> >> located
>> >> +              * (either on PCH or on SoC for platforms without PCH).
>> >> +              */
>> >> +             u32 mmio_base;
>> >> +
>> >> +             /*
>> >> +              * gmbus.mutex protects against concurrent usage of the
>> >> single
>> >> +              * hw gmbus controller on different i2c buses.
>> >> +              */
>> >> +             struct mutex mutex;
>> >> +
>> >> +             struct intel_gmbus *bus[GMBUS_NUM_PINS];
>> >> +
>> >> +             wait_queue_head_t wait_queue;
>> >> +     } gmbus;
>> >>  };
>> >
>> > Can this be moved to struct intel_dp?
>>
>> Well, no. The data here is shared across all of them.
>>
> Then maybe we might need to think on this. I somehow feel having this under display wont look good. Since it's a low level bus communication, can be tagged under bus related to interface.
> I agree from your other comments that this is just a first step to cleanup i915 and there is scope to cleanup further.

As I've explained elsewhere, the "display" here should be taken to be
very high level. Not "a display device", but "everything related to
display hardware". Only display needs gmbus, nothing else.

There may be further cleanups to be made, but this series has a very
specific purpose of splitting the display parts to a display sub-struct
of drm_i915_private. This is already 39 patches, it's not even complete
on that target, so need to not lose focus here.

BR,
Jani.



>
> Reviewed-by: Arun R Murthy <arun.r.murthy at intel.com>
>
> Thanks and Regards,
> Arun R Murthy
> -------------------

-- 
Jani Nikula, Intel Open Source Graphics Center