Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector

[Kuogee Hsieh <quic_khsieh@quicinc.com>]

On 2/18/2022 6:22 PM, Dmitry Baryshkov wrote:
> On Sat, 19 Feb 2022 at 03:55, Stephen Boyd <swboyd@chromium.org> wrote:
>> Quoting Dmitry Baryshkov (2022-02-18 14:32:53)
>>> On 19/02/2022 00:31, Kuogee Hsieh wrote:
>>>> On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote:
>>>>> There is little point in having both connector and root bridge
>>>>> implementation in the same driver. Move connector's functionality to the
>>>>> bridge to let next bridge in chain to override it.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> This patch break primary (edp) display
>>>>
>>>> -- right half of screen garbled
>>>>
>>>> -- screen shift vertically
>>>>
>>>> below are error messages seen --
>>>>
>>>> [   36.679216] panel-edp soc@0:edp_panel: No display modes
>>>> [   36.687272] panel-edp soc@0:edp_panel: No display modes
>>>> [   40.593709] panel-edp soc@0:edp_panel: No display modes
>>>> [   40.600285] panel-edp soc@0:edp_panel: No display modes
>>> So, before the patch the drm core was getting modes from the
>>> drm_connector (which means, modes from drm driver itself). With this
>>> patch the panel-edp tries to get modes.
>>>
>>> Could you please check, why panel_edp_get_modes() fails? Assuming that
>>> you use platform panel-edp binding (rather than 'edp-panel') could you
>>> please check you have either of the following:
>>> - ddc bus for EDID?
>> I don't see anywhere where the ddc pointer is set for the dp bridge in
>> msm_dp_bridge_init(). Is that required though? I'd think simple panel is
>> still being used here so reading EDID isn't required.
> I meant the 'ddc-i2c-bus' property for the corresponding eDP panel.
>
>>> - either num_timing or num_modes in your panel desc.
> After reading the panel-edp's code I don't have another cause for
> panel_edp_get_modes(). It should either have a DDC bus specified using
> the mentioned device tree property, or it should have specified the
> timings.
>
> Kuogee, which platform were you using when testing this patch? Could
> you please share the dts fragment?

I cherry-picked your patches on top of our internal release which is 
usually have some (or many) patches behind msm-next.

where is "ddc-i2c-bus" located?

                         msm_edp: edp@aea0000 {
                                 compatible = "qcom,sc7280-edp";

                                 reg = <0 0xaea0000 0 0x200>,
                                       <0 0xaea0200 0 0x200>,
                                       <0 0xaea0400 0 0xc00>,
                                       <0 0xaea1000 0 0x400>;

                                 interrupt-parent = <&mdss>;
                                 interrupts = <14>;

                                 clocks = <&rpmhcc RPMH_CXO_CLK>,
                                          <&gcc GCC_EDP_CLKREF_EN>,
                                          <&dispcc DISP_CC_MDSS_AHB_CLK>,
                                          <&dispcc 
DISP_CC_MDSS_EDP_AUX_CLK>,
                                          <&dispcc 
DISP_CC_MDSS_EDP_LINK_CLK>,
                                          <&dispcc 
DISP_CC_MDSS_EDP_LINK_INTF_CLK>,
                                          <&dispcc 
DISP_CC_MDSS_EDP_PIXEL_CLK>;
                                 clock-names = "core_xo",
                                               "core_ref",
                                               "core_iface",
                                               "core_aux",
                                               "ctrl_link",
                                               "ctrl_link_iface",
                                               "stream_pixel";
                                 #clock-cells = <1>;
                                 assigned-clocks = <&dispcc 
DISP_CC_MDSS_EDP_LINK_CLK_SRC>,
                                                   <&dispcc 
DISP_CC_MDSS_EDP_PIXEL_CLK_SRC>;
                                 assigned-clock-parents = <&edp_phy 0>, 
<&edp_phy 1>;

                                 phys = <&edp_phy>;
                                 phy-names = "dp";

                                 operating-points-v2 = <&edp_opp_table>;
                                 power-domains = <&rpmhpd SC7280_CX>;

                                 #address-cells = <1>;
                                 #size-cells = <0>;

                                 status = "disabled";

                                 ports {
                                         #address-cells = <1>;
                                         #size-cells = <0>;
                                         port@0 {
                                                 reg = <0>;
                                                 edp_in: endpoint {
remote-endpoint = <&dpu_intf5_out>;
                                                 };
                                         };
                                 };

                             edp_opp_table: opp-table {
                                         compatible = "operating-points-v2";

                                         opp-160000000 {
                                                 opp-hz = /bits/ 64 
<160000000>;
                                                 required-opps = 
<&rpmhpd_opp_low_svs>;
                                         };

                                         opp-270000000 {
                                                 opp-hz = /bits/ 64 
<270000000>;
                                                 required-opps = 
<&rpmhpd_opp_svs>;
                                         };

                                         opp-540000000 {
                                                 opp-hz = /bits/ 64 
<540000000>;
                                                 required-opps = 
<&rpmhpd_opp_nom>;
                                         };

                                         opp-810000000 {
                                                 opp-hz = /bits/ 64 
<810000000>;
                                                 required-opps = 
<&rpmhpd_opp_nom>;
                                         };
                                 };
                         };