[Freedreno] [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()

[Abhinav Kumar]

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 at 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 at 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 at 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