[PATCH] drm: Don't test for IRQ support in VBLANK ioctls
Thomas Zimmermann
tzimmermann at suse.de
Wed Jun 9 07:01:25 UTC 2021
Hi
Am 08.06.21 um 14:39 schrieb Daniel Vetter:
> On Tue, Jun 8, 2021 at 1:07 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:
>>
>> 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.
>
> Uh, I'd like to see the full picture for that one first. Both total
> amounts of bytes saved vs. gpu completely modularized (including all
> the fbdev/con stuff), and what this does to simpledrm. If we end up
> with duplicated code just to shave off a few bytes from the overal
> beast, then I'm not sure that's worth it.
Just some more context:
For booting with early simpledrm, the drm core and kms helpers need to
go into the kernel image. On my Suse Tumbleweed system (with v5.12.4),
the kernel binary is ~11 MiB, drm.ko is ~200 KiB and drm_kms_helper.ko
is ~120 KiB. The difference isn't that huge, but some of my colleagues
are concerned about the increase in binary's size.
So I'm looking for opportunities to improve this. Left-over legacy code
is an easy target to remove from the binary.
>
> Bunch more comments on this discussion here below.
>
>> 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().
>
> drm_driver->irq_enabled has nothing to do with request_irq/free_irq
> for modern drivers.
>
>> 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().
>
> Yes, for modern drivers. Not for legacy drivers.
>
> Also drivers shouldn't maint the drm_device->irq_enabled flag at all,
> that's only done for legacy drivers. So if the enable/disable the irq
> over suspend/resume then if we go with the full split between kms and
> legacy driver paths, then for kms drivers they should _not_ update
> irq_enabled.
>
>>>> 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.
>
> Yeah that's needed in there.
>
>>>> 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.
>
> I only see 3 cases
> - legacy drivers
> - kms drivers which set it to get around these tests
> - radeon (amdgpu is just copypasta from radeon), which probably carry
> this purely for hysterical reasons back from the days when radeon.ko
> could run in both legacy or in kms mode with a cmdline option.
>
> There's some more drivers that use it because they don't trust the irq
> cleanup, I'd also leave them as-is.
>
> Legacy drivers you can ignore, radeon/amd probably to big a fish to
> fry to clean up the confusione&mess, everyone else you should be able
> to just delete all the lines that set irq_enabled to something. With
> that (but not yet your full plan to make drm_irq.c optional) and the
> fixed version of this patch (i.e. not breaking legacy drivers) I think
> this here makes sense as a cleanup.
Great, thanks for the feedback.
Best regards
Thomas
> -Daniel
>
>>
>> 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
>>
>
>
--
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/20210609/5c9648ea/attachment-0001.sig>
More information about the dri-devel
mailing list