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

[Dmitry Baryshkov]

On 23/02/2022 20:21, Kuogee Hsieh wrote:
> 
> On 2/18/2022 6:22 PM, Dmitry Baryshkov wrote:
>> On Sat, 19 Feb 2022 at 03:55, Stephen Boyd <swboyd at 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 at 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 at 0:edp_panel: No display modes
>>>>> [   36.687272] panel-edp soc at 0:edp_panel: No display modes
>>>>> [   40.593709] panel-edp soc at 0:edp_panel: No display modes
>>>>> [   40.600285] panel-edp soc at 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?

In the panel device node.

Can you please share it too?

> 
>                          msm_edp: edp at 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 at 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>;
>                                          };
>                                  };
>                          };
> 


-- 
With best wishes
Dmitry