Re: [PATCH V2 1/6] drm: bridge: samsung-dsim: fix blanking packet size calculation

[Lucas Stach]

Am Mittwoch, dem 03.05.2023 um 22:05 +0530 schrieb Jagan Teki:
> On Mon, Apr 24, 2023 at 3:17 PM Adam Ford  wrote:
> > 
> > On Mon, Apr 24, 2023 at 4:03 AM Jagan Teki  
> > wrote:
> > > 
> > > On Sun, Apr 23, 2023 at 5:42 PM Adam Ford  wrote:
> > > > 
> > > > From: Lucas Stach 
> > > > 
> > > > Scale the blanking packet sizes to match the ratio between HS clock
> > > > and DPI interface clock. The controller seems to do internal scaling
> > > > to the number of active lanes, so we don't take those into account.
> > > > 
> > > > Signed-off-by: Lucas Stach 
> > > > Signed-off-by: Adam Ford 
> > > > ---
> > > >  drivers/gpu/drm/bridge/samsung-dsim.c | 18 +++---
> > > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
> > > > b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > index e0a402a85787..2be3b58624c3 100644
> > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > @@ -874,17 +874,29 @@ static void samsung_dsim_set_display_mode(struct 
> > > > samsung_dsim *dsi)
> > > > u32 reg;
> > > > 
> > > > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> > > > +   int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
> > > > +   int hfp = (m->hsync_start - m->hdisplay) * byte_clk_khz 
> > > > / m->clock;
> > > 
> > > I do not quite understand why it depends on burst_clk_rate, would you
> > > please explain? does it depends on bpp something like this
> > > 
> > > mipi_dsi_pixel_format_to_bpp(format) / 8
> > 
> > The pixel clock is currently set to the burst clock rate.  Dividing
> > the clock by 1000 gets the pixel clock in KHz, and dividing by 8
> > converts bits to bytes.
> > Later in the series, I change the clock from the burst clock to the
> > cached value returned from samsung_dsim_set_pll.
> 
> Okay.
> 
> > 
> > > 
> > > > +   int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / 
> > > > m->clock;
> > > > +   int hsa = (m->hsync_end - m->hsync_start) * 
> > > > byte_clk_khz / m->clock;
> > > > +
> > > > +   /* remove packet overhead when possible */
> > > > +   hfp = max(hfp - 6, 0);
> > > > +   hbp = max(hbp - 6, 0);
> > > > +   hsa = max(hsa - 6, 0);
> > > 
> > > 6 blanking packet overhead here means, 4 bytes + payload + 2 bytes
> > > format? does this packet overhead depends on the respective porch's
> > > like hpf, hbp and hsa has different packet overheads?
> > 
> > Lucas might be able to explain this better.  However, it does match
> > the values of the downstream NXP kernel, and I tried playing with
> > these values manually, and 6 appeared to be the only number that
> > seemed to work for me too.  I abandoned my approach for Lucas'
> > implementation, because it seemed more clear than mine.
> > Maybe Lucas can chime in, since this is really his patch.
> 
> Lucan, any inputs?
> 
The blanking packets are are MIPI long packets, so 4 byte header,
payload, 2 bytes footer. I experimented with different blanking sizes
and haven't found that it would make any difference. I don't think any
real-world horizontal blanking sizes would require multiple packets to
be sent.

Regards,
Lucas

Re: [PATCH V2 1/6] drm: bridge: samsung-dsim: fix blanking packet size calculation

[Jagan Teki]

