LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Peter Rosin <peda@axentia.se>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-kernel@vger.kernel.org,
	Archit Taneja <architt@codeaurora.org>,
	Andrzej Hajda <a.hajda@samsung.com>,
	David Airlie <airlied@linux.ie>,
	Peter Senna Tschudin <peter.senna@collabora.com>,
	Martin Donnelly <martin.donnelly@ge.com>,
	Martyn Welch <martyn.welch@collabora.co.uk>,
	Gustavo Padovan <gustavo@padovan.org>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Sean Paul <seanpaul@chromium.org>,
	Inki Dae <inki.dae@samsung.com>,
	Joonyoung Shim <jy0922.shim@samsung.com>,
	Seung-Woo Kim <sw0312.kim@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Kukjin Kim <kgene@kernel.org>,
	Krzysztof Kozlowski <krzk@kernel.org>, CK Hu <ck.hu@mediatek.com>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Rob Clark <robdclark@gmail.com>,
	Benjamin Gaignard <benjamin.gaignard@linaro.org>,
	Vincent Abriou <vincent.abriou@st.com>,
	Jyri Sarha <jsarha@ti.com>,
	dri-devel@lists.freedesktop.org,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org,
	linux-mediatek@lists.infradead.org,
	linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH 00/24] device link, bridge supplier <-> drm device
Date: Fri, 27 Apr 2018 09:27:58 +0200	[thread overview]
Message-ID: <11768d9a-91cb-55e3-a026-3663241bffe5@axentia.se> (raw)
In-Reply-To: <3483427.EaMxDYJt8U@avalon>

On 2018-04-27 01:18, Laurent Pinchart wrote:
> Hi Peter,
> 
> On Friday, 27 April 2018 02:09:14 EEST Peter Rosin wrote:
>> On 2018-04-27 00:40, Laurent Pinchart wrote:
>>> On Friday, 27 April 2018 01:31:15 EEST 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 23 patches do have merit on their
>>>> own.
>>>>
>>>> The last patch needs testing, while the others look trivial. That
>>>> said, I might have missed some subtlety.
>>>
>>> I don't think this is the way to go. We have a real lifetime management
>>> problem with bridges, and device links are just trying to hide the problem
>>> under the carpet. They will further corner us by making a real fix much
>>> more difficult to implement. I'll try to comment further in the next few
>>> days on what I think a better solution would be, but in a nutshell I
>>> believe that drm_bridge objects need to be refcounted, with a .release()
>>> operation to free the bridge resources when the reference count drops to
>>> zero. This shouldn't be difficult to implement and I'm willing to help.
>>
>> Ok, sp 24/24 is dead, and maybe 23/24 too.
> 
> Not necessarily, maybe I'll get proven wrong :-) Let's discuss, but I first 
> need some sleep.

Ok, I'll add my view...

I don't see how a refcount is going to resolve anything. Let's take some
device that allocs a bridge that is later attached to some encoder. In doing
so it adds hooks to some of the drm_bridge_funcs. If you add a refcount to
the bridge itself then yes, the bridge object might remain but the code
backing the hook functions will go away the moment the owner device goes
away. So, you'd have to refcount the owner device itself to prevent it
from going away. But, you can't stop a device from going away IIUC, you can
only bring other stuff down with it in an orderly fashion.

Components, that some bridges use, takes care of bringing the whole cluster
down *before* any device goes away, so that is obviously a solution. But
that solution is not in place everywhere.

Now, my device-link is pretty light-weight. And it should only matter if
the owner goes away before the consuming drm device has brought down the
encoder properly. If that ever happens, we're in trouble. So, the link can
at worst only substitute one problem with another, and at best it prevents
the fireworks.

Sure, there's the reprobe problem if the bridge's owner device shows up
again, but that's pretty minor compared to a hard crash. And there was a
patch for that, so in the end that may be a non-issue.

If some other solution is found, removing the device-link is trivial. It is
localized to bridge attach/detach and nothing else is affected (in terms of
code).

Cheers,
Peter

