Re: [PATCH v3 5/6] drm/bridge: imx: add driver for HDMI TX Parallel Audio Interface

[Liu Ying <victor.liu@nxp.com>]

On 08/08/2025, Shengjiu Wang wrote:
> On Fri, Aug 8, 2025 at 2:32 PM Liu Ying <victor.liu@nxp.com> wrote:
>>
>> On 08/07/2025, Shengjiu Wang wrote:
>>> On Wed, Aug 6, 2025 at 2:52 PM Liu Ying <victor.liu@nxp.com> wrote:
>>>>
>>>> On 08/06/2025, Shengjiu Wang wrote:
>>>>> On Tue, Aug 5, 2025 at 4:55 PM Liu Ying <victor.liu@nxp.com> wrote:
>>>>>>
>>>>>> On 08/04/2025, Shengjiu Wang wrote:
>>>>
>>>> [...]
>>>>
>>>>>>> +static int imx8mp_hdmi_pai_bind(struct device *dev, struct device *master, void *data)
>>>>>>> +{
>>>>>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
>>>>>>> +     struct imx8mp_hdmi_pai *hdmi_pai;
>>>>>>> +
>>>>>>> +     hdmi_pai = dev_get_drvdata(dev);
>>>>>>> +
>>>>>>> +     plat_data->enable_audio = imx8mp_hdmi_pai_enable;
>>>>>>> +     plat_data->disable_audio = imx8mp_hdmi_pai_disable;
>>>>>>> +     plat_data->priv_audio = hdmi_pai;
>>>>>>> +
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void imx8mp_hdmi_pai_unbind(struct device *dev, struct device *master, void *data)
>>>>>>> +{
>>>>>>> +     struct dw_hdmi_plat_data *plat_data = (struct dw_hdmi_plat_data *)data;
>>>>>>> +
>>>>>>> +     plat_data->enable_audio = NULL;
>>>>>>> +     plat_data->disable_audio = NULL;
>>>>>>> +     plat_data->priv_audio = NULL;
>>>>>>
>>>>>> Do you really need to set these ptrs to NULL?
>>>>>
>>>>> yes.  below code in dw-hdmi.c use the pdata->enable_audio as condition.
>>>>
>>>> Note that this is all about tearing down components.
>>>> If this is done properly as the below snippet of pseudo-code, then
>>>> hdmi->{enable,disable}_audio() and pdata->{enable,disable}_audio() won't be
>>>> called after audio device is removed by dw_hdmi_remove().  So, it's unnecessary
>>>> to set these pointers to NULL here.
>>>>
>>>> imx8mp_dw_hdmi_unbind()
>>>> {
>>>>    dw_hdmi_remove(); // platform_device_unregister(hdmi->audio);
>>>>    component_unbind_all(); //imx8mp_hdmi_pai_unbind()
>>>> }
>>>>
>>>> BTW, I suggest the below snippet[1] to bind components.
>>>>
>>>> imx8mp_dw_hdmi_bind()
>>>> {
>>>>    component_bind_all(); // imx8mp_hdmi_pai_bind()
>>>>                          //   set pdata->{enable,disable}_audio
>>>>    dw_hdmi_probe(); // hdmi->audio = platform_device_register_full(&pdevinfo);
>>>> }
>>>
>>> Looks like we should use dw_hdmi_bind() here to make unbind -> bind work.
>>
>> I don't get your idea here.
>>
>> What are you trying to make work?
>> Why dw_hdmi_probe() can't be used?
>> How does dw_hdmi_bind() help here?
> 
> bind() is ok.  but unbind(),  then bind() there is an issue.
> 
>>
>>> But can't get the encoder pointer.  the encoder pointer is from lcdif_drv.c,
>>> the probe sequence of lcdif, pvi, dw_hdmi should be dw_hdmi first, then pvi,
>>> then lcdif, because current implementation in lcdif and pvi driver.
>>
>> We use deferral probe to make sure the probe sequence is
>> DW_HDMI -> PVI -> LCDIF.
>>
>> LCDIF driver would call devm_drm_of_get_bridge() to get the next bridge PVI
>> and it defers probe if devm_drm_of_get_bridge() returns ERR_PTR(-EPROBE_DEFER).
>> Same to PVI driver, it would call of_drm_find_bridge() to get the next bridge
>> DW_HDMI and defers probe if needed.
> 
> right, probe is no problem,  but after probe,  if unbind pai, hdmi_tx,
>  then bind
> them again,  there is a problem,  because no one call the
> drm_bridge_attach() again.

In my mind, this is a common issue as DRM bridges are not properly detached
and attached again.
For now, only drm_encoder_cleanup() calls drm_bridge_detach().

Anyway, this issue is not introduced by this patch series, i.e. it's already
there.

> 
>>
>>>
>>> Should the lcdif and pvi driver be modified to use component helper?
>>
>> Why should they use component helper?
>>
>> BTW, I've tried testing the snippets suggested by me on i.MX8MP EVK and
>> the components bind successfully:
> 
> right, probe is no problem. but if try to unbind() then bind, there is issue.

I don't think the DRM bridge detach/attach issue would be addressed by
using component helper.

> 
> best regards
> shengjiu Wang
>>
>> cat /sys/kernel/debug/device_component/32fd8000.hdmi
>> aggregate_device name                                            status
>> -----------------------------------------------------------------------
>> 32fd8000.hdmi                                                     bound
>>
>> device name                                                      status
>> -----------------------------------------------------------------------
>> 32fc4800.audio-bridge                                             bound
>>
>>> This seems out of the scope of this patch set.
>>>
>>> Best regards
>>> Shengjiu Wang
>>
>> [...]
>>
>> --
>> Regards,
>> Liu Ying


-- 
Regards,
Liu Ying