| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0ae0fddb-07f4-3eb9-5a62-0f7f15153452@quicinc.com>
Date: Tue, 25 Jun 2024 12:26:34 -0700
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Rob Clark
<robdclark@...il.com>, Sean Paul <sean@...rly.run>,
Marijn Suijten
<marijn.suijten@...ainline.org>,
David Airlie <airlied@...il.com>, Daniel
Vetter <daniel@...ll.ch>
CC: Bjorn Andersson <andersson@...nel.org>, <linux-arm-msm@...r.kernel.org>,
<dri-devel@...ts.freedesktop.org>, <freedreno@...ts.freedesktop.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC v2] drm/msm/dpu: Configure DP INTF/PHY selector
On 6/24/2024 6:39 PM, Abhinav Kumar wrote:
>
>
> On 6/13/2024 4:17 AM, Dmitry Baryshkov wrote:
>> From: Bjorn Andersson <andersson@...nel.org>
>>
>> Some platforms provides a mechanism for configuring the mapping between
>> (one or two) DisplayPort intfs and their PHYs.
>>
>> In particular SC8180X provides this functionality, without a default
>> configuration, resulting in no connection between its two external
>> DisplayPort controllers and any PHYs.
>>
>
> I have to cross-check internally about what makes it mandatory to
> program this only for sc8180xp. We were not programming this so far for
> any chipset and this register is present all the way from sm8150 till
> xe10100 and all the chipsets do not have a correct default value which
> makes me think whether this is required to be programmed.
>
> Will update this thread once I do.
>
Ok, I checked more. The reason this is mandatory for sc8180xp is the
number of controllers is greater than number of PHYs needing this to be
programmed. On all other chipsets its a 1:1 mapping.
I am fine with the change once the genmap comment is addressed.
>> The change implements the logic for optionally configuring which PHY
>> each of the DP INTFs should be connected to and marks the SC8180X DPU to
>> program 2 entries.
>>
>> For now the request is simply to program the mapping 1:1, any support
>> for alternative mappings is left until the use case arrise.
>>
>> Note that e.g. msm-4.14 unconditionally maps INTF 0 to PHY 0 on all
>> rlatforms, so perhaps this is needed in order to get DisplayPort working
>> on some other platforms as well.
>>
>> Signed-off-by: Bjorn Andersson <andersson@...nel.org>
>> Co-developed-by: Bjorn Andersson <andersson@...nel.org>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>> ---
>> Changes in v2:
>> - Removed entry from the catalog.
>> - Reworked the interface of dpu_hw_dp_phy_intf_sel(). Pass two entries
>> for the PHYs instead of three entries.
>> - It seems the register isn't present on sdm845, enabled the callback
>> only for DPU >= 5.x
>> - Added a comment regarding the data being platform-specific.
>> - Link to v1:
>> https://lore.kernel.org/r/20230612221047.1886709-1-quic_bjorande@quicinc.com
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 39
>> +++++++++++++++++++++++++++---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 18 ++++++++++++--
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h | 7 ++++++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 11 ++++++++-
>> 4 files changed, 69 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> index 05e48cf4ec1d..a11fdbefc8d2 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> @@ -231,8 +231,38 @@ static void dpu_hw_intf_audio_select(struct
>> dpu_hw_mdp *mdp)
>> DPU_REG_WRITE(c, HDMI_DP_CORE_SELECT, 0x1);
>> }
>> +static void dpu_hw_dp_phy_intf_sel(struct dpu_hw_mdp *mdp,
>> + enum dpu_dp_phy_sel phys[2])
>> +{
>> + struct dpu_hw_blk_reg_map *c = &mdp->hw;
>> + unsigned int intf;
>> + u32 sel = 0;
>> +
>> + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_INTF0, phys[0]);
>> + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_INTF1, phys[1]);
>> +
>> + for (intf = 0; intf < 2; intf++) {
>
> I wonder if ARRAY_SIZE(phys) is better here.
>
>> + switch (phys[intf]) {
>> + case DPU_DP_PHY_0:
>> + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_PHY0, intf + 1);
>> + break;
>> + case DPU_DP_PHY_1:
>> + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_PHY1, intf + 1);
>> + break;
>> + case DPU_DP_PHY_2:
>> + sel |= FIELD_PREP(MDP_DP_PHY_INTF_SEL_PHY2, intf + 1);
>> + break;
>> + default:
>> + /* ignore */
>> + break;
>> + }
>> + }
>> +
>> + DPU_REG_WRITE(c, MDP_DP_PHY_INTF_SEL, sel);
>> +}
>> +
>> static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
>> - unsigned long cap)
>> + unsigned long cap, const struct dpu_mdss_version *mdss_rev)
>> {
>> ops->setup_split_pipe = dpu_hw_setup_split_pipe;
>> ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl;
>> @@ -245,6 +275,9 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops
>> *ops,
>> ops->get_safe_status = dpu_hw_get_safe_status;
>> + if (mdss_rev->core_major_ver >= 5)
>> + ops->dp_phy_intf_sel = dpu_hw_dp_phy_intf_sel;
>> +
>> if (cap & BIT(DPU_MDP_AUDIO_SELECT))
>> ops->intf_audio_select = dpu_hw_intf_audio_select;
>> }
>> @@ -252,7 +285,7 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops
>> *ops,
>> struct dpu_hw_mdp *dpu_hw_mdptop_init(struct drm_device *dev,
>> const struct dpu_mdp_cfg *cfg,
>> void __iomem *addr,
>> - const struct dpu_mdss_cfg *m)
>> + const struct dpu_mdss_version *mdss_rev)
>> {
>> struct dpu_hw_mdp *mdp;
>> @@ -270,7 +303,7 @@ struct dpu_hw_mdp *dpu_hw_mdptop_init(struct
>> drm_device *dev,
>> * Assign ops
>> */
>> mdp->caps = cfg;
>> - _setup_mdp_ops(&mdp->ops, mdp->caps->features);
>> + _setup_mdp_ops(&mdp->ops, mdp->caps->features, mdss_rev);
>> return mdp;
>> }
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
>> index 6f3dc98087df..3a17e63b851c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h
>> @@ -67,6 +67,13 @@ struct dpu_vsync_source_cfg {
>> u32 vsync_source;
>> };
>> +enum dpu_dp_phy_sel {
>> + DPU_DP_PHY_NONE,
>> + DPU_DP_PHY_0,
>> + DPU_DP_PHY_1,
>> + DPU_DP_PHY_2,
>> +};
>> +
>> /**
>> * struct dpu_hw_mdp_ops - interface to the MDP TOP Hw driver functions
>> * Assumption is these functions will be called after clocks are
>> enabled.
>> @@ -125,6 +132,13 @@ struct dpu_hw_mdp_ops {
>> void (*get_safe_status)(struct dpu_hw_mdp *mdp,
>> struct dpu_danger_safe_status *status);
>> + /**
>> + * dp_phy_intf_sel - configure intf to phy mapping
>> + * @mdp: mdp top context driver
>> + * @phys: list of phys the DP interfaces should be connected to.
>> 0 disables the INTF.
>> + */
>> + void (*dp_phy_intf_sel)(struct dpu_hw_mdp *mdp, enum
>> dpu_dp_phy_sel phys[2]);
>> +
>> /**
>> * intf_audio_select - select the external interface for audio
>> * @mdp: mdp top context driver
>> @@ -148,12 +162,12 @@ struct dpu_hw_mdp {
>> * @dev: Corresponding device for devres management
>> * @cfg: MDP TOP configuration from catalog
>> * @addr: Mapped register io address of MDP
>> - * @m: Pointer to mdss catalog data
>> + * @mdss_rev: dpu core's major and minor versions
>> */
>> struct dpu_hw_mdp *dpu_hw_mdptop_init(struct drm_device *dev,
>> const struct dpu_mdp_cfg *cfg,
>> void __iomem *addr,
>> - const struct dpu_mdss_cfg *m);
>> + const struct dpu_mdss_version *mdss_rev);
>> void dpu_hw_mdp_destroy(struct dpu_hw_mdp *mdp);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
>> index 5acd5683d25a..f1acc04089af 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
>> @@ -60,6 +60,13 @@
>> #define MDP_WD_TIMER_4_LOAD_VALUE 0x448
>> #define DCE_SEL 0x450
>> +#define MDP_DP_PHY_INTF_SEL 0x460
>> +#define MDP_DP_PHY_INTF_SEL_INTF0 GENMASK(3, 0)
>> +#define MDP_DP_PHY_INTF_SEL_INTF1 GENMASK(6, 3)
>> +#define MDP_DP_PHY_INTF_SEL_PHY0 GENMASK(9, 6)
>> +#define MDP_DP_PHY_INTF_SEL_PHY1 GENMASK(12, 9)
>> +#define MDP_DP_PHY_INTF_SEL_PHY2 GENMASK(15, 12)
>
> These masks do not match the docs, the below ones are what I see:
>
> #define MDP_DP_PHY_INTF_SEL_INTF0 GENMASK(2, 0)
> #define MDP_DP_PHY_INTF_SEL_INTF1 GENMASK(5, 3)
> #define MDP_DP_PHY_INTF_SEL_PHY0 GENMASK(8, 6)
> #define MDP_DP_PHY_INTF_SEL_PHY1 GENMASK(11, 9)
> #define MDP_DP_PHY_INTF_SEL_PHY2 GENMASK(14, 12)
>
>> +
>> #define MDP_PERIPH_TOP0 MDP_WD_TIMER_0_CTL
>> #define MDP_PERIPH_TOP0_END CLK_CTRL3
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 1955848b1b78..9db5a784c92f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -1102,7 +1102,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>> dpu_kms->hw_mdp = dpu_hw_mdptop_init(dev,
>> dpu_kms->catalog->mdp,
>> dpu_kms->mmio,
>> - dpu_kms->catalog);
>> + dpu_kms->catalog->mdss_ver);
>> if (IS_ERR(dpu_kms->hw_mdp)) {
>> rc = PTR_ERR(dpu_kms->hw_mdp);
>> DPU_ERROR("failed to get hw_mdp: %d\n", rc);
>> @@ -1137,6 +1137,15 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>> goto err_pm_put;
>> }
>> + /*
>> + * We need to program DP <-> PHY relationship only for SC8180X.
>> If any
>> + * other platform requires the same kind of programming, or if
>> the INTF
>> + * <->DP relationship isn't static anymore, this needs to be
>> configured
>> + * through the DT.
>> + */
>> + if (of_device_is_compatible(dpu_kms->pdev->dev.of_node,
>> "qcom,sc8180x-dpu"))
>> + dpu_kms->hw_mdp->ops.dp_phy_intf_sel(dpu_kms->hw_mdp,
>> (unsigned int[]){ 1, 2, });
>> +
>> dpu_kms->hw_intr = dpu_hw_intr_init(dev, dpu_kms->mmio,
>> dpu_kms->catalog);
>> if (IS_ERR(dpu_kms->hw_intr)) {
>> rc = PTR_ERR(dpu_kms->hw_intr);
>>
>> ---
>> base-commit: 03d44168cbd7fc57d5de56a3730427db758fc7f6
>> change-id: 20240613-dp-phy-sel-1b06dc48ed73
>>
>> Best regards,
Powered by blists - more mailing lists