* [PATCH] drm/qxl: Use drm_vblank_count() @ 2014-12-15 15:56 Thierry Reding 2014-12-17 2:57 ` Mario Kleiner 0 siblings, 1 reply; 4+ messages in thread From: Thierry Reding @ 2014-12-15 15:56 UTC (permalink / raw) To: dri-devel 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, -- 2.1.3 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/qxl: Use drm_vblank_count() 2014-12-15 15:56 [PATCH] drm/qxl: Use drm_vblank_count() Thierry Reding @ 2014-12-17 2:57 ` Mario Kleiner 2014-12-17 9:37 ` Ville Syrjälä 0 siblings, 1 reply; 4+ messages in thread From: Mario Kleiner @ 2014-12-17 2:57 UTC (permalink / raw) To: dri-devel 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. 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. -mario _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/qxl: Use drm_vblank_count() 2014-12-17 2:57 ` Mario Kleiner @ 2014-12-17 9:37 ` Ville Syrjälä 2014-12-17 18:46 ` Mario Kleiner 0 siblings, 1 reply; 4+ messages in thread From: Ville Syrjälä @ 2014-12-17 9:37 UTC (permalink / raw) To: Mario Kleiner; +Cc: dri-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/qxl: Use drm_vblank_count() 2014-12-17 9:37 ` Ville Syrjälä @ 2014-12-17 18:46 ` Mario Kleiner 0 siblings, 0 replies; 4+ messages in thread From: Mario Kleiner @ 2014-12-17 18:46 UTC (permalink / raw) To: Ville Syrjälä; +Cc: dri-devel On 12/17/2014 10:37 AM, Ville Syrjälä wrote: > 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. Agreed. The 5 second timeout is imho reasonable in practice. I just meant maybe we just check in the drm if that function is non-NULL, so drivers are not forced to implement no-op stubs of that function if they don't actually support it, just to avoid oopses. > 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. > Would be an option. Unsure if it is worth it or not in practice. -mario _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-12-17 18:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-15 15:56 [PATCH] drm/qxl: Use drm_vblank_count() Thierry Reding 2014-12-17 2:57 ` Mario Kleiner 2014-12-17 9:37 ` Ville Syrjälä 2014-12-17 18:46 ` Mario Kleiner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.