Re: [Freedreno] [PATCH 02/13] drm: drm_printer: Add printer for devcoredump
On Thu, Jul 12, 2018 at 08:40:55PM +0100, Chris Wilson wrote:
> Quoting Jordan Crouse (2018-07-12 19:59:19)
> > Add a drm printer suitable for use with the read callback for
> > devcoredump or other suitable buffer based output format that
> > isn't otherwise covered by seq_file.
> >
> > Signed-off-by: Jordan Crouse
> > ---
> > drivers/gpu/drm/drm_print.c | 74 +
> > include/drm/drm_print.h | 27 ++
> > 2 files changed, 101 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> > index b25f98f33f6c..03d1f98e5ac7 100644
> > --- a/drivers/gpu/drm/drm_print.c
> > +++ b/drivers/gpu/drm/drm_print.c
> > @@ -30,6 +30,80 @@
> > #include
> > #include
> >
> > +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf)
> > +{
> > + struct drm_print_iterator *iterator = p->arg;
> > + ssize_t len;
> > +
> > + if (!iterator->remain)
> > + return;
> > +
> > + /* Figure out how big the string will be */
> > + len = snprintf(NULL, 0, "%pV", vaf);
>
> I was thinking there's some duplication here (kmalloc + snprintf) that
> could be reduced to kasprintf here. Is avoiding that allocation
> important or frequent enough to merit open coding?
> It's pity the kernel's printk doesn't support %n, so that leaves with
>
> buf = kasprintf(GFP_... , "%pV", vaf);
> if (!buf)
> return;
>
> len = strlen(buf);
>
> and even the copy + increment looks like it can then be factored to share
> more code.
I did a quick test - using kasprintf() unconditionally increased the total
time in my use case by about 4x. I think this was mainly due to this case:
if (iterator->offset + len <= iterator->start) {
return;
}
That said, I was able to organize the code so that we can reuse much of the
same code for both the printf and puts funcs which saves us a bit of churn
later.
Jordan
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [Freedreno] [PATCH 02/13] drm: drm_printer: Add printer for devcoredump
> > Hm, why not add seq_file support to dev_coredump? Neither git blame > > nor google sched any light on why seq_file wasn't picked over the > > custom read interface ... > > > > Adding Johannes and Greg about this. > > Main reason was that this is used for devcoredump which has its own similar > but not quite seq_file compatible callback. If there is synergy to be had > there > that would be great because reinventing the wheel isn't fun. Adding or changing it to seq_file is fine with me, I don't think we really need the devm_coredump() much these days since we have the vmalloc one. (apologies for the footer and all - I'm on vacation and in a hurry) johannes -- Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [PATCH 02/13] drm: drm_printer: Add printer for devcoredump
On Thu, Jul 12, 2018 at 09:46:58PM +0200, Daniel Vetter wrote:
> On Thu, Jul 12, 2018 at 12:59:19PM -0600, Jordan Crouse wrote:
> > Add a drm printer suitable for use with the read callback for
> > devcoredump or other suitable buffer based output format that
> > isn't otherwise covered by seq_file.
> >
> > Signed-off-by: Jordan Crouse
>
> Hm, why not add seq_file support to dev_coredump? Neither git blame nor
> google sched any light on why seq_file wasn't picked over the custom read
> interface ...
>
> Adding Johannes and Greg about this.
Main reason was that this is used for devcoredump which has its own similar but
not quite seq_file compatible callback. If there is synergy to be had there
that would be great because reinventing the wheel isn't fun.
> If we go with this, one comment below.
>
> > ---
> > drivers/gpu/drm/drm_print.c | 74 +
> > include/drm/drm_print.h | 27 ++
> > 2 files changed, 101 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> > index b25f98f33f6c..03d1f98e5ac7 100644
> > --- a/drivers/gpu/drm/drm_print.c
> > +++ b/drivers/gpu/drm/drm_print.c
> > @@ -30,6 +30,80 @@
> > #include
> > #include
> >
> > +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf)
> > +{
> > + struct drm_print_iterator *iterator = p->arg;
> > + ssize_t len;
> > +
> > + if (!iterator->remain)
> > + return;
> > +
> > + /* Figure out how big the string will be */
> > + len = snprintf(NULL, 0, "%pV", vaf);
> > +
> > + if (iterator->offset < iterator->start) {
> > + char *buf;
> > + ssize_t copy;
> > +
> > + if (iterator->offset + len <= iterator->start) {
> > + iterator->offset += len;
> > + return;
> > + }
> > +
> > + /* Print the string into a temporary buffer */
> > + buf = kmalloc(len + 1,
> > + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
> > + if (!buf)
> > + return;
> > +
> > + snprintf(buf, len + 1, "%pV", vaf);
> > +
> > + copy = len - (iterator->start - iterator->offset);
> > +
> > + if (copy > iterator->remain)
> > + copy = iterator->remain;
> > +
> > + /* Copy out the bit of the string that we need */
> > + memcpy(iterator->data,
> > + buf + (iterator->start - iterator->offset), copy);
> > +
> > + iterator->offset = iterator->start + copy;
> > + iterator->remain -= copy;
> > +
> > + kfree(buf);
> > + } else {
> > + char *buf;
> > + ssize_t pos = iterator->offset - iterator->start;
> > +
> > + if (len < iterator->remain) {
> > + snprintf(((char *) iterator->data) + pos,
> > + iterator->remain, "%pV", vaf);
> > +
> > + iterator->offset += len;
> > + iterator->remain -= len;
> > +
> > + return;
> > + }
> > +
> > + /* Print the string into a temporary buffer */
> > + buf = kmalloc(len + 1,
> > + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
> > + if (!buf)
> > + return;
> > +
> > + snprintf(buf, len + 1, "%pV", vaf);
> > +
> > + /* Copy out the remaining bits */
> > + memcpy(iterator->data + pos, buf, iterator->remain);
> > +
> > + iterator->offset += iterator->remain;
> > + iterator->remain = 0;
> > +
> > + kfree(buf);
> > + }
> > +}
> > +EXPORT_SYMBOL(__drm_printfn_coredump);
> > +
> > void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf)
> > {
> > seq_printf(p->arg, "%pV", vaf);
> > diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> > index e1a46e9991cc..0ea440fb5ec3 100644
> > --- a/include/drm/drm_print.h
> > +++ b/include/drm/drm_print.h
> > @@ -73,6 +73,7 @@ struct drm_printer {
> > const char *prefix;
> > };
> >
> > +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf);
> > void __drm_printfn_seq_file(struct drm_printer *p, struct va_format *vaf);
> > void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf);
> > void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf);
> > @@ -104,6 +105,32 @@ drm_vprintf(struct drm_printer *p, const char *fmt,
> > va_list *va)
> > #define drm_printf_indent(printer, indent, fmt, ...) \
> > drm_printf((printer), "%.*s" fmt, (indent), "\t\t\t\t\tX", ##__VA_ARGS__)
> >
> > +struct drm_print_iterator {
> > + void *data;
> > +
> > + ssize_t start;
> > + ssize_t offset;
> > + ssize_t remain;
> > +};
> > +
> > +/**
> > + * drm_coredump_printer - construct a _printer that can output to a
> > buffer
> > + * from the read function for devcoredump
> > + * @iter: A pointer to a struct drm_print_iterator for the read instance
>
> Bit more flesh for the kerneldoc would be good here, maybe even with a
> small in-line example. Definitely a link to dev_coredumpm() which I assume
> is the function you're going to use this with.
> Pls also make sure it all looks nice using make htmldocs.
Can do. Thanks.
> -Daniel
>
>
> > + *
> > + * RETURNS:
> > + * The _printer object
> > + */
> > +static inline struct drm_printer
> > +drm_coredump_printer(struct drm_print_iterator *iter)
> > +{
> > + struct drm_printer p = {
> > + .printfn = __drm_printfn_coredump,
> > +
Re: [Freedreno] [PATCH 02/13] drm: drm_printer: Add printer for devcoredump
On Thu, Jul 12, 2018 at 08:40:55PM +0100, Chris Wilson wrote:
> Quoting Jordan Crouse (2018-07-12 19:59:19)
> > Add a drm printer suitable for use with the read callback for
> > devcoredump or other suitable buffer based output format that
> > isn't otherwise covered by seq_file.
> >
> > Signed-off-by: Jordan Crouse
> > ---
> > drivers/gpu/drm/drm_print.c | 74 +
> > include/drm/drm_print.h | 27 ++
> > 2 files changed, 101 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> > index b25f98f33f6c..03d1f98e5ac7 100644
> > --- a/drivers/gpu/drm/drm_print.c
> > +++ b/drivers/gpu/drm/drm_print.c
> > @@ -30,6 +30,80 @@
> > #include
> > #include
> >
> > +void __drm_printfn_coredump(struct drm_printer *p, struct va_format *vaf)
> > +{
> > + struct drm_print_iterator *iterator = p->arg;
> > + ssize_t len;
> > +
> > + if (!iterator->remain)
> > + return;
> > +
> > + /* Figure out how big the string will be */
> > + len = snprintf(NULL, 0, "%pV", vaf);
>
> I was thinking there's some duplication here (kmalloc + snprintf) that
> could be reduced to kasprintf here. Is avoiding that allocation
> important or frequent enough to merit open coding?
>
> It's pity the kernel's printk doesn't support %n, so that leaves with
>
> buf = kasprintf(GFP_... , "%pV", vaf);
> if (!buf)
> return;
>
> len = strlen(buf);
> and even the copy + increment looks like it can then be factored to share
> more code.
I could profile it to see if avoiding the allocation is worth while.
I have a use case that prints approximately 1MB (stupid GPU, be less complex)
so that is enough to be able to see noticeable deltas if they exist.
Jordan
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
