| Message ID | 1418658968-1684-1-git-send-email-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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,