Hi Linus, I feel kind of bad to keep requesting changes for this patch, but I found one last difference with the vendor kernel:
On Mon, Dec 16, 2019 at 10:06:47PM +0100, Linus Walleij wrote: > The video DSI mode had not really been tested. These fixes makes > it more likely to work on real hardware: > - 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. Test the calculations agains a > spreadsheet and confirmed by debug prints to be reasonable. > - Also verified the register values with relative confidence > to register dumps from the Samsung GT-I8190 boot loader > graphics. We are not identical but not off by far either. > - Error out if the current mode and refresh frequency doesn't > work out. (In the future we may simply want to scale down > the vrefresh.) > - Handle negative result in front/back/sync packages and fall > back to zero like in the vendor driver. > - Put in lots of clarifying comments and references to the > documentation where the code is hard to understand. > - Set the DSI_VID_VCA_SETTING2 field > DSI_VID_VCA_SETTING2_MAX_LINE_LIMIT to blkline_pck - 6 as in > the vendor driver and mask the field properly. > > 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> > --- > ChangeLog v4->v5: > - Restore log order, last verstion of the changelog inadvertedly > merged v3 and v4 into v3 since I thought I didn't send out v3... > - Parens in blkeol_pck = bpl - (mode->htotal * cpp) - 6 for > explicit priority. > - Use mask-and-set for DSI_VID_PCK_TIME and DSI_VID_VCA_SETTING1 > - Restore DSI_VID_VCA_SETTING2 field > DSI_VID_VCA_SETTING2_MAX_LINE_LIMIT to blkline_pck - 6 as in > the vendor driver and mask the field properly. > ChangeLog v3->v4: > - Calculate toward actual HS rate of the clock rather than the > idealized rate provided by the panel resolution, this is what > the vendor driver does. > - Add much comments and elaborate with references to the manual > so the code can be understood as far as possible. > - Compared register dumps to that on the Samsung GT-I8190 (Golden) > boot loader settings. We are now reasonably close to these, > it may be that the boot loader driver is using slightly different > settings for porches and syncs etc. But all figures makes sense. > - Duplicated the vendor code in a spread sheet and compared to > what this code gives and we have an identical match with one > small exception that the vendor code adds a small padding of 2 > lines to the vertical blanking area. This looks weird and might > be some hackishly specified porch. > ChangeLog v2->v3: > - Rename the "bpp" variable to "cpp" since it is "chars per pixel" > this was confusingly named in the vendor driver and it got > carried over. > - Assign the SETTING2_EXACT_BURST_LIMIT by first shifting > then masking. > - Also mask with the inverse of DSI_VID_BLKSIZE1_BLKEOL_PCK_MASK > before writing blkeol into DSI_VID_BLKSIZE1, so we make sure > to zero these bits first. > - Also mask with DSI_VID_BLKSIZE1_BLKLINE_EVENT_PCK_MASK > when writing event package length. > - Comb through the code and compare it to vendor code and try > to get closer to doing what the vendor driver is doing. > 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 | 238 +++++++++++++++++++++------ > drivers/gpu/drm/mcde/mcde_dsi_regs.h | 1 + > 2 files changed, 191 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c > index 21cee4d9d2fd..948841fc67df 100644 > --- a/drivers/gpu/drm/mcde/mcde_dsi.c > +++ b/drivers/gpu/drm/mcde/mcde_dsi.c > @@ -379,13 +379,14 @@ 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 pclk; > 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; > > val = 0; > @@ -422,11 +423,21 @@ 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 vertical blanking: go to LP mode > + * Like with the EOL setting, if this is not set, the EOL area will be > + * filled with NULL or blanking packets in the vblank area. > + * FIXME: some Samsung phones and display panels such as s6e63m0 use > + * DSI_VID_MAIN_CTL_REG_BLKLINE_MODE_BLANKING here instead, > + * figure out how to properly configure that from the panel. > + */ > val |= DSI_VID_MAIN_CTL_REG_BLKLINE_MODE_LP_0; > - /* Send EOL packet */ > + /* > + * During EOL: go to LP mode. If this is not set, the EOL area will be > + * filled with NULL or blanking packets. > + */ > val |= DSI_VID_MAIN_CTL_REG_BLKEOL_MODE_LP_0; > /* Recovery mode 1 */ > val |= 1 << DSI_VID_MAIN_CTL_RECOVERY_MODE_SHIFT; > @@ -434,13 +445,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; > @@ -448,36 +459,54 @@ static void mcde_dsi_setup_video_mode(struct mcde_dsi > *d, > > /* > * Horizontal frame parameters: > - * horizontal resolution is given in pixels and must be re-calculated > - * into bytes since this is what the hardware expects. > + * horizontal resolution is given in pixels but must be re-calculated > + * into bytes since this is what the hardware expects, these registers > + * define the payload size of the packet. > + * > + * 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) { > /* > + * Use sync pulse for sync: explicit HSA time > * 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 > + * Use event for sync: HBP includes both back porch and sync > * 6 is HBP header + checksum > * 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 */ > @@ -488,71 +517,184 @@ 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); > + dev_dbg(d->dev, "RGB length, visible area on a line: %u bytes\n", val); > > - /* TODO: further adjustments for TVG mode here */ > + /* > + * Calculate the time between two pixels in picoseconds using > + * the supplied refresh rate and total resolution including > + * porches and sync. > + */ > + /* (ps/s) / (pixels/s) = ps/pixels */ > + pclk = DIV_ROUND_UP_ULL(1000000000000, > + (mode->vrefresh * mode->htotal * mode->vtotal)); > + dev_dbg(d->dev, "picoseconds between two pixels: %llu\n", > + pclk); > > /* > - * EOL packet length from bits per line calculations: pixel clock > - * is given in kHz, calculate the time between two pixels in > - * picoseconds. > + * How many bytes per line will this update frequency yield? > + * > + * Calculate the number of picoseconds for one scanline (1), then > + * divide by 1000000000000 (2) to get in pixels per second we > + * want to output. > + * > + * Multiply with number of bytes per second at this video display > + * frequency (3) to get number of bytes transferred during this > + * time. Notice that we use the frequency the display wants, > + * not what we actually get from the DSI PLL, which is hs_freq. > + * > + * These arithmetics are done in a different order to avoid > + * overflow. > */ > - bpl = mode->clock * mode->htotal; > - bpl *= (d->hs_freq / 8); > - do_div(bpl, 1000000); /* microseconds */ > - do_div(bpl, 1000000); /* seconds */ > + bpl = pclk * mode->htotal; /* (1) picoseconds per line */ > + dev_dbg(d->dev, "picoseconds per line: %llu\n", bpl); > + /* Multiply with bytes per second (3) */ > + bpl *= (d->mdsi->hs_rate / 8); > + /* Pixels per second (2) */ > + bpl = DIV_ROUND_DOWN_ULL(bpl, 1000000); /* microseconds */ > + bpl = DIV_ROUND_DOWN_ULL(bpl, 1000000); /* seconds */ > + /* parallel transactions in all lanes */ > bpl *= d->mdsi->lanes; > - dev_dbg(d->dev, "calculated bytes per line: %llu\n", bpl); > + dev_dbg(d->dev, > + "calculated bytes per line: %llu @ %d Hz with HS %lu Hz\n", > + bpl, mode->vrefresh, d->mdsi->hs_rate); > + > /* > * 6 is header + checksum, header = 4 bytes, checksum = 2 bytes > * 4 is short packet for vsync/hsync > */ > if (d->mdsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) { > - /* Fixme: isn't the hsync width in pixels? */ > + /* Set the event packet size to 0 (not used) */ > + writel(0, d->regs + DSI_VID_BLKSIZE1); > + /* > + * FIXME: isn't the hsync width in pixels? The porch and > + * sync area size is in pixels here, but this -6 > + * seems to be for bytes. It looks like this in the vendor > + * code though. Is it completely untested? > + */ > blkline_pck = bpl - (mode->hsync_end - mode->hsync_start) - 6; > val = blkline_pck << DSI_VID_BLKSIZE2_BLKLINE_PULSE_PCK_SHIFT; > writel(val, d->regs + DSI_VID_BLKSIZE2); > } else { > + /* Set the sync pulse packet size to 0 (not used) */ > + writel(0, d->regs + DSI_VID_BLKSIZE2); > + /* Specifying payload size in bytes (-4-6 from manual) */ > blkline_pck = bpl - 4 - 6; > + if (blkline_pck > 0x1FFF) > + dev_err(d->dev, "blkline_pck too big %d bytes\n", > + blkline_pck); > val = blkline_pck << DSI_VID_BLKSIZE1_BLKLINE_EVENT_PCK_SHIFT; > + val &= DSI_VID_BLKSIZE1_BLKLINE_EVENT_PCK_MASK; > writel(val, d->regs + DSI_VID_BLKSIZE1); > } > > - line_duration = (blkline_pck + 6) / d->mdsi->lanes; > - dev_dbg(d->dev, "line duration %u\n", line_duration); > + /* > + * The line duration is used to scale back the frequency from > + * the max frequency supported by the HS clock to the desired > + * update frequency in vrefresh. > + */ > + line_duration = blkline_pck + 6; > + /* > + * The datasheet contains this complex condition to decreasing > + * the line duration by 1 under very specific circumstances. > + * Here we also imply that LP is used during burst EOL. > + */ > + if (d->mdsi->lanes == 2 && (hsa & 0x01) && (hfp & 0x01) > + && (d->mdsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)) > + line_duration--; > + line_duration = DIV_ROUND_CLOSEST(line_duration, d->mdsi->lanes); > + 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? > + * The manual says this is "system dependent". > + * 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_duration = (blkeol_pck + 6) / d->mdsi->lanes; > - dev_dbg(d->dev, "blkeol pck: %u, duration: %u\n", > - blkeol_pck, blkeol_duration); > - > + /* > + * See the manual figure 657 page 2203 for understanding the impact > + * of the different burst mode settings. > + */ > if (d->mdsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) { > - /* Set up EOL clock for burst mode */ > + int blkeol_pck, blkeol_duration; > + /* > + * Packet size at EOL for burst mode, this is only used > + * if DSI_VID_MAIN_CTL_REG_BLKEOL_MODE_LP_0 is NOT set, > + * but we instead send NULL or blanking packets at EOL. > + * This is given in number of bytes. > + * > + * See the manual page 2198 for the 13 reg_blkeol_pck bits. > + */ > + blkeol_pck = bpl - (mode->htotal * cpp) - 6; > + if (blkeol_pck < 0) { > + dev_err(d->dev, "video block does not fit on line!\n"); > + dev_err(d->dev, > + "calculated bytes per line: %llu @ %d Hz\n", > + bpl, mode->vrefresh); > + dev_err(d->dev, > + "bytes per line (blkline_pck) %u bytes\n", > + blkline_pck); > + dev_err(d->dev, > + "blkeol_pck becomes %d bytes\n", blkeol_pck); > + return; > + } > + dev_dbg(d->dev, "BLKEOL packet: %d bytes\n", blkeol_pck); > + > val = readl(d->regs + DSI_VID_BLKSIZE1); > + val &= ~DSI_VID_BLKSIZE1_BLKEOL_PCK_MASK; > 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_duration, d->regs + DSI_VID_PCK_TIME); > - writel(blkeol_duration - 6, d->regs + DSI_VID_VCA_SETTING1); > + /* Use the same value for exact burst limit */ > + val = blkeol_pck << > + DSI_VID_VCA_SETTING2_EXACT_BURST_LIMIT_SHIFT; > + val &= DSI_VID_VCA_SETTING2_EXACT_BURST_LIMIT_MASK; > + writel(val, d->regs + DSI_VID_VCA_SETTING2); > + /* > + * This BLKEOL duration is claimed to be the duration in clock > + * cycles of the BLLP end-of-line (EOL) period for each line if > + * DSI_VID_MAIN_CTL_REG_BLKEOL_MODE_LP_0 is set. > + * > + * It is hard to trust the manuals' claim that this is in clock > + * cycles as we mimic the behaviour of the vendor code, which > + * appears to write a number of bytes that would have been > + * transferred on a single lane. > + * > + * See the manual figure 657 page 2203 and page 2198 for the 13 > + * reg_blkeol_duration bits. > + * > + * FIXME: should this also be set up also for non-burst mode > + * according to figure 565 page 2202? > + */ > + blkeol_duration = DIV_ROUND_CLOSEST(blkeol_pck + 6, > + d->mdsi->lanes); > + dev_dbg(d->dev, "BLKEOL duration: %d clock cycles\n", > + blkeol_duration); > + > + val = readl(d->regs + DSI_VID_PCK_TIME); > + val &= ~DSI_VID_PCK_TIME_BLKEOL_DURATION_MASK; > + val |= blkeol_duration << > + DSI_VID_PCK_TIME_BLKEOL_DURATION_SHIFT; > + writel(val, d->regs + DSI_VID_PCK_TIME); > + > + /* Max burst limit, this is given in bytes */ > + val = readl(d->regs + DSI_VID_VCA_SETTING1); > + val &= ~DSI_VID_VCA_SETTING1_MAX_BURST_LIMIT_MASK; > + val |= blkeol_duration - 6; The vendor kernel writes blkeol_pck - 6 (instead of blkeol_duration) here: dsi_wfld(io, DSI_VID_VCA_SETTING1, MAX_BURST_LIMIT, vid_regs->blkeol_pck - 6); Also: It does not make a functional difference here but for clarity we should shift the value by DSI_VID_VCA_SETTING1_MAX_BURST_LIMIT_SHIFT (= 0), i.e. val |= blkeol_pck - 6 << DSI_VID_VCA_SETTING1_MAX_BURST_LIMIT_SHIFT; > + writel(val, d->regs + DSI_VID_VCA_SETTING1); > } > > /* Maximum line limit */ > val = readl(d->regs + DSI_VID_VCA_SETTING2); > - val |= blkline_pck << > - DSI_VID_VCA_SETTING2_EXACT_BURST_LIMIT_SHIFT; > + val &= ~DSI_VID_VCA_SETTING2_MAX_LINE_LIMIT_MASK; > + val |= blkline_pck - 6 << > + DSI_VID_VCA_SETTING2_MAX_LINE_LIMIT_SHIFT; > writel(val, d->regs + DSI_VID_VCA_SETTING2); > - > + dev_dbg(d->dev, "blkline pck: %d bytes\n", blkline_pck - 6); > } > > static void mcde_dsi_start(struct mcde_dsi *d) > diff --git a/drivers/gpu/drm/mcde/mcde_dsi_regs.h > b/drivers/gpu/drm/mcde/mcde_dsi_regs.h > index 8089db805c57..16551af1089e 100644 > --- a/drivers/gpu/drm/mcde/mcde_dsi_regs.h > +++ b/drivers/gpu/drm/mcde/mcde_dsi_regs.h > @@ -228,6 +228,7 @@ > > #define DSI_VID_PCK_TIME 0x000000A8 > #define DSI_VID_PCK_TIME_BLKEOL_DURATION_SHIFT 0 > +#define DSI_VID_PCK_TIME_BLKEOL_DURATION_MASK 0x00000FFF > > #define DSI_VID_DPHY_TIME 0x000000AC > #define DSI_VID_DPHY_TIME_REG_LINE_DURATION_SHIFT 0 > -- > 2.21.0 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel