Message-ID: <604c82ee-af16-5f34-b229-5e919c4adfdc@tronnes.org>
Date:   Tue, 6 Aug 2019 18:06:57 +0200
From:   Noralf Trønnes <noralf@...nnes.org>
To:     Jan Sebastian Götte <linux@...eg.net>,
        dri-devel@...ts.freedesktop.org
Cc:     David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] drm: tiny: gdepaper: add driver for 2/3 color epaper
 displays

Hi Jan,

A couple of drive by comments:

Den 30.07.2019 15.48, skrev Jan Sebastian Götte:
> These ePaper displays are made in b/w, b/w/red and b/w/yellow variations
> by Good Display (www.e-paper-display.com) and sold on breakout boards by
> waveshare (waveshare.com).
> 
> This driver was tested against a 2.7" b/w/r display (IL91874 driver) as
> well as 4.2" b/w/r display (IL0389 driver) but can probably be
> configured for other displays using only a single COG driver die (i.e.
> any model except for their largest 12.5" model).
> 
> Through tinydrm this driver presents a standard XRGB8888 or RGB565
> framebuffer device to userspace and internally converts RGB colors to
> the epaper's colors. RGB565 support is provided for compatibility with
> anything expecting a small-size TFT LCD whose drivers often support
> RGB565.
> 
> For userspace, the most notable difference between these epaper displays
> and more traditional TFT LCDs besides their limited color space is their
> enormously long refresh times-about 5s for a b/w display and >15s for
> b/w/r and b/w/y.
> 
> This driver uses the display's "4-line SPI" interface (~CS, SCK, MOSI,
> BUSY) and does not support the bi-directional "3-line SPI" interface
> since target support for bidirectional SPI is going to be patchy. This
> means it can't read the display drivers factory-programmed ROM to
> automatically recognize the display's model, resolution or color
> configuration. This information must be provided from the device tree,
> either with a compatible="" section containing the display's model or
> through manual configuration of its various parameters. See the device
> tree doc file for details.
> 
> We name this driver "gdepaper" instead of naming it after the driver
> IC's part number since Good Display seems to be using a large number of
> mutually compatible distinct variations of the same driver IC, named
> using several different numbering schemes. Our plan is to add this
> generic driver, and to provide support for individual chips' quirks as
> they are discovered.
> 
> This driver uses four ioctls:
> * Force full display refresh, which may be needed since some epaper
>   displays need to be refreshed periodically (every few minutes to
>   hours) or the image degrades. This ioctl allows a user program to
>   trigger this hardware refresh from the epapers internal frame buffer
>   without need for a full redraw.
> * Get/set refresh params, which controls the display's waveform LUTs and
>   voltage levels.  This is necessary in case a user wants to tune paint/
>   refresh performance to their particular content and temperature.  In
>   particular for acceptable-quality partial refresh on BWR/BWY panels
>   this is essential.
> * Enable partial mode, which enables partial update and refresh on fb
>   updates. By default this is disabled since careful LUT tuning is
>   required for this to not look terrible. Also not all content is suited
>   to this mode of operation.
> 
> Tested-by: Jan Sebastian Götte <linux@...eg.net>

No need for this, it's assumed that you have tested your own driver ;-)

> Signed-off-by: Jan Sebastian Götte <linux@...eg.net>
> ---
>  drivers/gpu/drm/tinydrm/gdepaper.c        | 1262 +++++++++++++++++++++
>  drivers/gpu/drm/tinydrm/gdepaper_models.h |  157 +++
>  2 files changed, 1419 insertions(+)
>  create mode 100644 drivers/gpu/drm/tinydrm/gdepaper.c
>  create mode 100644 drivers/gpu/drm/tinydrm/gdepaper_models.h
> 
> diff --git a/drivers/gpu/drm/tinydrm/gdepaper.c b/drivers/gpu/drm/tinydrm/gdepaper.c

