[Freedreno] [RFC PATCH v2 1/5] drm/msm/dp: fix panel bridge attachment
Abhinav Kumar
quic_abhinavk at quicinc.com
Fri Feb 25 17:11:50 UTC 2022
On 2/25/2022 1:04 AM, Dmitry Baryshkov wrote:
> On Fri, 25 Feb 2022 at 07:45, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 2/24/2022 8:22 PM, Dmitry Baryshkov wrote:
>>> On Fri, 25 Feb 2022 at 05:01, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2/24/2022 12:41 PM, Dmitry Baryshkov wrote:
>>>>> On Thu, 24 Feb 2022 at 21:25, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2/18/2022 6:26 PM, Dmitry Baryshkov wrote:
>>>>>>> On 19/02/2022 02:56, Stephen Boyd wrote:
>>>>>>>> Quoting Dmitry Baryshkov (2022-02-11 14:40:02)
>>>>>>>>> In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display
>>>>>>>>> enable and disable") the DP driver received a drm_bridge instance, which
>>>>>>>>> is always attached to the encoder as a root bridge. However it conflicts
>>>>>>>>> with the panel_bridge support for eDP panels. The panel bridge attaches
>>>>>>>>> to the encoder before the "dp" bridge has a chace to do so. Change
>>>>>>>>
>>>>>>>> s/chace/chance/
>>>>>>>>
>>>>>>>>> panel_bridge attachment to come after dp_bridge attachment.
>>>>>>>>
>>>>>>>> s/panel_bridge/panel bridge/ possibly? And maybe clarify that dp_bridge
>>>>>>>> is the "DP driver's drm_bridge instance created in
>>>>>>>> msm_dp_bridge_init()"?
>>>>>>>>
>>>>>>>> My understanding is that we want to pass the bridge created in
>>>>>>>> msm_dp_bridge_init() as the 'previous' bridge so that it attaches the
>>>>>>>> panel bridge to the output of the DP bridge that's attached to the
>>>>>>>> encoder.
>>>>>>>
>>>>>>> Thanks! I'll update the commit log when pushing the patches.
>>>>>>
>>>>>> Please correct if i am missing something here.
>>>>>>
>>>>>> You are right that the eDP panel's panel bridge attaches to the encoder
>>>>>> in dp_drm_connector_init() which happens before msm_dp_bridge_init() and
>>>>>> hence it will attach directly to the encoder.
>>>>>>
>>>>>> But we are talking about different encoders here. DP's dp_display has a
>>>>>> different encoder compared to the EDP's dp_display.
>>>>>
>>>>> The encoders are different. However both encoders use the same
>>>>> codepath, which includes dp_bridge. It controls dp_display by calling
>>>>> msm_dp_display_foo() functions.
>>>>>
>>>
>>>>>> So DP's bridge will still be attached to its encoder's root.
>>>>>
>>>>> There is one dp_bridge per each encoder. Consider sc8180x which has 3
>>>>> DP controllers (and thus 3 dp_bridges).
>>>>>
>>>>
>>>> Sorry, but I still didnt follow this.
>>>>
>>>> So for eDP, dp_drm_connector_init() will attach the panel_bridge
>>>> and then msm_dp_bridge_init() will add a drm_bridge.
>>>>
>>>> And yes in that case, the drm_bridge will be after the panel_bridge
>>>>
>>>> But since panel_bridge is at the root for eDP it should be okay.
>>>
>>> No. It is not.
>>> For both DP and eDP the chain should be:
>>> dpu_encoder -> dp_bridge -> external (panel) bridge, optional for DP
>>> -> [other bridges] -> connector
>>>
>>> Otherwise drm_bridge_chain_foo() functions would be called in the
>>> incorrect order.
>>
>> Agreed. For drm_bridge_chain_foo() ordering to be correct, for eDP chain
>> the order should be what you mentioned and panel_bridge should be at the
>> end ( should be the last one ).
>>
>> For the above reason,
>>
>> Reviewed-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>
>> I still didnt understand what gets affected by this for the
>> msm_dp_display_foo() functions you mentioned earlier and wanted to get
>> some clarity on that.
>
> They should be called at the correct time.
>
>>>
>>> Thus the dp_bridge should be attached directly to the encoder
>>> (drm_encoder) and panel_bridge should use dp_bridge as the 'previous'
>>> bridge.
>>>
>>
>> Agreed.
>>
>>> For example, for the DP port one can use a display-connector (which
>>> actually implements drm_bridge) as an external bridge to provide hpd
>>> or dp power GPIOs.
>>>
>>> Note, that the current dp_connector breaks layering. It makes calls
>>> directly into dp_display, not allowing external bridge (and other
>>> bridges) to override get_modes/mode_valid and other callbacks.
>>> Thus one of the next patches in series (the one that Kuogee had issues
>>> with) tries to replace the chain with the following one:
>>> dpu_encoder -> dp_bridge -> external (panel) bridge -> [other bridges]
>>> -> drm_bridge_connector.
>>>
>>>>
>>
>> So originally the plan was always that the DP connector layer intercepts
>> the call because panel-eDP file did not support reading of the EDID ( we
>> have not provided the aux bus ). So it was intended that we did not want
>> to goto the eDP panel to get the modes. Not an error but something which
>> we wanted to cleanup later when we moved to panel-eDP completely.
>
> panel_edp_get_modes() correctly handles this case and returns modes
> specified in the panel description. So the code should work even with
> panel-eDP and w/o the AUX bus.
>
If hard-coded modes are not present, it will fail in below case:
/*
* Add hard-coded panel modes. Don't call this if there are no timings
* and no modes (the generic edp-panel case) because it will clobber
* the display_info that was already set by drm_add_edid_modes().
*/
if (p->desc->num_timings || p->desc->num_modes)
num += panel_edp_get_non_edid_modes(p, connector);
else if (!num)
dev_warn(p->base.dev, "No display modes\n");
Thats exactly the error he was seeing.
>>
>> Till then we wanted the dp_connector to read the EDID and get the modes.
>>
>> So this was actually intended to happen till the point where we moved to
>> panel-eDP to get the modes.
>>
>> Hence what you have mentioned in your cover letter is right that the
>> chain was broken but there was no loss of functionality due to that today.
>>
>> Now that these changes are made, we can still goto panel-eDP file for
>> the modes because of the below change from Sankeerth where the mode is
>> hard-coded:
>>
>> https://patchwork.freedesktop.org/patch/473431/
>>
>> With this change cherry-picked it should work for kuogee. We will
>> re-test that with this change.
>
> I suppose he had both of the changes in the tree. Otherwise the
> panel_edp_get_modes() wouldn't be called.
No he did not.
That brings up another point.
Even Bjorn was relying on the DP connector's get_modes() for his eDP
panel if I am not mistaken.
Unless he makes an equivalent change for his panel OR supports reading
EDID in panel-edp, then he will hit the same error.
Given that your changes were only compile tested, lets wait for both
Kuogee and Bjorn to validate their resp setups.
>
>>>> Your commit text was mentioning about DP.
>>>>
>>>> For DP, for each controller in the catalog, we will call modeset_init()
>>>> which should skip this part for DP
>>>>
>>>> if (dp_display->panel_bridge) {
>>>> ret = drm_bridge_attach(dp_display->encoder,
>>>> dp_display->panel_bridge, NULL,
>>>
>>> as you see, NULL is incorrect. It should be a pointer to dp_bridge instead
>>>
>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>> if (ret < 0) {
>>>> DRM_ERROR("failed to attach panel bridge: %d\n", ret);
>>>> return ERR_PTR(ret);
>>>> }
>>>> }
>>>>
>>>> Followed by calling msm_dp_bridge_init() which will actually attach the
>>>> bridge:
>>>>
>>>> drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>
>>> And this bridge should be attached before the external bridge.
>>>
>>>>
>>>> Now, even for 3 DP controllers, this shall be true as there will be 3
>>>> separate encoders and 3 dp_displays and hence 3 drm_bridges.
>>>>
>>>> What am i missing here?
>>>>
>>>>>>
>>>>>> So what are we achieving with this change?
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display
>>>>>>>>> enable and disable")
>>>>>>>>> Cc: Kuogee Hsieh <quic_khsieh at quicinc.com>
>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>>>>>> ---
>>>>>>>>
>>>>>>>> Reviewed-by: Stephen Boyd <swboyd at chromium.org>
>>>>>>>>
>>>>>>>>> drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++----------
>>>>>>>>> 1 file changed, 11 insertions(+), 10 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
>>>>>>>>> b/drivers/gpu/drm/msm/dp/dp_drm.c
>>>>>>>>> index d4d360d19eba..26ef41a4c1b6 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
>>>>>>>>> @@ -169,16 +169,6 @@ struct drm_connector
>>>>>>>>> *dp_drm_connector_init(struct msm_dp *dp_display)
>>>>>>>>>
>>>>>>>>> drm_connector_attach_encoder(connector, dp_display->encoder);
>>>>>>>>>
>>>>>>>>> - if (dp_display->panel_bridge) {
>>>>>>>>> - ret = drm_bridge_attach(dp_display->encoder,
>>>>>>>>> - dp_display->panel_bridge, NULL,
>>>>>>>>> - DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>>>>>>> - if (ret < 0) {
>>>>>>>>> - DRM_ERROR("failed to attach panel bridge:
>>>>>>>>> %d\n", ret);
>>>>>>>>> - return ERR_PTR(ret);
>>>>>>>>> - }
>>>>>>>>> - }
>>>>>>>>> -
>>>>>>>>> return connector;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> @@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct
>>>>>>>>> msm_dp *dp_display, struct drm_devi
>>>>>>>>> return ERR_PTR(rc);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> + if (dp_display->panel_bridge) {
>>>>>>>>> + rc = drm_bridge_attach(dp_display->encoder,
>>>>>>>>> + dp_display->panel_bridge,
>>>>>>>>> bridge,
>>>>>>>>> + DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>>>>>>> + if (rc < 0) {
>>>>>>>>> + DRM_ERROR("failed to attach panel bridge:
>>>>>>>>> %d\n", rc);
>>>>>>>>> + drm_bridge_remove(bridge);
>>>>>>>>> + return ERR_PTR(rc);
>>>>>>>>> + }
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> return bridge;
>>>>>>>>
>>>>>>>> Not a problem with this patch, but what is this pointer used for? I see
>>>>>>>> it's assigned to priv->bridges and num_bridges is incremented but nobody
>>>>>>>> seems to look at that.
>>>>>>>
>>>>>>>
>>>>>>> That's on my todo list. to remove connectors array and to destroy
>>>>>>> created bridges during drm device destruction.
>>>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>
>
>
More information about the dri-devel
mailing list