Hi Inki,
On 2015-12-11 15:52, Inki Dae wrote:
2015-12-11 20:27 GMT+09:00 Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>:
On 2015-12-11 10:57, Inki Dae wrote:
2015년 12월 11일 18:26에 Marek Szyprowski 이(가) 쓴 글:
On 2015-12-11 10:02, Inki Dae wrote:
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.
Ok, If so - drm core updates plane->fb - then it's reasonable. But I
couldn't find the exact location where plane->fb is set to a fb to be
scanned out.
So could you let me know the exact location? it's not clear to me yet.
Setting plane->fb is wired somewhere in the atomic update logic, see
__setplane_internal() function in drivers/gpu/drm/drm_crtc.c. Some more
comments
are also in the drm_atomic_clean_old_fb() function in
drivers/gpu/drm/drm_atomic.c
However I don't know the exact call path.
I've checked this issue and the proper fix for is the following code:
--- 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);
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.
Actually, I used state->fb instead of plane->pending_fb but in
exynos_plane_atomic_update function, plane->pending_fb is set to
state->fb.
That is why I commented like below,
" 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."
Anyway, using state->fb looks like more consistent with other drivers,
fimd, decon and mixer.
Thanks for applying my fix and merging this patch.
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 <m.szyprowski@xxxxxxxxxxx>
Reviewed-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
---
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 : [<c02b1fb4>] lr : [<c02c3cc4>] 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] [<c02b1fb4>] (exynos_drm_fb_dma_addr) from [<c02c3cc4>]
(vidi_update_plane+0x4c/0xc4)
[ 6.248122] [<c02c3cc4>] (vidi_update_plane) from [<c028ac14>]
(drm_atomic_helper_commit_planes+0x1f4/0x258)
[ 6.257928] [<c028ac14>] (drm_atomic_helper_commit_planes) from
[<c02b08e4>] (exynos_atomic_commit_complete+0xe4/0x1c4)
[ 6.268688] [<c02b08e4>] (exynos_atomic_commit_complete) from
[<c02b12b8>] (exynos_atomic_commit+0x180/0x1cc)
[ 6.278584] [<c02b12b8>] (exynos_atomic_commit) from [<c028e0ec>]
(restore_fbdev_mode+0x260/0x290)
[ 6.287525] [<c028e0ec>] (restore_fbdev_mode) from [<c028f8d4>]
(drm_fb_helper_restore_fbdev_mode_unlocked+0x30/0x74)
[ 6.298111] [<c028f8d4>] (drm_fb_helper_restore_fbdev_mode_unlocked)
from [<c028f948>] (drm_fb_helper_set_par+0x30/0x54)
[ 6.308961] [<c028f948>] (drm_fb_helper_set_par) from [<c028f864>]
(drm_fb_helper_hotplug_event+0x9c/0xdc)
[ 6.318595] [<c028f864>] (drm_fb_helper_hotplug_event) from
[<c02853e4>] (drm_helper_hpd_irq_event+0xd4/0x160)
[ 6.328578] [<c02853e4>] (drm_helper_hpd_irq_event) from
[<c02c4278>] (vidi_store_connection+0x94/0xcc)
[ 6.337954] [<c02c4278>] (vidi_store_connection) from [<c01390ac>]
(kernfs_fop_write+0xb8/0x1bc)
[ 6.346723] [<c01390ac>] (kernfs_fop_write) from [<c00dbf70>]
(__vfs_write+0x20/0xd8)
[ 6.354531] [<c00dbf70>] (__vfs_write) from [<c00dc770>]
(vfs_write+0x90/0x164)
[ 6.361821] [<c00dc770>] (vfs_write) from [<c00dcf98>]
(SyS_write+0x44/0x9c)
[ 6.368855] [<c00dcf98>] (SyS_write) from [<c000f680>]
(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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
