[v6,2/8] drm/i915: Use cached cdclk value

Submitted by Mika Kahola on June 3, 2015, 12:45 p.m.

Details

Message ID 1433335514-4156-3-git-send-email-mika.kahola@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Mika Kahola June 3, 2015, 12:45 p.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Rather than reading out the current cdclk value use the cached value we
have tucked away in dev_priv.

v2: Rebased to the latest
v3: Rebased to the latest
v4: Fix for patch style problems

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kahola <mika.kahola@intel.com>

Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +--
 drivers/gpu/drm/i915/intel_dp.c      | 5 +++--
 drivers/gpu/drm/i915/intel_pm.c      | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9cf1553..d1dd8ab 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6610,8 +6610,7 @@  static int intel_crtc_compute_config(struct intel_crtc *crtc,
 
 	/* FIXME should check pixel clock limits on all platforms */
 	if (INTEL_INFO(dev)->gen < 4) {
-		int clock_limit =
-			dev_priv->display.get_display_clock_speed(dev);
+		int clock_limit = dev_priv->cdclk_freq;
 
 		/*
 		 * Enable pixel doubling when the dot clock
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6f525093..9a6517d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -708,7 +708,8 @@  static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 		return 0;
 
 	if (intel_dig_port->port == PORT_A) {
-		return DIV_ROUND_UP(dev_priv->display.get_display_clock_speed(dev), 2000);
+		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
+
 	} else {
 		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
 	}
@@ -723,7 +724,7 @@  static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
 	if (intel_dig_port->port == PORT_A) {
 		if (index)
 			return 0;
-		return DIV_ROUND_CLOSEST(dev_priv->display.get_display_clock_speed(dev), 2000);
+		return DIV_ROUND_CLOSEST(dev_priv->cdclk_freq, 2000);
 	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
 		/* Workaround for non-ULT HSW */
 		switch (index) {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index eadc15c..5db429e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1815,7 +1815,7 @@  hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
 	linetime = DIV_ROUND_CLOSEST(mode->crtc_htotal * 1000 * 8,
 				     mode->crtc_clock);
 	ips_linetime = DIV_ROUND_CLOSEST(mode->crtc_htotal * 1000 * 8,
-					 dev_priv->display.get_display_clock_speed(dev_priv->dev));
+					 dev_priv->cdclk_freq);
 
 	return PIPE_WM_LINETIME_IPS_LINETIME(ips_linetime) |
 	       PIPE_WM_LINETIME_TIME(linetime);

Comments

On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Rather than reading out the current cdclk value use the cached value we
> have tucked away in dev_priv.
> 
> v2: Rebased to the latest
> v3: Rebased to the latest
> v4: Fix for patch style problems
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>

This patch needs to be extended to also cover the recently added
skl_max_scale. Tvrtko has recently written a patch to add some checks to
that code too, would be good to resurrect that too. Chandra can help with
any questions wrt the skl scaler code.

Cheers, Daniel

> 
> Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>  drivers/gpu/drm/i915/intel_dp.c      | 5 +++--
>  drivers/gpu/drm/i915/intel_pm.c      | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9cf1553..d1dd8ab 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6610,8 +6610,7 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
>  
>  	/* FIXME should check pixel clock limits on all platforms */
>  	if (INTEL_INFO(dev)->gen < 4) {
> -		int clock_limit =
> -			dev_priv->display.get_display_clock_speed(dev);
> +		int clock_limit = dev_priv->cdclk_freq;
>  
>  		/*
>  		 * Enable pixel doubling when the dot clock
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6f525093..9a6517d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -708,7 +708,8 @@ static uint32_t ilk_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  		return 0;
>  
>  	if (intel_dig_port->port == PORT_A) {
> -		return DIV_ROUND_UP(dev_priv->display.get_display_clock_speed(dev), 2000);
> +		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
> +
>  	} else {
>  		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
>  	}
> @@ -723,7 +724,7 @@ static uint32_t hsw_get_aux_clock_divider(struct intel_dp *intel_dp, int index)
>  	if (intel_dig_port->port == PORT_A) {
>  		if (index)
>  			return 0;
> -		return DIV_ROUND_CLOSEST(dev_priv->display.get_display_clock_speed(dev), 2000);
> +		return DIV_ROUND_CLOSEST(dev_priv->cdclk_freq, 2000);
>  	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
>  		/* Workaround for non-ULT HSW */
>  		switch (index) {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index eadc15c..5db429e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1815,7 +1815,7 @@ hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
>  	linetime = DIV_ROUND_CLOSEST(mode->crtc_htotal * 1000 * 8,
>  				     mode->crtc_clock);
>  	ips_linetime = DIV_ROUND_CLOSEST(mode->crtc_htotal * 1000 * 8,
> -					 dev_priv->display.get_display_clock_speed(dev_priv->dev));
> +					 dev_priv->cdclk_freq);
>  
>  	return PIPE_WM_LINETIME_IPS_LINETIME(ips_linetime) |
>  	       PIPE_WM_LINETIME_TIME(linetime);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jun 15, 2015 at 01:54:40PM +0200, Daniel Vetter wrote:
> On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Rather than reading out the current cdclk value use the cached value we
> > have tucked away in dev_priv.
> > 
> > v2: Rebased to the latest
> > v3: Rebased to the latest
> > v4: Fix for patch style problems
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> 
> This patch needs to be extended to also cover the recently added
> skl_max_scale. Tvrtko has recently written a patch to add some checks to
> that code too, would be good to resurrect that too. Chandra can help with
> any questions wrt the skl scaler code.

Not quite I'm afraid. The CDCLK used in skl_max_scale() has to be part
of the atomic state, even bumping CDCLK if possible/needed.

If you use the cached cdclk in skl_max_scale(), it won't do the right
thing when CDCLK is off (ie cached frew is the fallback 24Mhz ref clock)
and we try to do the first modeset before waking up the display.

I filed a bug about it already to track it:

  https://bugs.freedesktop.org/show_bug.cgi?id=90874
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, June 15, 2015 2:55 PM
> To: Kahola, Mika
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v6 2/8] drm/i915: Use cached cdclk value
> 
> On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Rather than reading out the current cdclk value use the cached value
> > we have tucked away in dev_priv.
> >
> > v2: Rebased to the latest
> > v3: Rebased to the latest
> > v4: Fix for patch style problems
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> 
> This patch needs to be extended to also cover the recently added
> skl_max_scale. Tvrtko has recently written a patch to add some checks to
> that code too, would be good to resurrect that too. Chandra can help with
> any questions wrt the skl scaler code.
> 
> Cheers, Daniel
Jani has pushed these patches already so maybe this is an item for a separate patch?

Cheers,
Mika

> 
> >
> > Author:    Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 +--
> >  drivers/gpu/drm/i915/intel_dp.c      | 5 +++--
> >  drivers/gpu/drm/i915/intel_pm.c      | 2 +-
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 9cf1553..d1dd8ab 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6610,8 +6610,7 @@ static int intel_crtc_compute_config(struct
> > intel_crtc *crtc,
> >
> >  	/* FIXME should check pixel clock limits on all platforms */
> >  	if (INTEL_INFO(dev)->gen < 4) {
> > -		int clock_limit =
> > -			dev_priv->display.get_display_clock_speed(dev);
> > +		int clock_limit = dev_priv->cdclk_freq;
> >
> >  		/*
> >  		 * Enable pixel doubling when the dot clock diff --git
> > a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 6f525093..9a6517d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -708,7 +708,8 @@ static uint32_t ilk_get_aux_clock_divider(struct
> intel_dp *intel_dp, int index)
> >  		return 0;
> >
> >  	if (intel_dig_port->port == PORT_A) {
> > -		return DIV_ROUND_UP(dev_priv-
> >display.get_display_clock_speed(dev), 2000);
> > +		return DIV_ROUND_UP(dev_priv->cdclk_freq, 2000);
> > +
> >  	} else {
> >  		return DIV_ROUND_UP(intel_pch_rawclk(dev), 2);
> >  	}
> > @@ -723,7 +724,7 @@ static uint32_t hsw_get_aux_clock_divider(struct
> intel_dp *intel_dp, int index)
> >  	if (intel_dig_port->port == PORT_A) {
> >  		if (index)
> >  			return 0;
> > -		return DIV_ROUND_CLOSEST(dev_priv-
> >display.get_display_clock_speed(dev), 2000);
> > +		return DIV_ROUND_CLOSEST(dev_priv->cdclk_freq, 2000);
> >  	} else if (dev_priv->pch_id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
> >  		/* Workaround for non-ULT HSW */
> >  		switch (index) {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c index eadc15c..5db429e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -1815,7 +1815,7 @@ hsw_compute_linetime_wm(struct drm_device
> *dev, struct drm_crtc *crtc)
> >  	linetime = DIV_ROUND_CLOSEST(mode->crtc_htotal * 1000 * 8,
> >  				     mode->crtc_clock);
> >  	ips_linetime = DIV_ROUND_CLOSEST(mode->crtc_htotal * 1000 * 8,
> > -					 dev_priv-
> >display.get_display_clock_speed(dev_priv->dev));
> > +					 dev_priv->cdclk_freq);
> >
> >  	return PIPE_WM_LINETIME_IPS_LINETIME(ips_linetime) |
> >  	       PIPE_WM_LINETIME_TIME(linetime);
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
On 06/15/2015 01:14 PM, Damien Lespiau wrote:
> On Mon, Jun 15, 2015 at 01:54:40PM +0200, Daniel Vetter wrote:
>> On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Rather than reading out the current cdclk value use the cached value we
>>> have tucked away in dev_priv.
>>>
>>> v2: Rebased to the latest
>>> v3: Rebased to the latest
>>> v4: Fix for patch style problems
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>>
>> This patch needs to be extended to also cover the recently added
>> skl_max_scale. Tvrtko has recently written a patch to add some checks to
>> that code too, would be good to resurrect that too. Chandra can help with
>> any questions wrt the skl scaler code.
>
> Not quite I'm afraid. The CDCLK used in skl_max_scale() has to be part
> of the atomic state, even bumping CDCLK if possible/needed.
>
> If you use the cached cdclk in skl_max_scale(), it won't do the right
> thing when CDCLK is off (ie cached frew is the fallback 24Mhz ref clock)
> and we try to do the first modeset before waking up the display.
>
> I filed a bug about it already to track it:
>
>    https://bugs.freedesktop.org/show_bug.cgi?id=90874

I know nothing about these specific clocks, but FWIW, my patch was only 
about enabling new platforms - making skl_max_scale more robust in cases 
where clock querying does not yet work correctly.

My reasoning was based on a comment from Ville that one of those two 
clocks must never be lower than the other.

So it sounded reasonable to ignore such cases ie. assume no scaling is 
possible and allow a normal (unscaled) modeset to succeed rather than 
fail it and display nothing.

Regards,

Tvrtko
On Mon, Jun 15, 2015 at 12:21:17PM +0000, Kahola, Mika wrote:
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Monday, June 15, 2015 2:55 PM
> > To: Kahola, Mika
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH v6 2/8] drm/i915: Use cached cdclk value
> > 
> > On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Rather than reading out the current cdclk value use the cached value
> > > we have tucked away in dev_priv.
> > >
> > > v2: Rebased to the latest
> > > v3: Rebased to the latest
> > > v4: Fix for patch style problems
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > 
> > This patch needs to be extended to also cover the recently added
> > skl_max_scale. Tvrtko has recently written a patch to add some checks to
> > that code too, would be good to resurrect that too. Chandra can help with
> > any questions wrt the skl scaler code.
> > 
> > Cheers, Daniel
> Jani has pushed these patches already so maybe this is an item for a separate patch?

Yeah that's what I've meant with extending. Please work together with
Damien and Chandra and Maarten in figuring out what exactly is needed
here.

Thanks, Daniel
On Mon, Jun 15, 2015 at 01:40:24PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/15/2015 01:14 PM, Damien Lespiau wrote:
> >On Mon, Jun 15, 2015 at 01:54:40PM +0200, Daniel Vetter wrote:
> >>On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> >>>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>>Rather than reading out the current cdclk value use the cached value we
> >>>have tucked away in dev_priv.
> >>>
> >>>v2: Rebased to the latest
> >>>v3: Rebased to the latest
> >>>v4: Fix for patch style problems
> >>>
> >>>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >>
> >>This patch needs to be extended to also cover the recently added
> >>skl_max_scale. Tvrtko has recently written a patch to add some checks to
> >>that code too, would be good to resurrect that too. Chandra can help with
> >>any questions wrt the skl scaler code.
> >
> >Not quite I'm afraid. The CDCLK used in skl_max_scale() has to be part
> >of the atomic state, even bumping CDCLK if possible/needed.
> >
> >If you use the cached cdclk in skl_max_scale(), it won't do the right
> >thing when CDCLK is off (ie cached frew is the fallback 24Mhz ref clock)
> >and we try to do the first modeset before waking up the display.
> >
> >I filed a bug about it already to track it:
> >
> >   https://bugs.freedesktop.org/show_bug.cgi?id=90874
> 
> I know nothing about these specific clocks, but FWIW, my patch was only
> about enabling new platforms - making skl_max_scale more robust in cases
> where clock querying does not yet work correctly.
> 
> My reasoning was based on a comment from Ville that one of those two clocks
> must never be lower than the other.
> 
> So it sounded reasonable to ignore such cases ie. assume no scaling is
> possible and allow a normal (unscaled) modeset to succeed rather than fail
> it and display nothing.

So to be more specific, I believe this is because we detect CDCLK as
being "disabled" or on the ref clock in simulation?

Generally speaking, it's questionable if we want to work around such
limitations in the code like that, I'd rather go for defaulting to max
CDCLK in simulation.

In this particular case, we really shouldn't get cdclk < crtc_clock at
this point, I'd expect the cdclk we use (probably part of the atomic
state) to be bumped to cover crtc_clock prior to plane checks (See
Marteen's [PATCH v3 19/19] drm/i915: Make cdclk part of the atomic
state.), I guess we could add a WARN_ON(cdclk < crtc_clock) in
skl_max_scale() to ensure that's indeed the case?
On Mon, Jun 15, 2015 at 03:05:27PM +0200, Daniel Vetter wrote:
> On Mon, Jun 15, 2015 at 12:21:17PM +0000, Kahola, Mika wrote:
> > > -----Original Message-----
> > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > > Vetter
> > > Sent: Monday, June 15, 2015 2:55 PM
> > > To: Kahola, Mika
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH v6 2/8] drm/i915: Use cached cdclk value
> > > 
> > > On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > Rather than reading out the current cdclk value use the cached value
> > > > we have tucked away in dev_priv.
> > > >
> > > > v2: Rebased to the latest
> > > > v3: Rebased to the latest
> > > > v4: Fix for patch style problems
> > > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > 
> > > This patch needs to be extended to also cover the recently added
> > > skl_max_scale. Tvrtko has recently written a patch to add some checks to
> > > that code too, would be good to resurrect that too. Chandra can help with
> > > any questions wrt the skl scaler code.
> > > 
> > > Cheers, Daniel
> > Jani has pushed these patches already so maybe this is an item for a separate patch?
> 
> Yeah that's what I've meant with extending. Please work together with
> Damien and Chandra and Maarten in figuring out what exactly is needed
> here.

I think Maarten has it already covered in:

  [PATCH v3 19/19] drm/i915: Make cdclk part of the atomic state.

but I probably should test it :)
On 06/15/2015 02:09 PM, Damien Lespiau wrote:
> On Mon, Jun 15, 2015 at 01:40:24PM +0100, Tvrtko Ursulin wrote:
>>
>> On 06/15/2015 01:14 PM, Damien Lespiau wrote:
>>> On Mon, Jun 15, 2015 at 01:54:40PM +0200, Daniel Vetter wrote:
>>>> On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>
>>>>> Rather than reading out the current cdclk value use the cached value we
>>>>> have tucked away in dev_priv.
>>>>>
>>>>> v2: Rebased to the latest
>>>>> v3: Rebased to the latest
>>>>> v4: Fix for patch style problems
>>>>>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>>>>
>>>> This patch needs to be extended to also cover the recently added
>>>> skl_max_scale. Tvrtko has recently written a patch to add some checks to
>>>> that code too, would be good to resurrect that too. Chandra can help with
>>>> any questions wrt the skl scaler code.
>>>
>>> Not quite I'm afraid. The CDCLK used in skl_max_scale() has to be part
>>> of the atomic state, even bumping CDCLK if possible/needed.
>>>
>>> If you use the cached cdclk in skl_max_scale(), it won't do the right
>>> thing when CDCLK is off (ie cached frew is the fallback 24Mhz ref clock)
>>> and we try to do the first modeset before waking up the display.
>>>
>>> I filed a bug about it already to track it:
>>>
>>>    https://bugs.freedesktop.org/show_bug.cgi?id=90874
>>
>> I know nothing about these specific clocks, but FWIW, my patch was only
>> about enabling new platforms - making skl_max_scale more robust in cases
>> where clock querying does not yet work correctly.
>>
>> My reasoning was based on a comment from Ville that one of those two clocks
>> must never be lower than the other.
>>
>> So it sounded reasonable to ignore such cases ie. assume no scaling is
>> possible and allow a normal (unscaled) modeset to succeed rather than fail
>> it and display nothing.
>
> So to be more specific, I believe this is because we detect CDCLK as
> being "disabled" or on the ref clock in simulation?

