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@xxxxxxxxxxx> > --- > 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:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
