Re: [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()
Hi,
On Wed, Apr 14, 2021 at 6:56 PM Laurent Pinchart
wrote:
>
> Hi Doug,
>
> On Wed, Apr 14, 2021 at 06:19:13PM -0700, Doug Anderson wrote:
> > On Sun, Apr 4, 2021 at 5:50 PM Laurent Pinchart wrote:
> > > On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
> > > > The drm_bridge_chain_pre_enable() is not the proper opposite of
> > > > drm_bridge_chain_post_disable(). It continues along the chain to
> > > > _before_ the starting bridge. Let's fix that.
> > > >
> > > > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked
> > > > list")
> > > > Signed-off-by: Douglas Anderson
> > > > Reviewed-by: Andrzej Hajda
> > > > ---
> > > >
> > > > (no changes since v1)
> > > >
> > > > drivers/gpu/drm/drm_bridge.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > > index 64f0effb52ac..044acd07c153 100644
> > > > --- a/drivers/gpu/drm/drm_bridge.c
> > > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > > @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge
> > > > *bridge)
> > > > list_for_each_entry_reverse(iter, >bridge_chain,
> > > > chain_node) {
> > > > if (iter->funcs->pre_enable)
> > > > iter->funcs->pre_enable(iter);
> > > > +
> > > > + if (iter == bridge)
> > > > + break;
> > >
> > > This looks good as it matches drm_atomic_bridge_chain_disable().
> > >
> > > Reviewed-by: Laurent Pinchart
> >
> > Thanks for your review here and several of the other patches. Can you
> > suggest any plan for getting them landed? It would at least be nice to
> > get the non-controversial ones landed.
>
> Do you have commit access to drm-misc ? If not, given your
> contributions, I think you qualify for it.
No, I don't have access. I searched for how to get it and read through
the qualifications and, you're right, I think I do. I've hopefully
followed the right flow and created an issue to give me ssh access:
https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/348
Is that something you (or someone else on this CC list) approves?
> > > I'm curious though, given that the bridge passed to the function should
> > > be the one closest to the encoder, does this make a difference ?
> >
> > Yes, that's how I discovered it originally. Let's see. So if I don't
> > have this patch but have the rest of the series then I get a splat at
> > bootup. This shows that dsi_mgr_bridge_pre_enable() must be "earlier"
> > in the chain than my bridge chip. Here's the splat:
>
> Right, I think it's caused by a later patch in the series calling this
> function with a different bridge than the one closest to the encoder.
Yup! I still wanted this patch to be first in the series, though,
since it's a bugfix that we'd want to land even if the later patches
changed in some way.
-Doug
Re: [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()
Hi Doug,
On Wed, Apr 14, 2021 at 06:19:13PM -0700, Doug Anderson wrote:
> On Sun, Apr 4, 2021 at 5:50 PM Laurent Pinchart wrote:
> > On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
> > > The drm_bridge_chain_pre_enable() is not the proper opposite of
> > > drm_bridge_chain_post_disable(). It continues along the chain to
> > > _before_ the starting bridge. Let's fix that.
> > >
> > > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked
> > > list")
> > > Signed-off-by: Douglas Anderson
> > > Reviewed-by: Andrzej Hajda
> > > ---
> > >
> > > (no changes since v1)
> > >
> > > drivers/gpu/drm/drm_bridge.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index 64f0effb52ac..044acd07c153 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge
> > > *bridge)
> > > list_for_each_entry_reverse(iter, >bridge_chain,
> > > chain_node) {
> > > if (iter->funcs->pre_enable)
> > > iter->funcs->pre_enable(iter);
> > > +
> > > + if (iter == bridge)
> > > + break;
> >
> > This looks good as it matches drm_atomic_bridge_chain_disable().
> >
> > Reviewed-by: Laurent Pinchart
>
> Thanks for your review here and several of the other patches. Can you
> suggest any plan for getting them landed? It would at least be nice to
> get the non-controversial ones landed.
Do you have commit access to drm-misc ? If not, given your
contributions, I think you qualify for it.
> > I'm curious though, given that the bridge passed to the function should
> > be the one closest to the encoder, does this make a difference ?
>
> Yes, that's how I discovered it originally. Let's see. So if I don't
> have this patch but have the rest of the series then I get a splat at
> bootup. This shows that dsi_mgr_bridge_pre_enable() must be "earlier"
> in the chain than my bridge chip. Here's the splat:
Right, I think it's caused by a later patch in the series calling this
function with a different bridge than the one closest to the encoder.
> msm_dsi_host_get_phy_clk_req: unable to calc clk rate, -22
> [ cut here ]
> disp_cc_mdss_ahb_clk status stuck at 'off'
> WARNING: CPU: 7 PID: 404 at drivers/clk/qcom/clk-branch.c:92
> clk_branch_toggle+0x194/0x280
> Modules linked in: joydev
> CPU: 7 PID: 404 Comm: frecon Tainted: GB 5.12.0-rc3-lockdep+
> #2
> Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
> pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
> pc : clk_branch_toggle+0x194/0x280
> lr : clk_branch_toggle+0x190/0x280
> ...
> Call trace:
> clk_branch_toggle+0x194/0x280
> clk_branch2_enable+0x28/0x34
> clk_core_enable+0x2f4/0x6b4
> clk_enable+0x54/0x74
> dsi_phy_enable_resource+0x80/0xd8
> msm_dsi_phy_enable+0xa8/0x4a8
> enable_phy+0x9c/0xf4
> dsi_mgr_bridge_pre_enable+0x23c/0x4b0
> drm_bridge_chain_pre_enable+0xac/0xe4
> ti_sn_bridge_connector_get_modes+0x134/0x1b8
> drm_helper_probe_single_connector_modes+0x49c/0x1358
> drm_mode_getconnector+0x460/0xe98
> drm_ioctl_kernel+0x144/0x228
> drm_ioctl+0x418/0x7cc
> drm_compat_ioctl+0x1bc/0x230
> __arm64_compat_sys_ioctl+0x14c/0x188
> el0_svc_common+0x128/0x23c
> do_el0_svc_compat+0x50/0x60
> el0_svc_compat+0x24/0x34
> el0_sync_compat_handler+0xc0/0xf0
> el0_sync_compat+0x174/0x180
--
Regards,
Laurent Pinchart
Re: [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()
Hi,
On Sun, Apr 4, 2021 at 5:50 PM Laurent Pinchart
wrote:
>
> Hi Doug,
>
> Thank you for the patch.
>
> On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
> > The drm_bridge_chain_pre_enable() is not the proper opposite of
> > drm_bridge_chain_post_disable(). It continues along the chain to
> > _before_ the starting bridge. Let's fix that.
> >
> > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked
> > list")
> > Signed-off-by: Douglas Anderson
> > Reviewed-by: Andrzej Hajda
> > ---
> >
> > (no changes since v1)
> >
> > drivers/gpu/drm/drm_bridge.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 64f0effb52ac..044acd07c153 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge
> > *bridge)
> > list_for_each_entry_reverse(iter, >bridge_chain, chain_node)
> > {
> > if (iter->funcs->pre_enable)
> > iter->funcs->pre_enable(iter);
> > +
> > + if (iter == bridge)
> > + break;
>
> This looks good as it matches drm_atomic_bridge_chain_disable().
>
> Reviewed-by: Laurent Pinchart
Thanks for your review here and several of the other patches. Can you
suggest any plan for getting them landed? It would at least be nice to
get the non-controversial ones landed.
> I'm curious though, given that the bridge passed to the function should
> be the one closest to the encoder, does this make a difference ?
Yes, that's how I discovered it originally. Let's see. So if I don't
have this patch but have the rest of the series then I get a splat at
bootup. This shows that dsi_mgr_bridge_pre_enable() must be "earlier"
in the chain than my bridge chip. Here's the splat:
msm_dsi_host_get_phy_clk_req: unable to calc clk rate, -22
[ cut here ]
disp_cc_mdss_ahb_clk status stuck at 'off'
WARNING: CPU: 7 PID: 404 at drivers/clk/qcom/clk-branch.c:92
clk_branch_toggle+0x194/0x280
Modules linked in: joydev
CPU: 7 PID: 404 Comm: frecon Tainted: GB 5.12.0-rc3-lockdep+ #2
Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
pc : clk_branch_toggle+0x194/0x280
lr : clk_branch_toggle+0x190/0x280
...
Call trace:
clk_branch_toggle+0x194/0x280
clk_branch2_enable+0x28/0x34
clk_core_enable+0x2f4/0x6b4
clk_enable+0x54/0x74
dsi_phy_enable_resource+0x80/0xd8
msm_dsi_phy_enable+0xa8/0x4a8
enable_phy+0x9c/0xf4
dsi_mgr_bridge_pre_enable+0x23c/0x4b0
drm_bridge_chain_pre_enable+0xac/0xe4
ti_sn_bridge_connector_get_modes+0x134/0x1b8
drm_helper_probe_single_connector_modes+0x49c/0x1358
drm_mode_getconnector+0x460/0xe98
drm_ioctl_kernel+0x144/0x228
drm_ioctl+0x418/0x7cc
drm_compat_ioctl+0x1bc/0x230
__arm64_compat_sys_ioctl+0x14c/0x188
el0_svc_common+0x128/0x23c
do_el0_svc_compat+0x50/0x60
el0_svc_compat+0x24/0x34
el0_sync_compat_handler+0xc0/0xf0
el0_sync_compat+0x174/0x180
Re: [PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()
Hi Doug,
Thank you for the patch.
On Fri, Apr 02, 2021 at 03:28:35PM -0700, Douglas Anderson wrote:
> The drm_bridge_chain_pre_enable() is not the proper opposite of
> drm_bridge_chain_post_disable(). It continues along the chain to
> _before_ the starting bridge. Let's fix that.
>
> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> Signed-off-by: Douglas Anderson
> Reviewed-by: Andrzej Hajda
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/drm_bridge.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 64f0effb52ac..044acd07c153 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge
> *bridge)
> list_for_each_entry_reverse(iter, >bridge_chain, chain_node) {
> if (iter->funcs->pre_enable)
> iter->funcs->pre_enable(iter);
> +
> + if (iter == bridge)
> + break;
This looks good as it matches drm_atomic_bridge_chain_disable().
Reviewed-by: Laurent Pinchart
I'm curious though, given that the bridge passed to the function should
be the one closest to the encoder, does this make a difference ?
> }
> }
> EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
--
Regards,
Laurent Pinchart
[PATCH v3 01/12] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()
The drm_bridge_chain_pre_enable() is not the proper opposite of
drm_bridge_chain_post_disable(). It continues along the chain to
_before_ the starting bridge. Let's fix that.
Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
Signed-off-by: Douglas Anderson
Reviewed-by: Andrzej Hajda
---
(no changes since v1)
drivers/gpu/drm/drm_bridge.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 64f0effb52ac..044acd07c153 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
list_for_each_entry_reverse(iter, >bridge_chain, chain_node) {
if (iter->funcs->pre_enable)
iter->funcs->pre_enable(iter);
+
+ if (iter == bridge)
+ break;
}
}
EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
--
2.31.0.208.g409f899ff0-goog
