| Message ID | 1428248421-29641-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06.04.2015 00:40, Chris Wilson wrote: > Avoid adding to the waitqueue and reprobing the current vblank if the > caller is only querying the current vblank sequence and timestamp and > so we would return immediately. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Michel Dänzer <michel@daenzer.net> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Dave Airlie <airlied@redhat.com>, > Cc: Mario Kleiner <mario.kleiner.de@gmail.com> > --- > drivers/gpu/drm/drm_irq.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 6f5dc18779e2..ba80b51b4b00 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -1617,14 +1617,16 @@ int drm_wait_vblank(struct drm_device *dev, void *data, > vblwait->request.sequence = seq + 1; > } > > - DRM_DEBUG("waiting on vblank count %d, crtc %d\n", > - vblwait->request.sequence, crtc); > - vblank->last_wait = vblwait->request.sequence; > - DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, > - (((drm_vblank_count(dev, crtc) - > - vblwait->request.sequence) <= (1 << 23)) || > - !vblank->enabled || > - !dev->irq_enabled)); > + if (vblwait->request.sequence != seq) { > + DRM_DEBUG("waiting on vblank count %d, crtc %d\n", > + vblwait->request.sequence, crtc); > + vblank->last_wait = vblwait->request.sequence; BTW, it looks like the last_wait field is unused and could be removed. > + DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, > + (((drm_vblank_count(dev, crtc) - > + vblwait->request.sequence) <= (1 << 23)) || > + !vblank->enabled || > + !dev->irq_enabled)); > + } > > if (ret != -EINTR) { > struct timeval now; > Also, the two patches should have different and more specific shortlogs. But apart from that, this patch is Reviewed-by: Michel Dänzer <michel.daenzer@amd.com> and the second patch is Acked-by: Michel Dänzer <michel.daenzer@amd.com>
On Tue, Apr 14, 2015 at 06:42:03PM +0900, Michel Dänzer wrote:
> Also, the two patches should have different and more specific shortlogs.
Second patch:
drm: Query vblank counters directly for known accurate state
?
On 04/14/2015 04:21 PM, Chris Wilson wrote: > On Tue, Apr 14, 2015 at 06:42:03PM +0900, Michel Dänzer wrote: >> Also, the two patches should have different and more specific shortlogs. > > Second patch: > > drm: Query vblank counters directly for known accurate state > > ? > When i applied your patches, both patches showed a shortlog of "drm: Shortcircuit vblank queries", i think that's what Michel means. -mario
On 04/05/2015 05:40 PM, Chris Wilson wrote: > Avoid adding to the waitqueue and reprobing the current vblank if the > caller is only querying the current vblank sequence and timestamp and > so we would return immediately. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Michel Dänzer <michel@daenzer.net> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Dave Airlie <airlied@redhat.com>, > Cc: Mario Kleiner <mario.kleiner.de@gmail.com> > --- > drivers/gpu/drm/drm_irq.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 6f5dc18779e2..ba80b51b4b00 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -1617,14 +1617,16 @@ int drm_wait_vblank(struct drm_device *dev, void *data, > vblwait->request.sequence = seq + 1; > } > > - DRM_DEBUG("waiting on vblank count %d, crtc %d\n", > - vblwait->request.sequence, crtc); > - vblank->last_wait = vblwait->request.sequence; > - DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, > - (((drm_vblank_count(dev, crtc) - > - vblwait->request.sequence) <= (1 << 23)) || > - !vblank->enabled || > - !dev->irq_enabled)); > + if (vblwait->request.sequence != seq) { > + DRM_DEBUG("waiting on vblank count %d, crtc %d\n", > + vblwait->request.sequence, crtc); > + vblank->last_wait = vblwait->request.sequence; > + DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, > + (((drm_vblank_count(dev, crtc) - > + vblwait->request.sequence) <= (1 << 23)) || > + !vblank->enabled || > + !dev->irq_enabled)); > + } > > if (ret != -EINTR) { > struct timeval now; > It would be good to have some DRM_DEBUG output for the skip-the-wait case as well, so one can still follow from dmesg output when a client does a drmWaitVblank call even if it is only a query. Other than that, this one [1/2] is Reviewed-and-tested-by: Mario Kleiner <mario.kleiner.de@gmail.com> -mario
On Tue, Apr 14, 2015 at 08:17:21PM +0200, Mario Kleiner wrote: > On 04/14/2015 04:21 PM, Chris Wilson wrote: > >On Tue, Apr 14, 2015 at 06:42:03PM +0900, Michel Dänzer wrote: > >>Also, the two patches should have different and more specific shortlogs. > > > >Second patch: > > > >drm: Query vblank counters directly for known accurate state > > > >? > > > > When i applied your patches, both patches showed a shortlog of > "drm: Shortcircuit vblank queries", i think that's what Michel means. I was making a suggestion for retitling. :) -Chris
On Tue, Apr 14, 2015 at 08:22:20PM +0200, Mario Kleiner wrote: > On 04/05/2015 05:40 PM, Chris Wilson wrote: > >Avoid adding to the waitqueue and reprobing the current vblank if the > >caller is only querying the current vblank sequence and timestamp and > >so we would return immediately. > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >Cc: Daniel Vetter <daniel@ffwll.ch> > >Cc: Michel Dänzer <michel@daenzer.net> > >Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >Cc: Dave Airlie <airlied@redhat.com>, > >Cc: Mario Kleiner <mario.kleiner.de@gmail.com> > >--- > > drivers/gpu/drm/drm_irq.c | 18 ++++++++++-------- > > 1 file changed, 10 insertions(+), 8 deletions(-) > > > >diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > >index 6f5dc18779e2..ba80b51b4b00 100644 > >--- a/drivers/gpu/drm/drm_irq.c > >+++ b/drivers/gpu/drm/drm_irq.c > >@@ -1617,14 +1617,16 @@ int drm_wait_vblank(struct drm_device *dev, void *data, > > vblwait->request.sequence = seq + 1; > > } > > > >- DRM_DEBUG("waiting on vblank count %d, crtc %d\n", > >- vblwait->request.sequence, crtc); > >- vblank->last_wait = vblwait->request.sequence; > >- DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, > >- (((drm_vblank_count(dev, crtc) - > >- vblwait->request.sequence) <= (1 << 23)) || > >- !vblank->enabled || > >- !dev->irq_enabled)); > >+ if (vblwait->request.sequence != seq) { > >+ DRM_DEBUG("waiting on vblank count %d, crtc %d\n", > >+ vblwait->request.sequence, crtc); > >+ vblank->last_wait = vblwait->request.sequence; > >+ DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, > >+ (((drm_vblank_count(dev, crtc) - > >+ vblwait->request.sequence) <= (1 << 23)) || > >+ !vblank->enabled || > >+ !dev->irq_enabled)); > >+ } > > > > if (ret != -EINTR) { > > struct timeval now; > > > > It would be good to have some DRM_DEBUG output for the skip-the-wait > case as well, so one can still follow from dmesg output when a > client does a drmWaitVblank call even if it is only a query. We still have DRM_DEBUG("returning %d to client"), as well as the drmIoctl:DRM_DEBUG(ioctl->name), is that not sufficient? -Chris
On 04/14/2015 08:36 PM, Chris Wilson wrote: > On Tue, Apr 14, 2015 at 08:22:20PM +0200, Mario Kleiner wrote: >> On 04/05/2015 05:40 PM, Chris Wilson wrote: >>> Avoid adding to the waitqueue and reprobing the current vblank if the >>> caller is only querying the current vblank sequence and timestamp and >>> so we would return immediately. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: Daniel Vetter <daniel@ffwll.ch> >>> Cc: Michel Dänzer <michel@daenzer.net> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> Cc: Dave Airlie <airlied@redhat.com>, >>> Cc: Mario Kleiner <mario.kleiner.de@gmail.com> >>> --- >>> drivers/gpu/drm/drm_irq.c | 18 ++++++++++-------- >>> 1 file changed, 10 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c >>> index 6f5dc18779e2..ba80b51b4b00 100644 >>> --- a/drivers/gpu/drm/drm_irq.c >>> +++ b/drivers/gpu/drm/drm_irq.c >>> @@ -1617,14 +1617,16 @@ int drm_wait_vblank(struct drm_device *dev, void *data, >>> vblwait->request.sequence = seq + 1; >>> } >>> >>> - DRM_DEBUG("waiting on vblank count %d, crtc %d\n", >>> - vblwait->request.sequence, crtc); >>> - vblank->last_wait = vblwait->request.sequence; >>> - DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, >>> - (((drm_vblank_count(dev, crtc) - >>> - vblwait->request.sequence) <= (1 << 23)) || >>> - !vblank->enabled || >>> - !dev->irq_enabled)); >>> + if (vblwait->request.sequence != seq) { >>> + DRM_DEBUG("waiting on vblank count %d, crtc %d\n", >>> + vblwait->request.sequence, crtc); >>> + vblank->last_wait = vblwait->request.sequence; >>> + DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, >>> + (((drm_vblank_count(dev, crtc) - >>> + vblwait->request.sequence) <= (1 << 23)) || >>> + !vblank->enabled || >>> + !dev->irq_enabled)); >>> + } >>> >>> if (ret != -EINTR) { >>> struct timeval now; >>> >> >> It would be good to have some DRM_DEBUG output for the skip-the-wait >> case as well, so one can still follow from dmesg output when a >> client does a drmWaitVblank call even if it is only a query. > > We still have DRM_DEBUG("returning %d to client"), as well as the > drmIoctl:DRM_DEBUG(ioctl->name), is that not sufficient? > -Chris > Oh right, that's good enough. Maybe add "on crtc %d" to that DRM_DEBUG, to make it unambiguous for which crtc some count is returned? -mario
On 14.04.2015 23:21, Chris Wilson wrote: > On Tue, Apr 14, 2015 at 06:42:03PM +0900, Michel Dänzer wrote: >> Also, the two patches should have different and more specific shortlogs. > > Second patch: > > drm: Query vblank counters directly for known accurate state > > ? Sounds good to me.
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 6f5dc18779e2..ba80b51b4b00 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1617,14 +1617,16 @@ int drm_wait_vblank(struct drm_device *dev, void *data, vblwait->request.sequence = seq + 1; } - DRM_DEBUG("waiting on vblank count %d, crtc %d\n", - vblwait->request.sequence, crtc); - vblank->last_wait = vblwait->request.sequence; - DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, - (((drm_vblank_count(dev, crtc) - - vblwait->request.sequence) <= (1 << 23)) || - !vblank->enabled || - !dev->irq_enabled)); + if (vblwait->request.sequence != seq) { + DRM_DEBUG("waiting on vblank count %d, crtc %d\n", + vblwait->request.sequence, crtc); + vblank->last_wait = vblwait->request.sequence; + DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, + (((drm_vblank_count(dev, crtc) - + vblwait->request.sequence) <= (1 << 23)) || + !vblank->enabled || + !dev->irq_enabled)); + } if (ret != -EINTR) { struct timeval now;
Avoid adding to the waitqueue and reprobing the current vblank if the caller is only querying the current vblank sequence and timestamp and so we would return immediately. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Michel Dänzer <michel@daenzer.net> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Dave Airlie <airlied@redhat.com>, Cc: Mario Kleiner <mario.kleiner.de@gmail.com> --- drivers/gpu/drm/drm_irq.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)