Re: [PATCH 07/15] drm/i915: Track the psr dp connector in dev_priv->psr.enabled

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jun 16, 2014 at 05:10:14PM -0700, Rodrigo Vivi wrote:
> On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
> wrote:
> 
> > Trying to fish that one out through looping is a bit a locking
> > nightmare. So just set it and use it in the work struct.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  3 ++-
> >  drivers/gpu/drm/i915/intel_dp.c | 21 +++++++--------------
> >  2 files changed, 9 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index f5db29428406..759f7c6d1622 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -633,10 +633,11 @@ struct i915_drrs {
> >         struct intel_connector *connector;
> >  };
> >
> > +struct intel_dp;
> >  struct i915_psr {
> >         bool sink_support;
> >         bool source_ok;
> > -       bool enabled;
> > +       struct intel_dp *enabled;
> >         bool active;
> >         struct delayed_work work;
> >  };
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index ac1cef459a7b..10bcc052df4b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1826,7 +1826,7 @@ static void intel_edp_psr_do_enable(struct intel_dp
> > *intel_dp)
> >         /* Enable PSR on the host */
> >         intel_edp_psr_enable_source(intel_dp);
> >
> > -       dev_priv->psr.enabled = true;
> > +       dev_priv->psr.enabled = intel_dp;
> >
> 
> I liked this very much...
> 
> 
> >         dev_priv->psr.active = true;
> >  }
> >
> > @@ -1867,26 +1867,19 @@ void intel_edp_psr_disable(struct intel_dp
> > *intel_dp)
> >                        EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
> >                 DRM_ERROR("Timed out waiting for PSR Idle State\n");
> >
> > -       dev_priv->psr.enabled = false;
> > +       dev_priv->psr.enabled = NULL;
> >  }
> >
> >  static void intel_edp_psr_work(struct work_struct *work)
> >  {
> >         struct drm_i915_private *dev_priv =
> >                 container_of(work, typeof(*dev_priv), psr.work.work);
> > -       struct drm_device *dev = dev_priv->dev;
> > -       struct intel_encoder *encoder;
> > -       struct intel_dp *intel_dp = NULL;
> > -
> > -       list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> > base.head)
> > -               if (encoder->type == INTEL_OUTPUT_EDP) {
> > -                       intel_dp = enc_to_intel_dp(&encoder->base);
> > +       struct intel_dp *intel_dp = dev_priv->psr.enabled;
> >
> >
> but I'm afraid to give NULL to below functions...
> Shouldn't we add a if(!intel_dp) return;  at least just to be on the safe
> side?

Good catch, I'll put that into the series since it breaks bisect. With the
fixup from last night the end result is safe again.
-Daniel

> 
> -                       if (!intel_edp_psr_match_conditions(intel_dp))
> > -                               intel_edp_psr_disable(intel_dp);
> > -                       else
> > -                               intel_edp_psr_do_enable(intel_dp);
> > -               }
> > +       if (!intel_edp_psr_match_conditions(intel_dp))
> > +               intel_edp_psr_disable(intel_dp);
> > +       else
> > +               intel_edp_psr_do_enable(intel_dp);
> >  }
> >
> >  static void intel_edp_psr_inactivate(struct drm_device *dev)
> > --
> > 2.0.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch