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

["Christian König" <christian.koenig-5C7GfCeVMHo@public.gmane.org>]

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