On 23/06/2023 03:32, Abhinav Kumar wrote:
On 6/22/2023 5:17 PM, Dmitry Baryshkov wrote:
On 23/06/2023 03:14, Abhinav Kumar wrote:
On 6/19/2023 2:06 PM, Dmitry Baryshkov wrote:
Provide actual documentation for the pclk and hdisplay calculations in
the case of DSC compression being used.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
Changes since v1:
- Converted dsi_adjust_pclk_for_compression() into kerneldoc (Marijn)
- Added a pointer from dsi_timing_setup() docs to
dsi_adjust_pclk_for_compression() (Marijn)
- Fixed two typo (Marijn)
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 40
++++++++++++++++++++++++++++--
1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 3f6dfb4f9d5a..a8a31c3dd168 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -528,6 +528,25 @@ void dsi_link_clk_disable_v2(struct
msm_dsi_host *msm_host)
clk_disable_unprepare(msm_host->byte_clk);
}
+/**
+ * dsi_adjust_pclk_for_compression() - Adjust the pclk rate for
compression case
+ * @mode: the selected mode for the DSI output
+ * @dsc: DRM DSC configuration for this DSI output
+ *
+ * Adjust the pclk rate by calculating a new hdisplay proportional to
+ * the compression ratio such that:
+ * new_hdisplay = old_hdisplay * compressed_bpp / uncompressed_bpp
+ *
+ * Porches do not need to be adjusted:
+ * - For the VIDEO mode they are not compressed by DSC and are
passed as is.
+ * - For the CMD mode there are no actual porches. Instead these
fields
+ * currently represent the overhead to the image data transfer.
As such, they
+ * are calculated for the final mode parameters (after the
compression) and
+ * are not to be adjusted too.
+ *
+ * FIXME: Reconsider this if/when CMD mode handling is rewritten
to use
+ * refresh rate and data overhead as a starting point of the
calculations.
+ */
static unsigned long dsi_adjust_pclk_for_compression(const struct
drm_display_mode *mode,
const struct drm_dsc_config *dsc)
I am fine with this part of the doc.
{
@@ -926,8 +945,25 @@ static void dsi_timing_setup(struct
msm_dsi_host *msm_host, bool is_bonded_dsi)
if (ret)
return;
- /* Divide the display by 3 but keep back/font porch and
- * pulse width same
+ /*
+ * DPU sends 3 bytes per pclk cycle to DSI. If compression is
+ * not used, a single pixel is transferred at each pulse, no
+ * matter what bpp or pixel format is used. In case of DSC
+ * compression this results (due to data alignment
+ * requirements) in a transfer of 3 compressed pixel per pclk
+ * cycle.
+ *
I dont want to talk about data alignment nor formats and I will not
ack any references to those.
I would like to keep this simple and say that DPU sends 3 bytes of
compressed data / pclk (6 with widebus enabled) and all this math is
doing is that its calculating number of bytes and diving it by 3 OR 6
with widebus to calculate the pclk cycles. Thats it.
This makes it unclear, why do we simply by 3 rather than doing *
dsc_bpp / 24. My description might be inaccurate as I don't have hw
docs at hand, but simple description is not enough.
Why is it unclear? With compression, we are saying we process at 3
compressed bytes / pclk and this math is accurately giving the pclk cycles.
You are once again trying to arrive at 3 with compression factor in mind
by calculating target_bpp / src_bpp.
I am saying that its independent of that. Whenever we do compression
rate is 3 bytes of compressed data (and 6 with widebus) irrespective of
what your dsc_bpp was.
3 bytes of compressed data = 4 * 6bpp pixels. So hdisplay should have
been divided by 4 in such case. So, this is not just about bytes.
+ * If widebus is enabled, bus width is extended to 6 bytes.
+ * This way the DPU can transfer 6 compressed pixels with bpp
+ * less or equal to 8 or 3 compressed pixels in case bpp is
+ * greater than 8.
+ *
+ * The back/font porch and pulse width are kept intact. They
+ * represent timing parameters rather than actual data
+ * transfer. See the documentation of
+ * dsi_adjust_pclk_for_compression().
this part is fine.
+ *
+ * XXX: widebus is not supported by the driver (yet).
*/
h_total -= hdisplay;
hdisplay =
DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
--
With best wishes
Dmitry
