Re: [PATCH v2 1/1] drm/amdgpu: Enable scatter gather display support

["Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi Sam,

yeah sorry for that. It's already rather late here and I got a bit taken 
away.

But I still insist that you at least try as I advised, I'm pretty sure 
that this approach will work and cover all use cases discussed so far.

Regards,
Christian.

Am 12.04.2018 um 22:42 schrieb Samuel Li:
> Hi Christian,
>
> If you have any good proposal, please describe here.
> Otherwise kindly avoid saying anything based on your emotion.
>
> Regards,
> Sam
>
>
>
> On 2018-04-12 04:13 PM, Christian König wrote:
>> Am 12.04.2018 um 22:01 schrieb Samuel Li:
>>> The 4th proposal :)
>>>
>>>> In other words add something like the following:
>>>>
>>>> if (domain & AMDGPU_GEM_DOMAIN_GTT && bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT)
>>>>       domain = AMDGPU_GEM_DOMAIN_GTT;
>>>>
>>>> That should be everything we need here.
>>> This is basically against the idea of Marek's change: in Mesa, both GTT/VRAM are allowed; but now in your kernel change, all buffers uses GTT only(not limited to display buffer now).
>>> To compare, current patch still seems better, since it only circumscribes display buffer.
>> What I suggested here is for pinning preference, that only affects BOs used for scanout and it also only affects them while they are scanned out.
>>
>> Please implement as advised or otherwise we need to assign the work to somebody else.
>>
>> Thanks,
>> Christian.
>>
>>> Sam
>>>
>>>
>>> On 2018-04-12 02:47 PM, Christian König wrote:
>>>> Patch #1: Avoid the hardware bug!
>>>>
>>>> E.g. even when we avoid different placements it would be good to have a patch which turns off immediate flipping when switching from VRAM to GTT.
>>>>
>>>> That is as safety net and to document that we need to avoid this condition on the hardware.
>>>>
>>>> Patch #2: Go into amdgpu_bo_pin_restricted() and change the pinning preference.
>>>>
>>>> In other words add something like the following:
>>>>
>>>> if (domain & AMDGPU_GEM_DOMAIN_GTT && bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT)
>>>>       domain = AMDGPU_GEM_DOMAIN_GTT;
>>>>
>>>> That should be everything we need here.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> Am 12.04.2018 um 20:07 schrieb Samuel Li:
>>>>> Please clarify, Christian. How would you like it to be implemented?
>>>>>
>>>>> Sam
>>>>>
>>>>>
>>>>> On 2018-04-12 02:00 PM, Christian König wrote:
>>>>>>> 1) Turn off immediate mode when flipping between VRAM/GTT.
>>>>>> That must be implemented independently.
>>>>>>
>>>>>> See working around the hardware bug should be a different patch than implementing a placement policy.
>>>>>>
>>>>>>> As per discussion, the 3rd one, which is the current patch, seems the best so far.
>>>>>> And I can only repeat myself. Alex and I are the maintainers of the kernel module, so we are the one who decide on how to implement things here.
>>>>>>
>>>>>> And we both noted that this approach of overriding user space decisions is not a good design.
>>>>>>
>>>>>> The placement policy I suggest by preferring GTT over VRAM on APUs should be trivial to implement and as far as I can see avoids all negative side effects.
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>> Am 12.04.2018 um 19:21 schrieb Samuel Li:
>>>>>>>> The point is this kernel change now needs to be reworked and adapted to what Mesa is doing.
>>>>>>> Three options have been brought up for kernel,
>>>>>>> 1) Turn off immediate mode when flipping between VRAM/GTT.
>>>>>>> 2) Check the domain of the current fb and then adjust the new one before pinning it.
>>>>>>> 3) Use only VRAM or GTT depending on a threshhold.
>>>>>>>
>>>>>>> As per discussion, the 3rd one, which is the current patch, seems the best so far.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Samuel Li
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 2018-04-12 01:03 PM, Christian König wrote:
>>>>>>>>> Can you be more specific, Christian? Mesa has this, I don't think it needs anything else:
>>>>>>>> Completely agree, that's what I suggested to implement.
>>>>>>>>
>>>>>>>> The point is this kernel change now needs to be reworked and adapted to what Mesa is doing.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>> Am 12.04.2018 um 18:40 schrieb Marek Olšák:
>>>>>>>>> Can you be more specific, Christian? Mesa has this, I don't think it needs anything else:
>>>>>>>>> https://cgit.freedesktop.org/mesa/mesa/commit/?id=7d2079908d9ef05ec3f35b7078833e57846cab5b
>>>>>>>>>
>>>>>>>>> Marek
>>>>>>>>>
>>>>>>>>> On Wed, Mar 28, 2018 at 3:46 AM, Christian König <ckoenig.leichtzumerken@gmail.com <mailto:ckoenig.leichtzumerken@gmail.com>> wrote:
>>>>>>>>>
>>>>>>>>>         Am 28.03.2018 um 00:22 schrieb Samuel Li:
>>>>>>>>>
>>>>>>>>>             It's auto by default. For CZ/ST, auto setting enables sg display
>>>>>>>>>             when vram size is small; otherwise still uses vram.
>>>>>>>>>             This patch fixed some potential hang issue introduced by change
>>>>>>>>>             "allow framebuffer in GART memory as well" due to CZ/ST hardware
>>>>>>>>>             limitation.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>         Well that is still a NAK.
>>>>>>>>>
>>>>>>>>>         As discussed now multiple times please implement the necessary
>>>>>>>>>         changes in Mesa.
>>>>>>>>>
>>>>>>>>>         Regards,
>>>>>>>>>         Christian.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>             v2: Change default setting to auto, also some misc changes.
>>>>>>>>>             Signed-off-by: Samuel Li <Samuel.Li@amd.com
>>>>>>>>>             <mailto:Samuel.Li@amd.com>>
>>>>>>>>>             ---
>>>>>>>>>               drivers/gpu/drm/amd/amdgpu/amdgpu.h    |  1 +
>>>>>>>>>               drivers/gpu/drm/amd/amdgpu/amdgpu_display.c    | 10 ++++++++--
>>>>>>>>>               drivers/gpu/drm/amd/amdgpu/amdgpu_display.h    |  2 ++
>>>>>>>>>               drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  4 ++++
>>>>>>>>>               drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c   |  2 ++
>>>>>>>>>               drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  3 ++-
>>>>>>>>>               6 files changed, 19 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>>             diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>>>>             b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>>>>             index a7e2229..c942362 100644
>>>>>>>>>             --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>>>>             +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>>>>>>             @@ -129,6 +129,7 @@ extern int amdgpu_lbpw;
>>>>>>>>>               extern int amdgpu_compute_multipipe;
>>>>>>>>>               extern int amdgpu_gpu_recovery;
>>>>>>>>>               extern int amdgpu_emu_mode;
>>>>>>>>>             +extern int amdgpu_sg_display;
>>>>>>>>>                 #ifdef CONFIG_DRM_AMDGPU_SI
>>>>>>>>>               extern int amdgpu_si_support;
>>>>>>>>>             diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>>>>>>>             b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>>>>>>>             index 5495b29..1e7b950 100644
>>>>>>>>>             --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>>>>>>>             +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>>>>>>>>             @@ -513,8 +513,14 @@ uint32_t
>>>>>>>>>             amdgpu_display_framebuffer_domains(struct amdgpu_device *adev)
>>>>>>>>>               #if defined(CONFIG_DRM_AMD_DC)
>>>>>>>>>                     if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type
>>>>>>>>>             < CHIP_RAVEN &&
>>>>>>>>>                         adev->flags & AMD_IS_APU &&
>>>>>>>>>             -           amdgpu_device_asic_has_dc_support(adev->asic_type))
>>>>>>>>>             -               domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>>>>>>>             +           amdgpu_device_asic_has_dc_support(adev->asic_type)) {
>>>>>>>>>             +               if (amdgpu_sg_display == 1)
>>>>>>>>>             +                       domain = AMDGPU_GEM_DOMAIN_GTT;
>>>>>>>>>             +               else if (amdgpu_sg_display == -1) {
>>>>>>>>>             +                       if (adev->gmc.real_vram_size <
>>>>>>>>>             AMDGPU_SG_THRESHOLD)
>>>>>>>>>             +                               domain = AMDGPU_GEM_DOMAIN_GTT;
>>>>>>>>>             +               }
>>>>>>>>>             +       }
>>>>>>>>>               #endif
>>>>>>>>>                     return domain;
>>>>>>>>>             diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
>>>>>>>>>             b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
>>>>>>>>>             index 2b11d80..2b25393 100644
>>>>>>>>>             --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
>>>>>>>>>             +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
>>>>>>>>>             @@ -23,6 +23,8 @@
>>>>>>>>>               #ifndef __AMDGPU_DISPLAY_H__
>>>>>>>>>               #define __AMDGPU_DISPLAY_H__
>>>>>>>>>               +#define AMDGPU_SG_THRESHOLD  (256*1024*1024)
>>>>>>>>>             +
>>>>>>>>>               uint32_t amdgpu_display_framebuffer_domains(struct
>>>>>>>>>             amdgpu_device *adev);
>>>>>>>>>               struct drm_framebuffer *
>>>>>>>>>               amdgpu_display_user_framebuffer_create(struct drm_device *dev,
>>>>>>>>>             diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>>>>>             b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>>>>>             index 1bfce79..19f11a5 100644
>>>>>>>>>             --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>>>>>             +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>>>>>>             @@ -132,6 +132,7 @@ int amdgpu_lbpw = -1;
>>>>>>>>>               int amdgpu_compute_multipipe = -1;
>>>>>>>>>               int amdgpu_gpu_recovery = -1; /* auto */
>>>>>>>>>               int amdgpu_emu_mode = 0;
>>>>>>>>>             +int amdgpu_sg_display = -1;
>>>>>>>>>                 MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in
>>>>>>>>>             megabytes");
>>>>>>>>>               module_param_named(vramlimit, amdgpu_vram_limit, int, 0600);
>>>>>>>>>             @@ -290,6 +291,9 @@ module_param_named(gpu_recovery,
>>>>>>>>>             amdgpu_gpu_recovery, int, 0444);
>>>>>>>>>               MODULE_PARM_DESC(emu_mode, "Emulation mode, (1 = enable, 0 =
>>>>>>>>>             disable)");
>>>>>>>>>               module_param_named(emu_mode, amdgpu_emu_mode, int, 0444);
>>>>>>>>>               +MODULE_PARM_DESC(sg_display, "Enable scatter gather
>>>>>>>>>             display, (1 = enable, 0 = disable, -1 = auto");
>>>>>>>>>             +module_param_named(sg_display, amdgpu_sg_display, int, 0444);
>>>>>>>>>             +
>>>>>>>>>               #ifdef CONFIG_DRM_AMDGPU_SI
>>>>>>>>>                 #if defined(CONFIG_DRM_RADEON) ||
>>>>>>>>>             defined(CONFIG_DRM_RADEON_MODULE)
>>>>>>>>>             diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
>>>>>>>>>             b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
>>>>>>>>>             index 1206301..f57c355 100644
>>>>>>>>>             --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
>>>>>>>>>             +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
>>>>>>>>>             @@ -138,6 +138,8 @@ static int
>>>>>>>>>             amdgpufb_create_pinned_object(struct amdgpu_fbdev *rfbdev,
>>>>>>>>>                     mode_cmd->pitches[0] = amdgpu_align_pitch(adev,
>>>>>>>>>             mode_cmd->width, cpp,
>>>>>>>>>             fb_tiled);
>>>>>>>>>                     domain = amdgpu_display_framebuffer_domains(adev);
>>>>>>>>>             +       if (domain & AMDGPU_GEM_DOMAIN_GTT)
>>>>>>>>>             +               DRM_DEBUG_DRIVER("Scatter gather display:
>>>>>>>>>             enabled\n");
>>>>>>>>>                     height = ALIGN(mode_cmd->height, 8);
>>>>>>>>>                     size = mode_cmd->pitches[0] * height;
>>>>>>>>>             diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>>>             b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>>>             index 68ab325..7e9f247 100644
>>>>>>>>>             --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>>>             +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>>>>>             @@ -3074,7 +3074,8 @@ static int
>>>>>>>>>             dm_plane_helper_prepare_fb(struct drm_plane *plane,
>>>>>>>>>                             domain = AMDGPU_GEM_DOMAIN_VRAM;
>>>>>>>>>                     r = amdgpu_bo_pin(rbo, domain, &afb->address);
>>>>>>>>>             -
>>>>>>>>>             +       rbo->preferred_domains = domain;
>>>>>>>>>             +       rbo->allowed_domains = domain;
>>>>>>>>>                     amdgpu_bo_unreserve(rbo);
>>>>>>>>>                     if (unlikely(r != 0)) {
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>         _______________________________________________
>>>>>>>>>         amd-gfx mailing list
>>>>>>>>>         amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>>>>>>>>>         https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>>>>>         <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
>>>>>>>>>
>>>>>>>>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx