* [PATCH 0/9] drm/msm/dpu: be more friendly to X.org
@ 2024-03-19 13:21 Dmitry Baryshkov
2024-03-19 13:21 ` [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format Dmitry Baryshkov
` (8 more replies)
0 siblings, 9 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 13:21 UTC (permalink / raw
To: Rob Clark, Abhinav Kumar, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
Unlike other compositors X.org allocates a single framebuffer covering
the whole screen space. This is not an issue with the single screens,
but with the multi-monitor setup 5120x4096 becomes a limiting factor.
Check the hardware-bound limitations and lift the FB max size to
16383x16383.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (9):
drm/msm/dpu: drop dpu_format_check_modified_format
drm/msm/dpu: drop dpu_format_populate_layout from dpu_plane_sspp_atomic_update
drm/msm/dpu: split dpu_format_populate_layout
drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check
drm/msm/dpu: check for the plane pitch overflow
drm/msm/dpu: drop call to _dpu_crtc_setup_lm_bounds from atomic_begin
drm/msm/dpu: check for overflow in _dpu_crtc_setup_lm_bounds()
drm/msm/dpu: merge MAX_IMG_WIDTH/HEIGHT with DPU_MAX_IMG_WIDTH/HEIGHT
drm/msm/dpu: sync mode_config limits to the FB limits in dpu_plane.c
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 17 ++--
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 8 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 92 ++++++----------------
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 23 ++----
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 2 +
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 10 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 37 +++++----
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 3 +
drivers/gpu/drm/msm/msm_kms.h | 5 --
10 files changed, 75 insertions(+), 126 deletions(-)
---
base-commit: 8ffc8b1bbd505e27e2c8439d326b6059c906c9dd
change-id: 20240318-dpu-mode-config-width-626d3c7ad52a
Best regards,
--
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format
2024-03-19 13:21 [PATCH 0/9] drm/msm/dpu: be more friendly to X.org Dmitry Baryshkov
@ 2024-03-19 13:21 ` Dmitry Baryshkov
2024-04-19 23:43 ` Abhinav Kumar
2024-03-19 13:22 ` [PATCH 2/9] drm/msm/dpu: drop dpu_format_populate_layout from dpu_plane_sspp_atomic_update Dmitry Baryshkov
` (7 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 13:21 UTC (permalink / raw
To: Rob Clark, Abhinav Kumar, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
The msm_kms_funcs::check_modified_format() callback is not used by the
driver. Drop it completely.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
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(-)
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,
- &layout, 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
- * @msm_fmt: pointer to the msm_fmt base pointer of an dpu_format
- * @cmd: fb_cmd2 structure user request
- * @bos: gem buffer object list
- *
- * Return: error code on failure, 0 on success
- */
-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);
/**
* dpu_format_populate_layout - populate the given format layout based on
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index a1f5d7c4ab91..7257ac4020d8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -969,7 +969,6 @@ static const struct msm_kms_funcs kms_funcs = {
.complete_commit = dpu_kms_complete_commit,
.enable_vblank = dpu_kms_enable_vblank,
.disable_vblank = dpu_kms_disable_vblank,
- .check_modified_format = dpu_format_check_modified_format,
.get_format = dpu_get_msm_format,
.destroy = dpu_kms_destroy,
.snapshot = dpu_kms_mdp_snapshot,
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 0641f6111b93..b794ed918b56 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -96,11 +96,6 @@ struct msm_kms_funcs {
const struct msm_format *(*get_format)(struct msm_kms *kms,
const uint32_t format,
const uint64_t modifiers);
- /* do format checking on format modified through fb_cmd2 modifiers */
- int (*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);
/* misc: */
long (*round_pixclk)(struct msm_kms *kms, unsigned long rate,
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/9] drm/msm/dpu: drop dpu_format_populate_layout from dpu_plane_sspp_atomic_update
2024-03-19 13:21 [PATCH 0/9] drm/msm/dpu: be more friendly to X.org Dmitry Baryshkov
2024-03-19 13:21 ` [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format Dmitry Baryshkov
@ 2024-03-19 13:22 ` Dmitry Baryshkov
2024-04-19 23:55 ` Abhinav Kumar
2024-03-19 13:22 ` [PATCH 3/9] drm/msm/dpu: split dpu_format_populate_layout Dmitry Baryshkov
` (6 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 13:22 UTC (permalink / raw
To: Rob Clark, Abhinav Kumar, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
The dpu_plane_prepare_fb() already calls dpu_format_populate_layout().
Store the generated layour in the plane state and drop this call from
dpu_plane_sspp_update().
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 19 ++++---------------
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 3 +++
2 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index ff975ad51145..5b15d8068187 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -646,7 +646,6 @@ static int dpu_plane_prepare_fb(struct drm_plane *plane,
struct drm_framebuffer *fb = new_state->fb;
struct dpu_plane *pdpu = to_dpu_plane(plane);
struct dpu_plane_state *pstate = to_dpu_plane_state(new_state);
- struct dpu_hw_fmt_layout layout;
struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
int ret;
@@ -676,7 +675,8 @@ static int dpu_plane_prepare_fb(struct drm_plane *plane,
/* validate framebuffer layout before commit */
ret = dpu_format_populate_layout(pstate->aspace,
- new_state->fb, &layout);
+ new_state->fb,
+ &pstate->layout);
if (ret) {
DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret);
return ret;
@@ -1099,17 +1099,6 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
to_dpu_format(msm_framebuffer_format(fb));
struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg;
struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
- struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
- struct msm_gem_address_space *aspace = kms->base.aspace;
- struct dpu_hw_fmt_layout layout;
- bool layout_valid = false;
- int ret;
-
- ret = dpu_format_populate_layout(aspace, fb, &layout);
- if (ret)
- DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret);
- else
- layout_valid = true;
pstate->pending = true;
@@ -1124,12 +1113,12 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
dpu_plane_sspp_update_pipe(plane, pipe, pipe_cfg, fmt,
drm_mode_vrefresh(&crtc->mode),
- layout_valid ? &layout : NULL);
+ &pstate->layout);
if (r_pipe->sspp) {
dpu_plane_sspp_update_pipe(plane, r_pipe, r_pipe_cfg, fmt,
drm_mode_vrefresh(&crtc->mode),
- layout_valid ? &layout : NULL);
+ &pstate->layout);
}
if (pstate->needs_qos_remap)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
index abd6b21a049b..348b0075d1ce 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
@@ -31,6 +31,7 @@
* @plane_clk: calculated clk per plane
* @needs_dirtyfb: whether attached CRTC needs pixel data explicitly flushed
* @rotation: simplified drm rotation hint
+ * @layout: framebuffer memory layout
*/
struct dpu_plane_state {
struct drm_plane_state base;
@@ -48,6 +49,8 @@ struct dpu_plane_state {
bool needs_dirtyfb;
unsigned int rotation;
+
+ struct dpu_hw_fmt_layout layout;
};
#define to_dpu_plane_state(x) \
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/9] drm/msm/dpu: split dpu_format_populate_layout
2024-03-19 13:21 [PATCH 0/9] drm/msm/dpu: be more friendly to X.org Dmitry Baryshkov
2024-03-19 13:21 ` [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format Dmitry Baryshkov
2024-03-19 13:22 ` [PATCH 2/9] drm/msm/dpu: drop dpu_format_populate_layout from dpu_plane_sspp_atomic_update Dmitry Baryshkov
@ 2024-03-19 13:22 ` Dmitry Baryshkov
2024-04-20 0:08 ` Abhinav Kumar
2024-03-19 13:22 ` [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check Dmitry Baryshkov
` (5 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 13:22 UTC (permalink / raw
To: Rob Clark, Abhinav Kumar, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
Split dpu_format_populate_layout() into addess-related and
pitch/format-related parts.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 8 +++-
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 44 +++++++++++-----------
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 8 +++-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 12 ++++--
4 files changed, 45 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 1924a2b28e53..0fd85c0479ec 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -584,7 +584,13 @@ static void dpu_encoder_phys_wb_prepare_wb_job(struct dpu_encoder_phys *phys_enc
return;
}
- ret = dpu_format_populate_layout(aspace, job->fb, &wb_cfg->dest);
+ ret = dpu_format_populate_plane_sizes(job->fb, &wb_cfg->dest);
+ if (ret) {
+ DPU_DEBUG("failed to populate plane sizes%d\n", ret);
+ return;
+ }
+
+ ret = dpu_format_populate_addrs(aspace, job->fb, &wb_cfg->dest);
if (ret) {
DPU_DEBUG("failed to populate layout %d\n", ret);
return;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
index ff0df478c958..1fc9817278df 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
@@ -629,7 +629,7 @@ static int _dpu_format_get_media_color_ubwc(const struct dpu_format *fmt)
return color_fmt;
}
-static int _dpu_format_get_plane_sizes_ubwc(
+static int _dpu_format_populate_plane_sizes_ubwc(
const struct dpu_format *fmt,
const uint32_t width,
const uint32_t height,
@@ -708,7 +708,7 @@ static int _dpu_format_get_plane_sizes_ubwc(
return 0;
}
-static int _dpu_format_get_plane_sizes_linear(
+static int _dpu_format_populate_plane_sizes_linear(
const struct dpu_format *fmt,
const uint32_t width,
const uint32_t height,
@@ -780,27 +780,37 @@ static int _dpu_format_get_plane_sizes_linear(
return 0;
}
-static int dpu_format_get_plane_sizes(
- const struct dpu_format *fmt,
- const uint32_t w,
- const uint32_t h,
- struct dpu_hw_fmt_layout *layout,
- const uint32_t *pitches)
+/*
+ * dpu_format_populate_addrs - populate non-address part of the layout based on
+ * fb, and format found in the fb
+ * @fb: framebuffer pointer
+ * @layout: format layout structure to populate
+ *
+ * Return: error code on failure or 0 if new addresses were populated
+ */
+int dpu_format_populate_plane_sizes(
+ struct drm_framebuffer *fb,
+ struct dpu_hw_fmt_layout *layout)
{
- if (!layout || !fmt) {
+ struct dpu_format *fmt;
+
+ if (!layout || !fb) {
DRM_ERROR("invalid pointer\n");
return -EINVAL;
}
- if ((w > DPU_MAX_IMG_WIDTH) || (h > DPU_MAX_IMG_HEIGHT)) {
+ if ((fb->width > DPU_MAX_IMG_WIDTH) || (fb->height > DPU_MAX_IMG_HEIGHT)) {
DRM_ERROR("image dimensions outside max range\n");
return -ERANGE;
}
+ fmt = to_dpu_format(msm_framebuffer_format(fb));
+
if (DPU_FORMAT_IS_UBWC(fmt) || DPU_FORMAT_IS_TILE(fmt))
- return _dpu_format_get_plane_sizes_ubwc(fmt, w, h, layout);
+ return _dpu_format_populate_plane_sizes_ubwc(fmt, fb->width, fb->height, layout);
- return _dpu_format_get_plane_sizes_linear(fmt, w, h, layout, pitches);
+ return _dpu_format_populate_plane_sizes_linear(fmt, fb->width, fb->height,
+ layout, fb->pitches);
}
static int _dpu_format_populate_addrs_ubwc(
@@ -924,7 +934,7 @@ static int _dpu_format_populate_addrs_linear(
return 0;
}
-int dpu_format_populate_layout(
+int dpu_format_populate_addrs(
struct msm_gem_address_space *aspace,
struct drm_framebuffer *fb,
struct dpu_hw_fmt_layout *layout)
@@ -942,14 +952,6 @@ int dpu_format_populate_layout(
return -ERANGE;
}
- layout->format = to_dpu_format(msm_framebuffer_format(fb));
-
- /* Populate the plane sizes etc via get_format */
- ret = dpu_format_get_plane_sizes(layout->format, fb->width, fb->height,
- layout, fb->pitches);
- if (ret)
- return ret;
-
/* Populate the addresses given the fb */
if (DPU_FORMAT_IS_UBWC(layout->format) ||
DPU_FORMAT_IS_TILE(layout->format))
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
index 9442445f1a86..dc050e253db9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
@@ -56,7 +56,7 @@ const struct msm_format *dpu_get_msm_format(
/**
- * dpu_format_populate_layout - populate the given format layout based on
+ * dpu_format_populate_addrs - populate buffer addresses based on
* mmu, fb, and format found in the fb
* @aspace: address space pointer
* @fb: framebuffer pointer
@@ -65,9 +65,13 @@ const struct msm_format *dpu_get_msm_format(
* Return: error code on failure, -EAGAIN if success but the addresses
* are the same as before or 0 if new addresses were populated
*/
-int dpu_format_populate_layout(
+int dpu_format_populate_addrs(
struct msm_gem_address_space *aspace,
struct drm_framebuffer *fb,
struct dpu_hw_fmt_layout *fmtl);
+int dpu_format_populate_plane_sizes(
+ struct drm_framebuffer *fb,
+ struct dpu_hw_fmt_layout *layout);
+
#endif /*_DPU_FORMATS_H */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 5b15d8068187..d9631fe90228 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -673,10 +673,16 @@ static int dpu_plane_prepare_fb(struct drm_plane *plane,
}
}
+ ret = dpu_format_populate_plane_sizes(new_state->fb, &pstate->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_layout(pstate->aspace,
- new_state->fb,
- &pstate->layout);
+ ret = dpu_format_populate_addrs(pstate->aspace,
+ new_state->fb,
+ &pstate->layout);
if (ret) {
DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret);
return ret;
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check
2024-03-19 13:21 [PATCH 0/9] drm/msm/dpu: be more friendly to X.org Dmitry Baryshkov
` (2 preceding siblings ...)
2024-03-19 13:22 ` [PATCH 3/9] drm/msm/dpu: split dpu_format_populate_layout Dmitry Baryshkov
@ 2024-03-19 13:22 ` Dmitry Baryshkov
2024-04-20 0:14 ` Abhinav Kumar
2024-03-19 13:22 ` [PATCH 5/9] drm/msm/dpu: check for the plane pitch overflow Dmitry Baryshkov
` (4 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 13:22 UTC (permalink / raw
To: Rob Clark, Abhinav Kumar, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
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 <dmitry.baryshkov@linaro.org>
---
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, &pstate->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, &pstate->layout);
+ if (ret) {
+ DPU_ERROR_PLANE(pdpu, "failed to get format plane sizes, %d\n", ret);
+ return ret;
+ }
+
fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
max_linewidth = pdpu->catalog->caps->max_linewidth;
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 5/9] drm/msm/dpu: check for the plane pitch overflow
2024-03-19 13:21 [PATCH 0/9] drm/msm/dpu: be more friendly to X.org Dmitry Baryshkov
` (3 preceding siblings ...)
2024-03-19 13:22 ` [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check Dmitry Baryshkov
@ 2024-03-19 13:22 ` Dmitry Baryshkov
2024-04-20 0:16 ` Abhinav Kumar
2024-03-19 13:22 ` [PATCH 6/9] drm/msm/dpu: drop call to _dpu_crtc_setup_lm_bounds from atomic_begin Dmitry Baryshkov
` (3 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 13:22 UTC (permalink / raw
To: Rob Clark, Abhinav Kumar, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
Check that the plane pitch doesn't overflow the maximum pitch size
allowed by the hardware.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 6 +++++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
index b7dc52312c39..86b1defa5d21 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
@@ -12,6 +12,8 @@
struct dpu_hw_sspp;
+#define DPU_SSPP_MAX_PITCH_SIZE 0xffff
+
/**
* Flags
*/
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index a9de1fbd0df3..9e57c51f5343 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -790,7 +790,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
{
struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
plane);
- int ret = 0, min_scale;
+ int i, ret = 0, min_scale;
struct dpu_plane *pdpu = to_dpu_plane(plane);
struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
u64 max_mdp_clk_rate = kms->perf.max_core_clk_rate;
@@ -864,6 +864,10 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
return ret;
}
+ for (i = 0; i < pstate->layout.num_planes; i++)
+ if (pstate->layout.plane_pitch[i] > DPU_SSPP_MAX_PITCH_SIZE)
+ return -E2BIG;
+
fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
max_linewidth = pdpu->catalog->caps->max_linewidth;
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 6/9] drm/msm/dpu: drop call to _dpu_crtc_setup_lm_bounds from atomic_begin
2024-03-19 13:21 [PATCH 0/9] drm/msm/dpu: be more friendly to X.org Dmitry Baryshkov
` (4 preceding siblings ...)
2024-03-19 13:22 ` [PATCH 5/9] drm/msm/dpu: check for the plane pitch overflow Dmitry Baryshkov
@ 2024-03-19 13:22 ` Dmitry Baryshkov
2024-03-19 13:22 ` [PATCH 7/9] drm/msm/dpu: check for overflow in _dpu_crtc_setup_lm_bounds() Dmitry Baryshkov
` (2 subsequent siblings)
8 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 13:22 UTC (permalink / raw
To: Rob Clark, Abhinav Kumar, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
The dpu_crtc_atomic_check() already calls _dpu_crtc_setup_lm_bounds().
There is no need to call it from dpu_crtc_atomic_begin().
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 88c2e51ab166..64befead366f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -803,8 +803,6 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc,
DRM_DEBUG_ATOMIC("crtc%d\n", crtc->base.id);
- _dpu_crtc_setup_lm_bounds(crtc, crtc->state);
-
/* encoder will trigger pending mask now */
drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
dpu_encoder_trigger_kickoff_pending(encoder);
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 7/9] drm/msm/dpu: check for overflow in _dpu_crtc_setup_lm_bounds()
2024-03-19 13:21 [PATCH 0/9] drm/msm/dpu: be more friendly to X.org Dmitry Baryshkov
` (5 preceding siblings ...)
2024-03-19 13:22 ` [PATCH 6/9] drm/msm/dpu: drop call to _dpu_crtc_setup_lm_bounds from atomic_begin Dmitry Baryshkov
@ 2024-03-19 13:22 ` Dmitry Baryshkov
2024-03-19 13:22 ` [PATCH 8/9] drm/msm/dpu: merge MAX_IMG_WIDTH/HEIGHT with DPU_MAX_IMG_WIDTH/HEIGHT Dmitry Baryshkov
2024-03-19 13:22 ` [PATCH 9/9] drm/msm/dpu: sync mode_config limits to the FB limits in dpu_plane.c Dmitry Baryshkov
8 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 13:22 UTC (permalink / raw
To: Rob Clark, Abhinav Kumar, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
Check in _dpu_crtc_setup_lm_bounds() that CRTC width is not overflowing
LM requirements.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 64befead366f..2fc1c2729bfd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -711,12 +711,13 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc)
_dpu_crtc_complete_flip(crtc);
}
-static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
+static int _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
struct drm_crtc_state *state)
{
struct dpu_crtc_state *cstate = to_dpu_crtc_state(state);
struct drm_display_mode *adj_mode = &state->adjusted_mode;
u32 crtc_split_width = adj_mode->hdisplay / cstate->num_mixers;
+ struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
int i;
for (i = 0; i < cstate->num_mixers; i++) {
@@ -727,7 +728,12 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
r->y2 = adj_mode->vdisplay;
trace_dpu_crtc_setup_lm_bounds(DRMID(crtc), i, r);
+
+ if (drm_rect_width(r) > dpu_kms->catalog->caps->max_mixer_width)
+ return -E2BIG;
}
+
+ return 0;
}
static void _dpu_crtc_get_pcc_coeff(struct drm_crtc_state *state,
@@ -1195,8 +1201,11 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
if (crtc_state->active_changed)
crtc_state->mode_changed = true;
- if (cstate->num_mixers)
- _dpu_crtc_setup_lm_bounds(crtc, crtc_state);
+ if (cstate->num_mixers) {
+ rc = _dpu_crtc_setup_lm_bounds(crtc, crtc_state);
+ if (rc)
+ return rc;
+ }
/* FIXME: move this to dpu_plane_atomic_check? */
drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) {
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 8/9] drm/msm/dpu: merge MAX_IMG_WIDTH/HEIGHT with DPU_MAX_IMG_WIDTH/HEIGHT
2024-03-19 13:21 [PATCH 0/9] drm/msm/dpu: be more friendly to X.org Dmitry Baryshkov
` (6 preceding siblings ...)
2024-03-19 13:22 ` [PATCH 7/9] drm/msm/dpu: check for overflow in _dpu_crtc_setup_lm_bounds() Dmitry Baryshkov
@ 2024-03-19 13:22 ` Dmitry Baryshkov
2024-04-20 2:54 ` Abhinav Kumar
2024-03-19 13:22 ` [PATCH 9/9] drm/msm/dpu: sync mode_config limits to the FB limits in dpu_plane.c Dmitry Baryshkov
8 siblings, 1 reply; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 13:22 UTC (permalink / raw
To: Rob Clark, Abhinav Kumar, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
dpu_formats.c defines DPU_MAX_IMG_WIDTH and _HEIGHT, while
dpu_hw_catalog.h defines just MAX_IMG_WIDTH and _HEIGHT. Merge these
constants to remove duplication.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 3 ---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 ++--
3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
index 1fc9817278df..1ee78ba640b5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
@@ -20,9 +20,6 @@
#define DPU_TILE_HEIGHT_UBWC 4
#define DPU_TILE_HEIGHT_NV12 8
-#define DPU_MAX_IMG_WIDTH 0x3FFF
-#define DPU_MAX_IMG_HEIGHT 0x3FFF
-
/*
* DPU supported format packing, bpp, and other format
* information.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index d1aef778340b..af2ead1c4886 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -21,8 +21,8 @@
#define DPU_HW_BLK_NAME_LEN 16
-#define MAX_IMG_WIDTH 0x3fff
-#define MAX_IMG_HEIGHT 0x3fff
+#define DPU_MAX_IMG_WIDTH 0x3fff
+#define DPU_MAX_IMG_HEIGHT 0x3fff
#define CRTC_DUAL_MIXERS 2
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 9e57c51f5343..47165d2bd4ea 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -851,8 +851,8 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
fb_rect.y2 = new_plane_state->fb->height;
/* Ensure fb size is supported */
- if (drm_rect_width(&fb_rect) > MAX_IMG_WIDTH ||
- drm_rect_height(&fb_rect) > MAX_IMG_HEIGHT) {
+ if (drm_rect_width(&fb_rect) > DPU_MAX_IMG_WIDTH ||
+ drm_rect_height(&fb_rect) > DPU_MAX_IMG_HEIGHT) {
DPU_DEBUG_PLANE(pdpu, "invalid framebuffer " DRM_RECT_FMT "\n",
DRM_RECT_ARG(&fb_rect));
return -E2BIG;
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 9/9] drm/msm/dpu: sync mode_config limits to the FB limits in dpu_plane.c
2024-03-19 13:21 [PATCH 0/9] drm/msm/dpu: be more friendly to X.org Dmitry Baryshkov
` (7 preceding siblings ...)
2024-03-19 13:22 ` [PATCH 8/9] drm/msm/dpu: merge MAX_IMG_WIDTH/HEIGHT with DPU_MAX_IMG_WIDTH/HEIGHT Dmitry Baryshkov
@ 2024-03-19 13:22 ` Dmitry Baryshkov
2024-04-20 3:05 ` Abhinav Kumar
8 siblings, 1 reply; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-03-19 13:22 UTC (permalink / raw
To: Rob Clark, Abhinav Kumar, Sean Paul, Marijn Suijten, David Airlie,
Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
Lift mode_config limits set by the DPU driver to the actual FB limits as
handled by the dpu_plane.c.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7257ac4020d8..e7dda9eca466 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1136,13 +1136,8 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
dev->mode_config.min_width = 0;
dev->mode_config.min_height = 0;
- /*
- * max crtc width is equal to the max mixer width * 2 and max height is
- * is 4K
- */
- dev->mode_config.max_width =
- dpu_kms->catalog->caps->max_mixer_width * 2;
- dev->mode_config.max_height = 4096;
+ dev->mode_config.max_width = DPU_MAX_IMG_WIDTH;
+ dev->mode_config.max_height = DPU_MAX_IMG_HEIGHT;
dev->max_vblank_count = 0xffffffff;
/* Disable vblank irqs aggressively for power-saving */
--
2.39.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format
2024-03-19 13:21 ` [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format Dmitry Baryshkov
@ 2024-04-19 23:43 ` Abhinav Kumar
2024-04-20 1:26 ` Dmitry Baryshkov
0 siblings, 1 reply; 28+ messages in thread
From: Abhinav Kumar @ 2024-04-19 23:43 UTC (permalink / raw
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
[-- Attachment #1: Type: text/plain, Size: 4775 bytes --]
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 <dmitry.baryshkov@linaro.org>
> ---
> 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?
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,
> - &layout, 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
> - * @msm_fmt: pointer to the msm_fmt base pointer of an dpu_format
> - * @cmd: fb_cmd2 structure user request
> - * @bos: gem buffer object list
> - *
> - * Return: error code on failure, 0 on success
> - */
> -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);
>
> /**
> * dpu_format_populate_layout - populate the given format layout based on
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index a1f5d7c4ab91..7257ac4020d8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -969,7 +969,6 @@ static const struct msm_kms_funcs kms_funcs = {
> .complete_commit = dpu_kms_complete_commit,
> .enable_vblank = dpu_kms_enable_vblank,
> .disable_vblank = dpu_kms_disable_vblank,
> - .check_modified_format = dpu_format_check_modified_format,
> .get_format = dpu_get_msm_format,
> .destroy = dpu_kms_destroy,
> .snapshot = dpu_kms_mdp_snapshot,
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index 0641f6111b93..b794ed918b56 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -96,11 +96,6 @@ struct msm_kms_funcs {
> const struct msm_format *(*get_format)(struct msm_kms *kms,
> const uint32_t format,
> const uint64_t modifiers);
> - /* do format checking on format modified through fb_cmd2 modifiers */
> - int (*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);
>
> /* misc: */
> long (*round_pixclk)(struct msm_kms *kms, unsigned long rate,
>
[-- Attachment #2: modifiers.patch --]
[-- Type: text/plain, Size: 2147 bytes --]
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index 80166f702a0d..1ebf1699ba94 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -175,6 +175,7 @@ static struct drm_framebuffer *msm_framebuffer_init(struct drm_device *dev,
struct drm_framebuffer *fb;
const struct msm_format *format;
int ret, i, n;
+ bool is_modified = false;
drm_dbg_state(dev, "create framebuffer: mode_cmd=%p (%dx%d@%4.4s)\n",
mode_cmd, mode_cmd->width, mode_cmd->height,
@@ -200,28 +201,52 @@ static struct drm_framebuffer *msm_framebuffer_init(struct drm_device *dev,
msm_fb->format = format;
+ if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
+ for (i = 0; i < ARRAY_SIZE(mode_cmd->modifier); i++) {
+ if (mode_cmd->modifier[i]) {
+ is_modified = true;
+ break;
+ }
+ }
+ }
+
if (n > ARRAY_SIZE(fb->obj)) {
ret = -EINVAL;
goto fail;
}
- for (i = 0; i < n; i++) {
- unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
- unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
- unsigned int min_size;
-
- min_size = (height - 1) * mode_cmd->pitches[i]
- + width * info->cpp[i]
- + mode_cmd->offsets[i];
-
- if (bos[i]->size < min_size) {
+ if (is_modified) {
+ if (!kms->funcs->check_modified_format) {
+ DRM_DEV_ERROR(dev->dev, "can't check modified fb format\n");
ret = -EINVAL;
goto fail;
+ } else {
+ ret = kms->funcs->check_modified_format(
+ kms, msm_fb->format, mode_cmd, bos);
+ if (ret)
+ goto fail;
}
+ } else {
+ for (i = 0; i < n; i++) {
+ unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
+ unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
+ unsigned int min_size;
- msm_fb->base.obj[i] = bos[i];
+ min_size = (height - 1) * mode_cmd->pitches[i]
+ + width * info->cpp[i]
+ + mode_cmd->offsets[i];
+
+ if (bos[i]->size < min_size) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ }
}
+ for (i = 0; i < n; i++)
+ msm_fb->base.obj[i] = bos[i];
+
drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
ret = drm_framebuffer_init(dev, fb, &msm_framebuffer_funcs);
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 2/9] drm/msm/dpu: drop dpu_format_populate_layout from dpu_plane_sspp_atomic_update
2024-03-19 13:22 ` [PATCH 2/9] drm/msm/dpu: drop dpu_format_populate_layout from dpu_plane_sspp_atomic_update Dmitry Baryshkov
@ 2024-04-19 23:55 ` Abhinav Kumar
0 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-04-19 23:55 UTC (permalink / raw
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
> The dpu_plane_prepare_fb() already calls dpu_format_populate_layout().
> Store the generated layour in the plane state and drop this call from
> dpu_plane_sspp_update().
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 19 ++++---------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 3 +++
> 2 files changed, 7 insertions(+), 15 deletions(-)
>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/9] drm/msm/dpu: split dpu_format_populate_layout
2024-03-19 13:22 ` [PATCH 3/9] drm/msm/dpu: split dpu_format_populate_layout Dmitry Baryshkov
@ 2024-04-20 0:08 ` Abhinav Kumar
0 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-04-20 0:08 UTC (permalink / raw
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
> Split dpu_format_populate_layout() into addess-related and
> pitch/format-related parts.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 8 +++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 44 +++++++++++-----------
> drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 8 +++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 12 ++++--
> 4 files changed, 45 insertions(+), 27 deletions(-)
>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check
2024-03-19 13:22 ` [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check Dmitry Baryshkov
@ 2024-04-20 0:14 ` Abhinav Kumar
2024-04-20 1:34 ` Dmitry Baryshkov
0 siblings, 1 reply; 28+ messages in thread
From: Abhinav Kumar @ 2024-04-20 0:14 UTC (permalink / raw
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
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 <dmitry.baryshkov@linaro.org>
> ---
> 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, &pstate->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, &pstate->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.
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?
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/9] drm/msm/dpu: check for the plane pitch overflow
2024-03-19 13:22 ` [PATCH 5/9] drm/msm/dpu: check for the plane pitch overflow Dmitry Baryshkov
@ 2024-04-20 0:16 ` Abhinav Kumar
2024-04-20 1:56 ` Dmitry Baryshkov
0 siblings, 1 reply; 28+ messages in thread
From: Abhinav Kumar @ 2024-04-20 0:16 UTC (permalink / raw
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
> Check that the plane pitch doesn't overflow the maximum pitch size
> allowed by the hardware.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 6 +++++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> index b7dc52312c39..86b1defa5d21 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> @@ -12,6 +12,8 @@
>
> struct dpu_hw_sspp;
>
> +#define DPU_SSPP_MAX_PITCH_SIZE 0xffff
> +
You obtained this value from below code right?
if (pipe->multirect_index == DPU_SSPP_RECT_0) {
487 ystride0 = (ystride0 & 0xFFFF0000) |
488 (layout->plane_pitch[0] & 0x0000FFFF);
489 ystride1 = (ystride1 & 0xFFFF0000)|
490 (layout->plane_pitch[2] & 0x0000FFFF);
491 } else {
492 ystride0 = (ystride0 & 0x0000FFFF) |
493 ((layout->plane_pitch[0] << 16) &
494 0xFFFF0000);
495 ystride1 = (ystride1 & 0x0000FFFF) |
496 ((layout->plane_pitch[2] << 16) &
497 0xFFFF0000);
498 }
Seems correct, but was just curious
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format
2024-04-19 23:43 ` Abhinav Kumar
@ 2024-04-20 1:26 ` Dmitry Baryshkov
2024-04-20 2:32 ` Abhinav Kumar
0 siblings, 1 reply; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-04-20 1:26 UTC (permalink / raw
To: Abhinav Kumar
Cc: Rob Clark, Sean Paul, Marijn Suijten, David Airlie, Daniel Vetter,
Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
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 <dmitry.baryshkov@linaro.org>
> > ---
> > 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.
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,
> > - &layout, 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
> > - * @msm_fmt: pointer to the msm_fmt base pointer of an dpu_format
> > - * @cmd: fb_cmd2 structure user request
> > - * @bos: gem buffer object list
> > - *
> > - * Return: error code on failure, 0 on success
> > - */
> > -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);
> > /**
> > * dpu_format_populate_layout - populate the given format layout based on
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index a1f5d7c4ab91..7257ac4020d8 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -969,7 +969,6 @@ static const struct msm_kms_funcs kms_funcs = {
> > .complete_commit = dpu_kms_complete_commit,
> > .enable_vblank = dpu_kms_enable_vblank,
> > .disable_vblank = dpu_kms_disable_vblank,
> > - .check_modified_format = dpu_format_check_modified_format,
> > .get_format = dpu_get_msm_format,
> > .destroy = dpu_kms_destroy,
> > .snapshot = dpu_kms_mdp_snapshot,
> > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> > index 0641f6111b93..b794ed918b56 100644
> > --- a/drivers/gpu/drm/msm/msm_kms.h
> > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > @@ -96,11 +96,6 @@ struct msm_kms_funcs {
> > const struct msm_format *(*get_format)(struct msm_kms *kms,
> > const uint32_t format,
> > const uint64_t modifiers);
> > - /* do format checking on format modified through fb_cmd2 modifiers */
> > - int (*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);
> > /* misc: */
> > long (*round_pixclk)(struct msm_kms *kms, unsigned long rate,
> >
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check
2024-04-20 0:14 ` Abhinav Kumar
@ 2024-04-20 1:34 ` Dmitry Baryshkov
2024-04-20 2:37 ` Abhinav Kumar
0 siblings, 1 reply; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-04-20 1:34 UTC (permalink / raw
To: Abhinav Kumar
Cc: Rob Clark, Sean Paul, Marijn Suijten, David Airlie, Daniel Vetter,
Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
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 <dmitry.baryshkov@linaro.org>
> > ---
> > 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, &pstate->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, &pstate->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.
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/9] drm/msm/dpu: check for the plane pitch overflow
2024-04-20 0:16 ` Abhinav Kumar
@ 2024-04-20 1:56 ` Dmitry Baryshkov
0 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-04-20 1:56 UTC (permalink / raw
To: Abhinav Kumar
Cc: Rob Clark, Sean Paul, Marijn Suijten, David Airlie, Daniel Vetter,
Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
On Fri, Apr 19, 2024 at 05:16:30PM -0700, Abhinav Kumar wrote:
>
>
> On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
> > Check that the plane pitch doesn't overflow the maximum pitch size
> > allowed by the hardware.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 2 ++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 6 +++++-
> > 2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> > index b7dc52312c39..86b1defa5d21 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> > @@ -12,6 +12,8 @@
> > struct dpu_hw_sspp;
> > +#define DPU_SSPP_MAX_PITCH_SIZE 0xffff
> > +
>
> You obtained this value from below code right?
Yes. And also from DPU_MAX_IMG_WIDTH / MAX_IMG_WIDTH.
>
> if (pipe->multirect_index == DPU_SSPP_RECT_0) {
> 487 ystride0 = (ystride0 & 0xFFFF0000) |
> 488 (layout->plane_pitch[0] & 0x0000FFFF);
> 489 ystride1 = (ystride1 & 0xFFFF0000)|
> 490 (layout->plane_pitch[2] & 0x0000FFFF);
> 491 } else {
> 492 ystride0 = (ystride0 & 0x0000FFFF) |
> 493 ((layout->plane_pitch[0] << 16) &
> 494 0xFFFF0000);
> 495 ystride1 = (ystride1 & 0x0000FFFF) |
> 496 ((layout->plane_pitch[2] << 16) &
> 497 0xFFFF0000);
> 498 }
>
> Seems correct, but was just curious
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format
2024-04-20 1:26 ` Dmitry Baryshkov
@ 2024-04-20 2:32 ` Abhinav Kumar
2024-04-22 11:06 ` Dmitry Baryshkov
0 siblings, 1 reply; 28+ messages in thread
From: Abhinav Kumar @ 2024-04-20 2:32 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: Rob Clark, Sean Paul, Marijn Suijten, David Airlie, Daniel Vetter,
Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
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 <dmitry.baryshkov@linaro.org>
>>> ---
>>> 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 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,
>>> - &layout, 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
>>> - * @msm_fmt: pointer to the msm_fmt base pointer of an dpu_format
>>> - * @cmd: fb_cmd2 structure user request
>>> - * @bos: gem buffer object list
>>> - *
>>> - * Return: error code on failure, 0 on success
>>> - */
>>> -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);
>>> /**
>>> * dpu_format_populate_layout - populate the given format layout based on
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index a1f5d7c4ab91..7257ac4020d8 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -969,7 +969,6 @@ static const struct msm_kms_funcs kms_funcs = {
>>> .complete_commit = dpu_kms_complete_commit,
>>> .enable_vblank = dpu_kms_enable_vblank,
>>> .disable_vblank = dpu_kms_disable_vblank,
>>> - .check_modified_format = dpu_format_check_modified_format,
>>> .get_format = dpu_get_msm_format,
>>> .destroy = dpu_kms_destroy,
>>> .snapshot = dpu_kms_mdp_snapshot,
>>> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
>>> index 0641f6111b93..b794ed918b56 100644
>>> --- a/drivers/gpu/drm/msm/msm_kms.h
>>> +++ b/drivers/gpu/drm/msm/msm_kms.h
>>> @@ -96,11 +96,6 @@ struct msm_kms_funcs {
>>> const struct msm_format *(*get_format)(struct msm_kms *kms,
>>> const uint32_t format,
>>> const uint64_t modifiers);
>>> - /* do format checking on format modified through fb_cmd2 modifiers */
>>> - int (*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);
>>> /* misc: */
>>> long (*round_pixclk)(struct msm_kms *kms, unsigned long rate,
>>>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check
2024-04-20 1:34 ` Dmitry Baryshkov
@ 2024-04-20 2:37 ` Abhinav Kumar
2024-04-22 12:22 ` Dmitry Baryshkov
0 siblings, 1 reply; 28+ messages in thread
From: Abhinav Kumar @ 2024-04-20 2:37 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: Rob Clark, Sean Paul, Marijn Suijten, David Airlie, Daniel Vetter,
Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
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 <dmitry.baryshkov@linaro.org>
>>> ---
>>> 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, &pstate->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, &pstate->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.
> 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?
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 8/9] drm/msm/dpu: merge MAX_IMG_WIDTH/HEIGHT with DPU_MAX_IMG_WIDTH/HEIGHT
2024-03-19 13:22 ` [PATCH 8/9] drm/msm/dpu: merge MAX_IMG_WIDTH/HEIGHT with DPU_MAX_IMG_WIDTH/HEIGHT Dmitry Baryshkov
@ 2024-04-20 2:54 ` Abhinav Kumar
0 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-04-20 2:54 UTC (permalink / raw
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
> dpu_formats.c defines DPU_MAX_IMG_WIDTH and _HEIGHT, while
> dpu_hw_catalog.h defines just MAX_IMG_WIDTH and _HEIGHT. Merge these
> constants to remove duplication.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 3 ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 ++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 4 ++--
> 3 files changed, 4 insertions(+), 7 deletions(-)
>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 9/9] drm/msm/dpu: sync mode_config limits to the FB limits in dpu_plane.c
2024-03-19 13:22 ` [PATCH 9/9] drm/msm/dpu: sync mode_config limits to the FB limits in dpu_plane.c Dmitry Baryshkov
@ 2024-04-20 3:05 ` Abhinav Kumar
2024-04-20 3:06 ` Dmitry Baryshkov
0 siblings, 1 reply; 28+ messages in thread
From: Abhinav Kumar @ 2024-04-20 3:05 UTC (permalink / raw
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Marijn Suijten,
David Airlie, Daniel Vetter
Cc: Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
> Lift mode_config limits set by the DPU driver to the actual FB limits as
> handled by the dpu_plane.c.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 7257ac4020d8..e7dda9eca466 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1136,13 +1136,8 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
> dev->mode_config.min_width = 0;
> dev->mode_config.min_height = 0;
>
> - /*
> - * max crtc width is equal to the max mixer width * 2 and max height is
> - * is 4K
> - */
> - dev->mode_config.max_width =
> - dpu_kms->catalog->caps->max_mixer_width * 2;
> - dev->mode_config.max_height = 4096;
> + dev->mode_config.max_width = DPU_MAX_IMG_WIDTH;
> + dev->mode_config.max_height = DPU_MAX_IMG_HEIGHT;
>
Can you please explain a little more about why the previous limits did
not work in the multi-monitor case?
We support at the most using 2 LMs per display today. Quad pipe support
is not there yet. So by bounding to 2 * mixer_width should have been
same as rejecting the mixer width in atomic_check.
> dev->max_vblank_count = 0xffffffff;
> /* Disable vblank irqs aggressively for power-saving */
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 9/9] drm/msm/dpu: sync mode_config limits to the FB limits in dpu_plane.c
2024-04-20 3:05 ` Abhinav Kumar
@ 2024-04-20 3:06 ` Dmitry Baryshkov
2024-04-20 3:58 ` Abhinav Kumar
0 siblings, 1 reply; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-04-20 3:06 UTC (permalink / raw
To: Abhinav Kumar
Cc: Rob Clark, Sean Paul, Marijn Suijten, David Airlie, Daniel Vetter,
Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
On Sat, 20 Apr 2024 at 06:05, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
> > Lift mode_config limits set by the DPU driver to the actual FB limits as
> > handled by the dpu_plane.c.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-------
> > 1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 7257ac4020d8..e7dda9eca466 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -1136,13 +1136,8 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
> > dev->mode_config.min_width = 0;
> > dev->mode_config.min_height = 0;
> >
> > - /*
> > - * max crtc width is equal to the max mixer width * 2 and max height is
> > - * is 4K
> > - */
> > - dev->mode_config.max_width =
> > - dpu_kms->catalog->caps->max_mixer_width * 2;
> > - dev->mode_config.max_height = 4096;
> > + dev->mode_config.max_width = DPU_MAX_IMG_WIDTH;
> > + dev->mode_config.max_height = DPU_MAX_IMG_HEIGHT;
> >
>
> Can you please explain a little more about why the previous limits did
> not work in the multi-monitor case?
>
> We support at the most using 2 LMs per display today. Quad pipe support
> is not there yet. So by bounding to 2 * mixer_width should have been
> same as rejecting the mixer width in atomic_check.
This is the framebuffer limit, not a CRTC size limit.
>
> > dev->max_vblank_count = 0xffffffff;
> > /* Disable vblank irqs aggressively for power-saving */
> >
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 9/9] drm/msm/dpu: sync mode_config limits to the FB limits in dpu_plane.c
2024-04-20 3:06 ` Dmitry Baryshkov
@ 2024-04-20 3:58 ` Abhinav Kumar
0 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-04-20 3:58 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: Rob Clark, Sean Paul, Marijn Suijten, David Airlie, Daniel Vetter,
Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
On 4/19/2024 8:06 PM, Dmitry Baryshkov wrote:
> On Sat, 20 Apr 2024 at 06:05, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 3/19/2024 6:22 AM, Dmitry Baryshkov wrote:
>>> Lift mode_config limits set by the DPU driver to the actual FB limits as
>>> handled by the dpu_plane.c.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 9 ++-------
>>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 7257ac4020d8..e7dda9eca466 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -1136,13 +1136,8 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>>> dev->mode_config.min_width = 0;
>>> dev->mode_config.min_height = 0;
>>>
>>> - /*
>>> - * max crtc width is equal to the max mixer width * 2 and max height is
>>> - * is 4K
>>> - */
>>> - dev->mode_config.max_width =
>>> - dpu_kms->catalog->caps->max_mixer_width * 2;
>>> - dev->mode_config.max_height = 4096;
>>> + dev->mode_config.max_width = DPU_MAX_IMG_WIDTH;
>>> + dev->mode_config.max_height = DPU_MAX_IMG_HEIGHT;
>>>
>>
>> Can you please explain a little more about why the previous limits did
>> not work in the multi-monitor case?
>>
>> We support at the most using 2 LMs per display today. Quad pipe support
>> is not there yet. So by bounding to 2 * mixer_width should have been
>> same as rejecting the mixer width in atomic_check.
>
> This is the framebuffer limit, not a CRTC size limit.
>
As discussed on IRC, the DRM fwk uses this to limit the modes on the
connector, please check
2922 if (out_resp->count_modes == 0) {
2923 if (is_current_master)
2924 connector->funcs->fill_modes(connector,
2925 dev->mode_config.max_width,
2926 dev->mode_config.max_height);
2927 else
2928 drm_dbg_kms(dev, "User-space requested a forced probe on
[CONNECTOR:%d:%s] but is not the DRM master, demoting to read-only probe",
2929 connector->base.id, connector->name);
2930 }
So the documentation of this doesnt really align with the usage.
Unless we alter these pieces, I am hesitant to ack this.
>>
>>> dev->max_vblank_count = 0xffffffff;
>>> /* Disable vblank irqs aggressively for power-saving */
>>>
>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format
2024-04-20 2:32 ` Abhinav Kumar
@ 2024-04-22 11:06 ` Dmitry Baryshkov
2024-06-03 19:00 ` Abhinav Kumar
0 siblings, 1 reply; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-04-22 11:06 UTC (permalink / raw
To: Abhinav Kumar
Cc: Rob Clark, Sean Paul, Marijn Suijten, David Airlie, Daniel Vetter,
Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
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 <dmitry.baryshkov@linaro.org>
> > > > ---
> > > > 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,
> > > > - &layout, 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
> > > > - * @msm_fmt: pointer to the msm_fmt base pointer of an dpu_format
> > > > - * @cmd: fb_cmd2 structure user request
> > > > - * @bos: gem buffer object list
> > > > - *
> > > > - * Return: error code on failure, 0 on success
> > > > - */
> > > > -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);
> > > > /**
> > > > * dpu_format_populate_layout - populate the given format layout based on
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > > index a1f5d7c4ab91..7257ac4020d8 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > > @@ -969,7 +969,6 @@ static const struct msm_kms_funcs kms_funcs = {
> > > > .complete_commit = dpu_kms_complete_commit,
> > > > .enable_vblank = dpu_kms_enable_vblank,
> > > > .disable_vblank = dpu_kms_disable_vblank,
> > > > - .check_modified_format = dpu_format_check_modified_format,
> > > > .get_format = dpu_get_msm_format,
> > > > .destroy = dpu_kms_destroy,
> > > > .snapshot = dpu_kms_mdp_snapshot,
> > > > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> > > > index 0641f6111b93..b794ed918b56 100644
> > > > --- a/drivers/gpu/drm/msm/msm_kms.h
> > > > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > > > @@ -96,11 +96,6 @@ struct msm_kms_funcs {
> > > > const struct msm_format *(*get_format)(struct msm_kms *kms,
> > > > const uint32_t format,
> > > > const uint64_t modifiers);
> > > > - /* do format checking on format modified through fb_cmd2 modifiers */
> > > > - int (*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);
> > > > /* misc: */
> > > > long (*round_pixclk)(struct msm_kms *kms, unsigned long rate,
> > > >
> >
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check
2024-04-20 2:37 ` Abhinav Kumar
@ 2024-04-22 12:22 ` Dmitry Baryshkov
2024-06-03 19:20 ` Abhinav Kumar
0 siblings, 1 reply; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-04-22 12:22 UTC (permalink / raw
To: Abhinav Kumar
Cc: Rob Clark, Sean Paul, Marijn Suijten, David Airlie, Daniel Vetter,
Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
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 <dmitry.baryshkov@linaro.org>
> > > > ---
> > > > 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, &pstate->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, &pstate->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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format
2024-04-22 11:06 ` Dmitry Baryshkov
@ 2024-06-03 19:00 ` Abhinav Kumar
0 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-06-03 19:00 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: Rob Clark, Sean Paul, Marijn Suijten, David Airlie, Daniel Vetter,
Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
On 4/22/2024 4:06 AM, Dmitry Baryshkov wrote:
> 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 <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>> 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.
>
Alright, lets revisit this implementation later on and for this series
we can go with dropping dpu_format_check_modified_format().
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check
2024-04-22 12:22 ` Dmitry Baryshkov
@ 2024-06-03 19:20 ` Abhinav Kumar
0 siblings, 0 replies; 28+ messages in thread
From: Abhinav Kumar @ 2024-06-03 19:20 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: Rob Clark, Sean Paul, Marijn Suijten, David Airlie, Daniel Vetter,
Abel Vesa, Johan Hovold, linux-arm-msm, dri-devel, freedreno
On 4/22/2024 5:22 AM, Dmitry Baryshkov wrote:
> 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 <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>> 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, &pstate->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, &pstate->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?
>
As we discussed during the meeting, discarding rejected state is fine
and is a commonly used practice, hence this is okay. I was only trying
to avoid that.
>>
>>> 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?
>>>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2024-06-03 19:20 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-19 13:21 [PATCH 0/9] drm/msm/dpu: be more friendly to X.org Dmitry Baryshkov
2024-03-19 13:21 ` [PATCH 1/9] drm/msm/dpu: drop dpu_format_check_modified_format Dmitry Baryshkov
2024-04-19 23:43 ` Abhinav Kumar
2024-04-20 1:26 ` Dmitry Baryshkov
2024-04-20 2:32 ` Abhinav Kumar
2024-04-22 11:06 ` Dmitry Baryshkov
2024-06-03 19:00 ` Abhinav Kumar
2024-03-19 13:22 ` [PATCH 2/9] drm/msm/dpu: drop dpu_format_populate_layout from dpu_plane_sspp_atomic_update Dmitry Baryshkov
2024-04-19 23:55 ` Abhinav Kumar
2024-03-19 13:22 ` [PATCH 3/9] drm/msm/dpu: split dpu_format_populate_layout Dmitry Baryshkov
2024-04-20 0:08 ` Abhinav Kumar
2024-03-19 13:22 ` [PATCH 4/9] drm/msm/dpu: move dpu_format_populate_plane_sizes to atomic_check Dmitry Baryshkov
2024-04-20 0:14 ` Abhinav Kumar
2024-04-20 1:34 ` Dmitry Baryshkov
2024-04-20 2:37 ` Abhinav Kumar
2024-04-22 12:22 ` Dmitry Baryshkov
2024-06-03 19:20 ` Abhinav Kumar
2024-03-19 13:22 ` [PATCH 5/9] drm/msm/dpu: check for the plane pitch overflow Dmitry Baryshkov
2024-04-20 0:16 ` Abhinav Kumar
2024-04-20 1:56 ` Dmitry Baryshkov
2024-03-19 13:22 ` [PATCH 6/9] drm/msm/dpu: drop call to _dpu_crtc_setup_lm_bounds from atomic_begin Dmitry Baryshkov
2024-03-19 13:22 ` [PATCH 7/9] drm/msm/dpu: check for overflow in _dpu_crtc_setup_lm_bounds() Dmitry Baryshkov
2024-03-19 13:22 ` [PATCH 8/9] drm/msm/dpu: merge MAX_IMG_WIDTH/HEIGHT with DPU_MAX_IMG_WIDTH/HEIGHT Dmitry Baryshkov
2024-04-20 2:54 ` Abhinav Kumar
2024-03-19 13:22 ` [PATCH 9/9] drm/msm/dpu: sync mode_config limits to the FB limits in dpu_plane.c Dmitry Baryshkov
2024-04-20 3:05 ` Abhinav Kumar
2024-04-20 3:06 ` Dmitry Baryshkov
2024-04-20 3:58 ` Abhinav Kumar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.