Messages in this thread | | | Date | Sat, 10 Jun 2017 23:16:35 +0800 | From | icenowy@aosc ... | Subject | Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2 |
| |
在 2017-06-10 22:57,icenowy@aosc.io 写道: > 在 2017-06-09 22:46,Maxime Ripard 写道: >> On Thu, Jun 08, 2017 at 01:01:53PM +0800, icenowy@aosc.io wrote: >>> 在 2017-06-07 22:38,Maxime Ripard 写道: >>> > On Wed, Jun 07, 2017 at 06:01:02PM +0800, Icenowy Zheng wrote: >>> > > >I have no idea what this is supposed to be doing either. >>> > > > >>> > > >I might be wrong, but I really feel like there's a big mismatch >>> > > >between your commit log, and what you actually implement. >>> > > > >>> > > >In your commit log, you should state: >>> > > > >>> > > >A) What is the current behaviour >>> > > >B) Why that is a problem >>> > > >C) How do you address it >>> > > > >>> > > >And you don't. >>> > > > >>> > > >However, after discussing it with Chen-Yu, it seems like you're trying >>> > > >to have all the mixers probed before the TCONs. If that is so, there's >>> > > >nothing specific to the H3 here, and we also have the same issue on >>> > > >dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but >>> > > >the easiest solution would be to move from a DFS algorithm to walk >>> > > >down the graph to a BFS one. >>> > > > >>> > > >That way, we would add all mixers first, then the TCONs, then the >>> > > >encoders, and the component framework will probe them in order. >>> > > >>> > > No. I said that they're swappable, however, I don't want to >>> > > implement the swap now, but hardcode 0-0 1-1 connection. >>> > >>> > We're on the same page, it's definitely not what I was mentionning >>> > here. This would require a significant rework, and the usecase is >>> > still unclear for now. >>> > >>> > > However, as you and Chen-Yu said, device tree should reflect the >>> > > real hardware, there will be bonus endpoints for the swapped >>> > > connection. >>> > >>> > If by bonus you mean connections from mixer 0 to tcon 1 and mixer 1 to >>> > tcon 0, then yes, we're going to need it. >>> > >>> > > What I want to do is to ignore the bonus connection, in order to >>> > > prevent them from confusing the code. >>> > > >>> > > If you just change the bind sequence, I think it cannot be >>> > > prevented that wrong connections will be bound. >>> > >>> > This is where I don't follow you anymore. The component framework >>> > doesn't list connections but devices. The swapped connections do not >>> > matter here, we have the same set of devices: mixer0, mixer1, tcon0 >>> > and tcon1. >>> > >>> > The thing that does change with your patch is that before, the binding >>> > sequence would have been mixer0, tcon0, tcon1, mixer1. With your >>> > patch, it's mixer0, tcon0, mixer1, tcon1. >>> > >>> > So, again, stating what issue you were seeing before making this patch >>> > would be very helpful to see what you're trying to do / fix. >>> >>> So maybe I can drop the forward search (searching output) code, and >>> keep >>> only the backward search (search input) code in TCON? >>> >>> Forward search code is only used when binding, but backward search is >>> used >>> for TCON to find connected mixer. >> >> It is hard to talk about a solution, when it's not clear what the >> issue is. >> >> So please state >>> > > >A) What is the current behaviour >>> > > >B) Why that is a problem >>> > > >C) How do you address it >> >> We'll talk about a solution once this is done. > > (All those things are based on the assumption that mixer0, mixer1, > tcon0 > and tcon1 are all enabled in DT. If one group of mixer-tcon pair is > fully > disabled in DT it will behave properly.)
So there's a temporary workaround -- only enable one pipeline and disable the unused mixer and tcon totally.
It's shown to work with this commit reverted in my local TVE branch. (The swappable_input value is also deleted from H3 TCON's quirks)
> > For the backward search: > > A) The current behaviour is to take the first engine found, which will > be > wrong in the situation of tcon1 if mixer0 and mixer1 are both enabled: > mixer0 is taken for tcon1 instead of mixer1. > > B) It takes mixer0 as it matches the first endpoint of tcon0's input. > > C) It's a logic failure in the backward search, as it only considered > the DE1 situation, in which TCONs will only have one engine as input. > > For the bind process: > > A) The current behaviour is to try to bind all output endpoints of the > engine, during binding all outputs of mixer0, these will happen: > 1. tcon1 is bound with mixer0 as its engine if backward searching > is not fixed. > 2. tcon1 fails to be bound as its engine is not yet bound when > backward searching works properly, then sun4i_drv will refuse > to continue as a component is not properly bound. > B) The binding process in sun4i_drv will bind a component that is not > really an working output of the forward component, but only exists in > the endpoint list as a theortically possible output (in fact not an > real output). > C) I tested with this patch's sun4i_drv_node_is_swappable_de2_mixer > function masked (always return false), and then the multiple > mixer+tcon situations don't work properly. > > P.S. I think the BFS solution is really a dirty hack -- although we > bind components, not connections, we should decide the next component > to bind according to the connections -- not really connected > components shouldn't be bound. > > For example, if we enabled mixer0, tcon0 and tcon1, tcon1 shouldn't > be bound at all. However in BFS situation tcon1 will also be bound > and then fail to be bound if the backward engine searching is fixed. > >> >> Maxime > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
| |