Re: [PATCH v2 01/11] drm/i915: Store max dotclock

[Mika Kahola <mika.kahola@intel.com>]

On Thu, 2015-07-30 at 08:00 +0100, Chris Wilson wrote:
> On Thu, Jul 30, 2015 at 09:49:28AM +0300, Mika Kahola wrote:
> > Store max dotclock into dev_priv structure so we are able
> > to filter out the modes that are not supported by our
> > platforms.
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> >  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++++++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 04aa34a..1f69211b 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1777,6 +1777,7 @@ struct drm_i915_private {
> >  	unsigned int fsb_freq, mem_freq, is_ddr3;
> >  	unsigned int skl_boot_cdclk;
> >  	unsigned int cdclk_freq, max_cdclk_freq;
> > +	unsigned int max_dotclk;
> >  	unsigned int hpll_freq;
> >  
> >  	/**
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 43b0f17..9031261 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5259,6 +5259,24 @@ static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
> >  			modeset_put_power_domains(dev_priv, put_domains[i]);
> >  }
> >  
> > +static int intel_update_max_dotclk(struct drm_device *dev)
> 
> You don't update max dotclck, you are computing it. The caller is the
> one storing it dev_priv->max_dotclk (and so is the one actually doing
> the update).
> 
True. I'll fix the poor naming of that routine.

> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> 
> So why did you pass in dev if we never use it?
> 
> > +	int max_cdclk_freq = dev_priv->max_cdclk_freq;
> > +	int max_dotclk_freq;
> > +
> > +	if (IS_BROADWELL(dev) || IS_CHERRYVIEW(dev))
> 
> We already have dev_priv, so please stop doing dev->dev_priv over and
> over again.
> 
Yeah, that's pretty much unnecessary, so removing this one for the next
series of patches.

> > +		max_dotclk_freq = DIV_ROUND_UP(max_cdclk_freq * 100, 95);
> > +	else if (IS_VALLEYVIEW(dev))
> > +		max_dotclk_freq = DIV_ROUND_UP(max_cdclk_freq * 100, 90);
> > +	else if (IS_GEN2(dev) || IS_GEN3(dev))
> 
> If you reverse this pair and do
> 
> else if (INTEL_INFO(dev)->gen > 3)
> 	max_dotclk_freq = max_cdclk_freq;
> else
> 	max_dotclk_freq = DIV_ROUND_UP(2 * max_cdclk_freq * 100, 90);
> 
> Then the chain is mostly ordered in most-recent to oldest, always
> helpful for the next person.
> 
I'll apply this change as well on the next patch series. 

> Is this correct for gen9+?
This is something I need to double check. My current understanding is
that we should be able to support dot clock up to cd clock frequency
from HSW+ onwards. Actually, I did missed the Ville's comment to limit
dot clock to 90% for older platforms so I need to add this to the next
series as well.

Thanks again for valuable comments.

Cheers,
Mika 

> -Chris
> 


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