[PATCH 0/2] Adreno MAINTAINERS modifications
Separate out Adreno from the rest of the drm/msm driver, add myself as a reviewer for the former. Signed-off-by: Konrad Dybcio --- Konrad Dybcio (2): MAINTAINERS: Add a separate entry for Qualcomm Adreno GPU drivers MAINTAINERS: Add Konrad Dybcio as a reviewer for the Adreno driver MAINTAINERS | 20 +++- 1 file changed, 19 insertions(+), 1 deletion(-) --- base-commit: 6bd343537461b57f3efe5dfc5fc193a232dfef1e change-id: 20240423-topic-adreno_maintainers-4d220cba75e8 Best regards, -- Konrad Dybcio
[PATCH 2/2] MAINTAINERS: Add Konrad Dybcio as a reviewer for the Adreno driver
Add myself as a reviewer for Adreno driver changes. Signed-off-by: Konrad Dybcio --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 179f989a1e4b..80aa006f10bb 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6888,6 +6888,7 @@ F:drivers/gpu/drm/tiny/panel-mipi-dbi.c DRM DRIVER for Qualcomm Adreno GPUs M: Rob Clark R: Sean Paul +R: Konrad Dybcio L: linux-arm-...@vger.kernel.org L: dri-de...@lists.freedesktop.org L: freedreno@lists.freedesktop.org -- 2.40.1
[PATCH 1/2] MAINTAINERS: Add a separate entry for Qualcomm Adreno GPU drivers
The msm driver is.. gigantic and covers display hardware (incl. things concerning (e)DP, DSI, HDMI), as well as the entire lineup of Adreno GPUs (with hw bringup, memory mappings, userspace interaction etc.). Because of that, people listed as M:/R: receive patches concerning drivers for any part of the display block OR the GPU. Separate the latter, as it's both a functionally separate block and is of interest to different folks. Signed-off-by: Konrad Dybcio --- MAINTAINERS | 19 ++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index de49e2d24770..179f989a1e4b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6885,7 +6885,24 @@ T: git https://gitlab.freedesktop.org/drm/misc/kernel.git F: Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml F: drivers/gpu/drm/tiny/panel-mipi-dbi.c -DRM DRIVER FOR MSM ADRENO GPU +DRM DRIVER for Qualcomm Adreno GPUs +M: Rob Clark +R: Sean Paul +L: linux-arm-...@vger.kernel.org +L: dri-de...@lists.freedesktop.org +L: freedreno@lists.freedesktop.org +S: Maintained +B: https://gitlab.freedesktop.org/drm/msm/-/issues +T: git https://gitlab.freedesktop.org/drm/msm.git +F: Documentation/devicetree/bindings/display/msm/gpu.yaml +F: drivers/gpu/drm/msm/adreno/ +F: drivers/gpu/drm/msm/msm_gpu.* +F: drivers/gpu/drm/msm/msm_gpu_devfreq.* +F: drivers/gpu/drm/msm/msm_ringbuffer.* +F: drivers/gpu/drm/msm/registers/adreno/ +F: include/uapi/drm/msm_drm.h + +DRM DRIVER for Qualcomm display hardware M: Rob Clark M: Abhinav Kumar M: Dmitry Baryshkov -- 2.40.1
[PATCH 2/2] drm/msm/dsi: Remove dsi_phy_write_[un]delay()
These are dummy wrappers that do literally nothing interesting.
Remove them.
Signed-off-by: Konrad Dybcio
---
drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 3 --
drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 3 +-
drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c | 81 +++---
3 files changed, 54 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index 7df4d852e6fa..109d767a476b 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -12,9 +12,6 @@
#include "dsi.h"
-#define dsi_phy_write_udelay(offset, data, delay_us) { writel((data),
(offset)); udelay(delay_us); }
-#define dsi_phy_write_ndelay(offset, data, delay_ns) { writel((data),
(offset)); ndelay(delay_ns); }
-
struct msm_dsi_phy_ops {
int (*pll_init)(struct msm_dsi_phy *phy);
int (*enable)(struct msm_dsi_phy *phy,
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
index b128c4acea23..1723f0e4faa4 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
@@ -374,7 +374,8 @@ static void pll_14nm_software_reset(struct dsi_pll_14nm
*pll_14nm)
writel(0, cmn_base + REG_DSI_14nm_PHY_CMN_PLL_CNTRL);
/* pll sw reset */
- dsi_phy_write_udelay(cmn_base + REG_DSI_14nm_PHY_CMN_CTRL_1, 0x20, 10);
+ writel(0x20, cmn_base + REG_DSI_14nm_PHY_CMN_CTRL_1);
+ udelay(10);
wmb(); /* make sure register committed */
writel(0, cmn_base + REG_DSI_14nm_PHY_CMN_CTRL_1);
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
index b3e914954f4a..db99526d11df 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
@@ -104,9 +104,10 @@ static void pll_28nm_software_reset(struct dsi_pll_28nm
*pll_28nm)
* Add HW recommended delays after toggling the software
* reset bit off and back on.
*/
- dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_TEST_CFG,
-DSI_28nm_PHY_PLL_TEST_CFG_PLL_SW_RESET, 1);
- dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_TEST_CFG, 0x00, 1);
+ writel(DSI_28nm_PHY_PLL_TEST_CFG_PLL_SW_RESET, base +
REG_DSI_28nm_PHY_PLL_TEST_CFG);
+ udelay(1);
+ writel(0, base + REG_DSI_28nm_PHY_PLL_TEST_CFG);
+ udelay(1);
}
/*
@@ -303,21 +304,25 @@ static int _dsi_pll_28nm_vco_prepare_hpm(struct
dsi_pll_28nm *pll_28nm)
* Add necessary delays recommended by hardware.
*/
val = DSI_28nm_PHY_PLL_GLB_CFG_PLL_PWRDN_B;
- dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val, 1);
+ writel(val, base + REG_DSI_28nm_PHY_PLL_GLB_CFG);
+ udelay(1);
val |= DSI_28nm_PHY_PLL_GLB_CFG_PLL_PWRGEN_PWRDN_B;
- dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val, 200);
+ writel(val, base + REG_DSI_28nm_PHY_PLL_GLB_CFG);
+ udelay(200);
val |= DSI_28nm_PHY_PLL_GLB_CFG_PLL_LDO_PWRDN_B;
- dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val, 500);
+ writel(val, base + REG_DSI_28nm_PHY_PLL_GLB_CFG);
+ udelay(500);
val |= DSI_28nm_PHY_PLL_GLB_CFG_PLL_ENABLE;
- dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val, 600);
+ writel(val, base + REG_DSI_28nm_PHY_PLL_GLB_CFG);
+ udelay(600);
for (i = 0; i < 2; i++) {
/* DSI Uniphy lock detect setting */
- dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_LKDET_CFG2,
-0x0c, 100);
+ writel(0x0c, base + REG_DSI_28nm_PHY_PLL_LKDET_CFG2);
+ udelay(100);
writel(0x0d, base + REG_DSI_28nm_PHY_PLL_LKDET_CFG2);
/* poll for PLL ready status */
@@ -333,22 +338,28 @@ static int _dsi_pll_28nm_vco_prepare_hpm(struct
dsi_pll_28nm *pll_28nm)
* Add necessary delays recommended by hardware.
*/
val = DSI_28nm_PHY_PLL_GLB_CFG_PLL_PWRDN_B;
- dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val,
1);
+ writel(val, base + REG_DSI_28nm_PHY_PLL_GLB_CFG);
+ udelay(1);
val |= DSI_28nm_PHY_PLL_GLB_CFG_PLL_PWRGEN_PWRDN_B;
- dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val,
200);
+ writel(val, base + REG_DSI_28nm_PHY_PLL_GLB_CFG);
+ udelay(200);
val |= DSI_28nm_PHY_PLL_GLB_CFG_PLL_LDO_PWRDN_B;
- dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG, val,
250);
+ writel(val, base + REG_DSI_28nm_PHY_PLL_GLB_CFG);
+ udelay(250);
val &= ~DSI_28nm_PHY_PLL_GLB_CFG_PLL_LDO_PWRDN_B;
- dsi_phy_write_udelay(base + REG_DSI_28nm_PHY_PLL_GLB_CFG,
[PATCH 1/2] drm/msm/dsi: Remove dsi_phy_read/write()
These are dummy wrappers that do literally nothing interesting.
Remove them.
Signed-off-by: Konrad Dybcio
---
drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 2 -
drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 273 +---
drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 215
drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c | 109
drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c | 224 -
drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c | 205 +++
drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 320
7 files changed, 645 insertions(+), 703 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index 5a5dc3faa971..7df4d852e6fa 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -12,8 +12,6 @@
#include "dsi.h"
-#define dsi_phy_read(offset) readl((offset))
-#define dsi_phy_write(offset, data) writel((data), (offset))
#define dsi_phy_write_udelay(offset, data, delay_us) { writel((data),
(offset)); udelay(delay_us); }
#define dsi_phy_write_ndelay(offset, data, delay_ns) { writel((data),
(offset)); ndelay(delay_ns); }
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
index 27b592c776a3..677c62571811 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c
@@ -187,20 +187,20 @@ static void dsi_pll_ssc_commit(struct dsi_pll_10nm *pll,
struct dsi_pll_config *
if (config->enable_ssc) {
pr_debug("SSC is enabled\n");
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_SSC_STEPSIZE_LOW_1,
- config->ssc_stepsize & 0xff);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_SSC_STEPSIZE_HIGH_1,
- config->ssc_stepsize >> 8);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_SSC_DIV_PER_LOW_1,
- config->ssc_div_per & 0xff);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_SSC_DIV_PER_HIGH_1,
- config->ssc_div_per >> 8);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_SSC_DIV_ADJPER_LOW_1,
- config->ssc_adj_per & 0xff);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_SSC_DIV_ADJPER_HIGH_1,
- config->ssc_adj_per >> 8);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_SSC_CONTROL,
- SSC_EN | (config->ssc_center ? SSC_CENTER : 0));
+ writel(config->ssc_stepsize & 0xff,
+ base + REG_DSI_10nm_PHY_PLL_SSC_STEPSIZE_LOW_1);
+ writel(config->ssc_stepsize >> 8,
+ base + REG_DSI_10nm_PHY_PLL_SSC_STEPSIZE_HIGH_1);
+ writel(config->ssc_div_per & 0xff,
+ base + REG_DSI_10nm_PHY_PLL_SSC_DIV_PER_LOW_1);
+ writel(config->ssc_div_per >> 8,
+ base + REG_DSI_10nm_PHY_PLL_SSC_DIV_PER_HIGH_1);
+ writel(config->ssc_adj_per & 0xff,
+ base + REG_DSI_10nm_PHY_PLL_SSC_DIV_ADJPER_LOW_1);
+ writel(config->ssc_adj_per >> 8,
+ base + REG_DSI_10nm_PHY_PLL_SSC_DIV_ADJPER_HIGH_1);
+ writel(SSC_EN | (config->ssc_center ? SSC_CENTER : 0),
+ base + REG_DSI_10nm_PHY_PLL_SSC_CONTROL);
}
}
@@ -208,49 +208,43 @@ static void dsi_pll_config_hzindep_reg(struct
dsi_pll_10nm *pll)
{
void __iomem *base = pll->phy->pll_base;
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_ONE, 0x80);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_TWO, 0x03);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_ANALOG_CONTROLS_THREE, 0x00);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_DSM_DIVIDER, 0x00);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_FEEDBACK_DIVIDER, 0x4e);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_CALIBRATION_SETTINGS, 0x40);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_BAND_SEL_CAL_SETTINGS_THREE,
- 0xba);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_FREQ_DETECT_SETTINGS_ONE,
- 0x0c);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_OUTDIV, 0x00);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_CORE_OVERRIDE, 0x00);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_PLL_DIGITAL_TIMERS_TWO,
- 0x08);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_PLL_PROP_GAIN_RATE_1, 0x08);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_PLL_BAND_SET_RATE_1, 0xc0);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_PLL_INT_GAIN_IFILT_BAND_1,
- 0xfa);
- dsi_phy_write(base + REG_DSI_10nm_PHY_PLL_PLL_FL_INT_GAIN_PFILT_BAND_1,
- 0x4c);
- dsi_phy_write(base +
[PATCH 0/2] Remove more useless wrappers
Shaving off some cruft obj files seem to be identical pre and post cleanup which is always a good sign Signed-off-by: Konrad Dybcio --- Konrad Dybcio (2): drm/msm/dsi: Remove dsi_phy_read/write() drm/msm/dsi: Remove dsi_phy_write_[un]delay() drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 5 - drivers/gpu/drm/msm/dsi/phy/dsi_phy_10nm.c | 273 +--- drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 218 drivers/gpu/drm/msm/dsi/phy/dsi_phy_20nm.c | 109 drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c | 305 +++--- drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c | 205 +++ drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 320 7 files changed, 699 insertions(+), 736 deletions(-) --- base-commit: 33edc5592466996fe9610efc712da0a3539027ae change-id: 20240423-topic-msm_cleanup-b3e47bc8411d Best regards, -- Konrad Dybcio
Re: [PATCH 2/3] drm/msm/mdp4: don't destroy mdp4_kms in mdp4_kms_init error path
On Mon, Apr 22, 2024 at 11:17:15AM -0700, Abhinav Kumar wrote:
>
>
> On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote:
> > Since commit 3c74682637e6 ("drm/msm/mdp4: move resource allocation to
> > the _probe function") the mdp4_kms data is allocated during probe. It is
> > an error to destroy it during mdp4_kms_init(), as the data is still
> > referenced by the drivers's data and can be used later in case of probe
> > deferral.
> >
>
> mdp4_destroy() currently detaches mmu, calls msm_kms_destroy() which
> destroys pending timers and releases refcount on the aspace.
>
> It does not touch the mdp4_kms as that one is devm managed.
>
> In the comment
> https://patchwork.freedesktop.org/patch/590411/?series=132664=1#comment_1074306,
> we had discussed that the last component's reprobe attempt is affected
> (which is not dpu or mdp4 or mdp5 right? )
>
> If it was an interface (such as DSI OR DP), is it the aspace detach which is
> causing the crash?
I should have retained the trace log. I'll trigger the issue and post the trace.
>
> Another note is, mdp5 needs the same fix in that case.
>
> dpu_kms_init() looks fine.
>
> > Fixes: 3c74682637e6 ("drm/msm/mdp4: move resource allocation to the _probe
> > function")
> > Signed-off-by: Dmitry Baryshkov
> > ---
> > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 28 +---
> > 1 file changed, 9 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > index 4ba1cb74ad76..4c5f72b7e0e5 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> > @@ -392,7 +392,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > ret = mdp_kms_init(_kms->base, _funcs);
> > if (ret) {
> > DRM_DEV_ERROR(dev->dev, "failed to init kms\n");
> > - goto fail;
> > + return ret;
> > }
> > kms = priv->kms;
> > @@ -403,7 +403,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > ret = regulator_enable(mdp4_kms->vdd);
> > if (ret) {
> > DRM_DEV_ERROR(dev->dev, "failed to enable regulator
> > vdd: %d\n", ret);
> > - goto fail;
> > + return ret;
> > }
> > }
> > @@ -414,8 +414,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > if (major != 4) {
> > DRM_DEV_ERROR(dev->dev, "unexpected MDP version: v%d.%d\n",
> > major, minor);
> > - ret = -ENXIO;
> > - goto fail;
> > + return -ENXIO;
> > }
> > mdp4_kms->rev = minor;
> > @@ -423,8 +422,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > if (mdp4_kms->rev >= 2) {
> > if (!mdp4_kms->lut_clk) {
> > DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n");
> > - ret = -ENODEV;
> > - goto fail;
> > + return -ENODEV;
> > }
> > clk_set_rate(mdp4_kms->lut_clk, max_clk);
> > }
> > @@ -445,8 +443,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > mmu = msm_iommu_new(>dev, 0);
> > if (IS_ERR(mmu)) {
> > - ret = PTR_ERR(mmu);
> > - goto fail;
> > + return PTR_ERR(mmu);
> > } else if (!mmu) {
> > DRM_DEV_INFO(dev->dev, "no iommu, fallback to phys "
> > "contig buffers for scanout\n");
> > @@ -458,8 +455,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > if (IS_ERR(aspace)) {
> > if (!IS_ERR(mmu))
> > mmu->funcs->destroy(mmu);
> > - ret = PTR_ERR(aspace);
> > - goto fail;
> > + return PTR_ERR(aspace);
> > }
> > kms->aspace = aspace;
> > @@ -468,7 +464,7 @@ static int mdp4_kms_init(struct drm_device *dev)
> > ret = modeset_init(mdp4_kms);
> > if (ret) {
> > DRM_DEV_ERROR(dev->dev, "modeset_init failed: %d\n", ret);
> > - goto fail;
> > + return ret;
> > }
> > mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC |
> > MSM_BO_SCANOUT);
> > @@ -476,14 +472,14 @@ static int mdp4_kms_init(struct drm_device *dev)
> > ret = PTR_ERR(mdp4_kms->blank_cursor_bo);
> > DRM_DEV_ERROR(dev->dev, "could not allocate blank-cursor bo:
> > %d\n", ret);
> > mdp4_kms->blank_cursor_bo = NULL;
> > - goto fail;
> > + return ret;
> > }
> > ret = msm_gem_get_and_pin_iova(mdp4_kms->blank_cursor_bo, kms->aspace,
> > _kms->blank_cursor_iova);
> > if (ret) {
> > DRM_DEV_ERROR(dev->dev, "could not pin blank-cursor bo: %d\n",
> > ret);
> > - goto fail;
> > + return ret;
> > }
> > dev->mode_config.min_width = 0;
> > @@ -492,12 +488,6 @@ static int mdp4_kms_init(struct
Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
On Mon, Apr 22, 2024 at 09:12:20AM -0700, Abhinav Kumar wrote: > > > On 4/21/2024 3:35 PM, Dmitry Baryshkov wrote: > > On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote: > > > > > > > > > On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: > > > > MSM display drivers provide kms structure allocated during probe(). > > > > Don't clean up priv->kms field in case of an error. Otherwise probe > > > > functions might fail after KMS probe deferral. > > > > > > > > > > So just to understand this more, this will happen when master component > > > probe (dpu) succeeded but other sub-component probe (dsi) deferred? > > > > > > Because if master component probe itself deferred it will allocate > > > priv->kms > > > again isnt it and we will not even hit here. > > > > Master probing succeeds (so priv->kms is set), then kms_init fails at > > runtime, during binding of the master device. This results in probe > > deferral from the last component's component_add() function and reprobe > > attempt when possible (once the next device is added or probed). However > > as priv->kms is NULL, probe crashes. > > > > Got it, a better commit text would have helped here. Either way, I'll update the commit text with the text above. > > Reviewed-by: Abhinav Kumar -- With best wishes Dmitry
Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
On Mon, Apr 22, 2024 at 10:23:18AM -0700, Abhinav Kumar wrote:
>
>
> On 4/21/2024 3:35 PM, Dmitry Baryshkov wrote:
> > On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote:
> > >
> > >
> > > On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote:
> > > > MSM display drivers provide kms structure allocated during probe().
> > > > Don't clean up priv->kms field in case of an error. Otherwise probe
> > > > functions might fail after KMS probe deferral.
> > > >
> > >
> > > So just to understand this more, this will happen when master component
> > > probe (dpu) succeeded but other sub-component probe (dsi) deferred?
> > >
> > > Because if master component probe itself deferred it will allocate
> > > priv->kms
> > > again isnt it and we will not even hit here.
> >
> > Master probing succeeds (so priv->kms is set), then kms_init fails at
> > runtime, during binding of the master device. This results in probe
> > deferral from the last component's component_add() function and reprobe
> > attempt when possible (once the next device is added or probed). However
> > as priv->kms is NULL, probe crashes.
> >
> > >
> > > > Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to
> > > > msm_drv_probe()")
>
> Actually, Is this Fixes tag correct?
>
> OR is this one better
>
> Fixes: 506efcba3129 ("drm/msm: carve out KMS code from msm_drv.c")
No. The issue existed even before the carve-out.
>
>
> > > > Signed-off-by: Dmitry Baryshkov
> > > > ---
> > > >drivers/gpu/drm/msm/msm_kms.c | 1 -
> > > >1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/msm_kms.c
> > > > b/drivers/gpu/drm/msm/msm_kms.c
> > > > index af6a6fcb1173..6749f0fbca96 100644
> > > > --- a/drivers/gpu/drm/msm/msm_kms.c
> > > > +++ b/drivers/gpu/drm/msm/msm_kms.c
> > > > @@ -244,7 +244,6 @@ int msm_drm_kms_init(struct device *dev, const
> > > > struct drm_driver *drv)
> > > > ret = priv->kms_init(ddev);
> > > > if (ret) {
> > > > DRM_DEV_ERROR(dev, "failed to load kms\n");
> > > > - priv->kms = NULL;
> > > > return ret;
> > > > }
> > > >
> >
--
With best wishes
Dmitry
Re: [PATCH 2/3] drm/msm/mdp4: don't destroy mdp4_kms in mdp4_kms_init error path
On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote:
Since commit 3c74682637e6 ("drm/msm/mdp4: move resource allocation to
the _probe function") the mdp4_kms data is allocated during probe. It is
an error to destroy it during mdp4_kms_init(), as the data is still
referenced by the drivers's data and can be used later in case of probe
deferral.
mdp4_destroy() currently detaches mmu, calls msm_kms_destroy() which
destroys pending timers and releases refcount on the aspace.
It does not touch the mdp4_kms as that one is devm managed.
In the comment
https://patchwork.freedesktop.org/patch/590411/?series=132664=1#comment_1074306,
we had discussed that the last component's reprobe attempt is affected
(which is not dpu or mdp4 or mdp5 right? )
If it was an interface (such as DSI OR DP), is it the aspace detach
which is causing the crash?
Another note is, mdp5 needs the same fix in that case.
dpu_kms_init() looks fine.
Fixes: 3c74682637e6 ("drm/msm/mdp4: move resource allocation to the _probe
function")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 28 +---
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 4ba1cb74ad76..4c5f72b7e0e5 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -392,7 +392,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = mdp_kms_init(_kms->base, _funcs);
if (ret) {
DRM_DEV_ERROR(dev->dev, "failed to init kms\n");
- goto fail;
+ return ret;
}
kms = priv->kms;
@@ -403,7 +403,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = regulator_enable(mdp4_kms->vdd);
if (ret) {
DRM_DEV_ERROR(dev->dev, "failed to enable regulator vdd:
%d\n", ret);
- goto fail;
+ return ret;
}
}
@@ -414,8 +414,7 @@ static int mdp4_kms_init(struct drm_device *dev)
if (major != 4) {
DRM_DEV_ERROR(dev->dev, "unexpected MDP version: v%d.%d\n",
major, minor);
- ret = -ENXIO;
- goto fail;
+ return -ENXIO;
}
mdp4_kms->rev = minor;
@@ -423,8 +422,7 @@ static int mdp4_kms_init(struct drm_device *dev)
if (mdp4_kms->rev >= 2) {
if (!mdp4_kms->lut_clk) {
DRM_DEV_ERROR(dev->dev, "failed to get lut_clk\n");
- ret = -ENODEV;
- goto fail;
+ return -ENODEV;
}
clk_set_rate(mdp4_kms->lut_clk, max_clk);
}
@@ -445,8 +443,7 @@ static int mdp4_kms_init(struct drm_device *dev)
mmu = msm_iommu_new(>dev, 0);
if (IS_ERR(mmu)) {
- ret = PTR_ERR(mmu);
- goto fail;
+ return PTR_ERR(mmu);
} else if (!mmu) {
DRM_DEV_INFO(dev->dev, "no iommu, fallback to phys "
"contig buffers for scanout\n");
@@ -458,8 +455,7 @@ static int mdp4_kms_init(struct drm_device *dev)
if (IS_ERR(aspace)) {
if (!IS_ERR(mmu))
mmu->funcs->destroy(mmu);
- ret = PTR_ERR(aspace);
- goto fail;
+ return PTR_ERR(aspace);
}
kms->aspace = aspace;
@@ -468,7 +464,7 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = modeset_init(mdp4_kms);
if (ret) {
DRM_DEV_ERROR(dev->dev, "modeset_init failed: %d\n", ret);
- goto fail;
+ return ret;
}
mdp4_kms->blank_cursor_bo = msm_gem_new(dev, SZ_16K, MSM_BO_WC | MSM_BO_SCANOUT);
@@ -476,14 +472,14 @@ static int mdp4_kms_init(struct drm_device *dev)
ret = PTR_ERR(mdp4_kms->blank_cursor_bo);
DRM_DEV_ERROR(dev->dev, "could not allocate blank-cursor bo:
%d\n", ret);
mdp4_kms->blank_cursor_bo = NULL;
- goto fail;
+ return ret;
}
ret = msm_gem_get_and_pin_iova(mdp4_kms->blank_cursor_bo, kms->aspace,
_kms->blank_cursor_iova);
if (ret) {
DRM_DEV_ERROR(dev->dev, "could not pin blank-cursor bo: %d\n",
ret);
- goto fail;
+ return ret;
}
dev->mode_config.min_width = 0;
@@ -492,12 +488,6 @@ static int mdp4_kms_init(struct drm_device *dev)
dev->mode_config.max_height = 2048;
return 0;
-
-fail:
- if (kms)
- mdp4_destroy(kms);
-
- return ret;
}
static const struct dev_pm_ops mdp4_pm_ops = {
Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
On 4/21/2024 3:35 PM, Dmitry Baryshkov wrote:
On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote:
On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote:
MSM display drivers provide kms structure allocated during probe().
Don't clean up priv->kms field in case of an error. Otherwise probe
functions might fail after KMS probe deferral.
So just to understand this more, this will happen when master component
probe (dpu) succeeded but other sub-component probe (dsi) deferred?
Because if master component probe itself deferred it will allocate priv->kms
again isnt it and we will not even hit here.
Master probing succeeds (so priv->kms is set), then kms_init fails at
runtime, during binding of the master device. This results in probe
deferral from the last component's component_add() function and reprobe
attempt when possible (once the next device is added or probed). However
as priv->kms is NULL, probe crashes.
Fixes: a2ab5d5bb6b1 ("drm/msm: allow passing struct msm_kms to msm_drv_probe()")
Actually, Is this Fixes tag correct?
OR is this one better
Fixes: 506efcba3129 ("drm/msm: carve out KMS code from msm_drv.c")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/msm_kms.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index af6a6fcb1173..6749f0fbca96 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -244,7 +244,6 @@ int msm_drm_kms_init(struct device *dev, const struct
drm_driver *drv)
ret = priv->kms_init(ddev);
if (ret) {
DRM_DEV_ERROR(dev, "failed to load kms\n");
- priv->kms = NULL;
return ret;
}
Re: [PATCH 1/3] drm/msm: don't clean up priv->kms prematurely
On 4/21/2024 3:35 PM, Dmitry Baryshkov wrote: On Sat, Apr 20, 2024 at 04:02:00PM -0700, Abhinav Kumar wrote: On 4/19/2024 7:33 PM, Dmitry Baryshkov wrote: MSM display drivers provide kms structure allocated during probe(). Don't clean up priv->kms field in case of an error. Otherwise probe functions might fail after KMS probe deferral. So just to understand this more, this will happen when master component probe (dpu) succeeded but other sub-component probe (dsi) deferred? Because if master component probe itself deferred it will allocate priv->kms again isnt it and we will not even hit here. Master probing succeeds (so priv->kms is set), then kms_init fails at runtime, during binding of the master device. This results in probe deferral from the last component's component_add() function and reprobe attempt when possible (once the next device is added or probed). However as priv->kms is NULL, probe crashes. Got it, a better commit text would have helped here. Either way, Reviewed-by: Abhinav Kumar
Re: [PATCH 1/2] drm/print: drop include debugfs.h and include where needed
On Mon, Apr 22, 2024 at 03:10:10PM +0300, Jani Nikula wrote: > Surprisingly many places depend on debugfs.h to be included via > drm_print.h. Fix them. > > v3: Also fix armada, ite-it6505, imagination, msm, sti, vc4, and xe > > v2: Also fix ivpu and vmwgfx > > Reviewed-by: Andrzej Hajda > Acked-by: Maxime Ripard > Link: > https://patchwork.freedesktop.org/patch/msgid/20240410141434.157908-1-jani.nik...@intel.com > Signed-off-by: Jani Nikula > > --- > > Cc: Jacek Lawrynowicz > Cc: Stanislaw Gruszka > Cc: Oded Gabbay > Cc: Russell King > Cc: David Airlie > Cc: Daniel Vetter > Cc: Andrzej Hajda > Cc: Neil Armstrong > Cc: Robert Foss > Cc: Laurent Pinchart > Cc: Jonas Karlman > Cc: Jernej Skrabec > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: Jani Nikula > Cc: Rodrigo Vivi > Cc: Joonas Lahtinen > Cc: Tvrtko Ursulin > Cc: Frank Binns > Cc: Matt Coster > Cc: Rob Clark > Cc: Abhinav Kumar > Cc: Dmitry Baryshkov > Cc: Sean Paul > Cc: Marijn Suijten > Cc: Karol Herbst > Cc: Lyude Paul > Cc: Danilo Krummrich > Cc: Alex Deucher > Cc: "Christian König" > Cc: "Pan, Xinhui" > Cc: Alain Volmat > Cc: Huang Rui > Cc: Zack Rusin > Cc: Broadcom internal kernel review list > > Cc: Lucas De Marchi > Cc: "Thomas Hellström" > Cc: dri-de...@lists.freedesktop.org > Cc: intel-...@lists.freedesktop.org > Cc: intel...@lists.freedesktop.org > Cc: linux-arm-...@vger.kernel.org > Cc: freedreno@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: amd-...@lists.freedesktop.org > --- > drivers/accel/ivpu/ivpu_debugfs.c | 2 ++ > drivers/gpu/drm/armada/armada_debugfs.c | 1 + > drivers/gpu/drm/bridge/ite-it6505.c | 1 + > drivers/gpu/drm/bridge/panel.c | 2 ++ > drivers/gpu/drm/drm_print.c | 6 +++--- > drivers/gpu/drm/i915/display/intel_dmc.c| 1 + > drivers/gpu/drm/imagination/pvr_fw_trace.c | 1 + > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 2 ++ Acked-by: Dmitry Baryshkov # drm/msm > drivers/gpu/drm/nouveau/dispnv50/crc.c | 2 ++ > drivers/gpu/drm/radeon/r100.c | 1 + > drivers/gpu/drm/radeon/r300.c | 1 + > drivers/gpu/drm/radeon/r420.c | 1 + > drivers/gpu/drm/radeon/r600.c | 3 ++- > drivers/gpu/drm/radeon/radeon_fence.c | 1 + > drivers/gpu/drm/radeon/radeon_gem.c | 1 + > drivers/gpu/drm/radeon/radeon_ib.c | 2 ++ > drivers/gpu/drm/radeon/radeon_pm.c | 1 + > drivers/gpu/drm/radeon/radeon_ring.c| 2 ++ > drivers/gpu/drm/radeon/radeon_ttm.c | 1 + > drivers/gpu/drm/radeon/rs400.c | 1 + > drivers/gpu/drm/radeon/rv515.c | 1 + > drivers/gpu/drm/sti/sti_drv.c | 1 + > drivers/gpu/drm/ttm/ttm_device.c| 1 + > drivers/gpu/drm/ttm/ttm_resource.c | 3 ++- > drivers/gpu/drm/ttm/ttm_tt.c| 5 +++-- > drivers/gpu/drm/vc4/vc4_drv.h | 1 + > drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 2 ++ > drivers/gpu/drm/xe/xe_debugfs.c | 1 + > drivers/gpu/drm/xe/xe_gt_debugfs.c | 2 ++ > drivers/gpu/drm/xe/xe_uc_debugfs.c | 2 ++ > include/drm/drm_print.h | 2 +- > 31 files changed, 46 insertions(+), 8 deletions(-) -- With best wishes Dmitry
Re: [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check
On Fri, Apr 19, 2024 at 07:37:44PM -0700, Abhinav Kumar wrote:
>
>
> On 4/19/2024 6:34 PM, Dmitry Baryshkov wrote:
> > On Fri, Apr 19, 2024 at 05:14:01PM -0700, Abhinav Kumar wrote:
> > >
> > >
> > > On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
> > > > Move a call to dpu_format_populate_plane_sizes() to the atomic_check
> > > > step, so that any issues with the FB layout can be reported as early as
> > > > possible.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov
> > > > ---
> > > >drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 12 ++--
> > > >1 file changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > index d9631fe90228..a9de1fbd0df3 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > @@ -673,12 +673,6 @@ static int dpu_plane_prepare_fb(struct drm_plane
> > > > *plane,
> > > > }
> > > > }
> > > > - ret = dpu_format_populate_plane_sizes(new_state->fb,
> > > > >layout);
> > > > - if (ret) {
> > > > - DPU_ERROR_PLANE(pdpu, "failed to get format plane
> > > > sizes, %d\n", ret);
> > > > - return ret;
> > > > - }
> > > > -
> > > > /* validate framebuffer layout before commit */
> > > > ret = dpu_format_populate_addrs(pstate->aspace,
> > > > new_state->fb,
> > > > @@ -864,6 +858,12 @@ static int dpu_plane_atomic_check(struct drm_plane
> > > > *plane,
> > > > return -E2BIG;
> > > > }
> > > > + ret = dpu_format_populate_plane_sizes(new_plane_state->fb,
> > > > >layout);
> > > > + if (ret) {
> > > > + DPU_ERROR_PLANE(pdpu, "failed to get format plane
> > > > sizes, %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > >
> > > I think we need another function to do the check. It seems incorrect to
> > > populate the layout to the plane state knowing it can potentially fail.
> >
> > why? The state is interim object, which is subject to checks. In other
> > parts of the atomic_check we also fill parts of the state, perform
> > checks and then destroy it if the check fails.
> >
>
> Yes, the same thing you wrote.
>
> I felt we can perform the validation and reject it before populating it in
> the state as it seems thats doable here rather than populating it without
> knowing whether it can be discarded.
But populate function does the check. It seems like an overkill to add
another function. Also I still don't see the point. It was fine to call
this function from .prepare_fb, but it's not fine to call it from
.atomic_check?
>
> > Maybe I'm missing your point here. Could you please explain what is the
> > problem from your point of view?
> >
> > >
> > > Can we move the validation part of dpu_format_populate_plane_sizes() out
> > > to
> > > another helper dpu_format_validate_plane_sizes() and use that?
> > >
> > > And then make the remaining dpu_format_populate_plane_sizes() just a void
> > > API to fill the layout?
> >
--
With best wishes
Dmitry
[PATCH 1/2] drm/print: drop include debugfs.h and include where needed
Surprisingly many places depend on debugfs.h to be included via drm_print.h. Fix them. v3: Also fix armada, ite-it6505, imagination, msm, sti, vc4, and xe v2: Also fix ivpu and vmwgfx Reviewed-by: Andrzej Hajda Acked-by: Maxime Ripard Link: https://patchwork.freedesktop.org/patch/msgid/20240410141434.157908-1-jani.nik...@intel.com Signed-off-by: Jani Nikula --- Cc: Jacek Lawrynowicz Cc: Stanislaw Gruszka Cc: Oded Gabbay Cc: Russell King Cc: David Airlie Cc: Daniel Vetter Cc: Andrzej Hajda Cc: Neil Armstrong Cc: Robert Foss Cc: Laurent Pinchart Cc: Jonas Karlman Cc: Jernej Skrabec Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: Jani Nikula Cc: Rodrigo Vivi Cc: Joonas Lahtinen Cc: Tvrtko Ursulin Cc: Frank Binns Cc: Matt Coster Cc: Rob Clark Cc: Abhinav Kumar Cc: Dmitry Baryshkov Cc: Sean Paul Cc: Marijn Suijten Cc: Karol Herbst Cc: Lyude Paul Cc: Danilo Krummrich Cc: Alex Deucher Cc: "Christian König" Cc: "Pan, Xinhui" Cc: Alain Volmat Cc: Huang Rui Cc: Zack Rusin Cc: Broadcom internal kernel review list Cc: Lucas De Marchi Cc: "Thomas Hellström" Cc: dri-de...@lists.freedesktop.org Cc: intel-...@lists.freedesktop.org Cc: intel...@lists.freedesktop.org Cc: linux-arm-...@vger.kernel.org Cc: freedreno@lists.freedesktop.org Cc: nouv...@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- drivers/accel/ivpu/ivpu_debugfs.c | 2 ++ drivers/gpu/drm/armada/armada_debugfs.c | 1 + drivers/gpu/drm/bridge/ite-it6505.c | 1 + drivers/gpu/drm/bridge/panel.c | 2 ++ drivers/gpu/drm/drm_print.c | 6 +++--- drivers/gpu/drm/i915/display/intel_dmc.c| 1 + drivers/gpu/drm/imagination/pvr_fw_trace.c | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 2 ++ drivers/gpu/drm/nouveau/dispnv50/crc.c | 2 ++ drivers/gpu/drm/radeon/r100.c | 1 + drivers/gpu/drm/radeon/r300.c | 1 + drivers/gpu/drm/radeon/r420.c | 1 + drivers/gpu/drm/radeon/r600.c | 3 ++- drivers/gpu/drm/radeon/radeon_fence.c | 1 + drivers/gpu/drm/radeon/radeon_gem.c | 1 + drivers/gpu/drm/radeon/radeon_ib.c | 2 ++ drivers/gpu/drm/radeon/radeon_pm.c | 1 + drivers/gpu/drm/radeon/radeon_ring.c| 2 ++ drivers/gpu/drm/radeon/radeon_ttm.c | 1 + drivers/gpu/drm/radeon/rs400.c | 1 + drivers/gpu/drm/radeon/rv515.c | 1 + drivers/gpu/drm/sti/sti_drv.c | 1 + drivers/gpu/drm/ttm/ttm_device.c| 1 + drivers/gpu/drm/ttm/ttm_resource.c | 3 ++- drivers/gpu/drm/ttm/ttm_tt.c| 5 +++-- drivers/gpu/drm/vc4/vc4_drv.h | 1 + drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 2 ++ drivers/gpu/drm/xe/xe_debugfs.c | 1 + drivers/gpu/drm/xe/xe_gt_debugfs.c | 2 ++ drivers/gpu/drm/xe/xe_uc_debugfs.c | 2 ++ include/drm/drm_print.h | 2 +- 31 files changed, 46 insertions(+), 8 deletions(-) diff --git a/drivers/accel/ivpu/ivpu_debugfs.c b/drivers/accel/ivpu/ivpu_debugfs.c index d09d29775b3f..e07e447d08d1 100644 --- a/drivers/accel/ivpu/ivpu_debugfs.c +++ b/drivers/accel/ivpu/ivpu_debugfs.c @@ -3,6 +3,8 @@ * Copyright (C) 2020-2023 Intel Corporation */ +#include + #include #include #include diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c index 29f4b52e3c8d..a763349dd89f 100644 --- a/drivers/gpu/drm/armada/armada_debugfs.c +++ b/drivers/gpu/drm/armada/armada_debugfs.c @@ -5,6 +5,7 @@ */ #include +#include #include #include #include diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c index 27334173e911..3f68c82888c2 100644 --- a/drivers/gpu/drm/bridge/ite-it6505.c +++ b/drivers/gpu/drm/bridge/ite-it6505.c @@ -3,6 +3,7 @@ * Copyright (c) 2020, The Linux Foundation. All rights reserved. */ #include +#include #include #include #include diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 7f41525f7a6e..32506524d9a2 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -4,6 +4,8 @@ * Copyright (C) 2017 Broadcom */ +#include + #include #include #include diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c index 699b7dbffd7b..cf2efb44722c 100644 --- a/drivers/gpu/drm/drm_print.c +++ b/drivers/gpu/drm/drm_print.c @@ -23,13 +23,13 @@ * Rob Clark */ -#include - +#include +#include #include #include #include #include -#include +#include #include #include diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c index 3697a02b51b6..09346afd1f0e 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc.c +++ b/drivers/gpu/drm/i915/display/intel_dmc.c @@ -22,6 +22,7 @@ * */ +#include #include #include "i915_drv.h" diff --git
Re: [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format
On Fri, Apr 19, 2024 at 07:32:35PM -0700, Abhinav Kumar wrote:
>
>
> On 4/19/2024 6:26 PM, Dmitry Baryshkov wrote:
> > On Fri, Apr 19, 2024 at 04:43:20PM -0700, Abhinav Kumar wrote:
> > >
> > >
> > > On 3/19/2024 6:21 AM, Dmitry Baryshkov wrote:
> > > > The msm_kms_funcs::check_modified_format() callback is not used by the
> > > > driver. Drop it completely.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov
> > > > ---
> > > >drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 45
> > > > -
> > > >drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 15 --
> > > >drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 -
> > > >drivers/gpu/drm/msm/msm_kms.h | 5
> > > >4 files changed, 66 deletions(-)
> > > >
> > >
> > > I think in this case, I am leaning towards completing the implementation
> > > rather than dropping it as usual.
> > >
> > > It seems its easier to just add the support to call this like the attached
> > > patch?
> >
> > Please don't attach patches to the email. It makes it impossible to
> > respond to them.
> >
>
> I attached it because it was too much to paste over here.
>
> Please review msm_framebuffer_init() in the downstream sources.
>
> The only missing piece I can see is the handling of DRM_MODE_FB_MODIFIERS
> flags.
I checked and I don't like this approach.
With the generic formats database in place, there should be no
driver-specific code that handles formats. Moreover, I think this should
be handled by the generic code in framebuffer_check() if msm driver
implements a proper get_format_info() callback. Please consider sending
a patch that does it. For now I can only consider the function in
question to be a dead code which should be dropped.
>
> I am unable to trace back why this support was not present.
>
> > Anyway, what are we missing with the current codebase? Why wasn't the
> > callback / function used in the first place?
> >
> > >
> > > WDYT?
> > >
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > > > index e366ab134249..ff0df478c958 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
> > > > @@ -960,51 +960,6 @@ int dpu_format_populate_layout(
> > > > return ret;
> > > >}
> > > > -int dpu_format_check_modified_format(
> > > > - const struct msm_kms *kms,
> > > > - const struct msm_format *msm_fmt,
> > > > - const struct drm_mode_fb_cmd2 *cmd,
> > > > - struct drm_gem_object **bos)
> > > > -{
> > > > - const struct drm_format_info *info;
> > > > - const struct dpu_format *fmt;
> > > > - struct dpu_hw_fmt_layout layout;
> > > > - uint32_t bos_total_size = 0;
> > > > - int ret, i;
> > > > -
> > > > - if (!msm_fmt || !cmd || !bos) {
> > > > - DRM_ERROR("invalid arguments\n");
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > - fmt = to_dpu_format(msm_fmt);
> > > > - info = drm_format_info(fmt->base.pixel_format);
> > > > - if (!info)
> > > > - return -EINVAL;
> > > > -
> > > > - ret = dpu_format_get_plane_sizes(fmt, cmd->width, cmd->height,
> > > > - , cmd->pitches);
> > > > - if (ret)
> > > > - return ret;
> > > > -
> > > > - for (i = 0; i < info->num_planes; i++) {
> > > > - if (!bos[i]) {
> > > > - DRM_ERROR("invalid handle for plane %d\n", i);
> > > > - return -EINVAL;
> > > > - }
> > > > - if ((i == 0) || (bos[i] != bos[0]))
> > > > - bos_total_size += bos[i]->size;
> > > > - }
> > > > -
> > > > - if (bos_total_size < layout.total_size) {
> > > > - DRM_ERROR("buffers total size too small %u expected
> > > > %u\n",
> > > > - bos_total_size, layout.total_size);
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > - return 0;
> > > > -}
> > > > -
> > > >const struct dpu_format *dpu_get_dpu_format_ext(
> > > > const uint32_t format,
> > > > const uint64_t modifier)
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
> > > > index 84b8b3289f18..9442445f1a86 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
> > > > @@ -54,21 +54,6 @@ const struct msm_format *dpu_get_msm_format(
> > > > const uint32_t format,
> > > > const uint64_t modifiers);
> > > > -/**
> > > > - * dpu_format_check_modified_format - validate format and buffers for
> > > > - * dpu non-standard, i.e. modified format
> > > > - * @kms: kms driver
