Re: [PATCHv5] drm/i915: use waitqueue in wait for vblank

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, May 21, 2014 at 08:54:04AM +0100, Chris Wilson wrote:
> On Wed, May 21, 2014 at 01:09:58PM +0530, Arun R Murthy wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 56edff3..f97f0fe 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1513,8 +1513,11 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> >  		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >  
> >  		for_each_pipe(pipe) {
> > -			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> > +			if (pipe_stats[pipe] &
> > +					PIPE_START_VBLANK_INTERRUPT_STATUS) {
> >  				drm_handle_vblank(dev, pipe);
> > +				wake_up_interruptible(&dev_priv->wait_vblank);
> 
> We now have intel_handle_vblank() so these chunks can be simplified.
> 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4d4a0d9..e1eb564 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -757,12 +757,14 @@ enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
> >  static void g4x_wait_for_vblank(struct drm_device *dev, int pipe)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	u32 frame, frame_reg = PIPE_FRMCOUNT_GM45(pipe);
> > +	u32 vblank_cnt;
> >  
> > -	frame = I915_READ(frame_reg);
> > +	vblank_cnt = drm_vblank_count(dev, pipe);
> >  
> > -	if (wait_for(I915_READ_NOTRACE(frame_reg) != frame, 50))
> > -		DRM_DEBUG_KMS("vblank wait timed out\n");
> > +	/* TODO: get the vblank time dynamically or from platform data */
> > +	wait_event_interruptible_timeout(dev_priv->wait_vblank,
> > +			(vblank_cnt != drm_vblank_count(dev, pipe)),
> > +			msecs_to_jiffies(16));
> 
> Keep the ultimate timeout at 50 until you have evidence you can reduce
> it. And then it should be 2x vrefresh interval to be safe. However, you
> are likely still hitting the timeout as the vblank irq is not guaranteed
> to be enabled here. How safe calling drm_vblank_get() is during modeset
> I defer to Ville since he has just taken a pass over the whole mess.

The plan is to make drm_vblank_get() work until drm_vblank_off() has
been called. And when enabling, drm_vblank_get() will succeed only after
drm_vblank_on() has been called. The place where those should end up is
at the start/end of intel_crtc_{disable,enable}_planes(). So you have
access to vblank irqs while planes are getting enabled/disabled, but no
further since we can't guarantee their function once we start shutting
off pipes/ports/etc.

-- 
Ville Syrjälä
Intel OTC