[Intel-gfx] [PATCH] drm/i915/cnp: Properly handle VBT ddc pin out of bounds.

[Rodrigo Vivi]

On Thu, Jan 25, 2018 at 06:25:06PM +0000, Rodrigo Vivi wrote:
> On Thu, Jan 25, 2018 at 06:07:19PM +0000, Lucas De Marchi wrote:
> > On Thu, Jan 25, 2018 at 05:52:26PM +0200, Jani Nikula wrote:
> > > On Thu, 25 Jan 2018, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
> > > > If the table result is out of bounds on the array map
> > > > there is something really wrong with VBT pin so we don't
> > > > return that vbt_pin, but only return 0 instead.
> > > >
> > > > This basically reverts commit 'a8e6f3888b05 ("drm/i915/cnp:
> > > > Ignore VBT request for know invalid DDC pin.")'
> > > >
> > > > Also this properly fixes commit 9c3b2689d01f ("drm/i915/cnl:
> > > > Map VBT DDC Pin to BSpec DDC Pin.")
> > > >
> > > > v2: Do in a way that we don't break other platforms. (Jani)
> > > > v3: Keep debug message (Jani)
> > > >
> > > > Fixes: a8e6f3888b05 ("drm/i915/cnp: Ignore VBT request for know invalid DDC pin.")
> > > > Fixes: 9c3b2689d01f ("drm/i915/cnl: Map VBT DDC Pin to BSpec DDC Pin.")
> > > > Cc: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > > > Cc: Jani Nikula <jani.nikula at intel.com>
> > > > Cc: Kai Heng Feng <kai.heng.feng at canonical.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_bios.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > > > index 95f0b310d656..06526b17a011 100644
> > > > --- a/drivers/gpu/drm/i915/intel_bios.c
> > > > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > > > @@ -1116,9 +1116,9 @@ static const u8 cnp_ddc_pin_map[] = {
> > > >  static u8 map_ddc_pin(struct drm_i915_private *dev_priv, u8 vbt_pin)
> > > >  {
> > > >  	if (HAS_PCH_CNP(dev_priv)) {
> > > > -		if (vbt_pin > 0 && vbt_pin < ARRAY_SIZE(cnp_ddc_pin_map))
> > > > +		if (vbt_pin > 0 && vbt_pin < ARRAY_SIZE(cnp_ddc_pin_map)) {
> > > >  			return cnp_ddc_pin_map[vbt_pin];
> > > > -		if (vbt_pin > GMBUS_PIN_4_CNP) {
> > > > +		} else {
> > > 
> > > You're going to hate me, but I just realized this will now complain
> > > about vbt_pin == 0, which I guess is valid for N/A.
> > > 
> > > Why are simple things so hard sometimes... :(
> > > 
> > > else if (vbt_pin) ?
> > 
> > vpt_pin is unsigned so we are just special-casing the vpt_pin == 0
> > already. What about stopping doing that by adding it to the array, like
> > below (to be squashed)
> 
> That's a very good suggestion for the cnp part.
> cleaner...
> 
> and we don't want to make cnp more difficult because of icp...
> but if you also have a good idea for that icp case please raise that!

Sorry for the noise. As you showed and convinced me here this simple
solution already solves the icl case without needing to check for both sides
of the map.

> 
> > 
> > -------
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index 06526b17a011..cf3f8f1ba6f7 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -1107,6 +1107,7 @@ static void sanitize_aux_ch(struct drm_i915_private *dev_priv,
> >  }
> >  
> >  static const u8 cnp_ddc_pin_map[] = {
> > +	[0] = 0, /* N/A */
> 
> N/A for Not Alternate? hehe
> 
> maybe:
> +	[0] = 0, /* Per platform default */
> 
> >  	[DDC_BUS_DDI_B] = GMBUS_PIN_1_BXT,
> >  	[DDC_BUS_DDI_C] = GMBUS_PIN_2_BXT,
> >  	[DDC_BUS_DDI_D] = GMBUS_PIN_4_CNP, /* sic */
> > @@ -1116,7 +1117,7 @@ static const u8 cnp_ddc_pin_map[] = {
> >  static u8 map_ddc_pin(struct drm_i915_private *dev_priv, u8 vbt_pin)
> >  {
> >  	if (HAS_PCH_CNP(dev_priv)) {
> > -		if (vbt_pin > 0 && vbt_pin < ARRAY_SIZE(cnp_ddc_pin_map)) {
> > +		if (vbt_pin < ARRAY_SIZE(cnp_ddc_pin_map)) {
> >  			return cnp_ddc_pin_map[vbt_pin];
> >  		} else {
> >  			DRM_DEBUG_KMS("Ignoring alternate pin: VBT claims DDC pin %d, which is not valid for this platform\n", vbt_pin);
> > -------
> > 
> > 
> > Lucas De Marchi