On Mon, Apr 24, 2023 at 3:17 PM Adam Ford  wrote:
>
> On Mon, Apr 24, 2023 at 4:03 AM Jagan Teki  wrote:
> >
> > On Sun, Apr 23, 2023 at 5:42 PM Adam Ford  wrote:
> > >
> > > From: Lucas Stach 
> > >
> > > Scale the blanking packet sizes to match the ratio between HS clock
> > > and DPI interface clock. The controller seems to do internal scaling
> > > to the number of active lanes, so we don't take those into account.
> > >
> > > Signed-off-by: Lucas Stach 
> > > Signed-off-by: Adam Ford 
> > > ---
> > >  drivers/gpu/drm/bridge/samsung-dsim.c | 18 +++---
> > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
> > > b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > index e0a402a85787..2be3b58624c3 100644
> > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > @@ -874,17 +874,29 @@ static void samsung_dsim_set_display_mode(struct 
> > > samsung_dsim *dsi)
> > > u32 reg;
> > >
> > > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> > > +   int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
> > > +   int hfp = (m->hsync_start - m->hdisplay) * byte_clk_khz / 
> > > m->clock;
> >
> > I do not quite understand why it depends on burst_clk_rate, would you
> > please explain? does it depends on bpp something like this
> >
> > mipi_dsi_pixel_format_to_bpp(format) / 8
>
> The pixel clock is currently set to the burst clock rate.  Dividing
> the clock by 1000 gets the pixel clock in KHz, and dividing by 8
> converts bits to bytes.
> Later in the series, I change the clock from the burst clock to the
> cached value returned from samsung_dsim_set_pll.

Okay.

>
> >
> > > +   int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / 
> > > m->clock;
> > > +   int hsa = (m->hsync_end - m->hsync_start) * byte_clk_khz 
> > > / m->clock;
> > > +
> > > +   /* remove packet overhead when possible */
> > > +   hfp = max(hfp - 6, 0);
> > > +   hbp = max(hbp - 6, 0);
> > > +   hsa = max(hsa - 6, 0);
> >
> > 6 blanking packet overhead here means, 4 bytes + payload + 2 bytes
> > format? does this packet overhead depends on the respective porch's
> > like hpf, hbp and hsa has different packet overheads?
>
> Lucas might be able to explain this better.  However, it does match
> the values of the downstream NXP kernel, and I tried playing with
> these values manually, and 6 appeared to be the only number that
> seemed to work for me too.  I abandoned my approach for Lucas'
> implementation, because it seemed more clear than mine.
> Maybe Lucas can chime in, since this is really his patch.

Lucan, any inputs?

Jagan.

Re: [PATCH V2 1/6] drm: bridge: samsung-dsim: fix blanking packet size calculation

[Adam Ford]

On Mon, Apr 24, 2023 at 4:03 AM Jagan Teki  wrote:
>
> On Sun, Apr 23, 2023 at 5:42 PM Adam Ford  wrote:
> >
> > From: Lucas Stach 
> >
> > Scale the blanking packet sizes to match the ratio between HS clock
> > and DPI interface clock. The controller seems to do internal scaling
> > to the number of active lanes, so we don't take those into account.
> >
> > Signed-off-by: Lucas Stach 
> > Signed-off-by: Adam Ford 
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 18 +++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
> > b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index e0a402a85787..2be3b58624c3 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -874,17 +874,29 @@ static void samsung_dsim_set_display_mode(struct 
> > samsung_dsim *dsi)
> > u32 reg;
> >
> > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> > +   int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
> > +   int hfp = (m->hsync_start - m->hdisplay) * byte_clk_khz / 
> > m->clock;
>
> I do not quite understand why it depends on burst_clk_rate, would you
> please explain? does it depends on bpp something like this
>
> mipi_dsi_pixel_format_to_bpp(format) / 8

The pixel clock is currently set to the burst clock rate.  Dividing
the clock by 1000 gets the pixel clock in KHz, and dividing by 8
converts bits to bytes.
Later in the series, I change the clock from the burst clock to the
cached value returned from samsung_dsim_set_pll.

>
> > +   int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / 
> > m->clock;
> > +   int hsa = (m->hsync_end - m->hsync_start) * byte_clk_khz / 
> > m->clock;
> > +
> > +   /* remove packet overhead when possible */
> > +   hfp = max(hfp - 6, 0);
> > +   hbp = max(hbp - 6, 0);
> > +   hsa = max(hsa - 6, 0);
>
> 6 blanking packet overhead here means, 4 bytes + payload + 2 bytes
> format? does this packet overhead depends on the respective porch's
> like hpf, hbp and hsa has different packet overheads?

Lucas might be able to explain this better.  However, it does match
the values of the downstream NXP kernel, and I tried playing with
these values manually, and 6 appeared to be the only number that
seemed to work for me too.  I abandoned my approach for Lucas'
implementation, because it seemed more clear than mine.
Maybe Lucas can chime in, since this is really his patch.

