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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On Wed, 23 Feb 2022 at 21:27, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> 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";

I'd assume that the panel is supported by the patch
https://patchwork.kernel.org/project/linux-arm-msm/patch/1644494255-6632-5-git-send-email-quic_sbillaka@quicinc.com/
and the compatible value is just an old value.
Provided that the panel description defines modes, I'd ask for some
debug from the panel_edp_get_modes(). At least let's see why
panel_edp_get_non_edid_modes() / panel_edp_get_display_modes() returns
no modes.

Regarding the ddc bus, if you have separate i2c bus connected to this
panel, the ddc-i2c-bus = <&i2c_N>; property should go to this device
node.

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



-- 
With best wishes
Dmitry