Re: [PATCH 3/9] drm/i915: PCH_NOP

From: Daniel Vetter <daniel@ffwll.ch>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: intel-gfx@lists.freedesktop.org, "Mcallister,
	Jeffrey" <jeffrey.mcallister@intel.com>
Subject: Re: [PATCH 3/9] drm/i915: PCH_NOP
Date: Sun, 17 Mar 2013 22:14:56 +0100	[thread overview]
Message-ID: <20130317211455.GZ9021@phenom.ffwll.local> (raw)
In-Reply-To: <20130317210541.GX9021@phenom.ffwll.local>

On Sun, Mar 17, 2013 at 10:05:41PM +0100, Daniel Vetter wrote:
> On Wed, Mar 13, 2013 at 11:21:02AM -0700, Ben Widawsky wrote:
> > Given certain fusing options discussed in the previous patch, it's
> > possible to end up with platforms that normally have PCH but that PCH
> > doesn't actually exist. In many cases, this is easily remedied with
> > setting 0 pipes. This covers the other corners.
> > 
> > Requiring this is a symptom of improper code splitting (using
> > HAS_PCH_SPLIT instead of proper GEN checking, basically). I do not want
> > to fix this.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      | 2 ++
> >  drivers/gpu/drm/i915/intel_display.c | 4 +++-
> >  drivers/gpu/drm/i915/intel_lvds.c    | 4 ++++
> >  3 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 587dca0..ceed199 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -461,6 +461,7 @@ enum intel_pch {
> >  	PCH_IBX,	/* Ibexpeak PCH */
> >  	PCH_CPT,	/* Cougarpoint PCH */
> >  	PCH_LPT,	/* Lynxpoint PCH */
> > +	PCH_NOP,
> >  };
> >  
> >  enum intel_sbi_destination {
> > @@ -1351,6 +1352,7 @@ struct drm_i915_file_private {
> >  #define HAS_PCH_LPT(dev) (INTEL_PCH_TYPE(dev) == PCH_LPT)
> >  #define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT)
> >  #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX)
> > +#define HAS_PCH_NOP(dev) (INTEL_PCH_TYPE(dev) == PCH_NOP)
> >  #define HAS_PCH_SPLIT(dev) (INTEL_PCH_TYPE(dev) != PCH_NONE)
> >  
> >  #define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index d6dbffd..1f0624e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5085,7 +5085,9 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
> >   */
> >  void intel_init_pch_refclk(struct drm_device *dev)
> >  {
> 
> Since this function here only deals with display refclocks I think a
> num_pipes == 0 check should equally work. And also makes it clear what's
> exactly going on.

See my comment on the first patch, I think if we never run the code in
setup_outputs we should be fine here already and don't need anything more.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch