Re: [PATCH 08/15] drm/i915: Don't try to disable psr harder from the work item

From: Daniel Vetter <daniel@ffwll.ch>
To: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH 08/15] drm/i915: Don't try to disable psr harder from the work item
Date: Tue, 17 Jun 2014 09:29:50 +0200	[thread overview]
Message-ID: <20140617072950.GR5821@phenom.ffwll.local> (raw)
In-Reply-To: <CABVU7+vdKf-LKXJC3XUKO+6iL3krBD6LOUyCskeQj6EAzjaoUQ@mail.gmail.com>

On Mon, Jun 16, 2014 at 05:20:48PM -0700, Rodrigo Vivi wrote:
> Now I'm wondering about the psr_updated you removed and without this
> disabling at this point if you alternate to fbcon you might miss most of
> screen updates if not all...

The psr work only has a 100 ms timer, so won't catch too much really. And
we really can't call psr_match_conditions from the work again due to
locking inversion (once locking is added).

I agree that this change here breaks a few things, but the problem is that
I don't see a way to smoothly transition from your current scheme, which
mostly works due to massive amounts of invalidation as long as anything is
going on, to the new precise frontbuffer tracking. Smashing everything
into the final patch feels wrong, too.

If you're too worried about such issues I can merge more patches, but I
really have no idea how to split up the last psr patch to switch over to
invalidate/flush.
-Daniel

> 
> 
> On Mon, Jun 16, 2014 at 10:51 AM, Daniel Vetter <daniel.vetter@ffwll.ch>
> wrote:
> 
> > It's disabled already except when we've raced.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 10bcc052df4b..3e0861be9c5d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1876,9 +1876,7 @@ static void intel_edp_psr_work(struct work_struct
> > *work)
> >                 container_of(work, typeof(*dev_priv), psr.work.work);
> >         struct intel_dp *intel_dp = dev_priv->psr.enabled;
> >
> > -       if (!intel_edp_psr_match_conditions(intel_dp))
> > -               intel_edp_psr_disable(intel_dp);
> > -       else
> > +       if (intel_edp_psr_match_conditions(intel_dp))
> >                 intel_edp_psr_do_enable(intel_dp);
> >  }
> >
> > --
> > 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