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 at gmail.com <mailto:ckoenig.leichtzumerken at 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 at amd.com >>>>>>>>           <mailto:Samuel.Li at 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 at lists.freedesktop.org <mailto:amd-gfx at lists.freedesktop.org> >>>>>>>>       https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>>>>>>>       <https://lists.freedesktop.org/mailman/listinfo/amd-gfx> >>>>>>>> >>>>>>>> >
