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

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.