From mboxrd@z Thu Jan 1 00:00:00 1970 From: icenowy-h8G6r0blFSE@public.gmane.org Subject: Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2 Date: Sat, 10 Jun 2017 23:16:35 +0800 Message-ID: <0b7d3cc334929ab2464477bbccc37e31@aosc.io> References: <20170604160149.30230-1-icenowy@aosc.io> <20170604160149.30230-4-icenowy@aosc.io> <20170607093512.rfvpefmyskgjw3ik@flea.lan> <01A7F22E-C4FF-4B4B-A9BC-FF0C96B996B9@aosc.io> <20170607143827.4ng5gedvzn3f5pyx@flea.lan> <66881eac1dd06d918692482bdb1ea9e6@aosc.io> <20170609144649.67i3zscq26jt5hhe@flea.home> <03e5cb4ada3e19ed3476e1d562450f6f@aosc.io> Reply-To: icenowy-h8G6r0blFSE@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable Return-path: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org In-Reply-To: <03e5cb4ada3e19ed3476e1d562450f6f-h8G6r0blFSE@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Maxime Ripard Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, =?UTF-8?Q?Jernej_=C5=A0krabec?= , linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Chen-Yu Tsai , Rob Herring , linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org =E5=9C=A8 2017-06-10 22:57=EF=BC=8Cicenowy-h8G6r0blFSE@public.gmane.org =E5=86=99=E9=81=93=EF=BC= =9A > =E5=9C=A8 2017-06-09 22:46=EF=BC=8CMaxime Ripard =E5=86=99=E9=81=93=EF=BC= =9A >> On Thu, Jun 08, 2017 at 01:01:53PM +0800, icenowy-h8G6r0blFSE@public.gmane.org wrote: >>> =E5=9C=A8 2017-06-07 22:38=EF=BC=8CMaxime Ripard =E5=86=99=E9=81=93=EF= =BC=9A >>> > 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 tr= ying >>> > > >to have all the mixers probed before the TCONs. If that is so, the= re's >>> > > >nothing specific to the H3 here, and we also have the same issue o= n >>> > > >dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, b= ut >>> > > >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 t= o >>> > 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 bindin= g >>> > 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 patc= h >>> > would be very helpful to see what you're trying to do / fix. >>>=20 >>> So maybe I can drop the forward search (searching output) code, and=20 >>> keep >>> only the backward search (search input) code in TCON? >>>=20 >>> Forward search code is only used when binding, but backward search is= =20 >>> used >>> for TCON to find connected mixer. >>=20 >> It is hard to talk about a solution, when it's not clear what the >> issue is. >>=20 >> So please state >>> > > >A) What is the current behaviour >>> > > >B) Why that is a problem >>> > > >C) How do you address it >>=20 >> We'll talk about a solution once this is done. >=20 > (All those things are based on the assumption that mixer0, mixer1,=20 > tcon0 > and tcon1 are all enabled in DT. If one group of mixer-tcon pair is=20 > fully > disabled in DT it will behave properly.) So there's a temporary workaround -- only enable one pipeline and=20 disable the unused mixer and tcon totally. It's shown to work with this commit reverted in my local TVE branch.=20 (The swappable_input value is also deleted from H3 TCON's quirks) >=20 > For the backward search: >=20 > A) The current behaviour is to take the first engine found, which will=20 > be > wrong in the situation of tcon1 if mixer0 and mixer1 are both enabled: > mixer0 is taken for tcon1 instead of mixer1. >=20 > B) It takes mixer0 as it matches the first endpoint of tcon0's input. >=20 > 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. >=20 > For the bind process: >=20 > 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. >=20 > 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. >=20 > 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. >=20 >>=20 >> Maxime >=20 > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --=20 You received this message because you are subscribed to the Google Groups "= linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an e= mail to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org For more options, visit https://groups.google.com/d/optout.