Re: [Freedreno] [PATCH v3 6/6] drm/msm: make mdp5/dpu devices master components
Quoting Dmitry Baryshkov (2022-03-23 02:25:38)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 38627ccf3068..ab8a35e09bc9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -381,8 +381,8 @@ static int dpu_kms_parse_data_bus_icc_path(struct dpu_kms
> *dpu_kms)
> struct icc_path *path1;
> struct drm_device *dev = dpu_kms->dev;
>
> - path0 = of_icc_get(dev->dev, "mdp0-mem");
> - path1 = of_icc_get(dev->dev, "mdp1-mem");
> + path0 = of_icc_get(dev->dev->parent, "mdp0-mem");
dev->dev->parent is long
> + path1 = of_icc_get(dev->dev->parent, "mdp1-mem");
>
> if (IS_ERR_OR_NULL(path0))
> return PTR_ERR_OR_ZERO(path0);
> @@ -837,6 +837,9 @@ static void dpu_kms_destroy(struct msm_kms *kms)
> _dpu_kms_hw_destroy(dpu_kms);
>
> msm_kms_destroy(_kms->base);
> +
> + if (dpu_kms->rpm_enabled)
> + pm_runtime_disable(_kms->pdev->dev);
> }
>
> static irqreturn_t dpu_irq(struct msm_kms *kms)
> @@ -978,7 +981,7 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
> if (!domain)
> return 0;
>
> - mmu = msm_iommu_new(dpu_kms->dev->dev, domain);
> + mmu = msm_iommu_new(dpu_kms->dev->dev->parent, domain);
And dpu_kms->dev->dev->parent is longer. Can we get some local variable
or something that is more descriptive? I guess it is an 'mdss_dev'?
> if (IS_ERR(mmu)) {
> iommu_domain_free(domain);
> return PTR_ERR(mmu);
> @@ -1172,40 +1175,15 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
> return rc;
> }
>
> -static int dpu_kms_init(struct drm_device *dev)
> -{
> - struct msm_drm_private *priv;
> - struct dpu_kms *dpu_kms;
> - int irq;
> -
> - if (!dev) {
> - DPU_ERROR("drm device node invalid\n");
> - return -EINVAL;
> - }
> -
> - priv = dev->dev_private;
> - dpu_kms = to_dpu_kms(priv->kms);
> -
> - irq = irq_of_parse_and_map(dpu_kms->pdev->dev.of_node, 0);
> - if (irq < 0) {
> - DPU_ERROR("failed to get irq: %d\n", irq);
> - return irq;
> - }
> - dpu_kms->base.irq = irq;
> -
> - return 0;
> -}
> -
> -static int dpu_bind(struct device *dev, struct device *master, void *data)
> +static int dpu_kms_init(struct drm_device *ddev)
> {
> - struct msm_drm_private *priv = dev_get_drvdata(master);
> + struct msm_drm_private *priv = ddev->dev_private;
> + struct device *dev = ddev->dev;
> struct platform_device *pdev = to_platform_device(dev);
> - struct drm_device *ddev = priv->dev;
> struct dpu_kms *dpu_kms;
> + int irq;
> int ret = 0;
>
> - priv->kms_init = dpu_kms_init;
> -
> dpu_kms = devm_kzalloc(>dev, sizeof(*dpu_kms), GFP_KERNEL);
> if (!dpu_kms)
> return -ENOMEM;
> @@ -1227,8 +1205,6 @@ static int dpu_bind(struct device *dev, struct device
> *master, void *data)
> }
> dpu_kms->num_clocks = ret;
>
> - platform_set_drvdata(pdev, dpu_kms);
> -
> ret = msm_kms_init(_kms->base, _funcs);
> if (ret) {
> DPU_ERROR("failed to init kms, ret=%d\n", ret);
> @@ -1242,31 +1218,25 @@ static int dpu_bind(struct device *dev, struct device
> *master, void *data)
>
> priv->kms = _kms->base;
>
> - return ret;
> -}
> -
> -static void dpu_unbind(struct device *dev, struct device *master, void *data)
> -{
> - struct platform_device *pdev = to_platform_device(dev);
> - struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
> + irq = irq_of_parse_and_map(dpu_kms->pdev->dev.of_node, 0);
Why doesn't platform_get_irq() work? This is code movement but I'm
trying to understand why OF APIs are required.
> + if (irq < 0) {
> + DPU_ERROR("failed to get irq: %d\n", irq);
> + return irq;
> + }
> + dpu_kms->base.irq = irq;
>
> - if (dpu_kms->rpm_enabled)
> - pm_runtime_disable(>dev);
> + return 0;
> }
>
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index 1f571372e928..ab25fff271f9 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -194,9 +194,6 @@ static inline void msm_kms_destroy(struct msm_kms *kms)
> msm_atomic_destroy_pending_timer(>pending_timers[i]);
> }
>
> -extern const struct of_device_id dpu_dt_match[];
> -extern const struct of_device_id mdp5_dt_match[];
> -
> #define for_each_crtc_mask(dev, crtc, crtc_mask) \
> drm_for_each_crtc(crtc, dev) \
> for_each_if (drm_crtc_mask(crtc) & (crtc_mask))
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 7451105cbf01..9ecae833037d 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++
Re: [Freedreno] [PATCH v3 3/6] drm/msm: split the main platform driver
Quoting Dmitry Baryshkov (2022-03-23 02:25:35)
> Currently the msm platform driver is a multiplex handling several cases:
> - headless GPU-only driver,
> - MDP4 with flat device nodes,
> - MDP5/DPU MDSS with all the nodes being children of MDSS node.
>
> This results in not-so-perfect code, checking the hardware version
> (MDP4/MDP5/DPU) in several places, checking for mdss even when it can
> not exist, etc. Split the code into three handling subdrivers (mdp4,
> mdss and headless msm).
>
> Reviewed-by: Abhinav Kumar
> Signed-off-by: Dmitry Baryshkov
> ---
With the match table fixed and the nit below
Reviewed-by: Stephen Boyd
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index 3cf476c55158..c5c0650414c5 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -569,3 +569,59 @@ static struct mdp4_platform_config
> *mdp4_get_config(struct platform_device *dev)
>
> return
> }
> +
> +static const struct dev_pm_ops mdp4_pm_ops = {
> + .prepare = msm_pm_prepare,
> + .complete = msm_pm_complete,
> +};
> +
> +static int mdp4_probe(struct platform_device *pdev)
> +{
> + struct msm_drm_private *priv;
> +
> + priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + /*
> +* on MDP4 based platforms, the MDP platform device is the component
> +* master that adds other display interface components to itself.
Just delete master. It provides no value in this sentence.
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 62007a4f29a2..512708101931 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -255,3 +258,167 @@ struct msm_mdss *msm_mdss_init(struct platform_device
> *pdev, bool is_mdp5)
[...]
> + DRM_DEV_ERROR(dev, "failed to populate children devices\n");
> + goto fail;
> + }
> +
> + mdp_dev = device_find_child(dev, NULL, find_mdp_node);
> + if (!mdp_dev) {
> + DRM_DEV_ERROR(dev, "failed to find MDSS MDP node\n");
> + of_platform_depopulate(dev);
> + ret = -ENODEV;
> + goto fail;
> + }
> +
> + /*
> +* on MDP5 based platforms, the MDSS platform device is the component
> +* that adds MDP5 and other display interface components to
Like here.
Re: [Freedreno] [PATCH] dt-bindings: display/msm: another fix for the dpu-qcm2290 example
Quoting Dmitry Baryshkov (2022-03-24 04:55:36)
> Make dpu-qcm2290 example really follow the defined schema:
> - Drop qcom,mdss compatible. It's only used for MDP5 devices.
> - Change display controller name to display-controller as specified in
> the yaml
>
> Reported-by: Rob Herring
> Cc: Loic Poulain
> Fixes: 164f69d9d45a ("dt-bindings: msm: disp: add yaml schemas for QCM2290
> DPU bindings")
> Signed-off-by: Dmitry Baryshkov
> ---
Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH 2/3] drm/msm/dp: simplify dp_connector_get_modes()
Quoting Dmitry Baryshkov (2022-03-23 03:35:45) > Since dp_panel_get_modes() handling for dp_mode was removed, > dp_display_get_modes also doesn't change the passed dp_mode, drop the > unused dp_mode variable being allocated unused and then freed. > > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH 3/3] drm/msm/dp: remove max_pclk_khz field from dp_panel/dp_display
Quoting Dmitry Baryshkov (2022-03-23 03:35:46) > Since the last commit, the max_pclk_khz became constant, it's set to > DP_MAX_PIXEL_CLK_KHZ and never changed afterwards. Remove it completely > and use DP_MAX_PIXEL_CLK_KHZ directly. > > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Stephen Boyd > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > index af5f1b001192..a94c9b34f397 100644 > --- a/drivers/gpu/drm/msm/dp/dp_drm.c > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > @@ -88,9 +88,7 @@ static enum drm_mode_status dp_connector_mode_valid( > > dp_disp = to_dp_connector(connector)->dp_display; > > - if ((dp_disp->max_pclk_khz <= 0) || > - (dp_disp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) || > - (mode->clock > dp_disp->max_pclk_khz)) > + if (mode->clock > DP_MAX_PIXEL_CLK_KHZ) > return MODE_BAD; Can we have a followup patch to return MODE_CLOCK_HIGH?
Re: [Freedreno] [PATCH 1/3] drm/msm/dp: drop dp_mode argument from dp_panel_get_modes()
Quoting Dmitry Baryshkov (2022-03-23 03:35:44)
> Since the commit ab205927592b ("drm/msm/dp: remove mode hard-coding in
> case of DP CTS") the function dp_panel_get_modes() doesn't use (or fill)
> the dp_mode argument. Drop it completely.
>
> Signed-off-by: Dmitry Baryshkov
> ---
Reviewed-by: Stephen Boyd
Re: [Freedreno] [PATCH] drm/msm/mdp5: check the return of kzalloc()
On 3/24/2022 1:36 AM, xkernel.w...@foxmail.com wrote:
From: Xiaoke Wang
kzalloc() is a memory allocation function which can return NULL when
some internal memory errors happen. So it is better to check it to
prevent potential wrong memory access.
Signed-off-by: Xiaoke Wang
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index c6b69af..5f914cc 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -90,15 +90,18 @@ static void mdp5_plane_reset(struct drm_plane *plane)
__drm_atomic_helper_plane_destroy_state(plane->state);
kfree(to_mdp5_plane_state(plane->state));
- mdp5_state = kzalloc(sizeof(*mdp5_state), GFP_KERNEL);
+ plane->state = NULL;
- if (plane->type == DRM_PLANE_TYPE_PRIMARY)
- mdp5_state->base.zpos = STAGE_BASE;
- else
- mdp5_state->base.zpos = STAGE0 + drm_plane_index(plane);
- mdp5_state->base.normalized_zpos = mdp5_state->base.zpos;
+ mdp5_state = kzalloc(sizeof(*mdp5_state), GFP_KERNEL);
+ if (mdp5_state) {
+ if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+ mdp5_state->base.zpos = STAGE_BASE;
+ else
+ mdp5_state->base.zpos = STAGE0 + drm_plane_index(plane);
+ mdp5_state->base.normalized_zpos = mdp5_state->base.zpos;
- __drm_atomic_helper_plane_reset(plane, _state->base);
+ __drm_atomic_helper_plane_reset(plane, _state->base);
+ }
}
static struct drm_plane_state *
Re: [Freedreno] [PATCH] drm/msm/disp: check the return value of kzalloc()
On 3/24/2022 2:15 AM, xkernel.w...@foxmail.com wrote: From: Xiaoke Wang kzalloc() is a memory allocation function which can return NULL when some internal memory errors happen. So it is better to check it to prevent potential wrong memory access. Signed-off-by: Xiaoke Wang Reviewed-by: Abhinav Kumar --- drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c index cabe151..369e57f 100644 --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c @@ -169,6 +169,8 @@ void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, u32 len, va_list va; new_blk = kzalloc(sizeof(struct msm_disp_state_block), GFP_KERNEL); + if (!new_blk) + return; va_start(va, fmt);
Re: [Freedreno] [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API
Hi Liviu
Thanks for the response.
On 3/24/2022 3:12 AM, Liviu Dudau wrote:
On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
Hi Liviu
Hello,
Thanks for the review.
On 3/23/2022 9:46 AM, Liviu Dudau wrote:
On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
For vendors drivers which pass an already allocated and
initialized encoder especially for cases where the encoder
hardware is shared OR the writeback encoder shares the resources
with the rest of the display pipeline introduce a new API,
drm_writeback_connector_init_with_encoder() which expects
an initialized encoder as a parameter and only sets up the
writeback connector.
changes in v5:
- reorder this change to come before in the series
to avoid incorrect functionality in subsequent changes
- continue using struct drm_encoder instead of
struct drm_encoder * and switch it in next change
Signed-off-by: Abhinav Kumar
Hi Abhinav,
---
drivers/gpu/drm/drm_writeback.c | 143
include/drm/drm_writeback.h | 5 ++
2 files changed, 106 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dc2ef12..abe78b9 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -149,37 +149,15 @@ static const struct drm_encoder_funcs
drm_writeback_encoder_funcs = {
.destroy = drm_encoder_cleanup,
};
-/**
- * drm_writeback_connector_init - Initialize a writeback connector and its
properties
- * @dev: DRM device
- * @wb_connector: Writeback connector to initialize
- * @con_funcs: Connector funcs vtable
- * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal
encoder
- * @formats: Array of supported pixel formats for the writeback engine
- * @n_formats: Length of the formats array
- * @possible_crtcs: possible crtcs for the internal writeback encoder
- *
- * This function creates the writeback-connector-specific properties if they
- * have not been already created, initializes the connector as
- * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
- * values. It will also create an internal encoder associated with the
- * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
- * the encoder helper.
- *
- * Drivers should always use this function instead of drm_connector_init() to
- * set up writeback connectors.
- *
- * Returns: 0 on success, or a negative error code
- */
-int drm_writeback_connector_init(struct drm_device *dev,
-struct drm_writeback_connector *wb_connector,
-const struct drm_connector_funcs *con_funcs,
-const struct drm_encoder_helper_funcs
*enc_helper_funcs,
-const u32 *formats, int n_formats, uint32_t
possible_crtcs)
+static int drm_writeback_connector_setup(struct drm_device *dev,
+ struct drm_writeback_connector *wb_connector,
+ const struct drm_connector_funcs *con_funcs, const u32 *formats,
+ int n_formats)
{
struct drm_property_blob *blob;
- struct drm_connector *connector = _connector->base;
struct drm_mode_config *config = >mode_config;
+ struct drm_connector *connector = _connector->base;
+
Point of this reordering the statements is...?
This diff can be avoided. There was no reason to reorder this. I will remove
this re-order.
int ret = create_writeback_properties(dev);
if (ret != 0)
@@ -187,18 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev,
blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
formats);
- if (IS_ERR(blob))
- return PTR_ERR(blob);
-
- drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
-
- wb_connector->encoder.possible_crtcs = possible_crtcs;
-
- ret = drm_encoder_init(dev, _connector->encoder,
- _writeback_encoder_funcs,
- DRM_MODE_ENCODER_VIRTUAL, NULL);
- if (ret)
- goto fail;
+ if (IS_ERR(blob)) {
+ ret = PTR_ERR(blob);
+ return ret;
+ }
I don't see why you have changed the earlier code to store the result of
PTR_ERR into
ret other than to help your debugging. I suggest that you keep the existing
code that
returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.
Sure, i can fix this for a smaller diff stat.
connector->interlace_allowed = 0;
@@ -237,13 +207,102 @@ int drm_writeback_connector_init(struct drm_device *dev,
attach_fail:
drm_connector_cleanup(connector);
connector_fail:
- drm_encoder_cleanup(_connector->encoder);
-fail:
drm_property_blob_put(blob);
return ret;
}
+
+/**
+ *
Re: [Freedreno] [REPOST PATCH v4 06/13] drm/msm/disp/dpu1: Add DSC support in hw_ctl
On 17-02-22, 23:20, Marijn Suijten wrote:
> On 2022-02-10 16:04:16, Vinod Koul wrote:
> > Later gens of hardware have DSC bits moved to hw_ctl, so configure these
> > bits so that DSC would work there as well
> >
> > Reviewed-by: Dmitry Baryshkov
> > Signed-off-by: Vinod Koul
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 11 ++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 2 ++
> > 2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > index 02da9ecf71f1..49659165cea8 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > @@ -25,6 +25,8 @@
> > #define CTL_MERGE_3D_ACTIVE 0x0E4
> > #define CTL_INTF_ACTIVE 0x0F4
> > #define CTL_MERGE_3D_FLUSH0x100
> > +#define CTL_DSC_ACTIVE0x0E8
> > +#define CTL_DSC_FLUSH0x104
> > #define CTL_INTF_FLUSH0x110
> > #define CTL_INTF_MASTER 0x134
> > #define CTL_FETCH_PIPE_ACTIVE 0x0FC
> > @@ -34,6 +36,7 @@
> >
> > #define DPU_REG_RESET_TIMEOUT_US2000
> > #define MERGE_3D_IDX 23
> > +#define DSC_IDX22
>
> This define does not seem used in any of these patches. Is that
> intended?
This should used in the below case you pointed, updated now
> > static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> > index 806c171e5df2..9847c9c46d6f 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h
> > @@ -40,6 +40,7 @@ struct dpu_hw_stage_cfg {
> > * @merge_3d: 3d merge block used
> > * @intf_mode_sel: Interface mode, cmd / vid
> > * @stream_sel:Stream selection for multi-stream interfaces
> > + * @dsc: DSC BIT masks
>
> Bit masks of what? Enabled DSCs? A more verbose doc-comment is desired
> here, matching the rest of the fields :) - something like "DSC block(s)
> used" similar to merge_3d? Or copy the docs from `dsc_mask`, which is
> the value that is written into this field.
Updated
--
~Vinod
Re: [Freedreno] [REPOST PATCH v4 13/13] drm/msm/dsi: Add support for DSC configuration
On 21-02-22, 05:11, Dmitry Baryshkov wrote:
> On 10/02/2022 13:34, Vinod Koul wrote:
> > When DSC is enabled, we need to configure DSI registers accordingly and
> > configure the respective stream compression registers.
> >
> > Add support to calculate the register setting based on DSC params and
> > timing information and configure these registers.
> >
> > Signed-off-by: Dmitry Baryshkov
> > Signed-off-by: Vinod Koul
> > ---
> > drivers/gpu/drm/msm/dsi/dsi.xml.h | 10 +++
> > drivers/gpu/drm/msm/dsi/dsi_host.c | 109 -
> > 2 files changed, 118 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > index 49b551ad1bff..c1c85df58c4b 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > @@ -706,4 +706,14 @@ static inline uint32_t DSI_VERSION_MAJOR(uint32_t val)
> > #define REG_DSI_CPHY_MODE_CTRL
> > 0x02d4
> > +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL
> > 0x029c
> > +
> > +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL2
> > 0x02a0
> > +
> > +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL
> > 0x02a4
> > +
> > +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2
> > 0x02a8
> > +
> > +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL3
> > 0x02ac
> > +
> > #endif /* DSI_XML */
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 438c80750682..3d8d5a1daaa3 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -908,6 +908,20 @@ static void dsi_ctrl_config(struct msm_dsi_host
> > *msm_host, bool enable,
> > dsi_write(msm_host, REG_DSI_CPHY_MODE_CTRL, BIT(0));
> > }
> > +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> > + int pic_width, int pic_height)
> > +{
> > + if (!dsc || !pic_width || !pic_height) {
> > + pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n",
> > pic_width, pic_height);
> > + return -EINVAL;
> > + }
> > +
> > + dsc->drm->pic_width = pic_width;
> > + dsc->drm->pic_height = pic_height;
> > +
> > + return 0;
> > +}
> > +
> > static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool
> > is_bonded_dsi)
> > {
> > struct drm_display_mode *mode = msm_host->mode;
> > @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host
> > *msm_host, bool is_bonded_dsi)
> > hdisplay /= 2;
> > }
> > + if (msm_host->dsc) {
> > + struct msm_display_dsc_config *dsc = msm_host->dsc;
> > +
> > + /* update dsc params with timing params */
> > + dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
> > + DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width,
> > dsc->drm->pic_height);
> > +
> > + /* we do the calculations for dsc parameters here so that
> > +* panel can use these parameters
> > +*/
> > + dsi_populate_dsc_params(dsc);
> > +
> > + /* Divide the display by 3 but keep back/font porch and
> > +* pulse width same
> > +*/
> > + h_total -= hdisplay;
> > + hdisplay /= 3;
> > + h_total += hdisplay;
> > + ha_end = ha_start + hdisplay;
> > + }
> > +
> > if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
> > + if (msm_host->dsc) {
> > + struct msm_display_dsc_config *dsc = msm_host->dsc;
> > + u32 reg, intf_width, slice_per_intf;
> > + u32 total_bytes_per_intf;
> > +
> > + /* first calculate dsc parameters and then program
> > +* compress mode registers
> > +*/
> > + intf_width = hdisplay;
> > + slice_per_intf = DIV_ROUND_UP(intf_width,
> > dsc->drm->slice_width);
> > +
> > + dsc->drm->slice_count = 1;
> > + dsc->bytes_in_slice =
> > DIV_ROUND_UP(dsc->drm->slice_width * 8, 8);
> > + total_bytes_per_intf = dsc->bytes_in_slice *
> > slice_per_intf;
> > +
> > + dsc->eol_byte_num = total_bytes_per_intf % 3;
> > + dsc->pclk_per_line =
> > DIV_ROUND_UP(total_bytes_per_intf, 3);
> > + dsc->bytes_per_pkt = dsc->bytes_in_slice *
> > dsc->drm->slice_count;
> > + dsc->pkt_per_line = slice_per_intf /
> > dsc->drm->slice_count;
> > +
> > + reg = dsc->bytes_per_pkt << 16;
> > + reg |= (0x0b << 8);/* dtype of compressed image */
> > +
> > + /* pkt_per_line:
> > +* 0 == 1 pkt
> > +* 1 == 2 pkt
> > +
Re: [Freedreno] [REPOST PATCH v4 07/13] drm/msm/disp/dpu1: Add support for DSC in encoder
On 23-03-22, 20:10, Vinod Koul wrote: > On 17-02-22, 23:32, Marijn Suijten wrote: > > On 2022-02-10 16:04:17, Vinod Koul wrote: > > > + > > > + slice_count = dsc->drm->slice_count; > > > + slice_per_intf = DIV_ROUND_UP(width, dsc->drm->slice_width); > > > + > > > + /* > > > + * If slice_count is greater than slice_per_intf then default to 1. > > > + * This can happen during partial update. > > > + */ > > > + if (slice_count > slice_per_intf) > > > + slice_count = 1; > > > + > > > + bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * > > > + dsc->drm->bits_per_pixel, 8); > > > + total_bytes_per_intf = bytes_in_slice * slice_per_intf; > > > + > > > + dsc->eol_byte_num = total_bytes_per_intf % 3; > > > + dsc->pclk_per_line = DIV_ROUND_UP(total_bytes_per_intf, 3); > > > + dsc->bytes_in_slice = bytes_in_slice; > > > + dsc->bytes_per_pkt = bytes_in_slice * slice_count; > > > + dsc->pkt_per_line = slice_per_intf / slice_count; > > > +} > > > > I've seen the same calculations duplicated twice in dsi code. Since the > > msm_display_dsc_config struct is available in a header, perhaps a single > > - easily reviewable and maintainable - calculation function should be > > available there too? > > Let me try check if we can make it common.. I rechecked and we can actually remove this as we do this caln in timing and update the dsc structure there. So this fn is dropped now -- ~Vinod
[Freedreno] [PATCH] drm/msm/mdp5: check the return of kzalloc()
From: Xiaoke Wang
kzalloc() is a memory allocation function which can return NULL when
some internal memory errors happen. So it is better to check it to
prevent potential wrong memory access.
Signed-off-by: Xiaoke Wang
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index c6b69af..5f914cc 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -90,15 +90,18 @@ static void mdp5_plane_reset(struct drm_plane *plane)
__drm_atomic_helper_plane_destroy_state(plane->state);
kfree(to_mdp5_plane_state(plane->state));
- mdp5_state = kzalloc(sizeof(*mdp5_state), GFP_KERNEL);
+ plane->state = NULL;
- if (plane->type == DRM_PLANE_TYPE_PRIMARY)
- mdp5_state->base.zpos = STAGE_BASE;
- else
- mdp5_state->base.zpos = STAGE0 + drm_plane_index(plane);
- mdp5_state->base.normalized_zpos = mdp5_state->base.zpos;
+ mdp5_state = kzalloc(sizeof(*mdp5_state), GFP_KERNEL);
+ if (mdp5_state) {
+ if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+ mdp5_state->base.zpos = STAGE_BASE;
+ else
+ mdp5_state->base.zpos = STAGE0 + drm_plane_index(plane);
+ mdp5_state->base.normalized_zpos = mdp5_state->base.zpos;
- __drm_atomic_helper_plane_reset(plane, _state->base);
+ __drm_atomic_helper_plane_reset(plane, _state->base);
+ }
}
static struct drm_plane_state *
--
[Freedreno] [PATCH] drm/msm/disp: check the return value of kzalloc()
From: Xiaoke Wang kzalloc() is a memory allocation function which can return NULL when some internal memory errors happen. So it is better to check it to prevent potential wrong memory access. Signed-off-by: Xiaoke Wang --- drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c index cabe151..369e57f 100644 --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c @@ -169,6 +169,8 @@ void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, u32 len, va_list va; new_blk = kzalloc(sizeof(struct msm_disp_state_block), GFP_KERNEL); + if (!new_blk) + return; va_start(va, fmt); --
Re: [Freedreno] [PATCH] dt-bindings: display/msm: another fix for the dpu-qcm2290 example
On 24/03/2022 12:55, Dmitry Baryshkov wrote:
> Make dpu-qcm2290 example really follow the defined schema:
> - Drop qcom,mdss compatible. It's only used for MDP5 devices.
> - Change display controller name to display-controller as specified in
> the yaml
>
> Reported-by: Rob Herring
> Cc: Loic Poulain
> Fixes: 164f69d9d45a ("dt-bindings: msm: disp: add yaml schemas for QCM2290
> DPU bindings")
> Signed-off-by: Dmitry Baryshkov
> ---
> .../devicetree/bindings/display/msm/dpu-qcm2290.yaml | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dpu-qcm2290.yaml
> b/Documentation/devicetree/bindings/display/msm/dpu-qcm2290.yaml
> index d31483a78eab..6fb7e321f011 100644
> --- a/Documentation/devicetree/bindings/display/msm/dpu-qcm2290.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dpu-qcm2290.yaml
> @@ -160,7 +160,7 @@ examples:
> mdss: mdss@5e0 {
> #address-cells = <1>;
> #size-cells = <1>;
> -compatible = "qcom,qcm2290-mdss", "qcom,mdss";
> +compatible = "qcom,qcm2290-mdss";
That's quite unfortunate choice of compatibles. I would assume qcom,mdss
is a generic fallback compatible but it is used in different way - as a
specific compatible for MDP v5. The bindings here are for a newer
device, right?
It's already in the bindings, so not much could be fixed now...
Reviewed-by: Krzysztof Kozlowski
Best regards,
Krzysztof
[Freedreno] [PATCH] dt-bindings: display/msm: another fix for the dpu-qcm2290 example
Make dpu-qcm2290 example really follow the defined schema:
- Drop qcom,mdss compatible. It's only used for MDP5 devices.
- Change display controller name to display-controller as specified in
the yaml
Reported-by: Rob Herring
Cc: Loic Poulain
Fixes: 164f69d9d45a ("dt-bindings: msm: disp: add yaml schemas for QCM2290 DPU
bindings")
Signed-off-by: Dmitry Baryshkov
---
.../devicetree/bindings/display/msm/dpu-qcm2290.yaml | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/msm/dpu-qcm2290.yaml
b/Documentation/devicetree/bindings/display/msm/dpu-qcm2290.yaml
index d31483a78eab..6fb7e321f011 100644
--- a/Documentation/devicetree/bindings/display/msm/dpu-qcm2290.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dpu-qcm2290.yaml
@@ -160,7 +160,7 @@ examples:
mdss: mdss@5e0 {
#address-cells = <1>;
#size-cells = <1>;
-compatible = "qcom,qcm2290-mdss", "qcom,mdss";
+compatible = "qcom,qcm2290-mdss";
reg = <0x05e0 0x1000>;
reg-names = "mdss";
power-domains = < MDSS_GDSC>;
@@ -180,7 +180,7 @@ examples:
<_smmu 0x421 0x0>;
ranges;
-mdss_mdp: mdp@5e01000 {
+mdss_mdp: display-controller@5e01000 {
compatible = "qcom,qcm2290-dpu";
reg = <0x05e01000 0x8f000>,
<0x05eb 0x2008>;
--
2.35.1
Re: [Freedreno] [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API
On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
> Hi Liviu
Hello,
>
> Thanks for the review.
>
> On 3/23/2022 9:46 AM, Liviu Dudau wrote:
> > On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
> > > For vendors drivers which pass an already allocated and
> > > initialized encoder especially for cases where the encoder
> > > hardware is shared OR the writeback encoder shares the resources
> > > with the rest of the display pipeline introduce a new API,
> > > drm_writeback_connector_init_with_encoder() which expects
> > > an initialized encoder as a parameter and only sets up the
> > > writeback connector.
> > >
> > > changes in v5:
> > > - reorder this change to come before in the series
> > > to avoid incorrect functionality in subsequent changes
> > > - continue using struct drm_encoder instead of
> > > struct drm_encoder * and switch it in next change
> > >
> > > Signed-off-by: Abhinav Kumar
> >
> > Hi Abhinav,
> >
> > > ---
> > > drivers/gpu/drm/drm_writeback.c | 143
> > >
> > > include/drm/drm_writeback.h | 5 ++
> > > 2 files changed, 106 insertions(+), 42 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > b/drivers/gpu/drm/drm_writeback.c
> > > index dc2ef12..abe78b9 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs
> > > drm_writeback_encoder_funcs = {
> > > .destroy = drm_encoder_cleanup,
> > > };
> > > -/**
> > > - * drm_writeback_connector_init - Initialize a writeback connector and
> > > its properties
> > > - * @dev: DRM device
> > > - * @wb_connector: Writeback connector to initialize
> > > - * @con_funcs: Connector funcs vtable
> > > - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the
> > > internal encoder
> > > - * @formats: Array of supported pixel formats for the writeback engine
> > > - * @n_formats: Length of the formats array
> > > - * @possible_crtcs: possible crtcs for the internal writeback encoder
> > > - *
> > > - * This function creates the writeback-connector-specific properties if
> > > they
> > > - * have not been already created, initializes the connector as
> > > - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the
> > > property
> > > - * values. It will also create an internal encoder associated with the
> > > - * drm_writeback_connector and set it to use the @enc_helper_funcs
> > > vtable for
> > > - * the encoder helper.
> > > - *
> > > - * Drivers should always use this function instead of
> > > drm_connector_init() to
> > > - * set up writeback connectors.
> > > - *
> > > - * Returns: 0 on success, or a negative error code
> > > - */
> > > -int drm_writeback_connector_init(struct drm_device *dev,
> > > - struct drm_writeback_connector *wb_connector,
> > > - const struct drm_connector_funcs *con_funcs,
> > > - const struct drm_encoder_helper_funcs
> > > *enc_helper_funcs,
> > > - const u32 *formats, int n_formats, uint32_t
> > > possible_crtcs)
> > > +static int drm_writeback_connector_setup(struct drm_device *dev,
> > > + struct drm_writeback_connector *wb_connector,
> > > + const struct drm_connector_funcs *con_funcs, const u32 *formats,
> > > + int n_formats)
> > > {
> > > struct drm_property_blob *blob;
> > > - struct drm_connector *connector = _connector->base;
> > > struct drm_mode_config *config = >mode_config;
> > > + struct drm_connector *connector = _connector->base;
> > > +
> >
> > Point of this reordering the statements is...?
> This diff can be avoided. There was no reason to reorder this. I will remove
> this re-order.
> >
> > > int ret = create_writeback_properties(dev);
> > > if (ret != 0)
> > > @@ -187,18 +165,10 @@ int drm_writeback_connector_init(struct drm_device
> > > *dev,
> > > blob = drm_property_create_blob(dev, n_formats *
> > > sizeof(*formats),
> > > formats);
> > > - if (IS_ERR(blob))
> > > - return PTR_ERR(blob);
> > > -
> > > - drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> > > -
> > > - wb_connector->encoder.possible_crtcs = possible_crtcs;
> > > -
> > > - ret = drm_encoder_init(dev, _connector->encoder,
> > > -_writeback_encoder_funcs,
> > > -DRM_MODE_ENCODER_VIRTUAL, NULL);
> > > - if (ret)
> > > - goto fail;
> > > + if (IS_ERR(blob)) {
> > > + ret = PTR_ERR(blob);
> > > + return ret;
> > > + }
> >
> > I don't see why you have changed the earlier code to store the result of
> > PTR_ERR into
> > ret other than to help your debugging. I suggest that you keep the existing
> > code that
> > returns PTR_ERR(blob) directly and you will
