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

On 30.08.2018 14:32, Linus Walleij wrote:

On Thu, Apr 26, 2018 at 10:07 AM Jyri Sarha jsarha@ti.com wrote:

...

Add device_link from panel device (supplier) to drm device (consumer) when drm_panel_attach() is called. This patch should protect the master drm driver if an attached panel driver unbinds while it is in use. The device_link should make sure the drm device is unbound before the panel driver becomes unavailable.

The device_link is removed when drm_panel_detach() is called. The drm_panel_detach() should be called by the consumer DRM driver, not the panel driver, otherwise both drivers are racing to delete the same link.

Signed-off-by: Jyri Sarha jsarha@ti.com Reviewed-by: Eric Anholt eric@anholt.net

Hi I have a problem with an in-development driver and this patch.

Do not worry! It is no regression. I just need help to do things right now.

My panel is a DSI device on a DSI master:

            mcde@a0350000 {
                    status = "okay";

                    dsi@a0351000 {
                            port {
                                    dsi0_out: endpoint {
                                            remote-endpoint = <&panel_in>;
                                    };
                            };

                            panel: display {
                                    compatible = "samsung,s6d16d0";
                                    reg = <0>;
                                    /* VDD1 on the component */
                                    power-supply = <&ab8500_ldo_aux1_reg>;
                                    reset-gpios = <&gpio2 1

GPIO_ACTIVE_LOW>;

                                    port {
                                            panel_in: endpoint {

remote-endpoint = <&dsi0_out>; }; }; }; }; };

So in my DSI host driver .bind() I do:

drm_encoder_init(drm, encoder, &mcde_dsi_encoder_funcs,
         DRM_MODE_ENCODER_DSI, NULL);
drm_encoder_helper_add(encoder, &mcde_dsi_encoder_helper_funcs);
ret = drm_connector_init(encoder->dev, connector,
             &mcde_dsi_connector_funcs,
             DRM_MODE_CONNECTOR_DSI);

drm_connector_helper_add(connector, &mcde_dsi_connector_helper_funcs);
drm_connector_attach_encoder(connector, encoder);
drm_connector_register(connector);

This works fine. I managed to create the encoder and connector for this DSI. Then:

ret = drm_of_find_panel_or_bridge(dev->of_node,
                  0, 0, &panel, &bridge);
if (panel) {
    bridge = drm_panel_bridge_add(panel,
                      DRM_MODE_CONNECTOR_DSI);
    if (IS_ERR(bridge)) {
        dev_err(dev, "error adding panel bridge\n");
        return PTR_ERR(bridge);
    }
    dev_info(dev, "connected to panel\n");
    d->panel = panel;
}
d->connector.status = connector_status_connected;
ret = drm_bridge_attach(encoder, bridge, NULL);

And this is where it blows up: it fails in device_link_add(connector->dev->dev, panel->dev, 0); device_is_dependent(consumer, supplier) because the consumer (connector) is dependent on the supplier (bridge).

This happens because the connector struct device is the same as the bridge struct device, I suppose.

I guess it is rather because the code tries to make circular dependency: 1. panel depends on dsi-host because it is MIPI-DSI child device. 2. dsi-host probably depends on drm parent device (connector->dev->dev) - what drm driver do you use? 3. drm parent dev depends on panel: this patch adds this dependency.

If 2nd point is true it becomes circular dependency, but please verify it.

Regards Andrzej

Isn't that OK? Please help me to see what's wrong with my architecture here, and what I need to do :/ I tried to follow examples set by MSM and Exynos DSI.

Yours, Linus Walleij _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel