[PATCH V2] drm/exynos: fimd: calculate the correct address offset

[Paul Menzel]

Dear Leela,


thank you for your patch.


Am Mittwoch, den 06.03.2013, 00:20 -0500 schrieb Leela Krishna Amudala:
> Calculate the correct address offset values for alpha and color key
> control registers and clear size control register for window 0

1. The *and* implies this should be split up into two patches, right?
2. Why was it incorrect before and why is it correct now?
3. What were the indications of the incorrect calculation (command line
to check?)? How can be verified that they are correct now?

Could you please send two patches with an updated commit message and the
comments below fixed?

> Signed-off-by: Leela Krishna Amudala <l.krishna at samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 9537761..78bab4a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -38,21 +38,22 @@
>  /* position control register for hardware window 0, 2 ~ 4.*/
>  #define VIDOSD_A(win)		(VIDOSD_BASE + 0x00 + (win) * 16)
>  #define VIDOSD_B(win)		(VIDOSD_BASE + 0x04 + (win) * 16)
> +/* size control register is avaliable only for windows 0, 1 and 2. */

*available

>  /* size control register for hardware window 0. */
>  #define VIDOSD_C_SIZE_W0	(VIDOSD_BASE + 0x08)
> -/* alpha control register for hardware window 1 ~ 4. */
> -#define VIDOSD_C(win)		(VIDOSD_BASE + 0x18 + (win) * 16)
> -/* size control register for hardware window 1 ~ 4. */
> +/* size control register for hardware window 1 ~ 2. */
>  #define VIDOSD_D(win)		(VIDOSD_BASE + 0x0C + (win) * 16)
> +/* alpha control register for hardware window 1 ~ 4. */
> +#define VIDOSD_C(win)		(VIDOSD_BASE + 0x08 + (win) * 16)

The commit message should explain why 0x18 is wrong and 0x08 is right.

>  #define VIDWx_BUF_START(win, buf)	(VIDW_BUF_START(buf) + (win) * 8)
>  #define VIDWx_BUF_END(win, buf)		(VIDW_BUF_END(buf) + (win) * 8)
>  #define VIDWx_BUF_SIZE(win, buf)	(VIDW_BUF_SIZE(buf) + (win) * 4)
>  
>  /* color key control register for hardware window 1 ~ 4. */
> -#define WKEYCON0_BASE(x)		((WKEYCON0 + 0x140) + (x * 8))
> +#define WKEYCON0_BASE(x)		((WKEYCON0 + 0x140) + ((x - 1) * 8))
>  /* color key value register for hardware window 1 ~ 4. */
> -#define WKEYCON1_BASE(x)		((WKEYCON1 + 0x140) + (x * 8))
> +#define WKEYCON1_BASE(x)		((WKEYCON1 + 0x140) + ((x - 1) * 8))

Same here.

>  /* FIMD has totally five hardware windows. */
>  #define WINDOWS_NR	5
> @@ -782,11 +783,14 @@ static void fimd_clear_win(struct fimd_context *ctx, int win)
>  	writel(0, ctx->regs + WINCON(win));
>  	writel(0, ctx->regs + VIDOSD_A(win));
>  	writel(0, ctx->regs + VIDOSD_B(win));
> -	writel(0, ctx->regs + VIDOSD_C(win));
> +	if (win != 0)
> +		writel(0, ctx->regs + VIDOSD_C(win));
>  
>  	if (win == 1 || win == 2)
>  		writel(0, ctx->regs + VIDOSD_D(win));
>  
> +	if (win == 0)
> +		writel(0, ctx->regs + VIDOSD_C_SIZE_W0);

Separate patch? Use a `switch` statement (or (else if`)?

>  	val = readl(ctx->regs + SHADOWCON);
>  	val &= ~SHADOWCON_WINx_PROTECT(win);
>  	writel(val, ctx->regs + SHADOWCON);


Thanks,

Paul
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130306/4d6d3508/attachment.pgp>