| Message ID | 1488546102-27789-1-git-send-email-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > > + return; > + } > aux_channel = child->common.aux_channel; > ddc_pin = child->common.ddc_pin; > > -- > 2.1.4
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. > >> >> + return; >> + } >> aux_channel = child->common.aux_channel; >> ddc_pin = child->common.ddc_pin; >> >> -- >> 2.1.4
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
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; + return; + } aux_channel = child->common.aux_channel; ddc_pin = child->common.ddc_pin;
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(-)