Re: [PATCH] drm/i915: Clear the VBT defaults for unused ports

[Manasi Navare <manasi.d.navare@intel.com>]

On Tue, Mar 07, 2017 at 07:36:45PM +0200, Jani Nikula wrote:
> On Fri, 03 Mar 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Mar 03, 2017 at 05:01:42AM -0800, Manasi Navare wrote:
> >> If during VBT parsing we find that the port is unused,
> >> the driver code just bails out without clearing the
> >> defaults for that port. This can cause failures down
> >> the path through link training for unused Port.
> >> This patch fixes this issue by clearing the defaults
> >> before bailing out from the VBT parsing function.
> >> 
> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_bios.c | 10 ++++++++--
> >>  1 file changed, 8 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >> index e144f03..3ac3d24 100644
> >> --- a/drivers/gpu/drm/i915/intel_bios.c
> >> +++ b/drivers/gpu/drm/i915/intel_bios.c
> >> @@ -1148,9 +1148,15 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
> >>  			}
> >>  		}
> >>  	}
> >> -	if (!child)
> >> -		return;
> >> +	if (!child) {
> >> +		/* Clear the DDI VBT Port info values */
> >> +		info->supports_dvi = 0;
> >> +		info->supports_hdmi = 0;
> >> +		info->supports_dp = 0;
> >> +		info->supports_edp = 0;
> >
> > I would s/0/false/ here. Although they are apparently of type 'uint8_t:1'.
> > As a followup someone might want to s/uint8_t:1/bool:1/ the definitions
> > so that the compiler will protect us against values >1 when we assign
> > these.
> >
> > Otherwise lgtm. And based on a cursory glance the bogus port A has now
> > disappaered from the ci results on fi-skl-6700k, so looks like my
> > analysis of the problem was correct.
> >
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Bikeshedding after review...
> 
> I think the approach here is a bit magical, first setting defaults on
> all platforms, then removing same defaults on platforms that a) are DDI,
> b) have VBT, c) have a high enough VBT revision, and d) do not have
> child devices.
> 
> I think I'd go with simplicity, and split out a function from
> init_vbt_defaults() to be called on platforms without VBT. It would
> initially just contain the loop to initialize
> 
> 	info->supports_dvi = (port != PORT_A && port != PORT_E);
> 	info->supports_hdmi = info->supports_dvi;
> 	info->supports_dp = (port != PORT_E);
> 
> 
> BR,
> Jani.
>

How do we determine if the platforms have VBT or not?
So then in case of platforms with VBT, do not initialize them at all??

Regards
Manasi

 
> 
> >
> >>  
> >> +		return;
> >> +	}
> >>  	aux_channel = child->common.aux_channel;
> >>  	ddc_pin = child->common.ddc_pin;
> >>  
> >> -- 
> >> 2.1.4
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx