Re: [PATCH] [RFC] drm: mxsfb: Implement LCDIF scanout CRC32 support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Marek,

On Sun, 2022-02-06 at 19:56 +0100, Marek Vasut wrote:
> The LCDIF controller as present in i.MX6SX/i.MX8M Mini/Nano has a CRC_STAT
> register, which contains CRC32 of the frame as it was clocked out of the
> DPI interface of the LCDIF. This is likely meant as a functional safety
> register.
> 
> Unfortunatelly, there is zero documentation on how the CRC32 is calculated,
> there is no documentation of the polynomial, the init value, nor on which
> data is the checksum applied.
> 
> By applying brute-force on 8 pixel / 2 line frame, which is the minimum
> size LCDIF would work with, it turns out the polynomial is CRC32_POLY_LE
> 0xedb88320 , init value is 0xffffffff , the input data are bitrev32()
> of the entire frame and the resulting CRC has to be also bitrev32()ed.

No idea how the HW calculates the CRC value.
I didn't hear anyone internal tried this feature.

> 
> Doing this calculation in software for each frame is unrealistic due to
> the CPU demand, implement at least a sysfs attribute which permits testing
> the current frame on demand.

Why not using the existing debugfs CRC support implemented
in drivers/gpu/drm/drm_debugfs_crc.c?

> 
> Unfortunatelly, this functionality has another problem. On all of those SoCs,
> it is possible to overload interconnect e.g. by concurrent USB and uSDHC
> transfers, at which point the LCDIF LFIFO suffers an UNDERFLOW condition,
> which results in the image being shifted to the right by exactly LFIFO size
> pixels. On i.MX8M Mini, the LFIFO is 76x256 bits = 2432 Byte ~= 810 pixel
> at 24bpp. In this case, the LCDIF does not assert UNDERFLOW_IRQ bit, the
> frame CRC32 indicated in CRC_STAT register matches the CRC32 of the frame
> in DRAM, the RECOVER_ON_UNDERFLOW bit has no effect, so if this mode of
> failure occurs, the failure gets undetected and uncorrected.

Hmmm, interesting, no UNDERFLOW_IRQ bit asserted when LCDIF suffers an
UNDERFLOW condition?  Are you sure LCDIF really underflows?
If the shifted image is seen on a MIPI DSI display, could that be a
MIPI DSI or DPHY issue, like wrong horizontal parameter(s)?


