Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 25 Feb 2022 00:27:23 +0100
From:   Martin =?iso-8859-1?Q?J=FCcker?= <martin.juecker@gmail.com>
To:     Inki Dae <inki.dae@samsung.com>
Cc:     Joonyoung Shim <jy0922.shim@samsung.com>,
        Seung-Woo Kim <sw0312.kim@samsung.com>,
        Kyungmin Park <kyungmin.park@samsung.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        dri-devel@lists.freedesktop.org,
        linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
        Martin =?iso-8859-1?Q?J=FCcker?= <martin.juecker@gmail.com>
Subject: Re: [PATCH] drm/exynos: fimd: add BGR support for exynos4/5
Message-ID: <20220224232723.GA133007@adroid>
References: <CGME20220129220203epcas1p25b1704191dd7babfb8d5b8dc6704d566@epcas1p2.samsung.com>
 <20220129220153.GA33165@adroid>
 <5e18705f-79c1-18a7-57f2-74866abe21e9@samsung.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5e18705f-79c1-18a7-57f2-74866abe21e9@samsung.com>
Precedence: bulk

Hello Inki,

On Thu, Feb 24, 2022 at 10:41:04AM +0900, Inki Dae wrote:
> Hi Martin.
> 
> I found that exynos4 and 5 data sheet include documented same register.
> RGB_ORDER_E field of VIDCONx registers will do same thing.

If I read the manual correctly, this register combined with the
RGB_ORDER_O makes it possible to map the whole RGB interface output to a
different order. What my patch provides is a way to configure each
hardware plane separately while maintaining a consistent output on the
RGB interface.

Implementing the RGB_ORDER_O and E would need some logic to make sure
that all planes are always using the same RGB order.

> 
> I'm not sure whether the use of undocumented register is safe or not - maybe some HW bug exists.

I see, that makes sense. Would it be possible then to introduce a new
compatible, e.g. samsung,exynos4210-fimd-ext which can be used on tested
devices? I know that some other Galaxy Note and S devices with the
exynos4 chip have the same problem (and solution).

> 
> Anyway, I'd like to recommend you to use documented register only.
> 
> Sorry for late and thanks,
> Inki Dae

Kind Regards
Martin

> 
> 22. 1. 30. 07:01에 Martin Jücker 이(가) 쓴 글:
> > In the downstream kernels for exynos4 and exynos5 devices, there is an
> > undocumented register that controls the order of the RGB output. It can
> > be set to either normal order or reversed, which enables BGR support for
> > those SoCs.
> > 
> > This patch enables the BGR support for all the SoCs that were found to
> > have at least one device with this logic in the corresponding downstream
> > kernels.
> > 
> > Signed-off-by: Martin Jücker <martin.juecker@gmail.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 42 ++++++++++++++++++++++--
> >  include/video/samsung_fimd.h             |  4 +++
> >  2 files changed, 44 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > index c735e53939d8..cb632360c968 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > @@ -109,6 +109,7 @@ struct fimd_driver_data {
> >  	unsigned int has_dp_clk:1;
> >  	unsigned int has_hw_trigger:1;
> >  	unsigned int has_trigger_per_te:1;
> > +	unsigned int has_bgr_support:1;
> >  };
> >  
> >  static struct fimd_driver_data s3c64xx_fimd_driver_data = {
> > @@ -138,6 +139,7 @@ static struct fimd_driver_data exynos4_fimd_driver_data = {
> >  	.lcdblk_bypass_shift = 1,
> >  	.has_shadowcon = 1,
> >  	.has_vtsel = 1,
> > +	.has_bgr_support = 1,
> >  };
> >  
> >  static struct fimd_driver_data exynos5_fimd_driver_data = {
> > @@ -149,6 +151,7 @@ static struct fimd_driver_data exynos5_fimd_driver_data = {
> >  	.has_vidoutcon = 1,
> >  	.has_vtsel = 1,
> >  	.has_dp_clk = 1,
> > +	.has_bgr_support = 1,
> >  };
> >  
> >  static struct fimd_driver_data exynos5420_fimd_driver_data = {
> > @@ -162,6 +165,7 @@ static struct fimd_driver_data exynos5420_fimd_driver_data = {
> >  	.has_vtsel = 1,
> >  	.has_mic_bypass = 1,
> >  	.has_dp_clk = 1,
> > +	.has_bgr_support = 1,
> >  };
> >  
> >  struct fimd_context {
> > @@ -226,6 +230,18 @@ static const uint32_t fimd_formats[] = {
> >  	DRM_FORMAT_ARGB8888,
> >  };
> >  
> > +static const uint32_t fimd_extended_formats[] = {
> > +	DRM_FORMAT_C8,
> > +	DRM_FORMAT_XRGB1555,
> > +	DRM_FORMAT_XBGR1555,
> > +	DRM_FORMAT_RGB565,
> > +	DRM_FORMAT_BGR565,
> > +	DRM_FORMAT_XRGB8888,
> > +	DRM_FORMAT_XBGR8888,
> > +	DRM_FORMAT_ARGB8888,
> > +	DRM_FORMAT_ABGR8888,
> > +};
> > +
> >  static const unsigned int capabilities[WINDOWS_NR] = {
> >  	0,
> >  	EXYNOS_DRM_PLANE_CAP_WIN_BLEND | EXYNOS_DRM_PLANE_CAP_PIX_BLEND,
> > @@ -673,21 +689,25 @@ static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
> >  		val |= WINCONx_BYTSWP;
> >  		break;
> >  	case DRM_FORMAT_XRGB1555:
> > +	case DRM_FORMAT_XBGR1555:
> >  		val |= WINCON0_BPPMODE_16BPP_1555;
> >  		val |= WINCONx_HAWSWP;
> >  		val |= WINCONx_BURSTLEN_16WORD;
> >  		break;
> >  	case DRM_FORMAT_RGB565:
> > +	case DRM_FORMAT_BGR565:
> >  		val |= WINCON0_BPPMODE_16BPP_565;
> >  		val |= WINCONx_HAWSWP;
> >  		val |= WINCONx_BURSTLEN_16WORD;
> >  		break;
> >  	case DRM_FORMAT_XRGB8888:
> > +	case DRM_FORMAT_XBGR8888:
> >  		val |= WINCON0_BPPMODE_24BPP_888;
> >  		val |= WINCONx_WSWP;
> >  		val |= WINCONx_BURSTLEN_16WORD;
> >  		break;
> >  	case DRM_FORMAT_ARGB8888:
> > +	case DRM_FORMAT_ABGR8888:
> >  	default:
> >  		val |= WINCON1_BPPMODE_25BPP_A1888;
> >  		val |= WINCONx_WSWP;
> > @@ -695,6 +715,18 @@ static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
> >  		break;
> >  	}
> >  
> > +	switch (pixel_format) {
> > +	case DRM_FORMAT_XBGR1555:
> > +	case DRM_FORMAT_XBGR8888:
> > +	case DRM_FORMAT_ABGR8888:
> > +	case DRM_FORMAT_BGR565:
> > +		writel(WIN_RGB_ORDER_REVERSE, ctx->regs + WIN_RGB_ORDER(win));
> > +		break;
> > +	default:
> > +		writel(WIN_RGB_ORDER_FORWARD, ctx->regs + WIN_RGB_ORDER(win));
> > +		break;
> > +	}
> > +
> >  	/*
> >  	 * Setting dma-burst to 16Word causes permanent tearing for very small
> >  	 * buffers, e.g. cursor buffer. Burst Mode switching which based on
> > @@ -1074,8 +1106,14 @@ static int fimd_bind(struct device *dev, struct device *master, void *data)
> >  	ctx->drm_dev = drm_dev;
> >  
> >  	for (i = 0; i < WINDOWS_NR; i++) {
> > -		ctx->configs[i].pixel_formats = fimd_formats;
> > -		ctx->configs[i].num_pixel_formats = ARRAY_SIZE(fimd_formats);
> > +		if (ctx->driver_data->has_bgr_support) {
> > +			ctx->configs[i].pixel_formats = fimd_extended_formats;
> > +			ctx->configs[i].num_pixel_formats = ARRAY_SIZE(fimd_extended_formats);
> > +		} else {
> > +			ctx->configs[i].pixel_formats = fimd_formats;
> > +			ctx->configs[i].num_pixel_formats = ARRAY_SIZE(fimd_formats);
> > +		}
> > +
> >  		ctx->configs[i].zpos = i;
> >  		ctx->configs[i].type = fimd_win_types[i];
> >  		ctx->configs[i].capabilities = capabilities[i];
> > diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h
> > index c4a93ce1de48..e6966d187591 100644
> > --- a/include/video/samsung_fimd.h
> > +++ b/include/video/samsung_fimd.h
> > @@ -476,6 +476,10 @@
> >   * 1111		-none-	 -none-   -none-   -none-    -none-
> >  */
> >  
> > +#define WIN_RGB_ORDER(_win)			(0x2020 + ((_win) * 4))
> > +#define WIN_RGB_ORDER_FORWARD			(0 << 11)
> > +#define WIN_RGB_ORDER_REVERSE			(1 << 11)
> > +
> >  /* FIMD Version 8 register offset definitions */
> >  #define FIMD_V8_VIDTCON0	0x20010
> >  #define FIMD_V8_VIDTCON1	0x20014