Hi Am 08.06.21 um 11:23 schrieb Daniel Vetter:
On Tue, Jun 8, 2021 at 11:18 AM Daniel Vetter <daniel@xxxxxxxx> wrote:On Tue, Jun 08, 2021 at 11:03:01AM +0200, Thomas Zimmermann wrote:Replace the IRQ check in VBLANK ioctls with a check for vblank support. IRQs might be enabled wthout vblanking being supported.Nah, or if they are, that's a broken driver. irq_enabled here really only means vblank_irq_enabled (maybe we should rename it). I'd like to understand the motivation here a bit better to make sure we'r not just papering over a driver bug.
It's not about a driver bug.For using simpledrm early during boot, at least some parts of DRM need to be built into the kernel binary. I'm looking for ways to reduce the size of the DRM-core objects. One low-hanging fruit is all the HW mid-layers that are present in DRM. Moving them behind CONFIG_DRM_LEGACY reduces the size of the binary for most of us. Few build with UMS support.
The IRQ code is the final HW mid-layer that is still present. I have a patchset that pushes drm_irq_install() et al into KMS drivers and moves drm_irq.o behind CONFIG_DRM_LEGACY. But now all KMS drivers have to maintain the irq_enabled flag, even though it's not used by most of them. And in the DRM core (sans legacy) only these 3 VBLANK ioctls depend on it.
The patch attemps to replace the core's dependency, so that KMS drivers don't have to maintain irq_enabled. Most can then use plain request_irq()/free_irq().
VBLANK support is already test-able by calling the rsp function. Or there's per-CRTC state IIRC. If there really is a need for an additional global flag, it should be enabled via drm_vblank_init(), but not drm_irq_install().
Also as-is this breaks legacy drivers, which do enable/disable irqs at runtime with the legacy IRQ_CONTROL ioctl, so we can't just throw this out. Hence this cleanup here is only ok for non-legacy drivers.
That only affects drm_wait_vblank_ioctl(). We could have make the test a bit more fancy to only test this field for legacy drivers.
Finally if you do this cleanup I think we should go through drivers and drop the irq_enabled = true settings that are littered around. For that cleanup I think this patch makes sense.
As I said, it was the initial plan. For a quick grepping, drivers appear to use irq_enabled mostly redundantly to the core. For the few drivers that might need irq_enabled, we could duplicate it in the local device structure.
Best regards Thomas
I forgot to add: We already do this check that you're adding here because later on we validate the provided crtc index against dev->num_crtcs. vblank support is confusing because it lives both in a kms and legacy driver world. -DanielThis change also removes the DRM framework's only dependency on IRQ state for non-legacy drivers. Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> --- drivers/gpu/drm/drm_irq.c | 10 +++------- drivers/gpu/drm/drm_vblank.c | 6 +++--- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index c3bd664ea733..1d7785721323 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -74,10 +74,8 @@ * only supports devices with a single interrupt on the main device stored in * &drm_device.dev and set as the device paramter in drm_dev_alloc(). * - * These IRQ helpers are strictly optional. Drivers which roll their own only - * need to set &drm_device.irq_enabled to signal the DRM core that vblank - * interrupts are working. Since these helpers don't automatically clean up the - * requested interrupt like e.g. devm_request_irq() they're not really + * These IRQ helpers are strictly optional. Since these helpers don't automatically + * clean up the requested interrupt like e.g. devm_request_irq() they're not really * recommended. */ @@ -91,9 +89,7 @@ * and after the installation. * * This is the simplified helper interface provided for drivers with no special - * needs. Drivers which need to install interrupt handlers for multiple - * interrupts must instead set &drm_device.irq_enabled to signal the DRM core - * that vblank interrupts are available. + * needs. * * @irq must match the interrupt number that would be passed to request_irq(), * if called directly instead of using this helper function. diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 3417e1ac7918..165286fef478 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1748,7 +1748,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, unsigned int pipe_index; unsigned int flags, pipe, high_pipe; - if (!dev->irq_enabled) + if (!drm_dev_has_vblank(dev)) return -EOPNOTSUPP; if (vblwait->request.type & _DRM_VBLANK_SIGNAL) @@ -2023,7 +2023,7 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP; - if (!dev->irq_enabled) + if (!drm_dev_has_vblank(dev)) return -EOPNOTSUPP; crtc = drm_crtc_find(dev, file_priv, get_seq->crtc_id); @@ -2082,7 +2082,7 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP; - if (!dev->irq_enabled) + if (!drm_dev_has_vblank(dev)) return -EOPNOTSUPP; crtc = drm_crtc_find(dev, file_priv, queue_seq->crtc_id); -- 2.31.1-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
