Re: [PATCH] drm/i915/vbt: ignore extraneous child devices for a port

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

On Fri, Aug 11, 2017 at 02:39:07PM +0300, Jani Nikula wrote:
> Ever since we've parsed VBT child devices, starting from 6acab15a7b0d
> ("drm/i915: use the HDMI DDI buffer translations from VBT"), we've
> ignored the child device information if more than one child device
> references the same port. The rationale for this seems lost in time.
> 
> Since commit 311a20949f04 ("drm/i915: don't init DP or HDMI when not
> supported by DDI port") we started using this information more to skip
> HDMI/DP init if the port wasn't there per VBT child devices. However, at
> the same time it added port defaults without further explanation.
> 
> Thus, if the child device info was skipped due to multiple child devices
> referencing the same port, the device info would be retrieved from the
> somewhat arbitrary defaults.
> 
> Finally, when commit bb1d132935c2 ("drm/i915/vbt: split out defaults
> that are set when there is no VBT") stopped initializing the defaults
> whenever VBT is present, thus trusting the VBT more, we stopped
> initializing ports which were referenced by more than one child device.
> 
> Apparently at least Asus UX305UA, UX305U, and UX306U laptops have VBT
> child device blocks which cause this behaviour. Arguably they were
> shipped with a broken VBT.
> 
> Relax the rules for multiple references to the same port, and use the
> first child device info to reference a port. Retain the logic to debug
> log about this, though.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101745
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=196233
> Fixes: bb1d132935c2 ("drm/i915/vbt: split out defaults that are set when there is no VBT")
> Tested-by: Oliver Weißbarth <mail@oweissbarth.de>
> Reported-by: Oliver Weißbarth <mail@oweissbarth.de>
> Reported-by: Didier G <didierg-divers@orange.fr>
> Reported-by: Giles Anderson <agander@gmail.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: <stable@vger.kernel.org> # v4.12+
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 82b144cdfa1d..183e87e8ea31 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1120,8 +1120,8 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
>  	bool is_dvi, is_hdmi, is_dp, is_edp, is_crt;
>  	uint8_t aux_channel, ddc_pin;
>  	/* Each DDI port can have more than one value on the "DVO Port" field,
> -	 * so look for all the possible values for each port and abort if more
> -	 * than one is found. */
> +	 * so look for all the possible values for each port.
> +	 */
>  	int dvo_ports[][3] = {
>  		{DVO_PORT_HDMIA, DVO_PORT_DPA, -1},
>  		{DVO_PORT_HDMIB, DVO_PORT_DPB, -1},
> @@ -1130,7 +1130,10 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
>  		{DVO_PORT_CRT, DVO_PORT_HDMIE, DVO_PORT_DPE},
>  	};
>  
> -	/* Find the child device to use, abort if more than one found. */
> +	/*
> +	 * Find the first child device to reference the port, report if more
> +	 * than one found.
> +	 */
>  	for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
>  		it = dev_priv->vbt.child_dev + i;
>  
> @@ -1140,11 +1143,11 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
>  
>  			if (it->common.dvo_port == dvo_ports[port][j]) {
>  				if (child) {
> -					DRM_DEBUG_KMS("More than one child device for port %c in VBT.\n",
> +					DRM_DEBUG_KMS("More than one child device for port %c in VBT, using the first.\n",
>  						      port_name(port));
> -					return;

So the bug here was that in case of port referenced by multiple child devices because it would return from the function,
it would skip the initialization of flags like is_dp/is_hdmi?
It almost feels like they meant to have a break; here instead of return.
Now that this patch removes return it will still iterate through all the chile devices even
after finding second refernce to the same port, isnt that unnecessary?
Would adding a break there instead optimize it?

Regards
Manasi

> +				} else {
> +					child = it;
>  				}
> -				child = it;
>  			}
>  		}
>  	}
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx