Re: [PATCH] drm/qxl: Use drm_vblank_count()

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

On Wed, Dec 17, 2014 at 03:57:51AM +0100, Mario Kleiner wrote:
> On 12/15/2014 04:56 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > The QXL driver duplicates part of the core's drm_vblank_count(), so it
> > might as well use the core's variant for the extra goodies.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >   drivers/gpu/drm/qxl/qxl_drv.c | 7 +------
> >   1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> > index 1d9b80c91a15..497024461a3c 100644
> > --- a/drivers/gpu/drm/qxl/qxl_drv.c
> > +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> > @@ -196,11 +196,6 @@ static int qxl_pm_restore(struct device *dev)
> >   	return qxl_drm_resume(drm_dev, false);
> >   }
> >   
> > -static u32 qxl_noop_get_vblank_counter(struct drm_device *dev, int crtc)
> > -{
> > -	return dev->vblank[crtc].count.counter;
> > -}
> > -
> >   static int qxl_noop_enable_vblank(struct drm_device *dev, int crtc)
> >   {
> >   	return 0;
> > @@ -231,7 +226,7 @@ static struct drm_driver qxl_driver = {
> >   			   DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED,
> >   	.load = qxl_driver_load,
> >   	.unload = qxl_driver_unload,
> > -	.get_vblank_counter = qxl_noop_get_vblank_counter,
> > +	.get_vblank_counter = drm_vblank_count,
> >   	.enable_vblank = qxl_noop_enable_vblank,
> >   	.disable_vblank = qxl_noop_disable_vblank,
> >   
> 
> Hi
> 
> That doesn't really help, although it doesn't hurt either. Just wanted 
> to point out that both the old and new method implement a no-op. The 
> get_vblank_counter() driver function is meant to implement a hardware 
> vblank counter query. It's only use case atm. is to reinitialize the 
> dev->vblank[crtc].count.counter counter returned by drm_vblank_count().
> 
> The most honest implementation if there isn't any way to get a hw vblank 
> count would be to just "return 0;" - Same net effect, but at least a 
> marker in the code that there is future work to do.

Yeah 'return 0' is what we do in i915 when there's no hw counter. I did
consider changing it to drm_vblank_count() since that seems to be the
current fad. I was hoping it might allow removing some code from
drm_irq.c, but after some more thought that might not be the case.
I'll probably need to take another look at it.

> 
> I think a better solution would be if we wouldn't require 
> .get_vblank_counter to be non-NULL, don't fake implement it in 
> kms-drivers which can't do it, and make the drm core deal with lack of 
> hw counter queries, e.g., by not disabling vblank irqs.

That seems a bit drastic. The current delayed disable
seems quite reasonable to me. The count will remain accurate as long
as the vblank irq is enabled, and if you wait for so long that the
irq gets disabled, well, I don't think a very precise answer was
needed anyway.

I was hunting some bugs in the vblank code recently, and while doing
that I thought that I might change the code to use the timestamp
difference between disable->enable to calculate an approximate number
of vblanks lost and bump the counter appropriately. Didn't try it
yet though, but seems like a reasonable idea when there's no hw
counter. Though some care will be needed when dealing with
drm_vblank_off/on.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel