[PATCH -v2 10/14] drm/exynos: atomic phase 1: add atomic_begin()/atomic_flush()
Inki Dae
inki.dae at samsung.com
Mon Feb 9 06:53:18 PST 2015
Hi,
I'm merging this patch series but I found two issues. One is already
pointed out.
On 2015년 02월 07일 04:37, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
>
> Add CRTC callbacks .atomic_begin() .atomic_flush(). On exynos they
> unprotect the windows before the commit and protects it after based on
> a plane mask tha store which plane will be updated.
>
> For that we create two new exynos_crtc callbacks: .win_protect() and
> .win_unprotect(). The only driver that implement those now is FIMD.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> ---
> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 34 +++++++++++++++++++++
> drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 ++++
> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 49 ++++++++++++++++++++-----------
> drivers/gpu/drm/exynos/exynos_drm_plane.c | 4 +++
> 4 files changed, 76 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index 1c0d936..5e7c13e 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -153,6 +153,38 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
> }
> }
>
> +static void exynos_crtc_atomic_begin(struct drm_crtc *crtc)
> +{
> + struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> + struct drm_plane *plane;
> + int index = 0;
> +
> + list_for_each_entry(plane, &crtc->dev->mode_config.plane_list, head) {
> + if (exynos_crtc->ops->win_protect &&
> + exynos_crtc->plane_mask & (0x01 << index))
> + exynos_crtc->ops->win_protect(exynos_crtc, index);
> +
> + index++;
> + }
> +}
> +
> +static void exynos_crtc_atomic_flush(struct drm_crtc *crtc)
> +{
> + struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> + struct drm_plane *plane;
> + int index = 0;
> +
> + list_for_each_entry(plane, &crtc->dev->mode_config.plane_list, head) {
> + if (exynos_crtc->ops->win_unprotect &&
> + exynos_crtc->plane_mask & (0x01 << index))
> + exynos_crtc->ops->win_unprotect(exynos_crtc, index);
> +
> + index++;
> + }
> +
> + exynos_crtc->plane_mask = 0;
> +}
> +
> static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
> .dpms = exynos_drm_crtc_dpms,
> .prepare = exynos_drm_crtc_prepare,
> @@ -161,6 +193,8 @@ static struct drm_crtc_helper_funcs exynos_crtc_helper_funcs = {
> .mode_set = exynos_drm_crtc_mode_set,
> .mode_set_base = exynos_drm_crtc_mode_set_base,
> .disable = exynos_drm_crtc_disable,
> + .atomic_begin = exynos_crtc_atomic_begin,
> + .atomic_flush = exynos_crtc_atomic_flush,
> };
>
> static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index ab82772..f025a54 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -175,6 +175,8 @@ struct exynos_drm_display {
> * hardware overlay is updated.
> * @win_commit: apply hardware specific overlay data to registers.
> * @win_disable: disable hardware specific overlay.
> + * @win_protect: protect hardware specific window.
> + * @win_unprotect: unprotect hardware specific window.
> * @te_handler: trigger to transfer video image at the tearing effect
> * synchronization signal if there is a page flip request.
> */
> @@ -190,6 +192,8 @@ struct exynos_drm_crtc_ops {
> void (*wait_for_vblank)(struct exynos_drm_crtc *crtc);
> void (*win_commit)(struct exynos_drm_crtc *crtc, unsigned int zpos);
> void (*win_disable)(struct exynos_drm_crtc *crtc, unsigned int zpos);
> + void (*win_protect)(struct exynos_drm_crtc *crtc, unsigned int zpos);
> + void (*win_unprotect)(struct exynos_drm_crtc *crtc, unsigned int zpos);
> void (*te_handler)(struct exynos_drm_crtc *crtc);
> };
>
> @@ -206,6 +210,7 @@ struct exynos_drm_crtc_ops {
> * we can refer to the crtc to current hardware interrupt occurred through
> * this pipe value.
> * @dpms: store the crtc dpms value
> + * @plane_mask: store planes to be updated on atomic modesetting
> * @event: vblank event that is currently queued for flip
> * @ops: pointer to callbacks for exynos drm specific functionality
> * @ctx: A pointer to the crtc's implementation specific context
> @@ -215,6 +220,7 @@ struct exynos_drm_crtc {
> enum exynos_drm_output_type type;
> unsigned int pipe;
> unsigned int dpms;
> + unsigned int plane_mask;
> wait_queue_head_t pending_flip_queue;
> struct drm_pending_vblank_event *event;
> struct exynos_drm_crtc_ops *ops;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 990ead434..bea70f6 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -590,6 +590,16 @@ static void fimd_shadow_protect_win(struct fimd_context *ctx,
> {
> u32 reg, bits, val;
>
> + /*
> + * SHADOWCON/PRTCON register is used for enabling timing.
> + *
> + * for example, once only width value of a register is set,
> + * if the dma is started then fimd hardware could malfunction so
> + * with protect window setting, the register fields with prefix '_F'
> + * wouldn't be updated at vsync also but updated once unprotect window
> + * is set.
> + */
> +
> if (ctx->driver_data->has_shadowcon) {
> reg = SHADOWCON;
> bits = SHADOWCON_WINx_PROTECT(win);
> @@ -628,20 +638,6 @@ static void fimd_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
> return;
> }
>
> - /*
> - * SHADOWCON/PRTCON register is used for enabling timing.
> - *
> - * for example, once only width value of a register is set,
> - * if the dma is started then fimd hardware could malfunction so
> - * with protect window setting, the register fields with prefix '_F'
> - * wouldn't be updated at vsync also but updated once unprotect window
> - * is set.
> - */
> -
> - /* protect windows */
> - fimd_shadow_protect_win(ctx, win, true);
You removed to protect shadow register updating at vsync so fimd
hardware could malfunction if setcrtc/page flip/setplane are requested
by userspace. Actually, when I run modetest repeatedly, I could see
overlay isn't rarely turned on.
So how about calling win_protect/unprotect callbacks like below? If you
are ok, I can modify it and merge it.
static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
{
...
if (exynos_crtc->ops->win_protect)
exynos_crtc->ops->win_protect(exynos_crtc,
exynos_plane->zpos);
if (exynos_crtc->ops->win_commit)
exynos_crtc->ops->win_commit(exynos_crtc,
exynos_plane->zpos);
if (exynos_crtc->ops->win_unprotect)
exynos_crtc->ops->win_unprotect(exynos_crtc,
exynos_plane->zpos);
...
}
And other, I could see below warning messages when I tried to unload
Exynos dsi module with a command, "echo 11c80000.dsi
>/sys/bus/platform/drivers/exynos-dsi/unbind"
# echo 11c80000.dsi >/sys/bus/platform/drivers/exynos-dsi/unbind
[ 230.800467] Console: switching to colour dummy device 80x30
[ 230.805005] drm_kms_helper: drm: unregistered panic notifier
[ 230.849266] ------------[ cut here ]------------
[ 230.852516] WARNING: CPU: 3 PID: 1428 at
drivers/gpu/drm/drm_crtc.c:5455 drm_mode_config_cleanup+0x1f4/0x1fc()
[ 230.862532] Modules linked in:
[ 230.865472] CPU: 3 PID: 1428 Comm: sh Not tainted
3.19.0-rc6-161746-g9d96aee-dirty #1243
[ 230.873564] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 230.879677] [<c0014430>] (unwind_backtrace) from [<c001158c>]
(show_stack+0x10/0x14)
[ 230.887370] [<c001158c>] (show_stack) from [<c047cfbc>]
(dump_stack+0x84/0xc4)
[ 230.894580] [<c047cfbc>] (dump_stack) from [<c0020b00>]
(warn_slowpath_common+0x80/0xb0)
[ 230.902639] [<c0020b00>] (warn_slowpath_common) from [<c0020bcc>]
(warn_slowpath_null+0x1c/0x24)
[ 230.911405] [<c0020bcc>] (warn_slowpath_null) from [<c0274370>]
(drm_mode_config_cleanup+0x1f4/0x1fc)
[ 230.920608] [<c0274370>] (drm_mode_config_cleanup) from [<c0282970>]
(exynos_drm_unload+0x38/0x4c)
[ 230.929548] [<c0282970>] (exynos_drm_unload) from [<c026c250>]
(drm_dev_unregister+0x24/0x98)
[ 230.938050] [<c026c250>] (drm_dev_unregister) from [<c026c4dc>]
(drm_put_dev+0x2c/0x60)
[ 230.946046] [<c026c4dc>] (drm_put_dev) from [<c029b1d0>]
(take_down_master+0x24/0x44)
[ 230.953861] [<c029b1d0>] (take_down_master) from [<c029b328>]
(component_del+0x90/0xd0)
[ 230.961850] [<c029b328>] (component_del) from [<c0288184>]
(exynos_dsi_remove+0x18/0x2c)
[ 230.969919] [<c0288184>] (exynos_dsi_remove) from [<c02a10cc>]
(platform_drv_remove+0x18/0x30)
[ 230.978505] [<c02a10cc>] (platform_drv_remove) from [<c029f58c>]
(__device_release_driver+0x70/0xc4)
[ 230.987615] [<c029f58c>] (__device_release_driver) from [<c029f5fc>]
(device_release_driver+0x1c/0x28)
[ 230.996904] [<c029f5fc>] (device_release_driver) from [<c029e730>]
(unbind_store+0x78/0xf8)
[ 231.005240] [<c029e730>] (unbind_store) from [<c0125f1c>]
(kernfs_fop_write+0xb8/0x19c)
[ 231.013223] [<c0125f1c>] (kernfs_fop_write) from [<c00cd830>]
(vfs_write+0xa0/0x1ac)
[ 231.020946] [<c00cd830>] (vfs_write) from [<c00cde88>]
(SyS_write+0x44/0x9c)
[ 231.027983] [<c00cde88>] (SyS_write) from [<c000e6e0>]
(ret_fast_syscall+0x0/0x34)
[ 231.035523] ---[ end trace 954377a3aab4ab59 ]---
[ 231.042414] lcd0-power-domain: Power-off latency exceeded, new value
201333 ns
This warning would mean a fb object leak and while I have a review, I
couldn't find why it causes the leak.
However, I'd like to merge this patch series this time and fix this
issue at RC for phase 3.
Thanks,
Inki Dae
More information about the dri-devel
mailing list