Re: [PATCH v5] drm/exynos: enable fimd clocks in probe before accessing fimd registers
Thanks Inki,
I removed fimd_clear_win from fimd_probe and verified this change in chrome
setup. Not seeing any noticeable difference.
I have posted another patch at:
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg31629.html
Thanks everybody for your review effort.
Regards,
Rahul Sharma.
On 27 May 2014 18:28, Inki Dae inki@samsung.com wrote:
On 2014년 05월 27일 18:55, Rahul Sharma wrote:
Gentle Reminder
On 26 May 2014 14:21, Rahul Sharma rahul.sha...@samsung.com wrote:
Hi Daniel,
On 26 May 2014 13:11, Daniel Kurtz djku...@chromium.org wrote:
On Mon, May 26, 2014 at 2:59 PM, Rahul Sharma rahul.sha...@samsung.com
wrote:
Hi Inki,
Please review this patch.
[snip]
+
+ ret = clk_prepare_enable(ctx-lcd_clk);
Hi Rahul,
Can you explain why exactly we are clearing windows here in probe(),
anyway?
I am not sure why it was added there but it is present since the first
version of this
file. Probably Inki can explain about this :). I can see the change
coming from his
first patch for adding drm fimd driver.
IIUC, bus_clk is the clock that enables FIMD register access, and
lcd_clk clocks the scan out engine.
Therefore, if we only need to read/write some registers, we only need
the bus_clk, not lcd_clk, right?
Correct, bus_clk should be sufficient to access the registers. But unless we
are confident about all implicit clock requirements in all SoCs, it is
safer to follow
the power_on/off sequence. This implementation is as good as DPMS on -
perform
reg operation - DPMS Off. It was same in the original version but
later clock enables
were moved out of the probe.
However, fimd_clear_win() actually clears per-window registers.
Writes to per-window registers typically do not take effect until the
next vblank.
Therefore we do would need to enable lcd_clk to ensure that these
changes take effect.
Furthermore, to ensure the window clear completes during probe(), we
would also need to synchronously wait for the next vblank here - but
only if FIMD scanout is actually enabled already, otherwise there will
never be a next scanout, so we must check for that first.
Lastly, waiting around for a vblank could take a while. Doing so in
probe() is not very friendly to boot up time, so the waiting should
probably be moved out of the main probe() thread into some sort of
asynchronous handler, which could then signal back when the clear is
complete.
Do you agree, or am I missing something?
I agree. There seems a room for improvement. But at present we have two
options,
either fix the current implementation and try to improve it as you mentioned
above. OR remove fimd_clear_win from probe if it is just a legacy code which
Just let's remove fimd_clear_win. it wouldn't need to disable all
hardware overlays at probe.
Thanks,
Inki Dae
is no more required.
@Inki, need your inputs here.
Regards,
Rahul Sharma.
Thanks,
-djk
+ if (ret) {
[snip]
+pm_put_err:
+ return ret;
}
static void fimd_unbind(struct device *dev, struct device *master,
--
1.7.9.5
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] drm/exynos: enable fimd clocks in probe before accessing fimd registers
Gentle Reminder
On 26 May 2014 14:21, Rahul Sharma rahul.sha...@samsung.com wrote:
Hi Daniel,
On 26 May 2014 13:11, Daniel Kurtz djku...@chromium.org wrote:
On Mon, May 26, 2014 at 2:59 PM, Rahul Sharma rahul.sha...@samsung.com
wrote:
Hi Inki,
Please review this patch.
[snip]
+
+ ret = clk_prepare_enable(ctx-lcd_clk);
Hi Rahul,
Can you explain why exactly we are clearing windows here in probe(),
anyway?
I am not sure why it was added there but it is present since the first
version of this
file. Probably Inki can explain about this :). I can see the change
coming from his
first patch for adding drm fimd driver.
IIUC, bus_clk is the clock that enables FIMD register access, and
lcd_clk clocks the scan out engine.
Therefore, if we only need to read/write some registers, we only need
the bus_clk, not lcd_clk, right?
Correct, bus_clk should be sufficient to access the registers. But unless we
are confident about all implicit clock requirements in all SoCs, it is
safer to follow
the power_on/off sequence. This implementation is as good as DPMS on -
perform
reg operation - DPMS Off. It was same in the original version but
later clock enables
were moved out of the probe.
However, fimd_clear_win() actually clears per-window registers.
Writes to per-window registers typically do not take effect until the
next vblank.
Therefore we do would need to enable lcd_clk to ensure that these
changes take effect.
Furthermore, to ensure the window clear completes during probe(), we
would also need to synchronously wait for the next vblank here - but
only if FIMD scanout is actually enabled already, otherwise there will
never be a next scanout, so we must check for that first.
Lastly, waiting around for a vblank could take a while. Doing so in
probe() is not very friendly to boot up time, so the waiting should
probably be moved out of the main probe() thread into some sort of
asynchronous handler, which could then signal back when the clear is
complete.
Do you agree, or am I missing something?
I agree. There seems a room for improvement. But at present we have two
options,
either fix the current implementation and try to improve it as you mentioned
above. OR remove fimd_clear_win from probe if it is just a legacy code which
is no more required.
@Inki, need your inputs here.
Regards,
Rahul Sharma.
Thanks,
-djk
+ if (ret) {
[snip]
+pm_put_err:
+ return ret;
}
static void fimd_unbind(struct device *dev, struct device *master,
--
1.7.9.5
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] drm/exynos: enable fimd clocks in probe before accessing fimd registers
On 2014년 05월 27일 18:55, Rahul Sharma wrote:
Gentle Reminder
On 26 May 2014 14:21, Rahul Sharma rahul.sha...@samsung.com wrote:
Hi Daniel,
On 26 May 2014 13:11, Daniel Kurtz djku...@chromium.org wrote:
On Mon, May 26, 2014 at 2:59 PM, Rahul Sharma rahul.sha...@samsung.com
wrote:
Hi Inki,
Please review this patch.
[snip]
+
+ ret = clk_prepare_enable(ctx-lcd_clk);
Hi Rahul,
Can you explain why exactly we are clearing windows here in probe(),
anyway?
I am not sure why it was added there but it is present since the first
version of this
file. Probably Inki can explain about this :). I can see the change
coming from his
first patch for adding drm fimd driver.
IIUC, bus_clk is the clock that enables FIMD register access, and
lcd_clk clocks the scan out engine.
Therefore, if we only need to read/write some registers, we only need
the bus_clk, not lcd_clk, right?
Correct, bus_clk should be sufficient to access the registers. But unless we
are confident about all implicit clock requirements in all SoCs, it is
safer to follow
the power_on/off sequence. This implementation is as good as DPMS on -
perform
reg operation - DPMS Off. It was same in the original version but
later clock enables
were moved out of the probe.
However, fimd_clear_win() actually clears per-window registers.
Writes to per-window registers typically do not take effect until the
next vblank.
Therefore we do would need to enable lcd_clk to ensure that these
changes take effect.
Furthermore, to ensure the window clear completes during probe(), we
would also need to synchronously wait for the next vblank here - but
only if FIMD scanout is actually enabled already, otherwise there will
never be a next scanout, so we must check for that first.
Lastly, waiting around for a vblank could take a while. Doing so in
probe() is not very friendly to boot up time, so the waiting should
probably be moved out of the main probe() thread into some sort of
asynchronous handler, which could then signal back when the clear is
complete.
Do you agree, or am I missing something?
I agree. There seems a room for improvement. But at present we have two
options,
either fix the current implementation and try to improve it as you mentioned
above. OR remove fimd_clear_win from probe if it is just a legacy code which
Just let's remove fimd_clear_win. it wouldn't need to disable all
hardware overlays at probe.
Thanks,
Inki Dae
is no more required.
@Inki, need your inputs here.
Regards,
Rahul Sharma.
Thanks,
-djk
+ if (ret) {
[snip]
+pm_put_err:
+ return ret;
}
static void fimd_unbind(struct device *dev, struct device *master,
--
1.7.9.5
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] drm/exynos: enable fimd clocks in probe before accessing fimd registers
Hi Inki,
Please review this patch.
Regards,
Rahul Sharma
On 23 May 2014 17:17, Rahul Sharma rahul.sha...@samsung.com wrote:
From: Rahul Sharma rahul.sha...@samsung.com
Fimd probe is accessing fimd Registers without enabling the fimd
gate clocks. If FIMD clocks are kept disabled in Uboot or disbaled
during kernel boottime, the system hangs during boottime.
This issue got surfaced when verifying with sysmmu enabled. Probe of
fimd Sysmmu enables the master clock before accessing sysmmu regs and
then disables. Later fimd probe tries to read the register without
enabling the clock which is wrong and hangs the system.
Signed-off-by: Rahul Sharma rahul.sha...@samsung.com
---
v5:
1) Added pm_runtime_get_sync to enable the Display power domain.
v4:
1) Added clk_disable for prev clock when clk_enable fails.
v3:
1) Added checks for clk_enable.
v2:
Rebase.
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 35
+-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index bd30d0c..6a30415 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -898,18 +898,51 @@ 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;
+ int win, ret;
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);
+ ret = pm_runtime_get_sync(ctx-dev);
+ if (ret) {
+ dev_err(dev, pm runtime get has failed.\n);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(ctx-bus_clk);
+ if (ret) {
+ dev_err(dev, bus clock enable failed.\n);
+ goto bus_clk_err;
+ }
+
+ ret = clk_prepare_enable(ctx-lcd_clk);
+ if (ret) {
+ dev_err(dev, lcd clock enable failed.\n);
+ goto lcd_clk_err;
+ }
+
for (win = 0; win WINDOWS_NR; win++)
fimd_clear_win(ctx, win);
+ clk_disable_unprepare(ctx-lcd_clk);
+ clk_disable_unprepare(ctx-bus_clk);
+
+ ret = pm_runtime_put_sync(ctx-dev);
+ if (ret) {
+ dev_err(dev, pm runtime put has failed.\n);
+ goto pm_put_err;
+ }
+
return 0;
+lcd_clk_err:
+ clk_disable_unprepare(ctx-bus_clk);
+bus_clk_err:
+ pm_runtime_put_sync(ctx-dev);
+pm_put_err:
+ return ret;
}
static void fimd_unbind(struct device *dev, struct device *master,
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] drm/exynos: enable fimd clocks in probe before accessing fimd registers
On Mon, May 26, 2014 at 2:59 PM, Rahul Sharma rahul.sha...@samsung.com wrote:
Hi Inki,
Please review this patch.
Regards,
Rahul Sharma
On 23 May 2014 17:17, Rahul Sharma rahul.sha...@samsung.com wrote:
From: Rahul Sharma rahul.sha...@samsung.com
Fimd probe is accessing fimd Registers without enabling the fimd
gate clocks. If FIMD clocks are kept disabled in Uboot or disbaled
during kernel boottime, the system hangs during boottime.
This issue got surfaced when verifying with sysmmu enabled. Probe of
fimd Sysmmu enables the master clock before accessing sysmmu regs and
then disables. Later fimd probe tries to read the register without
enabling the clock which is wrong and hangs the system.
Signed-off-by: Rahul Sharma rahul.sha...@samsung.com
---
v5:
1) Added pm_runtime_get_sync to enable the Display power domain.
v4:
1) Added clk_disable for prev clock when clk_enable fails.
v3:
1) Added checks for clk_enable.
v2:
Rebase.
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 35
+-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index bd30d0c..6a30415 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -898,18 +898,51 @@ 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;
+ int win, ret;
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);
+ ret = pm_runtime_get_sync(ctx-dev);
+ if (ret) {
+ dev_err(dev, pm runtime get has failed.\n);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(ctx-bus_clk);
+ if (ret) {
+ dev_err(dev, bus clock enable failed.\n);
+ goto bus_clk_err;
+ }
+
+ ret = clk_prepare_enable(ctx-lcd_clk);
Hi Rahul,
Can you explain why exactly we are clearing windows here in probe(), anyway?
IIUC, bus_clk is the clock that enables FIMD register access, and
lcd_clk clocks the scan out engine.
Therefore, if we only need to read/write some registers, we only need
the bus_clk, not lcd_clk, right?
However, fimd_clear_win() actually clears per-window registers.
Writes to per-window registers typically do not take effect until the
next vblank.
Therefore we do would need to enable lcd_clk to ensure that these
changes take effect.
Furthermore, to ensure the window clear completes during probe(), we
would also need to synchronously wait for the next vblank here - but
only if FIMD scanout is actually enabled already, otherwise there will
never be a next scanout, so we must check for that first.
Lastly, waiting around for a vblank could take a while. Doing so in
probe() is not very friendly to boot up time, so the waiting should
probably be moved out of the main probe() thread into some sort of
asynchronous handler, which could then signal back when the clear is
complete.
Do you agree, or am I missing something?
Thanks,
-djk
+ if (ret) {
+ dev_err(dev, lcd clock enable failed.\n);
+ goto lcd_clk_err;
+ }
+
for (win = 0; win WINDOWS_NR; win++)
fimd_clear_win(ctx, win);
+ clk_disable_unprepare(ctx-lcd_clk);
+ clk_disable_unprepare(ctx-bus_clk);
+
+ ret = pm_runtime_put_sync(ctx-dev);
+ if (ret) {
+ dev_err(dev, pm runtime put has failed.\n);
+ goto pm_put_err;
+ }
+
return 0;
+lcd_clk_err:
+ clk_disable_unprepare(ctx-bus_clk);
+bus_clk_err:
+ pm_runtime_put_sync(ctx-dev);
+pm_put_err:
+ return ret;
}
static void fimd_unbind(struct device *dev, struct device *master,
--
1.7.9.5
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] drm/exynos: enable fimd clocks in probe before accessing fimd registers
Hi Daniel,
On 26 May 2014 13:11, Daniel Kurtz djku...@chromium.org wrote:
On Mon, May 26, 2014 at 2:59 PM, Rahul Sharma rahul.sha...@samsung.com
wrote:
Hi Inki,
Please review this patch.
[snip]
+
+ ret = clk_prepare_enable(ctx-lcd_clk);
Hi Rahul,
Can you explain why exactly we are clearing windows here in probe(), anyway?
I am not sure why it was added there but it is present since the first
version of this
file. Probably Inki can explain about this :). I can see the change
coming from his
first patch for adding drm fimd driver.
IIUC, bus_clk is the clock that enables FIMD register access, and
lcd_clk clocks the scan out engine.
Therefore, if we only need to read/write some registers, we only need
the bus_clk, not lcd_clk, right?
Correct, bus_clk should be sufficient to access the registers. But unless we
are confident about all implicit clock requirements in all SoCs, it is
safer to follow
the power_on/off sequence. This implementation is as good as DPMS on - perform
reg operation - DPMS Off. It was same in the original version but
later clock enables
were moved out of the probe.
However, fimd_clear_win() actually clears per-window registers.
Writes to per-window registers typically do not take effect until the
next vblank.
Therefore we do would need to enable lcd_clk to ensure that these
changes take effect.
Furthermore, to ensure the window clear completes during probe(), we
would also need to synchronously wait for the next vblank here - but
only if FIMD scanout is actually enabled already, otherwise there will
never be a next scanout, so we must check for that first.
Lastly, waiting around for a vblank could take a while. Doing so in
probe() is not very friendly to boot up time, so the waiting should
probably be moved out of the main probe() thread into some sort of
asynchronous handler, which could then signal back when the clear is
complete.
Do you agree, or am I missing something?
I agree. There seems a room for improvement. But at present we have two options,
either fix the current implementation and try to improve it as you mentioned
above. OR remove fimd_clear_win from probe if it is just a legacy code which
is no more required.
@Inki, need your inputs here.
Regards,
Rahul Sharma.
Thanks,
-djk
+ if (ret) {
[snip]
+pm_put_err:
+ return ret;
}
static void fimd_unbind(struct device *dev, struct device *master,
--
1.7.9.5
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5] drm/exynos: enable fimd clocks in probe before accessing fimd registers
From: Rahul Sharma rahul.sha...@samsung.com
Fimd probe is accessing fimd Registers without enabling the fimd
gate clocks. If FIMD clocks are kept disabled in Uboot or disbaled
during kernel boottime, the system hangs during boottime.
This issue got surfaced when verifying with sysmmu enabled. Probe of
fimd Sysmmu enables the master clock before accessing sysmmu regs and
then disables. Later fimd probe tries to read the register without
enabling the clock which is wrong and hangs the system.
Signed-off-by: Rahul Sharma rahul.sha...@samsung.com
---
v5:
1) Added pm_runtime_get_sync to enable the Display power domain.
v4:
1) Added clk_disable for prev clock when clk_enable fails.
v3:
1) Added checks for clk_enable.
v2:
Rebase.
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 35 +-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index bd30d0c..6a30415 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -898,18 +898,51 @@ 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;
+ int win, ret;
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);
+ ret = pm_runtime_get_sync(ctx-dev);
+ if (ret) {
+ dev_err(dev, pm runtime get has failed.\n);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(ctx-bus_clk);
+ if (ret) {
+ dev_err(dev, bus clock enable failed.\n);
+ goto bus_clk_err;
+ }
+
+ ret = clk_prepare_enable(ctx-lcd_clk);
+ if (ret) {
+ dev_err(dev, lcd clock enable failed.\n);
+ goto lcd_clk_err;
+ }
+
for (win = 0; win WINDOWS_NR; win++)
fimd_clear_win(ctx, win);
+ clk_disable_unprepare(ctx-lcd_clk);
+ clk_disable_unprepare(ctx-bus_clk);
+
+ ret = pm_runtime_put_sync(ctx-dev);
+ if (ret) {
+ dev_err(dev, pm runtime put has failed.\n);
+ goto pm_put_err;
+ }
+
return 0;
+lcd_clk_err:
+ clk_disable_unprepare(ctx-bus_clk);
+bus_clk_err:
+ pm_runtime_put_sync(ctx-dev);
+pm_put_err:
+ return ret;
}
static void fimd_unbind(struct device *dev, struct device *master,
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
