Re: [PATCH 20/22] drm/tilcdc: Nuke preclose hook

From: Daniel Vetter <daniel@ffwll.ch>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	"Sarha, Jyri" <jsarha@ti.com>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 20/22] drm/tilcdc: Nuke preclose hook
Date: Tue, 12 Jan 2016 16:12:55 +0100	[thread overview]
Message-ID: <20160112151255.GA19130@phenom.ffwll.local> (raw)
In-Reply-To: <56950B7B.1030006@ti.com>

On Tue, Jan 12, 2016 at 04:19:39PM +0200, Tomi Valkeinen wrote:
> 
> On 11/01/16 23:41, Daniel Vetter wrote:
> > Again since the drm core takes care of event unlinking/disarming this
> > is now just needless code.
> > 
> > v2: Fixup misplaced hunks.
> > 
> > Cc: Rob Clark <robdclark@gmail.com>
> > Acked-by: Daniel Stone <daniels@collabora.com>
> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> (v1)
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 20 --------------------
> >  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  8 --------
> >  drivers/gpu/drm/tilcdc/tilcdc_drv.h  |  1 -
> >  3 files changed, 29 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > index 7d07733bdc86..4802da8e6d6f 100644
> > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> > @@ -662,26 +662,6 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > -void tilcdc_crtc_cancel_page_flip(struct drm_crtc *crtc, struct drm_file *file)
> > -{
> > -	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> > -	struct drm_pending_vblank_event *event;
> > -	struct drm_device *dev = crtc->dev;
> > -	unsigned long flags;
> > -
> > -	/* Destroy the pending vertical blanking event associated with the
> > -	 * pending page flip, if any, and disable vertical blanking interrupts.
> > -	 */
> > -	spin_lock_irqsave(&dev->event_lock, flags);
> > -	event = tilcdc_crtc->event;
> > -	if (event && event->base.file_priv == file) {
> > -		tilcdc_crtc->event = NULL;
> > -		event->base.destroy(&event->base);
> > -		drm_vblank_put(dev, 0);
> > -	}
> > -	spin_unlock_irqrestore(&dev->event_lock, flags);
> > -}
> > -
> 
> Hmm, looks fine, but when I was comparing the omapdrm change and this
> one, I see tilcdc doing drm_vblank_put() in the removed code but omapdrm
> doesn't.
> 
> The other patches that nuke preclose hooks also contain vblank_put. Will
> there be a vblank_put call missing here, or will there be an extra
> vblank_put call happening somewhere on omapdrm?

Different approaches to the same problem:

- omap just unlinks the event from fpriv and still process it normally.
  But then before sending it out it checks whether the fpriv is still
  there or not and either sends it, or deletes the event directly. This
  way the vblank_put is always called from the worker/irq handler as part
  of the event processing.

  This is the same approach I implemented in core with this series.

- tilcdc (and most other drivers) entirely destroy the event in the
  preclose hook, which means they must also release any other resources
  acquired as part of that event. Therefore they have the vblank_put here.
  But the vblank_put is obviously also in the normal event processing
  paths, so with the new approach of only unlinking it we can handle this
  without any special cases in the driver.

I hope this explains what's going on. Since you're about driver maintainer
no. 3 with the same question: Can you pls review the kerneldoc and make
sure it explains this well? I tried to improve it already a bit after
Laurent's/Thomas' questions.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel