Re: [PATCH v2 03/11] drm: sun4i: ignore swapped mixer<->tcon connection for DE2

[Maxime Ripard]

On Mon, Jun 05, 2017 at 12:01:41AM +0800, Icenowy Zheng wrote:
> Some SoC's DE2 has two mixers. Defaultly the mixer0 is connected to
> tcon0 and mixer1 is connected to tcon1; however by setting a bit
> the connection can be swapped.
> 
> As we now hardcode the default connection, ignore the bonus endpoint for
> the mixer's output and the TCON's input, as they stands for the swapped
> connection.
> 
> Signed-off-by: Icenowy Zheng <icenowy@xxxxxxx>

So, I'm still not quite sure what this patch exactly is supposed to be
doing.

You mention that the routing between the mixers and tcons can be
changed, and that we need to ignore the TCON input...

> ---
> Changes in v2:
> - Change to use new endpoint reg definition.
> 
>  drivers/gpu/drm/sun4i/sun4i_drv.c  | 45 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 61 ++++++++++++++++++++++++++++++++------
>  drivers/gpu/drm/sun4i/sun4i_tcon.h |  2 ++
>  3 files changed, 99 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
> index f19100c91c2b..775eee82d8a9 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
> @@ -178,6 +178,13 @@ static bool sun4i_drv_node_is_frontend(struct device_node *node)
>  		of_device_is_compatible(node, "allwinner,sun8i-a33-display-frontend");
>  }
>  
> +static bool sun4i_drv_node_is_swappable_de2_mixer(struct device_node *node)
> +{
> +	/* The V3s has only one mixer-tcon pair, so it's not listed here. */
> +	return of_device_is_compatible(node, "allwinner,sun8i-h3-de2-mixer0") ||
> +		of_device_is_compatible(node, "allwinner,sun8i-h3-de2-mixer1");
> +}
> +
>  static bool sun4i_drv_node_is_tcon(struct device_node *node)
>  {
>  	return of_device_is_compatible(node, "allwinner,sun5i-a13-tcon") ||
> @@ -261,6 +268,44 @@ static int sun4i_drv_add_endpoints(struct device *dev,
>  			}
>  		}
>  
> +		/*
> +		 * The second endpoint of the output of a swappable DE2 mixer
> +		 * is the TCON after connection swapping.
> +		 * Ignore it now, as we now hardcode mixer0->tcon0,
> +		 * mixer1->tcon1 connection.
> +		 */
> +		if (sun4i_drv_node_is_swappable_de2_mixer(node)) {
> +			struct device_node *remote_ep_node;
> +			struct of_endpoint local_endpoint, remote_endpoint;
> +
> +			remote_ep_node = of_graph_get_remote_endpoint(ep);
> +			if (!remote_ep_node) {
> +				DRM_DEBUG_DRIVER("Couldn't get remote endpoint\n");
> +				continue;
> +			}
> +
> +			if (of_graph_parse_endpoint(ep, &local_endpoint)) {
> +				DRM_DEBUG_DRIVER("Couldn't parse local endpoint\n");
> +				of_node_put(remote_ep_node);
> +				continue;
> +			}
> +
> +			if (of_graph_parse_endpoint(remote_ep_node,
> +						    &remote_endpoint)) {
> +				DRM_DEBUG_DRIVER("Couldn't parse remote endpoint\n");
> +				of_node_put(remote_ep_node);
> +				continue;
> +			}
> +
> +			if (local_endpoint.id != remote_endpoint.id) {
> +				DRM_DEBUG_DRIVER("Endpoint is an unused connection for DE2 mixer... skipping\n");
> +				of_node_put(remote_ep_node);
> +				continue;
> +			}
> +
> +			of_node_put(remote_ep_node);
> +		}
> +
>  		/* Walk down our tree */
>  		count += sun4i_drv_add_endpoints(dev, match, remote);

... yet this is not parsing the input at all, but only the output nodes.


> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index d9791292553e..568cea0e5f8f 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -464,7 +464,8 @@ static int sun4i_tcon_init_regmap(struct device *dev,
>   * requested via the get_id function of the engine.
>   */
>  static struct sunxi_engine *sun4i_tcon_find_engine(struct sun4i_drv *drv,
> -						   struct device_node *node)
> +						   struct device_node *node,
> +						   bool skip_bonus_ep)
>  {
>  	struct device_node *port, *ep, *remote;
>  	struct sunxi_engine *engine;
> @@ -478,6 +479,42 @@ static struct sunxi_engine *sun4i_tcon_find_engine(struct sun4i_drv *drv,
>  		if (!remote)
>  			continue;
>  
> +		if (skip_bonus_ep) {
> +			struct device_node *remote_ep_node;
> +			struct of_endpoint local_endpoint, remote_endpoint;
> +
> +			remote_ep_node = of_graph_get_remote_endpoint(ep);
> +			if (!remote_ep_node) {
> +				DRM_DEBUG_DRIVER("Couldn't get remote endpoint\n");
> +				of_node_put(remote);
> +				continue;
> +			}
> +
> +			if (of_graph_parse_endpoint(ep, &local_endpoint)) {
> +				DRM_DEBUG_DRIVER("Couldn't parse local endpoint\n");
> +				of_node_put(remote);
> +				of_node_put(remote_ep_node);
> +				continue;
> +			}
> +
> +			if (of_graph_parse_endpoint(remote_ep_node,
> +						    &remote_endpoint)) {
> +				DRM_DEBUG_DRIVER("Couldn't parse remote endpoint\n");
> +				of_node_put(remote);
> +				of_node_put(remote_ep_node);
> +				continue;
> +			}
> +
> +			if (local_endpoint.id != remote_endpoint.id) {
> +				DRM_DEBUG_DRIVER("Skipping bonus mixer->TCON connection when searching engine\n");
> +				of_node_put(remote);
> +				of_node_put(remote_ep_node);
> +				continue;
> +			}
> +
> +			of_node_put(remote_ep_node);
> +		}
> +

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.

Maxime

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

Attachment: signature.asc
Description: PGP signature