Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Wed, 1 Nov 2023 16:52:43 +0200
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc:     Aradhya Bhatia <a-bhatia1@ti.com>,
        Devarsh Thakkar <devarsht@ti.com>,
        Jyri Sarha <jyri.sarha@iki.fi>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 09/10] drm/tidss: IRQ code cleanup
Message-ID: <20231101145243.GY12764@pendragon.ideasonboard.com>
References: <20231101-tidss-probe-v1-0-45149e0f9415@ideasonboard.com>
 <20231101-tidss-probe-v1-9-45149e0f9415@ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20231101-tidss-probe-v1-9-45149e0f9415@ideasonboard.com>
Precedence: bulk

Hi Tomi,

Thank you for the patch.

On Wed, Nov 01, 2023 at 11:17:46AM +0200, Tomi Valkeinen wrote:
> The IRQ setup code is overly complex. All we really need to do is
> initialize the related fields in struct tidss_device, and request the
> IRQ.
> 
> We can drop all the HW accesses, as they are pointless: the driver will
> set the IRQs correctly when it needs any of the IRQs, and at probe time
> we have done a reset, so we know that all the IRQs are masked by default
> in the hardware.

Even for K2G ?

> Thus we can combine the tidss_irq_preinstall() and
> tidss_irq_postinstall() into the tidss_irq_install() function, drop the
> HW accesses, and drop the use of spinlock, as this is done at init time
> and there can be no races.
> 
> We can also drop the HW access from the tidss_irq_uninstall(), as the
> driver will anyway disable and suspend the hardware at remove time.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_drv.c |  2 ++
>  drivers/gpu/drm/tidss/tidss_irq.c | 54 ++++++---------------------------------
>  2 files changed, 10 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index 64914331715a..37693f30d98b 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -138,6 +138,8 @@ static int tidss_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, tidss);
>  
> +	spin_lock_init(&tidss->wait_lock);
> +
>  	ret = dispc_init(tidss);
>  	if (ret) {
>  		dev_err(dev, "failed to initialize dispc: %d\n", ret);
> diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
> index 0c681c7600bc..604334ef526a 100644
> --- a/drivers/gpu/drm/tidss/tidss_irq.c
> +++ b/drivers/gpu/drm/tidss/tidss_irq.c
> @@ -93,33 +93,21 @@ void tidss_irq_resume(struct tidss_device *tidss)
>  	spin_unlock_irqrestore(&tidss->wait_lock, flags);
>  }
>  
> -static void tidss_irq_preinstall(struct drm_device *ddev)
> -{
> -	struct tidss_device *tidss = to_tidss(ddev);
> -
> -	spin_lock_init(&tidss->wait_lock);
> -
> -	tidss_runtime_get(tidss);
> -
> -	dispc_set_irqenable(tidss->dispc, 0);
> -	dispc_read_and_clear_irqstatus(tidss->dispc);
> -
> -	tidss_runtime_put(tidss);
> -}
> -
> -static void tidss_irq_postinstall(struct drm_device *ddev)
> +int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
>  {
>  	struct tidss_device *tidss = to_tidss(ddev);
> -	unsigned long flags;
> -	unsigned int i;
> +	int ret;
>  
> -	tidss_runtime_get(tidss);
> +	if (irq == IRQ_NOTCONNECTED)
> +		return -ENOTCONN;
>  
> -	spin_lock_irqsave(&tidss->wait_lock, flags);
> +	ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
> +	if (ret)
> +		return ret;
>  
>  	tidss->irq_mask = DSS_IRQ_DEVICE_OCP_ERR;
>  
> -	for (i = 0; i < tidss->num_crtcs; ++i) {
> +	for (unsigned int i = 0; i < tidss->num_crtcs; ++i) {
>  		struct tidss_crtc *tcrtc = to_tidss_crtc(tidss->crtcs[i]);
>  
>  		tidss->irq_mask |= DSS_IRQ_VP_SYNC_LOST(tcrtc->hw_videoport);
> @@ -127,28 +115,6 @@ static void tidss_irq_postinstall(struct drm_device *ddev)
>  		tidss->irq_mask |= DSS_IRQ_VP_FRAME_DONE(tcrtc->hw_videoport);
>  	}
>  
> -	tidss_irq_update(tidss);
> -
> -	spin_unlock_irqrestore(&tidss->wait_lock, flags);
> -
> -	tidss_runtime_put(tidss);
> -}
> -
> -int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
> -{
> -	int ret;
> -
> -	if (irq == IRQ_NOTCONNECTED)
> -		return -ENOTCONN;
> -
> -	tidss_irq_preinstall(ddev);
> -
> -	ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
> -	if (ret)
> -		return ret;
> -
> -	tidss_irq_postinstall(ddev);
> -
>  	return 0;
>  }
>  
> @@ -156,9 +122,5 @@ void tidss_irq_uninstall(struct drm_device *ddev)
>  {
>  	struct tidss_device *tidss = to_tidss(ddev);
>  
> -	tidss_runtime_get(tidss);
> -	dispc_set_irqenable(tidss->dispc, 0);
> -	tidss_runtime_put(tidss);
> -
>  	free_irq(tidss->irq, ddev);
>  }
> 

-- 
Regards,

Laurent Pinchart