Re: [PATCH 2/2] drm/tiny: add support for tft displays based on ilitek, ili9488

[Javier Martinez Canillas <javierm@redhat.com>]

Hello Tommaso,

On 10/18/22 18:45, Tommaso Merciai wrote:

[...]

> +config TINYDRM_ILI9488
> +	tristate "DRM support for ILI9488 display panels"
> +	depends on DRM && SPI
> +	select DRM_KMS_HELPER
> +	select DRM_GEM_CMA_HELPER
> +	select DRM_MIPI_DBI
> +	select BACKLIGHT_CLASS_DEVICE
> +	help
> +	  DRM driver for the following Ilitek ILI9488 panels:
> +	  * LCD 3.5" 320x480 TFT (Waveshare Pico-ResTouch-LCD-3.5")
> +
> +	  If M is selected the module will be called ili9486.
> +

Typo here, should be ili9488.

[...]

> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_modeset_helper.h>
> +

Please sort these alphabetically

> +
> +static void ili9488_rgb565_to_rgb666_line(u8 *dst, u16 *sbuf,
> +					  unsigned int pixels)
> +{
> +	unsigned int x;
> +
> +	for (x = 0; x < pixels; x++) {
> +		*dst++ = ((*sbuf & 0xF800) >> 8);
> +		*dst++ = ((*sbuf & 0x07E0) >> 3);
> +		*dst++ = ((*sbuf & 0x001F) << 3);
> +		sbuf++;
> +	}
> +}
> +

If these format conversions helpers are really needed, they need to be
added as helpers to drivers/gpu/drm/drm_format_helper.c instead.

> +static void ili9488_rgb565_to_rgb666(u8 *dst, void *vaddr,
> +				     struct drm_framebuffer *fb,
> +				     struct drm_rect *rect)
> +{
> +	unsigned long linepixels = drm_rect_width(rect);
> +	unsigned long lines = drm_rect_height(rect);
> +	size_t dst_len = linepixels * 3;
> +	size_t src_len = linepixels * fb->format->cpp[0];
> +	unsigned int y;
> +	u16 *sbuf;
> +
> +	/*
> +	 * The cma memory is write-combined so reads are uncached.
> +	 * Speed up by fetching one line at a time.
> +	 */
> +	sbuf = kmalloc(src_len, GFP_KERNEL);
> +	if (!sbuf)
> +		return;

This will cause an extra copy even when CMA isn't used. Please take a look
how the format helpers do this. You should pass a struct iosys_map param
and internally use the drm_fb_xfrm() helper that would handle both the I/O
mem and system memory cases.

> +static int ili9488_buf_copy(void *dst, struct drm_framebuffer *fb,
> +			    struct drm_rect *rect)
> +{
> +	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> +	void *src = cma_obj->vaddr;
> +	int ret = 0;
> +
> +	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> +	if (ret)
> +		return ret;
> +

If you rebase on top of the "[PATCH 0/5] drm: Add new plane helpers to
begin/end FB access" series then an explicit CPU access to GEM BOs sync
isn't needed anymore:

https://lore.kernel.org/dri-devel/20221017111510.20818-1-tzimmermann@suse.de/


> +	switch (fb->format->format) {
> +	case DRM_FORMAT_RGB565:
> +		ili9488_rgb565_to_rgb666(dst, src, fb, rect);

This is the biggest issue I see with this driver. The exported format is
RGB565 but RGB666 is used. I believe the policy is for the driver to expose
the native format to user-space.

I know that there isn't a DRM_FORMAT_RGB666 in neither DRM nor mesa, but
still I think that adding it should be part of this series. If you also
want to expose DRM_FORMAT_RGB565 for compatibility reasons, then I guess
that's OK but as mentioned the format helpers need to be in the DRM core.

[...]

> +static void ili9488_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
> +{

This looks very similar, if not the same than the mipi_dbi_fb_dirty() helper.

[...]

> +static void ili9488_pipe_update(struct drm_simple_display_pipe *pipe,
> +			       struct drm_plane_state *old_state)
> +{

And same for this, it's basically mipi_dbi_pipe_update() if you end using the
mipi_dbi_fb_dirty() helper instead of a custom ili9488_fb_dirty() handler.

> +	struct drm_plane_state *state = pipe->plane.state;
> +	struct drm_rect rect;
> +
> +	if (!pipe->crtc.state->active)
> +		return;
> +
> +	if (drm_atomic_helper_damage_merged(old_state, state, &rect))
> +		ili9488_fb_dirty(state->fb, &rect);

I see that most MIPI DBI drivers use this function to merge all the damage clips
into a big rectangle. Is there a reason why this is done in this way rather than
just iterating over all the damage areas and update them one by one?

Since for example there are multiple damage clips that aren't close to each other,
the resulting rectangle could be quite big.

[...]

> +DEFINE_DRM_GEM_CMA_FOPS(ili9488_fops);
> +

Do you really need CMA for this? Can't you just use DEFINE_DRM_GEM_FOPS() instead?

> +static struct drm_driver ili9488_driver = {
> +	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> +	.fops			= &ili9488_fops,
> +	DRM_GEM_CMA_DRIVER_OPS_VMAP,

	DRM_GEM_SHMEM_DRIVER_OPS, ?

> +	.debugfs_init		= mipi_dbi_debugfs_init,
> +	.name			= "ili9488",
> +	.desc			= "Ilitek ILI9488",
> +	.date			= "20221017",
> +	.major			= 1,
> +	.minor			= 0,
> +};
> +
> +static const struct of_device_id ili9488_of_match[] = {
> +	{ .compatible = "waveshare,pico-rt-lcd-35" },
> +	{ }
> +};

Do you really need to make the compatible that specific? I would just have an entry
for "ilitek,ili9488".

> +MODULE_DEVICE_TABLE(of, ili9488_of_match);
> +
> +static const struct spi_device_id ili9488_id[] = {
> +	{ "ili9488", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ili9488_id);
> +

The fact that this doesn't match the OF compatible string would break module auto
loading. Because the SPI core doesn't report an OF module alias, but always a SPI
alias regardless whether the device was registered using Device Trees or not.

[...]

> +	dbi->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(dbi->reset)) {
> +		DRM_DEV_ERROR(dev, "Failed to get gpio 'reset'\n");
> +		return PTR_ERR(dbi->reset);
> +	}

You could use dev_err_probe() instead. And same in other places.

> +static void ili9488_remove(struct spi_device *spi)
> +{
> +	struct drm_device *drm = spi_get_drvdata(spi);
> +
> +	drm_dev_unplug(drm);
> +	drm_atomic_helper_shutdown(drm);

I believe drm_atomic_helper_shutdown() isn't needed here since is done already
in ili9488_shutdown().

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat