Re: [PATCH v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
Quoting Abhinav Kumar (2024-04-03 14:28:52) > > > On 4/3/2024 1:04 PM, Stephen Boyd wrote: > > Quoting Abhinav Kumar (2024-04-03 12:58:50) > >> > >> > >> On 4/3/2024 12:51 PM, Stephen Boyd wrote: > >>> Quoting Kuogee Hsieh (2024-03-29 12:50:35) > Currently qmp_combo_dp_power_on() always return 0 in regardless of > return value of cfg->configure_dp_phy(). This patch propagate > return value of cfg->configure_dp_phy() all the way back to caller. > >>> > >>> Is this found via code inspection or because the phy is failing to power > >>> on sometimes? I ask because I'm looking at a DP bug on Trogdor with > >>> chromeos' v6.6 based kernel and wondering if this is related. > >>> > >> > >> No, we actually hit an issue. This issue was originally reported as a > >> link training issue while bringing up DP on x1e80100. > >> > >> While debugging that we noticed that we should not have even proceeded > >> to link training because the PLL was not getting locked and it was > >> failing silently since there are no other error prints (and hence the > >> second part of the patch to improve the error logs), and we do not > >> return any error code from this driver, we could not catch the PLL > >> unlocked issue. > > > > Did link training succeed in that case and the screen was black? Also, > > did you figure out why the PLL failed to lock? I sometimes see reports > > now with an "Unexpected interrupt:" message from the DP driver and the > > interrupt is the PLL unlocked one (DP_INTR_PLL_UNLOCKED). > > > > No the link training had failed. > > Yes, root-cause was that the PLL registers were misconfigured in the > x1e80100 DP PHY for HBR2. Once we programmed the correct values it > worked. This was specific to x1e80100. Ah ok, so that's what the x1e80100 patch is about. > > Yes, Doug mentioned this to me on IRC that this issue is still there. > Surprising because I thought we had pushed > https://patchwork.freedesktop.org/patch/551847/ long ago and it was > fixed. It certainly did that time when I had tested this. I see it on v6.6 and it is also on v5.15.y (stable kernel) so that has been picked back. Somehow the aux interrupt is still happening though when the PLL isn't locked. Maybe that interrupt bit should be masked in most cases and only unmasked when something in the driver is going to care about it.
Re: [PATCH v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
On 4/3/2024 1:04 PM, Stephen Boyd wrote: Quoting Abhinav Kumar (2024-04-03 12:58:50) On 4/3/2024 12:51 PM, Stephen Boyd wrote: Quoting Kuogee Hsieh (2024-03-29 12:50:35) Currently qmp_combo_dp_power_on() always return 0 in regardless of return value of cfg->configure_dp_phy(). This patch propagate return value of cfg->configure_dp_phy() all the way back to caller. Is this found via code inspection or because the phy is failing to power on sometimes? I ask because I'm looking at a DP bug on Trogdor with chromeos' v6.6 based kernel and wondering if this is related. No, we actually hit an issue. This issue was originally reported as a link training issue while bringing up DP on x1e80100. While debugging that we noticed that we should not have even proceeded to link training because the PLL was not getting locked and it was failing silently since there are no other error prints (and hence the second part of the patch to improve the error logs), and we do not return any error code from this driver, we could not catch the PLL unlocked issue. Did link training succeed in that case and the screen was black? Also, did you figure out why the PLL failed to lock? I sometimes see reports now with an "Unexpected interrupt:" message from the DP driver and the interrupt is the PLL unlocked one (DP_INTR_PLL_UNLOCKED). No the link training had failed. Yes, root-cause was that the PLL registers were misconfigured in the x1e80100 DP PHY for HBR2. Once we programmed the correct values it worked. This was specific to x1e80100. Yes, Doug mentioned this to me on IRC that this issue is still there. Surprising because I thought we had pushed https://patchwork.freedesktop.org/patch/551847/ long ago and it was fixed. It certainly did that time when I had tested this. Also, is the call to phy_power_on() going to be checked in the DP driver? $ git grep -n phy_power_on -- drivers/gpu/drm/msm/dp/ drivers/gpu/drm/msm/dp/dp_ctrl.c:1453: phy_power_on(phy); Yes, this is a good point. We should also post the patch to add the error checking in DP driver to fail if phy_power_on fails. Sounds great, thanks.
Re: [PATCH v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
Quoting Abhinav Kumar (2024-04-03 12:58:50) > > > On 4/3/2024 12:51 PM, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2024-03-29 12:50:35) > >> Currently qmp_combo_dp_power_on() always return 0 in regardless of > >> return value of cfg->configure_dp_phy(). This patch propagate > >> return value of cfg->configure_dp_phy() all the way back to caller. > > > > Is this found via code inspection or because the phy is failing to power > > on sometimes? I ask because I'm looking at a DP bug on Trogdor with > > chromeos' v6.6 based kernel and wondering if this is related. > > > > No, we actually hit an issue. This issue was originally reported as a > link training issue while bringing up DP on x1e80100. > > While debugging that we noticed that we should not have even proceeded > to link training because the PLL was not getting locked and it was > failing silently since there are no other error prints (and hence the > second part of the patch to improve the error logs), and we do not > return any error code from this driver, we could not catch the PLL > unlocked issue. Did link training succeed in that case and the screen was black? Also, did you figure out why the PLL failed to lock? I sometimes see reports now with an "Unexpected interrupt:" message from the DP driver and the interrupt is the PLL unlocked one (DP_INTR_PLL_UNLOCKED). > > > Also, is the call to phy_power_on() going to be checked in > > the DP driver? > > > > $ git grep -n phy_power_on -- drivers/gpu/drm/msm/dp/ > > drivers/gpu/drm/msm/dp/dp_ctrl.c:1453: phy_power_on(phy); > > Yes, this is a good point. We should also post the patch to add the > error checking in DP driver to fail if phy_power_on fails. Sounds great, thanks.
Re: [PATCH v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
On 4/3/2024 12:51 PM, Stephen Boyd wrote: Quoting Kuogee Hsieh (2024-03-29 12:50:35) Currently qmp_combo_dp_power_on() always return 0 in regardless of return value of cfg->configure_dp_phy(). This patch propagate return value of cfg->configure_dp_phy() all the way back to caller. Is this found via code inspection or because the phy is failing to power on sometimes? I ask because I'm looking at a DP bug on Trogdor with chromeos' v6.6 based kernel and wondering if this is related. No, we actually hit an issue. This issue was originally reported as a link training issue while bringing up DP on x1e80100. While debugging that we noticed that we should not have even proceeded to link training because the PLL was not getting locked and it was failing silently since there are no other error prints (and hence the second part of the patch to improve the error logs), and we do not return any error code from this driver, we could not catch the PLL unlocked issue. Also, is the call to phy_power_on() going to be checked in the DP driver? $ git grep -n phy_power_on -- drivers/gpu/drm/msm/dp/ drivers/gpu/drm/msm/dp/dp_ctrl.c:1453: phy_power_on(phy); Yes, this is a good point. We should also post the patch to add the error checking in DP driver to fail if phy_power_on fails.
Re: [PATCH v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
Quoting Kuogee Hsieh (2024-03-29 12:50:35) > Currently qmp_combo_dp_power_on() always return 0 in regardless of > return value of cfg->configure_dp_phy(). This patch propagate > return value of cfg->configure_dp_phy() all the way back to caller. Is this found via code inspection or because the phy is failing to power on sometimes? I ask because I'm looking at a DP bug on Trogdor with chromeos' v6.6 based kernel and wondering if this is related. Also, is the call to phy_power_on() going to be checked in the DP driver? $ git grep -n phy_power_on -- drivers/gpu/drm/msm/dp/ drivers/gpu/drm/msm/dp/dp_ctrl.c:1453: phy_power_on(phy);
Re: [PATCH v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
On Wed, Apr 03, 2024 at 10:22:37AM -0700, Kuogee Hsieh wrote:
> Dmitry,
>
> Any more comments?
>
> On 3/29/2024 12:50 PM, Kuogee Hsieh wrote:
> > Currently qmp_combo_dp_power_on() always return 0 in regardless of
> > return value of cfg->configure_dp_phy(). This patch propagate
> > return value of cfg->configure_dp_phy() all the way back to caller.
> >
> > Changes in V3:
> > -- add v2 changes log
> >
> > Changes in V2:
> > -- add Fixes tag
> > -- add dev_err() to qmp_v3_configure_dp_phy()
> > -- add dev_err() to qmp_v4_configure_dp_phy()
> >
> > Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo
> > phy")
> > Signed-off-by: Kuogee Hsieh
> > Reviewed-by: Abhinav Kumar
> > ---
> > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 13 +
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > index 36632fa..513d99d 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > @@ -2343,8 +2343,10 @@ static int qmp_v3_configure_dp_phy(struct qmp_combo
> > *qmp)
> > writel(0x05, qmp->dp_dp_phy + QSERDES_V3_DP_PHY_TX2_TX3_LANE_CTL);
> > ret = qmp_combo_configure_dp_clocks(qmp);
> > - if (ret)
> > + if (ret) {
> > + dev_err(qmp->dev, "dp phy configure failed, err=%d\n", ret);
> > return ret;
> > + }
dev_err() calls are not related to the fix itself. Please split them to
a separate patch.
> > writel(0x04, qmp->dp_dp_phy + QSERDES_DP_PHY_AUX_CFG2);
> > writel(0x01, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
> > @@ -2519,8 +2521,10 @@ static int qmp_v4_configure_dp_phy(struct qmp_combo
> > *qmp)
> > int ret;
> > ret = qmp_v456_configure_dp_phy(qmp);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + dev_err(qmp->dev, "dp phy configure failed, err=%d\n", ret);
> > return ret;
> > + }
> > /*
> > * At least for 7nm DP PHY this has to be done after enabling link
> > @@ -2754,6 +2758,7 @@ static int qmp_combo_dp_power_on(struct phy *phy)
> > const struct qmp_phy_cfg *cfg = qmp->cfg;
> > void __iomem *tx = qmp->dp_tx;
> > void __iomem *tx2 = qmp->dp_tx2;
> > + int ret;
> > mutex_lock(&qmp->phy_mutex);
> > @@ -2766,11 +2771,11 @@ static int qmp_combo_dp_power_on(struct phy *phy)
> > cfg->configure_dp_tx(qmp);
> > /* Configure link rate, swing, etc. */
> > - cfg->configure_dp_phy(qmp);
> > + ret = cfg->configure_dp_phy(qmp);
> > mutex_unlock(&qmp->phy_mutex);
> > - return 0;
> > + return ret;
> > }
> > static int qmp_combo_dp_power_off(struct phy *phy)
--
With best wishes
Dmitry
Re: [PATCH v3] drm/msm/dp: assign correct DP controller ID to x1e80100 interface table
On Wed, 3 Apr 2024 at 20:24, Kuogee Hsieh wrote: > > Dmitry, > > any more comments? > No, it was picked by Abhinav for msm-fixes. -- With best wishes Dmitry
Re: [PATCH v3] drm/msm/dp: assign correct DP controller ID to x1e80100 interface table
Dmitry,
any more comments?
On 3/29/2024 12:46 PM, Kuogee Hsieh wrote:
At current x1e80100 interface table, interface #3 is wrongly
connected to DP controller #0 and interface #4 wrongly connected
to DP controller #2. Fix this problem by connect Interface #3 to
DP controller #0 and interface #4 connect to DP controller #1.
Also add interface #6, #7 and #8 connections to DP controller to
complete x1e80100 interface table.
Changs in V3:
-- add v2 changes log
Changs in V2:
-- add x1e80100 to subject
-- add Fixes
Fixes: e3b1f369db5a ("drm/msm/dpu: Add X1E80100 support")
Signed-off-by: Kuogee Hsieh
Reviewed-by: Abhinav Kumar
Reviewed-by: Abel Vesa
---
.../drm/msm/disp/dpu1/catalog/dpu_9_2_x1e80100.h | 34 --
1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_2_x1e80100.h
b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_2_x1e80100.h
index 9a9f709..a3e60ac 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_2_x1e80100.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_9_2_x1e80100.h
@@ -324,6 +324,7 @@ static const struct dpu_wb_cfg x1e80100_wb[] = {
},
};
+/* TODO: INTF 3, 8 and 7 are used for MST, marked as INTF_NONE for now */
static const struct dpu_intf_cfg x1e80100_intf[] = {
{
.name = "intf_0", .id = INTF_0,
@@ -358,8 +359,8 @@ static const struct dpu_intf_cfg x1e80100_intf[] = {
.name = "intf_3", .id = INTF_3,
.base = 0x37000, .len = 0x280,
.features = INTF_SC7280_MASK,
- .type = INTF_DP,
- .controller_id = MSM_DP_CONTROLLER_1,
+ .type = INTF_NONE,
+ .controller_id = MSM_DP_CONTROLLER_0, /* pair with intf_0 for
DP MST */
.prog_fetch_lines_worst_case = 24,
.intr_underrun = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 30),
.intr_vsync = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 31),
@@ -368,7 +369,7 @@ static const struct dpu_intf_cfg x1e80100_intf[] = {
.base = 0x38000, .len = 0x280,
.features = INTF_SC7280_MASK,
.type = INTF_DP,
- .controller_id = MSM_DP_CONTROLLER_2,
+ .controller_id = MSM_DP_CONTROLLER_1,
.prog_fetch_lines_worst_case = 24,
.intr_underrun = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 20),
.intr_vsync = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 21),
@@ -381,6 +382,33 @@ static const struct dpu_intf_cfg x1e80100_intf[] = {
.prog_fetch_lines_worst_case = 24,
.intr_underrun = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 22),
.intr_vsync = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 23),
+ }, {
+ .name = "intf_6", .id = INTF_6,
+ .base = 0x3A000, .len = 0x280,
+ .features = INTF_SC7280_MASK,
+ .type = INTF_DP,
+ .controller_id = MSM_DP_CONTROLLER_2,
+ .prog_fetch_lines_worst_case = 24,
+ .intr_underrun = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 17),
+ .intr_vsync = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 16),
+ }, {
+ .name = "intf_7", .id = INTF_7,
+ .base = 0x3b000, .len = 0x280,
+ .features = INTF_SC7280_MASK,
+ .type = INTF_NONE,
+ .controller_id = MSM_DP_CONTROLLER_2, /* pair with intf_6 for
DP MST */
+ .prog_fetch_lines_worst_case = 24,
+ .intr_underrun = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 18),
+ .intr_vsync = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 19),
+ }, {
+ .name = "intf_8", .id = INTF_8,
+ .base = 0x3c000, .len = 0x280,
+ .features = INTF_SC7280_MASK,
+ .type = INTF_NONE,
+ .controller_id = MSM_DP_CONTROLLER_1, /* pair with intf_4 for
DP MST */
+ .prog_fetch_lines_worst_case = 24,
+ .intr_underrun = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 12),
+ .intr_vsync = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 13),
},
};
Re: [PATCH v3] phy/qcom-qmp-combo: propagate correct return value at phy_power_on()
Dmitry,
Any more comments?
On 3/29/2024 12:50 PM, Kuogee Hsieh wrote:
Currently qmp_combo_dp_power_on() always return 0 in regardless of
return value of cfg->configure_dp_phy(). This patch propagate
return value of cfg->configure_dp_phy() all the way back to caller.
Changes in V3:
-- add v2 changes log
Changes in V2:
-- add Fixes tag
-- add dev_err() to qmp_v3_configure_dp_phy()
-- add dev_err() to qmp_v4_configure_dp_phy()
Fixes: 52e013d0bffa ("phy: qcom-qmp: Add support for DP in USB3+DP combo phy")
Signed-off-by: Kuogee Hsieh
Reviewed-by: Abhinav Kumar
---
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 36632fa..513d99d 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -2343,8 +2343,10 @@ static int qmp_v3_configure_dp_phy(struct qmp_combo *qmp)
writel(0x05, qmp->dp_dp_phy + QSERDES_V3_DP_PHY_TX2_TX3_LANE_CTL);
ret = qmp_combo_configure_dp_clocks(qmp);
- if (ret)
+ if (ret) {
+ dev_err(qmp->dev, "dp phy configure failed, err=%d\n", ret);
return ret;
+ }
writel(0x04, qmp->dp_dp_phy + QSERDES_DP_PHY_AUX_CFG2);
writel(0x01, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
@@ -2519,8 +2521,10 @@ static int qmp_v4_configure_dp_phy(struct qmp_combo *qmp)
int ret;
ret = qmp_v456_configure_dp_phy(qmp);
- if (ret < 0)
+ if (ret < 0) {
+ dev_err(qmp->dev, "dp phy configure failed, err=%d\n", ret);
return ret;
+ }
/*
* At least for 7nm DP PHY this has to be done after enabling link
@@ -2754,6 +2758,7 @@ static int qmp_combo_dp_power_on(struct phy *phy)
const struct qmp_phy_cfg *cfg = qmp->cfg;
void __iomem *tx = qmp->dp_tx;
void __iomem *tx2 = qmp->dp_tx2;
+ int ret;
mutex_lock(&qmp->phy_mutex);
@@ -2766,11 +2771,11 @@ static int qmp_combo_dp_power_on(struct phy *phy)
cfg->configure_dp_tx(qmp);
/* Configure link rate, swing, etc. */
- cfg->configure_dp_phy(qmp);
+ ret = cfg->configure_dp_phy(qmp);
mutex_unlock(&qmp->phy_mutex);
- return 0;
+ return ret;
}
static int qmp_combo_dp_power_off(struct phy *phy)
[linux-next:master] BUILD REGRESSION 727900b675b749c40ba1f6669c7ae5eb7eb8e837
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master branch HEAD: 727900b675b749c40ba1f6669c7ae5eb7eb8e837 Add linux-next specific files for 20240403 Error/Warning reports: https://lore.kernel.org/oe-kbuild-all/202404031246.aq5yr5ko-...@intel.com https://lore.kernel.org/oe-kbuild-all/202404031346.wpihnpyf-...@intel.com https://lore.kernel.org/oe-kbuild-all/202404032101.skzrxcwh-...@intel.com Error/Warning: (recently discovered and may have been fixed) fs/smb/client/file.c:728:12: warning: variable 'rc' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] mm/kasan/hw_tags.c:280:14: warning: assignment to 'struct vm_struct *' from 'int' makes pointer from integer without a cast [-Wint-conversion] mm/kasan/hw_tags.c:280:16: error: implicit declaration of function 'find_vm_area'; did you mean 'find_vma_prev'? [-Werror=implicit-function-declaration] mm/kasan/hw_tags.c:284:29: error: invalid use of undefined type 'struct vm_struct' riscv32-linux-ld: section .data LMA [001f9000,009465d7] overlaps section .text LMA [000a7e84,0177d68b] Unverified Error/Warning (likely false positive, please contact us if interested): fs/smb/client/file.c:619 serverclose_work() error: uninitialized symbol 'rc'. fs/smb/client/file.c:732 _cifsFileInfo_put() error: uninitialized symbol 'rc'. Error/Warning ids grouped by kconfigs: gcc_recent_errors |-- alpha-allnoconfig | |-- mm-mempool.c:warning:Function-parameter-or-struct-member-gfp_mask-not-described-in-mempool_create_node | `-- mm-mempool.c:warning:Function-parameter-or-struct-member-node_id-not-described-in-mempool_create_node |-- alpha-allyesconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | |-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size | |-- mm-mempool.c:warning:Function-parameter-or-struct-member-gfp_mask-not-described-in-mempool_create_node | `-- mm-mempool.c:warning:Function-parameter-or-struct-member-node_id-not-described-in-mempool_create_node |-- alpha-defconfig | |-- mm-mempool.c:warning:Function-parameter-or-struct-member-gfp_mask-not-described-in-mempool_create_node | `-- mm-mempool.c:warning:Function-parameter-or-struct-member-node_id-not-described-in-mempool_create_node |-- arm-allmodconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | |-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size | |-- mm-mempool.c:warning:Function-parameter-or-struct-member-gfp_mask-not-described-in-mempool_create_node | `-- mm-mempool.c:warning:Function-parameter-or-struct-member-node_id-not-described-in-mempool_create_node |-- arm-allyesconfig | |-- drivers-gpu-drm-imx-ipuv3-imx-ldb.c:error:_sel-directive-output-may-be-truncated-writing-bytes-into-a-region-of-size-between-and | |-- drivers-gpu-drm-nouveau-nouveau_backlight.c:error:d-directive-output-may-be-truncated-writing-between-and-bytes-into-a-region-of-size | |-- mm-mempool.c:warning:Function-parameter-or-struct-member-gfp_mask-not-described-in-mempool_create_node | `-- mm-mempool.c:warning:Function-parameter-or-struct-member-node_id-not-described-in-mempool_create_node |-- arm-aspeed_g5_defconfig | |-- mm-mempool.c:warning:Function-parameter-or-struct-member-gfp_mask-not-described-in-mempool_create_node | `-- mm-mempool.c:warning:Function-parameter-or-struct-member-node_id-not-described-in-mempool_create_node |-- arm-randconfig-001-20240403 | |-- mm-mempool.c:warning:Function-parameter-or-struct-member-gfp_mask-not-described-in-mempool_create_node | `-- mm-mempool.c:warning:Function-parameter-or-struct-member-node_id-not-described-in-mempool_create_node |-- arm-randconfig-002-20240403 | |-- mm-mempool.c:warning:Function-parameter-or-struct-member-gfp_mask-not-described-in-mempool_create_node | `-- mm-mempool.c:warning:Function-parameter-or-struct-member-node_id-not-described-in-mempool_create_node |-- arm-randconfig-004-20240403 | |-- mm-mempool.c:warning:Function-parameter-or-struct-member-gfp_mask-not-described-in-mempool_create_node | `-- mm-mempool.c:warning:Function-parameter-or-struct-member-node_id-not-described-in-mempool_create_node |-- arm-randconfig-r061-20240403 | |-- mm-mempool.c:warning:Function-parameter-or-struct-member-gfp_mask-not-described-in-mempool_create_node | `-- mm-mempool.c:warning:Function-parameter-or-struct-member-node_id-not-described-in-mempool_create_node |-- arm64-allnoconfig | |-- mm-mempool.c:warning:Function-parameter-or-struct-member-gfp_mask-not-described-in-mempool_create_node
Re: [PATCH v3 1/6] drm/msm/dpu: fix video mode DSC for DSI
On Wed, 3 Apr 2024 at 17:44, Jun Nie wrote:
>
> Dmitry Baryshkov 于2024年4月3日周三 17:57写道：
> >
> > On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote:
> > >
> > > From: Jonathan Marek
> > >
> > > Add necessary DPU timing and control changes for DSC to work with DSI
> > > video mode.
> > >
> > > Signed-off-by: Jonathan Marek
> > > Signed-off-by: Jun Nie
> > > ---
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 +
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 8
> > > 2 files changed, 17 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > index d9e7dbf0499c..c7b009a60b63 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > @@ -115,6 +115,15 @@ static void drm_mode_to_intf_timing_params(
> > > timing->h_front_porch = timing->h_front_porch >> 1;
> > > timing->hsync_pulse_width = timing->hsync_pulse_width >>
> > > 1;
> > > }
> > > +
> > > + /*
> > > +* for DSI, if compression is enabled, then divide the horizonal
> > > active
> > > +* timing parameters by compression ratio.
> > > +*/
> > > + if (phys_enc->hw_intf->cap->type != INTF_DP &&
> > > timing->compression_en) {
> > > + timing->width = timing->width / 3; /* XXX: don't assume
> > > 3:1 compression ratio */
> > > + timing->xres = timing->width;
> > > + }
> > > }
> > >
> > > static u32 get_horizontal_total(const struct dpu_hw_intf_timing_params
> > > *timing)
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > > index 965692ef7892..079efb48db05 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > > @@ -167,6 +167,14 @@ static void dpu_hw_intf_setup_timing_engine(struct
> > > dpu_hw_intf *ctx,
> > > intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
> > >
> > > data_width = p->width;
> > > + if (p->wide_bus_en && !dp_intf)
> > > + data_width = p->width >> 1;
> >
> > How is wide_bus relevant to the DSC case?
> > Is there a need for the Fixes tag?
>
> 48bit bus width should be used when DSC is enabled. Without the
> widebus configuration,
> a lot dsi error happens as below in DSC case.
> [ 206.275992] dsi_err_worker: status=4
Still, separate patch please.
>
> For the Fixes tag, the previous patch mentioned to enable the widebus
> mode for any DSC case. So it is fair to add the tag.
Yes, please.
> >
> > > +
> > > + if (p->compression_en)
> > > + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
> > > +
> > > + if (p->compression_en && dp_intf)
> > > + DPU_ERROR("missing adjustments for DSC+DP\n");
> > >
> > > hsync_data_start_x = hsync_start_x;
> > > hsync_data_end_x = hsync_start_x + data_width - 1;
> > >
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry
Re: [PATCH v3 1/6] drm/msm/dpu: fix video mode DSC for DSI
Dmitry Baryshkov 于2024年4月3日周三 17:57写道：
>
> On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote:
> >
> > From: Jonathan Marek
> >
> > Add necessary DPU timing and control changes for DSC to work with DSI
> > video mode.
> >
> > Signed-off-by: Jonathan Marek
> > Signed-off-by: Jun Nie
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 +
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 8
> > 2 files changed, 17 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > index d9e7dbf0499c..c7b009a60b63 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > @@ -115,6 +115,15 @@ static void drm_mode_to_intf_timing_params(
> > timing->h_front_porch = timing->h_front_porch >> 1;
> > timing->hsync_pulse_width = timing->hsync_pulse_width >> 1;
> > }
> > +
> > + /*
> > +* for DSI, if compression is enabled, then divide the horizonal
> > active
> > +* timing parameters by compression ratio.
> > +*/
> > + if (phys_enc->hw_intf->cap->type != INTF_DP &&
> > timing->compression_en) {
> > + timing->width = timing->width / 3; /* XXX: don't assume 3:1
> > compression ratio */
> > + timing->xres = timing->width;
> > + }
> > }
> >
> > static u32 get_horizontal_total(const struct dpu_hw_intf_timing_params
> > *timing)
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > index 965692ef7892..079efb48db05 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > @@ -167,6 +167,14 @@ static void dpu_hw_intf_setup_timing_engine(struct
> > dpu_hw_intf *ctx,
> > intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
> >
> > data_width = p->width;
> > + if (p->wide_bus_en && !dp_intf)
> > + data_width = p->width >> 1;
>
> How is wide_bus relevant to the DSC case?
> Is there a need for the Fixes tag?
48bit bus width should be used when DSC is enabled. Without the
widebus configuration,
a lot dsi error happens as below in DSC case.
[ 206.275992] dsi_err_worker: status=4
For the Fixes tag, the previous patch mentioned to enable the widebus
mode for any DSC case. So it is fair to add the tag.
>
> > +
> > + if (p->compression_en)
> > + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
> > +
> > + if (p->compression_en && dp_intf)
> > + DPU_ERROR("missing adjustments for DSC+DP\n");
> >
> > hsync_data_start_x = hsync_start_x;
> > hsync_data_end_x = hsync_start_x + data_width - 1;
> >
> > --
> > 2.34.1
> >
>
>
> --
> With best wishes
> Dmitry
Re: [PATCH v3 0/6] Add DSC support to DSI video panel
On Wed, 3 Apr 2024 at 17:27, Jun Nie wrote: > > Dmitry Baryshkov 于2024年4月3日周三 17:49写道： > > > > On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote: > > > > > > This is follow up update to Jonathan's patch set. > > > > > > Changes vs V2: > > > - Rebase to latest mainline. > > > - Drop the INTF_CFG2_DATA_HCTL_EN change as it is handled in > > > latest mainline code. > > > - Drop the bonded DSI patch as I do not have device to test it. > > > - Address comments from version 2. > > > > Which comments? "Adress comments" is the worst case of changelog. > Adopted. Will add more details in next version. > > > > Also, what do you consider as version 2? Jonathan Marek has only sent v1. > > It's wired. I see v2 in patch title of below link. Just notice that > there is v1 in the link address. > https://patchwork.freedesktop.org/patch/567518/?series=126430&rev=1 > Ack, I didn't remember that there was v2. Please excuse me then. -- With best wishes Dmitry
Re: [PATCH v3 0/6] Add DSC support to DSI video panel
Dmitry Baryshkov 于2024年4月3日周三 17:49写道： > > On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote: > > > > This is follow up update to Jonathan's patch set. > > > > Changes vs V2: > > - Rebase to latest mainline. > > - Drop the INTF_CFG2_DATA_HCTL_EN change as it is handled in > > latest mainline code. > > - Drop the bonded DSI patch as I do not have device to test it. > > - Address comments from version 2. > > Which comments? "Adress comments" is the worst case of changelog. Adopted. Will add more details in next version. > > Also, what do you consider as version 2? Jonathan Marek has only sent v1. It's wired. I see v2 in patch title of below link. Just notice that there is v1 in the link address. https://patchwork.freedesktop.org/patch/567518/?series=126430&rev=1 > > > > > Signed-off-by: Jun Nie > > --- > > Jonathan Marek (5): > > drm/msm/dpu: fix video mode DSC for DSI > > drm/msm/dsi: set video mode widebus enable bit when widebus is enabled > > drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC (fix video mode DSC) > > drm/msm/dsi: add a comment to explain pkt_per_line encoding > > drm/msm/dsi: support DSC configurations with slice_per_pkt > 1 > > > > Jun Nie (1): > > drm/display: Add slice_per_pkt for dsc > > > > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 + > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 8 + > > drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 + > > drivers/gpu/drm/msm/dsi/dsi_host.c | 42 > > +++--- > > include/drm/display/drm_dsc.h | 4 +++ > > 5 files changed, 44 insertions(+), 20 deletions(-) > > --- > > base-commit: b1e6ec0a0fd0252af046e542f91234cd6c30b2cb > > change-id: 20240403-msm-drm-dsc-dsi-video-upstream-1156d110a53d > > > > Best regards, > > -- > > Jun Nie > > > > > -- > With best wishes > Dmitry
Re: [PATCH v3 6/6] drm/msm/dsi: support DSC configurations with slice_per_pkt > 1
On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote:
>
> From: Jonathan Marek
>
> Support slice_per_pkt in msm driver.
>
> Note that the removed "pkt_per_line = slice_per_intf * slice_per_pkt"
> comment is incorrect.
>
> Also trim the code to simplify the dsc reference.
>
> Signed-off-by: Jonathan Marek
> Signed-off-by: Jun Nie
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 35 ++-
> 1 file changed, 14 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index b0507a42ee6a..0c6f40dbd25c 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -866,17 +866,10 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> *msm_host, bool is_cmd_mod
> slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
>
> total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
> - bytes_per_pkt = dsc->slice_chunk_size; /* * slice_per_pkt; */
> -
> + bytes_per_pkt = dsc->slice_chunk_size * dsc->slice_per_pkt;
Please don't mix cleanup and functional changes.
> eol_byte_num = total_bytes_per_intf % 3;
>
> - /*
> -* Typically, pkt_per_line = slice_per_intf * slice_per_pkt.
> -*
> -* Since the current driver only supports slice_per_pkt = 1,
> -* pkt_per_line will be equal to slice per intf for now.
> -*/
> - pkt_per_line = slice_per_intf;
> + pkt_per_line = slice_per_intf / dsc->slice_per_pkt;
>
> if (is_cmd_mode) /* packet data type */
> reg =
> DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
> @@ -916,6 +909,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> *msm_host, bool is_cmd_mod
> static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool
> is_bonded_dsi)
> {
> struct drm_display_mode *mode = msm_host->mode;
> + struct drm_dsc_config *dsc = msm_host->dsc;
And here too. Please pull msm_host->dsc change to a separate patch.
> u32 hs_start = 0, vs_start = 0; /* take sync start as 0 */
> u32 h_total = mode->htotal;
> u32 v_total = mode->vtotal;
> @@ -947,8 +941,7 @@ static void dsi_timing_setup(struct msm_dsi_host
> *msm_host, bool is_bonded_dsi)
> hdisplay /= 2;
> }
>
> - if (msm_host->dsc) {
> - struct drm_dsc_config *dsc = msm_host->dsc;
> + if (dsc) {
> u32 bytes_per_pclk;
>
> /* update dsc params with timing params */
> @@ -988,14 +981,14 @@ static void dsi_timing_setup(struct msm_dsi_host
> *msm_host, bool is_bonded_dsi)
> else
> bytes_per_pclk = 3;
>
> - hdisplay =
> DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), bytes_per_pclk);
> + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(dsc),
> bytes_per_pclk);
>
> h_total += hdisplay;
> ha_end = ha_start + hdisplay;
> }
>
> if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
> - if (msm_host->dsc)
> + if (dsc)
> dsi_update_dsc_timing(msm_host, false,
> mode->hdisplay);
>
> dsi_write(msm_host, REG_DSI_ACTIVE_H,
> @@ -1016,21 +1009,17 @@ static void dsi_timing_setup(struct msm_dsi_host
> *msm_host, bool is_bonded_dsi)
> DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
> DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
> } else {/* command mode */
> - if (msm_host->dsc)
> + if (dsc)
> dsi_update_dsc_timing(msm_host, true, mode->hdisplay);
>
> /* image data and 1 byte write_memory_start cmd */
> - if (!msm_host->dsc)
> + if (!dsc)
> wc = hdisplay *
> mipi_dsi_pixel_format_to_bpp(msm_host->format) / 8 + 1;
> else
> /*
> * When DSC is enabled, WC = slice_chunk_size *
> slice_per_pkt + 1.
> -* Currently, the driver only supports default value
> of slice_per_pkt = 1
> -*
> -* TODO: Expand mipi_dsi_device struct to hold
> slice_per_pkt info
> -* and adjust DSC math to account for
> slice_per_pkt.
> */
> - wc = msm_host->dsc->slice_chunk_size + 1;
> + wc = dsc->slice_chunk_size * dsc->slice_per_pkt + 1;
>
> dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
> DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
> @@ -1657,8 +1646,12 @@ static int dsi_host_attach(struct mipi_dsi_host *host,
> msm_host->lanes = dsi->lanes;
> msm_host->format = dsi->format;
> msm_host->mode_flags = dsi->mode_flags;
>
Re: [PATCH v3 3/6] drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC (fix video mode DSC)
On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote:
>
> From: Jonathan Marek
>
> Video mode DSC won't work if this field is not set correctly. Set it to fix
> video mode DSC (for slice_per_pkt==1 cases at least).
>
> Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
> Signed-off-by: Jonathan Marek
> Reviewed-by: Dmitry Baryshkov
You S-o-b is missing
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 2a0422cad6de..80ea4f1d8274 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -858,6 +858,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> *msm_host, bool is_cmd_mod
> u32 slice_per_intf, total_bytes_per_intf;
> u32 pkt_per_line;
> u32 eol_byte_num;
> + u32 bytes_per_pkt;
>
> /* first calculate dsc parameters and then program
> * compress mode registers
> @@ -865,6 +866,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> *msm_host, bool is_cmd_mod
> slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
>
> total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
> + bytes_per_pkt = dsc->slice_chunk_size; /* * slice_per_pkt; */
>
> eol_byte_num = total_bytes_per_intf % 3;
>
> @@ -902,6 +904,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> *msm_host, bool is_cmd_mod
> dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL,
> reg_ctrl);
> dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2,
> reg_ctrl2);
> } else {
> + reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_WC(bytes_per_pkt);
> dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> }
> }
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
Re: [PATCH v3 2/6] drm/msm/dsi: set video mode widebus enable bit when widebus is enabled
On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote:
>
> From: Jonathan Marek
>
> The value returned by msm_dsi_wide_bus_enabled() doesn't match what the
> driver is doing in video mode. Fix that by actually enabling widebus for
> video mode.
>
> Fixes: efcbd6f9cdeb ("drm/msm/dsi: Enable widebus for DSI")
> Signed-off-by: Jonathan Marek
> Signed-off-by: Jun Nie
You have ignored all the review comments that were provided for v1.
None of the tags were picked up either.
Have you posted this for internal review like I have explicitly asked you?
> ---
> drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
> drivers/gpu/drm/msm/dsi/dsi_host.c | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> index 2a7d980e12c3..f0b3cdc020a1 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> @@ -231,6 +231,7 @@ static inline uint32_t DSI_VID_CFG0_TRAFFIC_MODE(enum
> dsi_traffic_mode val)
> #define DSI_VID_CFG0_HSA_POWER_STOP0x0001
> #define DSI_VID_CFG0_HBP_POWER_STOP0x0010
> #define DSI_VID_CFG0_HFP_POWER_STOP0x0100
> +#define DSI_VID_CFG0_DATABUS_WIDEN 0x0200
> #define DSI_VID_CFG0_PULSE_MODE_HSA_HE 0x1000
>From the top of the file:
/* Autogenerated file, DO NOT EDIT manually!
>
> #define REG_DSI_VID_CFG1 0x001c
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 9d86a6aca6f2..2a0422cad6de 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -754,6 +754,8 @@ static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
> data |=
> DSI_VID_CFG0_TRAFFIC_MODE(dsi_get_traffic_mode(flags));
> data |= DSI_VID_CFG0_DST_FORMAT(dsi_get_vid_fmt(mipi_fmt));
> data |= DSI_VID_CFG0_VIRT_CHANNEL(msm_host->channel);
> + if (msm_dsi_host_is_wide_bus_enabled(&msm_host->base))
> + data |= DSI_VID_CFG0_DATABUS_WIDEN;
> dsi_write(msm_host, REG_DSI_VID_CFG0, data);
>
> /* Do not swap RGB colors */
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
Re: [PATCH v3 1/6] drm/msm/dpu: fix video mode DSC for DSI
On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote:
>
> From: Jonathan Marek
>
> Add necessary DPU timing and control changes for DSC to work with DSI
> video mode.
>
> Signed-off-by: Jonathan Marek
> Signed-off-by: Jun Nie
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 8
> 2 files changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index d9e7dbf0499c..c7b009a60b63 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -115,6 +115,15 @@ static void drm_mode_to_intf_timing_params(
> timing->h_front_porch = timing->h_front_porch >> 1;
> timing->hsync_pulse_width = timing->hsync_pulse_width >> 1;
> }
> +
> + /*
> +* for DSI, if compression is enabled, then divide the horizonal
> active
> +* timing parameters by compression ratio.
> +*/
> + if (phys_enc->hw_intf->cap->type != INTF_DP &&
> timing->compression_en) {
> + timing->width = timing->width / 3; /* XXX: don't assume 3:1
> compression ratio */
> + timing->xres = timing->width;
> + }
> }
>
> static u32 get_horizontal_total(const struct dpu_hw_intf_timing_params
> *timing)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 965692ef7892..079efb48db05 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -167,6 +167,14 @@ static void dpu_hw_intf_setup_timing_engine(struct
> dpu_hw_intf *ctx,
> intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>
> data_width = p->width;
> + if (p->wide_bus_en && !dp_intf)
> + data_width = p->width >> 1;
How is wide_bus relevant to the DSC case?
Is there a need for the Fixes tag?
> +
> + if (p->compression_en)
> + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
> +
> + if (p->compression_en && dp_intf)
> + DPU_ERROR("missing adjustments for DSC+DP\n");
>
> hsync_data_start_x = hsync_start_x;
> hsync_data_end_x = hsync_start_x + data_width - 1;
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
Re: [PATCH v3 0/6] Add DSC support to DSI video panel
On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote: > > This is follow up update to Jonathan's patch set. > > Changes vs V2: > - Rebase to latest mainline. > - Drop the INTF_CFG2_DATA_HCTL_EN change as it is handled in > latest mainline code. > - Drop the bonded DSI patch as I do not have device to test it. > - Address comments from version 2. Which comments? "Adress comments" is the worst case of changelog. Also, what do you consider as version 2? Jonathan Marek has only sent v1. > > Signed-off-by: Jun Nie > --- > Jonathan Marek (5): > drm/msm/dpu: fix video mode DSC for DSI > drm/msm/dsi: set video mode widebus enable bit when widebus is enabled > drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC (fix video mode DSC) > drm/msm/dsi: add a comment to explain pkt_per_line encoding > drm/msm/dsi: support DSC configurations with slice_per_pkt > 1 > > Jun Nie (1): > drm/display: Add slice_per_pkt for dsc > > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 + > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 8 + > drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 + > drivers/gpu/drm/msm/dsi/dsi_host.c | 42 > +++--- > include/drm/display/drm_dsc.h | 4 +++ > 5 files changed, 44 insertions(+), 20 deletions(-) > --- > base-commit: b1e6ec0a0fd0252af046e542f91234cd6c30b2cb > change-id: 20240403-msm-drm-dsc-dsi-video-upstream-1156d110a53d > > Best regards, > -- > Jun Nie > -- With best wishes Dmitry
Re: [PATCH v3 5/6] drm/display: Add slice_per_pkt for dsc
On Wed, 3 Apr 2024 at 12:11, Jun Nie wrote:
>
> Add variable for slice number of a DSC compression bit stream packet.
> Its value shall be specified in panel driver, or default value can be set
> in display controller driver if panel driver does not set it.
This is not a part of the standard. Please justify it.
>
> Signed-off-by: Jun Nie
> ---
> include/drm/display/drm_dsc.h | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/drm/display/drm_dsc.h b/include/drm/display/drm_dsc.h
> index bc90273d06a6..4fac0a2746ae 100644
> --- a/include/drm/display/drm_dsc.h
> +++ b/include/drm/display/drm_dsc.h
> @@ -82,6 +82,10 @@ struct drm_dsc_config {
> * @bits_per_component: Bits per component to code (8/10/12)
> */
> u8 bits_per_component;
> + /**
> +* @slice_per_pkt: slice number per DSC bit stream packet
> +*/
> + u8 slice_per_pkt;
> /**
> * @convert_rgb:
> * Flag to indicate if RGB - YCoCg conversion is needed
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
[PATCH v3 6/6] drm/msm/dsi: support DSC configurations with slice_per_pkt > 1
From: Jonathan Marek
Support slice_per_pkt in msm driver.
Note that the removed "pkt_per_line = slice_per_intf * slice_per_pkt"
comment is incorrect.
Also trim the code to simplify the dsc reference.
Signed-off-by: Jonathan Marek
Signed-off-by: Jun Nie
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 35 ++-
1 file changed, 14 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index b0507a42ee6a..0c6f40dbd25c 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -866,17 +866,10 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
*msm_host, bool is_cmd_mod
slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
- bytes_per_pkt = dsc->slice_chunk_size; /* * slice_per_pkt; */
-
+ bytes_per_pkt = dsc->slice_chunk_size * dsc->slice_per_pkt;
eol_byte_num = total_bytes_per_intf % 3;
- /*
-* Typically, pkt_per_line = slice_per_intf * slice_per_pkt.
-*
-* Since the current driver only supports slice_per_pkt = 1,
-* pkt_per_line will be equal to slice per intf for now.
-*/
- pkt_per_line = slice_per_intf;
+ pkt_per_line = slice_per_intf / dsc->slice_per_pkt;
if (is_cmd_mode) /* packet data type */
reg =
DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
@@ -916,6 +909,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
*msm_host, bool is_cmd_mod
static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
{
struct drm_display_mode *mode = msm_host->mode;
+ struct drm_dsc_config *dsc = msm_host->dsc;
u32 hs_start = 0, vs_start = 0; /* take sync start as 0 */
u32 h_total = mode->htotal;
u32 v_total = mode->vtotal;
@@ -947,8 +941,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host,
bool is_bonded_dsi)
hdisplay /= 2;
}
- if (msm_host->dsc) {
- struct drm_dsc_config *dsc = msm_host->dsc;
+ if (dsc) {
u32 bytes_per_pclk;
/* update dsc params with timing params */
@@ -988,14 +981,14 @@ static void dsi_timing_setup(struct msm_dsi_host
*msm_host, bool is_bonded_dsi)
else
bytes_per_pclk = 3;
- hdisplay =
DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), bytes_per_pclk);
+ hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(dsc),
bytes_per_pclk);
h_total += hdisplay;
ha_end = ha_start + hdisplay;
}
if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
- if (msm_host->dsc)
+ if (dsc)
dsi_update_dsc_timing(msm_host, false, mode->hdisplay);
dsi_write(msm_host, REG_DSI_ACTIVE_H,
@@ -1016,21 +1009,17 @@ static void dsi_timing_setup(struct msm_dsi_host
*msm_host, bool is_bonded_dsi)
DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
} else {/* command mode */
- if (msm_host->dsc)
+ if (dsc)
dsi_update_dsc_timing(msm_host, true, mode->hdisplay);
/* image data and 1 byte write_memory_start cmd */
- if (!msm_host->dsc)
+ if (!dsc)
wc = hdisplay *
mipi_dsi_pixel_format_to_bpp(msm_host->format) / 8 + 1;
else
/*
* When DSC is enabled, WC = slice_chunk_size *
slice_per_pkt + 1.
-* Currently, the driver only supports default value of
slice_per_pkt = 1
-*
-* TODO: Expand mipi_dsi_device struct to hold
slice_per_pkt info
-* and adjust DSC math to account for
slice_per_pkt.
*/
- wc = msm_host->dsc->slice_chunk_size + 1;
+ wc = dsc->slice_chunk_size * dsc->slice_per_pkt + 1;
dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
@@ -1657,8 +1646,12 @@ static int dsi_host_attach(struct mipi_dsi_host *host,
msm_host->lanes = dsi->lanes;
msm_host->format = dsi->format;
msm_host->mode_flags = dsi->mode_flags;
- if (dsi->dsc)
+ if (dsi->dsc) {
msm_host->dsc = dsi->dsc;
+ /* for backwards compatibility, assume 1 if not set */
+ if (!dsi->dsc->slice_per_pkt)
+ dsi->dsc->slice_per_pkt = 1;
+ }
/* Some gpios defined in panel DT need to be controlled by host */
ret = dsi_host_init_panel_gpio
[PATCH v3 5/6] drm/display: Add slice_per_pkt for dsc
Add variable for slice number of a DSC compression bit stream packet.
Its value shall be specified in panel driver, or default value can be set
in display controller driver if panel driver does not set it.
Signed-off-by: Jun Nie
---
include/drm/display/drm_dsc.h | 4
1 file changed, 4 insertions(+)
diff --git a/include/drm/display/drm_dsc.h b/include/drm/display/drm_dsc.h
index bc90273d06a6..4fac0a2746ae 100644
--- a/include/drm/display/drm_dsc.h
+++ b/include/drm/display/drm_dsc.h
@@ -82,6 +82,10 @@ struct drm_dsc_config {
* @bits_per_component: Bits per component to code (8/10/12)
*/
u8 bits_per_component;
+ /**
+* @slice_per_pkt: slice number per DSC bit stream packet
+*/
+ u8 slice_per_pkt;
/**
* @convert_rgb:
* Flag to indicate if RGB - YCoCg conversion is needed
--
2.34.1
[PATCH v3 4/6] drm/msm/dsi: add a comment to explain pkt_per_line encoding
From: Jonathan Marek Make it clear why the pkt_per_line value is being "divided by 2". Signed-off-by: Jonathan Marek Reviewed-by: Dmitry Baryshkov Signed-off-by: Jun Nie --- drivers/gpu/drm/msm/dsi/dsi_host.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index 80ea4f1d8274..b0507a42ee6a 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -886,7 +886,11 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod /* DSI_VIDEO_COMPRESSION_MODE & DSI_COMMAND_COMPRESSION_MODE * registers have similar offsets, so for below common code use * DSI_VIDEO_COMPRESSION_MODE_ for setting bits +* +* pkt_per_line is log2 encoded, >>1 works for supported values (1,2,4) */ + if (pkt_per_line > 4) + drm_warn(msm_host->dev, "pkt_per_line too big"); reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_PKT_PER_LINE(pkt_per_line >> 1); reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_EOL_BYTE_NUM(eol_byte_num); reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_EN; -- 2.34.1
[PATCH v3 3/6] drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC (fix video mode DSC)
From: Jonathan Marek
Video mode DSC won't work if this field is not set correctly. Set it to fix
video mode DSC (for slice_per_pkt==1 cases at least).
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Jonathan Marek
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 2a0422cad6de..80ea4f1d8274 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -858,6 +858,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
*msm_host, bool is_cmd_mod
u32 slice_per_intf, total_bytes_per_intf;
u32 pkt_per_line;
u32 eol_byte_num;
+ u32 bytes_per_pkt;
/* first calculate dsc parameters and then program
* compress mode registers
@@ -865,6 +866,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
*msm_host, bool is_cmd_mod
slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
+ bytes_per_pkt = dsc->slice_chunk_size; /* * slice_per_pkt; */
eol_byte_num = total_bytes_per_intf % 3;
@@ -902,6 +904,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
*msm_host, bool is_cmd_mod
dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL,
reg_ctrl);
dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2,
reg_ctrl2);
} else {
+ reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_WC(bytes_per_pkt);
dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
}
}
--
2.34.1
[PATCH v3 2/6] drm/msm/dsi: set video mode widebus enable bit when widebus is enabled
From: Jonathan Marek
The value returned by msm_dsi_wide_bus_enabled() doesn't match what the
driver is doing in video mode. Fix that by actually enabling widebus for
video mode.
Fixes: efcbd6f9cdeb ("drm/msm/dsi: Enable widebus for DSI")
Signed-off-by: Jonathan Marek
Signed-off-by: Jun Nie
---
drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
drivers/gpu/drm/msm/dsi/dsi_host.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h
b/drivers/gpu/drm/msm/dsi/dsi.xml.h
index 2a7d980e12c3..f0b3cdc020a1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
@@ -231,6 +231,7 @@ static inline uint32_t DSI_VID_CFG0_TRAFFIC_MODE(enum
dsi_traffic_mode val)
#define DSI_VID_CFG0_HSA_POWER_STOP0x0001
#define DSI_VID_CFG0_HBP_POWER_STOP0x0010
#define DSI_VID_CFG0_HFP_POWER_STOP0x0100
+#define DSI_VID_CFG0_DATABUS_WIDEN 0x0200
#define DSI_VID_CFG0_PULSE_MODE_HSA_HE 0x1000
#define REG_DSI_VID_CFG1 0x001c
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 9d86a6aca6f2..2a0422cad6de 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -754,6 +754,8 @@ static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
data |= DSI_VID_CFG0_TRAFFIC_MODE(dsi_get_traffic_mode(flags));
data |= DSI_VID_CFG0_DST_FORMAT(dsi_get_vid_fmt(mipi_fmt));
data |= DSI_VID_CFG0_VIRT_CHANNEL(msm_host->channel);
+ if (msm_dsi_host_is_wide_bus_enabled(&msm_host->base))
+ data |= DSI_VID_CFG0_DATABUS_WIDEN;
dsi_write(msm_host, REG_DSI_VID_CFG0, data);
/* Do not swap RGB colors */
--
2.34.1
[PATCH v3 1/6] drm/msm/dpu: fix video mode DSC for DSI
From: Jonathan Marek
Add necessary DPU timing and control changes for DSC to work with DSI
video mode.
Signed-off-by: Jonathan Marek
Signed-off-by: Jun Nie
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 +
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 8
2 files changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index d9e7dbf0499c..c7b009a60b63 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -115,6 +115,15 @@ static void drm_mode_to_intf_timing_params(
timing->h_front_porch = timing->h_front_porch >> 1;
timing->hsync_pulse_width = timing->hsync_pulse_width >> 1;
}
+
+ /*
+* for DSI, if compression is enabled, then divide the horizonal active
+* timing parameters by compression ratio.
+*/
+ if (phys_enc->hw_intf->cap->type != INTF_DP && timing->compression_en) {
+ timing->width = timing->width / 3; /* XXX: don't assume 3:1
compression ratio */
+ timing->xres = timing->width;
+ }
}
static u32 get_horizontal_total(const struct dpu_hw_intf_timing_params *timing)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 965692ef7892..079efb48db05 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -167,6 +167,14 @@ static void dpu_hw_intf_setup_timing_engine(struct
dpu_hw_intf *ctx,
intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
data_width = p->width;
+ if (p->wide_bus_en && !dp_intf)
+ data_width = p->width >> 1;
+
+ if (p->compression_en)
+ intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
+
+ if (p->compression_en && dp_intf)
+ DPU_ERROR("missing adjustments for DSC+DP\n");
hsync_data_start_x = hsync_start_x;
hsync_data_end_x = hsync_start_x + data_width - 1;
--
2.34.1
[PATCH v3 0/6] Add DSC support to DSI video panel
This is follow up update to Jonathan's patch set. Changes vs V2: - Rebase to latest mainline. - Drop the INTF_CFG2_DATA_HCTL_EN change as it is handled in latest mainline code. - Drop the bonded DSI patch as I do not have device to test it. - Address comments from version 2. Signed-off-by: Jun Nie --- Jonathan Marek (5): drm/msm/dpu: fix video mode DSC for DSI drm/msm/dsi: set video mode widebus enable bit when widebus is enabled drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC (fix video mode DSC) drm/msm/dsi: add a comment to explain pkt_per_line encoding drm/msm/dsi: support DSC configurations with slice_per_pkt > 1 Jun Nie (1): drm/display: Add slice_per_pkt for dsc .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c| 8 + drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 + drivers/gpu/drm/msm/dsi/dsi_host.c | 42 +++--- include/drm/display/drm_dsc.h | 4 +++ 5 files changed, 44 insertions(+), 20 deletions(-) --- base-commit: b1e6ec0a0fd0252af046e542f91234cd6c30b2cb change-id: 20240403-msm-drm-dsc-dsi-video-upstream-1156d110a53d Best regards, -- Jun Nie