<snip>

> +static int gdepaper_spi_transfer_cstoggle(struct gdepaper *epap, u8 *data,
> +					  size_t len)
> +{
> +	int i, ret = 0;
> +
> +	for (i = 0; i < len; i++) {
> +		ret = tinydrm_spi_transfer(epap->spi, epap->spi_speed_hz,
> +					   NULL, 8, &data[i], 1);

tinydrm_spi_transfer() is gone now, but you can use spi_write() here. It
will run the transfer at ->max_speed_hz. The reason for
tinydrm_spi_transfer() to have a speed argument, is that some display
controllers can run pixel transfers at a higher speed than the commands,
hence the need to override and run commands at a lower speed.

> +		if (ret)
> +			return ret;
> +		udelay(1); /* FIXME necessary? */
> +	}
> +
> +	return ret;
> +}

<snip>

> +static void gdepaper_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
> +{
> +	struct gdepaper *epap = drm_to_gdepaper(fb->dev);
> +	struct device *dev = epap->drm.dev;
> +	unsigned int w, h;
> +	struct drm_rect rect_aligned;
> +	int idx, ret = 0;
> +
> +	dev_dbg(dev, "fbdirty\n");
> +
> +	if (!epap->partial_update_en) {
> +		*rect = (struct drm_rect) {
> +			.x1 = 0,
> +			.x2 = fb->width,
> +			.y1 = 0,
> +			.y2 = fb->height,
> +		};
> +	}
> +	w = rect->x2 - rect->x1;
> +	h = rect->y2 - rect->y1;
> +
> +	if (!epap->enabled) {
> +		dev_dbg(dev, "panel is disabled, returning\n");
> +		return;
> +	}
> +
> +	if (!drm_dev_enter(fb->dev, &idx)) {
> +		dev_dbg(dev, "can't acquire drm dev lock\n");
> +		return;
> +	}
> +
> +	dev_dbg(dev, "Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id,
> +		      DRM_RECT_ARG(rect));
> +
> +	ret = gdepaper_power_on(epap);
> +	if (ret)
> +		goto err_out;
> +
> +	if (w == fb->width && h == fb->height) { /* full refresh */
> +		/* black */
> +		ret = gdepaper_txbuf_pack((u8 *)epap->tx_buf, fb, rect,
> +					  GDEP_CH_BLACK);
> +
> +		dev_dbg(dev, "Sending %d byte full framebuf\n", ret);
> +		ret = gdepaper_command(epap, GDEP_CMD_DATA_START_TX_COL1,
> +			(u8 *)epap->tx_buf, ret);
> +		if (ret)
> +			goto err_out;
> +
> +		/* red/yellow */
> +		ret = gdepaper_txbuf_pack((u8 *)epap->tx_buf, fb, rect,
> +					  GDEP_CH_RED_YELLOW);
> +
> +		ret = gdepaper_command(epap, GDEP_CMD_DATA_START_TX_COL2,
> +			(u8 *)epap->tx_buf, ret);
> +		if (ret)
> +			goto err_out;
> +
> +		ret = gdepaper_command(epap, GDEP_CMD_DATA_STOP, NULL, 0);
> +		if (ret)
> +			goto err_out;
> +
> +		ret = gdepaper_command(epap, GDEP_CMD_DISP_RF, NULL, 0);
> +		if (ret)
> +			goto err_out;
> +
> +	} else {
> +		rect_aligned.x1 = rect->x1 & (~7U);

This can also be written as: round_down(rect->x1, 8);

> +		rect_aligned.y1 = rect->y1;
> +		rect_aligned.y2 = rect->y2;
> +		rect_aligned.x2 = rect_aligned.x1 +
> +			  ((rect->x2 - rect_aligned.x1 + 7) & (~7U));

Are you rounding up here?
If so: rect_aligned.x2 = round_up(rect->x2, 8);

> +
> +		/* black */
> +		ret = gdepaper_txbuf_pack((u8 *)epap->tx_buf, fb, &rect_aligned,
> +					  GDEP_CH_BLACK);
> +		dev_dbg(dev, "Sending %d byte partial framebuf\n", ret);
> +		if (ret < 0)
> +			goto err_out;
> +		ret = gdepaper_partial_cmd(epap, &rect_aligned,
> +			GDEP_CMD_PD_START_TX_COL1, (u8 *)epap->tx_buf, ret);
> +		if (ret)
> +			goto err_out;
> +
> +		/* red/yellow */
> +		ret = gdepaper_txbuf_pack((u8 *)epap->tx_buf, fb, &rect_aligned,
> +					  GDEP_CH_RED_YELLOW);
> +		if (ret < 0)
> +			goto err_out;
> +		ret = gdepaper_partial_cmd(epap, &rect_aligned,
> +			GDEP_CMD_PD_START_TX_COL2, (u8 *)epap->tx_buf,
> +			ret);
> +		if (ret)
> +			goto err_out;
> +		ret = gdepaper_command(epap, GDEP_CMD_DATA_STOP, NULL, 0);
> +		if (ret)
> +			goto err_out;
> +		ret = gdepaper_partial_cmd(epap, rect, GDEP_CMD_PART_DISP_RF,
> +			NULL, 0);
> +		if (ret)
> +			goto err_out;
> +	}
> +
> +	if (gdepaper_wait_busy(epap)) {
> +		dev_err(dev, "Timeout on partial refresh cmd\n");
> +		goto err_out;
> +	}
> +
> +	ret = gdepaper_power_off(epap);
> +	if (ret)
> +		goto err_out;
> +
> +	drm_dev_exit(idx);
> +	return;
> +
> +err_out:
> +	/* Try to power off anyway */
> +	gdepaper_power_off(epap);
> +
> +	dev_err(fb->dev->dev, "Failed to update display %d\n", ret);
> +	drm_dev_exit(idx);
> +}

