| Message ID | 20211207222901.988484-2-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dsi: switch msm/dsi to DRM_PANEL_BRIDGE | expand |
On 12/7/2021 2:29 PM, Dmitry Baryshkov wrote: > The DSI subsystem does not fully fall into the pre-enable/enable system > of callbacks, since typically DSI device bridge drivers expect to be > able to communicate with DSI devices at the pre-enable() callback. The > reason is that for some DSI hosts enabling the video stream would > prevent other drivers from sending DSI commands. For example see the > panel-bridge driver, which does drm_panel_prepare() from the > pre_enable() callback (which would be called before our pre_enable() > callback, resulting in panel preparation failures as the link is not yet > ready). > > Therewere several attempts to solve this issue, but currently the best > approach is to power up the DSI link from the mode_set() callback, > allowing next bridge/panel to use DSI transfers in the pre_enable() > time. Follow this approach. > Change looks okay. As per the programming guideline, we should set the VIDEO_MODE_EN register in the DSI controller followed by enabling the timing engine which will still happen even now because we will do it in modeset instead of the pre_enable(). But, this can potentially increase the delay between VIDEO_MODE_EN and TIMING_ENGINE_EN. I dont see anything in the programming guide against this but since this is a change from the original flow, I would like to do one test before acking this. Can you please try adding a huge delay like 200-300ms between VIDEO_MODE_EN and timing engine enable to make sure there are no issues? You can do that here: int msm_dsi_host_enable(struct mipi_dsi_host *host) { struct msm_dsi_host *msm_host = to_msm_dsi_host(host); dsi_op_mode_config(msm_host, !!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO), true); msleep(300); } > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/dsi/dsi_manager.c | 43 +++++++++++++++++++-------- > 1 file changed, 31 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > index 681ca74fe410..497719efb9e9 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > @@ -336,13 +336,12 @@ dsi_mgr_connector_best_encoder(struct drm_connector *connector) > return msm_dsi_get_encoder(msm_dsi); > } > > -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > +static void 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); > struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); > struct mipi_dsi_host *host = msm_dsi->host; > - struct drm_panel *panel = msm_dsi->panel; > struct msm_dsi_phy_shared_timings phy_shared_timings[DSI_MAX]; > bool is_bonded_dsi = IS_BONDED_DSI(); > int ret; > @@ -383,6 +382,34 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > if (is_bonded_dsi && msm_dsi1) > msm_dsi_host_enable_irq(msm_dsi1->host); > > + return; > + > +host1_on_fail: > + msm_dsi_host_power_off(host); > +host_on_fail: > + dsi_mgr_phy_disable(id); > +phy_en_fail: > + return; > +} > + > +static void dsi_mgr_bridge_pre_enable(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; > + struct drm_panel *panel = msm_dsi->panel; > + bool is_bonded_dsi = IS_BONDED_DSI(); > + int ret; > + > + DBG("id=%d", id); > + if (!msm_dsi_device_connected(msm_dsi)) > + return; > + > + /* Do nothing with the host if it is slave-DSI in case of bonded DSI */ > + if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) > + return; > + > /* Always call panel functions once, because even for dual panels, > * there is only one drm_panel instance. > */ > @@ -417,17 +444,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > if (panel) > drm_panel_unprepare(panel); > panel_prep_fail: > - msm_dsi_host_disable_irq(host); > - if (is_bonded_dsi && msm_dsi1) > - msm_dsi_host_disable_irq(msm_dsi1->host); > > - if (is_bonded_dsi && msm_dsi1) > - msm_dsi_host_power_off(msm_dsi1->host); > -host1_on_fail: > - msm_dsi_host_power_off(host); > -host_on_fail: > - dsi_mgr_phy_disable(id); > -phy_en_fail: > return; > } > > @@ -573,6 +590,8 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge, > msm_dsi_host_set_display_mode(host, adjusted_mode); > if (is_bonded_dsi && other_dsi) > msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode); > + > + dsi_mgr_bridge_power_on(bridge); > } > > static const struct drm_connector_funcs dsi_mgr_connector_funcs = {
On Tue, 18 Jan 2022 at 22:29, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 12/7/2021 2:29 PM, Dmitry Baryshkov wrote: > > The DSI subsystem does not fully fall into the pre-enable/enable system > > of callbacks, since typically DSI device bridge drivers expect to be > > able to communicate with DSI devices at the pre-enable() callback. The > > reason is that for some DSI hosts enabling the video stream would > > prevent other drivers from sending DSI commands. For example see the > > panel-bridge driver, which does drm_panel_prepare() from the > > pre_enable() callback (which would be called before our pre_enable() > > callback, resulting in panel preparation failures as the link is not yet > > ready). > > > > Therewere several attempts to solve this issue, but currently the best > > approach is to power up the DSI link from the mode_set() callback, > > allowing next bridge/panel to use DSI transfers in the pre_enable() > > time. Follow this approach. > > > Change looks okay. As per the programming guideline, we should set the > VIDEO_MODE_EN register in the DSI controller followed by enabling the > timing engine which will still happen even now because we will do it in > modeset instead of the pre_enable(). > But, this can potentially increase the delay between VIDEO_MODE_EN > and TIMING_ENGINE_EN. I dont see anything in the programming guide > against this but since this is a change from the original flow, I would > like to do one test before acking this. Can you please try adding a huge > delay like 200-300ms between VIDEO_MODE_EN and timing engine enable to > make sure there are no issues? You can do that here: Fine, I'll do the test as the time permits. > > int msm_dsi_host_enable(struct mipi_dsi_host *host) > { > struct msm_dsi_host *msm_host = to_msm_dsi_host(host); > > dsi_op_mode_config(msm_host, > !!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO), true); > > msleep(300); > } > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/dsi/dsi_manager.c | 43 +++++++++++++++++++-------- > > 1 file changed, 31 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c > > index 681ca74fe410..497719efb9e9 100644 > > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c > > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c > > @@ -336,13 +336,12 @@ dsi_mgr_connector_best_encoder(struct drm_connector *connector) > > return msm_dsi_get_encoder(msm_dsi); > > } > > > > -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > > +static void 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); > > struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); > > struct mipi_dsi_host *host = msm_dsi->host; > > - struct drm_panel *panel = msm_dsi->panel; > > struct msm_dsi_phy_shared_timings phy_shared_timings[DSI_MAX]; > > bool is_bonded_dsi = IS_BONDED_DSI(); > > int ret; > > @@ -383,6 +382,34 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > > if (is_bonded_dsi && msm_dsi1) > > msm_dsi_host_enable_irq(msm_dsi1->host); > > > > + return; > > + > > +host1_on_fail: > > + msm_dsi_host_power_off(host); > > +host_on_fail: > > + dsi_mgr_phy_disable(id); > > +phy_en_fail: > > + return; > > +} > > + > > +static void dsi_mgr_bridge_pre_enable(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; > > + struct drm_panel *panel = msm_dsi->panel; > > + bool is_bonded_dsi = IS_BONDED_DSI(); > > + int ret; > > + > > + DBG("id=%d", id); > > + if (!msm_dsi_device_connected(msm_dsi)) > > + return; > > + > > + /* Do nothing with the host if it is slave-DSI in case of bonded DSI */ > > + if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) > > + return; > > + > > /* Always call panel functions once, because even for dual panels, > > * there is only one drm_panel instance. > > */ > > @@ -417,17 +444,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) > > if (panel) > > drm_panel_unprepare(panel); > > panel_prep_fail: > > - msm_dsi_host_disable_irq(host); > > - if (is_bonded_dsi && msm_dsi1) > > - msm_dsi_host_disable_irq(msm_dsi1->host); > > > > - if (is_bonded_dsi && msm_dsi1) > > - msm_dsi_host_power_off(msm_dsi1->host); > > -host1_on_fail: > > - msm_dsi_host_power_off(host); > > -host_on_fail: > > - dsi_mgr_phy_disable(id); > > -phy_en_fail: > > return; > > } > > > > @@ -573,6 +590,8 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge, > > msm_dsi_host_set_display_mode(host, adjusted_mode); > > if (is_bonded_dsi && other_dsi) > > msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode); > > + > > + dsi_mgr_bridge_power_on(bridge); > > } > > > > static const struct drm_connector_funcs dsi_mgr_connector_funcs = {
On 18/01/2022 23:03, Dmitry Baryshkov wrote: > On Tue, 18 Jan 2022 at 22:29, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 12/7/2021 2:29 PM, Dmitry Baryshkov wrote: >>> The DSI subsystem does not fully fall into the pre-enable/enable system >>> of callbacks, since typically DSI device bridge drivers expect to be >>> able to communicate with DSI devices at the pre-enable() callback. The >>> reason is that for some DSI hosts enabling the video stream would >>> prevent other drivers from sending DSI commands. For example see the >>> panel-bridge driver, which does drm_panel_prepare() from the >>> pre_enable() callback (which would be called before our pre_enable() >>> callback, resulting in panel preparation failures as the link is not yet >>> ready). >>> >>> Therewere several attempts to solve this issue, but currently the best >>> approach is to power up the DSI link from the mode_set() callback, >>> allowing next bridge/panel to use DSI transfers in the pre_enable() >>> time. Follow this approach. >>> >> Change looks okay. As per the programming guideline, we should set the >> VIDEO_MODE_EN register in the DSI controller followed by enabling the >> timing engine which will still happen even now because we will do it in >> modeset instead of the pre_enable(). >> But, this can potentially increase the delay between VIDEO_MODE_EN >> and TIMING_ENGINE_EN. I dont see anything in the programming guide >> against this but since this is a change from the original flow, I would >> like to do one test before acking this. Can you please try adding a huge >> delay like 200-300ms between VIDEO_MODE_EN and timing engine enable to >> make sure there are no issues? You can do that here: > > > Fine, I'll do the test as the time permits. I did the tests, the display pipeline works as expected. Let's get this in, it allows using other DSI-controlled bridges. > >> >> int msm_dsi_host_enable(struct mipi_dsi_host *host) >> { >> struct msm_dsi_host *msm_host = to_msm_dsi_host(host); >> >> dsi_op_mode_config(msm_host, >> !!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO), true); >> >> msleep(300); >> } >> >> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/dsi/dsi_manager.c | 43 +++++++++++++++++++-------- >>> 1 file changed, 31 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c >>> index 681ca74fe410..497719efb9e9 100644 >>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c >>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c >>> @@ -336,13 +336,12 @@ dsi_mgr_connector_best_encoder(struct drm_connector *connector) >>> return msm_dsi_get_encoder(msm_dsi); >>> } >>> >>> -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) >>> +static void 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); >>> struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); >>> struct mipi_dsi_host *host = msm_dsi->host; >>> - struct drm_panel *panel = msm_dsi->panel; >>> struct msm_dsi_phy_shared_timings phy_shared_timings[DSI_MAX]; >>> bool is_bonded_dsi = IS_BONDED_DSI(); >>> int ret; >>> @@ -383,6 +382,34 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) >>> if (is_bonded_dsi && msm_dsi1) >>> msm_dsi_host_enable_irq(msm_dsi1->host); >>> >>> + return; >>> + >>> +host1_on_fail: >>> + msm_dsi_host_power_off(host); >>> +host_on_fail: >>> + dsi_mgr_phy_disable(id); >>> +phy_en_fail: >>> + return; >>> +} >>> + >>> +static void dsi_mgr_bridge_pre_enable(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; >>> + struct drm_panel *panel = msm_dsi->panel; >>> + bool is_bonded_dsi = IS_BONDED_DSI(); >>> + int ret; >>> + >>> + DBG("id=%d", id); >>> + if (!msm_dsi_device_connected(msm_dsi)) >>> + return; >>> + >>> + /* Do nothing with the host if it is slave-DSI in case of bonded DSI */ >>> + if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) >>> + return; >>> + >>> /* Always call panel functions once, because even for dual panels, >>> * there is only one drm_panel instance. >>> */ >>> @@ -417,17 +444,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) >>> if (panel) >>> drm_panel_unprepare(panel); >>> panel_prep_fail: >>> - msm_dsi_host_disable_irq(host); >>> - if (is_bonded_dsi && msm_dsi1) >>> - msm_dsi_host_disable_irq(msm_dsi1->host); >>> >>> - if (is_bonded_dsi && msm_dsi1) >>> - msm_dsi_host_power_off(msm_dsi1->host); >>> -host1_on_fail: >>> - msm_dsi_host_power_off(host); >>> -host_on_fail: >>> - dsi_mgr_phy_disable(id); >>> -phy_en_fail: >>> return; >>> } >>> >>> @@ -573,6 +590,8 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge, >>> msm_dsi_host_set_display_mode(host, adjusted_mode); >>> if (is_bonded_dsi && other_dsi) >>> msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode); >>> + >>> + dsi_mgr_bridge_power_on(bridge); >>> } >>> >>> static const struct drm_connector_funcs dsi_mgr_connector_funcs = { > > >
On 2/11/2022 11:37 AM, Dmitry Baryshkov wrote: > On 18/01/2022 23:03, Dmitry Baryshkov wrote: >> On Tue, 18 Jan 2022 at 22:29, Abhinav Kumar >> <quic_abhinavk@quicinc.com> wrote: >>> >>> >>> >>> On 12/7/2021 2:29 PM, Dmitry Baryshkov wrote: >>>> The DSI subsystem does not fully fall into the pre-enable/enable system >>>> of callbacks, since typically DSI device bridge drivers expect to be >>>> able to communicate with DSI devices at the pre-enable() callback. The >>>> reason is that for some DSI hosts enabling the video stream would >>>> prevent other drivers from sending DSI commands. For example see the >>>> panel-bridge driver, which does drm_panel_prepare() from the >>>> pre_enable() callback (which would be called before our pre_enable() >>>> callback, resulting in panel preparation failures as the link is not >>>> yet >>>> ready). >>>> >>>> Therewere several attempts to solve this issue, but currently the best >>>> approach is to power up the DSI link from the mode_set() callback, >>>> allowing next bridge/panel to use DSI transfers in the pre_enable() >>>> time. Follow this approach. >>>> >>> Change looks okay. As per the programming guideline, we should set the >>> VIDEO_MODE_EN register in the DSI controller followed by enabling the >>> timing engine which will still happen even now because we will do it in >>> modeset instead of the pre_enable(). >>> But, this can potentially increase the delay between VIDEO_MODE_EN >>> and TIMING_ENGINE_EN. I dont see anything in the programming guide >>> against this but since this is a change from the original flow, I would >>> like to do one test before acking this. Can you please try adding a huge >>> delay like 200-300ms between VIDEO_MODE_EN and timing engine enable to >>> make sure there are no issues? You can do that here: >> >> >> Fine, I'll do the test as the time permits. > > I did the tests, the display pipeline works as expected. > > Let's get this in, it allows using other DSI-controlled bridges. Alright, sounds good, Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > >> >>> >>> int msm_dsi_host_enable(struct mipi_dsi_host *host) >>> { >>> struct msm_dsi_host *msm_host = to_msm_dsi_host(host); >>> >>> dsi_op_mode_config(msm_host, >>> !!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO), true); >>> >>> msleep(300); >>> } >>> >>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/gpu/drm/msm/dsi/dsi_manager.c | 43 >>>> +++++++++++++++++++-------- >>>> 1 file changed, 31 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c >>>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c >>>> index 681ca74fe410..497719efb9e9 100644 >>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c >>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c >>>> @@ -336,13 +336,12 @@ dsi_mgr_connector_best_encoder(struct >>>> drm_connector *connector) >>>> return msm_dsi_get_encoder(msm_dsi); >>>> } >>>> >>>> -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) >>>> +static void 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); >>>> struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); >>>> struct mipi_dsi_host *host = msm_dsi->host; >>>> - struct drm_panel *panel = msm_dsi->panel; >>>> struct msm_dsi_phy_shared_timings phy_shared_timings[DSI_MAX]; >>>> bool is_bonded_dsi = IS_BONDED_DSI(); >>>> int ret; >>>> @@ -383,6 +382,34 @@ static void dsi_mgr_bridge_pre_enable(struct >>>> drm_bridge *bridge) >>>> if (is_bonded_dsi && msm_dsi1) >>>> msm_dsi_host_enable_irq(msm_dsi1->host); >>>> >>>> + return; >>>> + >>>> +host1_on_fail: >>>> + msm_dsi_host_power_off(host); >>>> +host_on_fail: >>>> + dsi_mgr_phy_disable(id); >>>> +phy_en_fail: >>>> + return; >>>> +} >>>> + >>>> +static void dsi_mgr_bridge_pre_enable(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; >>>> + struct drm_panel *panel = msm_dsi->panel; >>>> + bool is_bonded_dsi = IS_BONDED_DSI(); >>>> + int ret; >>>> + >>>> + DBG("id=%d", id); >>>> + if (!msm_dsi_device_connected(msm_dsi)) >>>> + return; >>>> + >>>> + /* Do nothing with the host if it is slave-DSI in case of >>>> bonded DSI */ >>>> + if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) >>>> + return; >>>> + >>>> /* Always call panel functions once, because even for dual >>>> panels, >>>> * there is only one drm_panel instance. >>>> */ >>>> @@ -417,17 +444,7 @@ static void dsi_mgr_bridge_pre_enable(struct >>>> drm_bridge *bridge) >>>> if (panel) >>>> drm_panel_unprepare(panel); >>>> panel_prep_fail: >>>> - msm_dsi_host_disable_irq(host); >>>> - if (is_bonded_dsi && msm_dsi1) >>>> - msm_dsi_host_disable_irq(msm_dsi1->host); >>>> >>>> - if (is_bonded_dsi && msm_dsi1) >>>> - msm_dsi_host_power_off(msm_dsi1->host); >>>> -host1_on_fail: >>>> - msm_dsi_host_power_off(host); >>>> -host_on_fail: >>>> - dsi_mgr_phy_disable(id); >>>> -phy_en_fail: >>>> return; >>>> } >>>> >>>> @@ -573,6 +590,8 @@ static void dsi_mgr_bridge_mode_set(struct >>>> drm_bridge *bridge, >>>> msm_dsi_host_set_display_mode(host, adjusted_mode); >>>> if (is_bonded_dsi && other_dsi) >>>> msm_dsi_host_set_display_mode(other_dsi->host, >>>> adjusted_mode); >>>> + >>>> + dsi_mgr_bridge_power_on(bridge); >>>> } >>>> >>>> static const struct drm_connector_funcs dsi_mgr_connector_funcs = { >> >> >> > >
Hi, On Tue, Dec 7, 2021 at 2:29 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > The DSI subsystem does not fully fall into the pre-enable/enable system > of callbacks, since typically DSI device bridge drivers expect to be > able to communicate with DSI devices at the pre-enable() callback. The > reason is that for some DSI hosts enabling the video stream would > prevent other drivers from sending DSI commands. For example see the > panel-bridge driver, which does drm_panel_prepare() from the > pre_enable() callback (which would be called before our pre_enable() > callback, resulting in panel preparation failures as the link is not yet > ready). > > Therewere several attempts to solve this issue, but currently the best > approach is to power up the DSI link from the mode_set() callback, > allowing next bridge/panel to use DSI transfers in the pre_enable() > time. Follow this approach. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/dsi/dsi_manager.c | 43 +++++++++++++++++++-------- > 1 file changed, 31 insertions(+), 12 deletions(-) I happened to be testing today on one of the sc7180-trogdor variants that has a parade-ps8640 bridge chip in it and ran into problems. A bisect pointed to this patch and, sure enough, reverting it fixes me again. The Chromebook in question is able to power the screen on at bootup but things don't work so well in other cases. Specifically, if I leave the Chromebook idle then it will turn the screen off (but in this case, not enter S3). Hitting a key should wake the screen up, but it doesn't. None of the error prints in dsi_mgr_bridge_power_on() are hitting when it fails and I even added extra error prints. It's not hitting any of the early returns. I did a little bit more debugging and it appears that nothing funny is going on. It's just the ordering difference that matters. With the patch reverted, I see this and it all works: boot: [ 9.653801] DOUG: dsi_mgr_bridge_mode_set [ 9.658687] DOUG: ps8640_pre_enable [ 9.664194] DOUG: dsi_mgr_bridge_pre_enable screen turns off: [ 82.130038] DOUG: dsi_mgr_bridge_post_disable [ 82.166817] DOUG: ps8640_post_disable screen turns on: [ 92.611846] DOUG: dsi_mgr_bridge_mode_set [ 92.617875] DOUG: ps8640_pre_enable [ 92.920237] DOUG: dsi_mgr_bridge_pre_enable Without the patch reverted, I see this and it fails: boot: [ 10.817810] DOUG: dsi_mgr_bridge_mode_set [ 10.822128] DOUG: dsi_mgr_bridge_power_on [ 10.852131] DOUG: ps8640_pre_enable [ 10.857942] DOUG: dsi_mgr_bridge_pre_enable screen turns off: [ 34.819953] DOUG: dsi_mgr_bridge_post_disable [ 34.883777] DOUG: ps8640_post_disable screen should turn on, but doesn't: [ 46.193589] DOUG: dsi_mgr_bridge_mode_set [ 46.197951] DOUG: dsi_mgr_bridge_power_on [ 46.248438] DOUG: ps8640_pre_enable [ 46.541700] DOUG: dsi_mgr_bridge_pre_enable Unfortunately, ps8640 is a pretty big black box. The Linux kernel driver does almost nothing at all and the parade bridge chip has a bunch of magic firmware on it. Though I don't know for sure, I assume that this magic firmware simply can't handle the fact that the MIPI side is already setup / running when the bridge is powered on. Rather than this patch, maybe you can jump on Dave Stevenson's patch series [1] which I believe would solve this problem in a more dynamic way? Would you accept a revert of ${SUBJECT} patch to fix my problem? [1] https://lore.kernel.org/r/cover.1646406653.git.dave.stevenson@raspberrypi.com -Doug
On 11/05/2022 23:06, Doug Anderson wrote: > Hi, > > On Tue, Dec 7, 2021 at 2:29 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> >> The DSI subsystem does not fully fall into the pre-enable/enable system >> of callbacks, since typically DSI device bridge drivers expect to be >> able to communicate with DSI devices at the pre-enable() callback. The >> reason is that for some DSI hosts enabling the video stream would >> prevent other drivers from sending DSI commands. For example see the >> panel-bridge driver, which does drm_panel_prepare() from the >> pre_enable() callback (which would be called before our pre_enable() >> callback, resulting in panel preparation failures as the link is not yet >> ready). >> >> Therewere several attempts to solve this issue, but currently the best >> approach is to power up the DSI link from the mode_set() callback, >> allowing next bridge/panel to use DSI transfers in the pre_enable() >> time. Follow this approach. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/msm/dsi/dsi_manager.c | 43 +++++++++++++++++++-------- >> 1 file changed, 31 insertions(+), 12 deletions(-) > > I happened to be testing today on one of the sc7180-trogdor variants > that has a parade-ps8640 bridge chip in it and ran into problems. A > bisect pointed to this patch and, sure enough, reverting it fixes me > again. > > The Chromebook in question is able to power the screen on at bootup > but things don't work so well in other cases. Specifically, if I leave > the Chromebook idle then it will turn the screen off (but in this > case, not enter S3). Hitting a key should wake the screen up, but it > doesn't. > > None of the error prints in dsi_mgr_bridge_power_on() are hitting when > it fails and I even added extra error prints. It's not hitting any of > the early returns. > > I did a little bit more debugging and it appears that nothing funny is > going on. It's just the ordering difference that matters. With the > patch reverted, I see this and it all works: > > boot: > [ 9.653801] DOUG: dsi_mgr_bridge_mode_set > [ 9.658687] DOUG: ps8640_pre_enable > [ 9.664194] DOUG: dsi_mgr_bridge_pre_enable > > screen turns off: > [ 82.130038] DOUG: dsi_mgr_bridge_post_disable > [ 82.166817] DOUG: ps8640_post_disable > > screen turns on: > [ 92.611846] DOUG: dsi_mgr_bridge_mode_set > [ 92.617875] DOUG: ps8640_pre_enable > [ 92.920237] DOUG: dsi_mgr_bridge_pre_enable > > Without the patch reverted, I see this and it fails: > > boot: > [ 10.817810] DOUG: dsi_mgr_bridge_mode_set > [ 10.822128] DOUG: dsi_mgr_bridge_power_on > [ 10.852131] DOUG: ps8640_pre_enable > [ 10.857942] DOUG: dsi_mgr_bridge_pre_enable > > screen turns off: > [ 34.819953] DOUG: dsi_mgr_bridge_post_disable > [ 34.883777] DOUG: ps8640_post_disable > > screen should turn on, but doesn't: > [ 46.193589] DOUG: dsi_mgr_bridge_mode_set > [ 46.197951] DOUG: dsi_mgr_bridge_power_on > [ 46.248438] DOUG: ps8640_pre_enable > [ 46.541700] DOUG: dsi_mgr_bridge_pre_enable > > Unfortunately, ps8640 is a pretty big black box. The Linux kernel > driver does almost nothing at all and the parade bridge chip has a > bunch of magic firmware on it. Though I don't know for sure, I assume > that this magic firmware simply can't handle the fact that the MIPI > side is already setup / running when the bridge is powered on. > > Rather than this patch, maybe you can jump on Dave Stevenson's patch > series [1] which I believe would solve this problem in a more dynamic > way? Would you accept a revert of ${SUBJECT} patch to fix my problem? I'm fine with the revert, but it will depend on [1]. Otherwise other MIPI DSI bridges are broken (see the discussion at [2]). > [1] https://lore.kernel.org/r/cover.1646406653.git.dave.stevenson@raspberrypi.com [2] https://lore.kernel.org/all/CAPY8ntBrhYAmsraDqJGuTrSL6VjGXBAMVoN7xweV7E4qZv+v3Q@mail.gmail.com/
On Wed, May 11, 2022 at 2:49 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On 11/05/2022 23:06, Doug Anderson wrote: > > Hi, > > > > On Tue, Dec 7, 2021 at 2:29 PM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > >> > >> The DSI subsystem does not fully fall into the pre-enable/enable system > >> of callbacks, since typically DSI device bridge drivers expect to be > >> able to communicate with DSI devices at the pre-enable() callback. The > >> reason is that for some DSI hosts enabling the video stream would > >> prevent other drivers from sending DSI commands. For example see the > >> panel-bridge driver, which does drm_panel_prepare() from the > >> pre_enable() callback (which would be called before our pre_enable() > >> callback, resulting in panel preparation failures as the link is not yet > >> ready). > >> > >> Therewere several attempts to solve this issue, but currently the best > >> approach is to power up the DSI link from the mode_set() callback, > >> allowing next bridge/panel to use DSI transfers in the pre_enable() > >> time. Follow this approach. > >> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >> --- > >> drivers/gpu/drm/msm/dsi/dsi_manager.c | 43 +++++++++++++++++++-------- > >> 1 file changed, 31 insertions(+), 12 deletions(-) > > > > I happened to be testing today on one of the sc7180-trogdor variants > > that has a parade-ps8640 bridge chip in it and ran into problems. A > > bisect pointed to this patch and, sure enough, reverting it fixes me > > again. > > > > The Chromebook in question is able to power the screen on at bootup > > but things don't work so well in other cases. Specifically, if I leave > > the Chromebook idle then it will turn the screen off (but in this > > case, not enter S3). Hitting a key should wake the screen up, but it > > doesn't. > > > > None of the error prints in dsi_mgr_bridge_power_on() are hitting when > > it fails and I even added extra error prints. It's not hitting any of > > the early returns. > > > > I did a little bit more debugging and it appears that nothing funny is > > going on. It's just the ordering difference that matters. With the > > patch reverted, I see this and it all works: > > > > boot: > > [ 9.653801] DOUG: dsi_mgr_bridge_mode_set > > [ 9.658687] DOUG: ps8640_pre_enable > > [ 9.664194] DOUG: dsi_mgr_bridge_pre_enable > > > > screen turns off: > > [ 82.130038] DOUG: dsi_mgr_bridge_post_disable > > [ 82.166817] DOUG: ps8640_post_disable > > > > screen turns on: > > [ 92.611846] DOUG: dsi_mgr_bridge_mode_set > > [ 92.617875] DOUG: ps8640_pre_enable > > [ 92.920237] DOUG: dsi_mgr_bridge_pre_enable > > > > Without the patch reverted, I see this and it fails: > > > > boot: > > [ 10.817810] DOUG: dsi_mgr_bridge_mode_set > > [ 10.822128] DOUG: dsi_mgr_bridge_power_on > > [ 10.852131] DOUG: ps8640_pre_enable > > [ 10.857942] DOUG: dsi_mgr_bridge_pre_enable > > > > screen turns off: > > [ 34.819953] DOUG: dsi_mgr_bridge_post_disable > > [ 34.883777] DOUG: ps8640_post_disable > > > > screen should turn on, but doesn't: > > [ 46.193589] DOUG: dsi_mgr_bridge_mode_set > > [ 46.197951] DOUG: dsi_mgr_bridge_power_on > > [ 46.248438] DOUG: ps8640_pre_enable > > [ 46.541700] DOUG: dsi_mgr_bridge_pre_enable > > > > Unfortunately, ps8640 is a pretty big black box. The Linux kernel > > driver does almost nothing at all and the parade bridge chip has a > > bunch of magic firmware on it. Though I don't know for sure, I assume > > that this magic firmware simply can't handle the fact that the MIPI > > side is already setup / running when the bridge is powered on. > > > > Rather than this patch, maybe you can jump on Dave Stevenson's patch > > series [1] which I believe would solve this problem in a more dynamic > > way? Would you accept a revert of ${SUBJECT} patch to fix my problem? > > I'm fine with the revert, but it will depend on [1]. Otherwise other > MIPI DSI bridges are broken (see the discussion at [2]). heh, well the problem is that MIPI DSI bridges, or at least one of them, is broken *without* the revert ;-) [1] looks like a bit much for -fixes, so I'd be inclined to revert this patch and at least go back to the broken/working state from before for the time being.. BR, -R > > [1] https://lore.kernel.org/r/cover.1646406653.git.dave.stevenson@raspberrypi.com > > [2] > https://lore.kernel.org/all/CAPY8ntBrhYAmsraDqJGuTrSL6VjGXBAMVoN7xweV7E4qZv+v3Q@mail.gmail.com/ > > > -- > With best wishes > Dmitry
On 12/05/2022 01:07, Rob Clark wrote: > On Wed, May 11, 2022 at 2:49 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> >> On 11/05/2022 23:06, Doug Anderson wrote: >>> Hi, >>> >>> On Tue, Dec 7, 2021 at 2:29 PM Dmitry Baryshkov >>> <dmitry.baryshkov@linaro.org> wrote: >>>> >>>> The DSI subsystem does not fully fall into the pre-enable/enable system >>>> of callbacks, since typically DSI device bridge drivers expect to be >>>> able to communicate with DSI devices at the pre-enable() callback. The >>>> reason is that for some DSI hosts enabling the video stream would >>>> prevent other drivers from sending DSI commands. For example see the >>>> panel-bridge driver, which does drm_panel_prepare() from the >>>> pre_enable() callback (which would be called before our pre_enable() >>>> callback, resulting in panel preparation failures as the link is not yet >>>> ready). >>>> >>>> Therewere several attempts to solve this issue, but currently the best >>>> approach is to power up the DSI link from the mode_set() callback, >>>> allowing next bridge/panel to use DSI transfers in the pre_enable() >>>> time. Follow this approach. >>>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/gpu/drm/msm/dsi/dsi_manager.c | 43 +++++++++++++++++++-------- >>>> 1 file changed, 31 insertions(+), 12 deletions(-) >>> >>> I happened to be testing today on one of the sc7180-trogdor variants >>> that has a parade-ps8640 bridge chip in it and ran into problems. A >>> bisect pointed to this patch and, sure enough, reverting it fixes me >>> again. >>> >>> The Chromebook in question is able to power the screen on at bootup >>> but things don't work so well in other cases. Specifically, if I leave >>> the Chromebook idle then it will turn the screen off (but in this >>> case, not enter S3). Hitting a key should wake the screen up, but it >>> doesn't. >>> >>> None of the error prints in dsi_mgr_bridge_power_on() are hitting when >>> it fails and I even added extra error prints. It's not hitting any of >>> the early returns. >>> >>> I did a little bit more debugging and it appears that nothing funny is >>> going on. It's just the ordering difference that matters. With the >>> patch reverted, I see this and it all works: >>> >>> boot: >>> [ 9.653801] DOUG: dsi_mgr_bridge_mode_set >>> [ 9.658687] DOUG: ps8640_pre_enable >>> [ 9.664194] DOUG: dsi_mgr_bridge_pre_enable >>> >>> screen turns off: >>> [ 82.130038] DOUG: dsi_mgr_bridge_post_disable >>> [ 82.166817] DOUG: ps8640_post_disable >>> >>> screen turns on: >>> [ 92.611846] DOUG: dsi_mgr_bridge_mode_set >>> [ 92.617875] DOUG: ps8640_pre_enable >>> [ 92.920237] DOUG: dsi_mgr_bridge_pre_enable >>> >>> Without the patch reverted, I see this and it fails: >>> >>> boot: >>> [ 10.817810] DOUG: dsi_mgr_bridge_mode_set >>> [ 10.822128] DOUG: dsi_mgr_bridge_power_on >>> [ 10.852131] DOUG: ps8640_pre_enable >>> [ 10.857942] DOUG: dsi_mgr_bridge_pre_enable >>> >>> screen turns off: >>> [ 34.819953] DOUG: dsi_mgr_bridge_post_disable >>> [ 34.883777] DOUG: ps8640_post_disable >>> >>> screen should turn on, but doesn't: >>> [ 46.193589] DOUG: dsi_mgr_bridge_mode_set >>> [ 46.197951] DOUG: dsi_mgr_bridge_power_on >>> [ 46.248438] DOUG: ps8640_pre_enable >>> [ 46.541700] DOUG: dsi_mgr_bridge_pre_enable >>> >>> Unfortunately, ps8640 is a pretty big black box. The Linux kernel >>> driver does almost nothing at all and the parade bridge chip has a >>> bunch of magic firmware on it. Though I don't know for sure, I assume >>> that this magic firmware simply can't handle the fact that the MIPI >>> side is already setup / running when the bridge is powered on. >>> >>> Rather than this patch, maybe you can jump on Dave Stevenson's patch >>> series [1] which I believe would solve this problem in a more dynamic >>> way? Would you accept a revert of ${SUBJECT} patch to fix my problem? >> >> I'm fine with the revert, but it will depend on [1]. Otherwise other >> MIPI DSI bridges are broken (see the discussion at [2]). > > heh, well the problem is that MIPI DSI bridges, or at least one of > them, is broken *without* the revert ;-) > > [1] looks like a bit much for -fixes, so I'd be inclined to revert > this patch and at least go back to the broken/working state from > before for the time being.. So... We either get the ps8640 or tc358762 & the rest broken. I'd suggest a modparam. > > BR, > -R > >>> [1] https://lore.kernel.org/r/cover.1646406653.git.dave.stevenson@raspberrypi.com >> >> [2] >> https://lore.kernel.org/all/CAPY8ntBrhYAmsraDqJGuTrSL6VjGXBAMVoN7xweV7E4qZv+v3Q@mail.gmail.com/ >> >> >> -- >> With best wishes >> Dmitry
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index 681ca74fe410..497719efb9e9 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -336,13 +336,12 @@ dsi_mgr_connector_best_encoder(struct drm_connector *connector) return msm_dsi_get_encoder(msm_dsi); } -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) +static void 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); struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1); struct mipi_dsi_host *host = msm_dsi->host; - struct drm_panel *panel = msm_dsi->panel; struct msm_dsi_phy_shared_timings phy_shared_timings[DSI_MAX]; bool is_bonded_dsi = IS_BONDED_DSI(); int ret; @@ -383,6 +382,34 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) if (is_bonded_dsi && msm_dsi1) msm_dsi_host_enable_irq(msm_dsi1->host); + return; + +host1_on_fail: + msm_dsi_host_power_off(host); +host_on_fail: + dsi_mgr_phy_disable(id); +phy_en_fail: + return; +} + +static void dsi_mgr_bridge_pre_enable(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; + struct drm_panel *panel = msm_dsi->panel; + bool is_bonded_dsi = IS_BONDED_DSI(); + int ret; + + DBG("id=%d", id); + if (!msm_dsi_device_connected(msm_dsi)) + return; + + /* Do nothing with the host if it is slave-DSI in case of bonded DSI */ + if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id)) + return; + /* Always call panel functions once, because even for dual panels, * there is only one drm_panel instance. */ @@ -417,17 +444,7 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) if (panel) drm_panel_unprepare(panel); panel_prep_fail: - msm_dsi_host_disable_irq(host); - if (is_bonded_dsi && msm_dsi1) - msm_dsi_host_disable_irq(msm_dsi1->host); - if (is_bonded_dsi && msm_dsi1) - msm_dsi_host_power_off(msm_dsi1->host); -host1_on_fail: - msm_dsi_host_power_off(host); -host_on_fail: - dsi_mgr_phy_disable(id); -phy_en_fail: return; } @@ -573,6 +590,8 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge, msm_dsi_host_set_display_mode(host, adjusted_mode); if (is_bonded_dsi && other_dsi) msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode); + + dsi_mgr_bridge_power_on(bridge); } static const struct drm_connector_funcs dsi_mgr_connector_funcs = {
The DSI subsystem does not fully fall into the pre-enable/enable system of callbacks, since typically DSI device bridge drivers expect to be able to communicate with DSI devices at the pre-enable() callback. The reason is that for some DSI hosts enabling the video stream would prevent other drivers from sending DSI commands. For example see the panel-bridge driver, which does drm_panel_prepare() from the pre_enable() callback (which would be called before our pre_enable() callback, resulting in panel preparation failures as the link is not yet ready). Therewere several attempts to solve this issue, but currently the best approach is to power up the DSI link from the mode_set() callback, allowing next bridge/panel to use DSI transfers in the pre_enable() time. Follow this approach. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/dsi/dsi_manager.c | 43 +++++++++++++++++++-------- 1 file changed, 31 insertions(+), 12 deletions(-)