From patchwork Fri Dec 11 11:27:07 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Szyprowski X-Patchwork-Id: 7828581 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 29419BEEE1 for ; Fri, 11 Dec 2015 11:27:18 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id B8DBA205B3 for ; Fri, 11 Dec 2015 11:27:16 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id BA856205B1 for ; Fri, 11 Dec 2015 11:27:14 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id D2AA76ED7F; Fri, 11 Dec 2015 03:27:13 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mailout3.w1.samsung.com (mailout3.w1.samsung.com [210.118.77.13]) by gabe.freedesktop.org (Postfix) with ESMTPS id 525196ED7D for ; Fri, 11 Dec 2015 03:27:12 -0800 (PST) MIME-version: 1.0 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout3.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0NZ600LKAYH8DC20@mailout3.w1.samsung.com> for dri-devel@lists.freedesktop.org; Fri, 11 Dec 2015 11:27:08 +0000 (GMT) X-AuditID: cbfec7f5-f79b16d000005389-f4-566ab30c4997 Received: from eusync4.samsung.com ( [203.254.199.214]) by eucpsbgm2.samsung.com (EUCPMTA) with SMTP id A0.6D.21385.C03BA665; Fri, 11 Dec 2015 11:27:08 +0000 (GMT) Received: from [106.116.147.30] by eusync4.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0NZ600G2RYH72490@eusync4.samsung.com>; Fri, 11 Dec 2015 11:27:08 +0000 (GMT) Subject: Re: [PATCH v2 06/22] drm/exynos: move dma_addr attribute from exynos plane to exynos fb To: Inki Dae , dri-devel@lists.freedesktop.org, linux-samsung-soc@vger.kernel.org References: <1448891617-18830-1-git-send-email-m.szyprowski@samsung.com> <1448891617-18830-7-git-send-email-m.szyprowski@samsung.com> <566978A1.60302@samsung.com> <566A9115.6040109@samsung.com> <566A96CB.4020309@samsung.com> <566A9E1C.3020404@samsung.com> From: Marek Szyprowski Message-id: <566AB30B.9070900@samsung.com> Date: Fri, 11 Dec 2015 12:27:07 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 In-reply-to: <566A9E1C.3020404@samsung.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrMLMWRmVeSWpSXmKPExsVy+t/xa7o8m7PCDH5fELO4te4cq8XGGetZ La58fc9msfPBLnaLSfcnsFi8ebuGyeLFvYssFq9fGFrMOL+PyWLG5JdsFm2rP7A6cHvc7z7O 5PHvGLvHlv677B47J+1l8ujbsorR4/MmuQC2KC6blNSczLLUIn27BK6Mma0H2QveZ1TsPjuF uYFxakgXIyeHhICJxJeXp1kgbDGJC/fWs3UxcnEICSxllFj/YxcjSIJXQFDix+R7QEUcHMwC 8hJHLmWDhJkFzIB6D7NC1D9nlDi07QJYvbBAkkT7v4VsILaIQKbEy+b1zBBFrUwSTTsWMII4 zAKfmSTm35oNtppNwFCi620XG8Q2LYmvv68zgdgsAqoSszauAYuLCsRIPF68lRXE5hTQllh+ sYl1AqPALCQHzkI4cBaSAxcwMq9iFE0tTS4oTkrPNdIrTswtLs1L10vOz93ECImKrzsYlx6z OsQowMGoxMO7gCMrTIg1say4MvcQowQHs5II768NQCHelMTKqtSi/Pii0pzU4kOM0hwsSuK8 M3e9DxESSE8sSc1OTS1ILYLJMnFwSjUwln6XcAjoTftcsDNrttWOvs9sskWT1z93OP9w0rJP 63/peR6Vmir9USCd90LSqhKLXAlbs7X3ZZ9Ib9tz+Mmv+F360/rrzE8o6sswX65Xuapx2Td2 75WM5r0J6rsXB9Re3XRXdMMi9Yrfcxecu+roxTazXzqk+qh/6q+LH5TnXT0+i3m2qoLoYiWW 4oxEQy3mouJEAF5+OgyGAgAA Cc: Javier Martinez Canillas , Krzysztof Kozlowski , Tobias Jakobi , Bartlomiej Zolnierkiewicz , Seung-Woo Kim , Andrzej Hajda X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Inki, On 2015-12-11 10:57, Inki Dae wrote: > Hi Marek, > > 2015? 12? 11? 18:26? Marek Szyprowski ?(?) ? ?: >> Hi Inki, >> >> On 2015-12-11 10:02, Inki Dae wrote: >>> Hi Marek, >>> >>> I found out why NULL point access happened. That was incurred by below your patch, >>> [PATCH] drm/exynos: move dma_addr attribute from exynos plane to exynos fb >>> >>> When another crtc driver is hotplugged in runtime such as HDMI or VIDI, >>> the drm frambuffer object of fb_helper is set to the plane state of the new crtc driver >>> so the driver should get the drm framebuffer object from the plane's state or >>> exynos_plane->pending_fb which is set by exynos_plane_atomic_update function. >>> >>> After that, I think the drm framebuffer should be set to drm plane as a current fb >>> which would be scanned out. >>> >>> Anyway, I can fix it like below if you are ok. >>> >>> Thanks, >>> Inki Dae >>> >>> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c >>> @@ -137,7 +137,7 @@ static void vidi_update_plane(struct exynos_drm_crtc *crtc, >>> if (ctx->suspended) >>> return; >>> - addr = exynos_drm_fb_dma_addr(plane->base.fb, 0); >>> + addr = exynos_drm_fb_dma_addr(plane->pending_fb, 0); >>> DRM_DEBUG_KMS("dma_addr = %pad\n", &addr); >>> >>> + plane->base.fb = plane->pending_fb; >> plane->base.fb should not be modified. I think that the following fix is more > Could you explain why plane->base.fb shouldn't be modified? All 'base' classes are modified by DRM core and there should be no need to modify them from the drivers. I've checked this issue and the proper fix for is the following code: if (ctx->vblank_on) plane->base.fb is updated from the core once all crtc/plane atomic update calls finishes. The drivers should use the fb stored in plane->base.state. I've missed that VIDI driver doesn't get the fb and incorrectly used plane->base.fb instad of state->fb while updating the code. > In case that userspace requests setplane, plane->base.fb would be updated after update_plane callback. > However, in other cases, I don't see how plane->base.fb could be updated. > In this case, I think vendor specific drivers would need to update it as a current fb to be scanned out like other some drivers did. > Of course, it may be possible for drm core part to take care of it appropriately later. > > Thanks, > Inki Dae > >> appropriate: >> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c >> @@ -132,12 +132,13 @@ static void vidi_update_plane(struct exynos_drm_crtc *crtc, >> struct exynos_drm_plane *plane) >> { >> struct vidi_context *ctx = crtc->ctx; >> - dma_addr_t addr; >> + dma_addr_t addr = 0; >> >> if (ctx->suspended) >> return; >> >> - addr = exynos_drm_fb_dma_addr(plane->base.fb, 0); >> + if (plane->base.fb) >> + addr = exynos_drm_fb_dma_addr(plane->base.fb, 0); >> DRM_DEBUG_KMS("dma_addr = %pad\n", &addr); >> >> if (ctx->vblank_on) >> >> I will investigate the case of NULL plane->state.fb, because it might be relevant >> to other crtc drivers as well. >> >> >>> if (ctx->vblank_on) >>> >>> >>> 2015? 12? 10? 22:05? Inki Dae ?(?) ? ?: >>>> 2015? 11? 30? 22:53? Marek Szyprowski ?(?) ? ?: >>>>> DMA address is a framebuffer attribute and the right place for it is >>>>> exynos_drm_framebuffer not exynos_drm_plane. This patch also introduces >>>>> helper function for getting dma address of the given framebuffer. >>>>> >>>>> Signed-off-by: Marek Szyprowski >>>>> Reviewed-by: Gustavo Padovan >>>>> --- >>>>> drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 13 ++++++++----- >>>>> drivers/gpu/drm/exynos/exynos7_drm_decon.c | 16 +++++++++------- >>>>> drivers/gpu/drm/exynos/exynos_drm_drv.h | 3 --- >>>>> drivers/gpu/drm/exynos/exynos_drm_fb.c | 16 ++++++---------- >>>>> drivers/gpu/drm/exynos/exynos_drm_fb.h | 3 +-- >>>>> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 10 ++++++---- >>>>> drivers/gpu/drm/exynos/exynos_drm_plane.c | 18 ------------------ >>>>> drivers/gpu/drm/exynos/exynos_drm_vidi.c | 5 ++++- >>>>> drivers/gpu/drm/exynos/exynos_mixer.c | 7 ++++--- >>>>> 9 files changed, 38 insertions(+), 53 deletions(-) >>>>> >>>> <--snip--> >>>> >>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c >>>>> index 669362c53f49..3ce141236fad 100644 >>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c >>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c >>>>> @@ -24,6 +24,7 @@ >>>>> #include "exynos_drm_drv.h" >>>>> #include "exynos_drm_crtc.h" >>>>> +#include "exynos_drm_fb.h" >>>>> #include "exynos_drm_plane.h" >>>>> #include "exynos_drm_vidi.h" >>>>> @@ -126,11 +127,13 @@ static void vidi_update_plane(struct exynos_drm_crtc *crtc, >>>>> struct exynos_drm_plane *plane) >>>>> { >>>>> struct vidi_context *ctx = crtc->ctx; >>>>> + dma_addr_t addr; >>>>> if (ctx->suspended) >>>>> return; >>>>> - DRM_DEBUG_KMS("dma_addr = %pad\n", plane->dma_addr); >>>>> + addr = exynos_drm_fb_dma_addr(plane->base.fb, 0); >>>> At this point, plane->base.fb is NULL so null pointer access happens like below, >>>> >>>> [ 5.969422] Unable to handle kernel NULL pointer dereference at virtual address 00000090 >>>> [ 5.977481] pgd = ee590000 >>>> [ 5.980142] [00000090] *pgd=6e526831, *pte=00000000, *ppte=00000000 >>>> [ 5.986347] Internal error: Oops: 17 [#1] PREEMPT SMP ARM >>>> [ 5.991712] Modules linked in: >>>> [ 5.994770] CPU: 3 PID: 1598 Comm: sh Not tainted 4.4.0-rc3-00052-gc60d7e2-dirty #199 >>>> [ 6.002565] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) >>>> [ 6.008647] task: ef328000 ti: ee4d4000 task.ti: ee4d4000 >>>> [ 6.014053] PC is at exynos_drm_fb_dma_addr+0x8/0x14 >>>> [ 6.018990] LR is at vidi_update_plane+0x4c/0xc4 >>>> [ 6.023581] pc : [] lr : [] psr: 80000013 >>>> [ 6.023581] sp : ee4d5d90 ip : 00000001 fp : 00000000 >>>> [ 6.035029] r10: 00000000 r9 : c05b965c r8 : ee813e00 >>>> [ 6.040241] r7 : 00000000 r6 : ee8e3330 r5 : 00000000 r4 : ee8e3010 >>>> [ 6.046749] r3 : 00000000 r2 : 00000000 r1 : 00000024 r0 : 00000000 >>>> [ 6.053264] Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none >>>> [ 6.060379] Control: 10c5387d Table: 6e59004a DAC: 00000051 >>>> [ 6.066107] Process sh (pid: 1598, stack limit = 0xee4d4210) >>>> [ 6.071748] Stack: (0xee4d5d90 to 0xee4d6000) >>>> [ 6.076100] 5d80: 00000000 ee813300 ee476e40 00000005 >>>> [ 6.084236] 5da0: ee8e3330 c028ac14 00000008 ee476e40 ee476fc0 ef3b3800 ee476fc0 eeb3e380 >>>> [ 6.092395] 5dc0: 00000002 c02b08e4 00000000 eeb3e3a4 ee476fc0 ee476e40 ef3b3800 eeb3e380 >>>> [ 6.100554] 5de0: 00000002 c02b12b8 ee854400 00000000 00000001 ee8501a8 00000000 ee476e40 >>>> [ 6.108714] 5e00: ef3b3800 00000001 ee476e40 00000050 ee850c80 00000001 ee476e40 ef3b3aac >>>> [ 6.116873] 5e20: 00000002 000000ff 00000000 c028e0ec 000a82b4 c02acc50 ee8e36a0 ee476c80 >>>> [ 6.125032] 5e40: ef3b3aac ef3b3800 ee476c9c ee850c80 ef3b3800 ef3b3800 ef3b3800 ef3b398c >>>> [ 6.133191] 5e60: c088c390 00000002 000a82b4 c028f8d4 00000000 ef3b3800 ef0f4300 c028f948 >>>> [ 6.141350] 5e80: ee850c80 c028f864 ef3b3a84 00000001 ef3b3a90 c02853e4 00000001 00000000 >>>> [ 6.149509] 5ea0: 000a82b4 ee4d5ec0 00000002 ee8e3010 00000002 00000002 ee4d5f88 00000000 >>>> [ 6.157669] 5ec0: 00000000 eeb8df00 000a82b4 c02c4278 00000002 ee476b00 eeb8df0c c01390ac >>>> [ 6.165828] 5ee0: 00000000 00000000 ee4e1f00 00000002 000a9540 ee4d5f88 c000f844 ee4d4000 >>>> [ 6.173987] 5f00: 00000000 c00dbf70 000a82b4 c00093dc ee4d4000 ee4d5f78 ef328234 c0579bec >>>> [ 6.182146] 5f20: 00000001 00000001 ee4d5f3c 00000001 ee45e9c4 00000001 000a82b4 c005ca74 >>>> [ 6.190306] 5f40: ee45e9c4 00000002 000a9540 c005cad4 ee4e1f00 00000002 000a9540 ee4d5f88 >>>> [ 6.198465] 5f60: c000f844 c00dc770 00000000 00000000 ee4e1f00 ee4e1f00 00000002 000a9540 >>>> [ 6.206624] 5f80: c000f844 c00dcf98 00000000 00000000 00000003 000a7c40 00000001 000a9540 >>>> [ 6.214783] 5fa0: 00000004 c000f680 000a7c40 00000001 00000001 000a9540 00000002 00000000 >>>> [ 6.222942] 5fc0: 000a7c40 00000001 000a9540 00000004 00000020 000a82c8 000a8294 000a82b4 >>>> [ 6.231102] 5fe0: 00000000 be8b1624 00012345 b6e94166 40000030 00000001 00000000 00000000 >>>> [ 6.239270] [] (exynos_drm_fb_dma_addr) from [] (vidi_update_plane+0x4c/0xc4) >>>> [ 6.248122] [] (vidi_update_plane) from [] (drm_atomic_helper_commit_planes+0x1f4/0x258) >>>> [ 6.257928] [] (drm_atomic_helper_commit_planes) from [] (exynos_atomic_commit_complete+0xe4/0x1c4) >>>> [ 6.268688] [] (exynos_atomic_commit_complete) from [] (exynos_atomic_commit+0x180/0x1cc) >>>> [ 6.278584] [] (exynos_atomic_commit) from [] (restore_fbdev_mode+0x260/0x290) >>>> [ 6.287525] [] (restore_fbdev_mode) from [] (drm_fb_helper_restore_fbdev_mode_unlocked+0x30/0x74) >>>> [ 6.298111] [] (drm_fb_helper_restore_fbdev_mode_unlocked) from [] (drm_fb_helper_set_par+0x30/0x54) >>>> [ 6.308961] [] (drm_fb_helper_set_par) from [] (drm_fb_helper_hotplug_event+0x9c/0xdc) >>>> [ 6.318595] [] (drm_fb_helper_hotplug_event) from [] (drm_helper_hpd_irq_event+0xd4/0x160) >>>> [ 6.328578] [] (drm_helper_hpd_irq_event) from [] (vidi_store_connection+0x94/0xcc) >>>> [ 6.337954] [] (vidi_store_connection) from [] (kernfs_fop_write+0xb8/0x1bc) >>>> [ 6.346723] [] (kernfs_fop_write) from [] (__vfs_write+0x20/0xd8) >>>> [ 6.354531] [] (__vfs_write) from [] (vfs_write+0x90/0x164) >>>> [ 6.361821] [] (vfs_write) from [] (SyS_write+0x44/0x9c) >>>> [ 6.368855] [] (SyS_write) from [] (ret_fast_syscall+0x0/0x3c) >>>> [ 6.376404] Code: eb0b17f1 eaffffe7 e3510003 d2811024 (d7900101) >>>> >>>> When vidi driver is intiated by triggering a connection sysfs file, vidi driver tries modeset binding by calling drm_fb_helper_hotplug_event. >>>> However, at this time it seems there is a case that plan->state->crtc exists but plane->fb is NULL, which would be related to vidi driver. >>>> >>>> I just looked into this issue roughly so we would need to check this issue in more details. >>>> >>>> Thanks, >>>> Inki Dae >>>> >>>>> + DRM_DEBUG_KMS("dma_addr = %pad\n", &addr); >>>>> if (ctx->vblank_on) >>>>> schedule_work(&ctx->work); >>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>> index 47777be1a754..f40de82848dc 100644 >>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >>>>> @@ -37,6 +37,7 @@ >>>>> #include "exynos_drm_drv.h" >>>>> #include "exynos_drm_crtc.h" >>>>> +#include "exynos_drm_fb.h" >>>>> #include "exynos_drm_plane.h" >>>>> #include "exynos_drm_iommu.h" >>>>> @@ -422,8 +423,8 @@ static void vp_video_buffer(struct mixer_context *ctx, >>>>> return; >>>>> } >>>>> - luma_addr[0] = plane->dma_addr[0]; >>>>> - chroma_addr[0] = plane->dma_addr[1]; >>>>> + luma_addr[0] = exynos_drm_fb_dma_addr(fb, 0); >>>>> + chroma_addr[0] = exynos_drm_fb_dma_addr(fb, 1); >>>>> if (mode->flags & DRM_MODE_FLAG_INTERLACE) { >>>>> ctx->interlace = true; >>>>> @@ -575,7 +576,7 @@ static void mixer_graph_buffer(struct mixer_context *ctx, >>>>> dst_y_offset = plane->crtc_y; >>>>> /* converting dma address base and source offset */ >>>>> - dma_addr = plane->dma_addr[0] >>>>> + dma_addr = exynos_drm_fb_dma_addr(fb, 0) >>>>> + (plane->src_x * fb->bits_per_pixel >> 3) >>>>> + (plane->src_y * fb->pitches[0]); >>>>> src_x_offset = 0; >>>>> >>>>> >>>>> Best regards --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c @@ -131,13 +131,14 @@ static void vidi_disable_vblank(struct exynos_drm_crtc *crtc) static void vidi_update_plane(struct exynos_drm_crtc *crtc, struct exynos_drm_plane *plane) { + struct drm_plane_state *state = plane->base.state; struct vidi_context *ctx = crtc->ctx; dma_addr_t addr; if (ctx->suspended) return; - addr = exynos_drm_fb_dma_addr(plane->base.fb, 0); + addr = exynos_drm_fb_dma_addr(state->fb, 0); DRM_DEBUG_KMS("dma_addr = %pad\n", &addr);