| Message ID | 1435923357-3821-2-git-send-email-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 ?
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@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 ? No, the user is free to supply a totally custom mode.
On Fri, 2015-07-03 at 15:57 +0300, 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. > Thanks for the comments. I rephrase this patch series and store the dotclock in dev_priv. This way, I hope, I can get rid of the repetitive code what Chris pointed out. Cheers, Mika > > +} > > + > > 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 >
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); + 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) + return DIV_ROUND_UP(dev_priv->max_cdclk_freq * 100, 95); + else + return dev_priv->max_cdclk_freq; +} + 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)
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(-)