Probably a reference clock. It definitely wasn't zero since 
skl_max_scale already handles that. But I forgot the exact details.

> Generally speaking, it's questionable if we want to work around such
> limitations in the code like that, I'd rather go for defaulting to max
> CDCLK in simulation.
>
> In this particular case, we really shouldn't get cdclk < crtc_clock at
> this point, I'd expect the cdclk we use (probably part of the atomic
> state) to be bumped to cover crtc_clock prior to plane checks (See
> Marteen's [PATCH v3 19/19] drm/i915: Make cdclk part of the atomic
> state.), I guess we could add a WARN_ON(cdclk < crtc_clock) in
> skl_max_scale() to ensure that's indeed the case?

WARN_ON sounds fine to me. For the other considerations - you're the 
expert. :)

Regards,

Tvrtko
> -----Original Message-----

> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of

> Damien Lespiau

> Sent: Monday, June 15, 2015 6:15 AM

> To: Daniel Vetter

> Cc: intel-gfx@lists.freedesktop.org

> Subject: Re: [Intel-gfx] [PATCH v6 2/8] drm/i915: Use cached cdclk value

> 

> On Mon, Jun 15, 2015 at 03:05:27PM +0200, Daniel Vetter wrote:

> > On Mon, Jun 15, 2015 at 12:21:17PM +0000, Kahola, Mika wrote:

