[PATCH] drm/tidss: Use the new api devm_drm_irq_install

[PATCH] drm/tidss: Use the new api devm_drm_irq_install

[Tian Tao]

Use devm_drm_irq_install to register interrupts so that
drm_irq_uninstall is not needed to be called.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
 drivers/gpu/drm/tidss/tidss_drv.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index 66e3c86e..48e1f9d 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -173,7 +173,7 @@ static int tidss_probe(struct platform_device *pdev)
 		goto err_runtime_suspend;
 	}
 
-	ret = drm_irq_install(ddev, irq);
+	ret = devm_irq_install(ddev, irq);
 	if (ret) {
 		dev_err(dev, "drm_irq_install failed: %d\n", ret);
 		goto err_runtime_suspend;
@@ -219,8 +219,6 @@ static int tidss_remove(struct platform_device *pdev)
 
 	drm_atomic_helper_shutdown(ddev);
 
-	drm_irq_uninstall(ddev);
-
 #ifndef CONFIG_PM
 	/* If we don't have PM, we need to call suspend manually */
 	dispc_runtime_suspend(tidss->dispc);
-- 
2.7.4

Re: [PATCH] drm/tidss: Use the new api devm_drm_irq_install

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3287 bytes --]

Hi Tian,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.10-rc7 next-20201207]
[cannot apply to drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Tian-Tao/drm-tidss-Use-the-new-api-devm_drm_irq_install/20201208-155323
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: h8300-randconfig-r016-20201208 (attached as .config)
compiler: h8300-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/c31dcdd7b0bbfc11fa4ff1f81164483b478025c4
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Tian-Tao/drm-tidss-Use-the-new-api-devm_drm_irq_install/20201208-155323
        git checkout c31dcdd7b0bbfc11fa4ff1f81164483b478025c4
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=h8300 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpu/drm/tidss/tidss_drv.c: In function 'tidss_probe':
>> drivers/gpu/drm/tidss/tidss_drv.c:176:8: error: implicit declaration of function 'devm_irq_install'; did you mean 'drm_irq_install'? [-Werror=implicit-function-declaration]
     176 |  ret = devm_irq_install(ddev, irq);
         |        ^~~~~~~~~~~~~~~~
         |        drm_irq_install
   cc1: some warnings being treated as errors

vim +176 drivers/gpu/drm/tidss/tidss_drv.c

   162	
   163		ret = tidss_modeset_init(tidss);
   164		if (ret < 0) {
   165			if (ret != -EPROBE_DEFER)
   166				dev_err(dev, "failed to init DRM/KMS (%d)\n", ret);
   167			goto err_runtime_suspend;
   168		}
   169	
   170		irq = platform_get_irq(pdev, 0);
   171		if (irq < 0) {
   172			ret = irq;
   173			goto err_runtime_suspend;
   174		}
   175	
 > 176		ret = devm_irq_install(ddev, irq);
   177		if (ret) {
   178			dev_err(dev, "drm_irq_install failed: %d\n", ret);
   179			goto err_runtime_suspend;
   180		}
   181	
   182		drm_kms_helper_poll_init(ddev);
   183	
   184		drm_mode_config_reset(ddev);
   185	
   186		ret = drm_dev_register(ddev, 0);
   187		if (ret) {
   188			dev_err(dev, "failed to register DRM device\n");
   189			goto err_irq_uninstall;
   190		}
   191	
   192		drm_fbdev_generic_setup(ddev, 32);
   193	
   194		dev_dbg(dev, "%s done\n", __func__);
   195	
   196		return 0;
   197	
   198	err_irq_uninstall:
   199		drm_irq_uninstall(ddev);
   200	
   201	err_runtime_suspend:
   202	#ifndef CONFIG_PM
   203		dispc_runtime_suspend(tidss->dispc);
   204	#endif
   205		pm_runtime_disable(dev);
   206	
   207		return ret;
   208	}
   209	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28925 bytes --]

Re: [PATCH] drm/tidss: Use the new api devm_drm_irq_install

[Daniel Vetter]

On Tue, Dec 08, 2020 at 03:50:59PM +0800, Tian Tao wrote:
> Use devm_drm_irq_install to register interrupts so that
> drm_irq_uninstall is not needed to be called.
> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>

There's another drm_irq_install in the error path. But I'm not sure this
is safe since you're chaning the order in which things get cleaned up now.
So leaving this up to Tomi.
-Daniel

