[PATCH v2 00/26] device link, bridge supplier <-> drm device

[PATCH v2 00/26] device link, bridge supplier <-> drm device

[Peter Rosin]

Hi!

It was noted by Russel King [1] that bridges (not using components)
might disappear unexpectedly if the owner of the bridge was unbound.
Jyri Sarha had previously noted the same thing with panels [2]. Jyri
came up with using device links to resolve the panel issue, which
was also my (independent) reaction to the note from Russel.

This series builds up to the addition of that link in the last
patch, but in my opinion the other 25 patches do have merit on their
own.

The last patch needs testing, while the others look trivial. Jyri, are
you able to test? That said, I might have missed some subtlety.

Oh and the reason I'm pushing this is of course so that the issue
noted by Russel in [1] is addressed which in turn means that the
tda998x bridge driver can be patched according to that series without
objection (hopefully) and then used from the atmel-hlcdc driver (and
other drivers that are not componentized).

Changes since v1    https://lkml.org/lkml/2018/4/26/1018

- rename .owner to .odev to not get mixed up with the module owner.
- added patches for new recent drivers thc63lvd1024 and cdns-dsi
- fix for problem in the rockchip_lvds driver reported by 0day
- added a WARN in drm_bridge_add if there is no .odev owner device

I did *not*:
- add any ack from Daniel since he suggested "pdev", and I ended up
  with "odev" in the rename since I disliked "pdev" about as much
  as "owner".
- add any port id. The current .of_node (that this series removes)
  does not identify the port, so that problem seems orthogonal
  to me.

Cheers,
Peter

[1] https://lkml.org/lkml/2018/4/23/769
[2] https://www.spinics.net/lists/dri-devel/msg174275.html

Peter Rosin (26):
  drm/bridge: allow optionally specifying an owner .odev device
  drm/bridge: adv7511: provide an owner .odev device
  drm/bridge/analogix: core: specify the owner .odev of the bridge
  drm/bridge: analogix-anx78xx: provide an owner .odev device
  drm/bridge: cdns-dsi: provide an owner .odev device
  drm/bridge: vga-dac: provide an owner .odev device
  drm/bridge: lvds-encoder: provide an owner .odev device
  drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: provide an owner .odev
    device
  drm/bridge: nxp-ptn3460: provide an owner .odev device
  drm/bridge: panel: provide an owner .odev device
  drm/bridge: ps8622: provide an owner .odev device
  drm/bridge: sii902x: provide an owner .odev device
  drm/bridge: sii9234: provide an owner .odev device
  drm/bridge: sii8620: provide an owner .odev device
  drm/bridge: synopsys: provide an owner .odev device for the bridges
  drm/bridge: tc358767: provide an owner .odev device
  drm/bridge: thc63lvd1024: provide an owner .odev device
  drm/bridge: ti-tfp410: provide an owner .odev device
  drm/exynos: mic: provide an owner .odev device for the bridge
  drm/mediatek: hdmi: provide an owner .odev device for the bridge
  drm/msm: specify the owner .odev of the bridges
  drm/rcar-du: lvds: provide an owner .odev device for the bridge
  drm/sti: provide an owner .odev device for the bridges
  drm/bridge: remove the .of_node member
  drm/bridge: require the owner .odev to be filled in on
    drm_bridge_add/attach
  drm/bridge: establish a link between the bridge supplier and consumer

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       |  2 +-
 drivers/gpu/drm/bridge/analogix-anx78xx.c          |  5 +----
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
 drivers/gpu/drm/bridge/cdns-dsi.c                  |  2 +-
 drivers/gpu/drm/bridge/dumb-vga-dac.c              |  2 +-
 drivers/gpu/drm/bridge/lvds-encoder.c              |  2 +-
 .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c   |  2 +-
 drivers/gpu/drm/bridge/nxp-ptn3460.c               |  2 +-
 drivers/gpu/drm/bridge/panel.c                     |  4 +---
 drivers/gpu/drm/bridge/parade-ps8622.c             |  2 +-
 drivers/gpu/drm/bridge/sii902x.c                   |  2 +-
 drivers/gpu/drm/bridge/sii9234.c                   |  2 +-
 drivers/gpu/drm/bridge/sil-sii8620.c               |  2 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          |  4 +---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c      |  4 +---
 drivers/gpu/drm/bridge/tc358767.c                  |  2 +-
 drivers/gpu/drm/bridge/thc63lvd1024.c              |  2 +-
 drivers/gpu/drm/bridge/ti-tfp410.c                 |  2 +-
 drivers/gpu/drm/drm_bridge.c                       | 26 +++++++++++++++++++++-
 drivers/gpu/drm/exynos/exynos_drm_mic.c            |  2 +-
 drivers/gpu/drm/mediatek/mtk_hdmi.c                |  2 +-
 drivers/gpu/drm/msm/dsi/dsi_manager.c              |  1 +
 drivers/gpu/drm/msm/edp/edp_bridge.c               |  1 +
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c             |  1 +
 drivers/gpu/drm/rcar-du/rcar_lvds.c                |  2 +-
 drivers/gpu/drm/rockchip/rockchip_lvds.c           |  2 +-
 drivers/gpu/drm/sti/sti_dvo.c                      |  2 +-
 drivers/gpu/drm/sti/sti_hda.c                      |  1 +
 drivers/gpu/drm/sti/sti_hdmi.c                     |  1 +
 include/drm/drm_bridge.h                           |  8 +++----
 30 files changed, 57 insertions(+), 36 deletions(-)

-- 
2.11.0

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH v2 02/26] drm/bridge: adv7511: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 2614cea538e2..fd2eef916b0b 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1204,8 +1204,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	if (ret)
 		goto err_unregister_cec;
 
+	adv7511->bridge.odev = dev;
 	adv7511->bridge.funcs = &adv7511_bridge_funcs;
-	adv7511->bridge.of_node = dev->of_node;
 
 	drm_bridge_add(&adv7511->bridge);
 
-- 
2.11.0

[PATCH v2 03/26] drm/bridge/analogix: core: specify the owner .odev of the bridge

[Peter Rosin]

This will become mandatory.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 2bcbfadb6ac5..c60f29216213 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1467,6 +1467,7 @@ static int analogix_dp_create_bridge(struct drm_device *drm_dev,
 
 	dp->bridge = bridge;
 
+	bridge->odev = dp->dev;
 	bridge->driver_private = dp;
 	bridge->funcs = &analogix_dp_bridge_funcs;
 
-- 
2.11.0

[PATCH v2 04/26] drm/bridge: analogix-anx78xx: provide an owner .odev device

[Peter Rosin]

It gets rid of an #if and the .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
index b49043866be6..1d6620aedbdb 100644
--- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
@@ -1332,10 +1332,6 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
 
 	mutex_init(&anx78xx->lock);
 
-#if IS_ENABLED(CONFIG_OF)
-	anx78xx->bridge.of_node = client->dev.of_node;
-#endif
-
 	anx78xx->client = client;
 	i2c_set_clientdata(client, anx78xx);
 
@@ -1433,6 +1429,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
 		goto err_poweroff;
 	}
 
+	anx78xx->bridge.odev = &client->dev;
 	anx78xx->bridge.funcs = &anx78xx_bridge_funcs;
 
 	drm_bridge_add(&anx78xx->bridge);
-- 
2.11.0

[PATCH v2 05/26] drm/bridge: cdns-dsi: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/cdns-dsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
index c255fc3e1be5..e9be5c3f0284 100644
--- a/drivers/gpu/drm/bridge/cdns-dsi.c
+++ b/drivers/gpu/drm/bridge/cdns-dsi.c
@@ -1548,8 +1548,8 @@ static int cdns_dsi_drm_probe(struct platform_device *pdev)
 	 * CDNS_DPI_INPUT.
 	 */
 	input->id = CDNS_DPI_INPUT;
+	input->bridge.odev = &pdev->dev;
 	input->bridge.funcs = &cdns_dsi_bridge_funcs;
-	input->bridge.of_node = pdev->dev.of_node;
 
 	/* Mask all interrupts before registering the IRQ handler. */
 	writel(0, dsi->regs + MCTL_MAIN_STS_CTL);
-- 
2.11.0

[PATCH v2 06/26] drm/bridge: vga-dac: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/dumb-vga-dac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index 9837c8d69e69..95cce18e8943 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -205,8 +205,8 @@ static int dumb_vga_probe(struct platform_device *pdev)
 		}
 	}
 
+	vga->bridge.odev = &pdev->dev;
 	vga->bridge.funcs = &dumb_vga_bridge_funcs;
-	vga->bridge.of_node = pdev->dev.of_node;
 	vga->bridge.timings = of_device_get_match_data(&pdev->dev);
 
 	drm_bridge_add(&vga->bridge);
-- 
2.11.0

[PATCH v2 07/26] drm/bridge: lvds-encoder: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/lvds-encoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
index 75b0d3f6e4de..a80eec17b13b 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -83,7 +83,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 	 * but we need a bridge attached to our of_node for our user
 	 * to look up.
 	 */
-	lvds_encoder->bridge.of_node = pdev->dev.of_node;
+	lvds_encoder->bridge.odev = &pdev->dev;
 	lvds_encoder->bridge.funcs = &funcs;
 	drm_bridge_add(&lvds_encoder->bridge);
 
-- 
2.11.0

[PATCH v2 08/26] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index 7ccadba7c98c..29d1b5ae5fb6 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -313,8 +313,8 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
 	i2c_set_clientdata(stdp4028_i2c, ge_b850v3_lvds_ptr);
 
 	/* drm bridge initialization */
+	ge_b850v3_lvds_ptr->bridge.odev = dev;
 	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
-	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
 
 	/* Clear pending interrupts since power up. */
-- 
2.11.0

[PATCH v2 09/26] drm/bridge: nxp-ptn3460: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/nxp-ptn3460.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index d64a3283822a..fa832f32b518 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -329,8 +329,8 @@ static int ptn3460_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	ptn_bridge->bridge.odev = dev;
 	ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs;
-	ptn_bridge->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ptn_bridge->bridge);
 
 	i2c_set_clientdata(client, ptn_bridge);
-- 
2.11.0

[PATCH v2 10/26] drm/bridge: panel: provide an owner .odev device

[Peter Rosin]

It gets rid of an #ifdef and the .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/panel.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 6d99d4a3beb3..f43d77b5ed20 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -169,10 +169,8 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
 	panel_bridge->connector_type = connector_type;
 	panel_bridge->panel = panel;
 
+	panel_bridge->bridge.odev = panel->dev;
 	panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs;
-#ifdef CONFIG_OF
-	panel_bridge->bridge.of_node = panel->dev->of_node;
-#endif
 
 	drm_bridge_add(&panel_bridge->bridge);
 
-- 
2.11.0

Re: [PATCH v2 10/26] drm/bridge: panel: provide an owner .odev device

[Jyri Sarha]

On 05/04/18 16:51, Peter Rosin wrote:
> It gets rid of an #ifdef and the .of_node member is going away.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/bridge/panel.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 6d99d4a3beb3..f43d77b5ed20 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -169,10 +169,8 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>  	panel_bridge->connector_type = connector_type;
>  	panel_bridge->panel = panel;
>  
> +	panel_bridge->bridge.odev = panel->dev;

I am afraid this approach will eventually conflict with my lately
accepted patch[1].

The panel already has a device pointer of its own, and technically the
bridge element created here is created by the master drm device itself.

I suggest assigning odev here to NULL or to master drm device itself.

Best regards,
Jyri

>  	panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs;
> -#ifdef CONFIG_OF
> -	panel_bridge->bridge.of_node = panel->dev->of_node;
> -#endif
>  
>  	drm_bridge_add(&panel_bridge->bridge);
>  
> 

[1] https://lists.freedesktop.org/archives/dri-devel/2018-April/174559.html
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v2 10/26] drm/bridge: panel: provide an owner .odev device

[Peter Rosin]

On 2018-05-08 08:51, Jyri Sarha wrote:
> On 05/04/18 16:51, Peter Rosin wrote:
>> It gets rid of an #ifdef and the .of_node member is going away.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/bridge/panel.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
>> index 6d99d4a3beb3..f43d77b5ed20 100644
>> --- a/drivers/gpu/drm/bridge/panel.c
>> +++ b/drivers/gpu/drm/bridge/panel.c
>> @@ -169,10 +169,8 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>>  	panel_bridge->connector_type = connector_type;
>>  	panel_bridge->panel = panel;
>>  
>> +	panel_bridge->bridge.odev = panel->dev;
> 
> I am afraid this approach will eventually conflict with my lately
> accepted patch[1].

I don't see how? The links are refcounted. So, if there is one link
each for the panel and bridge between the drm device and the panel
device that link will simply get two references. If/when the panel
device then goes away, the drm device will be brought down because
of that link (with two references, but that is irrelevant). When
the drm device is brought down, it will (presumably) bring down the
bridge as well (which will fix the refcount as the bridge link is
killed as part of that).

Or have you done some test and seen an actual problem?

> The panel already has a device pointer of its own, and technically the
> bridge element created here is created by the master drm device itself.

Not always, some bridges also call drm_panel_bridge_add for connecting
its output to either a panel or another bridge.

And by that line of argument, the devm_kzalloc in drm_panel_bridge_add
attaches the bridge memory to the wrong device as well. Maybe that
should be fixed? Seems orthogonal though, but it would introduce a
natural struct device in that function to assign to .odev. I think
the device owning the bridge memory should be the same as the .odev
device of the bridge.

> I suggest assigning odev here to NULL or to master drm device itself.

I'd rather not use NULL, since it is nice to be able to rely on the
.odev being there, and WARN if it isn't.

Cheers,
Peter

> Best regards,
> Jyri
> 
>>  	panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs;
>> -#ifdef CONFIG_OF
>> -	panel_bridge->bridge.of_node = panel->dev->of_node;
>> -#endif
>>  
>>  	drm_bridge_add(&panel_bridge->bridge);
>>  
>>
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2018-April/174559.html
> 

Re: [PATCH v2 10/26] drm/bridge: panel: provide an owner .odev device

[Daniel Vetter]