> > > > -----Original Message-----

> > > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel

> > > > Vetter

> > > > Sent: Monday, June 15, 2015 2:55 PM

> > > > To: Kahola, Mika

> > > > Cc: intel-gfx@lists.freedesktop.org

> > > > Subject: Re: [Intel-gfx] [PATCH v6 2/8] drm/i915: Use cached cdclk value

> > > >

> > > > On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:

> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>

> > > > >

> > > > > Rather than reading out the current cdclk value use the cached value

> > > > > we have tucked away in dev_priv.

> > > > >

> > > > > v2: Rebased to the latest

> > > > > v3: Rebased to the latest

> > > > > v4: Fix for patch style problems

> > > > >

> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> > > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>

> > > >

> > > > This patch needs to be extended to also cover the recently added

> > > > skl_max_scale. Tvrtko has recently written a patch to add some checks to

> > > > that code too, would be good to resurrect that too. Chandra can help with

> > > > any questions wrt the skl scaler code.

> > > >

> > > > Cheers, Daniel

> > > Jani has pushed these patches already so maybe this is an item for a separate

> patch?

> >

> > Yeah that's what I've meant with extending. Please work together with

> > Damien and Chandra and Maarten in figuring out what exactly is needed

> > here.

> 

> I think Maarten has it already covered in:

> 

>   [PATCH v3 19/19] drm/i915: Make cdclk part of the atomic state.

> 

> but I probably should test it :)


Yes, cdclk < crtc_clock shouldn't happen this late unless one of the value
is stale (cdclk?). 

Checked Maarten's other patch, and it should address this issue.


> 

> --

> Damien

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/intel-gfx