Re: [RFC PATCH 09/18] drm/i915: Cloned mode check

Daniel Vetter
2015-06-26T17:36:00Z

On Fri, Jun 26, 2015 at 06:14:07PM +0100, Chris Wilson wrote:
> On Fri, Jun 26, 2015 at 07:08:40PM +0200, Daniel Vetter wrote:
> > On Fri, Jun 26, 2015 at 07:21:53PM +0530, Ramalingam C wrote:
> > > If crtc is in clone mode, DRRS will be disabled. Because if the both
> > > the displays are not sharing the same vrefresh, then userspace
> > > activities based on vsync will go for toss.
> > >
> > > Clone mode will be rechecked on every restarting Idleness DRRS events.
> > >
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_drrs.c | 36 +++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 35 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_drrs.c b/drivers/gpu/drm/i915/intel_drrs.c
> > > index e5d8bcd..42b420d 100644
> > > --- a/drivers/gpu/drm/i915/intel_drrs.c
> > > +++ b/drivers/gpu/drm/i915/intel_drrs.c
> > > @@ -16,6 +16,7 @@
> > >
> > > #include <drm/i915_drm.h>
> > > #include <linux/delay.h>
> > > +#include <linux/list.h>
> > >
> > > #include "i915_drv.h"
> > > #include "intel_drv.h"
> > > @@ -85,6 +86,31 @@ int get_free_drrs_struct_index(struct drm_i915_private *dev_priv)
> > > return -EBUSY;
> > > }
> > >
> > > +/*
> > > + * TODO: This is identifying the multiple active crtcs at a time.
> > > + * Here we assume that this is clone state and disable DRRS.
> > > + * But need to implement a proper method to find the real cloned mode
> > > + * state. DRRS need not be disabled incase of multiple crtcs with
> > > + * different content.
> > > + */
> >
> > This is a pretty big hack. Why do you need it? fb tracking should keep any
> > display in the high mode as long as there's activity, so as long as
> > userspace flips both buffers for both pipes (which is should for cloned
> > mode) they'll both be running at max.
>
> It's simpler than than: cloned mode == 1 pipe, 1 framebuffer, 1 pageflip.
>
> Does having a vblank_ref count as activity? There is a large body of
> code (like OML_sync_control) that assumes a fixed refresh rate for an
> output. (Though OML_sync_control is vague about multiple monitor setups
> and ignore mode changed altogether.)

Atm we don't yet count vblank refcounts as activity, but would definitely
make tons of sense. Problem is that drm_irq.c is a mess, so for the
meantime I think we have to live with tuning the idleness timer of DRRS
such that for most timing critical apps it's long enough for them not to
notice the tricks we're playing.

And yeah fixing up drm_irq.c is somewhere on my list too - I want a proper
driver entry point for modeset drivers, where we could intercept vblank
waits and everything as we see fit. Last piece of modern drm that's still
midlayered (well ignoring that most drivers don't have a demidlayered
driver init yet).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation