[PATCH] drm: Don't test for IRQ support in VBLANK ioctls
Thomas Zimmermann
tzimmermann at suse.de
Tue Jun 8 11:07:07 UTC 2021
Hi
Am 08.06.21 um 11:23 schrieb Daniel Vetter:
> On Tue, Jun 8, 2021 at 11:18 AM Daniel Vetter <daniel at ffwll.ch> 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.
> -Daniel
>
>>> This change also removes the DRM framework's only dependency on
>>> IRQ state for non-legacy drivers.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>>> ---
>>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20210608/516ad6b5/attachment-0001.sig>
More information about the dri-devel
mailing list