<snip>

> +static int gdepaper_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct device_node *np = dev->of_node;
> +	const struct of_device_id *of_id;
> +	struct drm_device *drm;
> +	struct drm_display_mode *mode;
> +	struct gdepaper *epap;
> +	const struct gdepaper_type_descriptor *type_desc;
> +	int ret;
> +	size_t bufsize;
> +
> +	of_id = of_match_node(gdepaper_of_match, np);
> +	if (WARN_ON(of_id == NULL)) {
> +		dev_warn(dev, "dt node didn't match, aborting probe\n");
> +		return -EINVAL;
> +	}
> +	type_desc = of_id->data;
> +
> +	dev_dbg(dev, "Probing gdepaper module\n");
> +	epap = kzalloc(sizeof(*epap), GFP_KERNEL);
> +	if (!epap)
> +		return -ENOMEM;
> +
> +	epap->enabled = false;
> +	mutex_init(&epap->cmdlock);
> +	epap->tx_buf = NULL;
> +	epap->spi = spi;
> +
> +	drm = &epap->drm;
> +	ret = devm_drm_dev_init(dev, drm, &gdepaper_driver);
> +	if (ret) {
> +		dev_warn(dev, "failed to init drm dev\n");
> +		goto err_free;
> +	}
> +
> +	drm_mode_config_init(drm);
> +
> +	epap->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(epap->reset)) {
> +		dev_err(dev, "Failed to get reset GPIO\n");
> +		ret = PTR_ERR(epap->reset);
> +		goto err_free;
> +	}
> +
> +	epap->busy = devm_gpiod_get(dev, "busy", GPIOD_IN);
> +	if (IS_ERR(epap->busy)) {
> +		dev_err(dev, "Failed to get busy GPIO\n");
> +		ret = PTR_ERR(epap->busy);
> +		goto err_free;
> +	}
> +
> +	epap->dc = devm_gpiod_get(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(epap->dc)) {
> +		dev_err(dev, "Failed to get dc GPIO\n");
> +		ret = PTR_ERR(epap->dc);
> +		goto err_free;
> +	}
> +
> +	epap->spi_speed_hz = 2000000;
> +	epap->pll_div = 1;
> +	epap->framerate_mHz = 81850;
> +	epap->rfp.vg_lv = GDEP_PWR_VGHL_16V;
> +	epap->rfp.vcom_sel = 0;
> +	epap->rfp.vdh_bw_mv = 11000; /* drive high level, b/w pixel */
> +	epap->rfp.vdh_col_mv = 4200; /* drive high level, red/yellow pixel */
> +	epap->rfp.vdl_mv = -11000; /* drive low level */
> +	epap->rfp.border_data_sel = 2; /* "vbd" */
> +	epap->rfp.data_polarity = 0; /* "ddx" */
> +	epap->rfp.vcom_dc_mv = -1000;
> +	epap->rfp.vcom_data_ivl_hsync = 10; /* hsync periods */
> +	epap->rfp.use_otp_luts_flag = 1;
> +	epap->ss_param[0] = 0x07;
> +	epap->ss_param[1] = 0x07;
> +	epap->ss_param[2] = 0x17;
> +	epap->controller_res = GDEP_CTRL_RES_320X300;
> +
> +	ret = gdepaper_of_read_luts(epap, np, dev);
> +	if (ret) {
> +		dev_warn(dev, "can't read LUTs from dt\n");
> +		goto err_free;
> +	}
> +
> +	of_property_read_u32(np, "controller-resolution",
> +			&epap->controller_res);
> +	of_property_read_u32(np, "spi-speed-hz", &epap->spi_speed_hz);
> +	epap->partial_update_en = of_property_read_bool(np, "partial-update");
> +	ret = of_property_read_u32(np, "colors", &epap->display_colors);
> +	if (ret == -EINVAL) {
> +		if (type_desc) {
> +			epap->display_colors = type_desc->colors;
> +
> +		} else {
> +			dev_err(dev, "colors must be set in dt\n");
> +			ret = -EINVAL;
> +			goto err_free;
> +		}
> +	} else if (ret) {
> +		dev_err(dev, "Invalid dt colors property\n");
> +		goto err_free;
> +	}
> +	if (epap->display_colors < 0 ||
> +			epap->display_colors >= GDEPAPER_COL_END) {
> +		dev_err(dev, "invalid colors value\n");
> +		ret = -EINVAL;
> +		goto err_free;
> +	}
> +	epap->mirror_x = of_property_read_bool(np, "mirror-x");
> +	epap->mirror_y = of_property_read_bool(np, "mirror-y");
> +	of_property_read_u32(np, "pll-div", &epap->pll_div);
> +	of_property_read_u32(np, "fps-millihertz", &epap->framerate_mHz);
> +	of_property_read_u32(np, "vghl-level", &epap->rfp.vg_lv);
> +	epap->vds_en = !of_property_read_bool(np, "vds-external");
> +	epap->vdg_en = !of_property_read_bool(np, "vdg-external");
> +	of_property_read_u32(np, "vcom", &epap->rfp.vcom_sel);
> +	of_property_read_u32(np, "vdh-bw-millivolts", &epap->rfp.vdh_bw_mv);
> +	of_property_read_u32(np, "vdh-color-millivolts", &epap->rfp.vdh_col_mv);
> +	of_property_read_u32(np, "vdl-millivolts", &epap->rfp.vdl_mv);
> +	of_property_read_u32(np, "border-data", &epap->rfp.border_data_sel);
> +	of_property_read_u32(np, "data-polarity", &epap->rfp.data_polarity);
> +	ret = of_property_read_u8_array(np, "boost-soft-start",
> +			(u8 *)&epap->ss_param, sizeof(epap->ss_param));
> +	if (ret && ret != -EINVAL)
> +		dev_err(dev, "invalid boost-soft-start value, ignoring\n");
> +	of_property_read_u32(np, "vcom-data-interval-periods",
> +			&epap->rfp.vcom_data_ivl_hsync);

Why do you need these DT properties when you define compatibles for all
the panels, can't you include these settings in the type descriptor?

> +
> +	/* Accept both positive and negative notation */
> +	if (epap->rfp.vdl_mv < 0)
> +		epap->rfp.vdl_mv = -epap->rfp.vdl_mv;
> +	if (epap->rfp.vcom_dc_mv < 0)
> +		epap->rfp.vcom_dc_mv = -epap->rfp.vcom_dc_mv;
> +
> +	/* (from mipi-dbi.c:)
> +	 * Even though it's not the SPI device that does DMA (the master does),
> +	 * the dma mask is necessary for the dma_alloc_wc() in
> +	 * drm_gem_cma_create(). The dma_addr returned will be a physical
> +	 * address which might be different from the bus address, but this is
> +	 * not a problem since the address will not be used.
> +	 * The virtual address is used in the transfer and the SPI core
> +	 * re-maps it on the SPI master device using the DMA streaming API
> +	 * (spi_map_buf()).
> +	 */
> +	if (!dev->coherent_dma_mask) {
> +		ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> +		if (ret) {
> +			dev_warn(dev, "Failed to set dma mask %d\n", ret);
> +			goto err_free;
> +		}
> +	}
> +
> +	mode = gdepaper_of_read_mode(type_desc, np, dev);
> +	if (IS_ERR(mode)) {
> +		dev_warn(dev, "Failed to read mode: %ld\n", PTR_ERR(mode));
> +		ret = PTR_ERR(mode);
> +		goto err_free;
> +	}
> +
> +	/* 8 pixels per byte, bit-packed */
> +	bufsize = (mode->vdisplay * mode->hdisplay + 7)/8;

