Message-ID: <74594853-6ab9-c44d-6f67-38d65fc46466@quicinc.com>
Date:   Thu, 2 Feb 2023 12:10:04 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.2
Subject: Re: [RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up
 DSI hosts at modeset
Content-Language: en-US
To:     Doug Anderson <dianders@chromium.org>
CC:     <dri-devel@lists.freedesktop.org>, Rob Clark <robdclark@gmail.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Andrzej Hajda <andrzej.hajda@intel.com>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Sean Paul <sean@poorly.run>, Jonas Karlman <jonas@kwiboo.se>,
        Vinod Koul <vkoul@kernel.org>,
        Robert Foss <robert.foss@linaro.org>,
        <linux-arm-msm@vger.kernel.org>, Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@gmail.com>,
        <freedreno@lists.freedesktop.org>,
        Stephen Boyd <swboyd@chromium.org>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        <linux-kernel@vger.kernel.org>
References: <20230131141756.RFT.v2.1.I723a3761d57ea60c5dd754c144aed6c3b2ea6f5a@changeid>
 <20230131141756.RFT.v2.2.I4cfeab9d0e07e98ead23dd0736ab4461e6c69002@changeid>
 <43095d93-29c8-b30a-08c0-0a452770c1ce@quicinc.com>
 <CAD=FV=X6A4aZVCaqhT9yP0tD82R3fnaDak67w+p8+Z=WkaRxfw@mail.gmail.com>
From:   Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <CAD=FV=X6A4aZVCaqhT9yP0tD82R3fnaDak67w+p8+Z=WkaRxfw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2/1/2023 6:33 AM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jan 31, 2023 at 3:32 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> On 1/31/2023 2:18 PM, Douglas Anderson wrote:
>>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
>>> time"), we moved powering up DSI hosts to modeset time. This wasn't
>>> because it was an elegant design, but there were no better options.
>>>
>>> That commit actually ended up breaking ps8640, and thus was born
>>> commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
>>> parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
>>> the old way of doing things. It turns out that ps8640 _really_ doesn't
>>> like its pre_enable() function to be called after
>>> dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
>>> because I have any inside knowledge), it looks like the assertion of
>>> "RST#" in the ps8640 runtime resume handler seems like it's not
>>> allowed to happen after dsi_mgr_bridge_power_on()
>>>
>>> Recently, Dave Stevenson's series landed allowing bridges some control
>>> over pre_enable ordering. The meaty commit for our purposes is commit
>>> 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
>>> bridge init order"). As documented by that series, if a bridge doesn't
>>> set "pre_enable_prev_first" then we should use the old ordering.
>>>
>>> Now that we have the commit ("drm/bridge: tc358762: Set
>>> pre_enable_prev_first") we can go back to the old ordering, which also
>>> allows us to remove the ps8640 special case.
>>>
>>> One last note is that even without reverting commit 7d8e9a90509f
>>> ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
>>> revert the ps8640 special case and try it out then it doesn't seem to
>>> fail anymore. I spent time bisecting / debugging this and it turns out
>>> to be mostly luck, so we still want this patch to make sure it's
>>> solid. Specifically the reason it sorta works these days is because
>>> we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
>>> "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
>>> implemented means that we actually power the bridge chip up just a wee
>>> bit earlier and then the bridge happens to stay on because of
>>> autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
>>>
>>> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>> Changes in v2:
>>> - Don't fold dsi_mgr_bridge_power_on() back into dsi_mgr_bridge_pre_enable()
>>>
>>>    drivers/gpu/drm/msm/dsi/dsi_manager.c | 38 +--------------------------
>>>    1 file changed, 1 insertion(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> index 1bbac72dad35..2197a54b9b96 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
>>>    #define IS_SYNC_NEEDED()    (msm_dsim_glb.is_sync_needed)
>>>    #define IS_MASTER_DSI_LINK(id)      (msm_dsim_glb.master_dsi_link_id == id)
>>>
>>> -#ifdef CONFIG_OF
>>> -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
>>> -{
>>> -     struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
>>> -
>>> -     /*
>>> -      * If the next bridge in the chain is the Parade ps8640 bridge chip
>>> -      * then don't power on early since it seems to violate the expectations
>>> -      * of the firmware that the bridge chip is running.
>>> -      *
>>> -      * NOTE: this is expected to be a temporary special case. It's expected
>>> -      * that we'll eventually have a framework that allows the next level
>>> -      * bridge to indicate whether it needs us to power on before it or
>>> -      * after it. When that framework is in place then we'll use it and
>>> -      * remove this special case.
>>> -      */
>>> -     return !(next_bridge && next_bridge->of_node &&
>>> -              of_device_is_compatible(next_bridge->of_node, "parade,ps8640"));
>>> -}
>>> -#else
>>> -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
>>> -{
>>> -     return true;
>>> -}
>>> -#endif
>>> -
>>>    static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
>>>    {
>>>        return msm_dsim_glb.dsi[id];
>>> @@ -265,12 +239,6 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>>>        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;
>>>
>>
>> Why are these two checks removed?
> 
> After this patch there is now one caller to this function and the one
> caller does those exact same two checks immediately before calling
> this function. Thus, they no longer do anything useful.
> 
> -Doug

Ack, understood. dsi_mgr_bridge_pre_enable() has the same checks. Hence,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>