* Re: [PATCH V2] drm/exynos: fimd: calculate the correct address offset
2013-03-06 5:20 [PATCH V2] drm/exynos: fimd: calculate the correct address offset Leela Krishna Amudala
@ 2013-03-06 9:15 ` Paul Menzel
2013-03-06 10:19 ` Joonyoung Shim
1 sibling, 0 replies; 3+ messages in thread
From: Paul Menzel @ 2013-03-06 9:15 UTC (permalink / raw)
To: Leela Krishna Amudala; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 3321 bytes --]
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@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
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH V2] drm/exynos: fimd: calculate the correct address offset
2013-03-06 5:20 [PATCH V2] drm/exynos: fimd: calculate the correct address offset Leela Krishna Amudala
2013-03-06 9:15 ` Paul Menzel
@ 2013-03-06 10:19 ` Joonyoung Shim
1 sibling, 0 replies; 3+ messages in thread
From: Joonyoung Shim @ 2013-03-06 10:19 UTC (permalink / raw)
To: Leela Krishna Amudala; +Cc: dri-devel
On 03/06/2013 02:20 PM, Leela Krishna Amudala wrote:
> Calculate the correct address offset values for alpha and color key
> control registers and clear size control register for window 0
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@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. */
> /* 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)
>
> #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))
>
> /* 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);
> val = readl(ctx->regs + SHADOWCON);
> val &= ~SHADOWCON_WINx_PROTECT(win);
> writel(val, ctx->regs + SHADOWCON);
As i said at v1 patch, i prefer to remove VIDOSD_C_SIZE_W0 and it makes
codes are simple. Even if VIDOSD_C_SIZE_W0 is defined, it's not good
that VIDOSD_C(0) can mean VIDOSD_C_SIZE_W0.
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 7514bee..a4356b4 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -40,11 +40,12 @@
/* 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 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 windows 0 and alpha control register
+ * for hardware windows 1 ~ 4
+ */
+#define VIDOSD_C(win) (VIDOSD_BASE + 0x08 + (win) * 16)
+/* size control register for hardware window 1 ~ 2. */
#define VIDOSD_D(win) (VIDOSD_BASE + 0x0C + (win) * 16)
#define VIDWx_BUF_START(win, buf) (VIDW_BUF_START(buf) + (win) * 8)
@@ -52,9 +53,9 @@
#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))
/* FIMD has totally five hardware windows. */
#define WINDOWS_NR 5
@@ -583,7 +584,7 @@ static void fimd_win_commit(struct device *dev, int zpos)
if (win != 3 && win != 4) {
u32 offset = VIDOSD_D(win);
if (win == 0)
- offset = VIDOSD_C_SIZE_W0;
+ offset = VIDOSD_C(win);
val = win_data->ovl_width * win_data->ovl_height;
writel(val, ctx->regs + offset);
^ permalink raw reply related [flat|nested] 3+ messages in thread