Message-ID: <20170613094618.hxjuc5yx7udypbxh@flea.lan>
Date:   Tue, 13 Jun 2017 11:46:18 +0200
From:   Maxime Ripard <maxime.ripard@...e-electrons.com>
To:     icenowy@...c.io
Cc:     devicetree@...r.kernel.org,
        Jernej Škrabec <jernej.skrabec@...l.net>,
        linux-sunxi@...glegroups.com, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, Chen-Yu Tsai <wens@...e.org>,
        Rob Herring <robh+dt@...nel.org>, linux-clk@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon
 connection for DE2

On Sat, Jun 10, 2017 at 11:16:35PM +0800, icenowy@...c.io wrote:
> 在 2017-06-10 22:57，icenowy@...c.io 写道：
> > 在 2017-06-09 22:46，Maxime Ripard 写道：
> > > On Thu, Jun 08, 2017 at 01:01:53PM +0800, icenowy@...c.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.

As a short-term workaround, while we rework the code to enable both
pipelines, it definitely works for me.

Maxim

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)

Powered by blists - more mailing lists