On Tue, Jun 8, 2021 at 1:07 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: > > 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. 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@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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
