Re: [PATCH] drm/i915/cnl: Map VBT DDC Pin to BSpec DDC Pin.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 20, 2017 at 12:11:29PM +0300, Jani Nikula wrote:
> On Thu, 19 Oct 2017, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > Starting on CNL we now need to map VBT DDC Pin to
> > BSPec DDC Pin values. Not a direct translation anymore.
> >
> > According to VBT
> > Block 2 (General Bytes Definition)
> > DDC Bus
> >
> > +----------+-----------+--------------------+
> > | DDI Type | VBT Value | Bspec Mapped Value |
> > +----------+-----------+--------------------+
> > | DDI-B    | 0x1       | 0x1                |
> > | DDI-C    | 0x2       | 0x2                |
> > | DDI-D    | 0x3       | 0x4                |
> > | DDI-F    | 0x4       | 0x3                |
> > +----------+-----------+--------------------+
> >
> > v2: Move defines to a better place.
> >     This is actually CNL_PCH not CNL only.
> > v3: Accepting Ville's suggestions: enums and array to
> >     to make this future proof.
> >
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Clinton Taylor <clinton.a.taylor@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_bios.c     | 26 ++++++++++++++++----------
> >  drivers/gpu/drm/i915/intel_vbt_defs.h | 15 +++++++++++++++
> >  2 files changed, 31 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index dc4fad30bf4f..79638308664c 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -1071,6 +1071,21 @@ static void sanitize_aux_ch(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > +static const enum bspec_gmbus_pin cnp_ddc_pin_map[] = {
> > +	[DDC_BUS_DDI_B] = GMBUS_PIN_1,
> > +	[DDC_BUS_DDI_C] = GMBUS_PIN_2,
> > +	[DDC_BUS_DDI_D] = GMBUS_PIN_4,
> > +	[DDC_BUS_DDI_F] = GMBUS_PIN_3,
> 
> I think the two last lines deserve comments like /* sic */ or something.
> 
> > +};
> > +
> > +static u8 get_ddc_pin(struct drm_i915_private *dev_priv, u8 vbt_pin)
> 
> Nitpick, You're passing in a pin and you get a pin? How about
> "map_ddc_pin"? Like your array name suggests?
> 
> > +{
> > +	if (HAS_PCH_CNP(dev_priv))
> > +		return cnp_ddc_pin_map[vbt_pin];

Do we need an ARRAY_SIZE() check? I didn't actually check how many bits
we have in the VBT for this.

> > +
> > +	return vbt_pin;
> > +}
> > +
> >  static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
> >  			   u8 bdb_version)
> >  {
> > @@ -1164,16 +1179,7 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
> >  		DRM_DEBUG_KMS("Port %c is internal DP\n", port_name(port));
> >  
> >  	if (is_dvi) {
> > -		info->alternate_ddc_pin = ddc_pin;
> > -
> > -		/*
> > -		 * All VBTs that we got so far for B Stepping has this
> > -		 * information wrong for Port D. So, let's just ignore for now.
> > -		 */
> > -		if (IS_CNL_REVID(dev_priv, CNL_REVID_B0, CNL_REVID_B0) &&
> > -		    port == PORT_D) {
> > -			info->alternate_ddc_pin = 0;
> > -		}
> > +		info->alternate_ddc_pin = get_ddc_pin(dev_priv, ddc_pin);
> >  
> >  		sanitize_ddc_pin(dev_priv, port);
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h
> > index 7e2a96712d07..dfc9f7a43668 100644
> > --- a/drivers/gpu/drm/i915/intel_vbt_defs.h
> > +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h
> > @@ -308,6 +308,21 @@ struct bdb_general_features {
> >  
> >  #define LEGACY_CHILD_DEVICE_CONFIG_SIZE		33
> >  
> > +/* DDC Bus DDI Type 155+ */
> > +enum vbt_gmbus_ddi {
> > +	DDC_BUS_DDI_B = 0x1,
> > +	DDC_BUS_DDI_C,
> > +	DDC_BUS_DDI_D,
> > +	DDC_BUS_DDI_F,
> > +};
> > +
> > +enum bspec_gmbus_pin {
> > +	GMBUS_PIN_1 = 0x1,
> > +	GMBUS_PIN_2,
> > +	GMBUS_PIN_3,
> > +	GMBUS_PIN_4,
> 
> You already have these defined in i915_reg.h. The naming isn't pretty,
> but the above doesn't make much sense either.

+1

> 
> > +};
> > +
> >  /*
> >   * The child device config, aka the display device data structure, provides a
> >   * description of a port and its configuration on the platform.
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx