Re: [PATCH 1/9] drm/i915: Check pixel clock when setting mode for DP

[Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>]

On 7/3/2015 6:27 PM, Ville Syrjälä wrote:
> On Fri, Jul 03, 2015 at 02:35:49PM +0300, Mika Kahola wrote:
>> It is possible the we request to have a mode that has
>> higher pixel clock than our HW can support. This patch
>> checks if requested pixel clock is lower than the one
>> supported by the HW. The requested mode is discarded
>> if we cannot support the requested pixel clock.
>>
>> This patch applies to DisplayPort.
>>
>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 25 ++++++++++++++++++++++++-
>>   1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index fcc64e5..2e55dff 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -197,6 +197,26 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
>>   	return (max_link_clock * max_lanes * 8) / 10;
>>   }
>>   
>> +static int
>> +intel_dp_max_pixclk(struct intel_dp *intel_dp)
>> +{
>> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> +	struct intel_encoder *encoder = &intel_dig_port->base;
>> +	struct drm_device *dev = encoder->base.dev;
>> +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> +
>> +	if (IS_CHERRYVIEW(dev))
>> +		return  DIV_ROUND_UP(dev_priv->max_cdclk_freq * 100, 95);
> Maybe we should precompute this stuff and store as max_dotclock into dev_priv?
> It has really nothing to do with DP.
>
>> +	else if (IS_VALLEYVIEW(dev))
>> +		return  DIV_ROUND_UP(dev_priv->max_cdclk_freq * 100, 90);
>> +	else if (IS_BROADWELL(dev) && intel_crtc->config->ips_enabled)
> You can't look at the current crtc config at this point. Here we just
> need to consider the max we can support under any conditions and filter
> the modes based on that. That does mean that under some circumstances
> not all of the validated modes will actually work, but there's really
> nothing sensible we can do about that.
>
> In the specific case of IPS, we can always choose to disable it at
> modeset time if the mode would otherwise exceed the limit. So IPS is
> never a good reason for rejecting a mode.
>
>> +		return  DIV_ROUND_UP(dev_priv->max_cdclk_freq * 100, 95);
>> +	else
>> +		return dev_priv->max_cdclk_freq;
> 90% is the correct limit for all older platforms.
>
> Exceptions are CHV which has the 95% limit, and starting from HSW+
> we should be able to handle 100%.
>
> For gen2/3 we also have the option of using double wide mode which means
> the limit there should be 2 * cdclk * 9 / 10.
>
>> +}
>> +
>>   static enum drm_mode_status
>>   intel_dp_mode_valid(struct drm_connector *connector,
>>   		    struct drm_display_mode *mode)
>> @@ -206,6 +226,7 @@ intel_dp_mode_valid(struct drm_connector *connector,
>>   	struct drm_display_mode *fixed_mode = intel_connector->panel.fixed_mode;
>>   	int target_clock = mode->clock;
>>   	int max_rate, mode_rate, max_lanes, max_link_clock;
>> +	int max_pixclk;
>>   
>>   	if (is_edp(intel_dp) && fixed_mode) {
>>   		if (mode->hdisplay > fixed_mode->hdisplay)
>> @@ -223,7 +244,9 @@ intel_dp_mode_valid(struct drm_connector *connector,
>>   	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
>>   	mode_rate = intel_dp_link_required(target_clock, 18);
>>   
>> -	if (mode_rate > max_rate)
>> +	max_pixclk = intel_dp_max_pixclk(intel_dp);
>> +
>> +	if ((mode_rate > max_rate) || (target_clock > max_pixclk))
>>   		return MODE_CLOCK_HIGH;
>>   
>>   	if (mode->clock < 10000)
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
shouldn't we expect that only modes that were returned as valid through 
appropriate calls will be received as part of set_mode ?
i.e in case of dp through the intel_dp_mode_valid through the 
.mode_valid callback ?

-- 
regards,
Sivakumar

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx