diff mbox series

[v2] drm/mcde: Some fixes to handling video mode

Message ID 20190903091512.15780-1-linus.walleij@linaro.org
State New
Headers show
Series [v2] drm/mcde: Some fixes to handling video mode | expand

Commit Message

Linus Walleij Sept. 3, 2019, 9:15 a.m. UTC
The video DSI mode had not really been tested. These fixes makes
it more likely to work on real hardware:
- Set the HS clock to something the video mode reported by the
  panel can handle rather than the max HS rate.
- Put the active width (x width) in the right bits and the VSA
  (vertical sync active) in the right bits (those were swapped).
- Calculate the packet sizes in bytes as in the vendor driver,
  rather than in bits.
- Handle negative result in front/back/sync packages and fall
  back to zero like in the vendor driver.

Cc: Stephan Gerhold <stephan@gerhold.net>
Fixes: 5fc537bfd000 ("drm/mcde: Add new driver for ST-Ericsson MCDE")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Fix some more comments so we understand what is going on.
- Set up the maximum line limit size in the right register
  instead of setting it in the burst size register portion.
- Set some default wakeup time other than zero (still need
  fixing more).
---
 drivers/gpu/drm/mcde/mcde_dsi.c | 75 ++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 25 deletions(-)

Comments

Stephan Gerhold Sept. 3, 2019, 2:37 p.m. UTC | #1
On Tue, Sep 03, 2019 at 11:15:12AM +0200, Linus Walleij wrote:
> The video DSI mode had not really been tested. These fixes makes
> it more likely to work on real hardware:
> - Set the HS clock to something the video mode reported by the
>   panel can handle rather than the max HS rate.
> - Put the active width (x width) in the right bits and the VSA
>   (vertical sync active) in the right bits (those were swapped).
> - Calculate the packet sizes in bytes as in the vendor driver,
>   rather than in bits.
> - Handle negative result in front/back/sync packages and fall
>   back to zero like in the vendor driver.
> 
> Cc: Stephan Gerhold <stephan@gerhold.net>
> Fixes: 5fc537bfd000 ("drm/mcde: Add new driver for ST-Ericsson MCDE")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Fix some more comments so we understand what is going on.
> - Set up the maximum line limit size in the right register
>   instead of setting it in the burst size register portion.
> - Set some default wakeup time other than zero (still need
>   fixing more).
> ---
>  drivers/gpu/drm/mcde/mcde_dsi.c | 75 ++++++++++++++++++++++-----------
>  1 file changed, 50 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c
> index cd261c266f35..5c65cd70fcd3 100644
> --- a/drivers/gpu/drm/mcde/mcde_dsi.c
> +++ b/drivers/gpu/drm/mcde/mcde_dsi.c
> @@ -365,11 +365,12 @@ void mcde_dsi_te_request(struct mipi_dsi_device *mdsi)
>  static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
>  				      const struct drm_display_mode *mode)
>  {
> -	u8 bpp = mipi_dsi_pixel_format_to_bpp(d->mdsi->format);
> +	/* cpp, characters per pixel, number of bytes per pixel */
> +	u8 cpp = mipi_dsi_pixel_format_to_bpp(d->mdsi->format) / 8;
>  	u64 bpl;
> -	u32 hfp;
> -	u32 hbp;
> -	u32 hsa;
> +	int hfp;
> +	int hbp;
> +	int hsa;
>  	u32 blkline_pck, line_duration;
>  	u32 blkeol_pck, blkeol_duration;
>  	u32 val;
> @@ -408,11 +409,11 @@ static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
>  		return;
>  	}
>  
> -	/* TODO: TVG could be enabled here */
> +	/* TODO: TVG (test video generator) could be enabled here */
>  
> -	/* Send blanking packet */
> +	/* During blanking: go to LP mode */
>  	val |= DSI_VID_MAIN_CTL_REG_BLKLINE_MODE_LP_0;
> -	/* Send EOL packet */
> +	/* During EOL: go to LP mode */
>  	val |= DSI_VID_MAIN_CTL_REG_BLKEOL_MODE_LP_0;
>  	/* Recovery mode 1 */
>  	val |= 1 << DSI_VID_MAIN_CTL_RECOVERY_MODE_SHIFT;
> @@ -420,13 +421,13 @@ static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
>  	writel(val, d->regs + DSI_VID_MAIN_CTL);
>  
>  	/* Vertical frame parameters are pretty straight-forward */
> -	val = mode->vdisplay << DSI_VID_VSIZE_VSA_LENGTH_SHIFT;
> +	val = mode->vdisplay << DSI_VID_VSIZE_VACT_LENGTH_SHIFT;
>  	/* vertical front porch */
>  	val |= (mode->vsync_start - mode->vdisplay)
>  		<< DSI_VID_VSIZE_VFP_LENGTH_SHIFT;
>  	/* vertical sync active */
>  	val |= (mode->vsync_end - mode->vsync_start)
> -		<< DSI_VID_VSIZE_VACT_LENGTH_SHIFT;
> +		<< DSI_VID_VSIZE_VSA_LENGTH_SHIFT;
>  	/* vertical back porch */
>  	val |= (mode->vtotal - mode->vsync_end)
>  		<< DSI_VID_VSIZE_VBP_LENGTH_SHIFT;
> @@ -437,21 +438,25 @@ static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
>  	 * horizontal resolution is given in pixels and must be re-calculated
>  	 * into bytes since this is what the hardware expects.
>  	 *
> +	 * hfp = horizontal front porch in bytes
> +	 * hbp = horizontal back porch in bytes
> +	 * hsa = horizontal sync active in bytes
> +	 *
>  	 * 6 + 2 is HFP header + checksum
>  	 */
> -	hfp = (mode->hsync_start - mode->hdisplay) * bpp - 6 - 2;
> +	hfp = (mode->hsync_start - mode->hdisplay) * cpp - 6 - 2;
>  	if (d->mdsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) {
>  		/*
>  		 * 6 is HBP header + checksum
>  		 * 4 is RGB header + checksum
>  		 */
> -		hbp = (mode->htotal - mode->hsync_end) * bpp - 4 - 6;
> +		hbp = (mode->htotal - mode->hsync_end) * cpp - 4 - 6;
>  		/*
>  		 * 6 is HBP header + checksum
>  		 * 4 is HSW packet bytes
>  		 * 4 is RGB header + checksum
>  		 */
> -		hsa = (mode->hsync_end - mode->hsync_start) * bpp - 4 - 4 - 6;
> +		hsa = (mode->hsync_end - mode->hsync_start) * cpp - 4 - 4 - 6;
>  	} else {
>  		/*
>  		 * HBP includes both back porch and sync
> @@ -459,11 +464,23 @@ static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
>  		 * 4 is HSW packet bytes
>  		 * 4 is RGB header + checksum
>  		 */
> -		hbp = (mode->htotal - mode->hsync_start) * bpp - 4 - 4 - 6;
> -		/* HSA is not considered in this mode and set to 0 */
> +		hbp = (mode->htotal - mode->hsync_start) * cpp - 4 - 4 - 6;
> +		/* HSA is not present in this mode and set to 0 */
> +		hsa = 0;
> +	}
> +	if (hfp < 0) {
> +		dev_info(d->dev, "hfp negative, set to 0\n");
> +		hfp = 0;
> +	}
> +	if (hbp < 0) {
> +		dev_info(d->dev, "hbp negative, set to 0\n");
> +		hbp = 0;
> +	}
> +	if (hsa < 0) {
> +		dev_info(d->dev, "hsa negative, set to 0\n");
>  		hsa = 0;
>  	}
> -	dev_dbg(d->dev, "hfp: %u, hbp: %u, hsa: %u\n",
> +	dev_dbg(d->dev, "hfp: %u, hbp: %u, hsa: %u bytes\n",
>  		hfp, hbp, hsa);
>  
>  	/* Frame parameters: horizontal sync active */
> @@ -474,8 +491,8 @@ static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
>  	val |= hfp << DSI_VID_HSIZE1_HFP_LENGTH_SHIFT;
>  	writel(val, d->regs + DSI_VID_HSIZE1);
>  
> -	/* RGB data length (bytes on one scanline) */
> -	val = mode->hdisplay * (bpp / 8);
> +	/* RGB data length (visible bytes on one scanline) */
> +	val = mode->hdisplay * cpp;
>  	writel(val, d->regs + DSI_VID_HSIZE2);
>  
>  	/* TODO: further adjustments for TVG mode here */
> @@ -507,37 +524,43 @@ static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
>  	}
>  
>  	line_duration = (blkline_pck + 6) / d->mdsi->lanes;
> -	dev_dbg(d->dev, "line duration %u\n", line_duration);
> +	dev_dbg(d->dev, "line duration %u bytes\n", line_duration);
>  	val = line_duration << DSI_VID_DPHY_TIME_REG_LINE_DURATION_SHIFT;
>  	/*
>  	 * This is the time to perform LP->HS on D-PHY
>  	 * FIXME: nowhere to get this from: DT property on the DSI?
> +	 * values like 48 and 72 seen in the vendor code.
>  	 */
> -	val |= 0 << DSI_VID_DPHY_TIME_REG_WAKEUP_TIME_SHIFT;
> +	val |= 48 << DSI_VID_DPHY_TIME_REG_WAKEUP_TIME_SHIFT;
>  	writel(val, d->regs + DSI_VID_DPHY_TIME);

