Re: [PATCH 7/8] drm/i915: Shrink active_crtcs and active_pipes_changed to u8

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 7/8] drm/i915: Shrink active_crtcs and active_pipes_changed to u8
Date: Wed, 13 Sep 2017 19:30:39 +0300	[thread overview]
Message-ID: <20170913163039.GT4914@intel.com> (raw)
In-Reply-To: <150531415092.4880.11723095123917876755@mail.alporthouse.com>

On Wed, Sep 13, 2017 at 03:49:10PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjala (2017-09-13 15:08:59)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We only have three pipes, so 8 bits is more than sufficient to track
> > which is active. Also start using BIT() when populating them.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
> >  drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
> >  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
> >  3 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index d4ec44495c00..f8d7599cff43 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2383,7 +2383,7 @@ struct drm_i915_private {
> >          */
> >         struct mutex dpll_lock;
> >  
> > -       unsigned int active_crtcs;
> > +       u8 active_crtcs;
> >         /* minimum acceptable cdclk for each pipe */
> >         int min_cdclk[I915_MAX_PIPES];
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 35fe8a98080e..c33fce9d9824 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6032,7 +6032,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> >                 intel_display_power_put(dev_priv, domain);
> >         intel_crtc->enabled_power_domains = 0;
> >  
> > -       dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
> > +       dev_priv->active_crtcs &= ~BIT(intel_crtc->pipe);
> >         dev_priv->min_cdclk[intel_crtc->pipe] = 0;
> >  }
> >  
> > @@ -11956,12 +11956,12 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> >  
> >         for_each_oldnew_intel_crtc_in_state(intel_state, crtc, old_crtc_state, new_crtc_state, i) {
> >                 if (new_crtc_state->base.active)
> > -                       intel_state->active_crtcs |= 1 << i;
> > +                       intel_state->active_crtcs |= BIT(crtc->pipe);
> >                 else
> > -                       intel_state->active_crtcs &= ~(1 << i);
> > +                       intel_state->active_crtcs &= ~BIT(crtc->pipe);
> 
> There's an hweight32(intel_state->active_crtcs & (drm_crtc_mask(for_crtc) - 1))
> 
> mixing drm_crtc_mask() and BIT(crtc->pipe) leaves a nasty taste.

Hmm. Looks like that mixup goes even deeper in the SKL+ wm code. Not
sure I want to try untangling it all.

> And
> lots of hweight32() to cleanup.
> >  
> >                 if (old_crtc_state->base.active != new_crtc_state->base.active)
> > -                       intel_state->active_pipe_changes |= drm_crtc_mask(&crtc->base);
> > +                       intel_state->active_pipe_changes |= BIT(crtc->pipe);
> 
> Hmm. Looks very odd to be so isolated. It's treated as a bool which
> explains why there's no corresponding clear_bit().
> -Chris

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx