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

[Kuogee Hsieh <quic_khsieh@quicinc.com>]

On 2/23/2022 10:22 AM, Dmitry Baryshkov wrote:
> 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@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?
>
> In the panel device node.
>
> Can you please share it too?


&soc {
         edp_power_supply: edp_power {
                 compatible = "regulator-fixed";
                 regulator-name = "edp_backlight_power";

                 regulator-always-on;
                 regulator-boot-on;
         };

         edp_backlight: edp_backlight {
                 compatible = "pwm-backlight";

                 pwms = <&pm8350c_pwm 3 65535>;
                 power-supply = <&edp_power_supply>;
                 enable-gpio = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>;

                 pinctrl-names = "default";
                 pinctrl-0 = <&backlight_pwm_default>;
         };

         edp_panel: edp_panel {
                 compatible = "sharp_lq140m1jw46";

                 pinctrl-names = "default";
                 pinctrl-0 = <&edp_hot_plug_det>, 
<&edp_panel_power_default>;

                 power-supply = <&edp_power_supply>;
                 backlight = <&edp_backlight>;

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


>
>>
>>                          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>;
>>                                          };
>>                                  };
>>                          };
>>
>
>