Re: [Freedreno] [PATCH v1 0/3] Add support for next gen eDP driver on SnapDragon
On 2021-05-05 15:31, Dmitry Baryshkov wrote: Hi, On Wed, 5 May 2021 at 11:17, Sankeerth Billakanti wrote: These patches add support for the next generation eDP driver on SnapDragon with dpu support. The existing eDP driver cannot support the new eDP hardware. So, to maintain backward compatibility, the older eDP driver is moved to v200 folder and the new generation eDP driver is added in the v510 folder. What exactly does this version correspond to? I assume that v510 corresponds to sdmshrike/sc8180x. Is it right? [Sankeerth] This is for sc7280. Is it really so specific, or just v2/v5 would be enough? Not to mention that this is the MDP/ version, while other blocks tend to use block-specific versions/ids. [Sankeerth] I can rename it as edp-v1 and edp-v2. Edp v1 is very old chip and there is considerable HW delta between v1 and v2. So, we want to separate the driver. We followed similar model for DPU driver where, MDP4, MDP5 and DPU have separate folders. EDP v1 belongs to MDP4 generation. Also, how much does it differ from the current DP core supported via drivers/gpu/drm/msm/dp ? [Sankeerth] eDP is a native controller like DP but does not have audio, content protection and interoperability requirement. Upstream already supports eDP as a new interface driver found here: drivers/gpu/drm/msm/edp. I wanted to add the new controller driver as part of that folder. First two patches did not make it to the linux-msm, so I can not comment on each of the lines. [Sankeerth] I am also not sure why they did not make it to patchwork. I will repost them. However just my few cents (other reviewers might disagree though): - I see little benefit in renaming the folders just for the sake of renaming. You can put your code in drivers/gpu/drm/msm/edp-v510, if you really insist on that. Note that for all other (even incompatible) hardware types we still use single level of folders. - Also I see that significant parts of code (e.g. AUX, bridge, connector, maybe more) are just c&p of old edp code pieces. Please share the code instead of duplicating it. [Sankeerth] It is a baseline driver. As we add more features, it will considerably deviate a lot. The effort seems to be very high to maintain the common portion of code as I expect a lot of deviation. - Please consider updating register definitions in xml form and then providing both changed xml files (to mesa project (?)) and generated headers into the kernel. [Sankeerth] I followed what was done in the DP driver at /drivers/gpu/drm/msm/dp. I need to explore the xml approach to generate the register definitions. - Please consider using clk_bulk_* functions instead of using dss_module_power. I'm going to send a patchset reworking current users to use the generic clk_bulk_* function family. [Sankeerth] I will explore and rebase after your patch is available. - In generic, this eDP clock handling seems to match closely DP clocks handling (with all the name comparison, etc). Consider moving this to a generic piece of code - PHY seems to be a version of QMP PHY. Please use it, like it was done for the DP itself. There is support for combined USB+DP PHYs (both v3 and v4), so it should be possible to extend that for eDP. [Sankeerth] The DP phy is a combophy which supports both usb and dp phy concurrently, unlike eDP phy which is specific to only the eDP controller in sc7280. So, I implemented the edp phy sequences in the same folder. These are baseline changes with which we can enable display. The new eDP controller can also support additional features such as backlight control, PSR etc. which will be enabled in subsequent patch series. Summary of changes: DPU driver interface to the new eDP v510 display driver. New generation eDP controller and phy driver implementation. A common interface to choose enable the required eDP driver. Sankeerth Billakanti (3): drm/msm/edp: support multiple generations of edp hardware drm/msm/edp: add support for next gen edp drm/msm/disp/dpu1: add support for edp encoder drivers/gpu/drm/msm/Makefile | 19 +- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |7 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 33 + drivers/gpu/drm/msm/edp/edp.c | 198 --- drivers/gpu/drm/msm/edp/edp.h | 78 - drivers/gpu/drm/msm/edp/edp.xml.h | 380 - drivers/gpu/drm/msm/edp/edp_aux.c | 264 drivers/gpu/drm/msm/edp/edp_bridge.c | 111 -- drivers/gpu/drm/msm/edp/edp_common.c | 38 + drivers/gpu/drm/msm/edp/edp_common.h | 47 + drivers/gpu/drm/msm/edp/edp_connector.c | 132 -- drivers/gpu/drm/msm/edp/edp_ctrl.c| 1375 -- drivers/gpu/drm/msm/edp/edp_phy.c | 98 -- drivers/gpu/drm/msm/edp/v200/edp.xml.h| 380 + drivers/gpu/drm/msm/edp/v200/edp_v200.c | 210 ++
Re: [Freedreno] [PATCH v1 0/3] Add support for next gen eDP driver on SnapDragon
On 2021-05-06 20:32, Rob Clark wrote: On Wed, May 5, 2021 at 11:47 PM wrote: On 2021-05-05 15:31, Dmitry Baryshkov wrote: > Hi, > > On Wed, 5 May 2021 at 11:17, Sankeerth Billakanti > wrote: >> >> These patches add support for the next generation eDP driver on >> SnapDragon >> with dpu support. The existing eDP driver cannot support the new eDP >> hardware. So, to maintain backward compatibility, the older eDP driver >> is >> moved to v200 folder and the new generation eDP driver is added in >> the v510 folder. > > What exactly does this version correspond to? > I assume that v510 corresponds to sdmshrike/sc8180x. Is it right? [Sankeerth] This is for sc7280. > Is it really so specific, or just v2/v5 would be enough? Not to > mention that this is the MDP/ version, while other blocks tend to use > block-specific versions/ids. [Sankeerth] I can rename it as edp-v1 and edp-v2. Edp v1 is very old chip and there is considerable HW delta between v1 and v2. So, we want to separate the driver. We followed similar model for DPU driver where, MDP4, MDP5 and DPU have separate folders. EDP v1 belongs to MDP4 generation. Bjorn brought up the idea of just dropping the existing drm/msm/edp.. since the efforts to upstream the platform it worked on (8084?) fizzled out, I don't think there is any device which uses it. But it does sound like edp is a subset of the the newer dp driver, so seems sort of like the better approach would be to add edp support to dp. I believe Bjorn has something based on this approach which is working for sc8280 (although not sure if it is in shape to post patches yet) BR, -R Hi Rob, I will explore to integrate native eDP driver as part of DP driver. Will follow up with new patchsets. Hi Dmitry, I will move the eDP phy to qmp drivers folder in the new patchsets so that it can reuse the dp core driver. Sankeerth > > Also, how much does it differ from the current DP core supported via > drivers/gpu/drm/msm/dp ? [Sankeerth] eDP is a native controller like DP but does not have audio, content protection and interoperability requirement. Upstream already supports eDP as a new interface driver found here: drivers/gpu/drm/msm/edp. I wanted to add the new controller driver as part of that folder. > > First two patches did not make it to the linux-msm, so I can not > comment on each of the lines. [Sankeerth] I am also not sure why they did not make it to patchwork. I will repost them. > However just my few cents (other reviewers might disagree though): > > - I see little benefit in renaming the folders just for the sake of > renaming. You can put your code in drivers/gpu/drm/msm/edp-v510, if > you really insist on that. Note that for all other (even incompatible) > hardware types we still use single level of folders. > > - Also I see that significant parts of code (e.g. AUX, bridge, > connector, maybe more) are just c&p of old edp code pieces. Please > share the code instead of duplicating it. [Sankeerth] It is a baseline driver. As we add more features, it will considerably deviate a lot. The effort seems to be very high to maintain the common portion of code as I expect a lot of deviation. > > - Please consider updating register definitions in xml form and then > providing both changed xml files (to mesa project (?)) and generated > headers into the kernel. [Sankeerth] I followed what was done in the DP driver at /drivers/gpu/drm/msm/dp. I need to explore the xml approach to generate the register definitions. > > - Please consider using clk_bulk_* functions instead of using > dss_module_power. I'm going to send a patchset reworking current users > to use the generic clk_bulk_* function family. [Sankeerth] I will explore and rebase after your patch is available. > > - In generic, this eDP clock handling seems to match closely DP clocks > handling (with all the name comparison, etc). Consider moving this to > a generic piece of code > > - PHY seems to be a version of QMP PHY. Please use it, like it was > done for the DP itself. There is support for combined USB+DP PHYs > (both v3 and v4), so it should be possible to extend that for eDP. [Sankeerth] The DP phy is a combophy which supports both usb and dp phy concurrently, unlike eDP phy which is specific to only the eDP controller in sc7280. So, I implemented the edp phy sequences in the same folder. > > >> These are baseline changes with which we can enable display. The new >> eDP >> controller can also support additional features such as backlight >> control, >> PSR etc. which will be enabled in subsequent patch series. >> >> Summary of changes: >> DPU driver interface to the new eDP v510 display driver. >> New generation eDP controller and phy driver implementation. >> A common interface to choose enable the required eDP driver. >> >> Sankeerth Billakanti (3): >> drm/msm/edp: support multiple generations of edp hardware >> drm/msm/edp: add support for next gen edp >> drm/msm/disp/dpu1: add support for edp encoder
Re: [Freedreno] [PATCH v1 1/2] drm/msm/dp: Add support for SC7280 eDP
On 2021-08-12 06:11, Stephen Boyd wrote:
Quoting Sankeerth Billakanti (2021-08-11 17:08:01)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index b131fd37..1096c44 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -856,9 +856,9 @@ static const struct dpu_intf_cfg sm8150_intf[] = {
};
static const struct dpu_intf_cfg sc7280_intf[] = {
- INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 0, 24,
INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
+ INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, 1, 24,
INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24,
INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
- INTF_BLK("intf_5", INTF_5, 0x39000, INTF_EDP, 0, 24,
INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
+ INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, 0, 24,
INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
};
/*
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index d2569da..06d5a2d 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1244,7 +1244,9 @@ static int dp_ctrl_link_train(struct
dp_ctrl_private *ctrl,
struct dp_cr_status *cr, int *training_step)
{
int ret = 0;
+ u8 *dpcd = ctrl->panel->dpcd;
u8 encoding = DP_SET_ANSI_8B10B;
+ u8 ssc = 0, assr = 0;
Please don't initialize to zero and then overwrite it before using it.
It hides usage before actual initialization bugs.
Okay. I will change it.
struct dp_link_info link_info = {0};
dp_ctrl_config_ctrl(ctrl);
@@ -1254,9 +1256,21 @@ static int dp_ctrl_link_train(struct
dp_ctrl_private *ctrl,
link_info.capabilities = DP_LINK_CAP_ENHANCED_FRAMING;
dp_aux_link_configure(ctrl->aux, &link_info);
+
+ if (dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5) {
+ ssc = DP_SPREAD_AMP_0_5;
+ drm_dp_dpcd_write(ctrl->aux, DP_DOWNSPREAD_CTRL, &ssc,
1);
+ }
+
drm_dp_dpcd_write(ctrl->aux, DP_MAIN_LINK_CHANNEL_CODING_SET,
&encoding, 1);
+ if (dpcd[DP_EDP_CONFIGURATION_CAP] &
DP_ALTERNATE_SCRAMBLER_RESET_CAP) {
+ assr = DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
+ drm_dp_dpcd_write(ctrl->aux, DP_EDP_CONFIGURATION_SET,
+ &assr, 1);
+ }
+
ret = dp_ctrl_link_train_1(ctrl, cr, training_step);
if (ret) {
DRM_ERROR("link training #1 failed. ret=%d\n", ret);
@@ -1328,9 +1342,11 @@ static int
dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
struct dp_io *dp_io = &ctrl->parser->io;
struct phy *phy = dp_io->phy;
struct phy_configure_opts_dp *opts_dp = &dp_io->phy_opts.dp;
+ u8 *dpcd = ctrl->panel->dpcd;
const?
Okay. I will change to const u8 *dpcd at all the required places.
opts_dp->lanes = ctrl->link->link_params.num_lanes;
opts_dp->link_rate = ctrl->link->link_params.rate / 100;
+ opts_dp->ssc = dpcd[DP_MAX_DOWNSPREAD] &
DP_MAX_DOWNSPREAD_0_5;
dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link",
ctrl->link->link_params.rate *
1000);
@@ -1760,6 +1776,9 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
ctrl->link->link_params.num_lanes =
ctrl->panel->link_info.num_lanes;
ctrl->dp_ctrl.pixel_rate =
ctrl->panel->dp_mode.drm_mode.clock;
+ if (ctrl->dp_ctrl.pixel_rate == 0)
+ return -EINVAL;
+
Why are we enabling the stream with a zero pixel clk?
This was an error condition I encountered while bringing up sc7280. HPD
processing was delayed and I got a commit with pixel clock = 0. I will
recheck why this is happening.
DRM_DEBUG_DP("rate=%d, num_lanes=%d, pixel_rate=%d\n",
ctrl->link->link_params.rate,
ctrl->link->link_params.num_lanes,
ctrl->dp_ctrl.pixel_rate);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index ee5bf64..a772290 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -117,8 +117,36 @@ struct dp_display_private {
struct dp_audio *audio;
};
+struct msm_dp_config {
+ phys_addr_t io_start[3];
+ size_t num_dp;
+};
+
+static const struct msm_dp_config sc7180_dp_cfg = {
+ .io_start = { 0x0ae9 },
+ .num_dp = 1,
+};
+
+static const struct msm_dp_config sc8180x_dp_cfg = {
+ .io_start = { 0xae9, 0xae98000, 0 },
+ .num_dp = 3,
+};
+
+static const struct msm_dp_config sc8180x_edp_cfg = {
+ .io_start = { 0, 0, 0xae9a000 },
+ .num_dp = 3,
+};
+
+static const struct msm_dp_config sc7280_edp_cfg = {
+ .io_start = { 0xaea, 0 },
+
Re: [Freedreno] [PATCH v2 1/2] drm/msm/dp: Add support for SC7280 eDP
Hi Stephen,
On 2021-10-21 23:32, Stephen Boyd wrote:
Quoting Sankeerth Billakanti (2021-10-20 05:14:10)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 62e75dc..9fea49c 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1238,9 +1240,21 @@ static int dp_ctrl_link_train(struct
dp_ctrl_private *ctrl,
link_info.capabilities = DP_LINK_CAP_ENHANCED_FRAMING;
dp_aux_link_configure(ctrl->aux, &link_info);
+
+ if (dpcd[DP_MAX_DOWNSPREAD] & DP_MAX_DOWNSPREAD_0_5) {
Please add a static inline macro in include/drm/drm_dp_helper.h that
makes this more readable. Something similar to drm_dp_is_branch() but
with a human readable replacement for "is_branch". Maybe drm_dp_ssc()?
Okay, I will add a macro, drm_dp_max_downspread (to be consistent with
the spec and other macros in the file) in drm_dp_helper.h file.
+ ssc = DP_SPREAD_AMP_0_5;
+ drm_dp_dpcd_write(ctrl->aux, DP_DOWNSPREAD_CTRL, &ssc,
1);
+ }
+
drm_dp_dpcd_write(ctrl->aux, DP_MAIN_LINK_CHANNEL_CODING_SET,
&encoding, 1);
+ if (dpcd[DP_EDP_CONFIGURATION_CAP] &
DP_ALTERNATE_SCRAMBLER_RESET_CAP) {
And this one already has a helper,
drm_dp_alternate_scrambler_reset_cap().
Okay, I will use that.
+ assr = DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
+ drm_dp_dpcd_write(ctrl->aux, DP_EDP_CONFIGURATION_SET,
+ &assr, 1);
+ }
+
ret = dp_ctrl_link_train_1(ctrl, training_step);
if (ret) {
DRM_ERROR("link training #1 failed. ret=%d\n", ret);
@@ -1312,9 +1326,11 @@ static int
dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
struct dp_io *dp_io = &ctrl->parser->io;
struct phy *phy = dp_io->phy;
struct phy_configure_opts_dp *opts_dp = &dp_io->phy_opts.dp;
+ const u8 *dpcd = ctrl->panel->dpcd;
opts_dp->lanes = ctrl->link->link_params.num_lanes;
opts_dp->link_rate = ctrl->link->link_params.rate / 100;
+ opts_dp->ssc = dpcd[DP_MAX_DOWNSPREAD] &
DP_MAX_DOWNSPREAD_0_5;
dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link",
ctrl->link->link_params.rate *
1000);
@@ -1406,7 +1422,7 @@ void dp_ctrl_host_deinit(struct dp_ctrl
*dp_ctrl)
static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
{
- u8 *dpcd = ctrl->panel->dpcd;
+ const u8 *dpcd = ctrl->panel->dpcd;
/*
* For better interop experience, used a fixed NVID=0x8000
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index c867745..c16311b 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -144,8 +144,16 @@ static const struct msm_dp_config sc8180x_dp_cfg
= {
.num_descs = 3,
};
+static const struct msm_dp_config sc7280_dp_cfg = {
+ .descs = (struct msm_dp_desc[]) {
const
Will add it.
+ { .io_start = 0x0aea, .connector_type =
DRM_MODE_CONNECTOR_eDP },
+ },
+ .num_descs = 1,
+};
+
static const struct of_device_id dp_dt_match[] = {
{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
+ { .compatible = "qcom,sc7280-edp", .data = &sc7280_dp_cfg },
{ .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
{ .compatible = "qcom,sc8180x-edp", .data = &sc8180x_dp_cfg },
{}
@@ -1440,7 +1448,7 @@ void msm_dp_irq_postinstall(struct msm_dp
*dp_display)
dp_hpd_event_setup(dp);
- dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
+ dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1);
This has no explanation. What is it?
Will add explanation for it as a comment.
