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월 14일 18:15에 Marek Szyprowski 이(가) 쓴 글:
> Hi Inki,
> 
> On 2015-12-11 15:52, Inki Dae wrote:
>> 2015-12-11 20:27 GMT+09:00 Marek Szyprowski <m.szyprowski@samsung.com>:
>>> 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

This function wouldn't be related to what we are talking about. But...

> are also in the drm_atomic_clean_old_fb() function in drivers/gpu/drm/drm_atomic.c

Right. Seems that this function is called after exynos_plane_atomic_update function
call by atomic_update callback. So drm core updates plane->fb appropriately.

Thanks,
Inki Dae.

> 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@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