Re: [PATCH v2 0/8] drm/i915: refactor VLV IOSF SB for display usage

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 12 May 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, May 08, 2025 at 01:03:34PM +0300, Jani Nikula wrote:
>> This is v2 of [1] fixing a pretty bad goof-up in patch 4, adding the
>> last two patches to clean up style and ensure such goof-ups don't occur
>> in the future, and a couple of checkpatch fixes.
>> 
>> BR,
>> Jani.
>> 
>> [1] https://lore.kernel.org/r/cover.1745587049.git.jani.nikula@intel.com
>> 
>> 
>> Jani Nikula (8):
>>   drm/i915: rename vlv_sideband*.[ch] to vlv_iosf_sb*.[ch]
>>   drm/i915: add generic read/write functions for VLV IOSF SB
>>   drm/i915: rewrite VLV IOSF SB unit specific read/write functions
>>   drm/i915: switch i915 core to generic VLV IOSF SB functions
>>   drm/i915: move VLV IOSF SB unit specific helpers under display
>>   drm/i915: convert VLV IOSF SB interface to struct drm_device
>>   drm/i915: rename VLV IOSF SB ports parameter to unit_mask
>>   drm/i915: ensure correct VLV IOSF SB units have been get/put
>
> Didn't look too hard but looked all right to me. For the series
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks!

CI caught another mistake in what was supposed to be a sanity
check. vlv_dpio_get/put only gets/puts DPIO 1, but then the read/write
uses either DPIO 1 or 2 depending on the PHY, and the sanity check
complains when DPIO 2 is accessed but only DPIO 1 is locked.

I addressed this by having vlv_dpio_get/put cover both DPIO 1 and 2:


@@ -75,7 +75,7 @@ static inline void vlv_ccu_put(struct drm_device *drm)
 
 static inline void vlv_dpio_get(struct drm_device *drm)
 {
-       vlv_iosf_sb_get(drm, BIT(VLV_IOSF_SB_DPIO));
+       vlv_iosf_sb_get(drm, BIT(VLV_IOSF_SB_DPIO) | BIT(VLV_IOSF_SB_DPIO_2));
 }
 
 #ifdef I915
@@ -95,7 +95,7 @@ static inline void vlv_dpio_write(struct drm_device *drm,
 
 static inline void vlv_dpio_put(struct drm_device *drm)
 {
-       vlv_iosf_sb_put(drm, BIT(VLV_IOSF_SB_DPIO));
+       vlv_iosf_sb_put(drm, BIT(VLV_IOSF_SB_DPIO) | BIT(VLV_IOSF_SB_DPIO_2));
 }
 
 static inline void vlv_flisdsi_get(struct drm_device *drm)


v3 sent.

> AFAICS we have two warts left related to vlv/chv clocks:
> dev_priv->hpll_freq and dev_priv->czclk
>
> Not quite sure what to do about those. I suppose they are similar to
> fsb_freq and mem_freq where we either have to have a central place for
> them, or we try to duplicate the readout on both sides of fence (which
> would perhaps simplify the interface between the drivers, but would be
> annoying in other ways).

Yeah, I've been thinking about these, but decided I wanted to get this
merged first to not accummulate too many dependent series in progress.

BR,
Jani.


>
>> 
>>  drivers/gpu/drm/i915/Makefile                 |   5 +-
>>  drivers/gpu/drm/i915/display/i9xx_wm.c        |  35 ++-
>>  drivers/gpu/drm/i915/display/intel_cdclk.c    |  47 ++---
>>  drivers/gpu/drm/i915/display/intel_display.c  |  23 +-
>>  drivers/gpu/drm/i915/display/intel_display.h  |   7 +-
>>  .../drm/i915/display/intel_display_power.c    |   7 +-
>>  .../i915/display/intel_display_power_map.c    |   2 +-
>>  .../i915/display/intel_display_power_well.c   |  71 +++----
>>  drivers/gpu/drm/i915/display/intel_dpio_phy.c | 199 +++++++++---------
>>  drivers/gpu/drm/i915/display/intel_dpll.c     | 135 ++++++------
>>  drivers/gpu/drm/i915/display/vlv_dsi.c        |  26 ++-
>>  drivers/gpu/drm/i915/display/vlv_dsi_pll.c    |  46 ++--
>>  drivers/gpu/drm/i915/display/vlv_sideband.c   |  50 +++++
>>  drivers/gpu/drm/i915/display/vlv_sideband.h   | 156 ++++++++++++++
>>  drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c |   8 +-
>>  drivers/gpu/drm/i915/gt/intel_rps.c           |  58 ++---
>>  drivers/gpu/drm/i915/i915_driver.c            |   2 +-
>>  drivers/gpu/drm/i915/i915_drv.h               |   1 +
>>  drivers/gpu/drm/i915/intel_clock_gating.c     |   2 +-
>>  drivers/gpu/drm/i915/soc/intel_dram.c         |  14 +-
>>  .../i915/{vlv_sideband.c => vlv_iosf_sb.c}    | 176 ++++++----------
>>  drivers/gpu/drm/i915/vlv_iosf_sb.h            |  37 ++++
>>  .../{vlv_sideband_reg.h => vlv_iosf_sb_reg.h} |   6 +-
>>  drivers/gpu/drm/i915/vlv_sideband.h           | 125 -----------
>>  .../drm/xe/compat-i915-headers/vlv_iosf_sb.h  |  42 ++++
>>  .../{vlv_sideband_reg.h => vlv_iosf_sb_reg.h} |   2 +-
>>  .../drm/xe/compat-i915-headers/vlv_sideband.h | 132 ------------
>>  27 files changed, 689 insertions(+), 725 deletions(-)
>>  create mode 100644 drivers/gpu/drm/i915/display/vlv_sideband.c
>>  create mode 100644 drivers/gpu/drm/i915/display/vlv_sideband.h
>>  rename drivers/gpu/drm/i915/{vlv_sideband.c => vlv_iosf_sb.c} (55%)
>>  create mode 100644 drivers/gpu/drm/i915/vlv_iosf_sb.h
>>  rename drivers/gpu/drm/i915/{vlv_sideband_reg.h => vlv_iosf_sb_reg.h} (98%)
>>  delete mode 100644 drivers/gpu/drm/i915/vlv_sideband.h
>>  create mode 100644 drivers/gpu/drm/xe/compat-i915-headers/vlv_iosf_sb.h
>>  rename drivers/gpu/drm/xe/compat-i915-headers/{vlv_sideband_reg.h => vlv_iosf_sb_reg.h} (66%)
>>  delete mode 100644 drivers/gpu/drm/xe/compat-i915-headers/vlv_sideband.h
>> 
>> -- 
>> 2.39.5

-- 
Jani Nikula, Intel