[Intel-gfx] [PATCH 04/11] drm/i915/dp: Allow big joiner modes in intel_dp_mode_valid(), v3.

Maarten Lankhorst maarten.lankhorst at linux.intel.com
Tue Dec 3 09:18:51 UTC 2019


Op 28-11-2019 om 20:04 schreef Ville Syrjälä:
> On Thu, Nov 14, 2019 at 05:05:15PM +0100, Maarten Lankhorst wrote:
>> Small changes to intel_dp_mode_valid(), allow listing modes that
>> can only be supported in the bigjoiner configuration, which is
>> not supported yet.
>>
>> eDP does not support bigjoiner, so do not expose bigjoiner only
>> modes on the eDP port.
>>
>> Changes since v1:
>> - Disallow bigjoiner on eDP.
>> Changes since v2:
>> - Rename intel_dp_downstream_max_dotclock to intel_dp_max_dotclock,
>>   and split off the downstream and source checking to its own function.
>>   (Ville)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dp.c | 117 ++++++++++++++++++------
>>  1 file changed, 89 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 3123958e2081..9b7df8e85ea2 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -243,25 +243,37 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
>>  	return max_link_clock * max_lanes;
>>  }
>>  
>> -static int
>> -intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp)
>> +static int source_max_dotclock(struct intel_dp *intel_dp, bool allow_bigjoiner)
>>  {
>>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>>  	struct intel_encoder *encoder = &intel_dig_port->base;
>> -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> -	int max_dotclk = dev_priv->max_dotclk_freq;
>> -	int ds_max_dotclk;
>> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>> +
>> +	if (allow_bigjoiner && INTEL_GEN(i915) >= 11 && !intel_dp_is_edp(intel_dp))
> Should the edp check actually be check for the edp transcoder
> (ie. port A) on icl?
Isn't that equivalent to this check?
>
>> +		return 2 * i915->max_dotclk_freq;
>> +
>> +	return i915->max_dotclk_freq;
>> +}
>>  
>> +static int downstream_max_dotclock(struct intel_dp *intel_dp)
>> +{
>>  	int type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
>>  
>>  	if (type != DP_DS_PORT_TYPE_VGA)
>> -		return max_dotclk;
>> +		return 0;
>>  
>> -	ds_max_dotclk = drm_dp_downstream_max_clock(intel_dp->dpcd,
>> -						    intel_dp->downstream_ports);
>> +	return drm_dp_downstream_max_clock(intel_dp->dpcd,
>> +					   intel_dp->downstream_ports);
>> +}
>> +
>> +static int
>> +intel_dp_max_dotclock(struct intel_dp *intel_dp, bool allow_bigjoiner)
>> +{
>> +	int max_dotclk = source_max_dotclock(intel_dp, allow_bigjoiner);
>> +	int ds_max_dotclk = downstream_max_dotclock(intel_dp);
>>  
>>  	if (ds_max_dotclk != 0)
>> -		max_dotclk = min(max_dotclk, ds_max_dotclk);
>> +		return min(max_dotclk, ds_max_dotclk);
>>  
>>  	return max_dotclk;
>>  }
>> @@ -506,7 +518,8 @@ small_joiner_ram_size_bits(struct drm_i915_private *i915)
>>  
>>  static u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>  				       u32 link_clock, u32 lane_count,
>> -				       u32 mode_clock, u32 mode_hdisplay)
>> +				       u32 mode_clock, u32 mode_hdisplay,
>> +				       bool bigjoiner)
>>  {
>>  	u32 bits_per_pixel, max_bpp_small_joiner_ram;
>>  	int i;
>> @@ -524,6 +537,10 @@ static u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>  	/* Small Joiner Check: output bpp <= joiner RAM (bits) / Horiz. width */
>>  	max_bpp_small_joiner_ram = small_joiner_ram_size_bits(i915) /
>>  		mode_hdisplay;
>> +
>> +	if (bigjoiner)
>> +		max_bpp_small_joiner_ram *= 2;
>> +
>>  	DRM_DEBUG_KMS("Max small joiner bpp: %u\n", max_bpp_small_joiner_ram);
>>  
>>  	/*
>> @@ -532,6 +549,15 @@ static u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>  	 */
>>  	bits_per_pixel = min(bits_per_pixel, max_bpp_small_joiner_ram);
>>  
>> +	if (bigjoiner) {
>> +		u32 max_bpp_bigjoiner =
>> +			i915->max_cdclk_freq * 48 /
>> +			intel_dp_mode_to_fec_clock(mode_clock);
>> +
>> +		DRM_DEBUG_KMS("Max big joiner bpp: %u\n", max_bpp_bigjoiner);
>> +		bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
>> +	}
>> +
>>  	/* Error out if the max bpp is less than smallest allowed valid bpp */
>>  	if (bits_per_pixel < valid_dsc_bpp[0]) {
>>  		DRM_DEBUG_KMS("Unsupported BPP %u, min %u\n",
>> @@ -554,7 +580,8 @@ static u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>>  }
>>  
>>  static u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp,
>> -				       int mode_clock, int mode_hdisplay)
>> +				       int mode_clock, int mode_hdisplay,
>> +				       bool bigjoiner)
>>  {
>>  	u8 min_slice_count, i;
>>  	int max_slice_width;
>> @@ -579,12 +606,20 @@ static u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp,
>>  
>>  	/* Find the closest match to the valid slice count values */
>>  	for (i = 0; i < ARRAY_SIZE(valid_dsc_slicecount); i++) {
>> -		if (valid_dsc_slicecount[i] >
>> -		    drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
>> -						    false))
>> +		u8 test_slice_count = bigjoiner ?
> The test_ prefix is throwing me off. Just slice_count?
>
>> +			2 * valid_dsc_slicecount[i] :
>> +			valid_dsc_slicecount[i];
>> +
>> +		if (test_slice_count >
>> +		    drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd, false))
>>  			break;
>> -		if (min_slice_count  <= valid_dsc_slicecount[i])
>> -			return valid_dsc_slicecount[i];
>> +
>> +		/* big joiner needs small joiner to be enabled */
>> +		if (bigjoiner && test_slice_count < 4)
>> +			continue;
>> +
>> +		if (min_slice_count <= test_slice_count)
> slice_count > min_slice_count? Would feel a bit more naturalto me.
Yeah makes sense.
>> +			return test_slice_count;
>>  	}
>>  
>>  	DRM_DEBUG_KMS("Unsupported Slice Count %d\n", min_slice_count);
>> @@ -623,11 +658,15 @@ intel_dp_mode_valid(struct drm_connector *connector,
>>  	int max_dotclk;
>>  	u16 dsc_max_output_bpp = 0;
>>  	u8 dsc_slice_count = 0;
>> +	bool dsc = false, bigjoiner = false;
>>  
>>  	if (mode->flags & DRM_MODE_FLAG_DBLSCAN)
>>  		return MODE_NO_DBLESCAN;
>>  
>> -	max_dotclk = intel_dp_downstream_max_dotclock(intel_dp);
>> +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
>> +		return MODE_H_ILLEGAL;
>> +
>> +	max_dotclk = intel_dp_max_dotclock(intel_dp, false);
>>  
>>  	if (intel_dp_is_edp(intel_dp) && fixed_mode) {
>>  		if (mode->hdisplay > fixed_mode->hdisplay)
>> @@ -639,6 +678,21 @@ intel_dp_mode_valid(struct drm_connector *connector,
>>  		target_clock = fixed_mode->clock;
>>  	}
>>  
>> +	if (mode->clock < 10000)
>> +		return MODE_CLOCK_LOW;
>> +
>> +	if (target_clock > max_dotclk) {
>> +		if (intel_dp_is_edp(intel_dp))
>> +			return MODE_CLOCK_HIGH;
>> +
>> +		max_dotclk = intel_dp_max_dotclock(intel_dp, true);
>> +
>> +		if (target_clock > max_dotclk)
>> +			return MODE_CLOCK_HIGH;
>> +
>> +		bigjoiner = true;
>> +	}
>> +
>>  	max_link_clock = intel_dp_max_link_rate(intel_dp);
>>  	max_lanes = intel_dp_max_lane_count(intel_dp);
>>  
>> @@ -666,23 +720,28 @@ intel_dp_mode_valid(struct drm_connector *connector,
>>  							    max_link_clock,
>>  							    max_lanes,
>>  							    target_clock,
>> -							    mode->hdisplay) >> 4;
>> +							    mode->hdisplay,
>> +							    bigjoiner) >> 4;
>>  			dsc_slice_count =
>>  				intel_dp_dsc_get_slice_count(intel_dp,
>>  							     target_clock,
>> -							     mode->hdisplay);
>> +							     mode->hdisplay,
>> +							     bigjoiner);
>>  		}
>> +
>> +		dsc = dsc_max_output_bpp && dsc_slice_count;
>>  	}
>>  
>> -	if ((mode_rate > max_rate && !(dsc_max_output_bpp && dsc_slice_count)) ||
>> -	    target_clock > max_dotclk)
>> +	/* big joiner configuration needs DSC */
>> +	if (bigjoiner && !dsc) {
>> +		DRM_DEBUG_KMS("Link clock needs bigjoiner, but DSC or FEC not available\n");
> I don't think we spew debugs from mode_valid() elsewhere.
Good for debugging, but yeah can kill debug messages if wanted.
>>  		return MODE_CLOCK_HIGH;
>> +	}
>>  
>> -	if (mode->clock < 10000)
>> -		return MODE_CLOCK_LOW;
>> -
>> -	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
>> -		return MODE_H_ILLEGAL;
>> +	if (mode_rate > max_rate && !dsc) {
> Not a huge fan of this dsc boolean. Would feel more natural if the dsc
> thing would compute a new mode_rate with max achievable compression,
> or something along those lines. But this could be cleaned up later.
Yeah.
>
>> +		DRM_DEBUG_KMS("Cannot drive without DSC\n");
>> +		return MODE_CLOCK_HIGH;
>> +	}
>>  
>>  	return intel_mode_valid_max_plane_size(dev_priv, mode);
>>  }
>> @@ -2104,11 +2163,13 @@ static int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>  						    pipe_config->port_clock,
>>  						    pipe_config->lane_count,
>>  						    adjusted_mode->crtc_clock,
>> -						    adjusted_mode->crtc_hdisplay);
>> +						    adjusted_mode->crtc_hdisplay,
>> +						    false);
>>  		dsc_dp_slice_count =
>>  			intel_dp_dsc_get_slice_count(intel_dp,
>>  						     adjusted_mode->crtc_clock,
>> -						     adjusted_mode->crtc_hdisplay);
>> +						     adjusted_mode->crtc_hdisplay,
>> +						     false);
>>  		if (!dsc_max_output_bpp || !dsc_dp_slice_count) {
>>  			DRM_DEBUG_KMS("Compressed BPP/Slice Count not supported\n");
>>  			return -EINVAL;
>> -- 
>> 2.24.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx




More information about the Intel-gfx mailing list