>> But how do you feel about dropping .of_node in favour of .owner? That gets
>> rid of ugly #ifdefs...
> 
> I'll review that part and reply, I have nothing against it on principle at the 
> moment. The more generic the code is, the better in my opinion. We just need 
> to make sure that the OF node we're interested in will always be .owner-
>> of_node in all cases.
> 
>> I also have the nagging feeling that .driver_private serves very little real
>> purpose if there is a .owner so that you can do
>>
>> 	dev_get_drvdata(bridge->owner)
>>
>> for the cases where the container_of macro is not appropriate.
> 
> I'll review that too, it's a good point.
> 

  parent reply	other threads:[~2018-04-27  7:28 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20180426223215epcas2p38fe3b0b7b1ea1c8190805f9e2a89e11e@epcas2p3.samsung.com>
2018-04-26 22:31 ` Peter Rosin
2018-04-26 22:31   ` [PATCH 01/24] drm/bridge: allow optionally specifying an .owner device Peter Rosin
2018-04-26 22:31   ` [PATCH 02/24] drm/bridge: adv7511: provide " Peter Rosin
2018-04-26 22:31   ` [PATCH 03/24] drm/bridge/analogix: core: specify the .owner of the bridge Peter Rosin
2018-04-26 22:31   ` [PATCH 04/24] drm/bridge: analogix-anx78xx: provide an .owner device Peter Rosin
2018-04-26 22:31   ` [PATCH 05/24] drm/bridge: vga-dac: " Peter Rosin
2018-04-26 22:31   ` [PATCH 06/24] drm/bridge: lvds-encoder: " Peter Rosin
2018-04-26 22:31   ` [PATCH 07/24] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: " Peter Rosin
2018-04-26 22:31   ` [PATCH 08/24] drm/bridge: nxp-ptn3460: " Peter Rosin
2018-04-26 22:31   ` [PATCH 09/24] drm/bridge: panel: " Peter Rosin
2018-04-26 22:31   ` [PATCH 10/24] drm/bridge: ps8622: " Peter Rosin
2018-04-26 22:31   ` [PATCH 11/24] drm/bridge: sii902x: " Peter Rosin
2018-04-26 22:31   ` [PATCH 12/24] drm/bridge: sii9234: " Peter Rosin
2018-04-26 22:31   ` [PATCH 13/24] drm/bridge: sii8620: " Peter Rosin
2018-04-26 22:31   ` [PATCH 14/24] drm/bridge: synopsys: provide an .owner device for the bridges Peter Rosin
2018-04-26 22:31   ` [PATCH 15/24] drm/bridge: tc358767: provide an .owner device Peter Rosin
2018-04-26 22:31   ` [PATCH 16/24] drm/bridge: ti-tfp410: " Peter Rosin
2018-04-26 22:31   ` [PATCH 17/24] drm/exynos: mic: provide an .owner device for the bridge Peter Rosin
2018-04-26 22:31   ` [PATCH 18/24] drm/mediatek: hdmi: " Peter Rosin
2018-04-26 22:31   ` [PATCH 19/24] drm/msm: specify the .owner of the bridges Peter Rosin
2018-04-26 22:31   ` [PATCH 20/24] drm/rcar-du: lvds: provide an .owner device for the bridge Peter Rosin
2018-04-26 22:31   ` [PATCH 21/24] drm/sti: provide an .owner device for the bridges Peter Rosin
2018-04-26 22:31   ` [PATCH 22/24] drm/bridge: remove the .of_node member Peter Rosin
2018-04-28  8:09     ` kbuild test robot
2018-04-30 11:14       ` Peter Rosin
2018-04-26 22:31   ` [PATCH 23/24] drm/bridge: require the .owner to be filled in on drm_bridge_attach Peter Rosin
2018-04-30 15:24     ` Daniel Vetter
2018-04-30 20:32       ` Peter Rosin
2018-04-26 22:31   ` [PATCH 24/24] drm/bridge: establish a link between the bridge supplier and consumer Peter Rosin
2018-04-30 15:32     ` Daniel Vetter
2018-04-30 21:12       ` Peter Rosin
     [not found]   ` <1881012.XPHlTWYXH6@avalon>
2018-04-26 23:09     ` [PATCH 00/24] device link, bridge supplier <-> drm device Peter Rosin
     [not found]       ` <3483427.EaMxDYJt8U@avalon>
2018-04-27  7:27         ` Peter Rosin [this message]
2018-04-30 15:18     ` Daniel Vetter
2018-04-27  7:11   ` Andrzej Hajda
2018-04-27  7:37     ` Peter Rosin
2018-04-27  7:51       ` Peter Rosin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=11768d9a-91cb-55e3-a026-3663241bffe5@axentia.se \
    --to=peda@axentia.se \
    --cc=a.hajda@samsung.com \
    --cc=airlied@linux.ie \
    --cc=architt@codeaurora.org \
    --cc=benjamin.gaignard@linaro.org \
    --cc=ck.hu@mediatek.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=gustavo@padovan.org \
    --cc=inki.dae@samsung.com \
    --cc=jsarha@ti.com \
    --cc=jy0922.shim@samsung.com \
    --cc=kgene@kernel.org \
    --cc=krzk@kernel.org \
    --cc=kyungmin.park@samsung.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=martin.donnelly@ge.com \
    --cc=martyn.welch@collabora.co.uk \
    --cc=matthias.bgg@gmail.com \
    --cc=p.zabel@pengutronix.de \
    --cc=peter.senna@collabora.com \
    --cc=robdclark@gmail.com \
    --cc=seanpaul@chromium.org \
    --cc=sw0312.kim@samsung.com \
    --cc=vincent.abriou@st.com \
    --subject='Re: [PATCH 00/24] device link, bridge supplier <-> drm device' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).