On Tue, May 08, 2018 at 09:58:49AM +0200, Peter Rosin wrote:
> On 2018-05-08 08:51, Jyri Sarha wrote:
> > On 05/04/18 16:51, Peter Rosin wrote:
> >> It gets rid of an #ifdef and the .of_node member is going away.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>  drivers/gpu/drm/bridge/panel.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> >> index 6d99d4a3beb3..f43d77b5ed20 100644
> >> --- a/drivers/gpu/drm/bridge/panel.c
> >> +++ b/drivers/gpu/drm/bridge/panel.c
> >> @@ -169,10 +169,8 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> >>  	panel_bridge->connector_type = connector_type;
> >>  	panel_bridge->panel = panel;
> >>  
> >> +	panel_bridge->bridge.odev = panel->dev;
> > 
> > I am afraid this approach will eventually conflict with my lately
> > accepted patch[1].
> 
> I don't see how? The links are refcounted. So, if there is one link
> each for the panel and bridge between the drm device and the panel
> device that link will simply get two references. If/when the panel
> device then goes away, the drm device will be brought down because
> of that link (with two references, but that is irrelevant). When
> the drm device is brought down, it will (presumably) bring down the
> bridge as well (which will fix the refcount as the bridge link is
> killed as part of that).
> 
> Or have you done some test and seen an actual problem?
> 
> > The panel already has a device pointer of its own, and technically the
> > bridge element created here is created by the master drm device itself.
> 
> Not always, some bridges also call drm_panel_bridge_add for connecting
> its output to either a panel or another bridge.
> 
> And by that line of argument, the devm_kzalloc in drm_panel_bridge_add
> attaches the bridge memory to the wrong device as well. Maybe that
> should be fixed? Seems orthogonal though, but it would introduce a
> natural struct device in that function to assign to .odev. I think
> the device owning the bridge memory should be the same as the .odev
> device of the bridge.

Drive-by comment: You can't allocate anything with devm_* functions that
represents a core drm struct attached to a drm_device. There's no struct
device anywhere that has the right lifetime (since the drm_device can
easily outlive any physical device).

Yes that makes roughly 100% of all armsoc drm drivers buggy :-/ But it
doesn't matter, since you can't really unplug those devices physically,
hence will only blow up if you force an unbind through sysfs, and then you
get to keep all the pieces. Ignorance is bliss meanwhile ...
-Daniel

> 
> > I suggest assigning odev here to NULL or to master drm device itself.
> 
> I'd rather not use NULL, since it is nice to be able to rely on the
> .odev being there, and WARN if it isn't.
> 
> Cheers,
> Peter
> 
> > Best regards,
> > Jyri
> > 
> >>  	panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs;
> >> -#ifdef CONFIG_OF
> >> -	panel_bridge->bridge.of_node = panel->dev->of_node;
> >> -#endif
> >>  
> >>  	drm_bridge_add(&panel_bridge->bridge);
> >>  
> >>
> > 
> > [1] https://lists.freedesktop.org/archives/dri-devel/2018-April/174559.html
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 10/26] drm/bridge: panel: provide an owner .odev device

[Jyri Sarha]

On 05/08/18 10:58, Peter Rosin wrote:
> On 2018-05-08 08:51, Jyri Sarha wrote:
>> On 05/04/18 16:51, Peter Rosin wrote:
>>> It gets rid of an #ifdef and the .of_node member is going away.
>>>
>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>> ---
>>>  drivers/gpu/drm/bridge/panel.c | 4 +---
>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
>>> index 6d99d4a3beb3..f43d77b5ed20 100644
>>> --- a/drivers/gpu/drm/bridge/panel.c
>>> +++ b/drivers/gpu/drm/bridge/panel.c
>>> @@ -169,10 +169,8 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>>>  	panel_bridge->connector_type = connector_type;
>>>  	panel_bridge->panel = panel;
>>>  
>>> +	panel_bridge->bridge.odev = panel->dev;
>> I am afraid this approach will eventually conflict with my lately
>> accepted patch[1].
> I don't see how? The links are refcounted. So, if there is one link
> each for the panel and bridge between the drm device and the panel
> device that link will simply get two references. If/when the panel
> device then goes away, the drm device will be brought down because
> of that link (with two references, but that is irrelevant). When
> the drm device is brought down, it will (presumably) bring down the
> bridge as well (which will fix the refcount as the bridge link is
> killed as part of that).
> 

I guess you are right. If everything is done correctly the both links
should get removed in the tear down situation and all should be fine.


> Or have you done some test and seen an actual problem?
> 

No testing, just a hunch.

BR,
Jyri

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 11/26] drm/bridge: ps8622: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/parade-ps8622.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
index 81198f5e9afa..957420a1c924 100644
--- a/drivers/gpu/drm/bridge/parade-ps8622.c
+++ b/drivers/gpu/drm/bridge/parade-ps8622.c
@@ -595,8 +595,8 @@ static int ps8622_probe(struct i2c_client *client,
 		ps8622->bl->props.brightness = PS8622_MAX_BRIGHTNESS;
 	}
 
+	ps8622->bridge.odev = dev;
 	ps8622->bridge.funcs = &ps8622_bridge_funcs;
-	ps8622->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ps8622->bridge);
 
 	i2c_set_clientdata(client, ps8622);
-- 
2.11.0

[PATCH v2 12/26] drm/bridge: sii902x: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/sii902x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 60373d7eb220..894525b05985 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -427,8 +427,8 @@ static int sii902x_probe(struct i2c_client *client,
 			return ret;
 	}
 
+	sii902x->bridge.odev = dev;
 	sii902x->bridge.funcs = &sii902x_bridge_funcs;
-	sii902x->bridge.of_node = dev->of_node;
 	drm_bridge_add(&sii902x->bridge);
 
 	i2c_set_clientdata(client, sii902x);
-- 
2.11.0

[PATCH v2 13/26] drm/bridge: sii9234: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/sii9234.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c
index c77000626c22..54326357b2ee 100644
--- a/drivers/gpu/drm/bridge/sii9234.c
+++ b/drivers/gpu/drm/bridge/sii9234.c
@@ -948,8 +948,8 @@ static int sii9234_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, ctx);
 
+	ctx->bridge.odev = dev;
 	ctx->bridge.funcs = &sii9234_bridge_funcs;
-	ctx->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ctx->bridge);
 
 	sii9234_cable_in(ctx);
-- 
2.11.0

[PATCH v2 14/26] drm/bridge: sii8620: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/sil-sii8620.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 7ab36042a822..8e35578b0488 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2387,8 +2387,8 @@ static int sii8620_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, ctx);
 
+	ctx->bridge.odev = dev;
 	ctx->bridge.funcs = &sii8620_bridge_funcs;
-	ctx->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ctx->bridge);
 
 	if (!ctx->extcon)
-- 
2.11.0

[PATCH v2 15/26] drm/bridge: synopsys: provide an owner .odev device for the bridges

[Peter Rosin]

It gets rid of two #ifdefs and the .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c     | 4 +---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index ec8d0006ef7c..e3956a7e827c 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2471,11 +2471,9 @@ __dw_hdmi_probe(struct platform_device *pdev,
 			hdmi->ddc = NULL;
 	}
 
+	hdmi->bridge.odev = dev;
 	hdmi->bridge.driver_private = hdmi;
 	hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
-#ifdef CONFIG_OF
-	hdmi->bridge.of_node = pdev->dev.of_node;
-#endif
 
 	dw_hdmi_setup_i2c(hdmi);
 	if (hdmi->phy.ops->setup_hpd)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index fd7999642cf8..7c8d05f7cecd 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -930,11 +930,9 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
 		return ERR_PTR(ret);
 	}
 
+	dsi->bridge.odev = dev;
 	dsi->bridge.driver_private = dsi;
 	dsi->bridge.funcs = &dw_mipi_dsi_bridge_funcs;
-#ifdef CONFIG_OF
-	dsi->bridge.of_node = pdev->dev.of_node;
-#endif
 
 	return dsi;
 }
-- 
2.11.0

[PATCH v2 16/26] drm/bridge: tc358767: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/tc358767.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 0fd9cf27542c..75f93e1d0bf5 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1331,8 +1331,8 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	tc_connector_set_polling(tc, &tc->connector);
 
+	tc->bridge.odev = dev;
 	tc->bridge.funcs = &tc_bridge_funcs;
-	tc->bridge.of_node = dev->of_node;
 	drm_bridge_add(&tc->bridge);
 
 	i2c_set_clientdata(client, tc);
-- 
2.11.0

[PATCH v2 17/26] drm/bridge: thc63lvd1024: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/thc63lvd1024.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
index c8b9edd5a7f4..4765c9c45aef 100644
--- a/drivers/gpu/drm/bridge/thc63lvd1024.c
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -167,8 +167,8 @@ static int thc63_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	thc63->bridge.odev = &pdev->dev;
 	thc63->bridge.driver_private = thc63;
-	thc63->bridge.of_node = pdev->dev.of_node;
 	thc63->bridge.funcs = &thc63_bridge_func;
 
 	drm_bridge_add(&thc63->bridge);
-- 
2.11.0

[PATCH v2 18/26] drm/bridge: ti-tfp410: provide an owner .odev device

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index acb857030951..4745838fdf0e 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -215,8 +215,8 @@ static int tfp410_init(struct device *dev)
 		return -ENOMEM;
 	dev_set_drvdata(dev, dvi);
 
+	dvi->bridge.odev = dev;
 	dvi->bridge.funcs = &tfp410_bridge_funcs;
-	dvi->bridge.of_node = dev->of_node;
 	dvi->dev = dev;
 
 	ret = tfp410_get_connector_properties(dvi);
-- 
2.11.0

[PATCH v2 19/26] drm/exynos: mic: provide an owner .odev device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/exynos/exynos_drm_mic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c b/drivers/gpu/drm/exynos/exynos_drm_mic.c
index 2174814273e2..f9ff8d3ec937 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_mic.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
@@ -417,8 +417,8 @@ static int exynos_mic_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mic);
 
+	mic->bridge.odev = dev;
 	mic->bridge.funcs = &mic_bridge_funcs;
-	mic->bridge.of_node = dev->of_node;
 
 	drm_bridge_add(&mic->bridge);
 
-- 
2.11.0

[PATCH v2 20/26] drm/mediatek: hdmi: provide an owner .odev device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 59a11026dceb..d8c7d93d0a87 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -1694,8 +1694,8 @@ static int mtk_drm_hdmi_probe(struct platform_device *pdev)
 
 	mtk_hdmi_register_audio_driver(dev);
 
+	hdmi->bridge.odev = &pdev->dev;
 	hdmi->bridge.funcs = &mtk_hdmi_bridge_funcs;
-	hdmi->bridge.of_node = pdev->dev.of_node;
 	drm_bridge_add(&hdmi->bridge);
 
 	ret = mtk_hdmi_clk_enable_audio(hdmi);
-- 
2.11.0

[PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Peter Rosin]

Bridge drivers can now (temporarily, in a transition phase) select if
they want to provide a full owner device or keep just providing an
of_node.

By providing a full owner device, the bridge drivers no longer need
to provide an of_node since that node is available via the owner
device.