> 
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> Cc: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> Cc: Peng Fan <peng.fan@xxxxxxx>
> Cc: Robby Cai <robby.cai@xxxxxxx>
> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Cc: Stefan Agner <stefan@xxxxxxxx>
> ---
>  drivers/gpu/drm/mxsfb/mxsfb_drv.c  | 38 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/mxsfb/mxsfb_drv.h  |  3 +++
>  drivers/gpu/drm/mxsfb/mxsfb_kms.c  | 11 +++++----
>  drivers/gpu/drm/mxsfb/mxsfb_regs.h |  1 +
>  4 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index 4ff3c6195dd0c..6f296b398f28c 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/crc32.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> @@ -292,6 +293,37 @@ static void mxsfb_unload(struct drm_device *drm)
>  	pm_runtime_disable(drm->dev);
>  }
>  
> +static ssize_t mxsfb_frame_checksum_show(struct device *dev,
> +					     struct device_attribute *attr,
> +					     char *buf)
> +{
> +	struct drm_device *drm = dev_get_drvdata(dev);
> +	struct mxsfb_drm_private *mxsfb = drm->dev_private;
> +	u32 hwcrc = readl(mxsfb->base, LCDC_V4_CRC_STAT);

Access register without relevant clock(s) enabled?

LCDC_V4_CRC_STAT seems to hint that there should be some verion control
logic for MXSFB_V3/4/6.

Regards,
Liu Ying

> +	u32 swcrc = 0xffffffff;
> +	int i;
> +
> +	if (mxsfb->gem_vaddr) {
> +		for (i = 0; i < mxsfb->gem_size / 4; i++) {
> +			u32 data = bitrev32(((u32 *)mxsfb->gem_vaddr)[i]);
> +			swcrc = crc32(swcrc, &data, 4);
> +		}
> +		swcrc = bitrev32(swcrc);
> +	}
> +
> +	return sysfs_emit(buf, "HW:%08x,SW:%08x,OK:%d\n", hwcrc, swcrc, hwcrc == swcrc);
> +}
> +static DEVICE_ATTR(frame_checksum, 0444, mxsfb_frame_checksum_show, NULL);
> +
> +static struct attribute *mxsfb_attributes[] = {
> +	&dev_attr_frame_checksum.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group mxsfb_attr_group = {
> +	.attrs = mxsfb_attributes,
> +};
> +
>  DEFINE_DRM_GEM_CMA_FOPS(fops);
>  
>  static const struct drm_driver mxsfb_driver = {
> @@ -335,10 +367,16 @@ static int mxsfb_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_unload;
>  
> +	ret = devm_device_add_group(drm->dev, &mxsfb_attr_group);
> +	if (ret)
> +		goto err_attr;
> +
>  	drm_fbdev_generic_setup(drm, 32);
>  
>  	return 0;
>  
> +err_attr:
> +	drm_dev_unregister(drm);
>  err_unload:
>  	mxsfb_unload(drm);
>  err_free:
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> index ddb5b0417a82c..0a3e5dd1e8bab 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h
> @@ -44,6 +44,9 @@ struct mxsfb_drm_private {
>  	struct drm_encoder		encoder;
>  	struct drm_connector		*connector;
>  	struct drm_bridge		*bridge;
> +
> +	void				*gem_vaddr;
> +	size_t				gem_size;
>  };
>  
>  static inline struct mxsfb_drm_private *
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> index 03743a84c8e79..2a4edf5a2ac57 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -196,7 +196,7 @@ static int mxsfb_reset_block(struct mxsfb_drm_private *mxsfb)
>  	return clear_poll_bit(mxsfb->base + LCDC_CTRL, CTRL_CLKGATE);
>  }
>  
> -static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
> +static dma_addr_t mxsfb_get_fb_paddr(struct mxsfb_drm_private *mxsfb, struct drm_plane *plane)
>  {
>  	struct drm_framebuffer *fb = plane->state->fb;
>  	struct drm_gem_cma_object *gem;
> @@ -208,6 +208,9 @@ static dma_addr_t mxsfb_get_fb_paddr(struct drm_plane *plane)
>  	if (!gem)
>  		return 0;
>  
> +	mxsfb->gem_vaddr = gem->vaddr;
> +	mxsfb->gem_size = gem->base.size;
> +
>  	return gem->paddr;
>  }
>  
> @@ -370,7 +373,7 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
>  	mxsfb_crtc_mode_set_nofb(mxsfb, bus_format);
>  
>  	/* Write cur_buf as well to avoid an initial corrupt frame */
> -	paddr = mxsfb_get_fb_paddr(crtc->primary);
> +	paddr = mxsfb_get_fb_paddr(mxsfb, crtc->primary);
>  	if (paddr) {
>  		writel(paddr, mxsfb->base + mxsfb->devdata->cur_buf);
>  		writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
> @@ -476,7 +479,7 @@ static void mxsfb_plane_primary_atomic_update(struct drm_plane *plane,
>  	struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
>  	dma_addr_t paddr;
>  
> -	paddr = mxsfb_get_fb_paddr(plane);
> +	paddr = mxsfb_get_fb_paddr(mxsfb, plane);
>  	if (paddr)
>  		writel(paddr, mxsfb->base + mxsfb->devdata->next_buf);
>  }
> @@ -492,7 +495,7 @@ static void mxsfb_plane_overlay_atomic_update(struct drm_plane *plane,
>  	dma_addr_t paddr;
>  	u32 ctrl;
>  
> -	paddr = mxsfb_get_fb_paddr(plane);
> +	paddr = mxsfb_get_fb_paddr(mxsfb, plane);
>  	if (!paddr) {
>  		writel(0, mxsfb->base + LCDC_AS_CTRL);
>  		return;
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_regs.h b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> index 694fea13e893e..cf813a1da1d78 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_regs.h
> @@ -26,6 +26,7 @@
>  #define LCDC_VDCTRL2			0x90
>  #define LCDC_VDCTRL3			0xa0
>  #define LCDC_VDCTRL4			0xb0
> +#define LCDC_V4_CRC_STAT		0x1a0
>  #define LCDC_V4_DEBUG0			0x1d0
>  #define LCDC_V3_DEBUG0			0x1f0
>  #define LCDC_AS_CTRL			0x210




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux