Re: [Intel-gfx] [PATCH 5/9] drm/i915/display: move icl to description-based ddi init

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 23 Dec 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> By adding a hook that determines if a port is present, we are able to
> support Ice Lake in the new description-based DDI initialization.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 61 ++++++++++++++------
>  1 file changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index b3fb1e03cb0b..6b4d320ff92c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -16224,9 +16224,28 @@ static void intel_pps_init(struct drm_i915_private *dev_priv)
>  struct intel_output {
>  	/* Initialize DSI if present */
>  	void (*dsi_init)(struct drm_i915_private *i915);
> +
> +	/*
> +	 * Check if port is present before trying to initialize; if not provided
> +	 * it's assumed the port is present (or we can't check and fail
> +	 * gracefully
> +	 */
> +	bool (*is_port_present)(struct drm_i915_private *i915,
> +				const struct intel_ddi_port_info *port_info);
> +
>  	struct intel_ddi_port_info ddi_ports[];
>  };
>  
> +static bool icl_is_port_present(struct drm_i915_private *i915,
> +				const struct intel_ddi_port_info *port_info)
> +{
> +	if (port_info->port != PORT_F)
> +		return true;
> +
> +	return IS_ICL_WITH_PORT_F(i915) &&
> +		intel_bios_is_port_present(i915, PORT_F);
> +}
> +

You know, all of that is here because there were some boards with broken
VBTs claiming there was a port F on hardware that didn't have port
F. And now we're turning it into infrastructure for all platforms. :(

I actually preferred it when it was a localized hack for ICL. (Though I
said at the time we should not add hacks for VBTs because this shit
won't get fixed if we keep accommodating it.)

If we still need the port F hack, I think I'd rather move it to
intel_bios.c and skip port F description in VBT for platformsm that
don't have it. So we can rely on VBT info elsewhere.

Note that intel_ddi_init() will still check for VBT.

>  static const struct intel_output tgl_output = {
>  	.dsi_init = icl_dsi_init,
>  	.ddi_ports = {
> @@ -16242,6 +16261,20 @@ static const struct intel_output tgl_output = {
>  	}
>  };
>  
> +static const struct intel_output icl_output = {
> +	.dsi_init = icl_dsi_init,
> +	.is_port_present = icl_is_port_present,
> +	.ddi_ports = {
> +		{ .port = PORT_A },
> +		{ .port = PORT_B },
> +		{ .port = PORT_C },
> +		{ .port = PORT_D },
> +		{ .port = PORT_E },
> +		{ .port = PORT_F },
> +		{ .port = PORT_NONE }

At this stage of the series it seems to me we could have a ports mask in
intel_device_info, and just loop over it using for_each_port_masked().

BR,
Jani.

> +	}
> +};
> +
>  static const struct intel_output ehl_output = {
>  	.dsi_init = icl_dsi_init,
>  	.ddi_ports = {
> @@ -16276,12 +16309,19 @@ static void setup_ddi_outputs_desc(struct drm_i915_private *i915)
>  		output = &tgl_output;
>  	else if (IS_ELKHARTLAKE(i915))
>  		output = &ehl_output;
> +	else if (IS_GEN(i915, 11))
> +		output = &icl_output;
>  	else if (IS_GEN9_LP(i915))
>  		output = &gen9lp_output;
>  
>  	for (port_info = output->ddi_ports;
> -	     port_info->port != PORT_NONE; port_info++)
> +	     port_info->port != PORT_NONE; port_info++) {
> +		if (output->is_port_present &&
> +		    !output->is_port_present(i915, port_info))
> +			continue;
> +
>  		intel_ddi_init(i915, port_info->port);
> +	}
>  
>  	if (output->dsi_init)
>  		output->dsi_init(i915);
> @@ -16297,25 +16337,8 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>  	if (!HAS_DISPLAY(dev_priv) || !INTEL_DISPLAY_ENABLED(dev_priv))
>  		return;
>  
> -	if (INTEL_GEN(dev_priv) >= 12 || IS_ELKHARTLAKE(dev_priv) ||
> -	    IS_GEN9_LP(dev_priv)) {
> +	if (INTEL_GEN(dev_priv) >= 11 || IS_GEN9_LP(dev_priv)) {
>  		setup_ddi_outputs_desc(dev_priv);
> -	} else if (IS_GEN(dev_priv, 11)) {
> -		intel_ddi_init(dev_priv, PORT_A);
> -		intel_ddi_init(dev_priv, PORT_B);
> -		intel_ddi_init(dev_priv, PORT_C);
> -		intel_ddi_init(dev_priv, PORT_D);
> -		intel_ddi_init(dev_priv, PORT_E);
> -		/*
> -		 * On some ICL SKUs port F is not present. No strap bits for
> -		 * this, so rely on VBT.
> -		 * Work around broken VBTs on SKUs known to have no port F.
> -		 */
> -		if (IS_ICL_WITH_PORT_F(dev_priv) &&
> -		    intel_bios_is_port_present(dev_priv, PORT_F))
> -			intel_ddi_init(dev_priv, PORT_F);
> -
> -		icl_dsi_init(dev_priv);
>  	} else if (HAS_DDI(dev_priv)) {
>  		int found;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx