On Wed, Jan 23, 2019 at 11:54 PM Maxime Ripard
<maxime.ripard@xxxxxxxxxxx> wrote:
>
> From: Konstantin Sudakov <k.sudakov@xxxxxxxxxxxxxxxxxx>
>
> The current driver doesn't support the DSI burst operation mode.
>
> Let's add the needed quirks to make it work.
>
> Signed-off-by: Konstantin Sudakov <k.sudakov@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> ---
> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 178 +++++++++++++++++++-------
> 1 file changed, 136 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index e3e4ba90c059..6840b3512a45 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -23,7 +23,9 @@
> #include <drm/drm_mipi_dsi.h>
> #include <drm/drm_panel.h>
>
> +#include "sun4i_crtc.h"
> #include "sun4i_drv.h"
> +#include "sun4i_tcon.h"
> #include "sun6i_mipi_dsi.h"
>
> #include <video/mipi_display.h>
> @@ -32,6 +34,8 @@
> #define SUN6I_DSI_CTL_EN BIT(0)
>
> #define SUN6I_DSI_BASIC_CTL_REG 0x00c
> +#define SUN6I_DSI_BASIC_CTL_TRAIL_INV(n) (((n) & 0xf) << 4)
> +#define SUN6I_DSI_BASIC_CTL_TRAIL_FILL BIT(3)
> #define SUN6I_DSI_BASIC_CTL_HBP_DIS BIT(2)
> #define SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS BIT(1)
> #define SUN6I_DSI_BASIC_CTL_VIDEO_BURST BIT(0)
> @@ -152,6 +156,8 @@
>
> #define SUN6I_DSI_CMD_TX_REG(n) (0x300 + (n) * 0x04)
>
> +#define SUN6I_DSI_SYNC_POINT 40
> +
> enum sun6i_dsi_start_inst {
> DSI_START_LPRX,
> DSI_START_LPTX,
> @@ -365,31 +371,101 @@ static u16 sun6i_dsi_get_video_start_delay(struct sun6i_dsi *dsi,
> return delay;
> }
>
> +static u16 sun6i_dsi_get_line_num(struct sun6i_dsi *dsi,
> + struct drm_display_mode *mode)
> +{
> + struct mipi_dsi_device *device = dsi->device;
> + unsigned Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
> +
> + return mode->htotal * Bpp / device->lanes;
> +}
> +
> +static u16 sun6i_dsi_get_drq_edge0(struct sun6i_dsi *dsi,
> + struct drm_display_mode *mode,
> + u16 line_num, u16 edge1)
> +{
> + u16 edge0 = edge1;
> +
> + edge0 += (mode->hdisplay + 40) * SUN6I_DSI_TCON_DIV / 8;
> +
> + if (edge0 > line_num)
> + return edge0 - line_num;
> +
> + return 1;
> +}
> +
> +static u16 sun6i_dsi_get_drq_edge1(struct sun6i_dsi *dsi,
> + struct drm_display_mode *mode,
> + u16 line_num)
> +{
> + struct mipi_dsi_device *device = dsi->device;
> + unsigned Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
> + unsigned hbp = mode->htotal - mode->hsync_end;
Allwinner's hbp is actually horizontal back porch plus sync pulse.
So this should be
hbp = mode->htotal - mode->hsync_start;
> + u16 edge1;
> +
> +
> + edge1 = SUN6I_DSI_SYNC_POINT;
> + edge1 += mode->hdisplay + hbp + 20;
> + edge1 = edge1 * Bpp / device->lanes;
Compared to the A64 BSP, this seems to be incorrect. The original code was
edge1 = sync_point +
(panel->lcd_x + panel->lcd_hbp + 20) *
dsi_pixel_bits[panel->lcd_dsi_format] /
(8 * panel->lcd_dsi_lane);
Note that sync_point is outside of the parentheses and should not be
multiplied and divided.
This would make sense if sync_point is a fixed delay that is needed
regardless of the timings.
> +
> + if (edge1 > line_num)
> + return line_num;
> +
> + return edge1;
> +}
> +
> static void sun6i_dsi_setup_burst(struct sun6i_dsi *dsi,
> struct drm_display_mode *mode)
> {
> struct mipi_dsi_device *device = dsi->device;
> - u32 val = 0;
> + u32 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> + u32 tcon0_drq = 0;
> +
> + if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> + u16 line_num = sun6i_dsi_get_line_num(dsi, mode);
> + u16 edge0, edge1;
> +
> + edge1 = sun6i_dsi_get_drq_edge1(dsi, mode, line_num);
> + edge0 = sun6i_dsi_get_drq_edge0(dsi, mode, line_num, edge1);
>
> - if ((mode->hsync_end - mode->hdisplay) > 20) {
> + regmap_write(dsi->regs, SUN6I_DSI_BURST_DRQ_REG,
> + SUN6I_DSI_BURST_DRQ_EDGE0(edge0) |
> + SUN6I_DSI_BURST_DRQ_EDGE1(edge1));
> +
> + regmap_write(dsi->regs, SUN6I_DSI_BURST_LINE_REG,
> + SUN6I_DSI_BURST_LINE_NUM(line_num) |
> + SUN6I_DSI_BURST_LINE_SYNC_POINT(SUN6I_DSI_SYNC_POINT));
> +
> + tcon0_drq = SUN6I_DSI_TCON_DRQ_ENABLE_MODE;
> + } else if ((mode->hsync_end - mode->hdisplay) > 20) {
> /* Maaaaaagic */
> u16 drq = (mode->hsync_end - mode->hdisplay) - 20;
This and the above if clause should use hsync_start instead of hsync_end.
The A64 BSP specifies drq = vfp - 20.
> -
> - drq *= mipi_dsi_pixel_format_to_bpp(device->format);
> + drq *= bpp;
> drq /= 32;
>
> - val = (SUN6I_DSI_TCON_DRQ_ENABLE_MODE |
> - SUN6I_DSI_TCON_DRQ_SET(drq));
> + tcon0_drq = SUN6I_DSI_TCON_DRQ_ENABLE_MODE |
> + SUN6I_DSI_TCON_DRQ_SET(drq);
> }
>
> - regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG, val);
> + regmap_write(dsi->regs, SUN6I_DSI_TCON_DRQ_REG, tcon0_drq);
> }
>
> static void sun6i_dsi_setup_inst_loop(struct sun6i_dsi *dsi,
> struct drm_display_mode *mode)
> {
> + struct mipi_dsi_device *device = dsi->device;
> u16 delay = 50 - 1;
>
> + if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> + delay = (mode->htotal - mode->hdisplay) * 150;
> + delay /= (mode->clock / 1000) * 8;
> + delay -= 50;
> + }
> +
> + regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_SEL_REG,
> + 2 << (4 * DSI_INST_ID_LP11) |
> + 3 << (4 * DSI_INST_ID_DLY));
> +
> regmap_write(dsi->regs, SUN6I_DSI_INST_LOOP_NUM_REG(0),
> SUN6I_DSI_INST_LOOP_NUM_N0(50 - 1) |
> SUN6I_DSI_INST_LOOP_NUM_N1(delay));
> @@ -456,48 +532,66 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
> struct mipi_dsi_device *device = dsi->device;
> unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
> u16 hbp, hfp, hsa, hblk, vblk;
> + u32 basic_ctl = 0;
> size_t bytes;
> u8 *buffer;
>
> /* Do all timing calculations up front to allocate buffer space */
>
> - /*
> - * A sync period is composed of a blanking packet (4 bytes +
> - * payload + 2 bytes) and a sync event packet (4 bytes). Its
> - * minimal size is therefore 10 bytes
> - */
> + if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> + hsa = 0;
> + hbp = 0;
> + hfp = 0;
> + hblk = mode->hdisplay * Bpp;
> + vblk = 0;
> + basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST |
> + SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS |
> + SUN6I_DSI_BASIC_CTL_HBP_DIS;
> +
> + if (device->lanes == 4)
> + basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL |
> + SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc);
> + } else {
> + /*
> + * A sync period is composed of a blanking packet (4 bytes +
> + * payload + 2 bytes) and a sync event packet (4 bytes). Its
> + * minimal size is therefore 10 bytes
> + */
> #define HSA_PACKET_OVERHEAD 10
> - hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
> - (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD);
> -
> - /*
> - * The backporch is set using a blanking packet (4 bytes +
> - * payload + 2 bytes). Its minimal size is therefore 6 bytes
> - */
> + hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
> + (mode->hsync_end - mode->hsync_start) * Bpp -
> + HSA_PACKET_OVERHEAD);
I believe for all these packets, that minimum value is not needed. The
WC field of the long command packet refers to the length of the payload
only. The payload is used to pad the packet to the desired length, but
is otherwise useless. Likewise, the CRC value is only calculated against
the payload.
Specifying a minimum means you are likely skewing the timings.
> +
> + /*
> + * The backporch is set using a blanking packet (4 bytes +
> + * payload + 2 bytes). Its minimal size is therefore 6 bytes
> + */
> #define HBP_PACKET_OVERHEAD 6
> - hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
> - (mode->hsync_start - mode->hdisplay) * Bpp - HBP_PACKET_OVERHEAD);
> -
> - /*
> - * The frontporch is set using a blanking packet (4 bytes +
> - * payload + 2 bytes). Its minimal size is therefore 6 bytes
> - */
> + hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
> + (mode->hsync_start - mode->hdisplay) * Bpp -
> + HBP_PACKET_OVERHEAD);
> +
> + /*
> + * The frontporch is set using a blanking packet (4 bytes +
> + * payload + 2 bytes). Its minimal size is therefore 6 bytes
> + */
> #define HFP_PACKET_OVERHEAD 6
> - hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
> - (mode->htotal - mode->hsync_end) * Bpp - HFP_PACKET_OVERHEAD);
> -
> - /*
> - * hblk seems to be the line + porches length.
> - */
> - hblk = mode->htotal * Bpp - hsa;
> -
> - /*
> - * And I'm not entirely sure what vblk is about. The driver in
> - * Allwinner BSP is using a rather convoluted calculation
> - * there only for 4 lanes. However, using 0 (the !4 lanes
> - * case) even with a 4 lanes screen seems to work...
> - */
> - vblk = 0;
> + hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
> + (mode->htotal - mode->hsync_end) * Bpp - HFP_PACKET_OVERHEAD);
> +
> + /*
> + * hblk seems to be the line + porches length.
> + */
> + hblk = mode->htotal * Bpp - hsa;
Based on the timing diagram in section 8.11 of the MIPI DSI spec, hblk is
supposed to take up the remainder of the scan line after the sync event or
sync pulse during the vertical inactive period. It is the BLLP portion.
This seems like the correct value, though it is somewhat simplified.
The packet overhead is gone (cancelled out with the one from hsa).
> +
> + /*
> + * And I'm not entirely sure what vblk is about. The driver in
> + * Allwinner BSP is using a rather convoluted calculation
> + * there only for 4 lanes. However, using 0 (the !4 lanes
> + * case) even with a 4 lanes screen seems to work...
> + */
No idea either.
Hope this helps.
ChenYu
> + vblk = 0;
> + }
>
> /* How many bytes do we need to send all payloads? */
> bytes = max_t(size_t, max(max(hfp, hblk), max(hsa, hbp)), vblk);
> @@ -505,7 +599,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
> if (WARN_ON(!buffer))
> return;
>
> - regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, 0);
> + regmap_write(dsi->regs, SUN6I_DSI_BASIC_CTL_REG, basic_ctl);
>
> regmap_write(dsi->regs, SUN6I_DSI_SYNC_HSS_REG,
> sun6i_dsi_build_sync_pkt(MIPI_DSI_H_SYNC_START,
> --
> git-series 0.9.1
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