adam
>
> Jagan.

Re: [PATCH V2 1/6] drm: bridge: samsung-dsim: fix blanking packet size calculation

[Jagan Teki]

On Sun, Apr 23, 2023 at 5:42 PM Adam Ford  wrote:
>
> From: Lucas Stach 
>
> Scale the blanking packet sizes to match the ratio between HS clock
> and DPI interface clock. The controller seems to do internal scaling
> to the number of active lanes, so we don't take those into account.
>
> Signed-off-by: Lucas Stach 
> Signed-off-by: Adam Ford 
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 18 +++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index e0a402a85787..2be3b58624c3 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -874,17 +874,29 @@ static void samsung_dsim_set_display_mode(struct 
> samsung_dsim *dsi)
> u32 reg;
>
> if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
> +   int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
> +   int hfp = (m->hsync_start - m->hdisplay) * byte_clk_khz / 
> m->clock;

I do not quite understand why it depends on burst_clk_rate, would you
please explain? does it depends on bpp something like this

mipi_dsi_pixel_format_to_bpp(format) / 8

> +   int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / 
> m->clock;
> +   int hsa = (m->hsync_end - m->hsync_start) * byte_clk_khz / 
> m->clock;
> +
> +   /* remove packet overhead when possible */
> +   hfp = max(hfp - 6, 0);
> +   hbp = max(hbp - 6, 0);
> +   hsa = max(hsa - 6, 0);

6 blanking packet overhead here means, 4 bytes + payload + 2 bytes
format? does this packet overhead depends on the respective porch's
like hpf, hbp and hsa has different packet overheads?

Jagan.

[PATCH V2 1/6] drm: bridge: samsung-dsim: fix blanking packet size calculation

[Adam Ford]

From: Lucas Stach 

Scale the blanking packet sizes to match the ratio between HS clock
and DPI interface clock. The controller seems to do internal scaling
to the number of active lanes, so we don't take those into account.

Signed-off-by: Lucas Stach 
Signed-off-by: Adam Ford 
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 18 +++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
b/drivers/gpu/drm/bridge/samsung-dsim.c
index e0a402a85787..2be3b58624c3 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -874,17 +874,29 @@ static void samsung_dsim_set_display_mode(struct 
samsung_dsim *dsi)
u32 reg;
 
if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
+   int byte_clk_khz = dsi->burst_clk_rate / 1000 / 8;
+   int hfp = (m->hsync_start - m->hdisplay) * byte_clk_khz / 
m->clock;
+   int hbp = (m->htotal - m->hsync_end) * byte_clk_khz / m->clock;
+   int hsa = (m->hsync_end - m->hsync_start) * byte_clk_khz / 
m->clock;
+
+   /* remove packet overhead when possible */
+   hfp = max(hfp - 6, 0);
+   hbp = max(hbp - 6, 0);
+   hsa = max(hsa - 6, 0);
+
+   dev_dbg(dsi->dev, "calculated hfp: %u, hbp: %u, hsa: %u",
+   hfp, hbp, hsa);
+
reg = DSIM_CMD_ALLOW(0xf)
| DSIM_STABLE_VFP(m->vsync_start - m->vdisplay)
| DSIM_MAIN_VBP(m->vtotal - m->vsync_end);
samsung_dsim_write(dsi, DSIM_MVPORCH_REG, reg);
 
-   reg = DSIM_MAIN_HFP(m->hsync_start - m->hdisplay)
-   | DSIM_MAIN_HBP(m->htotal - m->hsync_end);
+   reg = DSIM_MAIN_HFP(hfp) | DSIM_MAIN_HBP(hbp);
samsung_dsim_write(dsi, DSIM_MHPORCH_REG, reg);
 
reg = DSIM_MAIN_VSA(m->vsync_end - m->vsync_start)
-   | DSIM_MAIN_HSA(m->hsync_end - m->hsync_start);
+   | DSIM_MAIN_HSA(hsa);
samsung_dsim_write(dsi, DSIM_MSYNC_REG, reg);
}
reg =  DSIM_MAIN_HRESOL(m->hdisplay, num_bits_resol) |
-- 
2.39.2