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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Sent: Tuesday, August 16, 2022 1:21 PM
> To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>
> Subject: RE:  [PATCH 09/39] drm/i915: move and group gmbus
> members under display.gmbus
> 
> On Tue, 16 Aug 2022, "Murthy, Arun R" <arun.r.murthy@xxxxxxxxx> wrote:
> >> -----Original Message-----
> >> From: Nikula, Jani <jani.nikula@xxxxxxxxx>
> >> Sent: Friday, August 12, 2022 12:26 PM
> >> To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>; intel-
> >> gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>
> >> Subject: RE:  [PATCH 09/39] drm/i915: move and group gmbus
> >> members under display.gmbus
> >>
> >> On Fri, 12 Aug 2022, "Murthy, Arun R" <arun.r.murthy@xxxxxxxxx> wrote:
> >> >> -----Original Message-----
> >> >> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On
> >> >> Behalf Of Jani Nikula
> >> >> Sent: Thursday, August 11, 2022 8:37 PM
> >> >> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> >> Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>; De Marchi, Lucas
> >> >> <lucas.demarchi@xxxxxxxxx>
> >> >> Subject:  [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@xxxxxxxxx>
> >> >> ---
> >> >>  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.
> 	
Totally agree, lets take it up later after this series gets merged.

Reviewed-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>

Thanks and Regards,
Arun R Murthy
--------------------




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux