1. Patchew
  2. linux
  3. [PATCH 0/7] drm/msm: Various snapshot fixes
  4. View patch

[PATCH 1/7] drm/msm: Add missing "location"s to devcoredump

Rob Clark posted 7 patches 2 months, 1 week ago
There is a newer version of this series
[PATCH 1/7] drm/msm: Add missing "location"s to devcoredump
Posted by Rob Clark 2 months, 1 week ago 
This is needed to properly interpret some of the sections.

Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
index faca2a0243ab..e586577e90de 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
@@ -1796,6 +1796,7 @@ static void a7xx_show_shader(struct a6xx_gpu_state_obj *obj,
 
 	print_name(p, "  - type: ", a7xx_statetype_names[block->statetype]);
 	print_name(p, "    - pipe: ", a7xx_pipe_names[block->pipeid]);
+	drm_printf(p, "    - location: %d", block->location);
 
 	for (i = 0; i < block->num_sps; i++) {
 		drm_printf(p, "      - sp: %d\n", i);
@@ -1873,6 +1874,7 @@ static void a7xx_show_dbgahb_cluster(struct a6xx_gpu_state_obj *obj,
 		print_name(p, "  - pipe: ", a7xx_pipe_names[dbgahb->pipe_id]);
 		print_name(p, "    - cluster-name: ", a7xx_cluster_names[dbgahb->cluster_id]);
 		drm_printf(p, "      - context: %d\n", dbgahb->context_id);
+		drm_printf(p, "      - location: %d\n", dbgahb->location_id);
 		a7xx_show_registers_indented(dbgahb->regs, obj->data, p, 4);
 	}
 }
-- 
2.50.1
Re: [PATCH 1/7] drm/msm: Add missing "location"s to devcoredump
Posted by Connor Abbott 2 months, 1 week ago 
On Mon, Jul 28, 2025 at 4:43 PM Rob Clark <robin.clark@oss.qualcomm.com> wrote:
>
> This is needed to properly interpret some of the sections.
>
> Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> index faca2a0243ab..e586577e90de 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> @@ -1796,6 +1796,7 @@ static void a7xx_show_shader(struct a6xx_gpu_state_obj *obj,
>
>         print_name(p, "  - type: ", a7xx_statetype_names[block->statetype]);
>         print_name(p, "    - pipe: ", a7xx_pipe_names[block->pipeid]);
> +       drm_printf(p, "    - location: %d", block->location);

We should probably at least try to keep it proper YAML by indenting
everything after another level...

>
>         for (i = 0; i < block->num_sps; i++) {
>                 drm_printf(p, "      - sp: %d\n", i);
> @@ -1873,6 +1874,7 @@ static void a7xx_show_dbgahb_cluster(struct a6xx_gpu_state_obj *obj,
>                 print_name(p, "  - pipe: ", a7xx_pipe_names[dbgahb->pipe_id]);
>                 print_name(p, "    - cluster-name: ", a7xx_cluster_names[dbgahb->cluster_id]);
>                 drm_printf(p, "      - context: %d\n", dbgahb->context_id);
> +               drm_printf(p, "      - location: %d\n", dbgahb->location_id);
>                 a7xx_show_registers_indented(dbgahb->regs, obj->data, p, 4);
>         }
>  }
> --
> 2.50.1
>
Re: [PATCH 1/7] drm/msm: Add missing "location"s to devcoredump
Posted by Rob Clark 2 months, 1 week ago 
On Mon, Jul 28, 2025 at 2:04 PM Connor Abbott <cwabbott0@gmail.com> wrote:
>
> On Mon, Jul 28, 2025 at 4:43 PM Rob Clark <robin.clark@oss.qualcomm.com> wrote:
> >
> > This is needed to properly interpret some of the sections.
> >
> > Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
> > ---
> >  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > index faca2a0243ab..e586577e90de 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > @@ -1796,6 +1796,7 @@ static void a7xx_show_shader(struct a6xx_gpu_state_obj *obj,
> >
> >         print_name(p, "  - type: ", a7xx_statetype_names[block->statetype]);
> >         print_name(p, "    - pipe: ", a7xx_pipe_names[block->pipeid]);
> > +       drm_printf(p, "    - location: %d", block->location);
>
> We should probably at least try to keep it proper YAML by indenting
> everything after another level...

this made me realize I missed a \n... but otherwise I think the indent
is correct?  Or should location not have a leading '-'?

BR,
-R

>
> >
> >         for (i = 0; i < block->num_sps; i++) {
> >                 drm_printf(p, "      - sp: %d\n", i);
> > @@ -1873,6 +1874,7 @@ static void a7xx_show_dbgahb_cluster(struct a6xx_gpu_state_obj *obj,
> >                 print_name(p, "  - pipe: ", a7xx_pipe_names[dbgahb->pipe_id]);
> >                 print_name(p, "    - cluster-name: ", a7xx_cluster_names[dbgahb->cluster_id]);
> >                 drm_printf(p, "      - context: %d\n", dbgahb->context_id);
> > +               drm_printf(p, "      - location: %d\n", dbgahb->location_id);
> >                 a7xx_show_registers_indented(dbgahb->regs, obj->data, p, 4);
> >         }
> >  }
> > --
> > 2.50.1
> >
Re: [PATCH 1/7] drm/msm: Add missing "location"s to devcoredump
Posted by Rob Clark 2 months, 1 week ago 
On Mon, Jul 28, 2025 at 3:15 PM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
>
> On Mon, Jul 28, 2025 at 2:04 PM Connor Abbott <cwabbott0@gmail.com> wrote:
> >
> > On Mon, Jul 28, 2025 at 4:43 PM Rob Clark <robin.clark@oss.qualcomm.com> wrote:
> > >
> > > This is needed to properly interpret some of the sections.
> > >
> > > Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
> > > ---
> > >  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > > index faca2a0243ab..e586577e90de 100644
> > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > > @@ -1796,6 +1796,7 @@ static void a7xx_show_shader(struct a6xx_gpu_state_obj *obj,
> > >
> > >         print_name(p, "  - type: ", a7xx_statetype_names[block->statetype]);
> > >         print_name(p, "    - pipe: ", a7xx_pipe_names[block->pipeid]);
> > > +       drm_printf(p, "    - location: %d", block->location);
> >
> > We should probably at least try to keep it proper YAML by indenting
> > everything after another level...
>
> this made me realize I missed a \n... but otherwise I think the indent
> is correct?  Or should location not have a leading '-'?

beyond that, even without the added location field, some random online
yaml checker is telling me that we were already not proper yaml.. so I
guess, :shrug:?

BR,
-R
Re: [PATCH 1/7] drm/msm: Add missing "location"s to devcoredump
Posted by Connor Abbott 2 months ago 
On Tue, Jul 29, 2025 at 9:40 AM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
>
> On Mon, Jul 28, 2025 at 3:15 PM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
> >
> > On Mon, Jul 28, 2025 at 2:04 PM Connor Abbott <cwabbott0@gmail.com> wrote:
> > >
> > > On Mon, Jul 28, 2025 at 4:43 PM Rob Clark <robin.clark@oss.qualcomm.com> wrote:
> > > >
> > > > This is needed to properly interpret some of the sections.
> > > >
> > > > Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
> > > > ---
> > > >  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > > > index faca2a0243ab..e586577e90de 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > > > @@ -1796,6 +1796,7 @@ static void a7xx_show_shader(struct a6xx_gpu_state_obj *obj,
> > > >
> > > >         print_name(p, "  - type: ", a7xx_statetype_names[block->statetype]);
> > > >         print_name(p, "    - pipe: ", a7xx_pipe_names[block->pipeid]);
> > > > +       drm_printf(p, "    - location: %d", block->location);
> > >
> > > We should probably at least try to keep it proper YAML by indenting
> > > everything after another level...
> >
> > this made me realize I missed a \n... but otherwise I think the indent
> > is correct?  Or should location not have a leading '-'?
>
> beyond that, even without the added location field, some random online
> yaml checker is telling me that we were already not proper yaml.. so I
> guess, :shrug:?
>
> BR,
> -R

Before this change, it looked like this:

  - pipe: A7XX_PIPE_BR
    - cluster-name: A7XX_CLUSTER_SP_PS
      - context: 3
        - { offset: 0x02a718, value: 0x00000003 }
        ...

Notice that each nested thing (pipe -> cluster -> context) has an
additional level of indentation. Now, it looks like this:

  - pipe: A7XX_PIPE_BR
    - cluster-name: A7XX_CLUSTER_SP_PS
      - context: 3
      - location: 4
        - { offset: 0x02a718, value: 0x00000003 }
        ...

So it looks a bit weird with the context and location not being
nested. Also, I think the correct nesting HW-wise is cluster ->
location -> context, rather than context-> location, so the location
should be first. But ultimately it's up to you.

Connor
Re: [PATCH 1/7] drm/msm: Add missing "location"s to devcoredump
Posted by Rob Clark 2 months ago 
On Thu, Jul 31, 2025 at 12:16 PM Connor Abbott <cwabbott0@gmail.com> wrote:
>
> On Tue, Jul 29, 2025 at 9:40 AM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
> >
> > On Mon, Jul 28, 2025 at 3:15 PM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
> > >
> > > On Mon, Jul 28, 2025 at 2:04 PM Connor Abbott <cwabbott0@gmail.com> wrote:
> > > >
> > > > On Mon, Jul 28, 2025 at 4:43 PM Rob Clark <robin.clark@oss.qualcomm.com> wrote:
> > > > >
> > > > > This is needed to properly interpret some of the sections.
> > > > >
> > > > > Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
> > > > > ---
> > > > >  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > > > > index faca2a0243ab..e586577e90de 100644
> > > > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > > > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> > > > > @@ -1796,6 +1796,7 @@ static void a7xx_show_shader(struct a6xx_gpu_state_obj *obj,
> > > > >
> > > > >         print_name(p, "  - type: ", a7xx_statetype_names[block->statetype]);
> > > > >         print_name(p, "    - pipe: ", a7xx_pipe_names[block->pipeid]);
> > > > > +       drm_printf(p, "    - location: %d", block->location);
> > > >
> > > > We should probably at least try to keep it proper YAML by indenting
> > > > everything after another level...
> > >
> > > this made me realize I missed a \n... but otherwise I think the indent
> > > is correct?  Or should location not have a leading '-'?
> >
> > beyond that, even without the added location field, some random online
> > yaml checker is telling me that we were already not proper yaml.. so I
> > guess, :shrug:?
> >
> > BR,
> > -R
>
> Before this change, it looked like this:
>
>   - pipe: A7XX_PIPE_BR
>     - cluster-name: A7XX_CLUSTER_SP_PS
>       - context: 3
>         - { offset: 0x02a718, value: 0x00000003 }
>         ...
>
> Notice that each nested thing (pipe -> cluster -> context) has an
> additional level of indentation. Now, it looks like this:
>
>   - pipe: A7XX_PIPE_BR
>     - cluster-name: A7XX_CLUSTER_SP_PS
>       - context: 3
>       - location: 4
>         - { offset: 0x02a718, value: 0x00000003 }
>         ...
>
> So it looks a bit weird with the context and location not being
> nested. Also, I think the correct nesting HW-wise is cluster ->
> location -> context, rather than context-> location, so the location
> should be first. But ultimately it's up to you.

In terms of nesting, type, pipe, and location are all at the same
level, and then for that tuple there is SPs nested under that, and
then USPTPs nested under the SPs.  Although I guess we already had
pipe nested under type..

BR,
-R

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.