Re: [PATCH] drm/amdgpu: add dce6 drm_panic support

[Lu Yao <yaolu@kylinos.cn>]

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