[PATCH v4 2/2] drm/panel: Add device_link from panel device to drm device

[Peter Rosin]

On 2018-05-23 14:38, Andrzej Hajda wrote:
> On 23.05.2018 12:46, Peter Rosin wrote:
>> On 2018-05-23 11:39, Andrzej Hajda wrote:
>>
>> *snip*
>>
>>> Panels are managed by dsi host only if dsi host implements it, and it is
>>> up to dsi host author, it is not mandatory. The only case I am aware of
>>> is exynos-dsi. I guess most developers are not aware of the problem, or
>>> they do not care, or ... whatever. This patch shows it very clearly.
>> *snip*
>>
>>> With such approach you do not protect dsi hosts without dynamic panel
>>> management support. As I said earlier: it is matter of panel consumers
>>> (for example dsi host), ie if it implements dynamic panel management, it
>>> should attach panel without creating device_links, as it take care of
>>> disappearing panels. If it does not perform dynamic management it should
>>> rely on default behavior - links should be created, to protect drm
>>> device from using dangling pointers.
>> So, you're saying that the only place that needs to be patched to call
>> drm_panel_attach_without_link is exynos_dsi_host_attach()?
> 
> It should be at least there, but I suppose only there.
> 
>>
>> What about the dw-mipi-dsi driver in rockchip/dw-mipi-dsi.c? It also
>> calls drm_panel_attach/drm_panel_detach in its dw_mipi_dsi_host_attach
>> and dw_mipi_dsi_host_detach functions.
> 
> dw_mipi_dsi_host_detach calls only drm_panel_detach, dangling pointer
> will stay in dsi->panel, and nothing prevents from using it. I guess it
> is just buggy code.

Yeah, now I see it.

>>
>> Similar thing in bridge/synopsis/dw-mipi-dsi.c, but it calls the
>> wrappers drm_panel_bridge_add/remove to create an implicit bridge
>> that in turn handles the panel. So, do we need a to add a
>> drm_panel_bridge_add_without_link too?
> 
> Again looks like a buggy code, before panel removal it should be turned
> off, it should be performed under lock,....

Yeah, that looks like crap code.

>>
>> What about tegra/dsi.c? It also calls drm_panel_attach in its
>> tegra_dsi_host_attach function? But that one doesn't detach the
>> panel in its tegra_dsi_host_detach. Is that a bug?
> 
> Also looks like a bug, but this is dual-channel device so I am not so sure.

It's very confusing and hard to get a grip on how things are supposed
to be done when everything appears to be riddled with misunderstandings...

While browsing the code base I originally thought many more sites needed
the drm_panel_attach_without_link variant, but that appears to not be the
case. Thanks for your patient explanations!

Cheers,
Peter