| Message ID | 20230131141756.RFT.v2.3.I3c87b53c4ab61a7d5e05f601a4eb44c7e3809a01@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFT,v2,1/3] drm/bridge: tc358762: Set pre_enable_prev_first | expand |
Hi Doug On 1/31/2023 2:18 PM, Douglas Anderson wrote: > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset > time") the error handling with regards to dsi_mgr_bridge_power_on() > got a bit worse. Specifically if we failed to power the bridge on then > nothing would really notice. The modeset function couldn't return an > error and thus we'd blindly go forward and try to do the pre-enable. > > In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time > for parade-ps8640") we added a special case to move the powerup back > to pre-enable time for ps8640. When we did that, we didn't try to > recover the old/better error handling just for ps8640. > > In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts > at modeset") we've now moved the powering up back to exclusively being > during pre-enable. That means we can add the better error handling > back in, so let's do it. To do so we'll add a new function > dsi_mgr_bridge_power_off() that's matches how errors were handled > prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to > modeset time"). > > NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we > should be calling it in dsi_mgr_bridge_post_disable(). That would make > some sense, but doing so would change the current behavior and thus > should be a separate patch. Specifically: > * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable() > even in the slave-DSI case of bonded DSI. We'd need to add special > handling for this if it's truly needed. > * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state() > midway through the poweroff. > * dsi_mgr_bridge_post_disable() has a different order of some of the > poweroffs / IRQ disables. > For now we'll leave dsi_mgr_bridge_post_disable() alone. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v2: > - ("More properly handle errors...") new for v2. > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++----- > 1 file changed, 26 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > index 2197a54b9b96..28b8012a21f2 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id) > } > } > > -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > { > int id = dsi_mgr_bridge_get_id(bridge); > struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); > @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > if (is_bonded_dsi && msm_dsi1) > msm_dsi_host_enable_irq(msm_dsi1->host); > > - return; > + return 0; > > host1_on_fail: > msm_dsi_host_power_off(host); > host_on_fail: > dsi_mgr_phy_disable(id); > phy_en_fail: > - return; > + return ret; > +} > + > +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge) > +{ > + int id = dsi_mgr_bridge_get_id(bridge); > + struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); > + struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); > + struct mipi_dsi_host *host = msm_dsi->host; > + bool is_bonded_dsi = IS_BONDED_DSI(); > + > + msm_dsi_host_disable_irq(host); > + if (is_bonded_dsi && msm_dsi1) { > + msm_dsi_host_disable_irq(msm_dsi1->host); > + msm_dsi_host_power_off(msm_dsi1->host); > + } The order of disabling the IRQs should be opposite of how they were enabled. So while enabling it was DSI0 and then DSI1. Hence while disabling it should be DSI1 and then DSI0. So the order here should be DSI1 irq disable DSI0 irq disable DSI1 host power off DSI0 host power off > + msm_dsi_host_power_off(host); > + dsi_mgr_phy_disable(id); > } > > static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > @@ -295,7 +312,11 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) > return; > > - dsi_mgr_bridge_power_on(bridge); > + ret = dsi_mgr_bridge_power_on(bridge); > + if (ret) { > + dev_err(&msm_dsi->pdev->dev, "Power on failed: %d\n", ret); > + return; > + } > > ret = msm_dsi_host_enable(host); > if (ret) { > @@ -316,8 +337,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > host1_en_fail: > msm_dsi_host_disable(host); > host_en_fail: > - > - return; > + dsi_mgr_bridge_power_off(bridge); > } > > void msm_dsi_manager_tpg_enable(void)
Hi, On Thu, Feb 2, 2023 at 2:37 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > Hi Doug > > On 1/31/2023 2:18 PM, Douglas Anderson wrote: > > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset > > time") the error handling with regards to dsi_mgr_bridge_power_on() > > got a bit worse. Specifically if we failed to power the bridge on then > > nothing would really notice. The modeset function couldn't return an > > error and thus we'd blindly go forward and try to do the pre-enable. > > > > In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time > > for parade-ps8640") we added a special case to move the powerup back > > to pre-enable time for ps8640. When we did that, we didn't try to > > recover the old/better error handling just for ps8640. > > > > In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts > > at modeset") we've now moved the powering up back to exclusively being > > during pre-enable. That means we can add the better error handling > > back in, so let's do it. To do so we'll add a new function > > dsi_mgr_bridge_power_off() that's matches how errors were handled > > prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to > > modeset time"). > > > > NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we > > should be calling it in dsi_mgr_bridge_post_disable(). That would make > > some sense, but doing so would change the current behavior and thus > > should be a separate patch. Specifically: > > * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable() > > even in the slave-DSI case of bonded DSI. We'd need to add special > > handling for this if it's truly needed. > > * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state() > > midway through the poweroff. > > * dsi_mgr_bridge_post_disable() has a different order of some of the > > poweroffs / IRQ disables. > > For now we'll leave dsi_mgr_bridge_post_disable() alone. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > Changes in v2: > > - ("More properly handle errors...") new for v2. > > > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++----- > > 1 file changed, 26 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > > index 2197a54b9b96..28b8012a21f2 100644 > > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > > @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id) > > } > > } > > > > -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > > +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > > { > > int id = dsi_mgr_bridge_get_id(bridge); > > struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); > > @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > > if (is_bonded_dsi && msm_dsi1) > > msm_dsi_host_enable_irq(msm_dsi1->host); > > > > - return; > > + return 0; > > > > host1_on_fail: > > msm_dsi_host_power_off(host); > > host_on_fail: > > dsi_mgr_phy_disable(id); > > phy_en_fail: > > - return; > > + return ret; > > +} > > + > > +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge) > > +{ > > + int id = dsi_mgr_bridge_get_id(bridge); > > + struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); > > + struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); > > + struct mipi_dsi_host *host = msm_dsi->host; > > + bool is_bonded_dsi = IS_BONDED_DSI(); > > + > > + msm_dsi_host_disable_irq(host); > > + if (is_bonded_dsi && msm_dsi1) { > > + msm_dsi_host_disable_irq(msm_dsi1->host); > > + msm_dsi_host_power_off(msm_dsi1->host); > > + } > > The order of disabling the IRQs should be opposite of how they were enabled. > > So while enabling it was DSI0 and then DSI1. > > Hence while disabling it should be DSI1 and then DSI0. > > So the order here should be > > DSI1 irq disable > DSI0 irq disable > DSI1 host power off > DSI0 host power off Right. Normally you want to go opposite. I guess a few points, though: 1. As talked about in the commit message, the order I have matches the order we had prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time"). 2. I'd be curious if it matters. The order you request means we need to check for `(is_bonded_dsi && msm_dsi1)` twice. While that's not a big deal if it's important, it's nice not to have to do so. 3. As talked about in the commit message, eventually we should probably resolve this order with the order of things in dsi_mgr_bridge_post_disable(), which is yet a different ordering. Ideally this resolution would be done by someone who actually has proper documentation of the hardware and how it's supposed to work (AKA not me). So my preference would be to either land or drop ${SUBJECT} patch (either is fine with me) and then someone at Qualcomm could then take over further cleanup. -Doug
Hi Doug Sorry for the delayed response. On 2/2/2023 2:46 PM, Doug Anderson wrote: > Hi, > > On Thu, Feb 2, 2023 at 2:37 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> Hi Doug >> >> On 1/31/2023 2:18 PM, Douglas Anderson wrote: >>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset >>> time") the error handling with regards to dsi_mgr_bridge_power_on() >>> got a bit worse. Specifically if we failed to power the bridge on then >>> nothing would really notice. The modeset function couldn't return an >>> error and thus we'd blindly go forward and try to do the pre-enable. >>> >>> In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time >>> for parade-ps8640") we added a special case to move the powerup back >>> to pre-enable time for ps8640. When we did that, we didn't try to >>> recover the old/better error handling just for ps8640. >>> >>> In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts >>> at modeset") we've now moved the powering up back to exclusively being >>> during pre-enable. That means we can add the better error handling >>> back in, so let's do it. To do so we'll add a new function >>> dsi_mgr_bridge_power_off() that's matches how errors were handled >>> prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to >>> modeset time"). >>> >>> NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we >>> should be calling it in dsi_mgr_bridge_post_disable(). That would make >>> some sense, but doing so would change the current behavior and thus >>> should be a separate patch. Specifically: >>> * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable() >>> even in the slave-DSI case of bonded DSI. We'd need to add special >>> handling for this if it's truly needed. >>> * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state() >>> midway through the poweroff. >>> * dsi_mgr_bridge_post_disable() has a different order of some of the >>> poweroffs / IRQ disables. >>> For now we'll leave dsi_mgr_bridge_post_disable() alone. >>> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>> --- >>> >>> Changes in v2: >>> - ("More properly handle errors...") new for v2. >>> >>> drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++----- >>> 1 file changed, 26 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c >>> index 2197a54b9b96..28b8012a21f2 100644 >>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c >>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c >>> @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id) >>> } >>> } >>> >>> -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) >>> +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) >>> { >>> int id = dsi_mgr_bridge_get_id(bridge); >>> struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); >>> @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) >>> if (is_bonded_dsi && msm_dsi1) >>> msm_dsi_host_enable_irq(msm_dsi1->host); >>> >>> - return; >>> + return 0; >>> >>> host1_on_fail: >>> msm_dsi_host_power_off(host); >>> host_on_fail: >>> dsi_mgr_phy_disable(id); >>> phy_en_fail: >>> - return; >>> + return ret; >>> +} >>> + >>> +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge) >>> +{ >>> + int id = dsi_mgr_bridge_get_id(bridge); >>> + struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); >>> + struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); >>> + struct mipi_dsi_host *host = msm_dsi->host; >>> + bool is_bonded_dsi = IS_BONDED_DSI(); >>> + >>> + msm_dsi_host_disable_irq(host); >>> + if (is_bonded_dsi && msm_dsi1) { >>> + msm_dsi_host_disable_irq(msm_dsi1->host); >>> + msm_dsi_host_power_off(msm_dsi1->host); >>> + } >> >> The order of disabling the IRQs should be opposite of how they were enabled. >> >> So while enabling it was DSI0 and then DSI1. >> >> Hence while disabling it should be DSI1 and then DSI0. >> >> So the order here should be >> >> DSI1 irq disable >> DSI0 irq disable >> DSI1 host power off >> DSI0 host power off > > Right. Normally you want to go opposite. I guess a few points, though: > > 1. As talked about in the commit message, the order I have matches the > order we had prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host > powerup to modeset time"). > > 2. I'd be curious if it matters. The order you request means we need > to check for `(is_bonded_dsi && msm_dsi1)` twice. While that's not a > big deal if it's important, it's nice not to have to do so. > > 3. As talked about in the commit message, eventually we should > probably resolve this order with the order of things in > dsi_mgr_bridge_post_disable(), which is yet a different ordering. > Ideally this resolution would be done by someone who actually has > proper documentation of the hardware and how it's supposed to work > (AKA not me). > > So my preference would be to either land or drop ${SUBJECT} patch > (either is fine with me) and then someone at Qualcomm could then take > over further cleanup. > I do think the ordering matters but you are right, this change brings back the ordering we had before so lets handle the re-ordering of all places in a separate change. I am okay with this change to go-in, hence Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> What is the plan to land the patches? 2 & 3 go in msm-next but 1 goes in drm-misc? Thanks Abhinav > -Doug
Hi, On Mon, Feb 13, 2023 at 6:02 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > Hi Doug > > Sorry for the delayed response. > > On 2/2/2023 2:46 PM, Doug Anderson wrote: > > Hi, > > > > On Thu, Feb 2, 2023 at 2:37 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> Hi Doug > >> > >> On 1/31/2023 2:18 PM, Douglas Anderson wrote: > >>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset > >>> time") the error handling with regards to dsi_mgr_bridge_power_on() > >>> got a bit worse. Specifically if we failed to power the bridge on then > >>> nothing would really notice. The modeset function couldn't return an > >>> error and thus we'd blindly go forward and try to do the pre-enable. > >>> > >>> In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time > >>> for parade-ps8640") we added a special case to move the powerup back > >>> to pre-enable time for ps8640. When we did that, we didn't try to > >>> recover the old/better error handling just for ps8640. > >>> > >>> In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts > >>> at modeset") we've now moved the powering up back to exclusively being > >>> during pre-enable. That means we can add the better error handling > >>> back in, so let's do it. To do so we'll add a new function > >>> dsi_mgr_bridge_power_off() that's matches how errors were handled > >>> prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to > >>> modeset time"). > >>> > >>> NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we > >>> should be calling it in dsi_mgr_bridge_post_disable(). That would make > >>> some sense, but doing so would change the current behavior and thus > >>> should be a separate patch. Specifically: > >>> * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable() > >>> even in the slave-DSI case of bonded DSI. We'd need to add special > >>> handling for this if it's truly needed. > >>> * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state() > >>> midway through the poweroff. > >>> * dsi_mgr_bridge_post_disable() has a different order of some of the > >>> poweroffs / IRQ disables. > >>> For now we'll leave dsi_mgr_bridge_post_disable() alone. > >>> > >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> > >>> --- > >>> > >>> Changes in v2: > >>> - ("More properly handle errors...") new for v2. > >>> > >>> drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++----- > >>> 1 file changed, 26 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > >>> index 2197a54b9b96..28b8012a21f2 100644 > >>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > >>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > >>> @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id) > >>> } > >>> } > >>> > >>> -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > >>> +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > >>> { > >>> int id = dsi_mgr_bridge_get_id(bridge); > >>> struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); > >>> @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) > >>> if (is_bonded_dsi && msm_dsi1) > >>> msm_dsi_host_enable_irq(msm_dsi1->host); > >>> > >>> - return; > >>> + return 0; > >>> > >>> host1_on_fail: > >>> msm_dsi_host_power_off(host); > >>> host_on_fail: > >>> dsi_mgr_phy_disable(id); > >>> phy_en_fail: > >>> - return; > >>> + return ret; > >>> +} > >>> + > >>> +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge) > >>> +{ > >>> + int id = dsi_mgr_bridge_get_id(bridge); > >>> + struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); > >>> + struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); > >>> + struct mipi_dsi_host *host = msm_dsi->host; > >>> + bool is_bonded_dsi = IS_BONDED_DSI(); > >>> + > >>> + msm_dsi_host_disable_irq(host); > >>> + if (is_bonded_dsi && msm_dsi1) { > >>> + msm_dsi_host_disable_irq(msm_dsi1->host); > >>> + msm_dsi_host_power_off(msm_dsi1->host); > >>> + } > >> > >> The order of disabling the IRQs should be opposite of how they were enabled. > >> > >> So while enabling it was DSI0 and then DSI1. > >> > >> Hence while disabling it should be DSI1 and then DSI0. > >> > >> So the order here should be > >> > >> DSI1 irq disable > >> DSI0 irq disable > >> DSI1 host power off > >> DSI0 host power off > > > > Right. Normally you want to go opposite. I guess a few points, though: > > > > 1. As talked about in the commit message, the order I have matches the > > order we had prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host > > powerup to modeset time"). > > > > 2. I'd be curious if it matters. The order you request means we need > > to check for `(is_bonded_dsi && msm_dsi1)` twice. While that's not a > > big deal if it's important, it's nice not to have to do so. > > > > 3. As talked about in the commit message, eventually we should > > probably resolve this order with the order of things in > > dsi_mgr_bridge_post_disable(), which is yet a different ordering. > > Ideally this resolution would be done by someone who actually has > > proper documentation of the hardware and how it's supposed to work > > (AKA not me). > > > > So my preference would be to either land or drop ${SUBJECT} patch > > (either is fine with me) and then someone at Qualcomm could then take > > over further cleanup. > > > > I do think the ordering matters but you are right, this change brings > back the ordering we had before so lets handle the re-ordering of all > places in a separate change. I am okay with this change to go-in, hence > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > What is the plan to land the patches? > > 2 & 3 go in msm-next but 1 goes in drm-misc? We can do that and I'm happy to land patch #1 in drm-misc. Then I assume we'd want to wait until the change makes its way into mainline before landing patch #2/#3? Given how tiny patch #1 is, though, it sure seems like it would be nice / easier if they all went through the msm tree. I guess we'd want one of the drm-misc maintainers (not just a committer like me) to Ack this? Maybe it's not worth it and we should just go the slow route? -Doug
On 01/02/2023 00:18, Douglas Anderson wrote: > In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset > time") the error handling with regards to dsi_mgr_bridge_power_on() > got a bit worse. Specifically if we failed to power the bridge on then > nothing would really notice. The modeset function couldn't return an > error and thus we'd blindly go forward and try to do the pre-enable. > > In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time > for parade-ps8640") we added a special case to move the powerup back > to pre-enable time for ps8640. When we did that, we didn't try to > recover the old/better error handling just for ps8640. > > In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts > at modeset") we've now moved the powering up back to exclusively being > during pre-enable. That means we can add the better error handling > back in, so let's do it. To do so we'll add a new function > dsi_mgr_bridge_power_off() that's matches how errors were handled > prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to > modeset time"). > > NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we > should be calling it in dsi_mgr_bridge_post_disable(). That would make > some sense, but doing so would change the current behavior and thus > should be a separate patch. Specifically: > * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable() > even in the slave-DSI case of bonded DSI. We'd need to add special > handling for this if it's truly needed. > * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state() > midway through the poweroff. > * dsi_mgr_bridge_post_disable() has a different order of some of the > poweroffs / IRQ disables. > For now we'll leave dsi_mgr_bridge_post_disable() alone. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index 2197a54b9b96..28b8012a21f2 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -228,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id) } } -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) +static int dsi_mgr_bridge_power_on(struct drm_bridge *bridge) { int id = dsi_mgr_bridge_get_id(bridge); struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); @@ -268,14 +268,31 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge) if (is_bonded_dsi && msm_dsi1) msm_dsi_host_enable_irq(msm_dsi1->host); - return; + return 0; host1_on_fail: msm_dsi_host_power_off(host); host_on_fail: dsi_mgr_phy_disable(id); phy_en_fail: - return; + return ret; +} + +static void dsi_mgr_bridge_power_off(struct drm_bridge *bridge) +{ + int id = dsi_mgr_bridge_get_id(bridge); + struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id); + struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); + struct mipi_dsi_host *host = msm_dsi->host; + bool is_bonded_dsi = IS_BONDED_DSI(); + + msm_dsi_host_disable_irq(host); + if (is_bonded_dsi && msm_dsi1) { + msm_dsi_host_disable_irq(msm_dsi1->host); + msm_dsi_host_power_off(msm_dsi1->host); + } + msm_dsi_host_power_off(host); + dsi_mgr_phy_disable(id); } static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) @@ -295,7 +312,11 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) return; - dsi_mgr_bridge_power_on(bridge); + ret = dsi_mgr_bridge_power_on(bridge); + if (ret) { + dev_err(&msm_dsi->pdev->dev, "Power on failed: %d\n", ret); + return; + } ret = msm_dsi_host_enable(host); if (ret) { @@ -316,8 +337,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) host1_en_fail: msm_dsi_host_disable(host); host_en_fail: - - return; + dsi_mgr_bridge_power_off(bridge); } void msm_dsi_manager_tpg_enable(void)
In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time") the error handling with regards to dsi_mgr_bridge_power_on() got a bit worse. Specifically if we failed to power the bridge on then nothing would really notice. The modeset function couldn't return an error and thus we'd blindly go forward and try to do the pre-enable. In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") we added a special case to move the powerup back to pre-enable time for ps8640. When we did that, we didn't try to recover the old/better error handling just for ps8640. In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset") we've now moved the powering up back to exclusively being during pre-enable. That means we can add the better error handling back in, so let's do it. To do so we'll add a new function dsi_mgr_bridge_power_off() that's matches how errors were handled prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset time"). NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we should be calling it in dsi_mgr_bridge_post_disable(). That would make some sense, but doing so would change the current behavior and thus should be a separate patch. Specifically: * dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable() even in the slave-DSI case of bonded DSI. We'd need to add special handling for this if it's truly needed. * dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state() midway through the poweroff. * dsi_mgr_bridge_post_disable() has a different order of some of the poweroffs / IRQ disables. For now we'll leave dsi_mgr_bridge_post_disable() alone. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v2: - ("More properly handle errors...") new for v2. drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 ++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 6 deletions(-)