diff mbox series

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

Message ID 20230423121232.1345909-2-aford173@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm: bridge: samsung-dsim: Support variable clocking | expand

Commit Message

Adam Ford April 23, 2023, 12:12 p.m. UTC 
From: Lucas Stach <l.stach@pengutronix.de>

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 <l.stach@pengutronix.de>
Signed-off-by: Adam Ford <aford173@gmail.com>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Comments

Jagan Teki April 24, 2023, 9:03 a.m. UTC | #1 
On Sun, Apr 23, 2023 at 5:42 PM Adam Ford <aford173@gmail.com> wrote:
>
> From: Lucas Stach <l.stach@pengutronix.de>
>
> 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 <l.stach@pengutronix.de>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> ---
>  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.
Adam Ford April 24, 2023, 9:47 a.m. UTC | #2 
On Mon, Apr 24, 2023 at 4:03 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Sun, Apr 23, 2023 at 5:42 PM Adam Ford <aford173@gmail.com> wrote:
> >
> > From: Lucas Stach <l.stach@pengutronix.de>
> >
> > 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 <l.stach@pengutronix.de>
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> >  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.
Jagan Teki May 3, 2023, 4:35 p.m. UTC | #3 
On Mon, Apr 24, 2023 at 3:17 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Apr 24, 2023 at 4:03 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Sun, Apr 23, 2023 at 5:42 PM Adam Ford <aford173@gmail.com> wrote:
> > >
> > > From: Lucas Stach <l.stach@pengutronix.de>
> > >
> > > 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 <l.stach@pengutronix.de>
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > ---
> > >  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.
Lucas Stach May 4, 2023, 7:14 a.m. UTC | #4 
Am Mittwoch, dem 03.05.2023 um 22:05 +0530 schrieb Jagan Teki:
> On Mon, Apr 24, 2023 at 3:17 PM Adam Ford <aford173@gmail.com> wrote:
> > 
> > On Mon, Apr 24, 2023 at 4:03 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > 
> > > On Sun, Apr 23, 2023 at 5:42 PM Adam Ford <aford173@gmail.com> wrote:
> > > > 
> > > > From: Lucas Stach <l.stach@pengutronix.de>
> > > > 
> > > > 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 <l.stach@pengutronix.de>
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > ---
> > > >  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
diff mbox series

Patch

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) |