DIV_ROUND_UP(mode->vdisplay * mode->hdisplay, 8)

> +	epap->tx_buf = devm_kmalloc(drm->dev, bufsize, GFP_KERNEL);
> +	if (!epap->tx_buf) {
> +		ret = -ENOMEM;
> +		goto err_free;
> +	}
> +
> +	/* TODO rotation support? */
> +	ret = tinydrm_display_pipe_init(drm, &epap->pipe, &gdepaper_pipe_funcs,
> +					DRM_MODE_CONNECTOR_VIRTUAL,
> +					gdepaper_formats,
> +					ARRAY_SIZE(gdepaper_formats), mode, 0);

tinydrm_display_pipe_init() is gone now, here's how I replaced it in the
other e-ink driver:

drm/tinydrm/repaper: Don't use tinydrm_display_pipe_init()
https://cgit.freedesktop.org/drm/drm-misc/commit?id=1321db837549a0ff9dc2c95ff76c46770f7f02a1

Noralf.

> +	if (ret) {
> +		dev_warn(dev, "Failed to initialize display pipe: %d\n", ret);
> +		goto err_free;
> +	}
> +
> +	drm->mode_config.funcs = &gdepaper_dbi_mode_config_funcs;
> +	drm->mode_config.preferred_depth = 32;
> +	drm_plane_enable_fb_damage_clips(&epap->pipe.plane);
> +	drm_mode_config_reset(drm);
> +
> +	ret = drm_dev_register(drm, 0);
> +	if (ret) {
> +		dev_warn(dev, "Failed to register drm device: %d\n", ret);
> +		goto err_free;
> +	}
> +
> +	spi_set_drvdata(spi, drm);
> +	drm_fbdev_generic_setup(drm, 0);
> +
> +	dev_dbg(dev, "Probed gdepaper module\n");
> +	return 0;
> +err_free:
> +	kfree(epap);
> +	return ret;
> +}

Powered by blists - more mailing lists