I just want to note that the Samsung S3 Mini (golden) seems to use 88
here. I suppose we will need to see how important this property is,
or if panels can also work with a slightly wrong value.

>  
>  	/* Calculate block end of line */
> -	blkeol_pck = bpl - mode->hdisplay * bpp - 6;
> +	blkeol_pck = bpl - mode->hdisplay * cpp - 6;
>  	blkeol_duration = (blkeol_pck + 6) / d->mdsi->lanes;
> -	dev_dbg(d->dev, "blkeol pck: %u, duration: %u\n",
> -		 blkeol_pck, blkeol_duration);
> +	dev_dbg(d->dev, "blkeol pck: %u bytes, duration: %u bytes\n",
> +		blkeol_pck, blkeol_duration);
>  
>  	if (d->mdsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
>  		/* Set up EOL clock for burst mode */
>  		val = readl(d->regs + DSI_VID_BLKSIZE1);
>  		val |= blkeol_pck << DSI_VID_BLKSIZE1_BLKEOL_PCK_SHIFT;
>  		writel(val, d->regs + DSI_VID_BLKSIZE1);
> -		writel(blkeol_pck, d->regs + DSI_VID_VCA_SETTING2);
> +		writel((blkeol_pck & DSI_VID_VCA_SETTING2_EXACT_BURST_LIMIT_MASK)
> +		       << DSI_VID_VCA_SETTING2_EXACT_BURST_LIMIT_SHIFT,
> +		       d->regs + DSI_VID_VCA_SETTING2);

It does not make a difference in this case because SHIFT = 0,
but shouldn't you normally shift before applying the mask?
Otherwise, you would wipe out the lower bits before you shift them.
Linus Walleij Sept. 5, 2019, 12:16 p.m. UTC | #2
On Tue, Sep 3, 2019 at 4:38 PM Stephan Gerhold <stephan@gerhold.net> wrote:
> On Tue, Sep 03, 2019 at 11:15:12AM +0200, Linus Walleij wrote:

> >       /*
> >        * This is the time to perform LP->HS on D-PHY
> >        * FIXME: nowhere to get this from: DT property on the DSI?
> > +      * values like 48 and 72 seen in the vendor code.
> >        */
> > -     val |= 0 << DSI_VID_DPHY_TIME_REG_WAKEUP_TIME_SHIFT;
> > +     val |= 48 << DSI_VID_DPHY_TIME_REG_WAKEUP_TIME_SHIFT;
> >       writel(val, d->regs + DSI_VID_DPHY_TIME);
>
> I just want to note that the Samsung S3 Mini (golden) seems to use 88
> here. I suppose we will need to see how important this property is,
> or if panels can also work with a slightly wrong value.

Yeah maybe we should just figure out what to do about this.

This is the wakeup time, in clock cycles, for a LP->HS
transition on the D-PHY.

The panel driver kind of knows knows this timing, so I
guess we should add a field to struct mipi_dsi_device
such as unsigned int lp_to_hs_wakep, but it needs to
come from the device tree since per the manual this
value is system dependent:

"reg_wakeup_time must be shorter than line duration and
depends on the D-PHY cell plus some pipelines delays inserted
by the DSI link. This value strongly depends on the PLL
programming and as it is a mix of analog and digital timing, it
is practically impossible to provide a general formula. By the
way, it has to be characterized at system level (validation
and/or simulation)."

> > -             writel(blkeol_pck, d->regs + DSI_VID_VCA_SETTING2);
> > +             writel((blkeol_pck & DSI_VID_VCA_SETTING2_EXACT_BURST_LIMIT_MASK)
> > +                    << DSI_VID_VCA_SETTING2_EXACT_BURST_LIMIT_SHIFT,
> > +                    d->regs + DSI_VID_VCA_SETTING2);
>
> It does not make a difference in this case because SHIFT = 0,
> but shouldn't you normally shift before applying the mask?
> Otherwise, you would wipe out the lower bits before you shift them.

OK you're right, I fix it up and resend.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c
index cd261c266f35..5c65cd70fcd3 100644
--- a/drivers/gpu/drm/mcde/mcde_dsi.c
+++ b/drivers/gpu/drm/mcde/mcde_dsi.c
@@ -365,11 +365,12 @@  void mcde_dsi_te_request(struct mipi_dsi_device *mdsi)
 static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
 				      const struct drm_display_mode *mode)
 {
-	u8 bpp = mipi_dsi_pixel_format_to_bpp(d->mdsi->format);
+	/* cpp, characters per pixel, number of bytes per pixel */
+	u8 cpp = mipi_dsi_pixel_format_to_bpp(d->mdsi->format) / 8;
 	u64 bpl;
-	u32 hfp;
-	u32 hbp;
-	u32 hsa;
+	int hfp;
+	int hbp;
+	int hsa;
 	u32 blkline_pck, line_duration;
 	u32 blkeol_pck, blkeol_duration;
 	u32 val;
@@ -408,11 +409,11 @@  static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
 		return;
 	}
 
