Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
On 6/23/2023 1:18 PM, Marijn Suijten wrote: On 2023-06-23 23:10:56, Dmitry Baryshkov wrote: There is no confusion between what was said earlier and now. This line is calculating the number of pclks needed to transmit one line of the compressed data: hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3); msm_dsc_get_bytes_per_line() is calculating the number of compressed bytes as it uses the target bits_per_pixel 126 * @bits_per_pixel: 127 * Target bits per pixel with 4 fractional bits, bits_per_pixel << 4 128 */ 129 u16 bits_per_pixel; (like I have said a few times, hdisplay is perhaps confusing us) If you calculate the bytes this way you are already accounting for the compression, so where is the confusion. The pclk calculation does the same thing of using the ratio instead. This is not answering my question whether the ratio for pclk calculation should also be adjusted to account for widebus. And if the ratio is fixed, why use a fixed factor here but the ratio between src_bpp:target_bpp here? It only adds extra confusion. Wide bus is dicussed separately. I think the question you are trying to ask is "why are we not using msm_dsc_get_bytes_per_line() in dsi_adjust_pclk_for_compression()?" I have asked that question before, and the answer was something incomprehensible. But indeed, it would look more natural if dsi_adjust_pclk_for_compression() replaces: int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc), dsc->bits_per_component * 3) With: int new_hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(dsc), 3); Which is the same value as we have here. And then it becomes more clear how widebus affects this calculation. - Marijn Ok, I finally got your question I think. That "why don't we account for widebus in the pixel clock calculation but only for calculating pclk cycles for REG_DSI_CMD_MDP_STREAM0_TOTAL" Even though DPU shall transmit 48 bits worth of data , DSI will be able to consume only one uncompressed pixel worth of data. Thats the rule. Hence even though we are able to account for widebus in the stream0_total to indicate DPU's increased data transfer rate with widebus, since DSI is still going to consume only one uncompressed pixel worth of data, we can only scale the pixel clock with compression ratio. Hope that clarifies it.
Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
On 2023-06-23 23:10:56, Dmitry Baryshkov wrote: > >> There is no confusion between what was said earlier and now. > >> > >> This line is calculating the number of pclks needed to transmit one line > >> of the compressed data: > >> > >> hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3); > >> > >> msm_dsc_get_bytes_per_line() is calculating the number of compressed > >> bytes as it uses the target bits_per_pixel > >> > >> 126 * @bits_per_pixel: > >> 127 * Target bits per pixel with 4 fractional bits, bits_per_pixel > >> << 4 > >> 128 */ > >> 129u16 bits_per_pixel; > >> > >> (like I have said a few times, hdisplay is perhaps confusing us) > >> > >> If you calculate the bytes this way you are already accounting for the > >> compression, so where is the confusion. > >> > >> The pclk calculation does the same thing of using the ratio instead. > > > > This is not answering my question whether the ratio for pclk calculation > > should also be adjusted to account for widebus. And if the ratio is > > fixed, why use a fixed factor here but the ratio between > > src_bpp:target_bpp here? It only adds extra confusion. > > Wide bus is dicussed separately. I think the question you are trying to > ask is "why are we not using msm_dsc_get_bytes_per_line() in > dsi_adjust_pclk_for_compression()?" I have asked that question before, and the answer was something incomprehensible. But indeed, it would look more natural if dsi_adjust_pclk_for_compression() replaces: int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc), dsc->bits_per_component * 3) With: int new_hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(dsc), 3); Which is the same value as we have here. And then it becomes more clear how widebus affects this calculation. - Marijn
Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
On 23/06/2023 23:02, Marijn Suijten wrote:
On 2023-06-23 12:54:17, Abhinav Kumar wrote:
On 6/23/2023 12:26 AM, Marijn Suijten wrote:
On 2023-06-22 17:32:17, 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
---
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.
Abhinav, this is exactly what the confusion the pclk series is about.
There it was said (and committed to mainline now!) that pclk is based on
the compression factor of target_bpp / src_bpp. Now you are saying
there is a fixed number of bytes sent by the (wide)bus between DPU-DSC
and DSI.
Is pclk used for more purposes besides just ticking for the data
transfer between DPU and DSI?
There is no confusion between what was said earlier and now.
This line is calculating the number of pclks needed to transmit one line
of the compressed data:
hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
msm_dsc_get_bytes_per_line() is calculating the number of compressed
bytes as it uses the target bits_per_pixel
126 * @bits_per_pixel:
127 * Target bits per pixel with 4 fractional bits, bits_per_pixel << 4
128 */
129 u16 bits_per_pixel;
(like I have said a few times, hdisplay is perhaps confusing us)
If you calculate the bytes this way you are already accounting for the
compression, so where is the confusion.
The pclk calculation does the same thing of using the ratio instead.
Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
On 2023-06-23 12:54:17, Abhinav Kumar wrote:
>
>
> On 6/23/2023 12:26 AM, Marijn Suijten wrote:
> > On 2023-06-22 17:32:17, 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
> > ---
> >
> > 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.
> >
> > Abhinav, this is exactly what the confusion the pclk series is about.
> > There it was said (and committed to mainline now!) that pclk is based on
> > the compression factor of target_bpp / src_bpp. Now you are saying
> > there is a fixed number of bytes sent by the (wide)bus between DPU-DSC
> > and DSI.
> >
> > Is pclk used for more purposes besides just ticking for the data
> > transfer between DPU and DSI?
> >
>
> There is no
Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
On 6/23/2023 12:26 AM, Marijn Suijten wrote:
On 2023-06-22 17:32:17, 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
---
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.
Abhinav, this is exactly what the confusion the pclk series is about.
There it was said (and committed to mainline now!) that pclk is based on
the compression factor of target_bpp / src_bpp. Now you are saying
there is a fixed number of bytes sent by the (wide)bus between DPU-DSC
and DSI.
Is pclk used for more purposes besides just ticking for the data
transfer between DPU and DSI?
There is no confusion between what was said earlier and now.
This line is calculating the number of pclks needed to transmit one line
of the compressed data:
hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
msm_dsc_get_bytes_per_line() is calculating the number of compressed
bytes as it uses the target bits_per_pixel
126 * @bits_per_pixel:
127 * Target bits per pixel with 4 fractional bits, bits_per_pixel << 4
128 */
129 u16 bits_per_pixel;
(like I have said a few times, hdisplay is perhaps confusing us)
If you calculate the bytes this way you are already accounting for the
compression, so where is the confusion.
The pclk calculation does the same thing of using the ratio instead.
- Marijn
Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
On 2023-06-22 17:32:17, 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
> >>> ---
> >>>
> >>> 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.
Abhinav, this is exactly what the confusion the pclk series is about.
There it was said (and committed to mainline now!) that pclk is based on
the compression factor of target_bpp / src_bpp. Now you are saying
there is a fixed number of bytes sent by the (wide)bus between DPU-DSC
and DSI.
Is pclk used for more purposes besides just ticking for the data
transfer between DPU and DSI?
- Marijn
Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
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
---
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
Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
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
---
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.
+ * 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);
Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
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
---
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.
+ * 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
Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
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
---
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.
+* 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);
Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
On 2023-06-20 17:27:46, Dmitry Baryshkov wrote:
> On 20/06/2023 15:05, Marijn Suijten wrote:
> > On 2023-06-20 00:06:47, 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
> >> ---
> >>
> >> 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
> >
> > The
> >
> >> + * @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
> >
> > And in v1 you explained that it is _not_ about bpp...
>
> Hmm, this bit stayed intact, so I'm slightly confused here.
It did, because no-one bothered to respond to my questions about this
BPP rate versus "pixels/bytes per pclk" inquiry. So I just keep
repeating it every patch that touches/mentions it, until someone
replies.
> This function is about BPP and compressed rate. dsi_timing_setup() is
> about bytes.
The question is whether that is _correct_. The discussion all this time
is about pclk - even for compressed mode - counting the number of
pixels, not the bpp ratio.
> >> + *
> >> + * 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
> >
> > I feel like "For VIDEO mode" and "For CMD mode" reads more naturally, no
> > need for "the", but I don't know the grammar rule that states so.
>
> Ack
>
> >
> >> + * 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.
> >
> > Nit: well, refresh rate is already part of this calculation (that's how
> > drm_display_mode's clock member comes to be, and how drm_mode_vrefresh()
> > figures out fps after the fact). It's all about the per-CMD transfer
> > overhead in bytes that is currently not part of the calculation.
>
> Please correct me if I'm wrong, we start from mode->clock. Refresh rate
> isn't even a part of struct drm_display_mode.
It is not a separate field because it is _embedded in mode->clock_ (see
how drm_mode_vrefresh() is implemented to divide ->clock by htotal and
vtotal to get the fps again), and hence already part of the values used
in this calculation.
> >> + */
> >> static unsigned long dsi_adjust_pclk_for_compression(const struct
> >> drm_display_mode *mode,
> >>const struct drm_dsc_config *dsc)
> >> {
> >> @@ -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
> >
> > Should this be pixels (1 pixel), not bytes, just like in the compressed
> > scenario?
>
> No.
So if the uncomrpessed BPP changes, pclk _also changes_?
- Marijn
> >> + * 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.
> >> + *
> >> + * 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.
> >
> > Okay, so one can not send more than 6 pixels even if the bpp is
Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
On 20/06/2023 15:05, Marijn Suijten wrote:
On 2023-06-20 00:06:47, 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
---
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
The
+ * @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
And in v1 you explained that it is _not_ about bpp...
Hmm, this bit stayed intact, so I'm slightly confused here.
This function is about BPP and compressed rate. dsi_timing_setup() is
about bytes.
+ *
+ * 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
I feel like "For VIDEO mode" and "For CMD mode" reads more naturally, no
need for "the", but I don't know the grammar rule that states so.
Ack
+ * 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.
Nit: well, refresh rate is already part of this calculation (that's how
drm_display_mode's clock member comes to be, and how drm_mode_vrefresh()
figures out fps after the fact). It's all about the per-CMD transfer
overhead in bytes that is currently not part of the calculation.
Please correct me if I'm wrong, we start from mode->clock. Refresh rate
isn't even a part of struct drm_display_mode.
+ */
static unsigned long dsi_adjust_pclk_for_compression(const struct
drm_display_mode *mode,
const struct drm_dsc_config *dsc)
{
@@ -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
Should this be pixels (1 pixel), not bytes, just like in the compressed
scenario?
No.
+* 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.
+*
+* 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.
Okay, so one can not send more than 6 pixels even if the bpp is less
than 8, is what this comes down to.
Yes.
+*
+* 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().
Nit: note that this is only for VIDEO mode, h_total and ha_end are
accurately unused in the CMDmode path below.
- Marijn
+*
+* 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);
--
2.39.2
--
With best wishes
Dmitry
Re: [PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
On 2023-06-20 00:06:47, 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
> ---
>
> 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
The
> + * @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
And in v1 you explained that it is _not_ about 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
I feel like "For VIDEO mode" and "For CMD mode" reads more naturally, no
need for "the", but I don't know the grammar rule that states so.
> + * 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.
Nit: well, refresh rate is already part of this calculation (that's how
drm_display_mode's clock member comes to be, and how drm_mode_vrefresh()
figures out fps after the fact). It's all about the per-CMD transfer
overhead in bytes that is currently not part of the calculation.
> + */
> static unsigned long dsi_adjust_pclk_for_compression(const struct
> drm_display_mode *mode,
> const struct drm_dsc_config *dsc)
> {
> @@ -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
Should this be pixels (1 pixel), not bytes, just like in the compressed
scenario?
> + * 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.
> + *
> + * 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.
Okay, so one can not send more than 6 pixels even if the bpp is less
than 8, is what this comes down to.
> + *
> + * 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().
Nit: note that this is only for VIDEO mode, h_total and ha_end are
accurately unused in the CMDmode path below.
- Marijn
> + *
> + * 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);
> --
> 2.39.2
>
[PATCH v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations
Provide actual documentation for the pclk and hdisplay calculations in
the case of DSC compression being used.
Signed-off-by: Dmitry Baryshkov
---
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)
{
@@ -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.
+*
+* 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().
+*
+* 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);
--
2.39.2
