| 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
| ||
|
Date: Tue, 25 Jun 2024 12:28:37 -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/25/2024 12:26 PM, Abhinav Kumar wrote:
>
>
> 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.
>
Correction, number of controllers is < number of PHYs.
> 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