-	/* TODO: TVG could be enabled here */
+	/* TODO: TVG (test video generator) could be enabled here */
 
-	/* Send blanking packet */
+	/* During blanking: go to LP mode */
 	val |= DSI_VID_MAIN_CTL_REG_BLKLINE_MODE_LP_0;
-	/* Send EOL packet */
+	/* During EOL: go to LP mode */
 	val |= DSI_VID_MAIN_CTL_REG_BLKEOL_MODE_LP_0;
 	/* Recovery mode 1 */
 	val |= 1 << DSI_VID_MAIN_CTL_RECOVERY_MODE_SHIFT;
@@ -420,13 +421,13 @@  static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
 	writel(val, d->regs + DSI_VID_MAIN_CTL);
 
 	/* Vertical frame parameters are pretty straight-forward */
-	val = mode->vdisplay << DSI_VID_VSIZE_VSA_LENGTH_SHIFT;
+	val = mode->vdisplay << DSI_VID_VSIZE_VACT_LENGTH_SHIFT;
 	/* vertical front porch */
 	val |= (mode->vsync_start - mode->vdisplay)
 		<< DSI_VID_VSIZE_VFP_LENGTH_SHIFT;
 	/* vertical sync active */
 	val |= (mode->vsync_end - mode->vsync_start)
