| Message ID | 1401257471-31994-1-git-send-email-rahul.sharma@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Rahul, On 05/28/2014 08:11 AM, Rahul Sharma wrote: > System hangs when FIMD registers are accessed to disable > hardware overlays. This is because of the clocks which are > not enabled before register access. > > 'Hardware overlay disable' is cleaned from the FIMD probe. > > Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com> This patch causes regression on some exynos4210-universal_c210 devices, everything works expect colors are incorrect - it seems blue component is very dark, almost black. Regards Andrzej > --- > Based on exynos-drm-next branch. > > drivers/gpu/drm/exynos/exynos_drm_fimd.c | 17 ----------------- > 1 file changed, 17 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > index bd30d0c..2ec634f 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > @@ -703,19 +703,6 @@ static void fimd_win_disable(struct exynos_drm_manager *mgr, int zpos) > win_data->enabled = false; > } > > -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 == 1 || win == 2) > - writel(0, ctx->regs + VIDOSD_D(win)); > - > - fimd_shadow_protect_win(ctx, win, false); > -} > - > static void fimd_window_suspend(struct exynos_drm_manager *mgr) > { > struct fimd_context *ctx = mgr->ctx; > @@ -898,16 +885,12 @@ static int fimd_bind(struct device *dev, struct device *master, void *data) > { > struct fimd_context *ctx = fimd_manager.ctx; > struct drm_device *drm_dev = data; > - int win; > > fimd_mgr_initialize(&fimd_manager, drm_dev); > exynos_drm_crtc_create(&fimd_manager); > if (ctx->display) > exynos_drm_create_enc_conn(drm_dev, ctx->display); > > - for (win = 0; win < WINDOWS_NR; win++) > - fimd_clear_win(ctx, win); > - > return 0; > > } >
On 2 June 2014 14:41, Andrzej Hajda <a.hajda@samsung.com> wrote: > Hi Rahul, > > On 05/28/2014 08:11 AM, Rahul Sharma wrote: >> System hangs when FIMD registers are accessed to disable >> hardware overlays. This is because of the clocks which are >> not enabled before register access. >> >> 'Hardware overlay disable' is cleaned from the FIMD probe. >> >> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com> > > This patch causes regression on some exynos4210-universal_c210 devices, > everything works expect colors are incorrect - it seems blue component > is very dark, almost black. > Oh.... Sorry for that. I did not see any problem on 5250/5420/5800. I do not have setup for 4210. Better we should revert this patch. Would you please help me by verifying the following patch on 4210? This is an alternate solution to the same problem. http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg31426.html Thanks Andrej, for bringing it to notice. Regards, Rahul Sharma > Regards > Andrzej > >> --- >> Based on exynos-drm-next branch. >> >> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 17 ----------------- >> 1 file changed, 17 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c >> index bd30d0c..2ec634f 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c >> @@ -703,19 +703,6 @@ static void fimd_win_disable(struct exynos_drm_manager *mgr, int zpos) >> win_data->enabled = false; >> } >> >> -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 == 1 || win == 2) >> - writel(0, ctx->regs + VIDOSD_D(win)); >> - >> - fimd_shadow_protect_win(ctx, win, false); >> -} >> - >> static void fimd_window_suspend(struct exynos_drm_manager *mgr) >> { >> struct fimd_context *ctx = mgr->ctx; >> @@ -898,16 +885,12 @@ static int fimd_bind(struct device *dev, struct device *master, void *data) >> { >> struct fimd_context *ctx = fimd_manager.ctx; >> struct drm_device *drm_dev = data; >> - int win; >> >> fimd_mgr_initialize(&fimd_manager, drm_dev); >> exynos_drm_crtc_create(&fimd_manager); >> if (ctx->display) >> exynos_drm_create_enc_conn(drm_dev, ctx->display); >> >> - for (win = 0; win < WINDOWS_NR; win++) >> - fimd_clear_win(ctx, win); >> - >> return 0; >> >> } >> >
Hi Rahul, Andrzej, On 02.06.2014 11:42, Rahul Sharma wrote: > On 2 June 2014 14:41, Andrzej Hajda <a.hajda@samsung.com> wrote: >> Hi Rahul, >> >> On 05/28/2014 08:11 AM, Rahul Sharma wrote: >>> System hangs when FIMD registers are accessed to disable >>> hardware overlays. This is because of the clocks which are >>> not enabled before register access. >>> >>> 'Hardware overlay disable' is cleaned from the FIMD probe. >>> >>> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com> >> >> This patch causes regression on some exynos4210-universal_c210 devices, >> everything works expect colors are incorrect - it seems blue component >> is very dark, almost black. >> > > Oh.... Sorry for that. I did not see any problem on 5250/5420/5800. I do not > have setup for 4210. Better we should revert this patch. > > Would you please help me by verifying the following patch on 4210? This > is an alternate solution to the same problem. > > http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg31426.html > > Thanks Andrej, for bringing it to notice. I don't see how this patch could introduce such regression, as all the affected registers seem to be properly reconfigured in fimd_win_commit() anyway. IMHO instead of reverting the patch, this issue should be investigated and fixed properly. Best regards, Tomasz
Hi Rahul, On 06/02/2014 06:42 PM, Rahul Sharma wrote: > On 2 June 2014 14:41, Andrzej Hajda <a.hajda@samsung.com> wrote: >> Hi Rahul, >> >> On 05/28/2014 08:11 AM, Rahul Sharma wrote: >>> System hangs when FIMD registers are accessed to disable >>> hardware overlays. This is because of the clocks which are >>> not enabled before register access. >>> >>> 'Hardware overlay disable' is cleaned from the FIMD probe. >>> >>> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com> >> >> This patch causes regression on some exynos4210-universal_c210 devices, >> everything works expect colors are incorrect - it seems blue component >> is very dark, almost black. >> > > Oh.... Sorry for that. I did not see any problem on 5250/5420/5800. I do not > have setup for 4210. Better we should revert this patch. > > Would you please help me by verifying the following patch on 4210? This > is an alternate solution to the same problem. > > http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg31426.html In my humble opinion, the patch is required to modify like below: + ret = pm_runtime_get_sync(ctx->dev); + if (ret < 0) { When the bootloader enabled FIMD to show logo(boot image) or animation, this function pm_runtime_get_sync() in fimd_bind() would return 1. Thank you. Best regards YJ > > Thanks Andrej, for bringing it to notice. > > Regards, > Rahul Sharma > >> Regards >> Andrzej >> >>> --- >>> Based on exynos-drm-next branch. >>> >>> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 17 ----------------- >>> 1 file changed, 17 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>> index bd30d0c..2ec634f 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>> @@ -703,19 +703,6 @@ static void fimd_win_disable(struct exynos_drm_manager *mgr, int zpos) >>> win_data->enabled = false; >>> } >>> >>> -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 == 1 || win == 2) >>> - writel(0, ctx->regs + VIDOSD_D(win)); >>> - >>> - fimd_shadow_protect_win(ctx, win, false); >>> -} >>> - >>> static void fimd_window_suspend(struct exynos_drm_manager *mgr) >>> { >>> struct fimd_context *ctx = mgr->ctx; >>> @@ -898,16 +885,12 @@ static int fimd_bind(struct device *dev, struct device *master, void *data) >>> { >>> struct fimd_context *ctx = fimd_manager.ctx; >>> struct drm_device *drm_dev = data; >>> - int win; >>> >>> fimd_mgr_initialize(&fimd_manager, drm_dev); >>> exynos_drm_crtc_create(&fimd_manager); >>> if (ctx->display) >>> exynos_drm_create_enc_conn(drm_dev, ctx->display); >>> >>> - for (win = 0; win < WINDOWS_NR; win++) >>> - fimd_clear_win(ctx, win); >>> - >>> return 0; >>> >>> } >>> >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
On 06/02/2014 12:11 PM, Tomasz Figa wrote: > Hi Rahul, Andrzej, > > On 02.06.2014 11:42, Rahul Sharma wrote: >> On 2 June 2014 14:41, Andrzej Hajda <a.hajda@samsung.com> wrote: >>> Hi Rahul, >>> >>> On 05/28/2014 08:11 AM, Rahul Sharma wrote: >>>> System hangs when FIMD registers are accessed to disable >>>> hardware overlays. This is because of the clocks which are >>>> not enabled before register access. >>>> >>>> 'Hardware overlay disable' is cleaned from the FIMD probe. >>>> >>>> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com> >>> >>> This patch causes regression on some exynos4210-universal_c210 devices, >>> everything works expect colors are incorrect - it seems blue component >>> is very dark, almost black. >>> >> >> Oh.... Sorry for that. I did not see any problem on 5250/5420/5800. I do not >> have setup for 4210. Better we should revert this patch. >> >> Would you please help me by verifying the following patch on 4210? This >> is an alternate solution to the same problem. >> >> http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg31426.html >> >> Thanks Andrej, for bringing it to notice. > > I don't see how this patch could introduce such regression, as all the > affected registers seem to be properly reconfigured in fimd_win_commit() > anyway. > > IMHO instead of reverting the patch, this issue should be investigated > and fixed properly. > > Best regards, > Tomasz > I am looking at the problem, it is quite strange as it happens only on one of two targets I have access to. Anyway it seems that something should be added to fimd initialization sequence if we want to remove hw accessing code from probe. Regards Andrzej
On 2014? 06? 02? 18:42, Rahul Sharma wrote: > On 2 June 2014 14:41, Andrzej Hajda <a.hajda@samsung.com> wrote: >> Hi Rahul, >> >> On 05/28/2014 08:11 AM, Rahul Sharma wrote: >>> System hangs when FIMD registers are accessed to disable >>> hardware overlays. This is because of the clocks which are >>> not enabled before register access. >>> >>> 'Hardware overlay disable' is cleaned from the FIMD probe. >>> >>> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com> >> >> This patch causes regression on some exynos4210-universal_c210 devices, >> everything works expect colors are incorrect - it seems blue component >> is very dark, almost black. That issue would be because your boot-loader enabled a hardware overlay but kernel driver enables another hardware overlay, which would also be why it happens only on one target of your targets, and why it works correctly by cleaning hardware overlays at fimd probe. >> > > Oh.... Sorry for that. I did not see any problem on 5250/5420/5800. I do not > have setup for 4210. Better we should revert this patch. > No, it should work correctly as is because fimd overlays will be reconfigured at booting time by register_framebuffer() if console framebuffer is enabled. Otherwise, a master process, X or Weston, will reconfigure fimd overlays after kernel booting. Thanks, Inki Dae > Would you please help me by verifying the following patch on 4210? This > is an alternate solution to the same problem. > > http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg31426.html > > Thanks Andrej, for bringing it to notice. > > Regards, > Rahul Sharma > >> Regards >> Andrzej >> >>> --- >>> Based on exynos-drm-next branch. >>> >>> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 17 ----------------- >>> 1 file changed, 17 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>> index bd30d0c..2ec634f 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c >>> @@ -703,19 +703,6 @@ static void fimd_win_disable(struct exynos_drm_manager *mgr, int zpos) >>> win_data->enabled = false; >>> } >>> >>> -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 == 1 || win == 2) >>> - writel(0, ctx->regs + VIDOSD_D(win)); >>> - >>> - fimd_shadow_protect_win(ctx, win, false); >>> -} >>> - >>> static void fimd_window_suspend(struct exynos_drm_manager *mgr) >>> { >>> struct fimd_context *ctx = mgr->ctx; >>> @@ -898,16 +885,12 @@ static int fimd_bind(struct device *dev, struct device *master, void *data) >>> { >>> struct fimd_context *ctx = fimd_manager.ctx; >>> struct drm_device *drm_dev = data; >>> - int win; >>> >>> fimd_mgr_initialize(&fimd_manager, drm_dev); >>> exynos_drm_crtc_create(&fimd_manager); >>> if (ctx->display) >>> exynos_drm_create_enc_conn(drm_dev, ctx->display); >>> >>> - for (win = 0; win < WINDOWS_NR; win++) >>> - fimd_clear_win(ctx, win); >>> - >>> return 0; >>> >>> } >>> >> >
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index bd30d0c..2ec634f 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c @@ -703,19 +703,6 @@ static void fimd_win_disable(struct exynos_drm_manager *mgr, int zpos) win_data->enabled = false; } -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 == 1 || win == 2) - writel(0, ctx->regs + VIDOSD_D(win)); - - fimd_shadow_protect_win(ctx, win, false); -} - static void fimd_window_suspend(struct exynos_drm_manager *mgr) { struct fimd_context *ctx = mgr->ctx; @@ -898,16 +885,12 @@ static int fimd_bind(struct device *dev, struct device *master, void *data) { struct fimd_context *ctx = fimd_manager.ctx; struct drm_device *drm_dev = data; - int win; fimd_mgr_initialize(&fimd_manager, drm_dev); exynos_drm_crtc_create(&fimd_manager); if (ctx->display) exynos_drm_create_enc_conn(drm_dev, ctx->display); - for (win = 0; win < WINDOWS_NR; win++) - fimd_clear_win(ctx, win); - return 0; }
System hangs when FIMD registers are accessed to disable hardware overlays. This is because of the clocks which are not enabled before register access. 'Hardware overlay disable' is cleaned from the FIMD probe. Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com> --- Based on exynos-drm-next branch. drivers/gpu/drm/exynos/exynos_drm_fimd.c | 17 ----------------- 1 file changed, 17 deletions(-)