| Message ID | 20180321103228.32205-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Tvrtko Ursulin (2018-03-21 10:32:28) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Log up to sseu->max_slices instead basing on ARRAY_SIZE since to avoid > printing impossible and empty slices for a platform. > > Also compact slice total and slice mask into one log line. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/i915/intel_device_info.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c > index 4babfc6ee45b..68aa9746d0e1 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -83,11 +83,11 @@ static void sseu_dump(const struct sseu_dev_info *sseu, struct drm_printer *p) > { > int s; > > - drm_printf(p, "slice mask: %04x\n", sseu->slice_mask); > - drm_printf(p, "slice total: %u\n", hweight8(sseu->slice_mask)); > + drm_printf(p, "slice total: %u, mask=%04x\n", > + hweight8(sseu->slice_mask), sseu->slice_mask); > drm_printf(p, "subslice total: %u\n", sseu_subslice_total(sseu)); > - for (s = 0; s < ARRAY_SIZE(sseu->subslice_mask); s++) { > - drm_printf(p, "slice%d %u subslices mask=%04x\n", > + for (s = 0; s < sseu->max_slices; s++) { > + drm_printf(p, "slice%d: %u subslices, mask=%04x\n", > s, hweight8(sseu->subslice_mask[s]), > sseu->subslice_mask[s]); Just idly testing the waters... In yaml, this would be "<indent>- slice%d: { subslices: %u, mask: 0x%04x }\n" How do we feel about that in our debug output? And gradually use that style by default? (I'm planning on converting the error state wholesale...) -chris
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> On 21/03/18 10:32, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Log up to sseu->max_slices instead basing on ARRAY_SIZE since to avoid > printing impossible and empty slices for a platform. > > Also compact slice total and slice mask into one log line. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/i915/intel_device_info.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c > index 4babfc6ee45b..68aa9746d0e1 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -83,11 +83,11 @@ static void sseu_dump(const struct sseu_dev_info *sseu, struct drm_printer *p) > { > int s; > > - drm_printf(p, "slice mask: %04x\n", sseu->slice_mask); > - drm_printf(p, "slice total: %u\n", hweight8(sseu->slice_mask)); > + drm_printf(p, "slice total: %u, mask=%04x\n", > + hweight8(sseu->slice_mask), sseu->slice_mask); > drm_printf(p, "subslice total: %u\n", sseu_subslice_total(sseu)); > - for (s = 0; s < ARRAY_SIZE(sseu->subslice_mask); s++) { > - drm_printf(p, "slice%d %u subslices mask=%04x\n", > + for (s = 0; s < sseu->max_slices; s++) { > + drm_printf(p, "slice%d: %u subslices, mask=%04x\n", > s, hweight8(sseu->subslice_mask[s]), > sseu->subslice_mask[s]); > }
Quoting Chris Wilson (2018-03-21 10:41:37) > Quoting Tvrtko Ursulin (2018-03-21 10:32:28) > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > Log up to sseu->max_slices instead basing on ARRAY_SIZE since to avoid > > printing impossible and empty slices for a platform. > > > > Also compact slice total and slice mask into one log line. > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > --- > > drivers/gpu/drm/i915/intel_device_info.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c > > index 4babfc6ee45b..68aa9746d0e1 100644 > > --- a/drivers/gpu/drm/i915/intel_device_info.c > > +++ b/drivers/gpu/drm/i915/intel_device_info.c > > @@ -83,11 +83,11 @@ static void sseu_dump(const struct sseu_dev_info *sseu, struct drm_printer *p) > > { > > int s; > > > > - drm_printf(p, "slice mask: %04x\n", sseu->slice_mask); > > - drm_printf(p, "slice total: %u\n", hweight8(sseu->slice_mask)); > > + drm_printf(p, "slice total: %u, mask=%04x\n", > > + hweight8(sseu->slice_mask), sseu->slice_mask); > > drm_printf(p, "subslice total: %u\n", sseu_subslice_total(sseu)); > > - for (s = 0; s < ARRAY_SIZE(sseu->subslice_mask); s++) { > > - drm_printf(p, "slice%d %u subslices mask=%04x\n", > > + for (s = 0; s < sseu->max_slices; s++) { > > + drm_printf(p, "slice%d: %u subslices, mask=%04x\n", > > s, hweight8(sseu->subslice_mask[s]), > > sseu->subslice_mask[s]); > > Just idly testing the waters... > > In yaml, this would be > "<indent>- slice%d: { subslices: %u, mask: 0x%04x }\n" Or if we keep the node name the same for easier parsing: "<indent>- slice: { id:%u, subslices:%u, mask:0x%04x }\n" -Chris
On Wed, 21 Mar 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Chris Wilson (2018-03-21 10:41:37) >> Quoting Tvrtko Ursulin (2018-03-21 10:32:28) >> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> > >> > Log up to sseu->max_slices instead basing on ARRAY_SIZE since to avoid >> > printing impossible and empty slices for a platform. >> > >> > Also compact slice total and slice mask into one log line. >> > >> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_device_info.c | 8 ++++---- >> > 1 file changed, 4 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c >> > index 4babfc6ee45b..68aa9746d0e1 100644 >> > --- a/drivers/gpu/drm/i915/intel_device_info.c >> > +++ b/drivers/gpu/drm/i915/intel_device_info.c >> > @@ -83,11 +83,11 @@ static void sseu_dump(const struct sseu_dev_info *sseu, struct drm_printer *p) >> > { >> > int s; >> > >> > - drm_printf(p, "slice mask: %04x\n", sseu->slice_mask); >> > - drm_printf(p, "slice total: %u\n", hweight8(sseu->slice_mask)); >> > + drm_printf(p, "slice total: %u, mask=%04x\n", >> > + hweight8(sseu->slice_mask), sseu->slice_mask); >> > drm_printf(p, "subslice total: %u\n", sseu_subslice_total(sseu)); >> > - for (s = 0; s < ARRAY_SIZE(sseu->subslice_mask); s++) { >> > - drm_printf(p, "slice%d %u subslices mask=%04x\n", >> > + for (s = 0; s < sseu->max_slices; s++) { >> > + drm_printf(p, "slice%d: %u subslices, mask=%04x\n", >> > s, hweight8(sseu->subslice_mask[s]), >> > sseu->subslice_mask[s]); >> >> Just idly testing the waters... >> >> In yaml, this would be >> "<indent>- slice%d: { subslices: %u, mask: 0x%04x }\n" > > Or if we keep the node name the same for easier parsing: > > "<indent>- slice: { id:%u, subslices:%u, mask:0x%04x }\n" I'm not against doing this, especially for gpu dumps. Wouldn't json be easier to generate and parse? Or do you prefer the slightly better human readability of yaml? I think it would be pretty straighforward to write drm printer helpers for printing valid json without having to actually manually print the colons and braces etc. And the struct drm_printer could even have checks for ensuring you don't burp verbatim stuff to a printer that's supposed to be json. Any considerations for the transition? Massive wholesale patch bomb conversion? Yikes. BR, Jani.
Quoting Jani Nikula (2018-03-21 11:47:06) > > On Wed, 21 Mar 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Chris Wilson (2018-03-21 10:41:37) > >> Quoting Tvrtko Ursulin (2018-03-21 10:32:28) > >> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > > >> > Log up to sseu->max_slices instead basing on ARRAY_SIZE since to avoid > >> > printing impossible and empty slices for a platform. > >> > > >> > Also compact slice total and slice mask into one log line. > >> > > >> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/intel_device_info.c | 8 ++++---- > >> > 1 file changed, 4 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c > >> > index 4babfc6ee45b..68aa9746d0e1 100644 > >> > --- a/drivers/gpu/drm/i915/intel_device_info.c > >> > +++ b/drivers/gpu/drm/i915/intel_device_info.c > >> > @@ -83,11 +83,11 @@ static void sseu_dump(const struct sseu_dev_info *sseu, struct drm_printer *p) > >> > { > >> > int s; > >> > > >> > - drm_printf(p, "slice mask: %04x\n", sseu->slice_mask); > >> > - drm_printf(p, "slice total: %u\n", hweight8(sseu->slice_mask)); > >> > + drm_printf(p, "slice total: %u, mask=%04x\n", > >> > + hweight8(sseu->slice_mask), sseu->slice_mask); > >> > drm_printf(p, "subslice total: %u\n", sseu_subslice_total(sseu)); > >> > - for (s = 0; s < ARRAY_SIZE(sseu->subslice_mask); s++) { > >> > - drm_printf(p, "slice%d %u subslices mask=%04x\n", > >> > + for (s = 0; s < sseu->max_slices; s++) { > >> > + drm_printf(p, "slice%d: %u subslices, mask=%04x\n", > >> > s, hweight8(sseu->subslice_mask[s]), > >> > sseu->subslice_mask[s]); > >> > >> Just idly testing the waters... > >> > >> In yaml, this would be > >> "<indent>- slice%d: { subslices: %u, mask: 0x%04x }\n" > > > > Or if we keep the node name the same for easier parsing: > > > > "<indent>- slice: { id:%u, subslices:%u, mask:0x%04x }\n" > > I'm not against doing this, especially for gpu dumps. > > Wouldn't json be easier to generate and parse? Or do you prefer the > slightly better human readability of yaml? I think for any of the debug output preferring to keep it as readable as possible is essential. libyaml isn't that hard to use, even for a beginner like myself. > I think it would be pretty straighforward to write drm printer helpers > for printing valid json without having to actually manually print the > colons and braces etc. And the struct drm_printer could even have checks > for ensuring you don't burp verbatim stuff to a printer that's supposed > to be json. About the biggest challenge is tracking indent; which drm_printer already does iirc. Still, I think we want to move this into lib/ > Any considerations for the transition? Massive wholesale patch bomb > conversion? Yikes. I think it's only worth converting bits and pieces that we are trying to parse. So quite a few debugfs are candidates, and the error-state being a prime example as we want to make it more amenable and flexible for future post-mortem capture depending on what userspace needs. (I might even go as far as all future debugfs should come with a parser for igt.) -Chris
On Wed, 21 Mar 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Jani Nikula (2018-03-21 11:47:06) >> >> On Wed, 21 Mar 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > Quoting Chris Wilson (2018-03-21 10:41:37) >> >> Quoting Tvrtko Ursulin (2018-03-21 10:32:28) >> >> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> > >> >> > Log up to sseu->max_slices instead basing on ARRAY_SIZE since to avoid >> >> > printing impossible and empty slices for a platform. >> >> > >> >> > Also compact slice total and slice mask into one log line. >> >> > >> >> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> >> > --- >> >> > drivers/gpu/drm/i915/intel_device_info.c | 8 ++++---- >> >> > 1 file changed, 4 insertions(+), 4 deletions(-) >> >> > >> >> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c >> >> > index 4babfc6ee45b..68aa9746d0e1 100644 >> >> > --- a/drivers/gpu/drm/i915/intel_device_info.c >> >> > +++ b/drivers/gpu/drm/i915/intel_device_info.c >> >> > @@ -83,11 +83,11 @@ static void sseu_dump(const struct sseu_dev_info *sseu, struct drm_printer *p) >> >> > { >> >> > int s; >> >> > >> >> > - drm_printf(p, "slice mask: %04x\n", sseu->slice_mask); >> >> > - drm_printf(p, "slice total: %u\n", hweight8(sseu->slice_mask)); >> >> > + drm_printf(p, "slice total: %u, mask=%04x\n", >> >> > + hweight8(sseu->slice_mask), sseu->slice_mask); >> >> > drm_printf(p, "subslice total: %u\n", sseu_subslice_total(sseu)); >> >> > - for (s = 0; s < ARRAY_SIZE(sseu->subslice_mask); s++) { >> >> > - drm_printf(p, "slice%d %u subslices mask=%04x\n", >> >> > + for (s = 0; s < sseu->max_slices; s++) { >> >> > + drm_printf(p, "slice%d: %u subslices, mask=%04x\n", >> >> > s, hweight8(sseu->subslice_mask[s]), >> >> > sseu->subslice_mask[s]); >> >> >> >> Just idly testing the waters... >> >> >> >> In yaml, this would be >> >> "<indent>- slice%d: { subslices: %u, mask: 0x%04x }\n" >> > >> > Or if we keep the node name the same for easier parsing: >> > >> > "<indent>- slice: { id:%u, subslices:%u, mask:0x%04x }\n" >> >> I'm not against doing this, especially for gpu dumps. >> >> Wouldn't json be easier to generate and parse? Or do you prefer the >> slightly better human readability of yaml? > > I think for any of the debug output preferring to keep it as readable as > possible is essential. libyaml isn't that hard to use, even for a > beginner like myself. Fair enough; I'm only familiar with json, and that's my natural reason to lean towards it. >> I think it would be pretty straighforward to write drm printer helpers >> for printing valid json without having to actually manually print the >> colons and braces etc. And the struct drm_printer could even have checks >> for ensuring you don't burp verbatim stuff to a printer that's supposed >> to be json. > > About the biggest challenge is tracking indent; which drm_printer > already does iirc. Still, I think we want to move this into lib/ I agree having this in lib is preferrable. But apparently that requires moving the whole abstracted drm_printer style printing there first, and then hoping to get yaml/json added on top. There's bound to be some friction there. >> Any considerations for the transition? Massive wholesale patch bomb >> conversion? Yikes. > > I think it's only worth converting bits and pieces that we are trying to > parse. So quite a few debugfs are candidates, and the error-state being > a prime example as we want to make it more amenable and flexible for > future post-mortem capture depending on what userspace needs. (I might > even go as far as all future debugfs should come with a parser for igt.) Oh, I did mean mass conversion on a per-debugfs-file basis, i.e. error state in one go. But I guess there's no way around that, anything else is just pain spread over a longer period. I'd definitely go as far as saying any debugfs file that's supposed to be in a serialized format should have a corresponding parser in igt, and written *before* merging the kernel change. There's also some discipline required wrt backward/forward compatibility to actually reap the benefits of the format. Can't just change the contents nilly willy with that either. It's not a magic bullet. BR, Jani.
Quoting Jani Nikula (2018-03-21 14:16:37) > On Wed, 21 Mar 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Jani Nikula (2018-03-21 11:47:06) > >> > >> > Quoting Chris Wilson (2018-03-21 10:41:37) > >> >> > >> >> Just idly testing the waters... > >> >> > >> >> In yaml, this would be > >> >> "<indent>- slice%d: { subslices: %u, mask: 0x%04x }\n" > >> > > >> > Or if we keep the node name the same for easier parsing: > >> > > >> > "<indent>- slice: { id:%u, subslices:%u, mask:0x%04x }\n" > >> > >> I'm not against doing this, especially for gpu dumps. > >> > >> Wouldn't json be easier to generate and parse? Or do you prefer the > >> slightly better human readability of yaml? > > > > I think for any of the debug output preferring to keep it as readable as > > possible is essential. libyaml isn't that hard to use, even for a > > beginner like myself. > > Fair enough; I'm only familiar with json, and that's my natural reason > to lean towards it. JSON gets irritating to look at by plain eye. So I would agree that YAML would serve better when developers are likely to scroll through the file contents manually, too. Then some more random thoughts: This discussion will of course bring closer the can of forms that when we've formalized the debugfs format, does somebody expect it to stay more stable than not? Having a counterpart in IGT and making that always match kernel, would be the obvious way to keep things sane. Regards, Joonas
Quoting Joonas Lahtinen (2018-03-23 12:29:54) > Quoting Jani Nikula (2018-03-21 14:16:37) > > On Wed, 21 Mar 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > Quoting Jani Nikula (2018-03-21 11:47:06) > > >> > > >> > Quoting Chris Wilson (2018-03-21 10:41:37) > > >> >> > > >> >> Just idly testing the waters... > > >> >> > > >> >> In yaml, this would be > > >> >> "<indent>- slice%d: { subslices: %u, mask: 0x%04x }\n" > > >> > > > >> > Or if we keep the node name the same for easier parsing: > > >> > > > >> > "<indent>- slice: { id:%u, subslices:%u, mask:0x%04x }\n" > > >> > > >> I'm not against doing this, especially for gpu dumps. > > >> > > >> Wouldn't json be easier to generate and parse? Or do you prefer the > > >> slightly better human readability of yaml? > > > > > > I think for any of the debug output preferring to keep it as readable as > > > possible is essential. libyaml isn't that hard to use, even for a > > > beginner like myself. > > > > Fair enough; I'm only familiar with json, and that's my natural reason > > to lean towards it. > > JSON gets irritating to look at by plain eye. So I would agree that YAML > would serve better when developers are likely to scroll through the file > contents manually, too. > > Then some more random thoughts: > > This discussion will of course bring closer the can of forms that when > we've formalized the debugfs format, does somebody expect it to stay > more stable than not? The starting point is that we do want to transition to an easier-to-extend file format for sysfs (the idea of it being single-valued is long gone :) There having a stable interface is without question, so getting the transition right is the challenge and then maintaining the hierarchy such that it is backwards and forward compatible with new content. > Having a counterpart in IGT and making that always match kernel, would > be the obvious way to keep things sane. Imo, if we aren't parsing debugfs in igt, then they can be dead code eliminated. So I expect yamlification to snow ball once we've got the first few done and a good feel for how to design kernel/igt API to suite us. Then it's just a matter of throwing each debugfs through a yamllint :) -Chris
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index 4babfc6ee45b..68aa9746d0e1 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -83,11 +83,11 @@ static void sseu_dump(const struct sseu_dev_info *sseu, struct drm_printer *p) { int s; - drm_printf(p, "slice mask: %04x\n", sseu->slice_mask); - drm_printf(p, "slice total: %u\n", hweight8(sseu->slice_mask)); + drm_printf(p, "slice total: %u, mask=%04x\n", + hweight8(sseu->slice_mask), sseu->slice_mask); drm_printf(p, "subslice total: %u\n", sseu_subslice_total(sseu)); - for (s = 0; s < ARRAY_SIZE(sseu->subslice_mask); s++) { - drm_printf(p, "slice%d %u subslices mask=%04x\n", + for (s = 0; s < sseu->max_slices; s++) { + drm_printf(p, "slice%d: %u subslices, mask=%04x\n", s, hweight8(sseu->subslice_mask[s]), sseu->subslice_mask[s]); }