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

[Ville Syrjälä]

On Mon, Jul 06, 2015 at 12:15:25PM +0530, Sivakumar Thulasimani wrote:
> 
> 
> 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 at 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 at 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 ?

No, the user is free to supply a totally custom mode.

-- 
Ville Syrjälä
Intel OTC