From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754910AbdCIOlK (ORCPT ); Thu, 9 Mar 2017 09:41:10 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:39260 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754776AbdCIOlG (ORCPT ); Thu, 9 Mar 2017 09:41:06 -0500 Date: Thu, 9 Mar 2017 15:40:09 +0100 From: Maxime Ripard To: Chen-Yu Tsai Cc: David Airlie , dri-devel , linux-sunxi , linux-arm-kernel , linux-kernel Subject: Re: [PATCH 09/11] drm/sun4i: Support two display pipelines Message-ID: <20170309144009.kgixcnrjg4cot6ut@lukather> References: <20170309100534.14023-1-wens@csie.org> <20170309100534.14023-10-wens@csie.org> <20170309103636.j5jtjhwjztzchr6u@lukather> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="smsj7k2syl2frb2l" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --smsj7k2syl2frb2l Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 09, 2017 at 07:20:30PM +0800, Chen-Yu Tsai wrote: > On Thu, Mar 9, 2017 at 6:36 PM, Maxime Ripard > wrote: > > Hi, > > > > On Thu, Mar 09, 2017 at 06:05:32PM +0800, Chen-Yu Tsai wrote: > >> Some Allwinner SoCs have two display pipelines (frontend -> backend -> > >> tcon). > >> > >> Previously we only supported one pipeline. This patch extends the > >> current driver to support two. It extends the tcon and backend pointers > >> in sun4i_drv into arrays, and makes the related bind functions store > >> the pointer into said arrays based on the id fetched from the device > >> tree. In the case of the tcons, it falls back to a first come order > >> if no encoders that can be used for differentiating the tcons are > >> defined. The driver's depth-first traversal of the of graph, coupled > >> with the increasing address ordering of the of graph endpoints, and > >> the fact that tcon0 should always be enabled for the tcon/encoder > >> mux to be accessible, means that tcon1 would always come after tcon0. > >> > >> Assignment of the device structure into sun4i_drv is moved to the end > >> of the bind function, when all possible error checks have passed. > >> > >> This patch also drops a trailing 0 in one of the backend probe message= s. > >> > >> Signed-off-by: Chen-Yu Tsai > >> --- > >> drivers/gpu/drm/sun4i/sun4i_backend.c | 9 +++++++-- > >> drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +- > >> drivers/gpu/drm/sun4i/sun4i_drv.h | 6 ++++-- > >> drivers/gpu/drm/sun4i/sun4i_tcon.c | 25 +++++++++++++++++-------- > >> 4 files changed, 29 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/s= un4i/sun4i_backend.c > >> index f3c92d54c8e4..8d22efd5a9cc 100644 > >> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c > >> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c > >> @@ -350,12 +350,15 @@ static int sun4i_backend_bind(struct device *dev= , struct device *master, > >> if (!backend) > >> return -ENOMEM; > >> dev_set_drvdata(dev, backend); > >> - drv->backend =3D backend; > >> > >> backend->id =3D sun4i_backend_of_get_id(dev->of_node); > >> if (backend->id < 0) > >> return backend->id; > >> > >> + /* We only support SUN4I_DRM_MAX_PIPELINES number of backends */ > >> + if (backend->id >=3D SUN4I_DRM_MAX_PIPELINES) > >> + return -EINVAL; > >> + > >> res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> regs =3D devm_ioremap_resource(dev, res); > >> if (IS_ERR(regs)) > >> @@ -364,7 +367,7 @@ static int sun4i_backend_bind(struct device *dev, = struct device *master, > >> backend->regs =3D devm_regmap_init_mmio(dev, regs, > >> &sun4i_backend_regmap_conf= ig); > >> if (IS_ERR(backend->regs)) { > >> - dev_err(dev, "Couldn't create the backend0 regmap\n"); > >> + dev_err(dev, "Couldn't create the backend regmap\n"); > >> return PTR_ERR(backend->regs); > >> } > >> > >> @@ -413,6 +416,8 @@ static int sun4i_backend_bind(struct device *dev, = struct device *master, > >> } > >> } > >> > >> + drv->backend[backend->id] =3D backend; > >> + > >> /* Reset the registers */ > >> for (i =3D 0x800; i < 0x1000; i +=3D 4) > >> regmap_write(backend->regs, i, 0); > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i= /sun4i_drv.c > >> index 767bbadcc85d..c15ecb8343d7 100644 > >> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c > >> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c > >> @@ -271,7 +271,7 @@ static int sun4i_drv_probe(struct platform_device = *pdev) > >> struct device_node *np =3D pdev->dev.of_node; > >> int i, count =3D 0; > >> > >> - for (i =3D 0;; i++) { > >> + for (i =3D 0; i < SUN4I_DRM_MAX_PIPELINES; i++) { > >> struct device_node *pipeline =3D of_parse_phandle(np, > >> "allwinn= er,pipelines", > >> i); > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.h b/drivers/gpu/drm/sun4i= /sun4i_drv.h > >> index 5df50126ff52..ec1c08af47e1 100644 > >> --- a/drivers/gpu/drm/sun4i/sun4i_drv.h > >> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.h > >> @@ -16,9 +16,11 @@ > >> #include > >> #include > >> > >> +#define SUN4I_DRM_MAX_PIPELINES 2 > >> + > >> struct sun4i_drv { > >> - struct sun4i_backend *backend; > >> - struct sun4i_tcon *tcon; > >> + struct sun4i_backend *backend[SUN4I_DRM_MAX_PIPELINES]; > >> + struct sun4i_tcon *tcon[SUN4I_DRM_MAX_PIPELINES]; > >> > >> struct drm_fbdev_cma *fbdev; > >> }; > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4= i/sun4i_tcon.c > >> index b774c9a50c55..7749c3133f38 100644 > >> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c > >> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c > >> @@ -532,7 +532,6 @@ static int sun4i_tcon_bind(struct device *dev, str= uct device *master, > >> if (!tcon) > >> return -ENOMEM; > >> dev_set_drvdata(dev, tcon); > >> - drv->tcon =3D tcon; > >> tcon->drm =3D drm; > >> tcon->dev =3D dev; > >> tcon->quirks =3D of_device_get_match_data(dev); > >> @@ -540,14 +539,22 @@ static int sun4i_tcon_bind(struct device *dev, s= truct device *master, > >> /* This can fail if the DT does not have any downstream encoders= =2E */ > >> tcon->id =3D sun4i_tcon_of_get_id(dev->of_node); > >> if (tcon->id < 0) { > >> - /* > >> - * TODO We currently support only 1 TCON, so we can > >> - * safely set this to 0. This should be revisited > >> - * when we add support for multiple pipelines. > >> - */ > >> - tcon->id =3D 0; > >> + int i; > >> + > >> + /* find the first empty tcon in sun4i_drv */ > >> + for (i =3D 0; i < SUN4I_DRM_MAX_PIPELINES; i++) > >> + if (!drv->tcon[i]) > >> + tcon->id =3D i; > >> + > >> + /* bail out if that failed */ > >> + if (tcon->id < 0) > >> + return tcon->id; > >> } > >> > >> + /* We only support SUN4I_DRM_MAX_PIPELINES number of tcons */ > >> + if (tcon->id >=3D SUN4I_DRM_MAX_PIPELINES) > >> + return -EINVAL; > >> + > >> tcon->lcd_rst =3D devm_reset_control_get(dev, "lcd"); > >> if (IS_ERR(tcon->lcd_rst)) { > >> dev_err(dev, "Couldn't get our reset line\n"); > >> @@ -588,7 +595,7 @@ static int sun4i_tcon_bind(struct device *dev, str= uct device *master, > >> goto err_free_dotclock; > >> } > >> > >> - tcon->crtc =3D sun4i_crtc_init(drm, drv->backend, tcon); > >> + tcon->crtc =3D sun4i_crtc_init(drm, drv->backend[tcon->id], tcon= ); > > > > I'm not a big fan of those IDs. The heuristic seems to be a bit > > fragile since we really never enforced any order in our bindings. >=20 > Yes. I seem to have forgotten that bit which I had intended to add. > The endpoint IDs would ideally match the actual mux values used. That works for me, but the binding documentation would need to be amended. > On the TCON side that's not doable, so we might have to just require > them being in an increasing order. What are you planning to use those IDs on for the TCON? >=20 > > You seem to use it for two things: > > - to match a TCON to its backend in our code > > - to not step on each others' toes when registering the backends/tcons > > > > I think the second could be easily addressed using a linked list, and > > the first one by storing the of_node. Then we just need to follow the > > OF graph to our input of_node, and then iterate through our registered > > backend list to find the one with the same of_node. >=20 > Yes that would work for the above purposes. >=20 > For getting the first TCON to access the mux registers, it kind of falls > short. We also need something like this for the TV encoders on A10/A20. > They too have a mux, which seems to be in the first TV encoder. This > controls how the 8 DACs are mapped to the 4 external pins. How are those pins muxed between the two? Can't we just create a connector that would be usable for both encoders? Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --smsj7k2syl2frb2l Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYwWlFAAoJEBx+YmzsjxAgbhkP/05A5X2NmM0id7khWHsZrjEn DxJ8x9bc+MVHVni5kKaf5rcWwgU5mpox1+oKq8ewvCdknGDOp92bG76TcHDBmt++ UW/UX8SoDE3KdpaBQxzxpmxsNqF+VW34n6WsUPibtjnA+Eizs7Vl9KNwojO6xtUP rwMqdhcL3nlnt+wldDMtKnGXanV4UbIotv1YZ1NVYgd+XOAyAVGecsKRmrlgYLWq cYlh+smrlbWHe4TiEEZAlYsVpgMXhQsIXaSObmrW6c8DRbtuaFUasKIdWpQRSn8U Sdoj8vBOvuRDDmJ0z6ElWnI2Ck1gz9k9pvaJPa2gkfwgZZPU8lMLzYfpXqqQTB6K L7hSR+UDSSU9lSH3JFPbsAeBwqMXbWkAZKglXt/cLu/yV43pGO7isCZMkXlYjfr6 VGm/iswPEAswPjLl8lEnRRxcfowuQHQc7oux9A9PKzPOPZAvFwZ0P4P/8nK+UGjU rJDNbY4hxFqyaIxA+0iKUyaTb4ImfSOVXskMLl5uPojYWn5wcIYdQL8koE7Fh8Jq HDLvpioaHsMLn8vPu4f+ac5mUyoLPlWUwr/mJAHHZwn6kG4NM5bykZzP4609gLFH lPp13H9Bn4Cdf9a5BeP0UU9rm9kipThAEEfFxU+/EKSzcg7ELUJcXUc1OvcVv6hx vZP9w4CYaj9Y3SnOJ9nq =G+LB -----END PGP SIGNATURE----- --smsj7k2syl2frb2l--