Re: [PATCH] drm/i915/icl: remove port A/E lane sharing limitation.

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

On Mon, 05 Feb 2018, "Kumar, Mahesh" <mahesh1.kumar@intel.com> wrote:
> Hi,
>
>
> On 2/2/2018 6:26 PM, Jani Nikula wrote:
>> On Fri, 02 Feb 2018, Mahesh Kumar <mahesh1.kumar@intel.com> wrote:
>>> Platforms before Gen11 were sharing lanes between port-A & port-E.
>>> This limitation is no more there.
>>>
>>> Changes since V1:
>>>   - optimize the code (Shashank/Jani)
>>>   - create helper function to get max lanes (ville)
>>> Changes since V2:
>>>   - Include BIOS fail fix-up in same helper function (ville)
>>>
>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>>> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_ddi.c | 71 +++++++++++++++++++---------------------
>>>   1 file changed, 33 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>> index cfcd9cb37d5d..ee9ba78d19c8 100644
>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>> @@ -2842,39 +2842,44 @@ static bool intel_ddi_a_force_4_lanes(struct intel_digital_port *dport)
>>>   	return false;
>>>   }
>>>   
>>> +static int
>>> +intel_ddi_max_lanes(struct drm_i915_private *dev_priv,
>>> +		    struct intel_digital_port *intel_dig_port)
>>> +{
>> Please ditch the dev_priv parameter and add:
>>
>> 	struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev);
> ok, sure.
>>
>>> +	enum port port = intel_dig_port->base.port;
>>> +	int max_lanes = 4;
>> Unnecessary initialization.
> for ports other than PORT_A/E will have max_lanes=4 that's the reason 
> initializing it here, will fix the usages.
>>
>>> +
>>> +	if (INTEL_GEN(dev_priv) >= 11) {
>>> +		return 4;
>> Please either set max_lanes = 4 here, or remove the else. On early
>> returns, you don't need the else. Having both is confusing.
> yes, agree, will remove the else
>>
>>> +	} else if (port == PORT_A || port == PORT_E) {
>>> +		if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES)
>>> +			max_lanes = port == PORT_A ? 4 : 0;
>>> +		else
>>> +			/* Both A and E share 2 lanes */
>>> +			max_lanes = 2;
>>> +	}
>>> +
>>> +	/*
>>> +	 * Some BIOS might fail to set this bit on port A if eDP
>>> +	 * wasn't lit up at boot.  Force this bit set when needed
>>> +	 * so we use the proper lane count for our calculations.
>>> +	 */
>>> +	if (intel_ddi_a_force_4_lanes(intel_dig_port)) {
>>> +		DRM_DEBUG_KMS("Forcing DDI_A_4_LANES for port A\n");
>>> +		intel_dig_port->saved_port_bits |= DDI_A_4_LANES;
>>> +		max_lanes = 4;
>>> +	}
>>> +
>>> +	return max_lanes;
>>> +}
>>> +
>>>   void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>>>   {
>>>   	struct intel_digital_port *intel_dig_port;
>>>   	struct intel_encoder *intel_encoder;
>>>   	struct drm_encoder *encoder;
>>>   	bool init_hdmi, init_dp, init_lspcon = false;
>>> -	int max_lanes;
>>>   
>>> -	if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) {
>>> -		switch (port) {
>>> -		case PORT_A:
>>> -			max_lanes = 4;
>>> -			break;
>>> -		case PORT_E:
>>> -			max_lanes = 0;
>>> -			break;
>>> -		default:
>>> -			max_lanes = 4;
>>> -			break;
>>> -		}
>>> -	} else {
>>> -		switch (port) {
>>> -		case PORT_A:
>>> -			max_lanes = 2;
>>> -			break;
>>> -		case PORT_E:
>>> -			max_lanes = 2;
>>> -			break;
>>> -		default:
>>> -			max_lanes = 4;
>>> -			break;
>>> -		}
>>> -	}
>>>   
>>>   	init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi ||
>>>   		     dev_priv->vbt.ddi_port_info[port].supports_hdmi);
>>> @@ -2954,19 +2959,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>>>   		MISSING_CASE(port);
>>>   	}
>>>   
>>> -	/*
>>> -	 * Some BIOS might fail to set this bit on port A if eDP
>>> -	 * wasn't lit up at boot.  Force this bit set when needed
>>> -	 * so we use the proper lane count for our calculations.
>>> -	 */
>>> -	if (intel_ddi_a_force_4_lanes(intel_dig_port)) {
>>> -		DRM_DEBUG_KMS("Forcing DDI_A_4_LANES for port A\n");
>>> -		intel_dig_port->saved_port_bits |= DDI_A_4_LANES;
>>> -		max_lanes = 4;
>>> -	}
>>> -
>>>   	intel_dig_port->dp.output_reg = INVALID_MMIO_REG;
>>> -	intel_dig_port->max_lanes = max_lanes;
>>>   
>>>   	intel_encoder->type = INTEL_OUTPUT_DDI;
>>>   	intel_encoder->power_domain = intel_port_to_power_domain(port);
>>> @@ -2974,6 +2967,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>>>   	intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
>>>   	intel_encoder->cloneable = 0;
>>>   
>>> +	intel_dig_port->max_lanes = intel_ddi_max_lanes(dev_priv,
>>> +							intel_dig_port);
>> Please keep this at the original location above.
> I moved this here because intel_encoder->port was not initialized in 
> original location, will pass port along with intel_dig_port & move it to 
> original location.

Right, so alternatively you could move all the intel_dig_port init to
the same place.

BR,
Jani.

> Thanks for review.
>
> -Mahesh
>>
>> BR,
>> Jani.
>>
>>>   	intel_infoframe_init(intel_dig_port);
>>>   
>>>   	if (init_dp) {
>

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