-		<< DSI_VID_VSIZE_VACT_LENGTH_SHIFT;
+		<< DSI_VID_VSIZE_VSA_LENGTH_SHIFT;
 	/* vertical back porch */
 	val |= (mode->vtotal - mode->vsync_end)
 		<< DSI_VID_VSIZE_VBP_LENGTH_SHIFT;
@@ -437,21 +438,25 @@  static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
 	 * horizontal resolution is given in pixels and must be re-calculated
 	 * into bytes since this is what the hardware expects.
 	 *
+	 * hfp = horizontal front porch in bytes
+	 * hbp = horizontal back porch in bytes
+	 * hsa = horizontal sync active in bytes
+	 *
 	 * 6 + 2 is HFP header + checksum
 	 */
-	hfp = (mode->hsync_start - mode->hdisplay) * bpp - 6 - 2;
+	hfp = (mode->hsync_start - mode->hdisplay) * cpp - 6 - 2;
 	if (d->mdsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) {
 		/*
 		 * 6 is HBP header + checksum
 		 * 4 is RGB header + checksum
 		 */
-		hbp = (mode->htotal - mode->hsync_end) * bpp - 4 - 6;
+		hbp = (mode->htotal - mode->hsync_end) * cpp - 4 - 6;
 		/*
 		 * 6 is HBP header + checksum
 		 * 4 is HSW packet bytes
 		 * 4 is RGB header + checksum
 		 */
-		hsa = (mode->hsync_end - mode->hsync_start) * bpp - 4 - 4 - 6;
+		hsa = (mode->hsync_end - mode->hsync_start) * cpp - 4 - 4 - 6;
 	} else {
 		/*
 		 * HBP includes both back porch and sync
@@ -459,11 +464,23 @@  static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
 		 * 4 is HSW packet bytes
 		 * 4 is RGB header + checksum
 		 */
-		hbp = (mode->htotal - mode->hsync_start) * bpp - 4 - 4 - 6;
-		/* HSA is not considered in this mode and set to 0 */
+		hbp = (mode->htotal - mode->hsync_start) * cpp - 4 - 4 - 6;
+		/* HSA is not present in this mode and set to 0 */
+		hsa = 0;
+	}
+	if (hfp < 0) {
+		dev_info(d->dev, "hfp negative, set to 0\n");
+		hfp = 0;
+	}
+	if (hbp < 0) {
+		dev_info(d->dev, "hbp negative, set to 0\n");
+		hbp = 0;
+	}
+	if (hsa < 0) {
+		dev_info(d->dev, "hsa negative, set to 0\n");
 		hsa = 0;
 	}
