| Message ID | e54838849f80454b863f9f5634dd10f79ef7bb8f.1645895582.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: JZ4780 and CI20 HDMI | expand |
Hi, On 26/02/2022 18:12, H. Nikolaus Schaller wrote: > so that specialization drivers like ingenic-dw-hdmi can enable polling. > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++ > include/drm/bridge/dw_hdmi.h | 1 + > 2 files changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 4befc104d2200..43e375da131e8 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi) > return 0; > } > > +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable) > +{ > + if (hdmi->bridge.dev) > + hdmi->bridge.dev->mode_config.poll_enabled = enable; > + else > + dev_warn(hdmi->dev, "no hdmi->bridge.dev"); > +} > +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll); > + > struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, > const struct dw_hdmi_plat_data *plat_data) > { > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index 2a1f85f9a8a3f..963960794b40e 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -196,5 +196,6 @@ enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, > void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > bool force, bool disabled, bool rxsense); > void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data); > +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable); > > #endif /* __IMX_HDMI_H__ */ As I understand, this is because the IRQ line of the dw-hdmi IP isn't connected right ? and you use the display-connector ddc gpio instead ? In this case I think the Ingenic DRM core should call drm_kms_helper_poll_init(drm) instead. Neil
Hi Neil, > Am 03.03.2022 um 17:23 schrieb Neil Armstrong <narmstrong@baylibre.com>: > > Hi, > > On 26/02/2022 18:12, H. Nikolaus Schaller wrote: >> so that specialization drivers like ingenic-dw-hdmi can enable polling. >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++ >> include/drm/bridge/dw_hdmi.h | 1 + >> 2 files changed, 10 insertions(+) >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> index 4befc104d2200..43e375da131e8 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi) >> return 0; >> } >> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable) >> +{ >> + if (hdmi->bridge.dev) >> + hdmi->bridge.dev->mode_config.poll_enabled = enable; >> + else >> + dev_warn(hdmi->dev, "no hdmi->bridge.dev"); >> +} >> +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll); >> + >> struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, >> const struct dw_hdmi_plat_data *plat_data) >> { >> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h >> index 2a1f85f9a8a3f..963960794b40e 100644 >> --- a/include/drm/bridge/dw_hdmi.h >> +++ b/include/drm/bridge/dw_hdmi.h >> @@ -196,5 +196,6 @@ enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, >> void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, >> bool force, bool disabled, bool rxsense); >> void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data); >> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable); >> #endif /* __IMX_HDMI_H__ */ > > As I understand, this is because the IRQ line of the dw-hdmi IP isn't connected right ? and you use the display-connector ddc gpio instead ? Yes. The IRQ line is not connected on all boards as far as I can see. > > In this case I think the Ingenic DRM core should call drm_kms_helper_poll_init(drm) instead. Ah, that is good. it seems to do "dw_hdmi_enable_poll()" in a more generic way. Will test and rework for v17 asap. BR and thanks, Nikolaus
Hi Neil, > Am 03.03.2022 um 17:30 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > > Hi Neil, > >> Am 03.03.2022 um 17:23 schrieb Neil Armstrong <narmstrong@baylibre.com>: >> >> Hi, >> >> On 26/02/2022 18:12, H. Nikolaus Schaller wrote: >>> so that specialization drivers like ingenic-dw-hdmi can enable polling. >>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>> --- >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++ >>> include/drm/bridge/dw_hdmi.h | 1 + >>> 2 files changed, 10 insertions(+) >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> index 4befc104d2200..43e375da131e8 100644 >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi) >>> return 0; >>> } >>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable) >>> +{ >>> + if (hdmi->bridge.dev) >>> + hdmi->bridge.dev->mode_config.poll_enabled = enable; >>> + else >>> + dev_warn(hdmi->dev, "no hdmi->bridge.dev"); >>> +} >>> +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll); >>> + >>> struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, >>> const struct dw_hdmi_plat_data *plat_data) >>> { >>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h >>> index 2a1f85f9a8a3f..963960794b40e 100644 >>> --- a/include/drm/bridge/dw_hdmi.h >>> +++ b/include/drm/bridge/dw_hdmi.h >>> @@ -196,5 +196,6 @@ enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, >>> void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, >>> bool force, bool disabled, bool rxsense); >>> void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data); >>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable); >>> #endif /* __IMX_HDMI_H__ */ >> >> As I understand, this is because the IRQ line of the dw-hdmi IP isn't connected right ? and you use the display-connector ddc gpio instead ? > > Yes. The IRQ line is not connected on all boards as far as I can see. > >> >> In this case I think the Ingenic DRM core should call drm_kms_helper_poll_init(drm) instead. > > Ah, that is good. it seems to do "dw_hdmi_enable_poll()" in a more generic way. Well, I looked through source code and it is defined as void drm_kms_helper_poll_init(struct drm_device *dev) But there is no direct pointer to some drm_device available. Neither in dw-hdmi nor ingenic-dw-hdmi. What should the parameter to drm_kms_helper_poll_init(drm) be? From comparing code to be able to set mode_config.poll_enabled = enable it should be &hdmi->bridge.dev but the struct dw_hdmi *hdmi is an opaque type for the ingenic-dw-hdmi driver. So it can't access the hdmi-bridge directly. What we can do is to make dw_hdmi_enable_poll() call drm_kms_helper_poll_init() or drm_kms_helper_poll_fini(). BR and thanks, Nikolaus
Hi Neil, Le jeu., mars 3 2022 at 17:23:02 +0100, Neil Armstrong <narmstrong@baylibre.com> a écrit : > Hi, > > On 26/02/2022 18:12, H. Nikolaus Schaller wrote: >> so that specialization drivers like ingenic-dw-hdmi can enable >> polling. >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++ >> include/drm/bridge/dw_hdmi.h | 1 + >> 2 files changed, 10 insertions(+) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> index 4befc104d2200..43e375da131e8 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi >> *hdmi) >> return 0; >> } >> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable) >> +{ >> + if (hdmi->bridge.dev) >> + hdmi->bridge.dev->mode_config.poll_enabled = enable; >> + else >> + dev_warn(hdmi->dev, "no hdmi->bridge.dev"); >> +} >> +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll); >> + >> struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, >> const struct dw_hdmi_plat_data *plat_data) >> { >> diff --git a/include/drm/bridge/dw_hdmi.h >> b/include/drm/bridge/dw_hdmi.h >> index 2a1f85f9a8a3f..963960794b40e 100644 >> --- a/include/drm/bridge/dw_hdmi.h >> +++ b/include/drm/bridge/dw_hdmi.h >> @@ -196,5 +196,6 @@ enum drm_connector_status >> dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, >> void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, >> bool force, bool disabled, bool rxsense); >> void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data); >> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable); >> #endif /* __IMX_HDMI_H__ */ > > As I understand, this is because the IRQ line of the dw-hdmi IP isn't > connected right ? and you use the display-connector ddc gpio instead ? According to the CI20 schematic, it is wired properly. So that's strange. > > In this case I think the Ingenic DRM core should call > drm_kms_helper_poll_init(drm) instead. Yes, the ingenic-drm driver does not poll for connectors because until now it never has been needed. Cheers, -Paul
Hi Nikolaus, Le jeu., mars 3 2022 at 17:43:05 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Neil, > >> Am 03.03.2022 um 17:30 schrieb H. Nikolaus Schaller >> <hns@goldelico.com>: >> >> Hi Neil, >> >>> Am 03.03.2022 um 17:23 schrieb Neil Armstrong >>> <narmstrong@baylibre.com>: >>> >>> Hi, >>> >>> On 26/02/2022 18:12, H. Nikolaus Schaller wrote: >>>> so that specialization drivers like ingenic-dw-hdmi can enable >>>> polling. >>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>>> --- >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++ >>>> include/drm/bridge/dw_hdmi.h | 1 + >>>> 2 files changed, 10 insertions(+) >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>> index 4befc104d2200..43e375da131e8 100644 >>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>> @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi >>>> *hdmi) >>>> return 0; >>>> } >>>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable) >>>> +{ >>>> + if (hdmi->bridge.dev) >>>> + hdmi->bridge.dev->mode_config.poll_enabled = enable; >>>> + else >>>> + dev_warn(hdmi->dev, "no hdmi->bridge.dev"); >>>> +} >>>> +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll); >>>> + >>>> struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, >>>> const struct dw_hdmi_plat_data *plat_data) >>>> { >>>> diff --git a/include/drm/bridge/dw_hdmi.h >>>> b/include/drm/bridge/dw_hdmi.h >>>> index 2a1f85f9a8a3f..963960794b40e 100644 >>>> --- a/include/drm/bridge/dw_hdmi.h >>>> +++ b/include/drm/bridge/dw_hdmi.h >>>> @@ -196,5 +196,6 @@ enum drm_connector_status >>>> dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, >>>> void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, >>>> bool force, bool disabled, bool rxsense); >>>> void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data); >>>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable); >>>> #endif /* __IMX_HDMI_H__ */ >>> >>> As I understand, this is because the IRQ line of the dw-hdmi IP >>> isn't connected right ? and you use the display-connector ddc gpio >>> instead ? >> >> Yes. The IRQ line is not connected on all boards as far as I can >> see. >> >>> >>> In this case I think the Ingenic DRM core should call >>> drm_kms_helper_poll_init(drm) instead. >> >> Ah, that is good. it seems to do "dw_hdmi_enable_poll()" in a more >> generic way. > > Well, I looked through source code and it is defined as > > void drm_kms_helper_poll_init(struct drm_device *dev) > > But there is no direct pointer to some drm_device available. > Neither in dw-hdmi nor ingenic-dw-hdmi. Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do have access to the main drm_device in the ingenic_drm_bind() function, so you can add it there (with a cleanup function calling drm_kms_helper_poll_fini() registered with drmm_add_action_or_reset()). Cheers, -Paul > What should the parameter to drm_kms_helper_poll_init(drm) be? > > From comparing code to be able to set mode_config.poll_enabled = > enable it should be > > &hdmi->bridge.dev > > but the struct dw_hdmi *hdmi is an opaque type for the > ingenic-dw-hdmi driver. > So it can't access the hdmi-bridge directly. > > What we can do is to make dw_hdmi_enable_poll() call > drm_kms_helper_poll_init() > or drm_kms_helper_poll_fini(). > > BR and thanks, > Nikolaus > >
Hi Paul and Neil, > Am 03.03.2022 um 17:46 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi Neil, > > Le jeu., mars 3 2022 at 17:23:02 +0100, Neil Armstrong <narmstrong@baylibre.com> a écrit : >> Hi, >> On 26/02/2022 18:12, H. Nikolaus Schaller wrote: >>> so that specialization drivers like ingenic-dw-hdmi can enable polling. >>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>> --- >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++ >>> include/drm/bridge/dw_hdmi.h | 1 + >>> 2 files changed, 10 insertions(+) >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> index 4befc104d2200..43e375da131e8 100644 >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi) >>> return 0; >>> } >>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable) >>> +{ >>> + if (hdmi->bridge.dev) >>> + hdmi->bridge.dev->mode_config.poll_enabled = enable; >>> + else >>> + dev_warn(hdmi->dev, "no hdmi->bridge.dev"); >>> +} >>> +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll); >>> + >>> struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, >>> const struct dw_hdmi_plat_data *plat_data) >>> { >>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h >>> index 2a1f85f9a8a3f..963960794b40e 100644 >>> --- a/include/drm/bridge/dw_hdmi.h >>> +++ b/include/drm/bridge/dw_hdmi.h >>> @@ -196,5 +196,6 @@ enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, >>> void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, >>> bool force, bool disabled, bool rxsense); >>> void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data); >>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable); >>> #endif /* __IMX_HDMI_H__ */ >> As I understand, this is because the IRQ line of the dw-hdmi IP isn't connected right ? and you use the display-connector ddc gpio instead ? Ah, I should finish work for today since I am no longer reading every word properly... No, we do NOT use the display connector for HPD. We use HPD of the dw-hdmi. Either IRQ is not enabled properly or not working in IRQ mode. But it works if polling is enabled. > > According to the CI20 schematic, it is wired properly. So that's strange. Yes, HTPLG input goes through an 1kΩ + 1µF low-pass filter to debounce HDMI_HTPLG. This goes to the HPD (BGA ball N19). There is an optional Q14 driving HDMI_DETE_N. This could become the hpd-gpios property of the connector in the device tree. But it is optional. So we have to use dw-hdmi HPD and make it work (in combination with a chained hdmi-connector). > >> In this case I think the Ingenic DRM core should call drm_kms_helper_poll_init(drm) instead. > > Yes, the ingenic-drm driver does not poll for connectors because until now it never has been needed. Well, if we go back a while we only needed it after introducing the hdmi-connectors and making dw-hdmi a bridge. Originally the dw-hdmi driver did properly take care of everything (by registering its own connector). BR and thanks, Nikolaus
Hi Paul, > Am 03.03.2022 um 17:51 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi Nikolaus, > > Le jeu., mars 3 2022 at 17:43:05 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Hi Neil, >>> Am 03.03.2022 um 17:30 schrieb H. Nikolaus Schaller <hns@goldelico.com>: >>> Hi Neil, >>>> Am 03.03.2022 um 17:23 schrieb Neil Armstrong <narmstrong@baylibre.com>: >>>> Hi, >>>> On 26/02/2022 18:12, H. Nikolaus Schaller wrote: >>>>> so that specialization drivers like ingenic-dw-hdmi can enable polling. >>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>>>> --- >>>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++ >>>>> include/drm/bridge/dw_hdmi.h | 1 + >>>>> 2 files changed, 10 insertions(+) >>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>>> index 4befc104d2200..43e375da131e8 100644 >>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>>> @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi) >>>>> return 0; >>>>> } >>>>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable) >>>>> +{ >>>>> + if (hdmi->bridge.dev) >>>>> + hdmi->bridge.dev->mode_config.poll_enabled = enable; >>>>> + else >>>>> + dev_warn(hdmi->dev, "no hdmi->bridge.dev"); >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll); >>>>> + >>>>> struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, >>>>> const struct dw_hdmi_plat_data *plat_data) >>>>> { >>>>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h >>>>> index 2a1f85f9a8a3f..963960794b40e 100644 >>>>> --- a/include/drm/bridge/dw_hdmi.h >>>>> +++ b/include/drm/bridge/dw_hdmi.h >>>>> @@ -196,5 +196,6 @@ enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, >>>>> void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, >>>>> bool force, bool disabled, bool rxsense); >>>>> void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data); >>>>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable); >>>>> #endif /* __IMX_HDMI_H__ */ >>>> As I understand, this is because the IRQ line of the dw-hdmi IP isn't connected right ? and you use the display-connector ddc gpio instead ? >>> Yes. The IRQ line is not connected on all boards as far as I can see. >>>> In this case I think the Ingenic DRM core should call drm_kms_helper_poll_init(drm) instead. >>> Ah, that is good. it seems to do "dw_hdmi_enable_poll()" in a more generic way. >> Well, I looked through source code and it is defined as >> void drm_kms_helper_poll_init(struct drm_device *dev) >> But there is no direct pointer to some drm_device available. >> Neither in dw-hdmi nor ingenic-dw-hdmi. > > Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do have access to the main drm_device in the ingenic_drm_bind() function, so you can add it there (with a cleanup function calling drm_kms_helper_poll_fini() registered with drmm_add_action_or_reset()). Well, do you really want to mix HPD detection between connector, Synopsys bridge and Ingenic DRM core? These are independent... Or should be accessed only through the bridge chain pointers. IMHO we should keep separate functions separate. And maybe this should also be conditional? Maybe not depend on compatible = jz4780 but compatible = ci20? Looks to me to be a quick fix in the wrong place. Let's fix the CSC issue first. BR, Nikolaus
Hi Nikolaus, [snip] >> Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do >> have access to the main drm_device in the ingenic_drm_bind() >> function, so you can add it there (with a cleanup function calling >> drm_kms_helper_poll_fini() registered with >> drmm_add_action_or_reset()). > > Well, do you really want to mix HPD detection between connector, > Synopsys bridge and Ingenic DRM core? These are independent... > Or should be accessed only through the bridge chain pointers. > > IMHO we should keep separate functions separate. The drm_kms_helper_poll_init() just says "this DRM device may have connectors that need to be polled" so it very well fits inside the main driver, IMHO. -Paul > > And maybe this should also be conditional? Maybe not depend on > compatible = jz4780 but compatible = ci20? > > Looks to me to be a quick fix in the wrong place. > > Let's fix the CSC issue first. > > BR, > Nikolaus >
Hi Paul, Neil, > Am 03.03.2022 um 18:20 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi Nikolaus, > > [snip] > >>> Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do have access to the main drm_device in the ingenic_drm_bind() function, so you can add it there (with a cleanup function calling drm_kms_helper_poll_fini() registered with drmm_add_action_or_reset()). >> Well, do you really want to mix HPD detection between connector, Synopsys bridge and Ingenic DRM core? These are independent... >> Or should be accessed only through the bridge chain pointers. >> IMHO we should keep separate functions separate. > > The drm_kms_helper_poll_init() just says "this DRM device may have connectors that need to be polled" so it very well fits inside the main driver, IMHO. As far as I understand, it has the side-effect to always set dev->mode_config.poll_enabled and schedule_delayed_work() for all devices. I am not sure if this is intended for arbitrary ingenic-drm devices. But you know better than me. Hm. But wait, I think I now finally remember why I have proposed it the way it is! It is always better to go back to requirements and find the least invasive solution. - HPD IRQ works and calls dw_hdmi_irq() [as can be shown by adding printk()] - it is just that the udevd is only notified if poll_enabled = true (but no polling takes place!). An earlier version (v4) to fix this proposed to add an explicit call to drm_kms_helper_hotplug_event() in dw_hdmi_irq() but that was rejected a while ago because drm_helper_hpd_irq_event() will already call it: https://www.spinics.net/lists/dri-devel/msg316846.html Since this did not take into account that dev->mode_config.poll_enabled must be set true, I then proposed the enable_poll() mechanism just to set this bit for the ingenic-dw-hdmi specialization. So a HPD event is delivered to the dw-hdmi driver as dw_hdmi_irq() and that calls drm_helper_hpd_irq_event() but not drm_kms_helper_hotplug_event() and user-space is not getting aware. It is all a hack because we mix the dw-hdmi driver which originally did register its own connector with an explicit connector... In summary I now thing that the v4 patch is the simplest and least invasive solution. We neither have to introduce a dw_hdmi_enable_poll() function or call drm_kms_helper_poll_init() anywhere. It is just a single line to add to dw-hdmi. And neither touches ingenic-dw-hdmi nor ingenic-drm-drv. So let's go back to v4 version (just modify commit message to better describe why we have to call drm_kms_helper_hotplug_event() explicitly) and forget about alternatives. BR, Nikolaus
Hi, On 03/03/2022 18:59, H. Nikolaus Schaller wrote: > Hi Paul, Neil, > >> Am 03.03.2022 um 18:20 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> Hi Nikolaus, >> >> [snip] >> >>>> Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do have access to the main drm_device in the ingenic_drm_bind() function, so you can add it there (with a cleanup function calling drm_kms_helper_poll_fini() registered with drmm_add_action_or_reset()). >>> Well, do you really want to mix HPD detection between connector, Synopsys bridge and Ingenic DRM core? These are independent... >>> Or should be accessed only through the bridge chain pointers. >>> IMHO we should keep separate functions separate. >> >> The drm_kms_helper_poll_init() just says "this DRM device may have connectors that need to be polled" so it very well fits inside the main driver, IMHO. > > As far as I understand, it has the side-effect to always set dev->mode_config.poll_enabled and > schedule_delayed_work() for all devices. > I am not sure if this is intended for arbitrary ingenic-drm devices. But you know better than me. > > > Hm. But wait, I think I now finally remember why I have proposed it the way it is! > It is always better to go back to requirements and find the least invasive solution. > > - HPD IRQ works and calls dw_hdmi_irq() [as can be shown by adding printk()] > - it is just that the udevd is only notified if poll_enabled = true (but no polling takes place!). > > An earlier version (v4) to fix this proposed to add an explicit call to drm_kms_helper_hotplug_event() > in dw_hdmi_irq() but that was rejected a while ago because drm_helper_hpd_irq_event() will already call it: > > https://www.spinics.net/lists/dri-devel/msg316846.html > > Since this did not take into account that dev->mode_config.poll_enabled must be set true, I then proposed the > enable_poll() mechanism just to set this bit for the ingenic-dw-hdmi specialization. > > So a HPD event is delivered to the dw-hdmi driver as dw_hdmi_irq() and that calls drm_helper_hpd_irq_event() > but not drm_kms_helper_hotplug_event() and user-space is not getting aware. > > It is all a hack because we mix the dw-hdmi driver which originally did register its own connector > with an explicit connector... > > In summary I now thing that the v4 patch is the simplest and least invasive solution. > > We neither have to introduce a dw_hdmi_enable_poll() function or call drm_kms_helper_poll_init() anywhere. > > It is just a single line to add to dw-hdmi. And neither touches ingenic-dw-hdmi nor ingenic-drm-drv. > > So let's go back to v4 version (just modify commit message to better describe why we have to call > drm_kms_helper_hotplug_event() explicitly) and forget about alternatives. Please don't and add drm_kms_helper_poll_init() from the ingenic-drm-drv.c like every other DRM driver. Adding drm_kms_helper_hotplug_event() in dw-hdmi will impact other drivers using dw-hdmi but correctly calling drm_kms_helper_poll_init(). Neil > > BR, > Nikolaus
Hi Neil, Le ven., mars 4 2022 at 14:30:46 +0100, Neil Armstrong <narmstrong@baylibre.com> a écrit : > Hi, > > On 03/03/2022 18:59, H. Nikolaus Schaller wrote: >> Hi Paul, Neil, >> >>> Am 03.03.2022 um 18:20 schrieb Paul Cercueil <paul@crapouillou.net>: >>> >>> Hi Nikolaus, >>> >>> [snip] >>> >>>>> Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do >>>>> have access to the main drm_device in the ingenic_drm_bind() >>>>> function, so you can add it there (with a cleanup function >>>>> calling drm_kms_helper_poll_fini() registered with >>>>> drmm_add_action_or_reset()). >>>> Well, do you really want to mix HPD detection between connector, >>>> Synopsys bridge and Ingenic DRM core? These are independent... >>>> Or should be accessed only through the bridge chain pointers. >>>> IMHO we should keep separate functions separate. >>> >>> The drm_kms_helper_poll_init() just says "this DRM device may have >>> connectors that need to be polled" so it very well fits inside the >>> main driver, IMHO. >> >> As far as I understand, it has the side-effect to always set >> dev->mode_config.poll_enabled and >> schedule_delayed_work() for all devices. >> I am not sure if this is intended for arbitrary ingenic-drm devices. >> But you know better than me. >> >> >> Hm. But wait, I think I now finally remember why I have proposed it >> the way it is! >> It is always better to go back to requirements and find the least >> invasive solution. >> >> - HPD IRQ works and calls dw_hdmi_irq() [as can be shown by adding >> printk()] >> - it is just that the udevd is only notified if poll_enabled = true >> (but no polling takes place!). >> >> An earlier version (v4) to fix this proposed to add an explicit call >> to drm_kms_helper_hotplug_event() >> in dw_hdmi_irq() but that was rejected a while ago because >> drm_helper_hpd_irq_event() will already call it: >> >> https://www.spinics.net/lists/dri-devel/msg316846.html >> >> Since this did not take into account that >> dev->mode_config.poll_enabled must be set true, I then proposed the >> enable_poll() mechanism just to set this bit for the ingenic-dw-hdmi >> specialization. >> >> So a HPD event is delivered to the dw-hdmi driver as dw_hdmi_irq() >> and that calls drm_helper_hpd_irq_event() >> but not drm_kms_helper_hotplug_event() and user-space is not getting >> aware. >> >> It is all a hack because we mix the dw-hdmi driver which originally >> did register its own connector >> with an explicit connector... >> >> In summary I now thing that the v4 patch is the simplest and least >> invasive solution. >> >> We neither have to introduce a dw_hdmi_enable_poll() function or >> call drm_kms_helper_poll_init() anywhere. >> >> It is just a single line to add to dw-hdmi. And neither touches >> ingenic-dw-hdmi nor ingenic-drm-drv. >> >> So let's go back to v4 version (just modify commit message to better >> describe why we have to call >> drm_kms_helper_hotplug_event() explicitly) and forget about >> alternatives. > > Please don't and add drm_kms_helper_poll_init() from the > ingenic-drm-drv.c like every other DRM driver. > > Adding drm_kms_helper_hotplug_event() in dw-hdmi will impact other > drivers using dw-hdmi but correctly > calling drm_kms_helper_poll_init(). From what I understood in Nikolaus' last message, HDMI hotplug is actually correctly detected, so there's no need for polling. What is missing is the call to drm_kms_helper_hotplug_event *somewhere*, so that the information is correctly relayed to userspace. I think this issue can be fixed by calling drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c. Cheers, -Paul
Hi Paul, Neil, > Am 04.03.2022 um 17:47 schrieb Paul Cercueil <paul@crapouillou.net>: > > From what I understood in Nikolaus' last message, HDMI hotplug is actually correctly detected, so there's no need for polling. What is missing is the call to drm_kms_helper_hotplug_event *somewhere*, so that the information is correctly relayed to userspace. Exactly. As Maxime pointed out it should already be called by drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't because mode_config.poll_enabled isn't enabled. So we can either a) enable mode_config.poll_enabled so that it is called by drm_helper_hpd_irq_event() or b) make drm_kms_helper_hotplug_event() being called explicitly in dw_hdmi_irq(). We could guard that by mode_config.poll_enabled to avoid drm_kms_helper_hotplug_event() being called twice (but I think the "changed" mechanism will take care of). > I think this issue can be fixed by calling drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c. I don't see yet how this would solve it, but it may work. Anyways, this all is a missing feature (sometimes called "bug") of the *dw-hdmi driver* and IMHO neither of the connector nor the ingenic-drm-drv. So I think it should not be solved outside dw-hdmi. BR and thanks, Nikolaus
Le ven., mars 4 2022 at 18:51:14 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Paul, Neil, > >> Am 04.03.2022 um 17:47 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> From what I understood in Nikolaus' last message, HDMI hotplug is >> actually correctly detected, so there's no need for polling. What is >> missing is the call to drm_kms_helper_hotplug_event *somewhere*, so >> that the information is correctly relayed to userspace. > > Exactly. > > As Maxime pointed out it should already be called by > drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't > because mode_config.poll_enabled isn't enabled. > > So we can either > a) enable mode_config.poll_enabled so that it is called by > drm_helper_hpd_irq_event() or > > b) make drm_kms_helper_hotplug_event() being called explicitly in > dw_hdmi_irq(). > We could guard that by mode_config.poll_enabled to avoid > drm_kms_helper_hotplug_event() > being called twice (but I think the "changed" mechanism will take > care of). > >> I think this issue can be fixed by calling >> drm_bridge_connector_enable_hpd() on the connector in >> ingenic-drm-drv.c. > > I don't see yet how this would solve it, but it may work. dw_hdmi_irq() calls drm_bridge_hpd_notify(), which would call bridge->hpd_cb() if it was non-NULL. Calling drm_bridge_connector_enable_hpd() will set the bridge->hpd_cb() callback to point to drm_bridge_connector_hpd_cb(), which itself will call drm_kms_helper_hotplug_event(). Therefore, all that is missing is one call to drm_bridge_connector_enable_hpd(). Cheers, -Paul > > Anyways, this all is a missing feature (sometimes called "bug") of > the *dw-hdmi driver* and IMHO > neither of the connector nor the ingenic-drm-drv. > > So I think it should not be solved outside dw-hdmi. > > BR and thanks, > Nikolaus >
Hi Paul, > Am 04.03.2022 um 19:04 schrieb Paul Cercueil <paul@crapouillou.net>: > > > > Le ven., mars 4 2022 at 18:51:14 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Hi Paul, Neil, >>> Am 04.03.2022 um 17:47 schrieb Paul Cercueil <paul@crapouillou.net>: >>> From what I understood in Nikolaus' last message, HDMI hotplug is actually correctly detected, so there's no need for polling. What is missing is the call to drm_kms_helper_hotplug_event *somewhere*, so that the information is correctly relayed to userspace. >> Exactly. >> As Maxime pointed out it should already be called by drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't >> because mode_config.poll_enabled isn't enabled. >> So we can either >> a) enable mode_config.poll_enabled so that it is called by drm_helper_hpd_irq_event() or >> b) make drm_kms_helper_hotplug_event() being called explicitly in dw_hdmi_irq(). >> We could guard that by mode_config.poll_enabled to avoid drm_kms_helper_hotplug_event() >> being called twice (but I think the "changed" mechanism will take care of). >>> I think this issue can be fixed by calling drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c. >> I don't see yet how this would solve it, but it may work. > > dw_hdmi_irq() calls drm_bridge_hpd_notify(), which would call bridge->hpd_cb() if it was non-NULL. Ok, this is a case c). I vaguely remember having tried to analyse what bridge->hpd_cb is but stopped since it is NULL... > > Calling drm_bridge_connector_enable_hpd() will set the bridge->hpd_cb() callback to point to drm_bridge_connector_hpd_cb(), which itself will call drm_kms_helper_hotplug_event(). Therefore, all that is missing is one call to drm_bridge_connector_enable_hpd(). Ah, ok, I see. >> Anyways, this all is a missing feature (sometimes called "bug") of the *dw-hdmi driver* and IMHO >> neither of the connector nor the ingenic-drm-drv. Well, a little more analysis shows that drm_bridge_connector_enable_hpd is called in the *-drv.c for some other plaforms: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-dev.c#L292 https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-kms.c#L145 https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/omapdrm/omap_drv.c#L393 https://elixir.bootlin.com/linux/v5.17-rc6/source/drivers/gpu/drm/msm/hdmi/hdmi.c#L317 >> So I think it should not be solved outside dw-hdmi. Hm. Can we call drm_bridge_connector_enable_hpd() from inside dw-hdmi? Or would this be the solution if merged? (I currently can't try code). https://lore.kernel.org/lkml/a7d0b013-6114-07b3-0a7b-0d17db8a3982@cogentembedded.com/T/ BR and thanks, Nikolaus
Le ven., mars 4 2022 at 19:15:13 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi Paul, > >> Am 04.03.2022 um 19:04 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> >> >> Le ven., mars 4 2022 at 18:51:14 +0100, H. Nikolaus Schaller >> <hns@goldelico.com> a écrit : >>> Hi Paul, Neil, >>>> Am 04.03.2022 um 17:47 schrieb Paul Cercueil >>>> <paul@crapouillou.net>: >>>> From what I understood in Nikolaus' last message, HDMI hotplug is >>>> actually correctly detected, so there's no need for polling. What >>>> is missing is the call to drm_kms_helper_hotplug_event >>>> *somewhere*, so that the information is correctly relayed to >>>> userspace. >>> Exactly. >>> As Maxime pointed out it should already be called by >>> drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't >>> because mode_config.poll_enabled isn't enabled. >>> So we can either >>> a) enable mode_config.poll_enabled so that it is called by >>> drm_helper_hpd_irq_event() or >>> b) make drm_kms_helper_hotplug_event() being called explicitly in >>> dw_hdmi_irq(). >>> We could guard that by mode_config.poll_enabled to avoid >>> drm_kms_helper_hotplug_event() >>> being called twice (but I think the "changed" mechanism will >>> take care of). >>>> I think this issue can be fixed by calling >>>> drm_bridge_connector_enable_hpd() on the connector in >>>> ingenic-drm-drv.c. >>> I don't see yet how this would solve it, but it may work. >> >> dw_hdmi_irq() calls drm_bridge_hpd_notify(), which would call >> bridge->hpd_cb() if it was non-NULL. > > Ok, this is a case c). > > I vaguely remember having tried to analyse what bridge->hpd_cb is but > stopped since it is NULL... > >> >> Calling drm_bridge_connector_enable_hpd() will set the >> bridge->hpd_cb() callback to point to drm_bridge_connector_hpd_cb(), >> which itself will call drm_kms_helper_hotplug_event(). Therefore, >> all that is missing is one call to drm_bridge_connector_enable_hpd(). > > Ah, ok, I see. > >>> Anyways, this all is a missing feature (sometimes called "bug") of >>> the *dw-hdmi driver* and IMHO >>> neither of the connector nor the ingenic-drm-drv. > > Well, a little more analysis shows that > drm_bridge_connector_enable_hpd is called > in the *-drv.c for some other plaforms: > > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-dev.c#L292 > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-kms.c#L145 > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/omapdrm/omap_drv.c#L393 > https://elixir.bootlin.com/linux/v5.17-rc6/source/drivers/gpu/drm/msm/hdmi/hdmi.c#L317 > >>> So I think it should not be solved outside dw-hdmi. > > Hm. Can we call drm_bridge_connector_enable_hpd() from inside dw-hdmi? > > Or would this be the solution if merged? (I currently can't try code). > > https://lore.kernel.org/lkml/a7d0b013-6114-07b3-0a7b-0d17db8a3982@cogentembedded.com/T/ Looks correct to me. It has been reviewed by two people so I believe it will be merged very soon. Cheers, -Paul
> Am 04.03.2022 um 19:33 schrieb Paul Cercueil <paul@crapouillou.net>: > > > > Le ven., mars 4 2022 at 19:15:13 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Hi Paul, >>> Am 04.03.2022 um 19:04 schrieb Paul Cercueil <paul@crapouillou.net>: >>> Le ven., mars 4 2022 at 18:51:14 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >>>> Hi Paul, Neil, >>>>> Am 04.03.2022 um 17:47 schrieb Paul Cercueil <paul@crapouillou.net>: >>>>> From what I understood in Nikolaus' last message, HDMI hotplug is actually correctly detected, so there's no need for polling. What is missing is the call to drm_kms_helper_hotplug_event *somewhere*, so that the information is correctly relayed to userspace. >>>> Exactly. >>>> As Maxime pointed out it should already be called by drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't >>>> because mode_config.poll_enabled isn't enabled. >>>> So we can either >>>> a) enable mode_config.poll_enabled so that it is called by drm_helper_hpd_irq_event() or >>>> b) make drm_kms_helper_hotplug_event() being called explicitly in dw_hdmi_irq(). >>>> We could guard that by mode_config.poll_enabled to avoid drm_kms_helper_hotplug_event() >>>> being called twice (but I think the "changed" mechanism will take care of). >>>>> I think this issue can be fixed by calling drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c. >>>> I don't see yet how this would solve it, but it may work. >>> dw_hdmi_irq() calls drm_bridge_hpd_notify(), which would call bridge->hpd_cb() if it was non-NULL. >> Ok, this is a case c). >> I vaguely remember having tried to analyse what bridge->hpd_cb is but stopped since it is NULL... >>> Calling drm_bridge_connector_enable_hpd() will set the bridge->hpd_cb() callback to point to drm_bridge_connector_hpd_cb(), which itself will call drm_kms_helper_hotplug_event(). Therefore, all that is missing is one call to drm_bridge_connector_enable_hpd(). >> Ah, ok, I see. >>>> Anyways, this all is a missing feature (sometimes called "bug") of the *dw-hdmi driver* and IMHO >>>> neither of the connector nor the ingenic-drm-drv. >> Well, a little more analysis shows that drm_bridge_connector_enable_hpd is called >> in the *-drv.c for some other plaforms: >> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-dev.c#L292 >> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-kms.c#L145 >> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/omapdrm/omap_drv.c#L393 >> https://elixir.bootlin.com/linux/v5.17-rc6/source/drivers/gpu/drm/msm/hdmi/hdmi.c#L317 >>>> So I think it should not be solved outside dw-hdmi. >> Hm. Can we call drm_bridge_connector_enable_hpd() from inside dw-hdmi? >> Or would this be the solution if merged? (I currently can't try code). >> https://lore.kernel.org/lkml/a7d0b013-6114-07b3-0a7b-0d17db8a3982@cogentembedded.com/T/ > > Looks correct to me. It has been reviewed by two people so I believe it will be merged very soon. Great. I will try asap. If it works we can drop all our private ideas... And focus on the last missing piece for jz4780 HDMI: the output format negotiation (which still is not working properly - but I have to analyse why). BR and thanks, Nikolaus
Hi Paul, > Am 04.03.2022 um 19:41 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > > > >> Am 04.03.2022 um 19:33 schrieb Paul Cercueil <paul@crapouillou.net>: >> >> >> >> Le ven., mars 4 2022 at 19:15:13 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >>> Hi Paul, >>>> Am 04.03.2022 um 19:04 schrieb Paul Cercueil <paul@crapouillou.net>: >>>> Le ven., mars 4 2022 at 18:51:14 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >>>>> Hi Paul, Neil, >>>>>> Am 04.03.2022 um 17:47 schrieb Paul Cercueil <paul@crapouillou.net>: >>>>>> From what I understood in Nikolaus' last message, HDMI hotplug is actually correctly detected, so there's no need for polling. What is missing is the call to drm_kms_helper_hotplug_event *somewhere*, so that the information is correctly relayed to userspace. >>>>> Exactly. >>>>> As Maxime pointed out it should already be called by drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't >>>>> because mode_config.poll_enabled isn't enabled. >>>>> So we can either >>>>> a) enable mode_config.poll_enabled so that it is called by drm_helper_hpd_irq_event() or >>>>> b) make drm_kms_helper_hotplug_event() being called explicitly in dw_hdmi_irq(). >>>>> We could guard that by mode_config.poll_enabled to avoid drm_kms_helper_hotplug_event() >>>>> being called twice (but I think the "changed" mechanism will take care of). >>>>>> I think this issue can be fixed by calling drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c. >>>>> I don't see yet how this would solve it, but it may work. >>>> dw_hdmi_irq() calls drm_bridge_hpd_notify(), which would call bridge->hpd_cb() if it was non-NULL. >>> Ok, this is a case c). >>> I vaguely remember having tried to analyse what bridge->hpd_cb is but stopped since it is NULL... >>>> Calling drm_bridge_connector_enable_hpd() will set the bridge->hpd_cb() callback to point to drm_bridge_connector_hpd_cb(), which itself will call drm_kms_helper_hotplug_event(). Therefore, all that is missing is one call to drm_bridge_connector_enable_hpd(). >>> Ah, ok, I see. >>>>> Anyways, this all is a missing feature (sometimes called "bug") of the *dw-hdmi driver* and IMHO >>>>> neither of the connector nor the ingenic-drm-drv. >>> Well, a little more analysis shows that drm_bridge_connector_enable_hpd is called >>> in the *-drv.c for some other plaforms: >>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-dev.c#L292 >>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-kms.c#L145 >>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/omapdrm/omap_drv.c#L393 >>> https://elixir.bootlin.com/linux/v5.17-rc6/source/drivers/gpu/drm/msm/hdmi/hdmi.c#L317 >>>>> So I think it should not be solved outside dw-hdmi. >>> Hm. Can we call drm_bridge_connector_enable_hpd() from inside dw-hdmi? >>> Or would this be the solution if merged? (I currently can't try code). >>> https://lore.kernel.org/lkml/a7d0b013-6114-07b3-0a7b-0d17db8a3982@cogentembedded.com/T/ >> >> Looks correct to me. It has been reviewed by two people so I believe it will be merged very soon. > > Great. I will try asap. If it works we can drop all our private ideas... > > And focus on the last missing piece for jz4780 HDMI: the output format negotiation (which still is not working properly - but I have to analyse why). Yes, it works. And I see you have merged it to drm-misc-next so that I can build on it. So there is only the bus format negotiation to be understood and finally solved for v17. Great and thanks, Nikolaus
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 4befc104d2200..43e375da131e8 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi) return 0; } +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable) +{ + if (hdmi->bridge.dev) + hdmi->bridge.dev->mode_config.poll_enabled = enable; + else + dev_warn(hdmi->dev, "no hdmi->bridge.dev"); +} +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll); + struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, const struct dw_hdmi_plat_data *plat_data) { diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h index 2a1f85f9a8a3f..963960794b40e 100644 --- a/include/drm/bridge/dw_hdmi.h +++ b/include/drm/bridge/dw_hdmi.h @@ -196,5 +196,6 @@ enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, bool force, bool disabled, bool rxsense); void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data); +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable); #endif /* __IMX_HDMI_H__ */
so that specialization drivers like ingenic-dw-hdmi can enable polling. Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++ include/drm/bridge/dw_hdmi.h | 1 + 2 files changed, 10 insertions(+)