> ---
>  drivers/gpu/drm/tidss/tidss_drv.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index 66e3c86e..48e1f9d 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -173,7 +173,7 @@ static int tidss_probe(struct platform_device *pdev)
>  		goto err_runtime_suspend;
>  	}
>  
> -	ret = drm_irq_install(ddev, irq);
> +	ret = devm_irq_install(ddev, irq);
>  	if (ret) {
>  		dev_err(dev, "drm_irq_install failed: %d\n", ret);
>  		goto err_runtime_suspend;
> @@ -219,8 +219,6 @@ static int tidss_remove(struct platform_device *pdev)
>  
>  	drm_atomic_helper_shutdown(ddev);
>  
> -	drm_irq_uninstall(ddev);
> -
>  #ifndef CONFIG_PM
>  	/* If we don't have PM, we need to call suspend manually */
>  	dispc_runtime_suspend(tidss->dispc);
> -- 
> 2.7.4
> 

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

Re: [PATCH] drm/tidss: Use the new api devm_drm_irq_install

[Tomi Valkeinen]

On 09/12/2020 02:48, Daniel Vetter wrote:
> On Tue, Dec 08, 2020 at 03:50:59PM +0800, Tian Tao wrote:
>> Use devm_drm_irq_install to register interrupts so that
>> drm_irq_uninstall is not needed to be called.
>>
>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> 
> There's another drm_irq_install in the error path. But I'm not sure this
> is safe since you're chaning the order in which things get cleaned up now.
> So leaving this up to Tomi.

Right, I don't think this works. tidss irq_uninstall uses runtime_get/put, which needs to happen
before pm_runtime_disable. With devm_drm_irq_install that's not the case.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH] drm/tidss: Use the new api devm_drm_irq_install

[Daniel Vetter]

On Wed, Dec 9, 2020 at 12:29 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> On 09/12/2020 02:48, Daniel Vetter wrote:
> > On Tue, Dec 08, 2020 at 03:50:59PM +0800, Tian Tao wrote:
> >> Use devm_drm_irq_install to register interrupts so that
> >> drm_irq_uninstall is not needed to be called.
> >>
> >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> >
> > There's another drm_irq_install in the error path. But I'm not sure this
> > is safe since you're chaning the order in which things get cleaned up now.
> > So leaving this up to Tomi.
>
> Right, I don't think this works. tidss irq_uninstall uses runtime_get/put, which needs to happen
> before pm_runtime_disable. With devm_drm_irq_install that's not the case.

Hm I don't spot devm_ versions of these, surely we're not the only
ones with this problem?
-Daniel

>  Tomi
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



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

Re: [PATCH] drm/tidss: Use the new api devm_drm_irq_install

[Tomi Valkeinen]

On 09/12/2020 13:56, Daniel Vetter wrote:
> On Wed, Dec 9, 2020 at 12:29 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>> On 09/12/2020 02:48, Daniel Vetter wrote:
>>> On Tue, Dec 08, 2020 at 03:50:59PM +0800, Tian Tao wrote:
>>>> Use devm_drm_irq_install to register interrupts so that
>>>> drm_irq_uninstall is not needed to be called.
>>>>
>>>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
>>>
>>> There's another drm_irq_install in the error path. But I'm not sure this
>>> is safe since you're chaning the order in which things get cleaned up now.
>>> So leaving this up to Tomi.
>>
>> Right, I don't think this works. tidss irq_uninstall uses runtime_get/put, which needs to happen
>> before pm_runtime_disable. With devm_drm_irq_install that's not the case.
> 
> Hm I don't spot devm_ versions of these, surely we're not the only
> ones with this problem?

drm-misc-next has these. hisilicon uses it, but doesn't have an irq_uninstall hook, so possibly late
uninstall is fine there.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH] drm/tidss: Use the new api devm_drm_irq_install

[Daniel Vetter]

On Wed, Dec 9, 2020 at 1:06 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> On 09/12/2020 13:56, Daniel Vetter wrote:
> > On Wed, Dec 9, 2020 at 12:29 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >>
> >> On 09/12/2020 02:48, Daniel Vetter wrote:
> >>> On Tue, Dec 08, 2020 at 03:50:59PM +0800, Tian Tao wrote:
> >>>> Use devm_drm_irq_install to register interrupts so that
> >>>> drm_irq_uninstall is not needed to be called.
> >>>>
> >>>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> >>>
> >>> There's another drm_irq_install in the error path. But I'm not sure this
> >>> is safe since you're chaning the order in which things get cleaned up now.
> >>> So leaving this up to Tomi.
> >>
> >> Right, I don't think this works. tidss irq_uninstall uses runtime_get/put, which needs to happen
> >> before pm_runtime_disable. With devm_drm_irq_install that's not the case.
> >
> > Hm I don't spot devm_ versions of these, surely we're not the only
> > ones with this problem?
>
> drm-misc-next has these. hisilicon uses it, but doesn't have an irq_uninstall hook, so possibly late
> uninstall is fine there.

I meant a devm_ version of pm_runtime_enable. Or some other way to
make this just work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/tidss: Use the new api devm_drm_irq_install

[Tomi Valkeinen]

On 09/12/2020 14:08, Daniel Vetter wrote:
> On Wed, Dec 9, 2020 at 1:06 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>> On 09/12/2020 13:56, Daniel Vetter wrote:
>>> On Wed, Dec 9, 2020 at 12:29 PM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>>>
>>>> On 09/12/2020 02:48, Daniel Vetter wrote:
>>>>> On Tue, Dec 08, 2020 at 03:50:59PM +0800, Tian Tao wrote:
>>>>>> Use devm_drm_irq_install to register interrupts so that
>>>>>> drm_irq_uninstall is not needed to be called.
>>>>>>
>>>>>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
>>>>>
>>>>> There's another drm_irq_install in the error path. But I'm not sure this
>>>>> is safe since you're chaning the order in which things get cleaned up now.
>>>>> So leaving this up to Tomi.
>>>>
>>>> Right, I don't think this works. tidss irq_uninstall uses runtime_get/put, which needs to happen
>>>> before pm_runtime_disable. With devm_drm_irq_install that's not the case.
>>>
>>> Hm I don't spot devm_ versions of these, surely we're not the only
>>> ones with this problem?
>>
>> drm-misc-next has these. hisilicon uses it, but doesn't have an irq_uninstall hook, so possibly late
>> uninstall is fine there.
> 
> I meant a devm_ version of pm_runtime_enable. Or some other way to
> make this just work.

I see. No, I don't think we have.

Also, I feel a bit uncomfortable with devm'ified irq request/free. devm is fine for allocs and
reserving stuff, but this one affects the HW state, and your irq handler could get called until devm
frees the irq at some late point of time.

Well, it can be made to work, but just need to be careful. I've had my irq handlers getting called
too early or too late so many times that I'm a bit paranoid about it =).

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki