| Message ID | 20190903091512.15780-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/mcde: Some fixes to handling video mode | expand |
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.
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 --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;
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(-)