On Wed, 23 Feb 2022 at 21:27, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> >>>>>> 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@xxxxxxxxxxx/
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
