diff mbox

[v4,04/11] drm/i915: LVDS pixel clock check

Message ID 1439546611-3191-5-git-send-email-mika.kahola@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mika Kahola Aug. 14, 2015, 10:03 a.m. UTC
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 LVDS.

V2:
- removed computation for max pixel clock

V3:
- cleanup by removing unnecessary lines

V4:
- moved supported dotclock check from mode_valid() to intel_lvds_init()

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/intel_lvds.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Ville Syrjälä Aug. 14, 2015, 1:09 p.m. UTC | #1
On Fri, Aug 14, 2015 at 01:03:24PM +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 LVDS.
> 
> V2:
> - removed computation for max pixel clock
> 
> V3:
> - cleanup by removing unnecessary lines
> 
> V4:
> - moved supported dotclock check from mode_valid() to intel_lvds_init()
> 
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lvds.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 881b5d1..06b9d1b 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -1098,8 +1098,11 @@ void intel_lvds_init(struct drm_device *dev)
>  			drm_mode_debug_printmodeline(scan);
>  
>  			fixed_mode = drm_mode_duplicate(dev, scan);
> -			if (fixed_mode)
> -				goto out;
> +			if (fixed_mode) {
> +				if (fixed_mode->clock <= dev_priv->max_dotclk_freq)
> +					goto out;
> +			}
> +			fixed_mode = NULL;
>  		}
>  	}
>  
> @@ -1111,8 +1114,10 @@ void intel_lvds_init(struct drm_device *dev)
>  		fixed_mode = drm_mode_duplicate(dev, dev_priv->vbt.lfp_lvds_vbt_mode);
>  		if (fixed_mode) {
>  			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
> -			goto out;
> +			if (fixed_mode->clock <= dev_priv->max_dotclk_freq)
> +				goto out;
>  		}
> +		fixed_mode = NULL;
>  	}
>  
>  	/*
> @@ -1135,8 +1140,10 @@ void intel_lvds_init(struct drm_device *dev)
>  			DRM_DEBUG_KMS("using current (BIOS) mode: ");
>  			drm_mode_debug_printmodeline(fixed_mode);
>  			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
> -			goto out;
> +			if (fixed_mode->clock <= dev_priv->max_dotclk_freq)
> +				goto out;
>  		}
> +		fixed_mode = NULL;

I don't think we want to special case just LVDS this way. Whatever we do
with fixed_mode validation should be done for all connector types that
have it. For now I think we can more or less ignore the issue.

So in that light, your previous patch was OK except it should have just
checked fixed_mode->clock instead of mode->clock. Sorry if my ramblings
confused you too much.

>  	}
>  
>  	/* If we still don't have a mode after all that, give up. */
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Mika Kahola Aug. 14, 2015, 1:24 p.m. UTC | #2
On Fri, 2015-08-14 at 16:09 +0300, Ville Syrjälä wrote:
> On Fri, Aug 14, 2015 at 01:03:24PM +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 LVDS.
> > 
> > V2:
> > - removed computation for max pixel clock
> > 
> > V3:
> > - cleanup by removing unnecessary lines
> > 
> > V4:
> > - moved supported dotclock check from mode_valid() to intel_lvds_init()
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lvds.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 881b5d1..06b9d1b 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -1098,8 +1098,11 @@ void intel_lvds_init(struct drm_device *dev)
> >  			drm_mode_debug_printmodeline(scan);
> >  
> >  			fixed_mode = drm_mode_duplicate(dev, scan);
> > -			if (fixed_mode)
> > -				goto out;
> > +			if (fixed_mode) {
> > +				if (fixed_mode->clock <= dev_priv->max_dotclk_freq)
> > +					goto out;
> > +			}
> > +			fixed_mode = NULL;
> >  		}
> >  	}
> >  
> > @@ -1111,8 +1114,10 @@ void intel_lvds_init(struct drm_device *dev)
> >  		fixed_mode = drm_mode_duplicate(dev, dev_priv->vbt.lfp_lvds_vbt_mode);
> >  		if (fixed_mode) {
> >  			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
> > -			goto out;
> > +			if (fixed_mode->clock <= dev_priv->max_dotclk_freq)
> > +				goto out;
> >  		}
> > +		fixed_mode = NULL;
> >  	}
> >  
> >  	/*
> > @@ -1135,8 +1140,10 @@ void intel_lvds_init(struct drm_device *dev)
> >  			DRM_DEBUG_KMS("using current (BIOS) mode: ");
> >  			drm_mode_debug_printmodeline(fixed_mode);
> >  			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
> > -			goto out;
> > +			if (fixed_mode->clock <= dev_priv->max_dotclk_freq)
> > +				goto out;
> >  		}
> > +		fixed_mode = NULL;
> 
> I don't think we want to special case just LVDS this way. Whatever we do
> with fixed_mode validation should be done for all connector types that
> have it. For now I think we can more or less ignore the issue.
> 
> So in that light, your previous patch was OK except it should have just
> checked fixed_mode->clock instead of mode->clock. Sorry if my ramblings
> confused you too much.
> 
No worries. I was a bit unsure if this check should have been done in
init anyway. I'll take a step back and modify the previous patch.

-Mika-
> >  	}
> >  
> >  	/* If we still don't have a mode after all that, give up. */
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Lukas Wunner Aug. 14, 2015, 8:10 p.m. UTC | #3
Hi Mika,

On Fri, Aug 14, 2015 at 01:03:24PM +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 LVDS.

The spec lists maximum dot clocks for each connector type.
Here's the spec for IVB for instance (see section 2.3):
https://01.org/sites/default/files/documentation/ivb_ihd_os_vol3_part4.pdf

E.g. for LVDS it's 224 MHz, for DP it's 270 MHz.
If this is identical across all generations (which I haven't checked,
but seems likely), you can use these constant values, which would be
more accurate than just using the same dev_priv->max_cdclk_freq for
all connector types.

Best regards,

Lukas
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 881b5d1..06b9d1b 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -1098,8 +1098,11 @@  void intel_lvds_init(struct drm_device *dev)
 			drm_mode_debug_printmodeline(scan);
 
 			fixed_mode = drm_mode_duplicate(dev, scan);
-			if (fixed_mode)
-				goto out;
+			if (fixed_mode) {
+				if (fixed_mode->clock <= dev_priv->max_dotclk_freq)
+					goto out;
+			}
+			fixed_mode = NULL;
 		}
 	}
 
@@ -1111,8 +1114,10 @@  void intel_lvds_init(struct drm_device *dev)
 		fixed_mode = drm_mode_duplicate(dev, dev_priv->vbt.lfp_lvds_vbt_mode);
 		if (fixed_mode) {
 			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
-			goto out;
+			if (fixed_mode->clock <= dev_priv->max_dotclk_freq)
+				goto out;
 		}
+		fixed_mode = NULL;
 	}
 
 	/*
@@ -1135,8 +1140,10 @@  void intel_lvds_init(struct drm_device *dev)
 			DRM_DEBUG_KMS("using current (BIOS) mode: ");
 			drm_mode_debug_printmodeline(fixed_mode);
 			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
-			goto out;
+			if (fixed_mode->clock <= dev_priv->max_dotclk_freq)
+				goto out;
 		}
+		fixed_mode = NULL;
 	}
 
 	/* If we still don't have a mode after all that, give up. */