| Message ID | 20240802071752.116541-1-yaolu@kylinos.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amdgpu: add dce6 drm_panic support | expand |
Am 02.08.24 um 09:17 schrieb Lu Yao: > Add support for the drm_panic module, which displays a pretty user > friendly message on the screen when a Linux kernel panic occurs. > > Signed-off-by: Lu Yao <yaolu@kylinos.cn> > --- > The patch can work properly on the TTY, but after start X, drawn > image is messy, it looks like the data isn't linearly arranged. > However at this time 'fb->modifier' is 'DRM_FORMAT_MOD_LINEAR'. > > Another difference I found is: > For TTY, the amdgpu_bo is created with flag > 'AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED|AMDGPU_GEM_CREATE_CPU_GTT_USWC| > AMDGPU_GEM_CREATE_VRAM_CLEARED|AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS'. > For X, the amdgpu_bo is created with flag > 'AMDGPU_GEM_CREATE_NO_CPU_ACCESS|AMDGPU_GEM_CREATE_CPU_GTT_USWC' > I try to use same flag for X, it looks like no difference. > > Can someone provide some insight into this problem or where I am going > wrong. Thanks a lot. > > Test environment: X86 arch + v6.6 kernel + R7340. > --- > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 32 +++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > index 05c0df97f01d..12c3801c264a 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > @@ -28,6 +28,8 @@ > #include <drm/drm_modeset_helper.h> > #include <drm/drm_modeset_helper_vtables.h> > #include <drm/drm_vblank.h> > +#include <drm/drm_panic.h> > +#include "../../drm_internal.h" Well that this file is named "internal" and not in a common include directory is a strong indicator that you should absolutely *not* include it in a driver. > > #include "amdgpu.h" > #include "amdgpu_pm.h" > @@ -2600,6 +2602,35 @@ static const struct drm_crtc_helper_funcs dce_v6_0_crtc_helper_funcs = { > .get_scanout_position = amdgpu_crtc_get_scanout_position, > }; > > +static int dce_v6_0_drm_primary_plane_get_scanout_buffer(struct drm_plane *plane, > + struct drm_scanout_buffer *sb) > +{ > + struct drm_framebuffer *fb; > + struct drm_gem_object *obj; > + struct amdgpu_bo *abo; > + int ret = 0; > + > + if (!plane->fb || plane->fb->modifier != DRM_FORMAT_MOD_LINEAR) > + return -ENODEV; > + > + fb = plane->fb; > + sb->width = fb->width; > + sb->height = fb->height; > + sb->format = fb->format; > + sb->pitch[0] = fb->pitches[0]; > + > + obj = fb->obj[0]; > + abo = gem_to_amdgpu_bo(obj); > + if (!abo || abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) > + return -EINVAL; > + > + return drm_gem_vmap(obj, &sb->map[0]); Yeah that will almost always not work. Most display buffers are tilled and not CPU accessible. Regards, Christian. > +} > + > +static const struct drm_plane_helper_funcs dce_v6_0_drm_primary_plane_helper_funcs = { > + .get_scanout_buffer = dce_v6_0_drm_primary_plane_get_scanout_buffer > +}; > + > static int dce_v6_0_crtc_init(struct amdgpu_device *adev, int index) > { > struct amdgpu_crtc *amdgpu_crtc; > @@ -2627,6 +2658,7 @@ static int dce_v6_0_crtc_init(struct amdgpu_device *adev, int index) > amdgpu_crtc->encoder = NULL; > amdgpu_crtc->connector = NULL; > drm_crtc_helper_add(&amdgpu_crtc->base, &dce_v6_0_crtc_helper_funcs); > + drm_plane_helper_add(amdgpu_crtc->base.primary, &dce_v6_0_drm_primary_plane_helper_funcs); > > return 0; > }
在 2024/8/2 17:39, Christian König 写道: > Am 02.08.24 um 09:17 schrieb Lu Yao: >> Add support for the drm_panic module, which displays a pretty user >> friendly message on the screen when a Linux kernel panic occurs. >> >> Signed-off-by: Lu Yao <yaolu@kylinos.cn> >> --- >> The patch can work properly on the TTY, but after start X, drawn >> image is messy, it looks like the data isn't linearly arranged. >> However at this time 'fb->modifier' is 'DRM_FORMAT_MOD_LINEAR'. >> >> Another difference I found is: >> For TTY, the amdgpu_bo is created with flag >> 'AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED|AMDGPU_GEM_CREATE_CPU_GTT_USWC| >> AMDGPU_GEM_CREATE_VRAM_CLEARED|AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS'. >> For X, the amdgpu_bo is created with flag >> 'AMDGPU_GEM_CREATE_NO_CPU_ACCESS|AMDGPU_GEM_CREATE_CPU_GTT_USWC' >> I try to use same flag for X, it looks like no difference. >> >> Can someone provide some insight into this problem or where I am going >> wrong. Thanks a lot. >> >> Test environment: X86 arch + v6.6 kernel + R7340. >> --- >> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 32 +++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >> b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >> index 05c0df97f01d..12c3801c264a 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >> @@ -28,6 +28,8 @@ >> #include <drm/drm_modeset_helper.h> >> #include <drm/drm_modeset_helper_vtables.h> >> #include <drm/drm_vblank.h> >> +#include <drm/drm_panic.h> > >> +#include "../../drm_internal.h" > > Well that this file is named "internal" and not in a common include > directory is a strong indicator that you should absolutely *not* > include it in a driver. > Okay, I'll fix it. >> #include "amdgpu.h" >> #include "amdgpu_pm.h" >> @@ -2600,6 +2602,35 @@ static const struct drm_crtc_helper_funcs >> dce_v6_0_crtc_helper_funcs = { >> .get_scanout_position = amdgpu_crtc_get_scanout_position, >> }; >> +static int dce_v6_0_drm_primary_plane_get_scanout_buffer(struct >> drm_plane *plane, >> + struct drm_scanout_buffer *sb) >> +{ >> + struct drm_framebuffer *fb; >> + struct drm_gem_object *obj; >> + struct amdgpu_bo *abo; >> + int ret = 0; >> + >> + if (!plane->fb || plane->fb->modifier != DRM_FORMAT_MOD_LINEAR) >> + return -ENODEV; >> + >> + fb = plane->fb; >> + sb->width = fb->width; >> + sb->height = fb->height; >> + sb->format = fb->format; >> + sb->pitch[0] = fb->pitches[0]; >> + >> + obj = fb->obj[0]; >> + abo = gem_to_amdgpu_bo(obj); >> + if (!abo || abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) >> + return -EINVAL; >> + >> + return drm_gem_vmap(obj, &sb->map[0]); > > Yeah that will almost always not work. Most display buffers are tilled > and not CPU accessible. > > Regards, > Christian. > I did some more tests. After removing the 'AMDGPU_GEM_CREATE_NO_CPU_ACCESS' judgment here, then starting X, it worked well at '1280x960' resolution, but others (e.g. 1920x1080, 640x480) not. So, for this problem, it doesn't seem to matter 'Tiled memory' or 'CPU can't access'. Or is it just a coincidence ? Thanks, Lu Yao >> +} >> + >> +static const struct drm_plane_helper_funcs >> dce_v6_0_drm_primary_plane_helper_funcs = { >> + .get_scanout_buffer = dce_v6_0_drm_primary_plane_get_scanout_buffer >> +}; >> + >> static int dce_v6_0_crtc_init(struct amdgpu_device *adev, int index) >> { >> struct amdgpu_crtc *amdgpu_crtc; >> @@ -2627,6 +2658,7 @@ static int dce_v6_0_crtc_init(struct >> amdgpu_device *adev, int index) >> amdgpu_crtc->encoder = NULL; >> amdgpu_crtc->connector = NULL; >> drm_crtc_helper_add(&amdgpu_crtc->base, >> &dce_v6_0_crtc_helper_funcs); >> + drm_plane_helper_add(amdgpu_crtc->base.primary, >> &dce_v6_0_drm_primary_plane_helper_funcs); >> return 0; >> } >
On 02/08/2024 11:39, Christian König wrote: > Am 02.08.24 um 09:17 schrieb Lu Yao: >> Add support for the drm_panic module, which displays a pretty user >> friendly message on the screen when a Linux kernel panic occurs. >> >> Signed-off-by: Lu Yao <yaolu@kylinos.cn> >> --- >> The patch can work properly on the TTY, but after start X, drawn >> image is messy, it looks like the data isn't linearly arranged. >> However at this time 'fb->modifier' is 'DRM_FORMAT_MOD_LINEAR'. >> >> Another difference I found is: >> For TTY, the amdgpu_bo is created with flag >> 'AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED|AMDGPU_GEM_CREATE_CPU_GTT_USWC| >> AMDGPU_GEM_CREATE_VRAM_CLEARED|AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS'. >> For X, the amdgpu_bo is created with flag >> 'AMDGPU_GEM_CREATE_NO_CPU_ACCESS|AMDGPU_GEM_CREATE_CPU_GTT_USWC' >> I try to use same flag for X, it looks like no difference. >> >> Can someone provide some insight into this problem or where I am going >> wrong. Thanks a lot. >> >> Test environment: X86 arch + v6.6 kernel + R7340. >> --- >> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 32 +++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >> b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >> index 05c0df97f01d..12c3801c264a 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >> @@ -28,6 +28,8 @@ >> #include <drm/drm_modeset_helper.h> >> #include <drm/drm_modeset_helper_vtables.h> >> #include <drm/drm_vblank.h> >> +#include <drm/drm_panic.h> > >> +#include "../../drm_internal.h" > > Well that this file is named "internal" and not in a common include > directory is a strong indicator that you should absolutely *not* include > it in a driver. > >> #include "amdgpu.h" >> #include "amdgpu_pm.h" >> @@ -2600,6 +2602,35 @@ static const struct drm_crtc_helper_funcs >> dce_v6_0_crtc_helper_funcs = { >> .get_scanout_position = amdgpu_crtc_get_scanout_position, >> }; >> +static int dce_v6_0_drm_primary_plane_get_scanout_buffer(struct >> drm_plane *plane, >> + struct drm_scanout_buffer *sb) >> +{ >> + struct drm_framebuffer *fb; >> + struct drm_gem_object *obj; >> + struct amdgpu_bo *abo; >> + int ret = 0; >> + >> + if (!plane->fb || plane->fb->modifier != DRM_FORMAT_MOD_LINEAR) >> + return -ENODEV; >> + >> + fb = plane->fb; >> + sb->width = fb->width; >> + sb->height = fb->height; >> + sb->format = fb->format; >> + sb->pitch[0] = fb->pitches[0]; >> + >> + obj = fb->obj[0]; >> + abo = gem_to_amdgpu_bo(obj); >> + if (!abo || abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) >> + return -EINVAL; >> + >> + return drm_gem_vmap(obj, &sb->map[0]); > > Yeah that will almost always not work. Most display buffers are tilled > and not CPU accessible. For the CPU accessible issue, Christian mentioned there was a debug interface on AMD GPU that can be used, to work around this: https://lore.kernel.org/dri-devel/0baabe1f-8924-2c9a-5cd4-59084a37dbb2@gmail.com/ and https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/ And you will need to use the scanout_buffer->set_pixel() callback to write the pixels one by one, similar to what I've tried for nouveau with https://patchwork.freedesktop.org/series/133963/ For the tiling format, the problem is that it is internal to the GPU, and currently the driver don't know which tiling format is being used. It might be possible to disable tiling and compression, but it requires some internal DC knowledge: https://lore.kernel.org/dri-devel/f76a3297-7d63-8615-45c5-47f02b64a1d5@amd.com/ Best regards,
On 2024/8/5 17:25, Jocelyn Falempe wrote: > > > On 02/08/2024 11:39, Christian König wrote: >> Am 02.08.24 um 09:17 schrieb Lu Yao: >>> Add support for the drm_panic module, which displays a pretty user >>> friendly message on the screen when a Linux kernel panic occurs. >>> >>> Signed-off-by: Lu Yao <yaolu@kylinos.cn> >>> --- >>> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 32 >>> +++++++++++++++++++++++++++ >>> 1 file changed, 32 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >>> b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >>> index 05c0df97f01d..12c3801c264a 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c >>> @@ -28,6 +28,8 @@ >>> #include <drm/drm_modeset_helper.h> >>> #include <drm/drm_modeset_helper_vtables.h> >>> #include <drm/drm_vblank.h> >>> +#include <drm/drm_panic.h> >> >>> +#include "../../drm_internal.h" >> >> Well that this file is named "internal" and not in a common include >> directory is a strong indicator that you should absolutely *not* >> include it in a driver. >> >>> #include "amdgpu.h" >>> #include "amdgpu_pm.h" >>> @@ -2600,6 +2602,35 @@ static const struct drm_crtc_helper_funcs >>> dce_v6_0_crtc_helper_funcs = { >>> .get_scanout_position = amdgpu_crtc_get_scanout_position, >>> }; >>> +static int dce_v6_0_drm_primary_plane_get_scanout_buffer(struct >>> drm_plane *plane, >>> + struct drm_scanout_buffer *sb) >>> +{ >>> + struct drm_framebuffer *fb; >>> + struct drm_gem_object *obj; >>> + struct amdgpu_bo *abo; >>> + int ret = 0; >>> + >>> + if (!plane->fb || plane->fb->modifier != DRM_FORMAT_MOD_LINEAR) >>> + return -ENODEV; >>> + >>> + fb = plane->fb; >>> + sb->width = fb->width; >>> + sb->height = fb->height; >>> + sb->format = fb->format; >>> + sb->pitch[0] = fb->pitches[0]; >>> + >>> + obj = fb->obj[0]; >>> + abo = gem_to_amdgpu_bo(obj); >>> + if (!abo || abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) >>> + return -EINVAL; >>> + >>> + return drm_gem_vmap(obj, &sb->map[0]); >> >> Yeah that will almost always not work. Most display buffers are >> tilled and not CPU accessible. > > For the CPU accessible issue, Christian mentioned there was a debug > interface on AMD GPU that can be used, to work around this: > > https://lore.kernel.org/dri-devel/0baabe1f-8924-2c9a-5cd4-59084a37dbb2@gmail.com/ > and > https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/ > > And you will need to use the scanout_buffer->set_pixel() callback to > write the pixels one by one, similar to what I've tried for nouveau with > https://patchwork.freedesktop.org/series/133963/ > > For the tiling format, the problem is that it is internal to the GPU, > and currently the driver don't know which tiling format is being used. > > It might be possible to disable tiling and compression, but it > requires some internal DC knowledge: > https://lore.kernel.org/dri-devel/f76a3297-7d63-8615-45c5-47f02b64a1d5@amd.com/ > > > Best regards, From the discussion provided, it is difficult to implement this feature without the relevant data book and knowledge.(Whether how tiled memory storage, or how to disable tiling of DC) It looks like I'll just have to wait for AMD engineers to implement this. Thanks a lot, Lu Yao
On Thu, Aug 8, 2024 at 2:35 AM Lu Yao <yaolu@kylinos.cn> wrote: > > On 2024/8/5 17:25, Jocelyn Falempe wrote: > > > > > > On 02/08/2024 11:39, Christian König wrote: > >> Am 02.08.24 um 09:17 schrieb Lu Yao: > >>> Add support for the drm_panic module, which displays a pretty user > >>> friendly message on the screen when a Linux kernel panic occurs. > >>> > >>> Signed-off-by: Lu Yao <yaolu@kylinos.cn> > >>> --- > >>> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 32 > >>> +++++++++++++++++++++++++++ > >>> 1 file changed, 32 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > >>> b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > >>> index 05c0df97f01d..12c3801c264a 100644 > >>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > >>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > >>> @@ -28,6 +28,8 @@ > >>> #include <drm/drm_modeset_helper.h> > >>> #include <drm/drm_modeset_helper_vtables.h> > >>> #include <drm/drm_vblank.h> > >>> +#include <drm/drm_panic.h> > >> > >>> +#include "../../drm_internal.h" > >> > >> Well that this file is named "internal" and not in a common include > >> directory is a strong indicator that you should absolutely *not* > >> include it in a driver. > >> > >>> #include "amdgpu.h" > >>> #include "amdgpu_pm.h" > >>> @@ -2600,6 +2602,35 @@ static const struct drm_crtc_helper_funcs > >>> dce_v6_0_crtc_helper_funcs = { > >>> .get_scanout_position = amdgpu_crtc_get_scanout_position, > >>> }; > >>> +static int dce_v6_0_drm_primary_plane_get_scanout_buffer(struct > >>> drm_plane *plane, > >>> + struct drm_scanout_buffer *sb) > >>> +{ > >>> + struct drm_framebuffer *fb; > >>> + struct drm_gem_object *obj; > >>> + struct amdgpu_bo *abo; > >>> + int ret = 0; > >>> + > >>> + if (!plane->fb || plane->fb->modifier != DRM_FORMAT_MOD_LINEAR) > >>> + return -ENODEV; > >>> + > >>> + fb = plane->fb; > >>> + sb->width = fb->width; > >>> + sb->height = fb->height; > >>> + sb->format = fb->format; > >>> + sb->pitch[0] = fb->pitches[0]; > >>> + > >>> + obj = fb->obj[0]; > >>> + abo = gem_to_amdgpu_bo(obj); > >>> + if (!abo || abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) > >>> + return -EINVAL; > >>> + > >>> + return drm_gem_vmap(obj, &sb->map[0]); > >> > >> Yeah that will almost always not work. Most display buffers are > >> tilled and not CPU accessible. > > > > For the CPU accessible issue, Christian mentioned there was a debug > > interface on AMD GPU that can be used, to work around this: > > > > https://lore.kernel.org/dri-devel/0baabe1f-8924-2c9a-5cd4-59084a37dbb2@gmail.com/ > > and > > https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/ > > > > And you will need to use the scanout_buffer->set_pixel() callback to > > write the pixels one by one, similar to what I've tried for nouveau with > > https://patchwork.freedesktop.org/series/133963/ > > > > For the tiling format, the problem is that it is internal to the GPU, > > and currently the driver don't know which tiling format is being used. > > > > It might be possible to disable tiling and compression, but it > > requires some internal DC knowledge: > > https://lore.kernel.org/dri-devel/f76a3297-7d63-8615-45c5-47f02b64a1d5@amd.com/ > > > > > > Best regards, > > From the discussion provided, it is difficult to implement this feature without the relevant data book and knowledge.(Whether how tiled memory storage, or how to disable tiling of DC) For DCE 6, the GRPH_ARRAY_MODE field in mmGRPH_CONTROL controls the display tiling. Set that field to GRPH_ARRAY_LINEAR_GENERAL (0) to disable tiling. Alex
On Thu, Aug 8, 2024 at 12:43 PM Alex Deucher <alexdeucher@gmail.com> wrote: > > On Thu, Aug 8, 2024 at 2:35 AM Lu Yao <yaolu@kylinos.cn> wrote: > > > > On 2024/8/5 17:25, Jocelyn Falempe wrote: > > > > > > > > > On 02/08/2024 11:39, Christian König wrote: > > >> Am 02.08.24 um 09:17 schrieb Lu Yao: > > >>> Add support for the drm_panic module, which displays a pretty user > > >>> friendly message on the screen when a Linux kernel panic occurs. > > >>> > > >>> Signed-off-by: Lu Yao <yaolu@kylinos.cn> > > >>> --- > > >>> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 32 > > >>> +++++++++++++++++++++++++++ > > >>> 1 file changed, 32 insertions(+) > > >>> > > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > > >>> b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > > >>> index 05c0df97f01d..12c3801c264a 100644 > > >>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > > >>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > > >>> @@ -28,6 +28,8 @@ > > >>> #include <drm/drm_modeset_helper.h> > > >>> #include <drm/drm_modeset_helper_vtables.h> > > >>> #include <drm/drm_vblank.h> > > >>> +#include <drm/drm_panic.h> > > >> > > >>> +#include "../../drm_internal.h" > > >> > > >> Well that this file is named "internal" and not in a common include > > >> directory is a strong indicator that you should absolutely *not* > > >> include it in a driver. > > >> > > >>> #include "amdgpu.h" > > >>> #include "amdgpu_pm.h" > > >>> @@ -2600,6 +2602,35 @@ static const struct drm_crtc_helper_funcs > > >>> dce_v6_0_crtc_helper_funcs = { > > >>> .get_scanout_position = amdgpu_crtc_get_scanout_position, > > >>> }; > > >>> +static int dce_v6_0_drm_primary_plane_get_scanout_buffer(struct > > >>> drm_plane *plane, > > >>> + struct drm_scanout_buffer *sb) > > >>> +{ > > >>> + struct drm_framebuffer *fb; > > >>> + struct drm_gem_object *obj; > > >>> + struct amdgpu_bo *abo; > > >>> + int ret = 0; > > >>> + > > >>> + if (!plane->fb || plane->fb->modifier != DRM_FORMAT_MOD_LINEAR) > > >>> + return -ENODEV; > > >>> + > > >>> + fb = plane->fb; > > >>> + sb->width = fb->width; > > >>> + sb->height = fb->height; > > >>> + sb->format = fb->format; > > >>> + sb->pitch[0] = fb->pitches[0]; > > >>> + > > >>> + obj = fb->obj[0]; > > >>> + abo = gem_to_amdgpu_bo(obj); > > >>> + if (!abo || abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) > > >>> + return -EINVAL; > > >>> + > > >>> + return drm_gem_vmap(obj, &sb->map[0]); > > >> > > >> Yeah that will almost always not work. Most display buffers are > > >> tilled and not CPU accessible. > > > > > > For the CPU accessible issue, Christian mentioned there was a debug > > > interface on AMD GPU that can be used, to work around this: > > > > > > https://lore.kernel.org/dri-devel/0baabe1f-8924-2c9a-5cd4-59084a37dbb2@gmail.com/ > > > and > > > https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/ > > > > > > And you will need to use the scanout_buffer->set_pixel() callback to > > > write the pixels one by one, similar to what I've tried for nouveau with > > > https://patchwork.freedesktop.org/series/133963/ > > > > > > For the tiling format, the problem is that it is internal to the GPU, > > > and currently the driver don't know which tiling format is being used. > > > > > > It might be possible to disable tiling and compression, but it > > > requires some internal DC knowledge: > > > https://lore.kernel.org/dri-devel/f76a3297-7d63-8615-45c5-47f02b64a1d5@amd.com/ > > > > > > > > > Best regards, > > > > From the discussion provided, it is difficult to implement this feature without the relevant data book and knowledge.(Whether how tiled memory storage, or how to disable tiling of DC) > > For DCE 6, the GRPH_ARRAY_MODE field in mmGRPH_CONTROL controls the > display tiling. Set that field to GRPH_ARRAY_LINEAR_GENERAL (0) to > disable tiling. For clarity that register is instanced so use mmGRPH_CONTROL + amdgpu_crtc->crtc_offset to get the right instance. Alex > > Alex
On Thu, Aug 8, 2024 at 13:24 PM Alex Deucher <alexdeucher@gmail.com> wrote: > > On Thu, Aug 8, 2024 at 12:43 PM Alex Deucher <alexdeucher@gmail.com> wrote: > > > > On Thu, Aug 8, 2024 at 2:35 AM Lu Yao <yaolu@kylinos.cn> wrote: > > > > > > On 2024/8/5 17:25, Jocelyn Falempe wrote: > > > > > > > > > > > > On 02/08/2024 11:39, Christian König wrote: > > > >> Am 02.08.24 um 09:17 schrieb Lu Yao: > > > >>> Add support for the drm_panic module, which displays a pretty user > > > >>> friendly message on the screen when a Linux kernel panic occurs. > > > >>> > > > >>> Signed-off-by: Lu Yao <yaolu@kylinos.cn> > > > >>> --- > > > >>> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 32 > > > >>> +++++++++++++++++++++++++++ > > > >>> 1 file changed, 32 insertions(+) > > > >>> > > > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > > > >>> b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > > > >>> index 05c0df97f01d..12c3801c264a 100644 > > > >>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > > > >>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c > > > >>> @@ -28,6 +28,8 @@ > > > >>> #include <drm/drm_modeset_helper.h> > > > >>> #include <drm/drm_modeset_helper_vtables.h> > > > >>> #include <drm/drm_vblank.h> > > > >>> +#include <drm/drm_panic.h> > > > >> > > > >>> +#include "../../drm_internal.h" > > > >> > > > >> Well that this file is named "internal" and not in a common include > > > >> directory is a strong indicator that you should absolutely *not* > > > >> include it in a driver. > > > >> > > > >>> #include "amdgpu.h" > > > >>> #include "amdgpu_pm.h" > > > >>> @@ -2600,6 +2602,35 @@ static const struct drm_crtc_helper_funcs > > > >>> dce_v6_0_crtc_helper_funcs = { > > > >>> .get_scanout_position = amdgpu_crtc_get_scanout_position, > > > >>> }; > > > >>> +static int dce_v6_0_drm_primary_plane_get_scanout_buffer(struct > > > >>> drm_plane *plane, > > > >>> + struct drm_scanout_buffer *sb) > > > >>> +{ > > > >>> + struct drm_framebuffer *fb; > > > >>> + struct drm_gem_object *obj; > > > >>> + struct amdgpu_bo *abo; > > > >>> + int ret = 0; > > > >>> + > > > >>> + if (!plane->fb || plane->fb->modifier != DRM_FORMAT_MOD_LINEAR) > > > >>> + return -ENODEV; > > > >>> + > > > >>> + fb = plane->fb; > > > >>> + sb->width = fb->width; > > > >>> + sb->height = fb->height; > > > >>> + sb->format = fb->format; > > > >>> + sb->pitch[0] = fb->pitches[0]; > > > >>> + > > > >>> + obj = fb->obj[0]; > > > >>> + abo = gem_to_amdgpu_bo(obj); > > > >>> + if (!abo || abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) > > > >>> + return -EINVAL; > > > >>> + > > > >>> + return drm_gem_vmap(obj, &sb->map[0]); > > > >> > > > >> Yeah that will almost always not work. Most display buffers are > > > >> tilled and not CPU accessible. > > > > > > > > For the CPU accessible issue, Christian mentioned there was a debug > > > > interface on AMD GPU that can be used, to work around this: > > > > > > > > https://lore.kernel.org/dri-devel/0baabe1f-8924-2c9a-5cd4-59084a37dbb2@gmail.com/ > > > > and > > > > https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/ > > > > > > > > And you will need to use the scanout_buffer->set_pixel() callback to > > > > write the pixels one by one, similar to what I've tried for nouveau with > > > > https://patchwork.freedesktop.org/series/133963/ > > > > > > > > For the tiling format, the problem is that it is internal to the GPU, > > > > and currently the driver don't know which tiling format is being used. > > > > > > > > It might be possible to disable tiling and compression, but it > > > > requires some internal DC knowledge: > > > > https://lore.kernel.org/dri-devel/f76a3297-7d63-8615-45c5-47f02b64a1d5@amd.com/ > > > > > > > > > > > > Best regards, > > > > > > From the discussion provided, it is difficult to implement this feature without the relevant data book and knowledge.(Whether how tiled memory storage, or how to disable tiling of DC) > > > > For DCE 6, the GRPH_ARRAY_MODE field in mmGRPH_CONTROL controls the > > display tiling. Set that field to GRPH_ARRAY_LINEAR_GENERAL (0) to > > disable tiling. > > For clarity that register is instanced so use mmGRPH_CONTROL + > amdgpu_crtc->crtc_offset to get the right instance. > It works, I'll submit a new patch soon. Thanks, Lu Yao > Alex
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c index 05c0df97f01d..12c3801c264a 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c @@ -28,6 +28,8 @@ #include <drm/drm_modeset_helper.h> #include <drm/drm_modeset_helper_vtables.h> #include <drm/drm_vblank.h> +#include <drm/drm_panic.h> +#include "../../drm_internal.h" #include "amdgpu.h" #include "amdgpu_pm.h" @@ -2600,6 +2602,35 @@ static const struct drm_crtc_helper_funcs dce_v6_0_crtc_helper_funcs = { .get_scanout_position = amdgpu_crtc_get_scanout_position, }; +static int dce_v6_0_drm_primary_plane_get_scanout_buffer(struct drm_plane *plane, + struct drm_scanout_buffer *sb) +{ + struct drm_framebuffer *fb; + struct drm_gem_object *obj; + struct amdgpu_bo *abo; + int ret = 0; + + if (!plane->fb || plane->fb->modifier != DRM_FORMAT_MOD_LINEAR) + return -ENODEV; + + fb = plane->fb; + sb->width = fb->width; + sb->height = fb->height; + sb->format = fb->format; + sb->pitch[0] = fb->pitches[0]; + + obj = fb->obj[0]; + abo = gem_to_amdgpu_bo(obj); + if (!abo || abo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) + return -EINVAL; + + return drm_gem_vmap(obj, &sb->map[0]); +} + +static const struct drm_plane_helper_funcs dce_v6_0_drm_primary_plane_helper_funcs = { + .get_scanout_buffer = dce_v6_0_drm_primary_plane_get_scanout_buffer +}; + static int dce_v6_0_crtc_init(struct amdgpu_device *adev, int index) { struct amdgpu_crtc *amdgpu_crtc; @@ -2627,6 +2658,7 @@ static int dce_v6_0_crtc_init(struct amdgpu_device *adev, int index) amdgpu_crtc->encoder = NULL; amdgpu_crtc->connector = NULL; drm_crtc_helper_add(&amdgpu_crtc->base, &dce_v6_0_crtc_helper_funcs); + drm_plane_helper_add(amdgpu_crtc->base.primary, &dce_v6_0_drm_primary_plane_helper_funcs); return 0; }
Add support for the drm_panic module, which displays a pretty user friendly message on the screen when a Linux kernel panic occurs. Signed-off-by: Lu Yao <yaolu@kylinos.cn> --- The patch can work properly on the TTY, but after start X, drawn image is messy, it looks like the data isn't linearly arranged. However at this time 'fb->modifier' is 'DRM_FORMAT_MOD_LINEAR'. Another difference I found is: For TTY, the amdgpu_bo is created with flag 'AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED|AMDGPU_GEM_CREATE_CPU_GTT_USWC| AMDGPU_GEM_CREATE_VRAM_CLEARED|AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS'. For X, the amdgpu_bo is created with flag 'AMDGPU_GEM_CREATE_NO_CPU_ACCESS|AMDGPU_GEM_CREATE_CPU_GTT_USWC' I try to use same flag for X, it looks like no difference. Can someone provide some insight into this problem or where I am going wrong. Thanks a lot. Test environment: X86 arch + v6.6 kernel + R7340. --- drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 32 +++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)