[PATCH] drm: Don't test for IRQ support in VBLANK ioctls

[PATCH] drm: Don't test for IRQ support in VBLANK ioctls

[Thomas Zimmermann]

Replace the IRQ check in VBLANK ioctls with a check for vblank
support. IRQs might be enabled wthout vblanking being supported.

This change also removes the DRM framework's only dependency on
IRQ state for non-legacy drivers.

Signed-off-by: Thomas Zimmermann <tzimmermann@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

Re: [PATCH] drm: Don't test for IRQ support in VBLANK ioctls

[Daniel Vetter]

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.

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.

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.
-Daniel
> 
> This change also removes the DRM framework's only dependency on
> IRQ state for non-legacy drivers.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@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

Re: [PATCH] drm: Don't test for IRQ support in VBLANK ioctls

[Daniel Vetter]

On Tue, Jun 8, 2021 at 11:18 AM Daniel Vetter <daniel@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.
>
> 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.
>
> 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.

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@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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm: Don't test for IRQ support in VBLANK ioctls

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 6353 bytes --]

Hi

Am 08.06.21 um 11:23 schrieb Daniel Vetter:
> On Tue, Jun 8, 2021 at 11:18 AM Daniel Vetter <daniel@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@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm: Don't test for IRQ support in VBLANK ioctls

[Daniel Vetter]

On Tue, Jun 8, 2021 at 1:07 PM Thomas Zimmermann <tzimmermann@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@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.

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.
-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@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
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm: Don't test for IRQ support in VBLANK ioctls

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 9331 bytes --]

Hi

Am 08.06.21 um 14:39 schrieb Daniel Vetter:
> On Tue, Jun 8, 2021 at 1:07 PM Thomas Zimmermann <tzimmermann@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@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@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]