When DSC is enabled, we need to configure DSI registers accordingly and
configure the respective stream compression registers.
Add support to calculate the register setting based on DSC params and
timing information and configure these registers.
Signed-off-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Vinod Koul <[email protected]>
---
drivers/gpu/drm/msm/dsi/dsi.xml.h | 10 +++
drivers/gpu/drm/msm/dsi/dsi_host.c | 109 ++++++++++++++++++++++++++++-
2 files changed, 118 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
index 49b551ad1bff..c1c85df58c4b 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
@@ -706,4 +706,14 @@ static inline uint32_t DSI_VERSION_MAJOR(uint32_t val)
#define REG_DSI_CPHY_MODE_CTRL 0x000002d4
+#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL 0x0000029c
+
+#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL2 0x000002a0
+
+#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL 0x000002a4
+
+#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2 0x000002a8
+
+#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL3 0x000002ac
+
#endif /* DSI_XML */
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 438c80750682..3d8d5a1daaa3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -908,6 +908,20 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
dsi_write(msm_host, REG_DSI_CPHY_MODE_CTRL, BIT(0));
}
+static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
+ int pic_width, int pic_height)
+{
+ if (!dsc || !pic_width || !pic_height) {
+ pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
+ return -EINVAL;
+ }
+
+ dsc->drm->pic_width = pic_width;
+ dsc->drm->pic_height = pic_height;
+
+ return 0;
+}
+
static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
{
struct drm_display_mode *mode = msm_host->mode;
@@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
hdisplay /= 2;
}
+ if (msm_host->dsc) {
+ struct msm_display_dsc_config *dsc = msm_host->dsc;
+
+ /* update dsc params with timing params */
+ dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
+ DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width, dsc->drm->pic_height);
+
+ /* we do the calculations for dsc parameters here so that
+ * panel can use these parameters
+ */
+ dsi_populate_dsc_params(dsc);
+
+ /* Divide the display by 3 but keep back/font porch and
+ * pulse width same
+ */
+ h_total -= hdisplay;
+ hdisplay /= 3;
+ h_total += hdisplay;
+ ha_end = ha_start + hdisplay;
+ }
+
if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
+ if (msm_host->dsc) {
+ struct msm_display_dsc_config *dsc = msm_host->dsc;
+ u32 reg, intf_width, slice_per_intf;
+ u32 total_bytes_per_intf;
+
+ /* first calculate dsc parameters and then program
+ * compress mode registers
+ */
+ intf_width = hdisplay;
+ slice_per_intf = DIV_ROUND_UP(intf_width, dsc->drm->slice_width);
+
+ dsc->drm->slice_count = 1;
+ dsc->bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * 8, 8);
+ total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
+
+ dsc->eol_byte_num = total_bytes_per_intf % 3;
+ dsc->pclk_per_line = DIV_ROUND_UP(total_bytes_per_intf, 3);
+ dsc->bytes_per_pkt = dsc->bytes_in_slice * dsc->drm->slice_count;
+ dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
+
+ reg = dsc->bytes_per_pkt << 16;
+ reg |= (0x0b << 8); /* dtype of compressed image */
+
+ /* pkt_per_line:
+ * 0 == 1 pkt
+ * 1 == 2 pkt
+ * 2 == 4 pkt
+ * 3 pkt is not supported
+ * above translates to ffs() - 1
+ */
+ reg |= (ffs(dsc->pkt_per_line) - 1) << 6;
+
+ dsc->eol_byte_num = total_bytes_per_intf % 3;
+ reg |= dsc->eol_byte_num << 4;
+ reg |= 1;
+
+ dsi_write(msm_host,
+ REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
+ }
+
dsi_write(msm_host, REG_DSI_ACTIVE_H,
DSI_ACTIVE_H_START(ha_start) |
DSI_ACTIVE_H_END(ha_end));
@@ -959,8 +1034,40 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
} else { /* command mode */
+ if (msm_host->dsc) {
+ struct msm_display_dsc_config *dsc = msm_host->dsc;
+ u32 reg, reg_ctrl, reg_ctrl2;
+ u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
+
+ reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
+ reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
+
+ slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->drm->slice_width);
+ bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
+ dsc->drm->bits_per_pixel, 8);
+ dsc->drm->slice_chunk_size = bytes_in_slice;
+ total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
+ dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
+
+ reg = 0x39 << 8;
+ reg |= ffs(dsc->pkt_per_line) << 6;
+
+ dsc->eol_byte_num = total_bytes_per_intf % 3;
+ reg |= dsc->eol_byte_num << 4;
+ reg |= 1;
+
+ reg_ctrl |= reg;
+ reg_ctrl2 |= bytes_in_slice;
+
+ dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
+ dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
+ }
+
/* image data and 1 byte write_memory_start cmd */
- wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
+ if (!msm_host->dsc)
+ wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
+ else
+ wc = mode->hdisplay / 2 + 1;
dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
--
2.31.1
Vinod,
On 2022-02-10 16:04:23, Vinod Koul wrote:
> When DSC is enabled, we need to configure DSI registers accordingly and
> configure the respective stream compression registers.
>
> Add support to calculate the register setting based on DSC params and
> timing information and configure these registers.
>
> Signed-off-by: Dmitry Baryshkov <[email protected]>
> Signed-off-by: Vinod Koul <[email protected]>
I supplied a rather extensive - yet merely scratching the surface -
review of this patch in:
https://lore.kernel.org/linux-arm-msm/[email protected]/
It seems none of those points have been addressed, bar creating a mesa
MR to update dsi.xml with a subpar description of the registers (offsets
only).
For every point that is intentionally ignored, please at least supply a
justification of why you think this is the right thing to do.
Thanks,
- Marijn
> ---
> drivers/gpu/drm/msm/dsi/dsi.xml.h | 10 +++
> drivers/gpu/drm/msm/dsi/dsi_host.c | 109 ++++++++++++++++++++++++++++-
> 2 files changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> index 49b551ad1bff..c1c85df58c4b 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> @@ -706,4 +706,14 @@ static inline uint32_t DSI_VERSION_MAJOR(uint32_t val)
> #define REG_DSI_CPHY_MODE_CTRL 0x000002d4
>
>
> +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL 0x0000029c
> +
> +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL2 0x000002a0
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL 0x000002a4
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2 0x000002a8
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL3 0x000002ac
> +
> #endif /* DSI_XML */
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 438c80750682..3d8d5a1daaa3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -908,6 +908,20 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
> dsi_write(msm_host, REG_DSI_CPHY_MODE_CTRL, BIT(0));
> }
>
> +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> + int pic_width, int pic_height)
> +{
> + if (!dsc || !pic_width || !pic_height) {
> + pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
> + return -EINVAL;
> + }
> +
> + dsc->drm->pic_width = pic_width;
> + dsc->drm->pic_height = pic_height;
> +
> + return 0;
> +}
> +
> static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> {
> struct drm_display_mode *mode = msm_host->mode;
> @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> hdisplay /= 2;
> }
>
> + if (msm_host->dsc) {
> + struct msm_display_dsc_config *dsc = msm_host->dsc;
> +
> + /* update dsc params with timing params */
> + dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
> + DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width, dsc->drm->pic_height);
> +
> + /* we do the calculations for dsc parameters here so that
> + * panel can use these parameters
> + */
> + dsi_populate_dsc_params(dsc);
> +
> + /* Divide the display by 3 but keep back/font porch and
> + * pulse width same
> + */
> + h_total -= hdisplay;
> + hdisplay /= 3;
> + h_total += hdisplay;
> + ha_end = ha_start + hdisplay;
> + }
> +
> if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
> + if (msm_host->dsc) {
> + struct msm_display_dsc_config *dsc = msm_host->dsc;
> + u32 reg, intf_width, slice_per_intf;
> + u32 total_bytes_per_intf;
> +
> + /* first calculate dsc parameters and then program
> + * compress mode registers
> + */
> + intf_width = hdisplay;
> + slice_per_intf = DIV_ROUND_UP(intf_width, dsc->drm->slice_width);
> +
> + dsc->drm->slice_count = 1;
> + dsc->bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * 8, 8);
> + total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> +
> + dsc->eol_byte_num = total_bytes_per_intf % 3;
> + dsc->pclk_per_line = DIV_ROUND_UP(total_bytes_per_intf, 3);
> + dsc->bytes_per_pkt = dsc->bytes_in_slice * dsc->drm->slice_count;
> + dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> +
> + reg = dsc->bytes_per_pkt << 16;
> + reg |= (0x0b << 8); /* dtype of compressed image */
> +
> + /* pkt_per_line:
> + * 0 == 1 pkt
> + * 1 == 2 pkt
> + * 2 == 4 pkt
> + * 3 pkt is not supported
> + * above translates to ffs() - 1
> + */
> + reg |= (ffs(dsc->pkt_per_line) - 1) << 6;
> +
> + dsc->eol_byte_num = total_bytes_per_intf % 3;
> + reg |= dsc->eol_byte_num << 4;
> + reg |= 1;
> +
> + dsi_write(msm_host,
> + REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> + }
> +
> dsi_write(msm_host, REG_DSI_ACTIVE_H,
> DSI_ACTIVE_H_START(ha_start) |
> DSI_ACTIVE_H_END(ha_end));
> @@ -959,8 +1034,40 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
> DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
> } else { /* command mode */
> + if (msm_host->dsc) {
> + struct msm_display_dsc_config *dsc = msm_host->dsc;
> + u32 reg, reg_ctrl, reg_ctrl2;
> + u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
> +
> + reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> + reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> +
> + slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->drm->slice_width);
> + bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
> + dsc->drm->bits_per_pixel, 8);
> + dsc->drm->slice_chunk_size = bytes_in_slice;
> + total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> + dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> +
> + reg = 0x39 << 8;
> + reg |= ffs(dsc->pkt_per_line) << 6;
> +
> + dsc->eol_byte_num = total_bytes_per_intf % 3;
> + reg |= dsc->eol_byte_num << 4;
> + reg |= 1;
> +
> + reg_ctrl |= reg;
> + reg_ctrl2 |= bytes_in_slice;
> +
> + dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> + dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
> + }
> +
> /* image data and 1 byte write_memory_start cmd */
> - wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
> + if (!msm_host->dsc)
> + wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
> + else
> + wc = mode->hdisplay / 2 + 1;
>
> dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
> DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
> --
> 2.31.1
>
Hi Marijn,
On 17-02-22, 10:27, Marijn Suijten wrote:
> Vinod,
>
> On 2022-02-10 16:04:23, Vinod Koul wrote:
> > When DSC is enabled, we need to configure DSI registers accordingly and
> > configure the respective stream compression registers.
> >
> > Add support to calculate the register setting based on DSC params and
> > timing information and configure these registers.
> >
> > Signed-off-by: Dmitry Baryshkov <[email protected]>
> > Signed-off-by: Vinod Koul <[email protected]>
>
> I supplied a rather extensive - yet merely scratching the surface -
> review of this patch in:
>
> https://lore.kernel.org/linux-arm-msm/[email protected]/
Sorry somehow I seem to have overlooked this one.
>
> It seems none of those points have been addressed, bar creating a mesa
> MR to update dsi.xml with a subpar description of the registers (offsets
> only).
Mesa MR was planned as we get more closer to having reviews.
> For every point that is intentionally ignored, please at least supply a
> justification of why you think this is the right thing to do.
Ofcourse, i will reply to these now and address as required.
--
~Vinod
On 2/10/2022 2:34 AM, Vinod Koul wrote:
> When DSC is enabled, we need to configure DSI registers accordingly and
> configure the respective stream compression registers.
>
> Add support to calculate the register setting based on DSC params and
> timing information and configure these registers.
>
> Signed-off-by: Dmitry Baryshkov <[email protected]>
> Signed-off-by: Vinod Koul <[email protected]>
> ---
> drivers/gpu/drm/msm/dsi/dsi.xml.h | 10 +++
> drivers/gpu/drm/msm/dsi/dsi_host.c | 109 ++++++++++++++++++++++++++++-
> 2 files changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> index 49b551ad1bff..c1c85df58c4b 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> @@ -706,4 +706,14 @@ static inline uint32_t DSI_VERSION_MAJOR(uint32_t val)
> #define REG_DSI_CPHY_MODE_CTRL 0x000002d4
>
>
> +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL 0x0000029c
> +
> +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL2 0x000002a0
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL 0x000002a4
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2 0x000002a8
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL3 0x000002ac
> +
This file should not be edited manually. The updates have to be
generated using the headergen tool.
> #endif /* DSI_XML */
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 438c80750682..3d8d5a1daaa3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -908,6 +908,20 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
> dsi_write(msm_host, REG_DSI_CPHY_MODE_CTRL, BIT(0));
> }
>
> +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> + int pic_width, int pic_height)
> +{
> + if (!dsc || !pic_width || !pic_height) {
> + pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
> + return -EINVAL;
> + }
> +
> + dsc->drm->pic_width = pic_width;
> + dsc->drm->pic_height = pic_height;
> +
> + return 0;
> +}
> +
> static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> {
> struct drm_display_mode *mode = msm_host->mode;
> @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> hdisplay /= 2;
> }
>
> + if (msm_host->dsc) {
> + struct msm_display_dsc_config *dsc = msm_host->dsc;
> +
> + /* update dsc params with timing params */
> + dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
> + DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width, dsc->drm->pic_height);
> +
> + /* we do the calculations for dsc parameters here so that
> + * panel can use these parameters
> + */
> + dsi_populate_dsc_params(dsc);
> +
> + /* Divide the display by 3 but keep back/font porch and
> + * pulse width same
> + */
> + h_total -= hdisplay;
> + hdisplay /= 3;
> + h_total += hdisplay;
> + ha_end = ha_start + hdisplay;
> + }
> +
> if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
> + if (msm_host->dsc) {
> + struct msm_display_dsc_config *dsc = msm_host->dsc;
> + u32 reg, intf_width, slice_per_intf;
> + u32 total_bytes_per_intf;
> +
> + /* first calculate dsc parameters and then program
> + * compress mode registers
> + */
> + intf_width = hdisplay;
> + slice_per_intf = DIV_ROUND_UP(intf_width, dsc->drm->slice_width);
> +
> + dsc->drm->slice_count = 1;
Why is this hard-coded to 1 here? Am i missing something?
I think I need another day to look into these calculations.
> + dsc->bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * 8, 8);
> + total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> +
> + dsc->eol_byte_num = total_bytes_per_intf % 3;
> + dsc->pclk_per_line = DIV_ROUND_UP(total_bytes_per_intf, 3);
> + dsc->bytes_per_pkt = dsc->bytes_in_slice * dsc->drm->slice_count;
> + dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> +
> + reg = dsc->bytes_per_pkt << 16;
> + reg |= (0x0b << 8); /* dtype of compressed image */
> +
> + /* pkt_per_line:
> + * 0 == 1 pkt
> + * 1 == 2 pkt
> + * 2 == 4 pkt
> + * 3 pkt is not supported
> + * above translates to ffs() - 1
> + */
> + reg |= (ffs(dsc->pkt_per_line) - 1) << 6;
> +
> + dsc->eol_byte_num = total_bytes_per_intf % 3;
> + reg |= dsc->eol_byte_num << 4;
> + reg |= 1;
> +
> + dsi_write(msm_host,
> + REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> + }
> +
> dsi_write(msm_host, REG_DSI_ACTIVE_H,
> DSI_ACTIVE_H_START(ha_start) |
> DSI_ACTIVE_H_END(ha_end));
> @@ -959,8 +1034,40 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
> DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
> } else { /* command mode */
> + if (msm_host->dsc) {
> + struct msm_display_dsc_config *dsc = msm_host->dsc;
> + u32 reg, reg_ctrl, reg_ctrl2;
> + u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
> +
> + reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> + reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> +
> + slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->drm->slice_width);
> + bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
> + dsc->drm->bits_per_pixel, 8);
> + dsc->drm->slice_chunk_size = bytes_in_slice;
> + total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> + dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> +
> + reg = 0x39 << 8;
> + reg |= ffs(dsc->pkt_per_line) << 6;
> +
> + dsc->eol_byte_num = total_bytes_per_intf % 3;
> + reg |= dsc->eol_byte_num << 4;
> + reg |= 1;
> +
> + reg_ctrl |= reg;
> + reg_ctrl2 |= bytes_in_slice;
> +
> + dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> + dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
> + }
> +
> /* image data and 1 byte write_memory_start cmd */
> - wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
> + if (!msm_host->dsc)
> + wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
> + else
> + wc = mode->hdisplay / 2 + 1;
>
> dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
> DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
Hi Vinod,
On 2022-02-17 16:21:35, Vinod Koul wrote:
> Hi Marijn,
>
> On 17-02-22, 10:27, Marijn Suijten wrote:
> > Vinod,
> >
> > On 2022-02-10 16:04:23, Vinod Koul wrote:
> > > When DSC is enabled, we need to configure DSI registers accordingly and
> > > configure the respective stream compression registers.
> > >
> > > Add support to calculate the register setting based on DSC params and
> > > timing information and configure these registers.
> > >
> > > Signed-off-by: Dmitry Baryshkov <[email protected]>
> > > Signed-off-by: Vinod Koul <[email protected]>
> >
> > I supplied a rather extensive - yet merely scratching the surface -
> > review of this patch in:
> >
> > https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> Sorry somehow I seem to have overlooked this one.
Glad to hear it was only just missed, hope it helps making these patches
truly a step-up from the downstream driver it was based on :)
> >
> > It seems none of those points have been addressed, bar creating a mesa
> > MR to update dsi.xml with a subpar description of the registers (offsets
> > only).
>
> Mesa MR was planned as we get more closer to having reviews.
I would have submitted them in parallel straight away and link them
together, so that review can commence immediately on both. Perhaps then
the XML could be merged up-front so that these patches don't need to
block on a stable commit-hash to regenerate the headers from?
In this case I/we would have immediately requested to move all the
register constants and offsets (even those that currently have an
unknown meaning) into the XML, and we can later fill in the details.
> > For every point that is intentionally ignored, please at least supply a
> > justification of why you think this is the right thing to do.
>
> Ofcourse, i will reply to these now and address as required.
Thanks, I'll try to clarify some points ASAP before you respin this.
- Marijn
On 10/02/2022 13:34, Vinod Koul wrote:
> When DSC is enabled, we need to configure DSI registers accordingly and
> configure the respective stream compression registers.
>
> Add support to calculate the register setting based on DSC params and
> timing information and configure these registers.
>
> Signed-off-by: Dmitry Baryshkov <[email protected]>
> Signed-off-by: Vinod Koul <[email protected]>
> ---
> drivers/gpu/drm/msm/dsi/dsi.xml.h | 10 +++
> drivers/gpu/drm/msm/dsi/dsi_host.c | 109 ++++++++++++++++++++++++++++-
> 2 files changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> index 49b551ad1bff..c1c85df58c4b 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> @@ -706,4 +706,14 @@ static inline uint32_t DSI_VERSION_MAJOR(uint32_t val)
> #define REG_DSI_CPHY_MODE_CTRL 0x000002d4
>
>
> +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL 0x0000029c
> +
> +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL2 0x000002a0
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL 0x000002a4
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2 0x000002a8
> +
> +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL3 0x000002ac
> +
> #endif /* DSI_XML */
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 438c80750682..3d8d5a1daaa3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -908,6 +908,20 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
> dsi_write(msm_host, REG_DSI_CPHY_MODE_CTRL, BIT(0));
> }
>
> +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> + int pic_width, int pic_height)
> +{
> + if (!dsc || !pic_width || !pic_height) {
> + pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
> + return -EINVAL;
> + }
> +
> + dsc->drm->pic_width = pic_width;
> + dsc->drm->pic_height = pic_height;
> +
> + return 0;
> +}
> +
> static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> {
> struct drm_display_mode *mode = msm_host->mode;
> @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> hdisplay /= 2;
> }
>
> + if (msm_host->dsc) {
> + struct msm_display_dsc_config *dsc = msm_host->dsc;
> +
> + /* update dsc params with timing params */
> + dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
> + DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width, dsc->drm->pic_height);
> +
> + /* we do the calculations for dsc parameters here so that
> + * panel can use these parameters
> + */
> + dsi_populate_dsc_params(dsc);
> +
> + /* Divide the display by 3 but keep back/font porch and
> + * pulse width same
> + */
> + h_total -= hdisplay;
> + hdisplay /= 3;
> + h_total += hdisplay;
> + ha_end = ha_start + hdisplay;
> + }
> +
> if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
> + if (msm_host->dsc) {
> + struct msm_display_dsc_config *dsc = msm_host->dsc;
> + u32 reg, intf_width, slice_per_intf;
> + u32 total_bytes_per_intf;
> +
> + /* first calculate dsc parameters and then program
> + * compress mode registers
> + */
> + intf_width = hdisplay;
> + slice_per_intf = DIV_ROUND_UP(intf_width, dsc->drm->slice_width);
> +
> + dsc->drm->slice_count = 1;
> + dsc->bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * 8, 8);
> + total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> +
> + dsc->eol_byte_num = total_bytes_per_intf % 3;
> + dsc->pclk_per_line = DIV_ROUND_UP(total_bytes_per_intf, 3);
> + dsc->bytes_per_pkt = dsc->bytes_in_slice * dsc->drm->slice_count;
> + dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> +
> + reg = dsc->bytes_per_pkt << 16;
> + reg |= (0x0b << 8); /* dtype of compressed image */
> +
> + /* pkt_per_line:
> + * 0 == 1 pkt
> + * 1 == 2 pkt
> + * 2 == 4 pkt
> + * 3 pkt is not supported
> + * above translates to ffs() - 1
> + */
> + reg |= (ffs(dsc->pkt_per_line) - 1) << 6;
> +
> + dsc->eol_byte_num = total_bytes_per_intf % 3;
> + reg |= dsc->eol_byte_num << 4;
> + reg |= 1;
> +
> + dsi_write(msm_host,
> + REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> + }
> +
> dsi_write(msm_host, REG_DSI_ACTIVE_H,
> DSI_ACTIVE_H_START(ha_start) |
> DSI_ACTIVE_H_END(ha_end));
> @@ -959,8 +1034,40 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
> DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
> } else { /* command mode */
> + if (msm_host->dsc) {
> + struct msm_display_dsc_config *dsc = msm_host->dsc;
> + u32 reg, reg_ctrl, reg_ctrl2;
> + u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
> +
> + reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> + reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> +
> + slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->drm->slice_width);
> + bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
> + dsc->drm->bits_per_pixel, 8);
> + dsc->drm->slice_chunk_size = bytes_in_slice;
> + total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
Should we use dsc->bytes_in_slice or just bytes_in_slice here?
> + dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> +
> + reg = 0x39 << 8;
> + reg |= ffs(dsc->pkt_per_line) << 6;
> +
> + dsc->eol_byte_num = total_bytes_per_intf % 3;
> + reg |= dsc->eol_byte_num << 4;
> + reg |= 1;
> +
> + reg_ctrl |= reg;
> + reg_ctrl2 |= bytes_in_slice;
> +
> + dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg);
> + dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
> + }
> +
> /* image data and 1 byte write_memory_start cmd */
> - wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
> + if (!msm_host->dsc)
> + wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
> + else
> + wc = mode->hdisplay / 2 + 1;
>
> dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
> DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
--
With best wishes
Dmitry
On 21-02-22, 05:11, Dmitry Baryshkov wrote:
> On 10/02/2022 13:34, Vinod Koul wrote:
> > When DSC is enabled, we need to configure DSI registers accordingly and
> > configure the respective stream compression registers.
> >
> > Add support to calculate the register setting based on DSC params and
> > timing information and configure these registers.
> >
> > Signed-off-by: Dmitry Baryshkov <[email protected]>
> > Signed-off-by: Vinod Koul <[email protected]>
> > ---
> > drivers/gpu/drm/msm/dsi/dsi.xml.h | 10 +++
> > drivers/gpu/drm/msm/dsi/dsi_host.c | 109 ++++++++++++++++++++++++++++-
> > 2 files changed, 118 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > index 49b551ad1bff..c1c85df58c4b 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > @@ -706,4 +706,14 @@ static inline uint32_t DSI_VERSION_MAJOR(uint32_t val)
> > #define REG_DSI_CPHY_MODE_CTRL 0x000002d4
> > +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL 0x0000029c
> > +
> > +#define REG_DSI_VIDEO_COMPRESSION_MODE_CTRL2 0x000002a0
> > +
> > +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL 0x000002a4
> > +
> > +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2 0x000002a8
> > +
> > +#define REG_DSI_COMMAND_COMPRESSION_MODE_CTRL3 0x000002ac
> > +
> > #endif /* DSI_XML */
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 438c80750682..3d8d5a1daaa3 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -908,6 +908,20 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable,
> > dsi_write(msm_host, REG_DSI_CPHY_MODE_CTRL, BIT(0));
> > }
> > +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> > + int pic_width, int pic_height)
> > +{
> > + if (!dsc || !pic_width || !pic_height) {
> > + pr_err("DSI: invalid input: pic_width: %d pic_height: %d\n", pic_width, pic_height);
> > + return -EINVAL;
> > + }
> > +
> > + dsc->drm->pic_width = pic_width;
> > + dsc->drm->pic_height = pic_height;
> > +
> > + return 0;
> > +}
> > +
> > static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > {
> > struct drm_display_mode *mode = msm_host->mode;
> > @@ -940,7 +954,68 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > hdisplay /= 2;
> > }
> > + if (msm_host->dsc) {
> > + struct msm_display_dsc_config *dsc = msm_host->dsc;
> > +
> > + /* update dsc params with timing params */
> > + dsi_dsc_update_pic_dim(dsc, mode->hdisplay, mode->vdisplay);
> > + DBG("Mode Width- %d x Height %d\n", dsc->drm->pic_width, dsc->drm->pic_height);
> > +
> > + /* we do the calculations for dsc parameters here so that
> > + * panel can use these parameters
> > + */
> > + dsi_populate_dsc_params(dsc);
> > +
> > + /* Divide the display by 3 but keep back/font porch and
> > + * pulse width same
> > + */
> > + h_total -= hdisplay;
> > + hdisplay /= 3;
> > + h_total += hdisplay;
> > + ha_end = ha_start + hdisplay;
> > + }
> > +
> > if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
> > + if (msm_host->dsc) {
> > + struct msm_display_dsc_config *dsc = msm_host->dsc;
> > + u32 reg, intf_width, slice_per_intf;
> > + u32 total_bytes_per_intf;
> > +
> > + /* first calculate dsc parameters and then program
> > + * compress mode registers
> > + */
> > + intf_width = hdisplay;
> > + slice_per_intf = DIV_ROUND_UP(intf_width, dsc->drm->slice_width);
> > +
> > + dsc->drm->slice_count = 1;
> > + dsc->bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * 8, 8);
> > + total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
> > +
> > + dsc->eol_byte_num = total_bytes_per_intf % 3;
> > + dsc->pclk_per_line = DIV_ROUND_UP(total_bytes_per_intf, 3);
> > + dsc->bytes_per_pkt = dsc->bytes_in_slice * dsc->drm->slice_count;
> > + dsc->pkt_per_line = slice_per_intf / dsc->drm->slice_count;
> > +
> > + reg = dsc->bytes_per_pkt << 16;
> > + reg |= (0x0b << 8); /* dtype of compressed image */
> > +
> > + /* pkt_per_line:
> > + * 0 == 1 pkt
> > + * 1 == 2 pkt
> > + * 2 == 4 pkt
> > + * 3 pkt is not supported
> > + * above translates to ffs() - 1
> > + */
> > + reg |= (ffs(dsc->pkt_per_line) - 1) << 6;
> > +
> > + dsc->eol_byte_num = total_bytes_per_intf % 3;
> > + reg |= dsc->eol_byte_num << 4;
> > + reg |= 1;
> > +
> > + dsi_write(msm_host,
> > + REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
> > + }
> > +
> > dsi_write(msm_host, REG_DSI_ACTIVE_H,
> > DSI_ACTIVE_H_START(ha_start) |
> > DSI_ACTIVE_H_END(ha_end));
> > @@ -959,8 +1034,40 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
> > DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
> > } else { /* command mode */
> > + if (msm_host->dsc) {
> > + struct msm_display_dsc_config *dsc = msm_host->dsc;
> > + u32 reg, reg_ctrl, reg_ctrl2;
> > + u32 slice_per_intf, bytes_in_slice, total_bytes_per_intf;
> > +
> > + reg_ctrl = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL);
> > + reg_ctrl2 = dsi_read(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2);
> > +
> > + slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->drm->slice_width);
> > + bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width *
> > + dsc->drm->bits_per_pixel, 8);
> > + dsc->drm->slice_chunk_size = bytes_in_slice;
> > + total_bytes_per_intf = dsc->bytes_in_slice * slice_per_intf;
>
> Should we use dsc->bytes_in_slice or just bytes_in_slice here?
This should be dsc->bytes_in_slice, fixed up now. Thanks for pointing
--
~Vinod