Re: [Freedreno] [PATCH v3 01/13] drm/msm/dsi: add support for dsc data
Hi Vinod
On 11/15/2021 10:22 PM, Vinod Koul wrote:
Display Stream Compression (DSC) parameters need to be calculated. Add
helpers and struct msm_display_dsc_config in msm_drv for this
msm_display_dsc_config uses drm_dsc_config for DSC parameters.
Signed-off-by: Vinod Koul
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 132 +
drivers/gpu/drm/msm/msm_drv.h | 20 +
2 files changed, 152 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index f69a125f9559..30c1e299aa52 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -31,6 +31,8 @@
#define DSI_RESET_TOGGLE_DELAY_MS 20
+static int dsi_populate_dsc_params(struct msm_display_dsc_config *dsc);
+
static int dsi_get_version(const void __iomem *base, u32 *major, u32 *minor)
{
u32 ver;
@@ -157,6 +159,7 @@ struct msm_dsi_host {
struct regmap *sfpb;
struct drm_display_mode *mode;
+ struct msm_display_dsc_config *dsc;
/* connected device info */
struct device_node *device_node;
@@ -1710,6 +1713,135 @@ static int dsi_host_parse_lane_data(struct msm_dsi_host
*msm_host,
return -EINVAL;
}
+static u32 dsi_dsc_rc_buf_thresh[DSC_NUM_BUF_RANGES - 1] = {
+ 0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54, 0x62,
+ 0x69, 0x70, 0x77, 0x79, 0x7b, 0x7d, 0x7e
+};
+
+/* only 8bpc, 8bpp added */
+static char min_qp[DSC_NUM_BUF_RANGES] = {
+ 0, 0, 1, 1, 3, 3, 3, 3, 3, 3, 5, 5, 5, 7, 13
+};
+
+static char max_qp[DSC_NUM_BUF_RANGES] = {
+ 4, 4, 5, 6, 7, 7, 7, 8, 9, 10, 11, 12, 13, 13, 15
+};
+
+static char bpg_offset[DSC_NUM_BUF_RANGES] = {
+ 2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -12, -12, -12, -12
+};
+
+static int dsi_populate_dsc_params(struct msm_display_dsc_config *dsc)
+{
+ int mux_words_size;
+ int groups_per_line, groups_total;
+ int min_rate_buffer_size;
+ int hrd_delay;
+ int pre_num_extra_mux_bits, num_extra_mux_bits;
+ int slice_bits;
+ int target_bpp_x16;
+ int data;
+ int final_value, final_scale;
+ int i;
+
+ dsc->drm->rc_model_size = 8192;
+ dsc->drm->first_line_bpg_offset = 12;
+ dsc->drm->rc_edge_factor = 6;
+ dsc->drm->rc_tgt_offset_high = 3;
+ dsc->drm->rc_tgt_offset_low = 3;
+ dsc->drm->simple_422 = 0;
+ dsc->drm->convert_rgb = 1;
+ dsc->drm->vbr_enable = 0;
+
+ /* handle only bpp = bpc = 8 */
+ for (i = 0; i < DSC_NUM_BUF_RANGES - 1 ; i++)
+ dsc->drm->rc_buf_thresh[i] = dsi_dsc_rc_buf_thresh[i];
+
+ for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
+ dsc->drm->rc_range_params[i].range_min_qp = min_qp[i];
+ dsc->drm->rc_range_params[i].range_max_qp = max_qp[i];
+ dsc->drm->rc_range_params[i].range_bpg_offset = bpg_offset[i];
+ }
+
+ dsc->drm->initial_offset = 6144; /* Not bpp 12 */
+ if (dsc->drm->bits_per_pixel != 8)
+ dsc->drm->initial_offset = 2048; /* bpp = 12 */
+
+ mux_words_size = 48;/* bpc == 8/10 */
+ if (dsc->drm->bits_per_component == 12)
+ mux_words_size = 64;
+
+ dsc->drm->initial_xmit_delay = 512;
+ dsc->drm->initial_scale_value = 32;
+ dsc->drm->first_line_bpg_offset = 12;
+ dsc->drm->line_buf_depth = dsc->drm->bits_per_component + 1;
+
+ /* bpc 8 */
+ dsc->drm->flatness_min_qp = 3;
+ dsc->drm->flatness_max_qp = 12;
+ dsc->det_thresh_flatness = 7 + 2 * (dsc->drm->bits_per_component - 8);
+ dsc->drm->rc_quant_incr_limit0 = 11;
+ dsc->drm->rc_quant_incr_limit1 = 11;
+ dsc->drm->mux_word_size = DSC_MUX_WORD_SIZE_8_10_BPC;
+
+ /* FIXME: need to call drm_dsc_compute_rc_parameters() so that rest of
+* params are calculated
+*/
since its been a while on this, before moving ahead with a FIXME
comment, I wanted to know if you had a chance to check what is the
discrepancy between this and drm_dsc_compute_rc_parameters().
The LOC saved can be quite a bit if we move to
drm_dsc_compute_rc_parameters(). Last time we synced, I think only one
parameter was mismatching. The code-churn to avoid one mismatch seems a
lot. If there are more conflicting parameters than one or two, we can go
ahead with this custom calculation with your FIXME.
+ dsc->slice_last_group_size = 3 - (dsc->drm->slice_width % 3);
+ groups_per_line = DIV_ROUND_UP(dsc->drm->slice_width, 3);
+ dsc->drm->slice_chunk_size = dsc->drm->slice_width *
dsc->drm->bits_per_pixel / 8;
+ if ((dsc->drm->slice_width * dsc->drm->bits_per_pixel) % 8)
+ dsc->drm->slice_chunk_size++;
+
+ /* rbs-min */
+ min_rate_buffer_size = dsc->drm->rc_model_size -
dsc->drm->initial_offset +
+ dsc->drm->initial_xmit_delay *
dsc->drm->bits_per_pixel +
+
[Freedreno] [PATCH igt 1/2] igt: Split out I/O helpers
From: Rob Clark
Split the readN()/writeN() helpers out into an igt_io module, so they
can be re-used by tests.
Signed-off-by: Rob Clark
---
lib/igt_io.c| 96 +
lib/igt_io.h| 33 +
lib/igt_sysfs.c | 45 +++
lib/meson.build | 1 +
4 files changed, 135 insertions(+), 40 deletions(-)
create mode 100644 lib/igt_io.c
create mode 100644 lib/igt_io.h
diff --git a/lib/igt_io.c b/lib/igt_io.c
new file mode 100644
index ..ad54cbe5
--- /dev/null
+++ b/lib/igt_io.c
@@ -0,0 +1,96 @@
+/*
+ * Copyright © 2016 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include
+#include
+#include
+
+#include "igt_io.h"
+
+/**
+ * SECTION:igt_io
+ * @short_description: Helpers for file I/O
+ * @title: io
+ * @include: igt_io.h
+ *
+ * This library provides helpers for file I/O
+ */
+
+/**
+ * igt_readn:
+ * @fd: the file descriptor
+ * @buf: buffer where the contents will be stored, allocated by the caller
+ * @size: size of the buffer
+ *
+ * Read from fd into provided buffer until the buffer is full or EOF
+ *
+ * Returns:
+ * -errorno on failure or bytes read on success
+ */
+ssize_t igt_readn(int fd, char *buf, size_t len)
+{
+ ssize_t ret;
+ size_t total = 0;
+
+ do {
+ ret = read(fd, buf + total, len - total);
+ if (ret < 0)
+ ret = -errno;
+ if (ret == -EINTR || ret == -EAGAIN)
+ continue;
+ if (ret <= 0)
+ break;
+ total += ret;
+ } while (total != len);
+ return total ?: ret;
+}
+
+/**
+ * igt_writen:
+ * @fd: the file descriptor
+ * @buf: the block with the contents to write
+ * @len: the length to write
+ *
+ * This writes @len bytes from @data to the sysfs file.
+ *
+ * Returns:
+ * -errorno on failure or bytes read on success
+ */
+ssize_t igt_writen(int fd, const char *buf, size_t len)
+{
+ ssize_t ret;
+ size_t total = 0;
+
+ do {
+ ret = write(fd, buf + total, len - total);
+ if (ret < 0)
+ ret = -errno;
+ if (ret == -EINTR || ret == -EAGAIN)
+ continue;
+ if (ret <= 0)
+ break;
+ total += ret;
+ } while (total != len);
+ return total ?: ret;
+}
diff --git a/lib/igt_io.h b/lib/igt_io.h
new file mode 100644
index ..eb9ffee1
--- /dev/null
+++ b/lib/igt_io.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright © 2016 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#ifndef __IGT_IO_H__
+#define __IGT_IO_H__
+
+#include
+
+ssize_t igt_readn(int fd, char *buf, size_t len);
+ssize_t igt_writen(int fd, const char *buf,
[Freedreno] [PATCH igt 2/2] msm: Add test for kernel buffer permissions
From: Rob Clark
Tests that reads and/or writes to kernel managed buffers which should be
inaccessible to userspace controlled cmdstream, are indeed inaccessible.
Signed-off-by: Rob Clark
---
lib/igt_msm.h | 1 +
tests/meson.build | 1 +
tests/msm_mapping.c | 257
3 files changed, 259 insertions(+)
create mode 100644 tests/msm_mapping.c
diff --git a/lib/igt_msm.h b/lib/igt_msm.h
index 421d23ed..6008020b 100644
--- a/lib/igt_msm.h
+++ b/lib/igt_msm.h
@@ -100,6 +100,7 @@ enum adreno_pm4_type3_packets {
CP_WAIT_MEM_GTE = 20,
CP_WAIT_REG_MEM = 60,
CP_MEM_WRITE = 61,
+ CP_MEM_TO_MEM = 115,
};
static inline unsigned
diff --git a/tests/meson.build b/tests/meson.build
index 7b7d6bf8..c14acf99 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -60,6 +60,7 @@ test_progs = [
'kms_vrr',
'kms_writeback',
'meta_test',
+ 'msm_mapping',
'msm_recovery',
'msm_submit',
'panfrost_get_param',
diff --git a/tests/msm_mapping.c b/tests/msm_mapping.c
new file mode 100644
index ..e1474f9f
--- /dev/null
+++ b/tests/msm_mapping.c
@@ -0,0 +1,257 @@
+/*
+ * Copyright © 2021 Google, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "igt.h"
+#include "igt_msm.h"
+#include "igt_io.h"
+
+/*
+ * Tests to ensure various kernel controlled buffers are mapped with the
+ * appropriate permissions (either read-only or not-accessible to userspace
+ * controlled cmdstream)
+ */
+
+/*
+ * Helper to get and clear devcore dumps
+ */
+
+static char *
+get_and_clear_devcore(void)
+{
+ glob_t glob_buf = {0};
+ char *buf = NULL;
+ int ret, fd;
+
+ ret = glob("/sys/class/devcoredump/devcd*/data", GLOB_NOSORT, NULL,
_buf);
+ if ((ret == GLOB_NOMATCH) || !glob_buf.gl_pathc)
+ return NULL;
+
+ fd = open(glob_buf.gl_pathv[0], O_RDWR);
+
+ if (fd >= 0) {
+ /* We don't need to read the entire devcore, the first bit is
+* sufficient for our purposes:
+*/
+ buf = calloc(1, 0x1000);
+ igt_readn(fd, buf, 0x1000);
+
+ /* Clear the devcore: */
+ igt_writen(fd, "1", 1);
+ }
+
+ globfree(_buf);
+
+ return buf;
+}
+
+/*
+ * Helper to find named buffer address
+ */
+
+static const char *
+get_line(char **buf)
+{
+ char *ret, *eol;
+
+ ret = *buf;
+ eol = strstr(*buf, "\n");
+
+ if (!eol) {
+ /* could be last line in file: */
+ *buf = NULL;
+ return ret;
+ }
+
+ *eol = '\0';
+ *buf += 1 + strlen(ret);
+
+ return ret;
+}
+
+static bool
+endswith(const char *str, const char *end)
+{
+ char *p = strstr(str, end);
+
+ /* Trim trailing whitespace: */
+ if (p) {
+ char *c = p;
+ while (c) {
+ if (isspace(*c)) {
+ *c = '\0';
+ break;
+ }
+ c++;
+ }
+ }
+
+ return p && (strlen(p) == strlen(end));
+}
+
+static uint64_t
+get_bo_addr(int drm_fd, const char *name)
+{
+ char buf[0x4000];
+ char *p = buf;
+
+ igt_debugfs_read(drm_fd, "gem", buf);
+
+ /* NOTE: the contents of the debugfs file look like:
+*
+*flags id ref offset kaddrsize madv
name
+*0004: I 0 ( 1) ffc0104b9000 4096
memptrs
+* vmas: [gpu: aspace=ff808bf03e00,
1,mapped,inuse=1]
+*00020002: I 0 ( 1) ffc012001000 00032768
ring0
+* vmas: [gpu:
[Freedreno] [PATCH igt 0/2] msm+lib: Add test for buffer mapping permissions
From: Rob Clark First patch just splits out a couple of helpers from igt_sysfs so they can be re-used. Second patch adds a test which locates the address of a given buffer, and verifies (depending on expected permissions) that writes and/or reads trigger an iova fault rather than succeeding. Rob Clark (2): igt: Split out I/O helpers msm: Add test for kernel buffer permissions lib/igt_io.c| 96 + lib/igt_io.h| 33 ++ lib/igt_msm.h | 1 + lib/igt_sysfs.c | 45 +--- lib/meson.build | 1 + tests/meson.build | 1 + tests/msm_mapping.c | 257 7 files changed, 394 insertions(+), 40 deletions(-) create mode 100644 lib/igt_io.c create mode 100644 lib/igt_io.h create mode 100644 tests/msm_mapping.c -- 2.33.1
Re: [Freedreno] [PATCH v4.5 12/14] dt-bindings: msm/dp: Add bindings for HDCP registers
On Mon, 15 Nov 2021 20:21:48 +, Sean Paul wrote: > From: Sean Paul > > This patch adds the bindings for the MSM DisplayPort HDCP registers > which are required to write the HDCP key into the display controller as > well as the registers to enable HDCP authentication/key > exchange/encryption. > > We'll use a new compatible string for this since the fields are optional. > > Cc: Rob Herring > Cc: Stephen Boyd > Signed-off-by: Sean Paul > Link: > https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-13-s...@poorly.run > #v1 > Link: > https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-13-s...@poorly.run > #v2 > Link: > https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-13-s...@poorly.run > #v3 > Link: > https://patchwork.freedesktop.org/patch/msgid/20211105030434.2828845-13-s...@poorly.run > #v4 > > Changes in v2: > -Drop register range names (Stephen) > -Fix yaml errors (Rob) > Changes in v3: > -Add new compatible string for dp-hdcp > -Add descriptions to reg > -Add minItems/maxItems to reg > -Make reg depend on the new hdcp compatible string > Changes in v4: > -Rebase on Bjorn's multi-dp patchset > Changes in v4.5: > -Remove maxItems from reg (Rob) > -Remove leading zeros in example (Rob) > --- > .../devicetree/bindings/display/msm/dp-controller.yaml | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > Reviewed-by: Rob Herring
Re: [Freedreno] [PATCH] drm/msm/gpu: Don't allow zero fence_id
On Mon, Nov 29, 2021 at 10:18 AM Rob Clark wrote:
>
> From: Rob Clark
>
> Elsewhere we treat zero as "no fence" and __msm_gem_submit_destroy()
> skips removal from fence_idr. We could alternately change this to use
> negative values for "no fence" but I think it is more clear to not allow
> zero as a valid fence_id.
>
probably should have added:
Fixes: a61acbbe9cf8 ("drm/msm: Track "seqno" fences by idr")
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 282628d6b72c..6cfa984dee6a 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -881,7 +881,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void
> *data,
> * to the underlying fence.
> */
> submit->fence_id = idr_alloc_cyclic(>fence_idr,
> - submit->user_fence, 0, INT_MAX, GFP_KERNEL);
> + submit->user_fence, 1, INT_MAX, GFP_KERNEL);
> if (submit->fence_id < 0) {
> ret = submit->fence_id = 0;
> submit->fence_id = 0;
> --
> 2.33.1
>
[Freedreno] [PATCH] drm/msm/gpu: Don't allow zero fence_id
From: Rob Clark
Elsewhere we treat zero as "no fence" and __msm_gem_submit_destroy()
skips removal from fence_idr. We could alternately change this to use
negative values for "no fence" but I think it is more clear to not allow
zero as a valid fence_id.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 282628d6b72c..6cfa984dee6a 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -881,7 +881,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
* to the underlying fence.
*/
submit->fence_id = idr_alloc_cyclic(>fence_idr,
- submit->user_fence, 0, INT_MAX, GFP_KERNEL);
+ submit->user_fence, 1, INT_MAX, GFP_KERNEL);
if (submit->fence_id < 0) {
ret = submit->fence_id = 0;
submit->fence_id = 0;
--
2.33.1
[Freedreno] [PATCH v5] drm/msm/dp: employ bridge mechanism for display enable and disable
Currently the msm_dp_*** functions implement the same sequence which would
happen when drm_bridge is used. hence get rid of this intermediate layer
and align with the drm_bridge usage to avoid customized implementation.
Signed-off-by: Kuogee Hsieh
Changes in v2:
-- revise commit text
-- rename dp_bridge to msm_dp_bridge
-- delete empty functions
Changes in v3:
-- replace kzalloc() with devm_kzalloc()
-- replace __dp_display_enable() with dp_display_enable()
-- replace __dp_display_disable() with dp_display_disable()
Changes in v4:
-- msm_dp_bridge_init() called from msm_dp_modeset_init() same as dsi
Changes in v5:
-- delete attach, mode_fixup and pre_enable from dp_bridge_ops
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 21
drivers/gpu/drm/msm/dp/dp_display.c | 16 +-
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 77 +
drivers/gpu/drm/msm/msm_drv.h | 12 +++--
5 files changed, 100 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 31050aa..c4e08c4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1003,9 +1003,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder
*drm_enc,
trace_dpu_enc_mode_set(DRMID(drm_enc));
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)
- msm_dp_display_mode_set(dpu_enc->dp, drm_enc, mode, adj_mode);
-
list_for_each_entry(conn_iter, connector_list, head)
if (conn_iter->encoder == drm_enc)
conn = conn_iter;
@@ -1181,14 +1178,6 @@ static void dpu_encoder_virt_enable(struct drm_encoder
*drm_enc)
_dpu_encoder_virt_enable_helper(drm_enc);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
- ret = msm_dp_display_enable(dpu_enc->dp, drm_enc);
- if (ret) {
- DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
- ret);
- goto out;
- }
- }
dpu_enc->enabled = true;
out:
@@ -1214,11 +1203,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
/* wait for idle */
dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
- if (msm_dp_display_pre_disable(dpu_enc->dp, drm_enc))
- DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
- }
-
dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_PRE_STOP);
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
@@ -1243,11 +1227,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
- if (msm_dp_display_disable(dpu_enc->dp, drm_enc))
- DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
- }
-
mutex_unlock(_enc->enc_lock);
}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 2f113ff..89a8d43 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1571,6 +1571,18 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
struct drm_device *dev,
}
priv->connectors[priv->num_connectors++] = dp_display->connector;
+
+ dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder);
+ if (IS_ERR(dp_display->bridge)) {
+ ret = PTR_ERR(dp_display->bridge);
+ DRM_DEV_ERROR(dev->dev,
+ "failed to create dp bridge: %d\n", ret);
+ dp_display->bridge = NULL;
+ return ret;
+ }
+
+ priv->bridges[priv->num_bridges++] = dp_display->bridge;
+
return 0;
}
@@ -1674,8 +1686,8 @@ int msm_dp_display_disable(struct msm_dp *dp, struct
drm_encoder *encoder)
}
void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
+ const struct drm_display_mode *mode,
+ const struct drm_display_mode *adjusted_mode)
{
struct dp_display_private *dp_display;
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
b/drivers/gpu/drm/msm/dp/dp_display.h
index 76f45f9..2237e80 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -13,6 +13,7 @@
struct msm_dp {
struct drm_device *drm_dev;
struct device *codec_dev;
+ struct drm_bridge *bridge;
struct drm_connector *connector;
struct drm_encoder *encoder;
struct drm_panel
Re: [Freedreno] [PATCH v3] drm/msm/dp: employ bridge mechanism for display enable and disable
On 11/24/2021 11:45 AM, Dmitry Baryshkov wrote:
On 15/11/2021 21:48, Kuogee Hsieh wrote:
Currently the msm_dp_*** functions implement the same sequence which
would
happen when drm_bridge is used. hence get rid of this intermediate layer
and align with the drm_bridge usage to avoid customized implementation.
Signed-off-by: Kuogee Hsieh
Changes in v2:
-- revise commit text
-- rename dp_bridge to msm_dp_bridge
-- delete empty functions
Changes in 3:
-- replace kzalloc() with devm_kzalloc()
-- replace __dp_display_enable() with dp_display_enable()
-- replace __dp_display_disable() with dp_display_disable()
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 21 ---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 7 +++
drivers/gpu/drm/msm/dp/dp_display.c | 4 +-
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 91
+
drivers/gpu/drm/msm/msm_drv.h | 16 +++--
6 files changed, 113 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 31050aa..c4e08c4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1003,9 +1003,6 @@ static void dpu_encoder_virt_mode_set(struct
drm_encoder *drm_enc,
trace_dpu_enc_mode_set(DRMID(drm_enc));
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)
- msm_dp_display_mode_set(dpu_enc->dp, drm_enc, mode, adj_mode);
-
list_for_each_entry(conn_iter, connector_list, head)
if (conn_iter->encoder == drm_enc)
conn = conn_iter;
@@ -1181,14 +1178,6 @@ static void dpu_encoder_virt_enable(struct
drm_encoder *drm_enc)
_dpu_encoder_virt_enable_helper(drm_enc);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
- ret = msm_dp_display_enable(dpu_enc->dp, drm_enc);
- if (ret) {
- DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
- ret);
- goto out;
- }
- }
dpu_enc->enabled = true;
out:
@@ -1214,11 +1203,6 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
/* wait for idle */
dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
- if (msm_dp_display_pre_disable(dpu_enc->dp, drm_enc))
- DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
- }
-
dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_PRE_STOP);
for (i = 0; i < dpu_enc->num_phys_encs; i++) {
@@ -1243,11 +1227,6 @@ static void dpu_encoder_virt_disable(struct
drm_encoder *drm_enc)
DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
- if (msm_dp_display_disable(dpu_enc->dp, drm_enc))
- DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
- }
-
mutex_unlock(_enc->enc_lock);
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 27d98b5..d16337f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -557,6 +557,13 @@ static int
_dpu_kms_initialize_displayport(struct drm_device *dev,
encoder->base.id, rc);
return rc;
}
+
+ rc = msm_dp_bridge_init(priv->dp[i], dev, encoder);
+ if (rc) {
+ DPU_ERROR("failed to setup DPU bridge %d: rc:%d\n",
+ encoder->base.id, rc);
+ return rc;
+ }
There is no need to teach DPU driver about all the gory details of DP
internals. Move this call to the msm_dp_modeset_init().
This has been done at v4.
I will submit v5 to address other commands.
}
return rc;
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 2f113ff..51770a4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1674,8 +1674,8 @@ int msm_dp_display_disable(struct msm_dp *dp,
struct drm_encoder *encoder)
}
void msm_dp_display_mode_set(struct msm_dp *dp, struct
drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
+ const struct drm_display_mode *mode,
+ const struct drm_display_mode *adjusted_mode)
{
struct dp_display_private *dp_display;
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
b/drivers/gpu/drm/msm/dp/dp_display.h
index 76f45f9..2237e80 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -13,6 +13,7 @@
struct msm_dp {
struct drm_device *drm_dev;
struct device *codec_dev;
+ struct drm_bridge *bridge;
struct drm_connector *connector;
struct drm_encoder *encoder;
struct drm_panel *drm_panel;
diff --git
Re: [Freedreno] [PATCH] drm/msm: Initialize MDSS irq domain at probe time
Il 29/11/21 15:53, Dmitry Baryshkov ha scritto:
Hi,
On Mon, 29 Nov 2021 at 17:15, AngeloGioacchino Del Regno
wrote:
Il 29/11/21 03:20, Dmitry Baryshkov ha scritto:
Hi,
On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
DSI host gets initialized earlier, but this caused unability to probe
the entire stack of components because they all depend on interrupts
coming from the main `mdss` node (mdp5, or dpu1).
To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
them at msm_pdev_probe() time: this will make sure that we add the
required interrupt controller mapping before dsi and/or other components
try to initialize, finally satisfying the dependency.
While at it, also change the allocation of msm_drm_private to use the
devm variant of kzalloc().
Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
Signed-off-by: AngeloGioacchino Del Regno
I have been thinking about this. I do not feel that this is the correct
approach.
Currently DRM device exists only when all components are bound. If any of the
subdevices is removed, corresponding component is delteted (and thus all
components
are unbound), the DRM device is taken down. This results in the state cleanup,
userspace notifications, etc.
With your changes, DRM device will continue to exist even after one of
subdevices
is removed. This is not an expected behaviour, since subdrivers do not perform
full
cleanup, delegating that to DRM device takedown.
I suppose that proper solution would be to split msm_drv.c into into:
- generic components & drm code to be called from mdp4/mdp5/dpu driver (making
mdp4, mdp5 or dpu1 the components master)
- bare mdss driver, taking care only about IRQs, OF devices population - calling
proper mdss_init/mdss_destroy functions. Most probably we can drop this part
altogether and just make md5_mdss.c/dpu_mdss.c proper platform drivers.
Hmm... getting a better look on how things are structured... yes, I mostly agree
with you, though I'm not sure about making MDP{4,5}/DPU1 the component master;
that
would result in a major change in drm-msm, which may be "a bit too much".
Don't misunderstand me here, please, major changes are fine - but I feel urgency
to get this bug solved ASAP (since drm-msm is currently broken at least for drm
bridges) and, if we do anything major, that would require a very careful slow
review process that will leave this driver broken for a lot of time.
Yep. I wanted to hear your and Rob's opinion, that's why I did not
rush into implementing it.
I still think this is the way to go in the long term. What I really
liked in your patchset was untying the knot between component binds
returning EPROBE_DEFER and mdss subdevices being in place. This solved
the "dsi host registration" infinite loop for me.
Thanks. I'm also curious about Rob's opinion on this, as that'd be very
valuable.
I actually tried something else that "simplifies" the former approach, so here's
my proposal:
* we introduce {mdp5,dpu}_mdss_early_init(struct device, struct msm_drm_private)
* allocate only msm_drm_private in msm_pdev_probe, leaving the drm_dev_alloc
call
into msm_drm_init(), so that the drm_dev_put() stays in msm_drm_uninit()
* pass msm_drm_private as drvdata instead of drm_device
* change all the drvdata users to get drm_device from priv->dev, instead of
getting
msm_drm_private from drm_device->dev_private (like many other drm drivers
are
currently doing)
This sounds in a way that it should work. I'm looking forward to
seeing (and testing) your patches.
Alright then, I'm running some more tests and cleaning up the patches.
Expect a v2 between today and tomorrow at max! :))
This way, we keep the current flow of creating the DRM device at msm_drm_init
time
and tearing it down at msm_drm_unbind time, solving the issue that you are
describing.
If you're okay with this kind of approach, I have two patches here that are 95%
ready, can finish them off and send briefly.
Though, something else must be noted here... in the last mail where I'm pasting
a crash that happens when running 'rmmod panel_edp ti_sn65dsi86', I have implied
that this is happening due to the patch that I've sent: after some more
research,
I'm not convinced anymore that this is a consequence of that. That crash may not
be related to my fix at all, but to something else (perhaps also related to
commit
8f59ee9a570c, the one that we're fixing here).
Of course, that crash still happens even with the approach that I've just
proposed.
Looking forward for your opinion!
Cheers,
- Angelo
--
AngeloGioacchino Del Regno
Software Engineer
Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718
Re: [Freedreno] [PATCH] drm/msm: Initialize MDSS irq domain at probe time
Hi,
On Mon, 29 Nov 2021 at 17:15, AngeloGioacchino Del Regno
wrote:
>
> Il 29/11/21 03:20, Dmitry Baryshkov ha scritto:
> > Hi,
> >
> > On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
> >> Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
> >> DSI host gets initialized earlier, but this caused unability to probe
> >> the entire stack of components because they all depend on interrupts
> >> coming from the main `mdss` node (mdp5, or dpu1).
> >>
> >> To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
> >> them at msm_pdev_probe() time: this will make sure that we add the
> >> required interrupt controller mapping before dsi and/or other components
> >> try to initialize, finally satisfying the dependency.
> >>
> >> While at it, also change the allocation of msm_drm_private to use the
> >> devm variant of kzalloc().
> >>
> >> Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
> >> Signed-off-by: AngeloGioacchino Del Regno
> >>
> >
> > I have been thinking about this. I do not feel that this is the correct
> > approach.
> > Currently DRM device exists only when all components are bound. If any of
> > the
> > subdevices is removed, corresponding component is delteted (and thus all
> > components
> > are unbound), the DRM device is taken down. This results in the state
> > cleanup,
> > userspace notifications, etc.
> >
> > With your changes, DRM device will continue to exist even after one of
> > subdevices
> > is removed. This is not an expected behaviour, since subdrivers do not
> > perform full
> > cleanup, delegating that to DRM device takedown.
> >
> > I suppose that proper solution would be to split msm_drv.c into into:
> > - generic components & drm code to be called from mdp4/mdp5/dpu driver
> > (making
> > mdp4, mdp5 or dpu1 the components master)
> >
> > - bare mdss driver, taking care only about IRQs, OF devices population -
> > calling
> > proper mdss_init/mdss_destroy functions. Most probably we can drop this part
> > altogether and just make md5_mdss.c/dpu_mdss.c proper platform drivers.
> >
>
>
> Hmm... getting a better look on how things are structured... yes, I mostly
> agree
> with you, though I'm not sure about making MDP{4,5}/DPU1 the component
> master; that
> would result in a major change in drm-msm, which may be "a bit too much".
>
> Don't misunderstand me here, please, major changes are fine - but I feel
> urgency
> to get this bug solved ASAP (since drm-msm is currently broken at least for
> drm
> bridges) and, if we do anything major, that would require a very careful slow
> review process that will leave this driver broken for a lot of time.
Yep. I wanted to hear your and Rob's opinion, that's why I did not
rush into implementing it.
I still think this is the way to go in the long term. What I really
liked in your patchset was untying the knot between component binds
returning EPROBE_DEFER and mdss subdevices being in place. This solved
the "dsi host registration" infinite loop for me.
>
> I actually tried something else that "simplifies" the former approach, so
> here's
> my proposal:
> * we introduce {mdp5,dpu}_mdss_early_init(struct device, struct
> msm_drm_private)
> * allocate only msm_drm_private in msm_pdev_probe, leaving the drm_dev_alloc
> call
>into msm_drm_init(), so that the drm_dev_put() stays in msm_drm_uninit()
> * pass msm_drm_private as drvdata instead of drm_device
> * change all the drvdata users to get drm_device from priv->dev, instead of
> getting
>msm_drm_private from drm_device->dev_private (like many other drm drivers
> are
>currently doing)
This sounds in a way that it should work. I'm looking forward to
seeing (and testing) your patches.
>
> This way, we keep the current flow of creating the DRM device at msm_drm_init
> time
> and tearing it down at msm_drm_unbind time, solving the issue that you are
> describing.
>
> If you're okay with this kind of approach, I have two patches here that are
> 95%
> ready, can finish them off and send briefly.
>
> Though, something else must be noted here... in the last mail where I'm
> pasting
> a crash that happens when running 'rmmod panel_edp ti_sn65dsi86', I have
> implied
> that this is happening due to the patch that I've sent: after some more
> research,
> I'm not convinced anymore that this is a consequence of that. That crash may
> not
> be related to my fix at all, but to something else (perhaps also related to
> commit
> 8f59ee9a570c, the one that we're fixing here).
>
> Of course, that crash still happens even with the approach that I've just
> proposed.
>
>
> Looking forward for your opinion!
>
> Cheers,
> - Angelo
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] drm/msm: Initialize MDSS irq domain at probe time
Il 29/11/21 03:20, Dmitry Baryshkov ha scritto:
Hi,
On 25/11/2021 18:09, AngeloGioacchino Del Regno wrote:
Since commit 8f59ee9a570c ("drm/msm/dsi: Adjust probe order"), the
DSI host gets initialized earlier, but this caused unability to probe
the entire stack of components because they all depend on interrupts
coming from the main `mdss` node (mdp5, or dpu1).
To fix this issue, also anticipate probing mdp5 or dpu1 by initializing
them at msm_pdev_probe() time: this will make sure that we add the
required interrupt controller mapping before dsi and/or other components
try to initialize, finally satisfying the dependency.
While at it, also change the allocation of msm_drm_private to use the
devm variant of kzalloc().
Fixes: 8f59ee9a570c ("drm/msm/dsi: Adjust probe order")
Signed-off-by: AngeloGioacchino Del Regno
I have been thinking about this. I do not feel that this is the correct approach.
Currently DRM device exists only when all components are bound. If any of the
subdevices is removed, corresponding component is delteted (and thus all components
are unbound), the DRM device is taken down. This results in the state cleanup,
userspace notifications, etc.
With your changes, DRM device will continue to exist even after one of subdevices
is removed. This is not an expected behaviour, since subdrivers do not perform full
cleanup, delegating that to DRM device takedown.
I suppose that proper solution would be to split msm_drv.c into into:
- generic components & drm code to be called from mdp4/mdp5/dpu driver (making
mdp4, mdp5 or dpu1 the components master)
- bare mdss driver, taking care only about IRQs, OF devices population - calling
proper mdss_init/mdss_destroy functions. Most probably we can drop this part
altogether and just make md5_mdss.c/dpu_mdss.c proper platform drivers.
Hmm... getting a better look on how things are structured... yes, I mostly agree
with you, though I'm not sure about making MDP{4,5}/DPU1 the component master;
that
would result in a major change in drm-msm, which may be "a bit too much".
Don't misunderstand me here, please, major changes are fine - but I feel urgency
to get this bug solved ASAP (since drm-msm is currently broken at least for drm
bridges) and, if we do anything major, that would require a very careful slow
review process that will leave this driver broken for a lot of time.
I actually tried something else that "simplifies" the former approach, so here's
my proposal:
* we introduce {mdp5,dpu}_mdss_early_init(struct device, struct msm_drm_private)
* allocate only msm_drm_private in msm_pdev_probe, leaving the drm_dev_alloc
call
into msm_drm_init(), so that the drm_dev_put() stays in msm_drm_uninit()
* pass msm_drm_private as drvdata instead of drm_device
* change all the drvdata users to get drm_device from priv->dev, instead of
getting
msm_drm_private from drm_device->dev_private (like many other drm drivers are
currently doing)
This way, we keep the current flow of creating the DRM device at msm_drm_init
time
and tearing it down at msm_drm_unbind time, solving the issue that you are
describing.
If you're okay with this kind of approach, I have two patches here that are 95%
ready, can finish them off and send briefly.
Though, something else must be noted here... in the last mail where I'm pasting
a crash that happens when running 'rmmod panel_edp ti_sn65dsi86', I have implied
that this is happening due to the patch that I've sent: after some more
research,
I'm not convinced anymore that this is a consequence of that. That crash may not
be related to my fix at all, but to something else (perhaps also related to
commit
8f59ee9a570c, the one that we're fixing here).
Of course, that crash still happens even with the approach that I've just
proposed.
Looking forward for your opinion!
Cheers,
- Angelo
