Re: [PATCH 2/2] drm/mgag200: Add vblank support

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote:
> Support for vblank requires VSYNC to signal an interrupt, which is broken
> on Matrox chipsets. The workaround that is used here and in other free
> Matrox drivers is to program <linecomp> to the value of <vdisplay> and
> enable the VLINE interrupt. This triggers an interrupt at the same time
> when VSYNC begins.
> 
> VLINE uses separate registers for enabling and clearing pending interrupts.
> No extra syncronization between irq handler and the rest of the driver is
> required.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_drv.c  |  1 +
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
>  drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++---
>  4 files changed, 73 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 4f9df3b93598..cff265973154 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -67,6 +67,7 @@ static struct drm_driver driver = {
>  	.driver_features = DRIVER_GEM | DRIVER_MODESET,
>  	.load = mgag200_driver_load,
>  	.unload = mgag200_driver_unload,
> +	.irq_handler = mgag200_irq_handler,
>  	.fops = &mgag200_driver_fops,
>  	.name = DRIVER_NAME,
>  	.desc = DRIVER_DESC,
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 1c93f8dc08c7..88cf256d135f 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -195,6 +195,7 @@ void mgag200_modeset_fini(struct mga_device *mdev);
>  				/* mgag200_main.c */
>  int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
>  void mgag200_driver_unload(struct drm_device *dev);
> +irqreturn_t mgag200_irq_handler(int irq, void *arg);
>  
>  				/* mgag200_i2c.c */
>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index a9773334dedf..5941607796e8 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -10,7 +10,9 @@
>  
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_irq.h>
>  #include <drm/drm_pci.h>
> +#include <drm/drm_vblank.h>
>  
>  #include "mgag200_drv.h"
>  
> @@ -186,10 +188,18 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	}
>  	mdev->cursor.pixels_current = NULL;
>  
> +	r = drm_vblank_init(dev, 1);
> +	if (r)
> +		goto err_modeset;
> +
>  	r = drm_fbdev_generic_setup(mdev->dev, 0);
>  	if (r)
>  		goto err_modeset;
>  
> +	r = drm_irq_install(dev, dev->pdev->irq);
> +	if (r)
> +		goto err_modeset;
> +
>  	return 0;
>  
>  err_modeset:
> @@ -207,8 +217,31 @@ void mgag200_driver_unload(struct drm_device *dev)
>  
>  	if (mdev == NULL)
>  		return;
> +	drm_irq_uninstall(dev);
>  	mgag200_modeset_fini(mdev);
>  	drm_mode_config_cleanup(dev);
>  	mgag200_mm_fini(mdev);
>  	dev->dev_private = NULL;
>  }
> +
> +irqreturn_t mgag200_irq_handler(int irq, void *arg)
> +{
> +	struct drm_device *dev = arg;
> +	struct mga_device *mdev = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	u32 status, iclear;
> +
> +	status = RREG32(0x1e14);
> +
> +	if (status & 0x00000020) { /* test <vlinepen> */

On further inspection this looks a bit iffy. IIRC the
status bits aren't masked out by IEN, so I would do
something like this:

irq_handler() {
	status = read(STATUS) & read(IEN):
	if (status == 0)
		return IRQ_NONE;

	write(ICLEAR, status);
	drm_handle_vblank();
	return IRQ_HANDLED;
}

vblank_enable() {
	write(ICLEAR, vline);
	write(IEN, vline);
}

vblank_disable() {
	write(IEN, 0);
}

Otherwise you maybe try to handle a stale interrupt when
enabling the vblank irq, and also could accidentally
handle bogus interrupts from other devices on the same irq
line.

And probably clear IEN on before registering the irq handler
for good measure.


> +		drm_for_each_crtc(crtc, dev) {
> +			drm_crtc_handle_vblank(crtc);
> +		}
> +		iclear = RREG32(0x1e18);
> +		iclear |= 0x00000020; /* set <vlineiclr> */
> +		WREG32(0x1e18, iclear);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +};

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel