Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[H. Nikolaus Schaller]

Hi Paul,

> Am 03.03.2022 um 17:51 schrieb Paul Cercueil <paul@xxxxxxxxxxxxxxx>:
> 
> Hi Nikolaus,
> 
> Le jeu., mars 3 2022 at 17:43:05 +0100, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> a écrit :
>> Hi Neil,
>>> Am 03.03.2022 um 17:30 schrieb H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>:
>>> Hi Neil,
>>>> Am 03.03.2022 um 17:23 schrieb Neil Armstrong <narmstrong@xxxxxxxxxxxx>:
>>>> 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@xxxxxxxxxxxxx>
>>>>> ---
>>>>> 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