-	dev_dbg(d->dev, "hfp: %u, hbp: %u, hsa: %u\n",
+	dev_dbg(d->dev, "hfp: %u, hbp: %u, hsa: %u bytes\n",
 		hfp, hbp, hsa);
 
 	/* Frame parameters: horizontal sync active */
@@ -474,8 +491,8 @@  static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
 	val |= hfp << DSI_VID_HSIZE1_HFP_LENGTH_SHIFT;
 	writel(val, d->regs + DSI_VID_HSIZE1);
 
-	/* RGB data length (bytes on one scanline) */
-	val = mode->hdisplay * (bpp / 8);
+	/* RGB data length (visible bytes on one scanline) */
+	val = mode->hdisplay * cpp;
 	writel(val, d->regs + DSI_VID_HSIZE2);
 
 	/* TODO: further adjustments for TVG mode here */
@@ -507,37 +524,43 @@  static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
 	}
 
 	line_duration = (blkline_pck + 6) / d->mdsi->lanes;
-	dev_dbg(d->dev, "line duration %u\n", line_duration);
+	dev_dbg(d->dev, "line duration %u bytes\n", line_duration);
 	val = line_duration << DSI_VID_DPHY_TIME_REG_LINE_DURATION_SHIFT;
 	/*
 	 * This is the time to perform LP->HS on D-PHY
 	 * FIXME: nowhere to get this from: DT property on the DSI?
+	 * values like 48 and 72 seen in the vendor code.
 	 */
-	val |= 0 << DSI_VID_DPHY_TIME_REG_WAKEUP_TIME_SHIFT;
+	val |= 48 << DSI_VID_DPHY_TIME_REG_WAKEUP_TIME_SHIFT;
 	writel(val, d->regs + DSI_VID_DPHY_TIME);
 
 	/* Calculate block end of line */
-	blkeol_pck = bpl - mode->hdisplay * bpp - 6;
+	blkeol_pck = bpl - mode->hdisplay * cpp - 6;
 	blkeol_duration = (blkeol_pck + 6) / d->mdsi->lanes;
-	dev_dbg(d->dev, "blkeol pck: %u, duration: %u\n",
-		 blkeol_pck, blkeol_duration);
+	dev_dbg(d->dev, "blkeol pck: %u bytes, duration: %u bytes\n",
+		blkeol_pck, blkeol_duration);
 
 	if (d->mdsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
 		/* Set up EOL clock for burst mode */
 		val = readl(d->regs + DSI_VID_BLKSIZE1);
 		val |= blkeol_pck << DSI_VID_BLKSIZE1_BLKEOL_PCK_SHIFT;
 		writel(val, d->regs + DSI_VID_BLKSIZE1);
-		writel(blkeol_pck, d->regs + DSI_VID_VCA_SETTING2);
+		writel((blkeol_pck & DSI_VID_VCA_SETTING2_EXACT_BURST_LIMIT_MASK)
+		       << DSI_VID_VCA_SETTING2_EXACT_BURST_LIMIT_SHIFT,
+		       d->regs + DSI_VID_VCA_SETTING2);
 
 		writel(blkeol_duration, d->regs + DSI_VID_PCK_TIME);
+		/* Max burst limit */
 		writel(blkeol_duration - 6, d->regs + DSI_VID_VCA_SETTING1);
 	}
 
 	/* Maximum line limit */
 	val = readl(d->regs + DSI_VID_VCA_SETTING2);
+	val &= ~DSI_VID_VCA_SETTING2_MAX_LINE_LIMIT_MASK;
 	val |= blkline_pck <<
-		DSI_VID_VCA_SETTING2_EXACT_BURST_LIMIT_SHIFT;
+		DSI_VID_VCA_SETTING2_MAX_LINE_LIMIT_SHIFT;
 	writel(val, d->regs + DSI_VID_VCA_SETTING2);
+	dev_dbg(d->dev, "blkline pck: %u bytes\n", blkline_pck);
 
 	/* Put IF1 into video mode */
 	val = readl(d->regs + DSI_MCTL_MAIN_DATA_CTL);
@@ -699,7 +722,9 @@  static void mcde_dsi_bridge_mode_set(struct drm_bridge *bridge,
 		lp_freq = d->mdsi->lp_rate;
 	else
 		lp_freq = DSI_DEFAULT_LP_FREQ_HZ;
-	if (d->mdsi->hs_rate)
+	if (pixel_clock_hz)
+		hs_freq = pixel_clock_hz;
+	else if (d->mdsi->hs_rate)
 		hs_freq = d->mdsi->hs_rate;
 	else
 		hs_freq = DSI_DEFAULT_HS_FREQ_HZ;