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
