Re: [PATCH v2 06/22] drm/exynos: move dma_addr attribute from exynos plane to exynos fb

[Inki Dae <inki.dae@samsung.com>]

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?

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@samsung.com>
>>>> Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>> ---
>>>>   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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel