| Message ID | 1607413859-63365-1-git-send-email-tiantao6@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tidss: Use the new api devm_drm_irq_install | expand |
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
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 >
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
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
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
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
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
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);
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(-)