When all bridge drivers provide an owner device, that will become
mandatory and the .of_node member will be removed.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c             | 3 ++-
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
 include/drm/drm_bridge.h                 | 2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 1638bfe9627c..3872f5379998 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 	mutex_lock(&bridge_lock);
 
 	list_for_each_entry(bridge, &bridge_list, list) {
-		if (bridge->of_node == np) {
+		if ((bridge->odev && bridge->odev->of_node == np) ||
+		    bridge->of_node == np) {
 			mutex_unlock(&bridge_lock);
 			return bridge;
 		}
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 4bd94b167d2c..557e0079c98d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 	}
 	if (lvds->panel)
 		remote = lvds->panel->dev->of_node;
-	else
+	else if (lvds->bridge->of_node)
 		remote = lvds->bridge->of_node;
+	else
+		remote = lvds->bridge->odev->of_node;
 	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
 		/* default set it as output rgb */
 		lvds->output = DISPLAY_OUTPUT_RGB;
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3270fec46979..7c17977c3537 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -254,6 +254,7 @@ struct drm_bridge_timings {
 
 /**
  * struct drm_bridge - central DRM bridge control structure
+ * @odev: device that owns the bridge
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
@@ -265,6 +266,7 @@ struct drm_bridge_timings {
  * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+	struct device *odev;
 	struct drm_device *dev;
 	struct drm_encoder *encoder;
 	struct drm_bridge *next;
-- 
2.11.0

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Andrzej Hajda]

On 04.05.2018 15:51, Peter Rosin wrote:
> Bridge drivers can now (temporarily, in a transition phase) select if
> they want to provide a full owner device or keep just providing an
> of_node.
>
> By providing a full owner device, the bridge drivers no longer need
> to provide an of_node since that node is available via the owner
> device.
>
> When all bridge drivers provide an owner device, that will become
> mandatory and the .of_node member will be removed.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/drm_bridge.c             | 3 ++-
>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-

What is the reason to put rockchip here? Shouldn't be in separate patch?

>  include/drm/drm_bridge.h                 | 2 ++
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..3872f5379998 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>  	mutex_lock(&bridge_lock);
>  
>  	list_for_each_entry(bridge, &bridge_list, list) {
> -		if (bridge->of_node == np) {
> +		if ((bridge->odev && bridge->odev->of_node == np) ||
> +		    bridge->of_node == np) {
>  			mutex_unlock(&bridge_lock);
>  			return bridge;
>  		}
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 4bd94b167d2c..557e0079c98d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  	}
>  	if (lvds->panel)
>  		remote = lvds->panel->dev->of_node;
> -	else
> +	else if (lvds->bridge->of_node)
>  		remote = lvds->bridge->of_node;
> +	else
> +		remote = lvds->bridge->odev->of_node;

I guess odev should be NULL here, or have I missed something.

Regards
Andrzej

>  	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
>  		/* default set it as output rgb */
>  		lvds->output = DISPLAY_OUTPUT_RGB;
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3270fec46979..7c17977c3537 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -254,6 +254,7 @@ struct drm_bridge_timings {
>  
>  /**
>   * struct drm_bridge - central DRM bridge control structure
> + * @odev: device that owns the bridge
>   * @dev: DRM device this bridge belongs to
>   * @encoder: encoder to which this bridge is connected
>   * @next: the next bridge in the encoder chain
> @@ -265,6 +266,7 @@ struct drm_bridge_timings {
>   * @driver_private: pointer to the bridge driver's internal context
>   */
>  struct drm_bridge {
> +	struct device *odev;
>  	struct drm_device *dev;
>  	struct drm_encoder *encoder;
>  	struct drm_bridge *next;


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

Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Peter Rosin]

On 2018-05-09 17:08, Andrzej Hajda wrote:
> On 04.05.2018 15:51, Peter Rosin wrote:
>> Bridge drivers can now (temporarily, in a transition phase) select if
>> they want to provide a full owner device or keep just providing an
>> of_node.
>>
>> By providing a full owner device, the bridge drivers no longer need
>> to provide an of_node since that node is available via the owner
>> device.
>>
>> When all bridge drivers provide an owner device, that will become
>> mandatory and the .of_node member will be removed.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c             | 3 ++-
>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
> 
> What is the reason to put rockchip here? Shouldn't be in separate patch?

Because the rockchip driver peeks into the bridge struct and all the
changes in this patch relate to making the new .odev member optional in
the transition phase, when the bridge can have either a new-style odev
or an old style of_node.

I guess this rockchip change could be patch 2, but it has to come first
after this patch and it makes no sense on its own. Hence, one patch.

This rockchip_lvds interaction is also present in patch 24/26
drm/bridge: remove the .of_node member

I can split them if you want, but I personally don't see the point.

>>  include/drm/drm_bridge.h                 | 2 ++
>>  3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 1638bfe9627c..3872f5379998 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -365,7 +365,8 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>>  	mutex_lock(&bridge_lock);
>>  
>>  	list_for_each_entry(bridge, &bridge_list, list) {
>> -		if (bridge->of_node == np) {
>> +		if ((bridge->odev && bridge->odev->of_node == np) ||
>> +		    bridge->of_node == np) {
>>  			mutex_unlock(&bridge_lock);
>>  			return bridge;
>>  		}
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> index 4bd94b167d2c..557e0079c98d 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> @@ -377,8 +377,10 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>>  	}
>>  	if (lvds->panel)
>>  		remote = lvds->panel->dev->of_node;
>> -	else
>> +	else if (lvds->bridge->of_node)
>>  		remote = lvds->bridge->of_node;
>> +	else
>> +		remote = lvds->bridge->odev->of_node;
> 
> I guess odev should be NULL here, or have I missed something.

s/should/could/ ???

Assuming that was what you meant and answering accordingly...

No, .odev cannot be NULL in that else branch. drm_of_find_panel_or_bridge
either found a panel or a bridge (or it failed). If it found a bridge
(which is the relevant branch for this question) that bridge would have
to have either an of_node (in the transition phase) or a valid .odev.
Otherwise the bridge could not have been found by
drm_of_find_panel_or_bridge.

*time passes*

Ahh, yes, .odev is always NULL here so you probably did mean "should".
But after patches 2-23 when bridges start populating .odev *instead*
of .of_node, .odev will not remain NULL. But as I said above, while
.odev is NULL, .of_node will never be NULL and vice versa (or
drm_of_find_panel_or_bridge could not have found the bridge) so there
is no risk of a NULL dereference.

Cheers,
Peter

> 
> Regards
> Andrzej
> 
>>  	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
>>  		/* default set it as output rgb */
>>  		lvds->output = DISPLAY_OUTPUT_RGB;
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 3270fec46979..7c17977c3537 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -254,6 +254,7 @@ struct drm_bridge_timings {
>>  
>>  /**
>>   * struct drm_bridge - central DRM bridge control structure
>> + * @odev: device that owns the bridge
>>   * @dev: DRM device this bridge belongs to
>>   * @encoder: encoder to which this bridge is connected
>>   * @next: the next bridge in the encoder chain
>> @@ -265,6 +266,7 @@ struct drm_bridge_timings {
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>>  struct drm_bridge {
>> +	struct device *odev;
>>  	struct drm_device *dev;
>>  	struct drm_encoder *encoder;
>>  	struct drm_bridge *next;
> 
> 

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Peter Rosin]

On 2018-05-09 17:53, Peter Rosin wrote:
> On 2018-05-09 17:08, Andrzej Hajda wrote:
>> On 04.05.2018 15:51, Peter Rosin wrote:
>>> Bridge drivers can now (temporarily, in a transition phase) select if
>>> they want to provide a full owner device or keep just providing an
>>> of_node.
>>>
>>> By providing a full owner device, the bridge drivers no longer need
>>> to provide an of_node since that node is available via the owner
>>> device.
>>>
>>> When all bridge drivers provide an owner device, that will become
>>> mandatory and the .of_node member will be removed.
>>>
>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>> ---
>>>  drivers/gpu/drm/drm_bridge.c             | 3 ++-
>>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
>>
>> What is the reason to put rockchip here? Shouldn't be in separate patch?
> 
> Because the rockchip driver peeks into the bridge struct and all the
> changes in this patch relate to making the new .odev member optional in
> the transition phase, when the bridge can have either a new-style odev
> or an old style of_node.
> 
> I guess this rockchip change could be patch 2, but it has to come first
> after this patch and it makes no sense on its own. Hence, one patch.
> 
> This rockchip_lvds interaction is also present in patch 24/26
> drm/bridge: remove the .of_node member
> 
> I can split them if you want, but I personally don't see the point.

I had a second look, and maybe the series should start with a patch like
this instead, so that the rockchip_lvds.c hunks can be removed from
patch 1/26 and 24/26. That would perhaps be slightly cleaner?

On the other hand, it's orthogonal and this series can be left as is
(with the benefit of me not having to do another iteration and you all
not having another bunch of messages to sift through). The below
patch could easily be (adjusted and) applied later instead. Or not,
since the right fix is to do this with the newfangled static image
format mechanism from Jacopo Mondi, and it might be easier to just do
it right.

State your preference.

Cheers,
Peter

From dee27b36a36acd271459d1489336b56132097425 Mon Sep 17 00:00:00 2001
From: Peter Rosin <peda@axentia.se>
Date: Wed, 9 May 2018 23:58:24 +0200
Subject: [PATCH] drm/rockchip: lvds: do not dig into the DT of remote bridges

The driver is trying to find the needed "data-mapping" for
interacting with the following bridge in the graphics chain.
But, doing so is bad since it is done w/o regard of the
compatible of the remote bridge, so the value of "data-mapping"
might not mean what this driver assumes. It is also pointless
since no bridge has any documented "data-mapping" DT property
and no dts file show any undocumented use.

Just remove the inappropriate code.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 4bd94b167d2c..fa3f4bf9712f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,6 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 	}
 	if (lvds->panel)
 		remote = lvds->panel->dev->of_node;
-	else
-		remote = lvds->bridge->of_node;
 	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
 		/* default set it as output rgb */
 		lvds->output = DISPLAY_OUTPUT_RGB;
-- 
2.11.0


_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 01/26] drm/bridge: allow optionally specifying an owner .odev device

[Andrzej Hajda]

On 10.05.2018 00:21, Peter Rosin wrote:
> On 2018-05-09 17:53, Peter Rosin wrote:
>> On 2018-05-09 17:08, Andrzej Hajda wrote:
>>> On 04.05.2018 15:51, Peter Rosin wrote:
>>>> Bridge drivers can now (temporarily, in a transition phase) select if
>>>> they want to provide a full owner device or keep just providing an
>>>> of_node.
>>>>
>>>> By providing a full owner device, the bridge drivers no longer need
>>>> to provide an of_node since that node is available via the owner
>>>> device.
>>>>
>>>> When all bridge drivers provide an owner device, that will become
>>>> mandatory and the .of_node member will be removed.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>> ---
>>>>  drivers/gpu/drm/drm_bridge.c             | 3 ++-
>>>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 4 +++-
>>> What is the reason to put rockchip here? Shouldn't be in separate patch?
>> Because the rockchip driver peeks into the bridge struct and all the
>> changes in this patch relate to making the new .odev member optional in
>> the transition phase, when the bridge can have either a new-style odev
>> or an old style of_node.
>>
>> I guess this rockchip change could be patch 2, but it has to come first
>> after this patch and it makes no sense on its own. Hence, one patch.
>>
>> This rockchip_lvds interaction is also present in patch 24/26
>> drm/bridge: remove the .of_node member
>>
>> I can split them if you want, but I personally don't see the point.
> I had a second look, and maybe the series should start with a patch like
> this instead, so that the rockchip_lvds.c hunks can be removed from
> patch 1/26 and 24/26. That would perhaps be slightly cleaner?
>
> On the other hand, it's orthogonal and this series can be left as is
> (with the benefit of me not having to do another iteration and you all
> not having another bunch of messages to sift through). The below
> patch could easily be (adjusted and) applied later instead. Or not,
> since the right fix is to do this with the newfangled static image
> format mechanism from Jacopo Mondi, and it might be easier to just do
> it right.
>
> State your preference.

For me the current version is OK, it maybe lacks explanation why do you
need to touch rockchip, from my PoV it did not seem so obvious.
Somebody should fix rockchip to use Jacopo's approach instead of
violating abstractions, but this is another story.

With or without added missing explanation:

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


>
> Cheers,
> Peter
>
> >From dee27b36a36acd271459d1489336b56132097425 Mon Sep 17 00:00:00 2001
> From: Peter Rosin <peda@axentia.se>
> Date: Wed, 9 May 2018 23:58:24 +0200
> Subject: [PATCH] drm/rockchip: lvds: do not dig into the DT of remote bridges
>
> The driver is trying to find the needed "data-mapping" for
> interacting with the following bridge in the graphics chain.
> But, doing so is bad since it is done w/o regard of the
> compatible of the remote bridge, so the value of "data-mapping"
> might not mean what this driver assumes. It is also pointless
> since no bridge has any documented "data-mapping" DT property
> and no dts file show any undocumented use.
>
> Just remove the inappropriate code.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 4bd94b167d2c..fa3f4bf9712f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -377,8 +377,6 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  	}
>  	if (lvds->panel)
>  		remote = lvds->panel->dev->of_node;
> -	else
> -		remote = lvds->bridge->of_node;
>  	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
>  		/* default set it as output rgb */
>  		lvds->output = DISPLAY_OUTPUT_RGB;


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

[PATCH v2 21/26] drm/msm: specify the owner .odev of the bridges

[Peter Rosin]

This will become mandatory.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c  | 1 +
 drivers/gpu/drm/msm/edp/edp_bridge.c   | 1 +
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 4cb1cb68878b..1668e8abe5c1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -710,6 +710,7 @@ struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
 	encoder = msm_dsi->encoder;
 
 	bridge = &dsi_bridge->base;
+	bridge->odev = msm_dsi->dev->dev;
 	bridge->funcs = &dsi_mgr_bridge_funcs;
 
 	ret = drm_bridge_attach(encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/msm/edp/edp_bridge.c b/drivers/gpu/drm/msm/edp/edp_bridge.c
index 931a5c97cccf..4c56e29c57b7 100644
--- a/drivers/gpu/drm/msm/edp/edp_bridge.c
+++ b/drivers/gpu/drm/msm/edp/edp_bridge.c
@@ -104,6 +104,7 @@ struct drm_bridge *msm_edp_bridge_init(struct msm_edp *edp)
 	edp_bridge->edp = edp;
 
 	bridge = &edp_bridge->base;
+	bridge->odev = edp->dev->dev;
 	bridge->funcs = &edp_bridge_funcs;
 
 	ret = drm_bridge_attach(edp->encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 7e357077ed26..aa6dd1bc5dc0 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -293,6 +293,7 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
 	hdmi_bridge->hdmi = hdmi;
 
 	bridge = &hdmi_bridge->base;
+	bridge->odev = hdmi->dev->dev;
 	bridge->funcs = &msm_hdmi_bridge_funcs;
 
 	ret = drm_bridge_attach(hdmi->encoder, bridge, NULL);
-- 
2.11.0

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH v2 24/26] drm/bridge: remove the .of_node member

[Peter Rosin]

It is unused.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c             | 3 +--
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 include/drm/drm_bridge.h                 | 4 ----
 3 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 3872f5379998..df084db33494 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -365,8 +365,7 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 	mutex_lock(&bridge_lock);
 
 	list_for_each_entry(bridge, &bridge_list, list) {
-		if ((bridge->odev && bridge->odev->of_node == np) ||
-		    bridge->of_node == np) {
+		if (bridge->odev->of_node == np) {
 			mutex_unlock(&bridge_lock);
 			return bridge;
 		}
diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 557e0079c98d..e77d4c909582 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -377,8 +377,6 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 	}
 	if (lvds->panel)
 		remote = lvds->panel->dev->of_node;
-	else if (lvds->bridge->of_node)
-		remote = lvds->bridge->of_node;
 	else
 		remote = lvds->bridge->odev->of_node;
 	if (of_property_read_string(dev->of_node, "rockchip,output", &name))
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 7c17977c3537..b656e505d11e 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -258,7 +258,6 @@ struct drm_bridge_timings {
  * @dev: DRM device this bridge belongs to
  * @encoder: encoder to which this bridge is connected
  * @next: the next bridge in the encoder chain
- * @of_node: device node pointer to the bridge
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
@@ -270,9 +269,6 @@ struct drm_bridge {
 	struct drm_device *dev;
 	struct drm_encoder *encoder;
 	struct drm_bridge *next;
-#ifdef CONFIG_OF
-	struct device_node *of_node;
-#endif
 	struct list_head list;
 	const struct drm_bridge_timings *timings;
 
-- 
2.11.0

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach

[Peter Rosin]

The .odev owner device will be handy to have around.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index df084db33494..78d186b6831b 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -70,6 +70,9 @@ static LIST_HEAD(bridge_list);
  */
 void drm_bridge_add(struct drm_bridge *bridge)
 {
+	if (WARN_ON(!bridge->odev))
+		return;
+
 	mutex_lock(&bridge_lock);
 	list_add_tail(&bridge->list, &bridge_list);
 	mutex_unlock(&bridge_lock);
@@ -115,6 +118,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	if (!encoder || !bridge)
 		return -EINVAL;
 
+	if (WARN_ON(!bridge->odev))
+		return -EINVAL;
+
 	if (previous && (!previous->dev || previous->encoder != encoder))
 		return -EINVAL;
 
-- 
2.11.0

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 25/26] drm/bridge: require the owner .odev to be filled in on drm_bridge_add/attach

[Andrzej Hajda]

On 04.05.2018 15:52, Peter Rosin wrote:
> The .odev owner device will be handy to have around.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
> ---
>  drivers/gpu/drm/drm_bridge.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index df084db33494..78d186b6831b 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -70,6 +70,9 @@ static LIST_HEAD(bridge_list);
>   */
>  void drm_bridge_add(struct drm_bridge *bridge)
>  {
> +	if (WARN_ON(!bridge->odev))
> +		return;
> +
>  	mutex_lock(&bridge_lock);
>  	list_add_tail(&bridge->list, &bridge_list);
>  	mutex_unlock(&bridge_lock);
> @@ -115,6 +118,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (!encoder || !bridge)
>  		return -EINVAL;
>  
> +	if (WARN_ON(!bridge->odev))
> +		return -EINVAL;
> +
>  	if (previous && (!previous->dev || previous->encoder != encoder))
>  		return -EINVAL;
>  


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

[PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

If the bridge supplier is unbound, this will bring the bridge consumer
down along with the bridge. Thus, there will no longer linger any
dangling pointers from the bridge consumer (the drm_device) to some
non-existent bridge supplier.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
 include/drm/drm_bridge.h     |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 78d186b6831b..0259f0a3ff27 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -26,6 +26,7 @@
 #include <linux/mutex.h>
 
 #include <drm/drm_bridge.h>
+#include <drm/drm_device.h>
 #include <drm/drm_encoder.h>
 
 #include "drm_crtc_internal.h"
@@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	if (bridge->dev)
 		return -EBUSY;
 
+	if (encoder->dev->dev != bridge->odev) {
+		bridge->link = device_link_add(encoder->dev->dev,
+					       bridge->odev, 0);
+		if (!bridge->link) {
+			dev_err(bridge->odev, "failed to link bridge to %s\n",
+				dev_name(encoder->dev->dev));
+			return -EINVAL;
+		}
+	}
+
 	bridge->dev = encoder->dev;
 	bridge->encoder = encoder;
 
 	if (bridge->funcs->attach) {
 		ret = bridge->funcs->attach(bridge);
 		if (ret < 0) {
+			if (bridge->link)
+				device_link_del(bridge->link);
+			bridge->link = NULL;
 			bridge->dev = NULL;
 			bridge->encoder = NULL;
 			return ret;
@@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
 	if (bridge->funcs->detach)
 		bridge->funcs->detach(bridge);
 
+	if (bridge->link)
+		device_link_del(bridge->link);
+	bridge->link = NULL;
+
 	bridge->dev = NULL;
 }
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index b656e505d11e..804189c63a4c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -261,6 +261,7 @@ struct drm_bridge_timings {
  * @list: to keep track of all added bridges
  * @timings: the timing specification for the bridge, if any (may
  * be NULL)
+ * @link: drm consumer <-> bridge supplier
  * @funcs: control functions
  * @driver_private: pointer to the bridge driver's internal context
  */
@@ -271,6 +272,7 @@ struct drm_bridge {
 	struct drm_bridge *next;
 	struct list_head list;
 	const struct drm_bridge_timings *timings;
+	struct device_link *link;
 
 	const struct drm_bridge_funcs *funcs;
 	void *driver_private;
-- 
2.11.0

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Andrzej Hajda]

On 04.05.2018 15:52, Peter Rosin wrote:
> If the bridge supplier is unbound, this will bring the bridge consumer
> down along with the bridge. Thus, there will no longer linger any
> dangling pointers from the bridge consumer (the drm_device) to some
> non-existent bridge supplier.

I understand rationales behind this patch, but it is another step into
making drm_dev one big driver with subcomponents, where drm will work
only if every subcomponent is working/loaded. Do we need to go this way?
In case of many platforms such approach results in display turned on
very late on boot for example due to late initialization of some
regulator exposed by some i2c device, which is used by hdmi bridge. And
this hdmi bridge is just to provide alternative(rarely used) display
path, the main display path would work anyway.

So the main question to drm maintainers is about evolution of bridges,
if drm_bridges should become mandatory components of drm device or they
could be added/removed dynamically?

Regards
Andrzej


>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>  include/drm/drm_bridge.h     |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 78d186b6831b..0259f0a3ff27 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -26,6 +26,7 @@
>  #include <linux/mutex.h>
>  
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_device.h>
>  #include <drm/drm_encoder.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (bridge->dev)
>  		return -EBUSY;
>  
> +	if (encoder->dev->dev != bridge->odev) {
> +		bridge->link = device_link_add(encoder->dev->dev,
> +					       bridge->odev, 0);
> +		if (!bridge->link) {
> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
> +				dev_name(encoder->dev->dev));
> +			return -EINVAL;
> +		}
> +	}
> +
>  	bridge->dev = encoder->dev;
>  	bridge->encoder = encoder;
>  
>  	if (bridge->funcs->attach) {
>  		ret = bridge->funcs->attach(bridge);
>  		if (ret < 0) {
> +			if (bridge->link)
> +				device_link_del(bridge->link);
> +			bridge->link = NULL;
>  			bridge->dev = NULL;
>  			bridge->encoder = NULL;
>  			return ret;
> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  	if (bridge->funcs->detach)
>  		bridge->funcs->detach(bridge);
>  
> +	if (bridge->link)
> +		device_link_del(bridge->link);
> +	bridge->link = NULL;
> +
>  	bridge->dev = NULL;
>  }
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index b656e505d11e..804189c63a4c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>   * @list: to keep track of all added bridges
>   * @timings: the timing specification for the bridge, if any (may
>   * be NULL)
> + * @link: drm consumer <-> bridge supplier
>   * @funcs: control functions
>   * @driver_private: pointer to the bridge driver's internal context
>   */
> @@ -271,6 +272,7 @@ struct drm_bridge {
>  	struct drm_bridge *next;
>  	struct list_head list;
>  	const struct drm_bridge_timings *timings;
> +	struct device_link *link;
>  
>  	const struct drm_bridge_funcs *funcs;
>  	void *driver_private;

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

On 2018-05-07 14:59, Andrzej Hajda wrote:
> On 04.05.2018 15:52, Peter Rosin wrote:
>> If the bridge supplier is unbound, this will bring the bridge consumer
>> down along with the bridge. Thus, there will no longer linger any
>> dangling pointers from the bridge consumer (the drm_device) to some
>> non-existent bridge supplier.
> 
> I understand rationales behind this patch, but it is another step into
> making drm_dev one big driver with subcomponents, where drm will work
> only if every subcomponent is working/loaded.

The step is very small IMHO. Just a device-link, which is very easy to
remove once whatever other solution is ready.

>                                               Do we need to go this way?

If the drivers expect the parts to be there, and there is no other safety
net in place if they are not, what is the (short-term) alternative?

> In case of many platforms such approach results in display turned on
> very late on boot for example due to late initialization of some
> regulator exposed by some i2c device, which is used by hdmi bridge. And
> this hdmi bridge is just to provide alternative(rarely used) display
> path, the main display path would work anyway.

This patch does not contribute to any late init and any such delay is not
affected by this. At all.

> So the main question to drm maintainers is about evolution of bridges,
> if drm_bridges should become mandatory components of drm device or they
> could be added/removed dynamically?

That is a much bigger question than this patch/series. Conflating the
two is not fair IMHO. You could run this very same argument for every
driver that gets added, since any additional driver will just make it
harder to make everything dynamic. Should we stop development right
away?

Besides, as long as the drm devices are in fact acting as big static
drivers (built from smaller parts), this should be considered a bug-fix
that will prevent dereference of stale pointers.

Or will some other solution appear and magically make all bridges and
drm drivers capable of dynamic reconfiguration in the next few weeks?
Yeah, right...

Cheers,
Peter

> Regards
> Andrzej
> 
> 
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>  include/drm/drm_bridge.h     |  2 ++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 78d186b6831b..0259f0a3ff27 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -26,6 +26,7 @@
>>  #include <linux/mutex.h>
>>  
>>  #include <drm/drm_bridge.h>
>> +#include <drm/drm_device.h>
>>  #include <drm/drm_encoder.h>
>>  
>>  #include "drm_crtc_internal.h"
>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>  	if (bridge->dev)
>>  		return -EBUSY;
>>  
>> +	if (encoder->dev->dev != bridge->odev) {
>> +		bridge->link = device_link_add(encoder->dev->dev,
>> +					       bridge->odev, 0);
>> +		if (!bridge->link) {
>> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
>> +				dev_name(encoder->dev->dev));
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>>  	bridge->dev = encoder->dev;
>>  	bridge->encoder = encoder;
>>  
>>  	if (bridge->funcs->attach) {
>>  		ret = bridge->funcs->attach(bridge);
>>  		if (ret < 0) {
>> +			if (bridge->link)
>> +				device_link_del(bridge->link);
>> +			bridge->link = NULL;
>>  			bridge->dev = NULL;
>>  			bridge->encoder = NULL;
>>  			return ret;
>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>  	if (bridge->funcs->detach)
>>  		bridge->funcs->detach(bridge);
>>  
>> +	if (bridge->link)
>> +		device_link_del(bridge->link);
>> +	bridge->link = NULL;
>> +
>>  	bridge->dev = NULL;
>>  }
>>  
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index b656e505d11e..804189c63a4c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>   * @list: to keep track of all added bridges
>>   * @timings: the timing specification for the bridge, if any (may
>>   * be NULL)
>> + * @link: drm consumer <-> bridge supplier
>>   * @funcs: control functions
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>  	struct drm_bridge *next;
>>  	struct list_head list;
>>  	const struct drm_bridge_timings *timings;
>> +	struct device_link *link;
>>  
>>  	const struct drm_bridge_funcs *funcs;
>>  	void *driver_private;
> 
> 

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Andrzej Hajda]

On 07.05.2018 15:43, Peter Rosin wrote:
> On 2018-05-07 14:59, Andrzej Hajda wrote:
>> On 04.05.2018 15:52, Peter Rosin wrote:
>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>> down along with the bridge. Thus, there will no longer linger any
>>> dangling pointers from the bridge consumer (the drm_device) to some
>>> non-existent bridge supplier.
>> I understand rationales behind this patch, but it is another step into
>> making drm_dev one big driver with subcomponents, where drm will work
>> only if every subcomponent is working/loaded.
> The step is very small IMHO. Just a device-link, which is very easy to
> remove once whatever other solution is ready.
>
>>                                               Do we need to go this way?
> If the drivers expect the parts to be there, and there is no other safety
> net in place if they are not, what is the (short-term) alternative?
>
>> In case of many platforms such approach results in display turned on
>> very late on boot for example due to late initialization of some
>> regulator exposed by some i2c device, which is used by hdmi bridge. And
>> this hdmi bridge is just to provide alternative(rarely used) display
>> path, the main display path would work anyway.
> This patch does not contribute to any late init and any such delay is not
> affected by this. At all.
>
>> So the main question to drm maintainers is about evolution of bridges,
>> if drm_bridges should become mandatory components of drm device or they
>> could be added/removed dynamically?
> That is a much bigger question than this patch/series. Conflating the
> two is not fair IMHO. You could run this very same argument for every
> driver that gets added, since any additional driver will just make it
> harder to make everything dynamic. Should we stop development right
> away?
>
> Besides, as long as the drm devices are in fact acting as big static
> drivers (built from smaller parts), 

not true

> this should be considered a bug-fix
> that will prevent dereference of stale pointers.
>
> Or will some other solution appear and magically make all bridges and
> drm drivers capable of dynamic reconfiguration in the next few weeks?
> Yeah, right...

You are not changing single driver, you are changing framework and it
affects all the drivers using it, being more cautious about such patches
seems quite natural.

Anyway, I have realized that since drm_bridge_detach will remove the
link, so with properly written dynamic bridge removal, your patch should
not be a blocker.

Regards
Andrzej

>
> Cheers,
> Peter
>
>> Regards
>> Andrzej
>>
>>
>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>> ---
>>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>>  include/drm/drm_bridge.h     |  2 ++
>>>  2 files changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>> index 78d186b6831b..0259f0a3ff27 100644
>>> --- a/drivers/gpu/drm/drm_bridge.c
>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>> @@ -26,6 +26,7 @@
>>>  #include <linux/mutex.h>
>>>  
>>>  #include <drm/drm_bridge.h>
>>> +#include <drm/drm_device.h>
>>>  #include <drm/drm_encoder.h>
>>>  
>>>  #include "drm_crtc_internal.h"
>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>  	if (bridge->dev)
>>>  		return -EBUSY;
>>>  
>>> +	if (encoder->dev->dev != bridge->odev) {
>>> +		bridge->link = device_link_add(encoder->dev->dev,
>>> +					       bridge->odev, 0);
>>> +		if (!bridge->link) {
>>> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
>>> +				dev_name(encoder->dev->dev));
>>> +			return -EINVAL;
>>> +		}
>>> +	}
>>> +
>>>  	bridge->dev = encoder->dev;
>>>  	bridge->encoder = encoder;
>>>  
>>>  	if (bridge->funcs->attach) {
>>>  		ret = bridge->funcs->attach(bridge);
>>>  		if (ret < 0) {
>>> +			if (bridge->link)
>>> +				device_link_del(bridge->link);
>>> +			bridge->link = NULL;
>>>  			bridge->dev = NULL;
>>>  			bridge->encoder = NULL;
>>>  			return ret;
>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>>  	if (bridge->funcs->detach)
>>>  		bridge->funcs->detach(bridge);
>>>  
>>> +	if (bridge->link)
>>> +		device_link_del(bridge->link);
>>> +	bridge->link = NULL;
>>> +
>>>  	bridge->dev = NULL;
>>>  }
>>>  
>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>> index b656e505d11e..804189c63a4c 100644
>>> --- a/include/drm/drm_bridge.h
>>> +++ b/include/drm/drm_bridge.h
>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>>   * @list: to keep track of all added bridges
>>>   * @timings: the timing specification for the bridge, if any (may
>>>   * be NULL)
>>> + * @link: drm consumer <-> bridge supplier
>>>   * @funcs: control functions
>>>   * @driver_private: pointer to the bridge driver's internal context
>>>   */
>>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>>  	struct drm_bridge *next;
>>>  	struct list_head list;
>>>  	const struct drm_bridge_timings *timings;
>>> +	struct device_link *link;
>>>  
>>>  	const struct drm_bridge_funcs *funcs;
>>>  	void *driver_private;
>>
>
>
>

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

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Daniel Vetter]

On Mon, May 07, 2018 at 02:59:45PM +0200, Andrzej Hajda wrote:
> On 04.05.2018 15:52, Peter Rosin wrote:
> > If the bridge supplier is unbound, this will bring the bridge consumer
> > down along with the bridge. Thus, there will no longer linger any
> > dangling pointers from the bridge consumer (the drm_device) to some
> > non-existent bridge supplier.
> 
> I understand rationales behind this patch, but it is another step into
> making drm_dev one big driver with subcomponents, where drm will work
> only if every subcomponent is working/loaded. Do we need to go this way?
> In case of many platforms such approach results in display turned on
> very late on boot for example due to late initialization of some
> regulator exposed by some i2c device, which is used by hdmi bridge. And
> this hdmi bridge is just to provide alternative(rarely used) display
> path, the main display path would work anyway.
> 
> So the main question to drm maintainers is about evolution of bridges,
> if drm_bridges should become mandatory components of drm device or they
> could be added/removed dynamically?

This is already the case. You currently cannot hotplug a drm_bridge,
everything must be present. I don't think it makes sense to change that
until we have physically hotpluggable drm_bridges in real hardware. I
definitely don't want to refcount stuff to work around driver load
bonghits on DT platforms, because refcounting is way too hard to get right
:-)

Afaik there's out-of-tree patches to solve 99% of the driver load fun on
DT platforms, but because it's not a 100% solution it's blocked since
forever.
-Daniel

> 
> Regards
> Andrzej
> 
> 
> >
> > Signed-off-by: Peter Rosin <peda@axentia.se>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
> >  include/drm/drm_bridge.h     |  2 ++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 78d186b6831b..0259f0a3ff27 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/mutex.h>
> >  
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_device.h>
> >  #include <drm/drm_encoder.h>
> >  
> >  #include "drm_crtc_internal.h"
> > @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >  	if (bridge->dev)
> >  		return -EBUSY;
> >  
> > +	if (encoder->dev->dev != bridge->odev) {
> > +		bridge->link = device_link_add(encoder->dev->dev,
> > +					       bridge->odev, 0);
> > +		if (!bridge->link) {
> > +			dev_err(bridge->odev, "failed to link bridge to %s\n",
> > +				dev_name(encoder->dev->dev));
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> >  	bridge->dev = encoder->dev;
> >  	bridge->encoder = encoder;
> >  
> >  	if (bridge->funcs->attach) {
> >  		ret = bridge->funcs->attach(bridge);
> >  		if (ret < 0) {
> > +			if (bridge->link)
> > +				device_link_del(bridge->link);
> > +			bridge->link = NULL;
> >  			bridge->dev = NULL;
> >  			bridge->encoder = NULL;
> >  			return ret;
> > @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >  	if (bridge->funcs->detach)
> >  		bridge->funcs->detach(bridge);
> >  
> > +	if (bridge->link)
> > +		device_link_del(bridge->link);
> > +	bridge->link = NULL;
> > +
> >  	bridge->dev = NULL;
> >  }
> >  
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index b656e505d11e..804189c63a4c 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -261,6 +261,7 @@ struct drm_bridge_timings {
> >   * @list: to keep track of all added bridges
> >   * @timings: the timing specification for the bridge, if any (may
> >   * be NULL)
> > + * @link: drm consumer <-> bridge supplier
> >   * @funcs: control functions
> >   * @driver_private: pointer to the bridge driver's internal context
> >   */
> > @@ -271,6 +272,7 @@ struct drm_bridge {
> >  	struct drm_bridge *next;
> >  	struct list_head list;
> >  	const struct drm_bridge_timings *timings;
> > +	struct device_link *link;
> >  
> >  	const struct drm_bridge_funcs *funcs;
> >  	void *driver_private;
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Andrzej Hajda]

On 07.05.2018 15:53, Daniel Vetter wrote:
> On Mon, May 07, 2018 at 02:59:45PM +0200, Andrzej Hajda wrote:
>> On 04.05.2018 15:52, Peter Rosin wrote:
>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>> down along with the bridge. Thus, there will no longer linger any
>>> dangling pointers from the bridge consumer (the drm_device) to some
>>> non-existent bridge supplier.
>> I understand rationales behind this patch, but it is another step into
>> making drm_dev one big driver with subcomponents, where drm will work
>> only if every subcomponent is working/loaded. Do we need to go this way?
>> In case of many platforms such approach results in display turned on
>> very late on boot for example due to late initialization of some
>> regulator exposed by some i2c device, which is used by hdmi bridge. And
>> this hdmi bridge is just to provide alternative(rarely used) display
>> path, the main display path would work anyway.
>>
>> So the main question to drm maintainers is about evolution of bridges,
>> if drm_bridges should become mandatory components of drm device or they
>> could be added/removed dynamically?
> This is already the case. You currently cannot hotplug a drm_bridge,
> everything must be present.

Are you sure? DRM core is changing quite fast, so maybe I have missed
something, but AFAIK core calls bridge code only if full display
pipeline is created and connector is in connected state.
So adding and removing bridges from inactive pipelines should work if
coded properly.

>  I don't think it makes sense to change that
> until we have physically hotpluggable drm_bridges in real hardware.

But kernel core already assumes that device drivers are hot-pluggable
:), even this patch is created to solve issues regarding driver hot
unplugging.

>  I
> definitely don't want to refcount stuff to work around driver load
> bonghits on DT platforms, because refcounting is way too hard to get right
> :-)

I am not sure about bridges, but I have successfully (IMO) experimented
with hot (un)plugging panel driver, see panel-samsung-s6e8aa0.c driver
and exynos_drm_dsi.c, panel driver can be safely
plugged/unplugged/replugged without any refcounting (but with help of
mipi_dsi attach/detach callbacks, which are not available for
non-mipi-dsi drivers).

Regards
Andrzej

>
> Afaik there's out-of-tree patches to solve 99% of the driver load fun on
> DT platforms, but because it's not a 100% solution it's blocked since
> forever.
> -Daniel
>
>> Regards
>> Andrzej
>>
>>
>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>> ---
>>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>>  include/drm/drm_bridge.h     |  2 ++
>>>  2 files changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>> index 78d186b6831b..0259f0a3ff27 100644
>>> --- a/drivers/gpu/drm/drm_bridge.c
>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>> @@ -26,6 +26,7 @@
>>>  #include <linux/mutex.h>
>>>  
>>>  #include <drm/drm_bridge.h>
>>> +#include <drm/drm_device.h>
>>>  #include <drm/drm_encoder.h>
>>>  
>>>  #include "drm_crtc_internal.h"
>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>  	if (bridge->dev)
>>>  		return -EBUSY;
>>>  
>>> +	if (encoder->dev->dev != bridge->odev) {
>>> +		bridge->link = device_link_add(encoder->dev->dev,
>>> +					       bridge->odev, 0);
>>> +		if (!bridge->link) {
>>> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
>>> +				dev_name(encoder->dev->dev));
>>> +			return -EINVAL;
>>> +		}
>>> +	}
>>> +
>>>  	bridge->dev = encoder->dev;
>>>  	bridge->encoder = encoder;
>>>  
>>>  	if (bridge->funcs->attach) {
>>>  		ret = bridge->funcs->attach(bridge);
>>>  		if (ret < 0) {
>>> +			if (bridge->link)
>>> +				device_link_del(bridge->link);
>>> +			bridge->link = NULL;
>>>  			bridge->dev = NULL;
>>>  			bridge->encoder = NULL;
>>>  			return ret;
>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>>  	if (bridge->funcs->detach)
>>>  		bridge->funcs->detach(bridge);
>>>  
>>> +	if (bridge->link)
>>> +		device_link_del(bridge->link);
>>> +	bridge->link = NULL;
>>> +
>>>  	bridge->dev = NULL;
>>>  }
>>>  
>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>> index b656e505d11e..804189c63a4c 100644
>>> --- a/include/drm/drm_bridge.h
>>> +++ b/include/drm/drm_bridge.h
>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>>   * @list: to keep track of all added bridges
>>>   * @timings: the timing specification for the bridge, if any (may
>>>   * be NULL)
>>> + * @link: drm consumer <-> bridge supplier
>>>   * @funcs: control functions
>>>   * @driver_private: pointer to the bridge driver's internal context
>>>   */
>>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>>  	struct drm_bridge *next;
>>>  	struct list_head list;
>>>  	const struct drm_bridge_timings *timings;
>>> +	struct device_link *link;
>>>  
>>>  	const struct drm_bridge_funcs *funcs;
>>>  	void *driver_private;
>>

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

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Andrzej Hajda]

On 04.05.2018 15:52, Peter Rosin wrote:
> If the bridge supplier is unbound, this will bring the bridge consumer
> down along with the bridge. Thus, there will no longer linger any
> dangling pointers from the bridge consumer (the drm_device) to some
> non-existent bridge supplier.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>  include/drm/drm_bridge.h     |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 78d186b6831b..0259f0a3ff27 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -26,6 +26,7 @@
>  #include <linux/mutex.h>
>  
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_device.h>
>  #include <drm/drm_encoder.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  	if (bridge->dev)
>  		return -EBUSY;
>  
> +	if (encoder->dev->dev != bridge->odev) {

I wonder why device_link_add does not handle this case (self dependency)
silently as noop, as it seems to be a correct behavior.

> +		bridge->link = device_link_add(encoder->dev->dev,
> +					       bridge->odev, 0);
> +		if (!bridge->link) {
> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
> +				dev_name(encoder->dev->dev));
> +			return -EINVAL;
> +		}
> +	}
> +
>  	bridge->dev = encoder->dev;
>  	bridge->encoder = encoder;
>  
>  	if (bridge->funcs->attach) {
>  		ret = bridge->funcs->attach(bridge);
>  		if (ret < 0) {
> +			if (bridge->link)
> +				device_link_del(bridge->link);
> +			bridge->link = NULL;
>  			bridge->dev = NULL;
>  			bridge->encoder = NULL;
>  			return ret;
> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  	if (bridge->funcs->detach)
>  		bridge->funcs->detach(bridge);
>  
> +	if (bridge->link)
> +		device_link_del(bridge->link);
> +	bridge->link = NULL;
> +
>  	bridge->dev = NULL;
>  }
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index b656e505d11e..804189c63a4c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>   * @list: to keep track of all added bridges
>   * @timings: the timing specification for the bridge, if any (may
>   * be NULL)
> + * @link: drm consumer <-> bridge supplier

Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
to the bridge" would be better.

Anyway:
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

>   * @funcs: control functions
>   * @driver_private: pointer to the bridge driver's internal context
>   */
> @@ -271,6 +272,7 @@ struct drm_bridge {
>  	struct drm_bridge *next;
>  	struct list_head list;
>  	const struct drm_bridge_timings *timings;
> +	struct device_link *link;
>  
>  	const struct drm_bridge_funcs *funcs;
>  	void *driver_private;


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

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

On 2018-05-10 10:10, Andrzej Hajda wrote:
> On 04.05.2018 15:52, Peter Rosin wrote:
>> If the bridge supplier is unbound, this will bring the bridge consumer
>> down along with the bridge. Thus, there will no longer linger any
>> dangling pointers from the bridge consumer (the drm_device) to some
>> non-existent bridge supplier.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>  include/drm/drm_bridge.h     |  2 ++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index 78d186b6831b..0259f0a3ff27 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -26,6 +26,7 @@
>>  #include <linux/mutex.h>
>>  
>>  #include <drm/drm_bridge.h>
>> +#include <drm/drm_device.h>
>>  #include <drm/drm_encoder.h>
>>  
>>  #include "drm_crtc_internal.h"
>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>  	if (bridge->dev)
>>  		return -EBUSY;
>>  
>> +	if (encoder->dev->dev != bridge->odev) {
> 
> I wonder why device_link_add does not handle this case (self dependency)
> silently as noop, as it seems to be a correct behavior.

It's kind-of a silly corner-case though, so perfectly understandable
that it isn't handled.

>> +		bridge->link = device_link_add(encoder->dev->dev,
>> +					       bridge->odev, 0);
>> +		if (!bridge->link) {
>> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
>> +				dev_name(encoder->dev->dev));
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>>  	bridge->dev = encoder->dev;
>>  	bridge->encoder = encoder;
>>  
>>  	if (bridge->funcs->attach) {
>>  		ret = bridge->funcs->attach(bridge);
>>  		if (ret < 0) {
>> +			if (bridge->link)
>> +				device_link_del(bridge->link);
>> +			bridge->link = NULL;
>>  			bridge->dev = NULL;
>>  			bridge->encoder = NULL;
>>  			return ret;
>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>  	if (bridge->funcs->detach)
>>  		bridge->funcs->detach(bridge);
>>  
>> +	if (bridge->link)
>> +		device_link_del(bridge->link);
>> +	bridge->link = NULL;
>> +
>>  	bridge->dev = NULL;
>>  }
>>  
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index b656e505d11e..804189c63a4c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>   * @list: to keep track of all added bridges
>>   * @timings: the timing specification for the bridge, if any (may
>>   * be NULL)
>> + * @link: drm consumer <-> bridge supplier
> 
> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
> to the bridge" would be better.

I meant "<->" to indicate that the link is bidirectional, not that the
relationship is in any way symmetric. I wasn't aware of any implication
of a symmetric relationship when using "<->", do you have a reference?
But I guess the different arrow notations in math are somewhat overloaded
and that someone at some point must have used "<->" to indicate a
symmetric relationship...

> Anyway:
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Thanks!

Cheers,
Peter

>  --
> Regards
> Andrzej
> 
>>   * @funcs: control functions
>>   * @driver_private: pointer to the bridge driver's internal context
>>   */
>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>  	struct drm_bridge *next;
>>  	struct list_head list;
>>  	const struct drm_bridge_timings *timings;
>> +	struct device_link *link;
>>  
>>  	const struct drm_bridge_funcs *funcs;
>>  	void *driver_private;
> 
> 

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Daniel Vetter]

On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
> On 2018-05-10 10:10, Andrzej Hajda wrote:
> > On 04.05.2018 15:52, Peter Rosin wrote:
> >> If the bridge supplier is unbound, this will bring the bridge consumer
> >> down along with the bridge. Thus, there will no longer linger any
> >> dangling pointers from the bridge consumer (the drm_device) to some
> >> non-existent bridge supplier.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
> >>  include/drm/drm_bridge.h     |  2 ++
> >>  2 files changed, 20 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >> index 78d186b6831b..0259f0a3ff27 100644
> >> --- a/drivers/gpu/drm/drm_bridge.c
> >> +++ b/drivers/gpu/drm/drm_bridge.c
> >> @@ -26,6 +26,7 @@
> >>  #include <linux/mutex.h>
> >>  
> >>  #include <drm/drm_bridge.h>
> >> +#include <drm/drm_device.h>
> >>  #include <drm/drm_encoder.h>
> >>  
> >>  #include "drm_crtc_internal.h"
> >> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >>  	if (bridge->dev)
> >>  		return -EBUSY;
> >>  
> >> +	if (encoder->dev->dev != bridge->odev) {
> > 
> > I wonder why device_link_add does not handle this case (self dependency)
> > silently as noop, as it seems to be a correct behavior.
> 
> It's kind-of a silly corner-case though, so perfectly understandable
> that it isn't handled.
> 
> >> +		bridge->link = device_link_add(encoder->dev->dev,
> >> +					       bridge->odev, 0);
> >> +		if (!bridge->link) {
> >> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
> >> +				dev_name(encoder->dev->dev));
> >> +			return -EINVAL;
> >> +		}
> >> +	}
> >> +
> >>  	bridge->dev = encoder->dev;
> >>  	bridge->encoder = encoder;
> >>  
> >>  	if (bridge->funcs->attach) {
> >>  		ret = bridge->funcs->attach(bridge);
> >>  		if (ret < 0) {
> >> +			if (bridge->link)
> >> +				device_link_del(bridge->link);
> >> +			bridge->link = NULL;
> >>  			bridge->dev = NULL;
> >>  			bridge->encoder = NULL;
> >>  			return ret;
> >> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >>  	if (bridge->funcs->detach)
> >>  		bridge->funcs->detach(bridge);
> >>  
> >> +	if (bridge->link)
> >> +		device_link_del(bridge->link);
> >> +	bridge->link = NULL;
> >> +
> >>  	bridge->dev = NULL;
> >>  }
> >>  
> >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> >> index b656e505d11e..804189c63a4c 100644
> >> --- a/include/drm/drm_bridge.h
> >> +++ b/include/drm/drm_bridge.h
> >> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
> >>   * @list: to keep track of all added bridges
> >>   * @timings: the timing specification for the bridge, if any (may
> >>   * be NULL)
> >> + * @link: drm consumer <-> bridge supplier
> > 
> > Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
> > to the bridge" would be better.
> 
> I meant "<->" to indicate that the link is bidirectional, not that the
> relationship is in any way symmetric. I wasn't aware of any implication
> of a symmetric relationship when using "<->", do you have a reference?
> But I guess the different arrow notations in math are somewhat overloaded
> and that someone at some point must have used "<->" to indicate a
> symmetric relationship...

Yeah I agree with Andrzej here, for me <-> implies a symmetric
relationship. Spelling it out like Andrzej suggested sounds like the
better idea.
-Daniel

> 
> > Anyway:
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> Thanks!
> 
> Cheers,
> Peter
> 
> >  --
> > Regards
> > Andrzej
> > 
> >>   * @funcs: control functions
> >>   * @driver_private: pointer to the bridge driver's internal context
> >>   */
> >> @@ -271,6 +272,7 @@ struct drm_bridge {
> >>  	struct drm_bridge *next;
> >>  	struct list_head list;
> >>  	const struct drm_bridge_timings *timings;
> >> +	struct device_link *link;
> >>  
> >>  	const struct drm_bridge_funcs *funcs;
> >>  	void *driver_private;
> > 
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

On 2018-05-14 18:28, Daniel Vetter wrote:
> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
>> On 2018-05-10 10:10, Andrzej Hajda wrote:
>>> On 04.05.2018 15:52, Peter Rosin wrote:
>>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>>> down along with the bridge. Thus, there will no longer linger any
>>>> dangling pointers from the bridge consumer (the drm_device) to some
>>>> non-existent bridge supplier.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>> ---
>>>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>>>  include/drm/drm_bridge.h     |  2 ++
>>>>  2 files changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>> index 78d186b6831b..0259f0a3ff27 100644
>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>> @@ -26,6 +26,7 @@
>>>>  #include <linux/mutex.h>
>>>>  
>>>>  #include <drm/drm_bridge.h>
>>>> +#include <drm/drm_device.h>
>>>>  #include <drm/drm_encoder.h>
>>>>  
>>>>  #include "drm_crtc_internal.h"
>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>>  	if (bridge->dev)
>>>>  		return -EBUSY;
>>>>  
>>>> +	if (encoder->dev->dev != bridge->odev) {
>>>
>>> I wonder why device_link_add does not handle this case (self dependency)
>>> silently as noop, as it seems to be a correct behavior.
>>
>> It's kind-of a silly corner-case though, so perfectly understandable
>> that it isn't handled.
>>
>>>> +		bridge->link = device_link_add(encoder->dev->dev,
>>>> +					       bridge->odev, 0);
>>>> +		if (!bridge->link) {
>>>> +			dev_err(bridge->odev, "failed to link bridge to %s\n",
>>>> +				dev_name(encoder->dev->dev));
>>>> +			return -EINVAL;
>>>> +		}
>>>> +	}
>>>> +
>>>>  	bridge->dev = encoder->dev;
>>>>  	bridge->encoder = encoder;
>>>>  
>>>>  	if (bridge->funcs->attach) {
>>>>  		ret = bridge->funcs->attach(bridge);
>>>>  		if (ret < 0) {
>>>> +			if (bridge->link)
>>>> +				device_link_del(bridge->link);
>>>> +			bridge->link = NULL;
>>>>  			bridge->dev = NULL;
>>>>  			bridge->encoder = NULL;
>>>>  			return ret;
>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>>>  	if (bridge->funcs->detach)
>>>>  		bridge->funcs->detach(bridge);
>>>>  
>>>> +	if (bridge->link)
>>>> +		device_link_del(bridge->link);
>>>> +	bridge->link = NULL;
>>>> +
>>>>  	bridge->dev = NULL;
>>>>  }
>>>>  
>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>> index b656e505d11e..804189c63a4c 100644
>>>> --- a/include/drm/drm_bridge.h
>>>> +++ b/include/drm/drm_bridge.h
>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>>>   * @list: to keep track of all added bridges
>>>>   * @timings: the timing specification for the bridge, if any (may
>>>>   * be NULL)
>>>> + * @link: drm consumer <-> bridge supplier
>>>
>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
>>> to the bridge" would be better.
>>
>> I meant "<->" to indicate that the link is bidirectional, not that the
>> relationship is in any way symmetric. I wasn't aware of any implication
>> of a symmetric relationship when using "<->", do you have a reference?
>> But I guess the different arrow notations in math are somewhat overloaded
>> and that someone at some point must have used "<->" to indicate a
>> symmetric relationship...
> 
> Yeah I agree with Andrzej here, for me <-> implies a symmetric
> relationship. Spelling it out like Andrzej suggested sounds like the
> better idea.
> -Daniel

Ok, I guess that means I have to do a v3 after all. Or can this
trivial documentation update be done by the committer? I hate to
spam everyone with another volley...

Or perhaps I should squash patches 2-23 that are all rather similar
and mechanic? I separated them to allow for easier review from
individual driver maintainers, but that didn't seem to happen
anyway...

Cheers,
Peter

> 
>>
>>> Anyway:
>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>
>> Thanks!
>>
>> Cheers,
>> Peter
>>
>>>  --
>>> Regards
>>> Andrzej
>>>
>>>>   * @funcs: control functions
>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>   */
>>>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>>>  	struct drm_bridge *next;
>>>>  	struct list_head list;
>>>>  	const struct drm_bridge_timings *timings;
>>>> +	struct device_link *link;
>>>>  
>>>>  	const struct drm_bridge_funcs *funcs;
>>>>  	void *driver_private;
>>>
>>>
>>
> 

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

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Daniel Vetter]

On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <peda@axentia.se> wrote:
> On 2018-05-14 18:28, Daniel Vetter wrote:
>> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
>>> On 2018-05-10 10:10, Andrzej Hajda wrote:
>>>> On 04.05.2018 15:52, Peter Rosin wrote:
>>>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>>>> down along with the bridge. Thus, there will no longer linger any
>>>>> dangling pointers from the bridge consumer (the drm_device) to some
>>>>> non-existent bridge supplier.
>>>>>
>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>>>>  include/drm/drm_bridge.h     |  2 ++
>>>>>  2 files changed, 20 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>>> index 78d186b6831b..0259f0a3ff27 100644
>>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>>> @@ -26,6 +26,7 @@
>>>>>  #include <linux/mutex.h>
>>>>>
>>>>>  #include <drm/drm_bridge.h>
>>>>> +#include <drm/drm_device.h>
>>>>>  #include <drm/drm_encoder.h>
>>>>>
>>>>>  #include "drm_crtc_internal.h"
>>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>>>    if (bridge->dev)
>>>>>            return -EBUSY;
>>>>>
>>>>> +  if (encoder->dev->dev != bridge->odev) {
>>>>
>>>> I wonder why device_link_add does not handle this case (self dependency)
>>>> silently as noop, as it seems to be a correct behavior.
>>>
>>> It's kind-of a silly corner-case though, so perfectly understandable
>>> that it isn't handled.
>>>
>>>>> +          bridge->link = device_link_add(encoder->dev->dev,
>>>>> +                                         bridge->odev, 0);
>>>>> +          if (!bridge->link) {
>>>>> +                  dev_err(bridge->odev, "failed to link bridge to %s\n",
>>>>> +                          dev_name(encoder->dev->dev));
>>>>> +                  return -EINVAL;
>>>>> +          }
>>>>> +  }
>>>>> +
>>>>>    bridge->dev = encoder->dev;
>>>>>    bridge->encoder = encoder;
>>>>>
>>>>>    if (bridge->funcs->attach) {
>>>>>            ret = bridge->funcs->attach(bridge);
>>>>>            if (ret < 0) {
>>>>> +                  if (bridge->link)
>>>>> +                          device_link_del(bridge->link);
>>>>> +                  bridge->link = NULL;
>>>>>                    bridge->dev = NULL;
>>>>>                    bridge->encoder = NULL;
>>>>>                    return ret;
>>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>>>>    if (bridge->funcs->detach)
>>>>>            bridge->funcs->detach(bridge);
>>>>>
>>>>> +  if (bridge->link)
>>>>> +          device_link_del(bridge->link);
>>>>> +  bridge->link = NULL;
>>>>> +
>>>>>    bridge->dev = NULL;
>>>>>  }
>>>>>
>>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>>> index b656e505d11e..804189c63a4c 100644
>>>>> --- a/include/drm/drm_bridge.h
>>>>> +++ b/include/drm/drm_bridge.h
>>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>>>>   * @list: to keep track of all added bridges
>>>>>   * @timings: the timing specification for the bridge, if any (may
>>>>>   * be NULL)
>>>>> + * @link: drm consumer <-> bridge supplier
>>>>
>>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
>>>> to the bridge" would be better.
>>>
>>> I meant "<->" to indicate that the link is bidirectional, not that the
>>> relationship is in any way symmetric. I wasn't aware of any implication
>>> of a symmetric relationship when using "<->", do you have a reference?
>>> But I guess the different arrow notations in math are somewhat overloaded
>>> and that someone at some point must have used "<->" to indicate a
>>> symmetric relationship...
>>
>> Yeah I agree with Andrzej here, for me <-> implies a symmetric
>> relationship. Spelling it out like Andrzej suggested sounds like the
>> better idea.
>> -Daniel
>
> Ok, I guess that means I have to do a v3 after all. Or can this
> trivial documentation update be done by the committer? I hate to
> spam everyone with another volley...
>
> Or perhaps I should squash patches 2-23 that are all rather similar
> and mechanic? I separated them to allow for easier review from
> individual driver maintainers, but that didn't seem to happen
> anyway...

Do another volley of the full set, or in-reply-to to just replace the
patch that needs to be respun (but some people don't like that).

When resending just make sure you're picking up all the acks/r-bs you
have already.
-Daniel
> Cheers,
> Peter
>
>>
>>>
>>>> Anyway:
>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>>
>>> Thanks!
>>>
>>> Cheers,
>>> Peter
>>>
>>>>  --
>>>> Regards
>>>> Andrzej
>>>>
>>>>>   * @funcs: control functions
>>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>>   */
>>>>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>>>>    struct drm_bridge *next;
>>>>>    struct list_head list;
>>>>>    const struct drm_bridge_timings *timings;
>>>>> +  struct device_link *link;
>>>>>
>>>>>    const struct drm_bridge_funcs *funcs;
>>>>>    void *driver_private;
>>>>
>>>>
>>>
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Peter Rosin]

On 2018-05-15 12:22, Daniel Vetter wrote:
> On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <peda@axentia.se> wrote:
>> On 2018-05-14 18:28, Daniel Vetter wrote:
>>> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
>>>> On 2018-05-10 10:10, Andrzej Hajda wrote:
>>>>> On 04.05.2018 15:52, Peter Rosin wrote:
>>>>>> If the bridge supplier is unbound, this will bring the bridge consumer
>>>>>> down along with the bridge. Thus, there will no longer linger any
>>>>>> dangling pointers from the bridge consumer (the drm_device) to some
>>>>>> non-existent bridge supplier.
>>>>>>
>>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>>> ---
>>>>>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
>>>>>>  include/drm/drm_bridge.h     |  2 ++
>>>>>>  2 files changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>>>> index 78d186b6831b..0259f0a3ff27 100644
>>>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>>>> @@ -26,6 +26,7 @@
>>>>>>  #include <linux/mutex.h>
>>>>>>
>>>>>>  #include <drm/drm_bridge.h>
>>>>>> +#include <drm/drm_device.h>
>>>>>>  #include <drm/drm_encoder.h>
>>>>>>
>>>>>>  #include "drm_crtc_internal.h"
>>>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>>>>>>    if (bridge->dev)
>>>>>>            return -EBUSY;
>>>>>>
>>>>>> +  if (encoder->dev->dev != bridge->odev) {
>>>>>
>>>>> I wonder why device_link_add does not handle this case (self dependency)
>>>>> silently as noop, as it seems to be a correct behavior.
>>>>
>>>> It's kind-of a silly corner-case though, so perfectly understandable
>>>> that it isn't handled.
>>>>
>>>>>> +          bridge->link = device_link_add(encoder->dev->dev,
>>>>>> +                                         bridge->odev, 0);
>>>>>> +          if (!bridge->link) {
>>>>>> +                  dev_err(bridge->odev, "failed to link bridge to %s\n",
>>>>>> +                          dev_name(encoder->dev->dev));
>>>>>> +                  return -EINVAL;
>>>>>> +          }
>>>>>> +  }
>>>>>> +
>>>>>>    bridge->dev = encoder->dev;
>>>>>>    bridge->encoder = encoder;
>>>>>>
>>>>>>    if (bridge->funcs->attach) {
>>>>>>            ret = bridge->funcs->attach(bridge);
>>>>>>            if (ret < 0) {
>>>>>> +                  if (bridge->link)
>>>>>> +                          device_link_del(bridge->link);
>>>>>> +                  bridge->link = NULL;
>>>>>>                    bridge->dev = NULL;
>>>>>>                    bridge->encoder = NULL;
>>>>>>                    return ret;
>>>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>>>>>>    if (bridge->funcs->detach)
>>>>>>            bridge->funcs->detach(bridge);
>>>>>>
>>>>>> +  if (bridge->link)
>>>>>> +          device_link_del(bridge->link);
>>>>>> +  bridge->link = NULL;
>>>>>> +
>>>>>>    bridge->dev = NULL;
>>>>>>  }
>>>>>>
>>>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>>>>>> index b656e505d11e..804189c63a4c 100644
>>>>>> --- a/include/drm/drm_bridge.h
>>>>>> +++ b/include/drm/drm_bridge.h
>>>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
>>>>>>   * @list: to keep track of all added bridges
>>>>>>   * @timings: the timing specification for the bridge, if any (may
>>>>>>   * be NULL)
>>>>>> + * @link: drm consumer <-> bridge supplier
>>>>>
>>>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
>>>>> to the bridge" would be better.
>>>>
>>>> I meant "<->" to indicate that the link is bidirectional, not that the
>>>> relationship is in any way symmetric. I wasn't aware of any implication
>>>> of a symmetric relationship when using "<->", do you have a reference?
>>>> But I guess the different arrow notations in math are somewhat overloaded
>>>> and that someone at some point must have used "<->" to indicate a
>>>> symmetric relationship...
>>>
>>> Yeah I agree with Andrzej here, for me <-> implies a symmetric
>>> relationship. Spelling it out like Andrzej suggested sounds like the
>>> better idea.
>>> -Daniel
>>
>> Ok, I guess that means I have to do a v3 after all. Or can this
>> trivial documentation update be done by the committer? I hate to
>> spam everyone with another volley...
>>
>> Or perhaps I should squash patches 2-23 that are all rather similar
>> and mechanic? I separated them to allow for easier review from
>> individual driver maintainers, but that didn't seem to happen
>> anyway...
> 
> Do another volley of the full set, or in-reply-to to just replace the
> patch that needs to be respun (but some people don't like that).
> 
> When resending just make sure you're picking up all the acks/r-bs you
> have already.

Right, I always try to do that. One Ack that I did not include in v2
was the one you had on v1 24/24 (i.e. this patch). The reason I did
not add your Ack for v2 even on the patch where it obviously applied
was that I didn't know if you'd barf on the odev name.

But it was (and still is) a bit unclear if that was on Ack on the
last patch only, or if it was for the whole series? I think it might
have been for the whole series, but I'm not sure and I hate to be a
presumptuous idiot...

Cheers,
Peter

> -Daniel
>> Cheers,
>> Peter
>>
>>>
>>>>
>>>>> Anyway:
>>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>>>
>>>> Thanks!
>>>>
>>>> Cheers,
>>>> Peter
>>>>
>>>>>  --
>>>>> Regards
>>>>> Andrzej
>>>>>
>>>>>>   * @funcs: control functions
>>>>>>   * @driver_private: pointer to the bridge driver's internal context
>>>>>>   */
>>>>>> @@ -271,6 +272,7 @@ struct drm_bridge {
>>>>>>    struct drm_bridge *next;
>>>>>>    struct list_head list;
>>>>>>    const struct drm_bridge_timings *timings;
>>>>>> +  struct device_link *link;
>>>>>>
>>>>>>    const struct drm_bridge_funcs *funcs;
>>>>>>    void *driver_private;
>>>>>
>>>>>
>>>>
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 26/26] drm/bridge: establish a link between the bridge supplier and consumer

[Daniel Vetter]

On Tue, May 15, 2018 at 01:09:59PM +0200, Peter Rosin wrote:
> On 2018-05-15 12:22, Daniel Vetter wrote:
> > On Mon, May 14, 2018 at 10:40 PM, Peter Rosin <peda@axentia.se> wrote:
> >> On 2018-05-14 18:28, Daniel Vetter wrote:
> >>> On Fri, May 11, 2018 at 09:37:47AM +0200, Peter Rosin wrote:
> >>>> On 2018-05-10 10:10, Andrzej Hajda wrote:
> >>>>> On 04.05.2018 15:52, Peter Rosin wrote:
> >>>>>> If the bridge supplier is unbound, this will bring the bridge consumer
> >>>>>> down along with the bridge. Thus, there will no longer linger any
> >>>>>> dangling pointers from the bridge consumer (the drm_device) to some
> >>>>>> non-existent bridge supplier.
> >>>>>>
> >>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/drm_bridge.c | 18 ++++++++++++++++++
> >>>>>>  include/drm/drm_bridge.h     |  2 ++
> >>>>>>  2 files changed, 20 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >>>>>> index 78d186b6831b..0259f0a3ff27 100644
> >>>>>> --- a/drivers/gpu/drm/drm_bridge.c
> >>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
> >>>>>> @@ -26,6 +26,7 @@
> >>>>>>  #include <linux/mutex.h>
> >>>>>>
> >>>>>>  #include <drm/drm_bridge.h>
> >>>>>> +#include <drm/drm_device.h>
> >>>>>>  #include <drm/drm_encoder.h>
> >>>>>>
> >>>>>>  #include "drm_crtc_internal.h"
> >>>>>> @@ -127,12 +128,25 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >>>>>>    if (bridge->dev)
> >>>>>>            return -EBUSY;
> >>>>>>
> >>>>>> +  if (encoder->dev->dev != bridge->odev) {
> >>>>>
> >>>>> I wonder why device_link_add does not handle this case (self dependency)
> >>>>> silently as noop, as it seems to be a correct behavior.
> >>>>
> >>>> It's kind-of a silly corner-case though, so perfectly understandable
> >>>> that it isn't handled.
> >>>>
> >>>>>> +          bridge->link = device_link_add(encoder->dev->dev,
> >>>>>> +                                         bridge->odev, 0);
> >>>>>> +          if (!bridge->link) {
> >>>>>> +                  dev_err(bridge->odev, "failed to link bridge to %s\n",
> >>>>>> +                          dev_name(encoder->dev->dev));
> >>>>>> +                  return -EINVAL;
> >>>>>> +          }
> >>>>>> +  }
> >>>>>> +
> >>>>>>    bridge->dev = encoder->dev;
> >>>>>>    bridge->encoder = encoder;
> >>>>>>
> >>>>>>    if (bridge->funcs->attach) {
> >>>>>>            ret = bridge->funcs->attach(bridge);
> >>>>>>            if (ret < 0) {
> >>>>>> +                  if (bridge->link)
> >>>>>> +                          device_link_del(bridge->link);
> >>>>>> +                  bridge->link = NULL;
> >>>>>>                    bridge->dev = NULL;
> >>>>>>                    bridge->encoder = NULL;
> >>>>>>                    return ret;
> >>>>>> @@ -159,6 +173,10 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> >>>>>>    if (bridge->funcs->detach)
> >>>>>>            bridge->funcs->detach(bridge);
> >>>>>>
> >>>>>> +  if (bridge->link)
> >>>>>> +          device_link_del(bridge->link);
> >>>>>> +  bridge->link = NULL;
> >>>>>> +
> >>>>>>    bridge->dev = NULL;
> >>>>>>  }
> >>>>>>
> >>>>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> >>>>>> index b656e505d11e..804189c63a4c 100644
> >>>>>> --- a/include/drm/drm_bridge.h
> >>>>>> +++ b/include/drm/drm_bridge.h
> >>>>>> @@ -261,6 +261,7 @@ struct drm_bridge_timings {
> >>>>>>   * @list: to keep track of all added bridges
> >>>>>>   * @timings: the timing specification for the bridge, if any (may
> >>>>>>   * be NULL)
> >>>>>> + * @link: drm consumer <-> bridge supplier
> >>>>>
> >>>>> Nitpick: "<->" suggests symmetry, maybe "device link from drm consumer
> >>>>> to the bridge" would be better.
> >>>>
> >>>> I meant "<->" to indicate that the link is bidirectional, not that the
> >>>> relationship is in any way symmetric. I wasn't aware of any implication
> >>>> of a symmetric relationship when using "<->", do you have a reference?
> >>>> But I guess the different arrow notations in math are somewhat overloaded
> >>>> and that someone at some point must have used "<->" to indicate a
> >>>> symmetric relationship...
> >>>
> >>> Yeah I agree with Andrzej here, for me <-> implies a symmetric
> >>> relationship. Spelling it out like Andrzej suggested sounds like the
> >>> better idea.
> >>> -Daniel
> >>
> >> Ok, I guess that means I have to do a v3 after all. Or can this
> >> trivial documentation update be done by the committer? I hate to
> >> spam everyone with another volley...
> >>
> >> Or perhaps I should squash patches 2-23 that are all rather similar
> >> and mechanic? I separated them to allow for easier review from
> >> individual driver maintainers, but that didn't seem to happen
> >> anyway...
> > 
> > Do another volley of the full set, or in-reply-to to just replace the
> > patch that needs to be respun (but some people don't like that).
> > 
> > When resending just make sure you're picking up all the acks/r-bs you
> > have already.
> 
> Right, I always try to do that. One Ack that I did not include in v2
> was the one you had on v1 24/24 (i.e. this patch). The reason I did
> not add your Ack for v2 even on the patch where it obviously applied
> was that I didn't know if you'd barf on the odev name.
> 
> But it was (and still is) a bit unclear if that was on Ack on the
> last patch only, or if it was for the whole series? I think it might
> have been for the whole series, but I'm not sure and I hate to be a
> presumptuous idiot...

Ack on the overall concept, and I'm ok with odev too. But definitely get
acks from relevant people (bridge/driver maintainers). In terms of
patches, I'd say patch 1 + 24-26 have my

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

-Daniel

> 
> Cheers,
> Peter
> 
> > -Daniel
> >> Cheers,
> >> Peter
> >>
> >>>
> >>>>
> >>>>> Anyway:
> >>>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> >>>>
> >>>> Thanks!
> >>>>
> >>>> Cheers,
> >>>> Peter
> >>>>
> >>>>>  --
> >>>>> Regards
> >>>>> Andrzej
> >>>>>
> >>>>>>   * @funcs: control functions
> >>>>>>   * @driver_private: pointer to the bridge driver's internal context
> >>>>>>   */
> >>>>>> @@ -271,6 +272,7 @@ struct drm_bridge {
> >>>>>>    struct drm_bridge *next;
> >>>>>>    struct list_head list;
> >>>>>>    const struct drm_bridge_timings *timings;
> >>>>>> +  struct device_link *link;
> >>>>>>
> >>>>>>    const struct drm_bridge_funcs *funcs;
> >>>>>>    void *driver_private;
> >>>>>
> >>>>>
> >>>>
> >>>
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > 
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 22/26] drm/rcar-du: lvds: provide an owner .odev device for the bridge

[Peter Rosin]

The .of_node member is going away.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3d2d3bbd1342..efda02f55c95 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -463,9 +463,9 @@ static int rcar_lvds_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	lvds->bridge.odev = &pdev->dev;
 	lvds->bridge.driver_private = lvds;
 	lvds->bridge.funcs = &rcar_lvds_bridge_ops;
-	lvds->bridge.of_node = pdev->dev.of_node;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	lvds->mmio = devm_ioremap_resource(&pdev->dev, mem);
-- 
2.11.0

[PATCH v2 23/26] drm/sti: provide an owner .odev device for the bridges

[Peter Rosin]

The .of_node member is going away and providing an .odev will become
mandatory.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/sti/sti_dvo.c  | 2 +-
 drivers/gpu/drm/sti/sti_hda.c  | 1 +
 drivers/gpu/drm/sti/sti_hdmi.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
index a5979cd25cc7..f2609725f8f1 100644
--- a/drivers/gpu/drm/sti/sti_dvo.c
+++ b/drivers/gpu/drm/sti/sti_dvo.c
@@ -460,9 +460,9 @@ static int sti_dvo_bind(struct device *dev, struct device *master, void *data)
 	if (!bridge)
 		return -ENOMEM;
 
+	bridge->odev = &dvo->dev;
 	bridge->driver_private = dvo;
 	bridge->funcs = &sti_dvo_bridge_funcs;
-	bridge->of_node = dvo->dev.of_node;
 	drm_bridge_add(bridge);
 
 	err = drm_bridge_attach(encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index 199db13f565c..7fcd24664cd0 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -694,6 +694,7 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
 	if (!bridge)
 		return -ENOMEM;
 
+	bridge->odev = dev;
 	bridge->driver_private = hda;
 	bridge->funcs = &sti_hda_bridge_funcs;
 	drm_bridge_attach(encoder, bridge, NULL);
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 932724784942..d1d2e1a1920c 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -1270,6 +1270,7 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
 	if (!bridge)
 		return -EINVAL;
 
+	bridge->odev = dev;
 	bridge->driver_private = hdmi;
 	bridge->funcs = &sti_hdmi_bridge_funcs;
 	drm_bridge_attach(encoder, bridge, NULL);
-- 
2.11.0

Re: [PATCH v2 00/26] device link, bridge supplier <-> drm device

[Daniel Vetter]

On Fri, May 04, 2018 at 03:51:46PM +0200, Peter Rosin wrote:
> Hi!
> 
> It was noted by Russel King [1] that bridges (not using components)
> might disappear unexpectedly if the owner of the bridge was unbound.
> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
> came up with using device links to resolve the panel issue, which
> was also my (independent) reaction to the note from Russel.
> 
> This series builds up to the addition of that link in the last
> patch, but in my opinion the other 25 patches do have merit on their
> own.
> 
> The last patch needs testing, while the others look trivial. Jyri, are
> you able to test? That said, I might have missed some subtlety.
> 
> Oh and the reason I'm pushing this is of course so that the issue
> noted by Russel in [1] is addressed which in turn means that the
> tda998x bridge driver can be patched according to that series without
> objection (hopefully) and then used from the atmel-hlcdc driver (and
> other drivers that are not componentized).
> 
> Changes since v1    https://lkml.org/lkml/2018/4/26/1018
> 
> - rename .owner to .odev to not get mixed up with the module owner.
> - added patches for new recent drivers thc63lvd1024 and cdns-dsi
> - fix for problem in the rockchip_lvds driver reported by 0day
> - added a WARN in drm_bridge_add if there is no .odev owner device
> 
> I did *not*:
> - add any ack from Daniel since he suggested "pdev", and I ended up
>   with "odev" in the rename since I disliked "pdev" about as much
>   as "owner".

As long as it's not owner, I'm fine :-) Ack on the idea still holds.

> - add any port id. The current .of_node (that this series removes)
>   does not identify the port, so that problem seems orthogonal
>   to me.

Hm, from my cursory DT/of code reading last week I thought the port is
used to lookup the right node, but there's no port thing on the target for
a phandle? At least that's how current drm_of_find_panel_or_bridge seems
to work ...
-Daniel
> 
> Cheers,
> Peter
> 
> [1] https://lkml.org/lkml/2018/4/23/769
> [2] https://www.spinics.net/lists/dri-devel/msg174275.html
> 
> Peter Rosin (26):
>   drm/bridge: allow optionally specifying an owner .odev device
>   drm/bridge: adv7511: provide an owner .odev device
>   drm/bridge/analogix: core: specify the owner .odev of the bridge
>   drm/bridge: analogix-anx78xx: provide an owner .odev device
>   drm/bridge: cdns-dsi: provide an owner .odev device
>   drm/bridge: vga-dac: provide an owner .odev device
>   drm/bridge: lvds-encoder: provide an owner .odev device
>   drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: provide an owner .odev
>     device
>   drm/bridge: nxp-ptn3460: provide an owner .odev device
>   drm/bridge: panel: provide an owner .odev device
>   drm/bridge: ps8622: provide an owner .odev device
>   drm/bridge: sii902x: provide an owner .odev device
>   drm/bridge: sii9234: provide an owner .odev device
>   drm/bridge: sii8620: provide an owner .odev device
>   drm/bridge: synopsys: provide an owner .odev device for the bridges
>   drm/bridge: tc358767: provide an owner .odev device
>   drm/bridge: thc63lvd1024: provide an owner .odev device
>   drm/bridge: ti-tfp410: provide an owner .odev device
>   drm/exynos: mic: provide an owner .odev device for the bridge
>   drm/mediatek: hdmi: provide an owner .odev device for the bridge
>   drm/msm: specify the owner .odev of the bridges
>   drm/rcar-du: lvds: provide an owner .odev device for the bridge
>   drm/sti: provide an owner .odev device for the bridges
>   drm/bridge: remove the .of_node member
>   drm/bridge: require the owner .odev to be filled in on
>     drm_bridge_add/attach
>   drm/bridge: establish a link between the bridge supplier and consumer
> 
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       |  2 +-
>  drivers/gpu/drm/bridge/analogix-anx78xx.c          |  5 +----
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
>  drivers/gpu/drm/bridge/cdns-dsi.c                  |  2 +-
>  drivers/gpu/drm/bridge/dumb-vga-dac.c              |  2 +-
>  drivers/gpu/drm/bridge/lvds-encoder.c              |  2 +-
>  .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c   |  2 +-
>  drivers/gpu/drm/bridge/nxp-ptn3460.c               |  2 +-
>  drivers/gpu/drm/bridge/panel.c                     |  4 +---
>  drivers/gpu/drm/bridge/parade-ps8622.c             |  2 +-
>  drivers/gpu/drm/bridge/sii902x.c                   |  2 +-
>  drivers/gpu/drm/bridge/sii9234.c                   |  2 +-
>  drivers/gpu/drm/bridge/sil-sii8620.c               |  2 +-
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          |  4 +---
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c      |  4 +---
>  drivers/gpu/drm/bridge/tc358767.c                  |  2 +-
>  drivers/gpu/drm/bridge/thc63lvd1024.c              |  2 +-
>  drivers/gpu/drm/bridge/ti-tfp410.c                 |  2 +-
>  drivers/gpu/drm/drm_bridge.c                       | 26 +++++++++++++++++++++-
>  drivers/gpu/drm/exynos/exynos_drm_mic.c            |  2 +-
>  drivers/gpu/drm/mediatek/mtk_hdmi.c                |  2 +-
>  drivers/gpu/drm/msm/dsi/dsi_manager.c              |  1 +
>  drivers/gpu/drm/msm/edp/edp_bridge.c               |  1 +
>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c             |  1 +
>  drivers/gpu/drm/rcar-du/rcar_lvds.c                |  2 +-
>  drivers/gpu/drm/rockchip/rockchip_lvds.c           |  2 +-
>  drivers/gpu/drm/sti/sti_dvo.c                      |  2 +-
>  drivers/gpu/drm/sti/sti_hda.c                      |  1 +
>  drivers/gpu/drm/sti/sti_hdmi.c                     |  1 +
>  include/drm/drm_bridge.h                           |  8 +++----
>  30 files changed, 57 insertions(+), 36 deletions(-)
> 
> -- 
> 2.11.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 00/26] device link, bridge supplier <-> drm device

[Peter Rosin]

On 2018-05-07 15:56, Daniel Vetter wrote:
> On Fri, May 04, 2018 at 03:51:46PM +0200, Peter Rosin wrote:
>> Hi!
>>
>> It was noted by Russel King [1] that bridges (not using components)
>> might disappear unexpectedly if the owner of the bridge was unbound.
>> Jyri Sarha had previously noted the same thing with panels [2]. Jyri
>> came up with using device links to resolve the panel issue, which
>> was also my (independent) reaction to the note from Russel.
>>
>> This series builds up to the addition of that link in the last
>> patch, but in my opinion the other 25 patches do have merit on their
>> own.
>>
>> The last patch needs testing, while the others look trivial. Jyri, are
>> you able to test? That said, I might have missed some subtlety.
>>
>> Oh and the reason I'm pushing this is of course so that the issue
>> noted by Russel in [1] is addressed which in turn means that the
>> tda998x bridge driver can be patched according to that series without
>> objection (hopefully) and then used from the atmel-hlcdc driver (and
>> other drivers that are not componentized).
>>
>> Changes since v1    https://lkml.org/lkml/2018/4/26/1018
>>
>> - rename .owner to .odev to not get mixed up with the module owner.
>> - added patches for new recent drivers thc63lvd1024 and cdns-dsi
>> - fix for problem in the rockchip_lvds driver reported by 0day
>> - added a WARN in drm_bridge_add if there is no .odev owner device
>>
>> I did *not*:
>> - add any ack from Daniel since he suggested "pdev", and I ended up
>>   with "odev" in the rename since I disliked "pdev" about as much
>>   as "owner".
> 
> As long as it's not owner, I'm fine :-) Ack on the idea still holds.
> 
>> - add any port id. The current .of_node (that this series removes)
>>   does not identify the port, so that problem seems orthogonal
>>   to me.
> 
> Hm, from my cursory DT/of code reading last week I thought the port is
> used to lookup the right node, but there's no port thing on the target for
> a phandle? At least that's how current drm_of_find_panel_or_bridge seems
> to work ...

drm_of_find_panel_or_bridge calls of_graph_get_remote_node and that
function looks up the main remote device node, i.e. not the remote
port or endpoint node but the parent node. So, bridges using .of_node
have stored their main device node in the .of_node member. I.e. the
same value as of_node in struct device for all current cases.

Cheers,
Peter

> -Daniel
>>
>> Cheers,
>> Peter
>>
>> [1] https://lkml.org/lkml/2018/4/23/769
>> [2] https://www.spinics.net/lists/dri-devel/msg174275.html
>>
>> Peter Rosin (26):
>>   drm/bridge: allow optionally specifying an owner .odev device
>>   drm/bridge: adv7511: provide an owner .odev device
>>   drm/bridge/analogix: core: specify the owner .odev of the bridge
>>   drm/bridge: analogix-anx78xx: provide an owner .odev device
>>   drm/bridge: cdns-dsi: provide an owner .odev device
>>   drm/bridge: vga-dac: provide an owner .odev device
>>   drm/bridge: lvds-encoder: provide an owner .odev device
>>   drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: provide an owner .odev
>>     device
>>   drm/bridge: nxp-ptn3460: provide an owner .odev device
>>   drm/bridge: panel: provide an owner .odev device
>>   drm/bridge: ps8622: provide an owner .odev device
>>   drm/bridge: sii902x: provide an owner .odev device
>>   drm/bridge: sii9234: provide an owner .odev device
>>   drm/bridge: sii8620: provide an owner .odev device
>>   drm/bridge: synopsys: provide an owner .odev device for the bridges
>>   drm/bridge: tc358767: provide an owner .odev device
>>   drm/bridge: thc63lvd1024: provide an owner .odev device
>>   drm/bridge: ti-tfp410: provide an owner .odev device
>>   drm/exynos: mic: provide an owner .odev device for the bridge
>>   drm/mediatek: hdmi: provide an owner .odev device for the bridge
>>   drm/msm: specify the owner .odev of the bridges
>>   drm/rcar-du: lvds: provide an owner .odev device for the bridge
>>   drm/sti: provide an owner .odev device for the bridges
>>   drm/bridge: remove the .of_node member
>>   drm/bridge: require the owner .odev to be filled in on
>>     drm_bridge_add/attach
>>   drm/bridge: establish a link between the bridge supplier and consumer
>>
>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c       |  2 +-
>>  drivers/gpu/drm/bridge/analogix-anx78xx.c          |  5 +----
>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  1 +
>>  drivers/gpu/drm/bridge/cdns-dsi.c                  |  2 +-
>>  drivers/gpu/drm/bridge/dumb-vga-dac.c              |  2 +-
>>  drivers/gpu/drm/bridge/lvds-encoder.c              |  2 +-
>>  .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c   |  2 +-
>>  drivers/gpu/drm/bridge/nxp-ptn3460.c               |  2 +-
>>  drivers/gpu/drm/bridge/panel.c                     |  4 +---
>>  drivers/gpu/drm/bridge/parade-ps8622.c             |  2 +-
>>  drivers/gpu/drm/bridge/sii902x.c                   |  2 +-
>>  drivers/gpu/drm/bridge/sii9234.c                   |  2 +-
>>  drivers/gpu/drm/bridge/sil-sii8620.c               |  2 +-
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c          |  4 +---
>>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c      |  4 +---
>>  drivers/gpu/drm/bridge/tc358767.c                  |  2 +-
>>  drivers/gpu/drm/bridge/thc63lvd1024.c              |  2 +-
>>  drivers/gpu/drm/bridge/ti-tfp410.c                 |  2 +-
>>  drivers/gpu/drm/drm_bridge.c                       | 26 +++++++++++++++++++++-
>>  drivers/gpu/drm/exynos/exynos_drm_mic.c            |  2 +-
>>  drivers/gpu/drm/mediatek/mtk_hdmi.c                |  2 +-
>>  drivers/gpu/drm/msm/dsi/dsi_manager.c              |  1 +
>>  drivers/gpu/drm/msm/edp/edp_bridge.c               |  1 +
>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c             |  1 +
>>  drivers/gpu/drm/rcar-du/rcar_lvds.c                |  2 +-
>>  drivers/gpu/drm/rockchip/rockchip_lvds.c           |  2 +-
>>  drivers/gpu/drm/sti/sti_dvo.c                      |  2 +-
>>  drivers/gpu/drm/sti/sti_hda.c                      |  1 +
>>  drivers/gpu/drm/sti/sti_hdmi.c                     |  1 +
>>  include/drm/drm_bridge.h                           |  8 +++----
>>  30 files changed, 57 insertions(+), 36 deletions(-)
>>
>> -- 
>> 2.11.0
>>
> 

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