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 >>>>>>>> <[email protected] >>>>>>>> <mailto:[email protected]>> 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 <[email protected] >>>>>>>> <mailto:[email protected]>> >>>>>>>> --- >>>>>>>> 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 >>>>>>>> [email protected] >>>>>>>> <mailto:[email protected]> >>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>>>>>>> <https://lists.freedesktop.org/mailman/listinfo/amd-gfx> >>>>>>>> >>>>>>>> > _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
