Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
On 3/16/2022 9:45 AM, Sankeerth Billakanti (QUIC) wrote:
Subject: Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with
drm_bridge_connector
Date: Wed, 23 Feb 2022 16:40:56 -0800
From: Kuogee Hsieh
To: Stephen Boyd , Dmitry Baryshkov
CC: Abhinav Kumar , Bjorn Andersson
, Rob Clark , Sean Paul
, David Airlie , Daniel Vetter
, linux-arm-...@vger.kernel.org, dri-
de...@lists.freedesktop.org, freedr...@lists.freedesktop.org
On 2/23/2022 1:33 PM, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2022-02-23 10:27:26)
On 2/23/2022 10:22 AM, Dmitry Baryshkov wrote:
On 23/02/2022 20:21, Kuogee Hsieh wrote:
In the panel device node.
Can you please share it too?
{
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 = <_pwm 3 65535>;
power-supply = <_power_supply>;
enable-gpio = <_gpios 7 GPIO_ACTIVE_HIGH>;
pinctrl-names = "default";
pinctrl-0 = <_pwm_default>;
};
edp_panel: edp_panel {
compatible = "sharp_lq140m1jw46";
Is that supposed to be sharp,lq140m1jw46 with a comma instead of an
underscore?
Stephen,
This is our internal branch which does not have patches up to date yet.
I will cherry-pick newer edp related patches which are under review now to
re test it.
Tested-by: Sankeerth Billakanti
Reviewed-by: Abhinav Kumar
Detect returned eDP not connected because the panel power was not on and
mode_valid was failing because the DP mode_valid is different from eDP
RE: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
> Subject: Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with
> drm_bridge_connector
> Date: Wed, 23 Feb 2022 16:40:56 -0800
> From: Kuogee Hsieh
> To: Stephen Boyd , Dmitry Baryshkov
>
> CC: Abhinav Kumar , Bjorn Andersson
> , Rob Clark , Sean Paul
> , David Airlie , Daniel Vetter
> , linux-arm-...@vger.kernel.org, dri-
> de...@lists.freedesktop.org, freedr...@lists.freedesktop.org
>
>
> On 2/23/2022 1:33 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-02-23 10:27:26)
> >> On 2/23/2022 10:22 AM, Dmitry Baryshkov wrote:
> >>> On 23/02/2022 20:21, Kuogee Hsieh wrote:
> >>>
> >>> In the panel device node.
> >>>
> >>> Can you please share it too?
> >>
> >> {
> >> 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 = <_pwm 3 65535>;
> >> power-supply = <_power_supply>;
> >> enable-gpio = <_gpios 7 GPIO_ACTIVE_HIGH>;
> >>
> >> pinctrl-names = "default";
> >> pinctrl-0 = <_pwm_default>;
> >> };
> >>
> >> edp_panel: edp_panel {
> >> compatible = "sharp_lq140m1jw46";
> > Is that supposed to be sharp,lq140m1jw46 with a comma instead of an
> > underscore?
>
> Stephen,
>
> This is our internal branch which does not have patches up to date yet.
>
> I will cherry-pick newer edp related patches which are under review now to
> re test it.
Tested-by: Sankeerth Billakanti
Detect returned eDP not connected because the panel power was not on and
mode_valid was failing because the DP mode_valid is different from eDP
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
On 2/23/2022 1:33 PM, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2022-02-23 10:27:26)
On 2/23/2022 10:22 AM, Dmitry Baryshkov wrote:
On 23/02/2022 20:21, Kuogee Hsieh wrote:
In the panel device node.
Can you please share it too?
{
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 = <_pwm 3 65535>;
power-supply = <_power_supply>;
enable-gpio = <_gpios 7 GPIO_ACTIVE_HIGH>;
pinctrl-names = "default";
pinctrl-0 = <_pwm_default>;
};
edp_panel: edp_panel {
compatible = "sharp_lq140m1jw46";
Is that supposed to be sharp,lq140m1jw46 with a comma instead of an
underscore?
Stephen,
This is our internal branch which does not have patches up to date yet.
I will cherry-pick newer edp related patches which are under review now
to re test it.
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
Quoting Kuogee Hsieh (2022-02-23 10:27:26)
>
> On 2/23/2022 10:22 AM, Dmitry Baryshkov wrote:
> > On 23/02/2022 20:21, Kuogee Hsieh wrote:
> >
> > In the panel device node.
> >
> > Can you please share it too?
>
>
> {
> 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 = <_pwm 3 65535>;
> power-supply = <_power_supply>;
> enable-gpio = <_gpios 7 GPIO_ACTIVE_HIGH>;
>
> pinctrl-names = "default";
> pinctrl-0 = <_pwm_default>;
> };
>
> edp_panel: edp_panel {
> compatible = "sharp_lq140m1jw46";
Is that supposed to be sharp,lq140m1jw46 with a comma instead of an
underscore?
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
On Wed, 23 Feb 2022 at 21:27, Kuogee Hsieh 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 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
> >> 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?
>
>
> {
> 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 = <_pwm 3 65535>;
> power-supply = <_power_supply>;
> enable-gpio = <_gpios 7 GPIO_ACTIVE_HIGH>;
>
> pinctrl-names = "default";
> pinctrl-0 = <_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_sbill...@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 = <_N>; property should go to this device
node.
> pinctrl-names = "default";
> pinctrl-0 = <_hot_plug_det>,
> <_panel_power_default>;
>
> power-supply = <_power_supply>;
> backlight = <_backlight>;
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
> port@0 {
> reg = <0>;
> edp_panel_in: endpoint {
> remote-endpoint = <_out>;
> };
> };
> };
> };
> };
>
>
> >
> >>
> >> msm_edp: edp@aea {
> >> compatible = "qcom,sc7280-edp";
> >>
> >> reg = <0 0xaea 0 0x200>,
> >><0 0xaea0200 0 0x200>,
> >><0 0xaea0400 0 0xc00>,
> >><0 0xaea1000 0 0x400>;
> >>
> >>
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
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 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
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?
{
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 = <_pwm 3 65535>;
power-supply = <_power_supply>;
enable-gpio = <_gpios 7 GPIO_ACTIVE_HIGH>;
pinctrl-names = "default";
pinctrl-0 = <_pwm_default>;
};
edp_panel: edp_panel {
compatible = "sharp_lq140m1jw46";
pinctrl-names = "default";
pinctrl-0 = <_hot_plug_det>,
<_panel_power_default>;
power-supply = <_power_supply>;
backlight = <_backlight>;
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
edp_panel_in: endpoint {
remote-endpoint = <_out>;
};
};
};
};
};
msm_edp: edp@aea {
compatible = "qcom,sc7280-edp";
reg = <0 0xaea 0 0x200>,
<0 0xaea0200 0 0x200>,
<0 0xaea0400 0 0xc00>,
<0 0xaea1000 0 0x400>;
interrupt-parent = <>;
interrupts = <14>;
clocks = < RPMH_CXO_CLK>,
< GCC_EDP_CLKREF_EN>,
<
DISP_CC_MDSS_AHB_CLK>,
<
DISP_CC_MDSS_EDP_AUX_CLK>,
<
DISP_CC_MDSS_EDP_LINK_CLK>,
<
DISP_CC_MDSS_EDP_LINK_INTF_CLK>,
<
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 = <
DISP_CC_MDSS_EDP_LINK_CLK_SRC>,
< DISP_CC_MDSS_EDP_PIXEL_CLK_SRC>;
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
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 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
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?
msm_edp: edp@aea {
compatible = "qcom,sc7280-edp";
reg = <0 0xaea 0 0x200>,
<0 0xaea0200 0 0x200>,
<0 0xaea0400 0 0xc00>,
<0 0xaea1000 0 0x400>;
interrupt-parent = <>;
interrupts = <14>;
clocks = < RPMH_CXO_CLK>,
< GCC_EDP_CLKREF_EN>,
< DISP_CC_MDSS_AHB_CLK>,
<
DISP_CC_MDSS_EDP_AUX_CLK>,
<
DISP_CC_MDSS_EDP_LINK_CLK>,
<
DISP_CC_MDSS_EDP_LINK_INTF_CLK>,
<
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 = <
DISP_CC_MDSS_EDP_LINK_CLK_SRC>,
<
DISP_CC_MDSS_EDP_PIXEL_CLK_SRC>;
assigned-clock-parents = <_phy 0>,
<_phy 1>;
phys = <_phy>;
phy-names = "dp";
operating-points-v2 = <_opp_table>;
power-domains = < 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 = <_intf5_out>;
};
};
};
edp_opp_table: opp-table {
compatible =
"operating-points-v2";
opp-16000 {
opp-hz = /bits/ 64
<16000>;
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
On 2/18/2022 6:22 PM, Dmitry Baryshkov wrote:
On Sat, 19 Feb 2022 at 03:55, Stephen Boyd 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
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@aea {
compatible = "qcom,sc7280-edp";
reg = <0 0xaea 0 0x200>,
<0 0xaea0200 0 0x200>,
<0 0xaea0400 0 0xc00>,
<0 0xaea1000 0 0x400>;
interrupt-parent = <>;
interrupts = <14>;
clocks = < RPMH_CXO_CLK>,
< GCC_EDP_CLKREF_EN>,
< DISP_CC_MDSS_AHB_CLK>,
<
DISP_CC_MDSS_EDP_AUX_CLK>,
<
DISP_CC_MDSS_EDP_LINK_CLK>,
<
DISP_CC_MDSS_EDP_LINK_INTF_CLK>,
<
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 = <
DISP_CC_MDSS_EDP_LINK_CLK_SRC>,
<
DISP_CC_MDSS_EDP_PIXEL_CLK_SRC>;
assigned-clock-parents = <_phy 0>,
<_phy 1>;
phys = <_phy>;
phy-names = "dp";
operating-points-v2 = <_opp_table>;
power-domains = < 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 = <_intf5_out>;
};
};
};
edp_opp_table: opp-table {
compatible = "operating-points-v2";
opp-16000 {
opp-hz = /bits/ 64
<16000>;
required-opps =
<_opp_low_svs>;
};
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
On Sat, 19 Feb 2022 at 03:55, Stephen Boyd 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 > > > > > > 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? -- With best wishes Dmitry
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
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 > > > > 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. > - either num_timing or num_modes in your panel desc. >
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
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
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?
- either num_timing or num_modes in your panel desc.
---
drivers/gpu/drm/msm/dp/dp_display.c | 22 +++---
drivers/gpu/drm/msm/dp/dp_drm.c | 113 ++--
2 files changed, 52 insertions(+), 83 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 45f9a912ecc5..59e5e5b8e5b4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1501,6 +1501,17 @@ int msm_dp_modeset_init(struct msm_dp
*dp_display, struct drm_device *dev,
dp_display->encoder = encoder;
+ dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
+ if (IS_ERR(dp_display->bridge)) {
+ ret = PTR_ERR(dp_display->bridge);
+ DRM_DEV_ERROR(dev->dev,
+ "failed to create dp bridge: %d\n", ret);
+ dp_display->bridge = NULL;
+ return ret;
+ }
+
+ priv->bridges[priv->num_bridges++] = dp_display->bridge;
+
dp_display->connector = dp_drm_connector_init(dp_display);
if (IS_ERR(dp_display->connector)) {
ret = PTR_ERR(dp_display->connector);
@@ -1514,17 +1525,6 @@ int msm_dp_modeset_init(struct msm_dp
*dp_display, struct drm_device *dev,
priv->connectors[priv->num_connectors++] = dp_display->connector;
- dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
- if (IS_ERR(dp_display->bridge)) {
- ret = PTR_ERR(dp_display->bridge);
- DRM_DEV_ERROR(dev->dev,
- "failed to create dp bridge: %d\n", ret);
- dp_display->bridge = NULL;
- return ret;
- }
-
- priv->bridges[priv->num_bridges++] = dp_display->bridge;
-
return 0;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
b/drivers/gpu/drm/msm/dp/dp_drm.c
index 80f59cf99089..57800b865fe6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include "msm_drv.h"
@@ -20,24 +21,16 @@ struct msm_dp_bridge {
#define to_dp_display(x) container_of((x), struct msm_dp_bridge,
bridge)
-struct dp_connector {
- struct drm_connector base;
- struct msm_dp *dp_display;
-};
-#define to_dp_connector(x) container_of(x, struct dp_connector, base)
-
/**
- * dp_connector_detect - callback to determine if connector is connected
- * @conn: Pointer to drm connector structure
- * @force: Force detect setting from drm framework
- * Returns: Connector 'is connected' status
+ * dp_bridge_detect - callback to determine if connector is connected
+ * @bridge: Pointer to drm bridge structure
+ * Returns: Bridge's 'is connected' status
*/
-static enum drm_connector_status dp_connector_detect(struct
drm_connector *conn,
- bool force)
+static enum drm_connector_status dp_bridge_detect(struct drm_bridge
*bridge)
{
struct msm_dp *dp;
- dp = to_dp_connector(conn)->dp_display;
+ dp = to_dp_display(bridge)->dp_display;
DRM_DEBUG_DP("is_connected = %s\n",
(dp->is_connected) ? "true" : "false");
@@ -47,11 +40,12 @@ static enum drm_connector_status
dp_connector_detect(struct drm_connector *conn,
}
/**
- * dp_connector_get_modes - callback to add drm modes via
drm_mode_probed_add()
+ * dp_bridge_get_modes - callback to add drm modes via
drm_mode_probed_add()
+ * @bridge: Poiner to drm bridge
* @connector: Pointer to drm connector structure
* Returns: Number of modes added
*/
-static int dp_connector_get_modes(struct drm_connector *connector)
+static int dp_bridge_get_modes(struct drm_bridge *bridge, struct
drm_connector *connector)
{
int rc = 0;
struct msm_dp *dp;
@@ -61,7 +55,7 @@ static int dp_connector_get_modes(struct
drm_connector *connector)
if (!connector)
return 0;
- dp = to_dp_connector(connector)->dp_display;
+ dp = to_dp_display(bridge)->dp_display;
dp_mode =
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
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
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
Thanks for testing this series!
Could you please post the result of `modetest -c` after this patch?
---
drivers/gpu/drm/msm/dp/dp_display.c | 22 +++---
drivers/gpu/drm/msm/dp/dp_drm.c | 113 ++--
2 files changed, 52 insertions(+), 83 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 45f9a912ecc5..59e5e5b8e5b4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1501,6 +1501,17 @@ int msm_dp_modeset_init(struct msm_dp
*dp_display, struct drm_device *dev,
dp_display->encoder = encoder;
+ dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
+ if (IS_ERR(dp_display->bridge)) {
+ ret = PTR_ERR(dp_display->bridge);
+ DRM_DEV_ERROR(dev->dev,
+ "failed to create dp bridge: %d\n", ret);
+ dp_display->bridge = NULL;
+ return ret;
+ }
+
+ priv->bridges[priv->num_bridges++] = dp_display->bridge;
+
dp_display->connector = dp_drm_connector_init(dp_display);
if (IS_ERR(dp_display->connector)) {
ret = PTR_ERR(dp_display->connector);
@@ -1514,17 +1525,6 @@ int msm_dp_modeset_init(struct msm_dp
*dp_display, struct drm_device *dev,
priv->connectors[priv->num_connectors++] = dp_display->connector;
- dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
- if (IS_ERR(dp_display->bridge)) {
- ret = PTR_ERR(dp_display->bridge);
- DRM_DEV_ERROR(dev->dev,
- "failed to create dp bridge: %d\n", ret);
- dp_display->bridge = NULL;
- return ret;
- }
-
- priv->bridges[priv->num_bridges++] = dp_display->bridge;
-
return 0;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
b/drivers/gpu/drm/msm/dp/dp_drm.c
index 80f59cf99089..57800b865fe6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include "msm_drv.h"
@@ -20,24 +21,16 @@ struct msm_dp_bridge {
#define to_dp_display(x) container_of((x), struct msm_dp_bridge,
bridge)
-struct dp_connector {
- struct drm_connector base;
- struct msm_dp *dp_display;
-};
-#define to_dp_connector(x) container_of(x, struct dp_connector, base)
-
/**
- * dp_connector_detect - callback to determine if connector is connected
- * @conn: Pointer to drm connector structure
- * @force: Force detect setting from drm framework
- * Returns: Connector 'is connected' status
+ * dp_bridge_detect - callback to determine if connector is connected
+ * @bridge: Pointer to drm bridge structure
+ * Returns: Bridge's 'is connected' status
*/
-static enum drm_connector_status dp_connector_detect(struct
drm_connector *conn,
- bool force)
+static enum drm_connector_status dp_bridge_detect(struct drm_bridge
*bridge)
{
struct msm_dp *dp;
- dp = to_dp_connector(conn)->dp_display;
+ dp = to_dp_display(bridge)->dp_display;
DRM_DEBUG_DP("is_connected = %s\n",
(dp->is_connected) ? "true" : "false");
@@ -47,11 +40,12 @@ static enum drm_connector_status
dp_connector_detect(struct drm_connector *conn,
}
/**
- * dp_connector_get_modes - callback to add drm modes via
drm_mode_probed_add()
+ * dp_bridge_get_modes - callback to add drm modes via
drm_mode_probed_add()
+ * @bridge: Poiner to drm bridge
* @connector: Pointer to drm connector structure
* Returns: Number of modes added
*/
-static int dp_connector_get_modes(struct drm_connector *connector)
+static int dp_bridge_get_modes(struct drm_bridge *bridge, struct
drm_connector *connector)
{
int rc = 0;
struct msm_dp *dp;
@@ -61,7 +55,7 @@ static int dp_connector_get_modes(struct
drm_connector *connector)
if (!connector)
return 0;
- dp = to_dp_connector(connector)->dp_display;
+ dp = to_dp_display(bridge)->dp_display;
dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL);
if (!dp_mode)
@@ -102,18 +96,20 @@ static int dp_connector_get_modes(struct
drm_connector *connector)
}
/**
- * dp_connector_mode_valid - callback to determine if specified mode
is valid
- * @connector: Pointer to drm connector structure
+ * dp_bridge_mode_valid - callback to determine if specified mode
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
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
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
---
drivers/gpu/drm/msm/dp/dp_display.c | 22 +++---
drivers/gpu/drm/msm/dp/dp_drm.c | 113 ++--
2 files changed, 52 insertions(+), 83 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 45f9a912ecc5..59e5e5b8e5b4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1501,6 +1501,17 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
struct drm_device *dev,
dp_display->encoder = encoder;
+ dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
+ if (IS_ERR(dp_display->bridge)) {
+ ret = PTR_ERR(dp_display->bridge);
+ DRM_DEV_ERROR(dev->dev,
+ "failed to create dp bridge: %d\n", ret);
+ dp_display->bridge = NULL;
+ return ret;
+ }
+
+ priv->bridges[priv->num_bridges++] = dp_display->bridge;
+
dp_display->connector = dp_drm_connector_init(dp_display);
if (IS_ERR(dp_display->connector)) {
ret = PTR_ERR(dp_display->connector);
@@ -1514,17 +1525,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
struct drm_device *dev,
priv->connectors[priv->num_connectors++] = dp_display->connector;
- dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
- if (IS_ERR(dp_display->bridge)) {
- ret = PTR_ERR(dp_display->bridge);
- DRM_DEV_ERROR(dev->dev,
- "failed to create dp bridge: %d\n", ret);
- dp_display->bridge = NULL;
- return ret;
- }
-
- priv->bridges[priv->num_bridges++] = dp_display->bridge;
-
return 0;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 80f59cf99089..57800b865fe6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include "msm_drv.h"
@@ -20,24 +21,16 @@ struct msm_dp_bridge {
#define to_dp_display(x) container_of((x), struct msm_dp_bridge, bridge)
-struct dp_connector {
- struct drm_connector base;
- struct msm_dp *dp_display;
-};
-#define to_dp_connector(x) container_of(x, struct dp_connector, base)
-
/**
- * dp_connector_detect - callback to determine if connector is connected
- * @conn: Pointer to drm connector structure
- * @force: Force detect setting from drm framework
- * Returns: Connector 'is connected' status
+ * dp_bridge_detect - callback to determine if connector is connected
+ * @bridge: Pointer to drm bridge structure
+ * Returns: Bridge's 'is connected' status
*/
-static enum drm_connector_status dp_connector_detect(struct drm_connector
*conn,
- bool force)
+static enum drm_connector_status dp_bridge_detect(struct drm_bridge *bridge)
{
struct msm_dp *dp;
- dp = to_dp_connector(conn)->dp_display;
+ dp = to_dp_display(bridge)->dp_display;
DRM_DEBUG_DP("is_connected = %s\n",
(dp->is_connected) ? "true" : "false");
@@ -47,11 +40,12 @@ static enum drm_connector_status dp_connector_detect(struct
drm_connector *conn,
}
/**
- * dp_connector_get_modes - callback to add drm modes via drm_mode_probed_add()
+ * dp_bridge_get_modes - callback to add drm modes via drm_mode_probed_add()
+ * @bridge: Poiner to drm bridge
* @connector: Pointer to drm connector structure
* Returns: Number of modes added
*/
-static int dp_connector_get_modes(struct drm_connector *connector)
+static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector
*connector)
{
int rc = 0;
struct msm_dp *dp;
@@ -61,7 +55,7 @@ static int dp_connector_get_modes(struct drm_connector
*connector)
if (!connector)
return 0;
- dp = to_dp_connector(connector)->dp_display;
+ dp = to_dp_display(bridge)->dp_display;
dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL);
if (!dp_mode)
@@ -102,18 +96,20 @@ static int dp_connector_get_modes(struct drm_connector
*connector)
}
/**
- * dp_connector_mode_valid - callback to determine if specified mode is valid
- * @connector: Pointer to drm connector structure
+ * dp_bridge_mode_valid - callback to determine if
[RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
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
---
drivers/gpu/drm/msm/dp/dp_display.c | 22 +++---
drivers/gpu/drm/msm/dp/dp_drm.c | 113 ++--
2 files changed, 52 insertions(+), 83 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 45f9a912ecc5..59e5e5b8e5b4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1501,6 +1501,17 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
struct drm_device *dev,
dp_display->encoder = encoder;
+ dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
+ if (IS_ERR(dp_display->bridge)) {
+ ret = PTR_ERR(dp_display->bridge);
+ DRM_DEV_ERROR(dev->dev,
+ "failed to create dp bridge: %d\n", ret);
+ dp_display->bridge = NULL;
+ return ret;
+ }
+
+ priv->bridges[priv->num_bridges++] = dp_display->bridge;
+
dp_display->connector = dp_drm_connector_init(dp_display);
if (IS_ERR(dp_display->connector)) {
ret = PTR_ERR(dp_display->connector);
@@ -1514,17 +1525,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
struct drm_device *dev,
priv->connectors[priv->num_connectors++] = dp_display->connector;
- dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
- if (IS_ERR(dp_display->bridge)) {
- ret = PTR_ERR(dp_display->bridge);
- DRM_DEV_ERROR(dev->dev,
- "failed to create dp bridge: %d\n", ret);
- dp_display->bridge = NULL;
- return ret;
- }
-
- priv->bridges[priv->num_bridges++] = dp_display->bridge;
-
return 0;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 80f59cf99089..57800b865fe6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include "msm_drv.h"
@@ -20,24 +21,16 @@ struct msm_dp_bridge {
#define to_dp_display(x) container_of((x), struct msm_dp_bridge, bridge)
-struct dp_connector {
- struct drm_connector base;
- struct msm_dp *dp_display;
-};
-#define to_dp_connector(x) container_of(x, struct dp_connector, base)
-
/**
- * dp_connector_detect - callback to determine if connector is connected
- * @conn: Pointer to drm connector structure
- * @force: Force detect setting from drm framework
- * Returns: Connector 'is connected' status
+ * dp_bridge_detect - callback to determine if connector is connected
+ * @bridge: Pointer to drm bridge structure
+ * Returns: Bridge's 'is connected' status
*/
-static enum drm_connector_status dp_connector_detect(struct drm_connector
*conn,
- bool force)
+static enum drm_connector_status dp_bridge_detect(struct drm_bridge *bridge)
{
struct msm_dp *dp;
- dp = to_dp_connector(conn)->dp_display;
+ dp = to_dp_display(bridge)->dp_display;
DRM_DEBUG_DP("is_connected = %s\n",
(dp->is_connected) ? "true" : "false");
@@ -47,11 +40,12 @@ static enum drm_connector_status dp_connector_detect(struct
drm_connector *conn,
}
/**
- * dp_connector_get_modes - callback to add drm modes via drm_mode_probed_add()
+ * dp_bridge_get_modes - callback to add drm modes via drm_mode_probed_add()
+ * @bridge: Poiner to drm bridge
* @connector: Pointer to drm connector structure
* Returns: Number of modes added
*/
-static int dp_connector_get_modes(struct drm_connector *connector)
+static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector
*connector)
{
int rc = 0;
struct msm_dp *dp;
@@ -61,7 +55,7 @@ static int dp_connector_get_modes(struct drm_connector
*connector)
if (!connector)
return 0;
- dp = to_dp_connector(connector)->dp_display;
+ dp = to_dp_display(bridge)->dp_display;
dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL);
if (!dp_mode)
@@ -102,18 +96,20 @@ static int dp_connector_get_modes(struct drm_connector
*connector)
}
/**
- * dp_connector_mode_valid - callback to determine if specified mode is valid
- * @connector: Pointer to drm connector structure
+ * dp_bridge_mode_valid - callback to determine if specified mode is valid
+ * @bridge: Pointer to drm bridge structure
+ * @info: display info
* @mode: Pointer to drm mode structure
* Returns: Validity status for specified mode
*/
-static enum drm_mode_status dp_connector_mode_valid(
- struct drm_connector *connector,
- struct drm_display_mode *mode)
+static enum drm_mode_status dp_bridge_mode_valid(
+ struct drm_bridge *bridge,
