On Mon, Sep 02, 2019 at 09:17:14AM +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 <step...@gerhold.net>
> Fixes: 5fc537bfd000 ("drm/mcde: Add new driver for ST-Ericsson MCDE")
> Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
> ---
> drivers/gpu/drm/mcde/mcde_dsi.c | 60 ++++++++++++++++++++++-----------
> 1 file changed, 41 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c
> index 90659d190d78..f5079f0e24ca 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;
> @@ -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 */
> @@ -475,7 +492,7 @@ static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
> writel(val, d->regs + DSI_VID_HSIZE1);
>
> /* RGB data length (bytes on one scanline) */
> - val = mode->hdisplay * (bpp / 8);
> + val = mode->hdisplay * cpp;
> writel(val, d->regs + DSI_VID_HSIZE2);
>
> /* TODO: further adjustments for TVG mode here */
> @@ -507,7 +524,7 @@ 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
> @@ -517,17 +534,18 @@ static void mcde_dsi_setup_video_mode(struct mcde_dsi
> *d,
> 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,
> + d->regs + DSI_VID_VCA_SETTING2);
No functional difference, but it might be more clear to also shift this
by DSI_VID_VCA_SETTING2_EXACT_BURST_LIMIT_SHIFT.
>
> writel(blkeol_duration, d->regs + DSI_VID_PCK_TIME);
> writel(blkeol_duration - 6, d->regs + DSI_VID_VCA_SETTING1);
> @@ -535,9 +553,11 @@ static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
>
> /* 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;
This should be DSI_VID_VCA_SETTING2_MAX_LINE_LIMIT_SHIFT
to write the maximum line limit. Otherwise it will just
replace the "exact burst limit" written earlier.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
