| Message ID | 1604369441-65254-1-git-send-email-tiantao6@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm: Add the new api to install irq | expand |
在 2020/11/3 15:56, Thomas Zimmermann 写道: > Hi > > Thanks, the code looks good already. There just are a few nits below. > Thanks for the help with the review code. Add the new api devm_drm_irq_install and himbc use the new interface as one patch or two? > Am 03.11.20 um 03:10 schrieb Tian Tao: >> Add new api devm_drm_irq_install() to register interrupts, >> no need to call drm_irq_uninstall() when the drm module is removed. >> >> v2: >> fixed the wrong parameter. >> >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> >> --- >> drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ >> include/drm/drm_drv.h | 3 ++- >> 2 files changed, 25 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> index cd162d4..0fe5243 100644 >> --- a/drivers/gpu/drm/drm_drv.c >> +++ b/drivers/gpu/drm/drm_drv.c > > The implementation should rather go to drm_irq.c > >> @@ -39,6 +39,7 @@ >> #include <drm/drm_color_mgmt.h> >> #include <drm/drm_drv.h> >> #include <drm/drm_file.h> >> +#include <drm/drm_irq.h> >> #include <drm/drm_managed.h> >> #include <drm/drm_mode_object.h> >> #include <drm/drm_print.h> >> @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, >> return ret; >> } >> >> +static void devm_drm_dev_irq_uninstall(void *data) >> +{ >> + drm_irq_uninstall(data); >> +} >> + >> +int devm_drm_irq_install(struct device *parent, >> + struct drm_device *dev, int irq) >> +{ >> + int ret; >> + >> + ret = drm_irq_install(dev, irq); >> + if (ret) >> + return ret; >> + >> + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); >> + if (ret) >> + devm_drm_dev_irq_uninstall(dev); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(devm_drm_irq_install); >> + >> void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, >> size_t size, size_t offset) >> { >> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h >> index 0230762..fec1776 100644 >> --- a/include/drm/drm_drv.h >> +++ b/include/drm/drm_drv.h > > And the declaration should go to drm_irq.h > > We generally don't merge unused code, so you should convert at least one > KMS driver, say hibmc, to use the new interface. > > Best regards > Thomas > >> @@ -513,7 +513,8 @@ struct drm_driver { >> >> void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, >> size_t size, size_t offset); >> - >> +int devm_drm_irq_install(struct device *parent, struct drm_device *dev, >> + int irq); >> /** >> * devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance >> * @parent: Parent device object >> >
Hi Am 03.11.20 um 09:57 schrieb tiantao (H): > > > 在 2020/11/3 15:56, Thomas Zimmermann 写道: >> Hi >> >> Thanks, the code looks good already. There just are a few nits below. >> > Thanks for the help with the review code. > Add the new api devm_drm_irq_install and himbc use the new interface as > one patch or two? Better make two patches from it. Best regards Thomas > >> Am 03.11.20 um 03:10 schrieb Tian Tao: >>> Add new api devm_drm_irq_install() to register interrupts, >>> no need to call drm_irq_uninstall() when the drm module is removed. >>> >>> v2: >>> fixed the wrong parameter. >>> >>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> >>> --- >>> drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ >>> include/drm/drm_drv.h | 3 ++- >>> 2 files changed, 25 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>> index cd162d4..0fe5243 100644 >>> --- a/drivers/gpu/drm/drm_drv.c >>> +++ b/drivers/gpu/drm/drm_drv.c >> >> The implementation should rather go to drm_irq.c >> >>> @@ -39,6 +39,7 @@ >>> #include <drm/drm_color_mgmt.h> >>> #include <drm/drm_drv.h> >>> #include <drm/drm_file.h> >>> +#include <drm/drm_irq.h> >>> #include <drm/drm_managed.h> >>> #include <drm/drm_mode_object.h> >>> #include <drm/drm_print.h> >>> @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, >>> return ret; >>> } >>> +static void devm_drm_dev_irq_uninstall(void *data) >>> +{ >>> + drm_irq_uninstall(data); >>> +} >>> + >>> +int devm_drm_irq_install(struct device *parent, >>> + struct drm_device *dev, int irq) >>> +{ >>> + int ret; >>> + >>> + ret = drm_irq_install(dev, irq); >>> + if (ret) >>> + return ret; >>> + >>> + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); >>> + if (ret) >>> + devm_drm_dev_irq_uninstall(dev); >>> + >>> + return ret; >>> +} >>> +EXPORT_SYMBOL(devm_drm_irq_install); >>> + >>> void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver >>> *driver, >>> size_t size, size_t offset) >>> { >>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h >>> index 0230762..fec1776 100644 >>> --- a/include/drm/drm_drv.h >>> +++ b/include/drm/drm_drv.h >> >> And the declaration should go to drm_irq.h >> >> We generally don't merge unused code, so you should convert at least one >> KMS driver, say hibmc, to use the new interface. >> >> Best regards >> Thomas >> >>> @@ -513,7 +513,8 @@ struct drm_driver { >>> void *__devm_drm_dev_alloc(struct device *parent, struct >>> drm_driver *driver, >>> size_t size, size_t offset); >>> - >>> +int devm_drm_irq_install(struct device *parent, struct drm_device *dev, >>> + int irq); >>> /** >>> * devm_drm_dev_alloc - Resource managed allocation of a >>> &drm_device instance >>> * @parent: Parent device object >>> >> >
Hi Tian. Good to see more infrastructure support so one does not have to think about cleanup. On Tue, Nov 03, 2020 at 10:10:41AM +0800, Tian Tao wrote: > Add new api devm_drm_irq_install() to register interrupts, > no need to call drm_irq_uninstall() when the drm module is removed. > > v2: > fixed the wrong parameter. > > Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > --- > drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ > include/drm/drm_drv.h | 3 ++- > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index cd162d4..0fe5243 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -39,6 +39,7 @@ > #include <drm/drm_color_mgmt.h> > #include <drm/drm_drv.h> > #include <drm/drm_file.h> > +#include <drm/drm_irq.h> > #include <drm/drm_managed.h> > #include <drm/drm_mode_object.h> > #include <drm/drm_print.h> > @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, > return ret; > } > > +static void devm_drm_dev_irq_uninstall(void *data) devm_drm_irq_uninstall?? It matches other names if you drop the _dev part. > +{ > + drm_irq_uninstall(data); > +} > + > +int devm_drm_irq_install(struct device *parent, > + struct drm_device *dev, int irq) As this is an exported function please add appropriate kernel-doc documentation. It should for example explain that there is no need to uninstall as this is done automagically. > +{ > + int ret; > + > + ret = drm_irq_install(dev, irq); > + if (ret) > + return ret; > + > + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); > + if (ret) > + devm_drm_dev_irq_uninstall(dev); > + > + return ret; > +} > +EXPORT_SYMBOL(devm_drm_irq_install); The above are in addition to Thomas' feedback. Sam
On Tue, Nov 03, 2020 at 10:10:41AM +0800, Tian Tao wrote: > Add new api devm_drm_irq_install() to register interrupts, > no need to call drm_irq_uninstall() when the drm module is removed. > > v2: > fixed the wrong parameter. > > Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > --- > drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ > include/drm/drm_drv.h | 3 ++- > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index cd162d4..0fe5243 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -39,6 +39,7 @@ > #include <drm/drm_color_mgmt.h> > #include <drm/drm_drv.h> > #include <drm/drm_file.h> > +#include <drm/drm_irq.h> > #include <drm/drm_managed.h> > #include <drm/drm_mode_object.h> > #include <drm/drm_print.h> > @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, > return ret; > } > > +static void devm_drm_dev_irq_uninstall(void *data) > +{ > + drm_irq_uninstall(data); > +} > + > +int devm_drm_irq_install(struct device *parent, > + struct drm_device *dev, int irq) > +{ > + int ret; > + > + ret = drm_irq_install(dev, irq); > + if (ret) > + return ret; > + > + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); > + if (ret) > + devm_drm_dev_irq_uninstall(dev); > + > + return ret; > +} > +EXPORT_SYMBOL(devm_drm_irq_install); > + Shouldn't we tie the IRQ to the drm device (so with drmm_add_action) instead of tying it to the underlying device? Maxime >
Hi Am 03.11.20 um 10:52 schrieb Maxime Ripard: > On Tue, Nov 03, 2020 at 10:10:41AM +0800, Tian Tao wrote: >> Add new api devm_drm_irq_install() to register interrupts, >> no need to call drm_irq_uninstall() when the drm module is removed. >> >> v2: >> fixed the wrong parameter. >> >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> >> --- >> drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ >> include/drm/drm_drv.h | 3 ++- >> 2 files changed, 25 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >> index cd162d4..0fe5243 100644 >> --- a/drivers/gpu/drm/drm_drv.c >> +++ b/drivers/gpu/drm/drm_drv.c >> @@ -39,6 +39,7 @@ >> #include <drm/drm_color_mgmt.h> >> #include <drm/drm_drv.h> >> #include <drm/drm_file.h> >> +#include <drm/drm_irq.h> >> #include <drm/drm_managed.h> >> #include <drm/drm_mode_object.h> >> #include <drm/drm_print.h> >> @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, >> return ret; >> } >> >> +static void devm_drm_dev_irq_uninstall(void *data) >> +{ >> + drm_irq_uninstall(data); >> +} >> + >> +int devm_drm_irq_install(struct device *parent, >> + struct drm_device *dev, int irq) >> +{ >> + int ret; >> + >> + ret = drm_irq_install(dev, irq); >> + if (ret) >> + return ret; >> + >> + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); >> + if (ret) >> + devm_drm_dev_irq_uninstall(dev); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(devm_drm_irq_install); >> + > > Shouldn't we tie the IRQ to the drm device (so with drmm_add_action) > instead of tying it to the underlying device? If the HW device goes away, there won't be any more interrupts. So it's similar to devm_ functions for I/O memory. Why would you use the drmm_ interface? Best regards Thomas > > Maxime >>
On Tue, Nov 03, 2020 at 10:10:41AM +0800, Tian Tao wrote: > Add new api devm_drm_irq_install() to register interrupts, > no need to call drm_irq_uninstall() when the drm module is removed. > > v2: > fixed the wrong parameter. > > Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > --- > drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ > include/drm/drm_drv.h | 3 ++- > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index cd162d4..0fe5243 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -39,6 +39,7 @@ > #include <drm/drm_color_mgmt.h> > #include <drm/drm_drv.h> > #include <drm/drm_file.h> > +#include <drm/drm_irq.h> > #include <drm/drm_managed.h> > #include <drm/drm_mode_object.h> > #include <drm/drm_print.h> > @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, > return ret; > } > > +static void devm_drm_dev_irq_uninstall(void *data) > +{ > + drm_irq_uninstall(data); > +} > + > +int devm_drm_irq_install(struct device *parent, parent argument should not be needed, we have drm_device->dev. If that doesn't work, then don't use the drm irq helpers, they're optional (and there's already devm versions of the core irq functions). Just a detail aside from all the other things alreay mentioned. -Daniel > + struct drm_device *dev, int irq) > +{ > + int ret; > + > + ret = drm_irq_install(dev, irq); > + if (ret) > + return ret; > + > + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); > + if (ret) > + devm_drm_dev_irq_uninstall(dev); > + > + return ret; > +} > +EXPORT_SYMBOL(devm_drm_irq_install); > + > void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, > size_t size, size_t offset) > { > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h > index 0230762..fec1776 100644 > --- a/include/drm/drm_drv.h > +++ b/include/drm/drm_drv.h > @@ -513,7 +513,8 @@ struct drm_driver { > > void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, > size_t size, size_t offset); > - > +int devm_drm_irq_install(struct device *parent, struct drm_device *dev, > + int irq); > /** > * devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance > * @parent: Parent device object > -- > 2.7.4 >
On Tue, Nov 03, 2020 at 11:10:27AM +0100, Thomas Zimmermann wrote: > Hi > > Am 03.11.20 um 10:52 schrieb Maxime Ripard: > > On Tue, Nov 03, 2020 at 10:10:41AM +0800, Tian Tao wrote: > >> Add new api devm_drm_irq_install() to register interrupts, > >> no need to call drm_irq_uninstall() when the drm module is removed. > >> > >> v2: > >> fixed the wrong parameter. > >> > >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > >> --- > >> drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ > >> include/drm/drm_drv.h | 3 ++- > >> 2 files changed, 25 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > >> index cd162d4..0fe5243 100644 > >> --- a/drivers/gpu/drm/drm_drv.c > >> +++ b/drivers/gpu/drm/drm_drv.c > >> @@ -39,6 +39,7 @@ > >> #include <drm/drm_color_mgmt.h> > >> #include <drm/drm_drv.h> > >> #include <drm/drm_file.h> > >> +#include <drm/drm_irq.h> > >> #include <drm/drm_managed.h> > >> #include <drm/drm_mode_object.h> > >> #include <drm/drm_print.h> > >> @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, > >> return ret; > >> } > >> > >> +static void devm_drm_dev_irq_uninstall(void *data) > >> +{ > >> + drm_irq_uninstall(data); > >> +} > >> + > >> +int devm_drm_irq_install(struct device *parent, > >> + struct drm_device *dev, int irq) > >> +{ > >> + int ret; > >> + > >> + ret = drm_irq_install(dev, irq); > >> + if (ret) > >> + return ret; > >> + > >> + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); > >> + if (ret) > >> + devm_drm_dev_irq_uninstall(dev); > >> + > >> + return ret; > >> +} > >> +EXPORT_SYMBOL(devm_drm_irq_install); > >> + > > > > Shouldn't we tie the IRQ to the drm device (so with drmm_add_action) > > instead of tying it to the underlying device? > > If the HW device goes away, there won't be any more interrupts. So it's > similar to devm_ functions for I/O memory. Why would you use the drmm_ > interface? drm_irq_install/uninstall do more that just calling request_irq and free_irq though, they will also run (among other things) the irq-related hooks in the drm driver (irq_preinstall, irq_postinstall irq_uninstall) and wake anything waiting for a vblank to occur, so we need the DRM device and driver to still be around when we run drm_irq_uninstall. That's why (I assume) you have to pass the drm_device as an argument and not simply the device. This probably works in most case since you would allocate the drm_device with devm_drm_dev_alloc, and then run drm_irq_install, so in the undoing phase you would have first drm_irq_uninstall to run, and everything is fine. However, if one doesn't use devm_drm_dev_alloc but would use devm_drm_irq_install, you would have first remove being called that would free the drm device, and then drm_irq_uninstall which will use a free'd pointer. So yeah, even though the interrupt line itself is tied to the device, all the logic we have around the interrupt that is dealt with in drm_irq_install is really tied to the drm_device. And since we tie the life of drm_device to its underlying device already (either implicitly through devm_drm_dev_alloc, or explictly through manual allocation in probe and free in remove) we can't end up in a situation where the drm_device outlives its device. Maxime
On Tue, Nov 03, 2020 at 11:38:32AM +0100, Maxime Ripard wrote: > On Tue, Nov 03, 2020 at 11:10:27AM +0100, Thomas Zimmermann wrote: > > Hi > > > > Am 03.11.20 um 10:52 schrieb Maxime Ripard: > > > On Tue, Nov 03, 2020 at 10:10:41AM +0800, Tian Tao wrote: > > >> Add new api devm_drm_irq_install() to register interrupts, > > >> no need to call drm_irq_uninstall() when the drm module is removed. > > >> > > >> v2: > > >> fixed the wrong parameter. > > >> > > >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > > >> --- > > >> drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ > > >> include/drm/drm_drv.h | 3 ++- > > >> 2 files changed, 25 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > >> index cd162d4..0fe5243 100644 > > >> --- a/drivers/gpu/drm/drm_drv.c > > >> +++ b/drivers/gpu/drm/drm_drv.c > > >> @@ -39,6 +39,7 @@ > > >> #include <drm/drm_color_mgmt.h> > > >> #include <drm/drm_drv.h> > > >> #include <drm/drm_file.h> > > >> +#include <drm/drm_irq.h> > > >> #include <drm/drm_managed.h> > > >> #include <drm/drm_mode_object.h> > > >> #include <drm/drm_print.h> > > >> @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, > > >> return ret; > > >> } > > >> > > >> +static void devm_drm_dev_irq_uninstall(void *data) > > >> +{ > > >> + drm_irq_uninstall(data); > > >> +} > > >> + > > >> +int devm_drm_irq_install(struct device *parent, > > >> + struct drm_device *dev, int irq) > > >> +{ > > >> + int ret; > > >> + > > >> + ret = drm_irq_install(dev, irq); > > >> + if (ret) > > >> + return ret; > > >> + > > >> + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); > > >> + if (ret) > > >> + devm_drm_dev_irq_uninstall(dev); > > >> + > > >> + return ret; > > >> +} > > >> +EXPORT_SYMBOL(devm_drm_irq_install); > > >> + > > > > > > Shouldn't we tie the IRQ to the drm device (so with drmm_add_action) > > > instead of tying it to the underlying device? > > > > If the HW device goes away, there won't be any more interrupts. So it's > > similar to devm_ functions for I/O memory. Why would you use the drmm_ > > interface? > > drm_irq_install/uninstall do more that just calling request_irq and > free_irq though, they will also run (among other things) the irq-related > hooks in the drm driver (irq_preinstall, irq_postinstall irq_uninstall) > and wake anything waiting for a vblank to occur, so we need the DRM > device and driver to still be around when we run drm_irq_uninstall. > That's why (I assume) you have to pass the drm_device as an argument and > not simply the device. drm_device is guaranteed to outlive devm_, plus the hooks are meant to shut down hw. hw isn't around anymore when we do drmm_ cleanup, at least not in full generality. > This probably works in most case since you would allocate the drm_device > with devm_drm_dev_alloc, and then run drm_irq_install, so in the undoing > phase you would have first drm_irq_uninstall to run, and everything is > fine. > > However, if one doesn't use devm_drm_dev_alloc but would use > devm_drm_irq_install, you would have first remove being called that > would free the drm device, and then drm_irq_uninstall which will use a > free'd pointer. Don't do that, it's broken :-) > So yeah, even though the interrupt line itself is tied to the device, > all the logic we have around the interrupt that is dealt with in > drm_irq_install is really tied to the drm_device. And since we tie the > life of drm_device to its underlying device already (either implicitly > through devm_drm_dev_alloc, or explictly through manual allocation in > probe and free in remove) we can't end up in a situation where the > drm_device outlives its device. Most drivers get their drm_device lifetime completely wrong. That's why I typed drmm_ stuff. So this isn't a surprise at all, but it might motiveate a bunch more people to fix this up correctly. Also, these drm_irq functions are 100% optional helpers (should maybe rename them to make this clearer) for modern drivers. They're only built in for DRIVER_LEGACY, because hysterical raisins. -Daniel
Hi Am 03.11.20 um 11:55 schrieb Daniel Vetter: > On Tue, Nov 03, 2020 at 11:38:32AM +0100, Maxime Ripard wrote: >> On Tue, Nov 03, 2020 at 11:10:27AM +0100, Thomas Zimmermann wrote: >>> Hi >>> >>> Am 03.11.20 um 10:52 schrieb Maxime Ripard: >>>> On Tue, Nov 03, 2020 at 10:10:41AM +0800, Tian Tao wrote: >>>>> Add new api devm_drm_irq_install() to register interrupts, >>>>> no need to call drm_irq_uninstall() when the drm module is removed. >>>>> >>>>> v2: >>>>> fixed the wrong parameter. >>>>> >>>>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> >>>>> --- >>>>> drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ >>>>> include/drm/drm_drv.h | 3 ++- >>>>> 2 files changed, 25 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>>>> index cd162d4..0fe5243 100644 >>>>> --- a/drivers/gpu/drm/drm_drv.c >>>>> +++ b/drivers/gpu/drm/drm_drv.c >>>>> @@ -39,6 +39,7 @@ >>>>> #include <drm/drm_color_mgmt.h> >>>>> #include <drm/drm_drv.h> >>>>> #include <drm/drm_file.h> >>>>> +#include <drm/drm_irq.h> >>>>> #include <drm/drm_managed.h> >>>>> #include <drm/drm_mode_object.h> >>>>> #include <drm/drm_print.h> >>>>> @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, >>>>> return ret; >>>>> } >>>>> >>>>> +static void devm_drm_dev_irq_uninstall(void *data) >>>>> +{ >>>>> + drm_irq_uninstall(data); >>>>> +} >>>>> + >>>>> +int devm_drm_irq_install(struct device *parent, >>>>> + struct drm_device *dev, int irq) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + ret = drm_irq_install(dev, irq); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); >>>>> + if (ret) >>>>> + devm_drm_dev_irq_uninstall(dev); >>>>> + >>>>> + return ret; >>>>> +} >>>>> +EXPORT_SYMBOL(devm_drm_irq_install); >>>>> + >>>> >>>> Shouldn't we tie the IRQ to the drm device (so with drmm_add_action) >>>> instead of tying it to the underlying device? >>> >>> If the HW device goes away, there won't be any more interrupts. So it's >>> similar to devm_ functions for I/O memory. Why would you use the drmm_ >>> interface? >> >> drm_irq_install/uninstall do more that just calling request_irq and >> free_irq though, they will also run (among other things) the irq-related >> hooks in the drm driver (irq_preinstall, irq_postinstall irq_uninstall) >> and wake anything waiting for a vblank to occur, so we need the DRM >> device and driver to still be around when we run drm_irq_uninstall. >> That's why (I assume) you have to pass the drm_device as an argument and >> not simply the device. > > drm_device is guaranteed to outlive devm_, plus the hooks are meant to > shut down hw. hw isn't around anymore when we do drmm_ cleanup, at least > not in full generality. > >> This probably works in most case since you would allocate the drm_device >> with devm_drm_dev_alloc, and then run drm_irq_install, so in the undoing >> phase you would have first drm_irq_uninstall to run, and everything is >> fine. >> >> However, if one doesn't use devm_drm_dev_alloc but would use >> devm_drm_irq_install, you would have first remove being called that >> would free the drm device, and then drm_irq_uninstall which will use a >> free'd pointer. > > Don't do that, it's broken :-) Umm, I just saw that hibmc doesn't use devm_drm_dev_alloc. So maybe we have to convert it first before using the managed irq function. OTOH converting it is a good idea in any case, so why not. :) Best regards Thomas > >> So yeah, even though the interrupt line itself is tied to the device, >> all the logic we have around the interrupt that is dealt with in >> drm_irq_install is really tied to the drm_device. And since we tie the >> life of drm_device to its underlying device already (either implicitly >> through devm_drm_dev_alloc, or explictly through manual allocation in >> probe and free in remove) we can't end up in a situation where the >> drm_device outlives its device. > > Most drivers get their drm_device lifetime completely wrong. That's why I > typed drmm_ stuff. So this isn't a surprise at all, but it might motiveate > a bunch more people to fix this up correctly. > > Also, these drm_irq functions are 100% optional helpers (should maybe > rename them to make this clearer) for modern drivers. They're only built > in for DRIVER_LEGACY, because hysterical raisins. > -Daniel >
Hi! On Tue, Nov 03, 2020 at 11:55:08AM +0100, Daniel Vetter wrote: > On Tue, Nov 03, 2020 at 11:38:32AM +0100, Maxime Ripard wrote: > > On Tue, Nov 03, 2020 at 11:10:27AM +0100, Thomas Zimmermann wrote: > > > Hi > > > > > > Am 03.11.20 um 10:52 schrieb Maxime Ripard: > > > > On Tue, Nov 03, 2020 at 10:10:41AM +0800, Tian Tao wrote: > > > >> Add new api devm_drm_irq_install() to register interrupts, > > > >> no need to call drm_irq_uninstall() when the drm module is removed. > > > >> > > > >> v2: > > > >> fixed the wrong parameter. > > > >> > > > >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > > > >> --- > > > >> drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ > > > >> include/drm/drm_drv.h | 3 ++- > > > >> 2 files changed, 25 insertions(+), 1 deletion(-) > > > >> > > > >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > > >> index cd162d4..0fe5243 100644 > > > >> --- a/drivers/gpu/drm/drm_drv.c > > > >> +++ b/drivers/gpu/drm/drm_drv.c > > > >> @@ -39,6 +39,7 @@ > > > >> #include <drm/drm_color_mgmt.h> > > > >> #include <drm/drm_drv.h> > > > >> #include <drm/drm_file.h> > > > >> +#include <drm/drm_irq.h> > > > >> #include <drm/drm_managed.h> > > > >> #include <drm/drm_mode_object.h> > > > >> #include <drm/drm_print.h> > > > >> @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, > > > >> return ret; > > > >> } > > > >> > > > >> +static void devm_drm_dev_irq_uninstall(void *data) > > > >> +{ > > > >> + drm_irq_uninstall(data); > > > >> +} > > > >> + > > > >> +int devm_drm_irq_install(struct device *parent, > > > >> + struct drm_device *dev, int irq) > > > >> +{ > > > >> + int ret; > > > >> + > > > >> + ret = drm_irq_install(dev, irq); > > > >> + if (ret) > > > >> + return ret; > > > >> + > > > >> + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); > > > >> + if (ret) > > > >> + devm_drm_dev_irq_uninstall(dev); > > > >> + > > > >> + return ret; > > > >> +} > > > >> +EXPORT_SYMBOL(devm_drm_irq_install); > > > >> + > > > > > > > > Shouldn't we tie the IRQ to the drm device (so with drmm_add_action) > > > > instead of tying it to the underlying device? > > > > > > If the HW device goes away, there won't be any more interrupts. So it's > > > similar to devm_ functions for I/O memory. Why would you use the drmm_ > > > interface? > > > > drm_irq_install/uninstall do more that just calling request_irq and > > free_irq though, they will also run (among other things) the irq-related > > hooks in the drm driver (irq_preinstall, irq_postinstall irq_uninstall) > > and wake anything waiting for a vblank to occur, so we need the DRM > > device and driver to still be around when we run drm_irq_uninstall. > > That's why (I assume) you have to pass the drm_device as an argument and > > not simply the device. > > drm_device is guaranteed to outlive devm_, plus the hooks are meant to > shut down hw. hw isn't around anymore when we do drmm_ cleanup, at least > not in full generality. drm_dev_put is either called through devm or in remove / unbind, and the drm_device takes a reference on its parent device, so how can the drm_device outlive its parent device? > > This probably works in most case since you would allocate the drm_device > > with devm_drm_dev_alloc, and then run drm_irq_install, so in the undoing > > phase you would have first drm_irq_uninstall to run, and everything is > > fine. > > > > However, if one doesn't use devm_drm_dev_alloc but would use > > devm_drm_irq_install, you would have first remove being called that > > would free the drm device, and then drm_irq_uninstall which will use a > > free'd pointer. > > Don't do that, it's broken :-) Well, yeah it's usually a pretty bad situation, but if we can fix it for free it doesn't hurt? Maxime
On Tue, Nov 03, 2020 at 12:25:06PM +0100, Thomas Zimmermann wrote: > Hi > > Am 03.11.20 um 11:55 schrieb Daniel Vetter: > > On Tue, Nov 03, 2020 at 11:38:32AM +0100, Maxime Ripard wrote: > >> On Tue, Nov 03, 2020 at 11:10:27AM +0100, Thomas Zimmermann wrote: > >>> Hi > >>> > >>> Am 03.11.20 um 10:52 schrieb Maxime Ripard: > >>>> On Tue, Nov 03, 2020 at 10:10:41AM +0800, Tian Tao wrote: > >>>>> Add new api devm_drm_irq_install() to register interrupts, > >>>>> no need to call drm_irq_uninstall() when the drm module is removed. > >>>>> > >>>>> v2: > >>>>> fixed the wrong parameter. > >>>>> > >>>>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > >>>>> --- > >>>>> drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ > >>>>> include/drm/drm_drv.h | 3 ++- > >>>>> 2 files changed, 25 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > >>>>> index cd162d4..0fe5243 100644 > >>>>> --- a/drivers/gpu/drm/drm_drv.c > >>>>> +++ b/drivers/gpu/drm/drm_drv.c > >>>>> @@ -39,6 +39,7 @@ > >>>>> #include <drm/drm_color_mgmt.h> > >>>>> #include <drm/drm_drv.h> > >>>>> #include <drm/drm_file.h> > >>>>> +#include <drm/drm_irq.h> > >>>>> #include <drm/drm_managed.h> > >>>>> #include <drm/drm_mode_object.h> > >>>>> #include <drm/drm_print.h> > >>>>> @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, > >>>>> return ret; > >>>>> } > >>>>> > >>>>> +static void devm_drm_dev_irq_uninstall(void *data) > >>>>> +{ > >>>>> + drm_irq_uninstall(data); > >>>>> +} > >>>>> + > >>>>> +int devm_drm_irq_install(struct device *parent, > >>>>> + struct drm_device *dev, int irq) > >>>>> +{ > >>>>> + int ret; > >>>>> + > >>>>> + ret = drm_irq_install(dev, irq); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + > >>>>> + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); > >>>>> + if (ret) > >>>>> + devm_drm_dev_irq_uninstall(dev); > >>>>> + > >>>>> + return ret; > >>>>> +} > >>>>> +EXPORT_SYMBOL(devm_drm_irq_install); > >>>>> + > >>>> > >>>> Shouldn't we tie the IRQ to the drm device (so with drmm_add_action) > >>>> instead of tying it to the underlying device? > >>> > >>> If the HW device goes away, there won't be any more interrupts. So it's > >>> similar to devm_ functions for I/O memory. Why would you use the drmm_ > >>> interface? > >> > >> drm_irq_install/uninstall do more that just calling request_irq and > >> free_irq though, they will also run (among other things) the irq-related > >> hooks in the drm driver (irq_preinstall, irq_postinstall irq_uninstall) > >> and wake anything waiting for a vblank to occur, so we need the DRM > >> device and driver to still be around when we run drm_irq_uninstall. > >> That's why (I assume) you have to pass the drm_device as an argument and > >> not simply the device. > > > > drm_device is guaranteed to outlive devm_, plus the hooks are meant to > > shut down hw. hw isn't around anymore when we do drmm_ cleanup, at least > > not in full generality. > > > >> This probably works in most case since you would allocate the drm_device > >> with devm_drm_dev_alloc, and then run drm_irq_install, so in the undoing > >> phase you would have first drm_irq_uninstall to run, and everything is > >> fine. > >> > >> However, if one doesn't use devm_drm_dev_alloc but would use > >> devm_drm_irq_install, you would have first remove being called that > >> would free the drm device, and then drm_irq_uninstall which will use a > >> free'd pointer. > > > > Don't do that, it's broken :-) > > Umm, I just saw that hibmc doesn't use devm_drm_dev_alloc. So maybe we > have to convert it first before using the managed irq function. OTOH > converting it is a good idea in any case, so why not. :) Yeah that doesn't work as-is. I guess the second option would be if devm_drm_dev_irqinstall would take a drm_dev_get() reference of its own. Not sure that's a good idea, but it would make the thing a bit more flexible. -Daniel > > Best regards > Thomas > > > > >> So yeah, even though the interrupt line itself is tied to the device, > >> all the logic we have around the interrupt that is dealt with in > >> drm_irq_install is really tied to the drm_device. And since we tie the > >> life of drm_device to its underlying device already (either implicitly > >> through devm_drm_dev_alloc, or explictly through manual allocation in > >> probe and free in remove) we can't end up in a situation where the > >> drm_device outlives its device. > > > > Most drivers get their drm_device lifetime completely wrong. That's why I > > typed drmm_ stuff. So this isn't a surprise at all, but it might motiveate > > a bunch more people to fix this up correctly. > > > > Also, these drm_irq functions are 100% optional helpers (should maybe > > rename them to make this clearer) for modern drivers. They're only built > > in for DRIVER_LEGACY, because hysterical raisins. > > -Daniel > > > > -- > 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 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Nov 03, 2020 at 12:25:22PM +0100, Maxime Ripard wrote: > Hi! > > On Tue, Nov 03, 2020 at 11:55:08AM +0100, Daniel Vetter wrote: > > On Tue, Nov 03, 2020 at 11:38:32AM +0100, Maxime Ripard wrote: > > > On Tue, Nov 03, 2020 at 11:10:27AM +0100, Thomas Zimmermann wrote: > > > > Hi > > > > > > > > Am 03.11.20 um 10:52 schrieb Maxime Ripard: > > > > > On Tue, Nov 03, 2020 at 10:10:41AM +0800, Tian Tao wrote: > > > > >> Add new api devm_drm_irq_install() to register interrupts, > > > > >> no need to call drm_irq_uninstall() when the drm module is removed. > > > > >> > > > > >> v2: > > > > >> fixed the wrong parameter. > > > > >> > > > > >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > > > > >> --- > > > > >> drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ > > > > >> include/drm/drm_drv.h | 3 ++- > > > > >> 2 files changed, 25 insertions(+), 1 deletion(-) > > > > >> > > > > >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > > > >> index cd162d4..0fe5243 100644 > > > > >> --- a/drivers/gpu/drm/drm_drv.c > > > > >> +++ b/drivers/gpu/drm/drm_drv.c > > > > >> @@ -39,6 +39,7 @@ > > > > >> #include <drm/drm_color_mgmt.h> > > > > >> #include <drm/drm_drv.h> > > > > >> #include <drm/drm_file.h> > > > > >> +#include <drm/drm_irq.h> > > > > >> #include <drm/drm_managed.h> > > > > >> #include <drm/drm_mode_object.h> > > > > >> #include <drm/drm_print.h> > > > > >> @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, > > > > >> return ret; > > > > >> } > > > > >> > > > > >> +static void devm_drm_dev_irq_uninstall(void *data) > > > > >> +{ > > > > >> + drm_irq_uninstall(data); > > > > >> +} > > > > >> + > > > > >> +int devm_drm_irq_install(struct device *parent, > > > > >> + struct drm_device *dev, int irq) > > > > >> +{ > > > > >> + int ret; > > > > >> + > > > > >> + ret = drm_irq_install(dev, irq); > > > > >> + if (ret) > > > > >> + return ret; > > > > >> + > > > > >> + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); > > > > >> + if (ret) > > > > >> + devm_drm_dev_irq_uninstall(dev); > > > > >> + > > > > >> + return ret; > > > > >> +} > > > > >> +EXPORT_SYMBOL(devm_drm_irq_install); > > > > >> + > > > > > > > > > > Shouldn't we tie the IRQ to the drm device (so with drmm_add_action) > > > > > instead of tying it to the underlying device? > > > > > > > > If the HW device goes away, there won't be any more interrupts. So it's > > > > similar to devm_ functions for I/O memory. Why would you use the drmm_ > > > > interface? > > > > > > drm_irq_install/uninstall do more that just calling request_irq and > > > free_irq though, they will also run (among other things) the irq-related > > > hooks in the drm driver (irq_preinstall, irq_postinstall irq_uninstall) > > > and wake anything waiting for a vblank to occur, so we need the DRM > > > device and driver to still be around when we run drm_irq_uninstall. > > > That's why (I assume) you have to pass the drm_device as an argument and > > > not simply the device. > > > > drm_device is guaranteed to outlive devm_, plus the hooks are meant to > > shut down hw. hw isn't around anymore when we do drmm_ cleanup, at least > > not in full generality. > > drm_dev_put is either called through devm or in remove / unbind, and the > drm_device takes a reference on its parent device, so how can the > drm_device outlive its parent device? Oh there's more than just that going on. struct device has 2 lifetime things: - devres resources: These are release on a) on driver unbind b) driver bind failure. Which means if you hotunplug, then devres is gone - the kmalloced piece of memory containing the struct device, refcounted with kref. Totally independent. So hw resources like irq should be managed with devres. Memory allocations (to prevent use-after-free) should be refcounted by a kref somewhere. In the case of struct device that's done by the driver core. In the case of struct drm_device and all the stuff hanging off that, it's done by drmm_ (ideally at least, since in practice all drivers except i915 get it wrong without drmm_). Managing memory allocations with devres is almost always a bug. So when you unbind/hotunplug a device, the following happens: - driver unbind gets called and finishes - devres cleans up hw resources - as one of the last devres action the drm_dev_put gets called - (if no userspace is around or anything else that holds a drm_device reference) drmm_ cleans up drm_device resources - as the last cleanup drmm_ calls put_device() - the actual struct device gets released > > > This probably works in most case since you would allocate the drm_device > > > with devm_drm_dev_alloc, and then run drm_irq_install, so in the undoing > > > phase you would have first drm_irq_uninstall to run, and everything is > > > fine. > > > > > > However, if one doesn't use devm_drm_dev_alloc but would use > > > devm_drm_irq_install, you would have first remove being called that > > > would free the drm device, and then drm_irq_uninstall which will use a > > > free'd pointer. > > > > Don't do that, it's broken :-) > > Well, yeah it's usually a pretty bad situation, but if we can fix it for > free it doesn't hurt? See my comment somewhere, if the devm_drm_irq_install also holds a drm_dev_get reference, then no matter which wrong way you set stuff up, the right thing should happen. -Daniel
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index cd162d4..0fe5243 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -39,6 +39,7 @@ #include <drm/drm_color_mgmt.h> #include <drm/drm_drv.h> #include <drm/drm_file.h> +#include <drm/drm_irq.h> #include <drm/drm_managed.h> #include <drm/drm_mode_object.h> #include <drm/drm_print.h> @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, return ret; } +static void devm_drm_dev_irq_uninstall(void *data) +{ + drm_irq_uninstall(data); +} + +int devm_drm_irq_install(struct device *parent, + struct drm_device *dev, int irq) +{ + int ret; + + ret = drm_irq_install(dev, irq); + if (ret) + return ret; + + ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev); + if (ret) + devm_drm_dev_irq_uninstall(dev); + + return ret; +} +EXPORT_SYMBOL(devm_drm_irq_install); + void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset) { diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 0230762..fec1776 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -513,7 +513,8 @@ struct drm_driver { void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset); - +int devm_drm_irq_install(struct device *parent, struct drm_device *dev, + int irq); /** * devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance * @parent: Parent device object
Add new api devm_drm_irq_install() to register interrupts, no need to call drm_irq_uninstall() when the drm module is removed. v2: fixed the wrong parameter. Signed-off-by: Tian Tao <tiantao6@hisilicon.com> --- drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ include/drm/drm_drv.h | 3 ++- 2 files changed, 25 insertions(+), 1 deletion(-)