| Message ID | 20170604160149.30230-4-icenowy@aosc.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@aosc.io> 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
于 2017年6月7日 GMT+08:00 下午5:35:12, Maxime Ripard <maxime.ripard@free-electrons.com> 写到: >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@aosc.io> > >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... Yes, from the TCON perspective, the connection from mixer0 to TCON1 and from mixer 1 to TCON0 should be ignored. And from the mixer perspective, the connections should also be omitted when binding in sun4i_drv.c. > >> --- >> 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. Yes. Mixers will have two output endpoints, but we will only use the same id one and ignore the swapped one. So does the situation of TCON input. > > >> 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. No. I said that they're swappable, however, I don't want to implement the swap now, but hardcode 0-0 1-1 connection. However, as you and Chen-Yu said, device tree should reflect the real hardware, there will be bonus endpoints for the swapped connection. 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. > >Maxime
于 2017年6月7日 GMT+08:00 下午5:35:12, Maxime Ripard <maxime.ripard@free-electrons.com> 写到: >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@aosc.io> > >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... Yes, from the TCON perspective, the connection from mixer0 to TCON1 and from mixer 1 to TCON0 should be ignored. And from the mixer perspective, the connections should also be omitted when binding in sun4i_drv.c. > >> --- >> 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. Yes. Mixers will have two output endpoints, but we will only use the same id one and ignore the swapped one. So does the situation of TCON input. > > >> 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. No. I said that they're swappable, however, I don't want to implement the swap now, but hardcode 0-0 1-1 connection. However, as you and Chen-Yu said, device tree should reflect the real hardware, there will be bonus endpoints for the swapped connection. 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. > >Maxime
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. Maxime
Hi! Dne sreda, 07. junij 2017 ob 16:38:27 CEST je Maxime Ripard napisal(a): > 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. If I understand correctly, she wants to make sure that DT has mixer0 - tcon0 and mixer1 - tcon1 relationship and discard mixed relationship. I also believe that this is just temporary measure until mixed relationship is supported by the driver. Maybe we could just leave this out for now and define only one endpoint in DT for TVE instead of two? Vast majority of users will have 0-0 and 1-1 relationship, since there is not much point connecting HDMI with less capable mixer than TVE. Best regards, Jernej
在 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. > > Maxime > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Jernej, On Wed, Jun 07, 2017 at 08:15:12PM +0200, Jernej Škrabec wrote: > Hi! > > Dne sreda, 07. junij 2017 ob 16:38:27 CEST je Maxime Ripard napisal(a): > > 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. > > If I understand correctly, she wants to make sure that DT has mixer0 - tcon0 > and mixer1 - tcon1 relationship and discard mixed relationship. I also believe > that this is just temporary measure until mixed relationship is supported by > the driver. yeah, but all the component stuff doesn't care about the relationships themselves, it just cares about the set of devices, and we use those relationships to build that set. In this case, even with the swapped connections, the set of devices remains the same, which is what puzzles me. > Maybe we could just leave this out for now and define only one endpoint in DT > for TVE instead of two? Vast majority of users will have 0-0 and 1-1 > relationship, since there is not much point connecting HDMI with less capable > mixer than TVE. Yeah, but unfortunately, we have to have a perfect representation from day one now... Maxime
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. Maxime
在 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.) 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
在 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
On Sat, Jun 10, 2017 at 10:57:28PM +0800, icenowy@aosc.io wrote: > 在 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.) > > 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. That's not true. DE1's can output to several TCONs (or rather, TCONs can select multiple engines as their input). The A31 for example is in this case. I think what Chen-Yu did so far is that he only enables one engine and TCON for now. That will leave us some time to rework and improve things gradually. It would probably be easier (and faster) for you too. > 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. So this is the ordering issue I was mentionning earlier. The way to fix this is to use BFS instead of DFS when building the component list. > 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). I'm not sure what you mean by that. Isn't the tcon1 a real output for mixer0? > 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. This isn't really about whether you follow connections or not, in both cases you do. It's how you follow those connections that matters, and it does make sense to follow them stage by stage in our pipeline. ie. something that would bind all the engines, then all the TCONs, then all the encoders. All of them would have connections between them. > 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. Short term view: we shouldn't be in that case in the first place. Long term view: there's no reason it shouldn't work. Maxime
On Sat, Jun 10, 2017 at 11:16:35PM +0800, icenowy@aosc.io wrote: > 在 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. As a short-term workaround, while we rework the code to enable both pipelines, it definitely works for me. Maxim
On Tue, Jun 13, 2017 at 5:43 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Sat, Jun 10, 2017 at 10:57:28PM +0800, icenowy@aosc.io wrote: >> 在 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.) >> >> 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. Then you should fix it so it works with DE2. For DE2 it would be something like this: Mixer probe ----------- You have two choices on how to get its ID: a) Look downstream to TCONs, and get the reg value from the remote endpoint, or b) get the display-engine node, and check the index of the phandle that points to itself (the mixer). A is easier, and it is why I asked you to follow the connection ID rule in the device tree binding. TCON probe ---------- Similar to A) for the mixers, look upstream and get the reg value from the remote endpoint. Afterwards you would have correct IDs on both sides, then it's just a matter of going through the list of mixers and TCONs and pairing them up. Mixer0 0 ---- 0 TCON0 1 1 <- This would be the remote endpoint reg value for mixer1. \ / \ / \/ /\ / \ / \ 0 0 <- This would be the remote endpoint reg value for mixer0. Mixer1 1 ---- 1 TCON1 Hope this makes things clearer. > > That's not true. DE1's can output to several TCONs (or rather, TCONs > can select multiple engines as their input). The A31 for example is in > this case. Actually that's not true. The TCON is bound to the backend. I don't see any controls for muxing that. So the TCON-backend search routine is very simple for DE1. The frontends are free to feed either backend though. > I think what Chen-Yu did so far is that he only enables one engine and > TCON for now. That will leave us some time to rework and improve > things gradually. It would probably be easier (and faster) for you too. My code deals with muxes between the TCON and the downstream encoders (HDMI/TV). You need something similar to deal with the mux between the mixers and TCONs. My code doesn't fit DE2, because the case hadn't been added yet. >> 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. > > So this is the ordering issue I was mentionning earlier. The way to > fix this is to use BFS instead of DFS when building the component > list. > >> 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). > > I'm not sure what you mean by that. Isn't the tcon1 a real output for > mixer0? > >> 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. > > This isn't really about whether you follow connections or not, in both > cases you do. It's how you follow those connections that matters, and > it does make sense to follow them stage by stage in our > pipeline. ie. something that would bind all the engines, then all the > TCONs, then all the encoders. > > All of them would have connections between them. > >> 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. > > Short term view: we shouldn't be in that case in the first place. > Long term view: there's no reason it shouldn't work. Maybe I missed something? TCONs and everything before them should always be enabled. There's no reason not to. This is especially true for TCON0 which holds the mux register on some SoCs. About Maxime's long term view: there's no reason we can't just silently ignore a component if its supposed companion is missing, like a TCON missing its backend, or the other way around. The current code already covers the latter case: since the CRTC (and by extension, all DRM/KMS functionality related to that pipeline) is created in the TCON bind function, if the TCON is not enabled, the backend just idles. ChenYu
Hi, On Tue, Jun 13, 2017 at 06:05:57PM +0800, Chen-Yu Tsai wrote: > > That's not true. DE1's can output to several TCONs (or rather, TCONs > > can select multiple engines as their input). The A31 for example is in > > this case. > > Actually that's not true. The TCON is bound to the backend. I don't see > any controls for muxing that. So the TCON-backend search routine is very > simple for DE1. The frontends are free to feed either backend though. I think it does, look at the lower bits of TCON0_CTL_REG and TCON1_CTL_REG in the A31 datasheet. It clearly seems used to control from which DE you fetch your data from, and you have both options. > >> 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. > > > > Short term view: we shouldn't be in that case in the first place. > > Long term view: there's no reason it shouldn't work. > > Maybe I missed something? TCONs and everything before them should always > be enabled. There's no reason not to. This is especially true for TCON0 > which holds the mux register on some SoCs. If we're not able to use anything connected to TCON1, disabling it still seems like a good stop-gap measure. > About Maxime's long term view: there's no reason we can't just silently > ignore a component if its supposed companion is missing, like a TCON > missing its backend, or the other way around. It's a bit more complicated than that. TCON0 is probably mandatory, and even if TCON1 is missing, we can entirely route around it in hardware, and I think it's the case for all the stages in our pipelines. There's no reason we could not operate that way. But this is clearly something that we should aim for, not something that needs to be done today. Maxime
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); 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); + } + /* does this node match any registered engines? */ list_for_each_entry(engine, &drv->engine_list, list) { if (remote == engine->node) { @@ -488,7 +525,7 @@ static struct sunxi_engine *sun4i_tcon_find_engine(struct sun4i_drv *drv, } /* keep looking through upstream ports */ - engine = sun4i_tcon_find_engine(drv, remote); + engine = sun4i_tcon_find_engine(drv, remote, skip_bonus_ep); if (!IS_ERR(engine)) { of_node_put(remote); of_node_put(port); @@ -508,21 +545,27 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master, struct sun4i_tcon *tcon; int ret; - engine = sun4i_tcon_find_engine(drv, dev->of_node); - if (IS_ERR(engine)) { - dev_err(dev, "Couldn't find matching engine\n"); - return -EPROBE_DEFER; - } - tcon = devm_kzalloc(dev, sizeof(*tcon), GFP_KERNEL); if (!tcon) return -ENOMEM; dev_set_drvdata(dev, tcon); tcon->drm = drm; tcon->dev = dev; - tcon->id = engine->id; tcon->quirks = of_device_get_match_data(dev); + /* + * As we keep the connection between DE2 mixer and TCON not swapped, + * skip the bonus endpoints (which stand for swapped connection) + * when finding the correspoing engine. + */ + engine = sun4i_tcon_find_engine(drv, dev->of_node, + tcon->quirks->swappable_input); + if (IS_ERR(engine)) { + dev_err(dev, "Couldn't find matching engine\n"); + return -EPROBE_DEFER; + } + tcon->id = engine->id; + tcon->lcd_rst = devm_reset_control_get(dev, "lcd"); if (IS_ERR(tcon->lcd_rst)) { dev_err(dev, "Couldn't get our reset line\n"); diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h index e3c50ecdcd04..c3e01c06e9a0 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h @@ -146,6 +146,8 @@ struct sun4i_tcon_quirks { bool has_unknown_mux; /* sun5i has undocumented mux */ bool has_channel_1; /* a33 does not have channel 1 */ + /* Some DE2 can swap the mixer<->TCON connection */ + bool swappable_input; }; struct sun4i_tcon {
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@aosc.io> --- 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(-)