* [PATCH 1/2] drm: Add DRM_DEBUG_GEM @ 2015-07-28 13:27 Chris Wilson 2015-07-28 13:27 ` [PATCH 2/2] drm/i915: Soften error messages in i915_gem_object_create_from_data() Chris Wilson 0 siblings, 1 reply; 6+ messages in thread From: Chris Wilson @ 2015-07-28 13:27 UTC (permalink / raw) To: intel-gfx, dri-devel Add a new debug value to distinguish and filter upon debug messages emanating from GEM support code. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- include/drm/drmP.h | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/include/drm/drmP.h b/include/drm/drmP.h index f2d68d185274..e7b58e37c583 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -107,6 +107,11 @@ struct dma_buf_attachment; * ATOMIC: used in the atomic code. * This is the category used by the DRM_DEBUG_ATOMIC() macro. * + * GEM: used by portions of code backing the GEM interface, both in the + * core (e.g. drm_gem.c) and vendor specific code. + * This is the category used by the DRM_DEBUG_GEM() macro. + * + * * Enabling verbose debug messages is done through the drm.debug parameter, * each category being enabled by a bit. * @@ -114,17 +119,18 @@ struct dma_buf_attachment; * drm.debug=0x2 will enable DRIVER messages * drm.debug=0x3 will enable CORE and DRIVER messages * ... - * drm.debug=0xf will enable all messages + * drm.debug=0xff will enable all messages * * An interesting feature is that it's possible to enable verbose logging at * run-time by echoing the debug value in its sysfs node: - * # echo 0xf > /sys/module/drm/parameters/debug + * # echo 0xff > /sys/module/drm/parameters/debug */ #define DRM_UT_CORE 0x01 #define DRM_UT_DRIVER 0x02 #define DRM_UT_KMS 0x04 #define DRM_UT_PRIME 0x08 #define DRM_UT_ATOMIC 0x10 +#define DRM_UT_GEM 0x20 extern __printf(2, 3) void drm_ut_debug_printk(const char *function_name, @@ -254,6 +260,11 @@ void drm_err(const char *format, ...); if (unlikely(drm_debug & DRM_UT_ATOMIC)) \ drm_ut_debug_printk(__func__, fmt, ##args); \ } while (0) +#define DRM_DEBUG_GEM(fmt, args...) \ + do { \ + if (unlikely(drm_debug & DRM_UT_GEM)) \ + drm_ut_debug_printk(__func__, fmt, ##args); \ + } while (0) /*@}*/ -- 2.4.6 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/i915: Soften error messages in i915_gem_object_create_from_data() 2015-07-28 13:27 [PATCH 1/2] drm: Add DRM_DEBUG_GEM Chris Wilson @ 2015-07-28 13:27 ` Chris Wilson 2015-07-28 16:29 ` Dave Gordon 0 siblings, 1 reply; 6+ messages in thread From: Chris Wilson @ 2015-07-28 13:27 UTC (permalink / raw) To: intel-gfx, dri-devel; +Cc: Dave Gordon, Tom O'Rourke Since we already return -EFAULT to the user, emitting an error message *and* WARN is overkill. If the caller is upset, they can do so, but for the purposes of debugging we need only log the erroneous values. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc:: Alex Dai <yu.dai@intel.com> Cc: Dave Gordon <david.s.gordon@intel.com> Cc: Tom O'Rourke <Tom.O'Rourke@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c1ded76a6eb4..2039798f4403 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -5243,8 +5243,8 @@ i915_gem_object_create_from_data(struct drm_device *dev, i915_gem_object_unpin_pages(obj); - if (WARN_ON(bytes != size)) { - DRM_ERROR("Incomplete copy, wrote %zu of %zu", bytes, size); + if (bytes != size) { + DRM_DEBUG_GEM("Incomplete copy, wrote %zu of %zu", bytes, size); ret = -EFAULT; goto fail; } -- 2.4.6 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Soften error messages in i915_gem_object_create_from_data() 2015-07-28 13:27 ` [PATCH 2/2] drm/i915: Soften error messages in i915_gem_object_create_from_data() Chris Wilson @ 2015-07-28 16:29 ` Dave Gordon 2015-07-28 16:34 ` Chris Wilson 0 siblings, 1 reply; 6+ messages in thread From: Dave Gordon @ 2015-07-28 16:29 UTC (permalink / raw) To: Chris Wilson, intel-gfx, dri-devel; +Cc: Tom O'Rourke On 28/07/15 14:27, Chris Wilson wrote: > Since we already return -EFAULT to the user, emitting an error message > *and* WARN is overkill. If the caller is upset, they can do so, but for > the purposes of debugging we need only log the erroneous values. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc:: Alex Dai <yu.dai@intel.com> > Cc: Dave Gordon <david.s.gordon@intel.com> > Cc: Tom O'Rourke <Tom.O'Rourke@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index c1ded76a6eb4..2039798f4403 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -5243,8 +5243,8 @@ i915_gem_object_create_from_data(struct drm_device *dev, > > i915_gem_object_unpin_pages(obj); > > - if (WARN_ON(bytes != size)) { > - DRM_ERROR("Incomplete copy, wrote %zu of %zu", bytes, size); > + if (bytes != size) { > + DRM_DEBUG_GEM("Incomplete copy, wrote %zu of %zu", bytes, size); > ret = -EFAULT; > goto fail; > } I agree the WARN_ON() is overkill, but I think maybe the DRM_ERROR() is useful. The (one current) caller will report an error, but at that level it's just: DRM_ERROR("Failed to fetch GuC firmware from %s\n", ...) with no more detail as to whether that was due to file-not-found, bad-file-size, out-of-memory, failed-to-get-pages, or any of the other errors that might arise. At present this code is only called once, and I think this copy failure "shouldn't ever happen", so it won't be filling the logfile. But emitting the error here for the truly unexpected case (as opposed to the commonplace "out-of-memory" and suchlike) helps current and future callers avoid doing the detailed failure analysis. Or maybe it's a good place for your (other) new macro? DRM_NOTICE_ON(bytes != size, "Incomplete copy, wrote %zu of %zu", bytes, size); .Dave. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Soften error messages in i915_gem_object_create_from_data() 2015-07-28 16:29 ` Dave Gordon @ 2015-07-28 16:34 ` Chris Wilson 2015-07-28 17:09 ` Dave Gordon 0 siblings, 1 reply; 6+ messages in thread From: Chris Wilson @ 2015-07-28 16:34 UTC (permalink / raw) To: Dave Gordon; +Cc: intel-gfx, dri-devel On Tue, Jul 28, 2015 at 05:29:09PM +0100, Dave Gordon wrote: > On 28/07/15 14:27, Chris Wilson wrote: > >Since we already return -EFAULT to the user, emitting an error message > >*and* WARN is overkill. If the caller is upset, they can do so, but for > >the purposes of debugging we need only log the erroneous values. > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >Cc:: Alex Dai <yu.dai@intel.com> > >Cc: Dave Gordon <david.s.gordon@intel.com> > >Cc: Tom O'Rourke <Tom.O'Rourke@intel.com> > >--- > > drivers/gpu/drm/i915/i915_gem.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >index c1ded76a6eb4..2039798f4403 100644 > >--- a/drivers/gpu/drm/i915/i915_gem.c > >+++ b/drivers/gpu/drm/i915/i915_gem.c > >@@ -5243,8 +5243,8 @@ i915_gem_object_create_from_data(struct drm_device *dev, > > > > i915_gem_object_unpin_pages(obj); > > > >- if (WARN_ON(bytes != size)) { > >- DRM_ERROR("Incomplete copy, wrote %zu of %zu", bytes, size); > >+ if (bytes != size) { > >+ DRM_DEBUG_GEM("Incomplete copy, wrote %zu of %zu", bytes, size); > > ret = -EFAULT; > > goto fail; > > } > > I agree the WARN_ON() is overkill, but I think maybe the DRM_ERROR() > is useful. The (one current) caller will report an error, but at > that level it's just: > > DRM_ERROR("Failed to fetch GuC firmware from %s\n", ...) > > with no more detail as to whether that was due to file-not-found, > bad-file-size, out-of-memory, failed-to-get-pages, or any of the > other errors that might arise. > > At present this code is only called once, and I think this copy > failure "shouldn't ever happen", so it won't be filling the logfile. > But emitting the error here for the truly unexpected case (as > opposed to the commonplace "out-of-memory" and suchlike) helps > current and future callers avoid doing the detailed failure > analysis. The message is still there though, it's just a debug message for the developer. And all it could mean is that the caller passed in a bad value for the size. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Soften error messages in i915_gem_object_create_from_data() 2015-07-28 16:34 ` Chris Wilson @ 2015-07-28 17:09 ` Dave Gordon 2015-07-28 17:18 ` Chris Wilson 0 siblings, 1 reply; 6+ messages in thread From: Dave Gordon @ 2015-07-28 17:09 UTC (permalink / raw) To: Chris Wilson, intel-gfx, dri-devel, Tom O'Rourke On 28/07/15 17:34, Chris Wilson wrote: > On Tue, Jul 28, 2015 at 05:29:09PM +0100, Dave Gordon wrote: >> On 28/07/15 14:27, Chris Wilson wrote: >>> Since we already return -EFAULT to the user, emitting an error message >>> *and* WARN is overkill. If the caller is upset, they can do so, but for >>> the purposes of debugging we need only log the erroneous values. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc:: Alex Dai <yu.dai@intel.com> >>> Cc: Dave Gordon <david.s.gordon@intel.com> >>> Cc: Tom O'Rourke <Tom.O'Rourke@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_gem.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >>> index c1ded76a6eb4..2039798f4403 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem.c >>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>> @@ -5243,8 +5243,8 @@ i915_gem_object_create_from_data(struct drm_device *dev, >>> >>> i915_gem_object_unpin_pages(obj); >>> >>> - if (WARN_ON(bytes != size)) { >>> - DRM_ERROR("Incomplete copy, wrote %zu of %zu", bytes, size); >>> + if (bytes != size) { >>> + DRM_DEBUG_GEM("Incomplete copy, wrote %zu of %zu", bytes, size); >>> ret = -EFAULT; >>> goto fail; >>> } >> >> I agree the WARN_ON() is overkill, but I think maybe the DRM_ERROR() >> is useful. The (one current) caller will report an error, but at >> that level it's just: >> >> DRM_ERROR("Failed to fetch GuC firmware from %s\n", ...) >> >> with no more detail as to whether that was due to file-not-found, >> bad-file-size, out-of-memory, failed-to-get-pages, or any of the >> other errors that might arise. >> >> At present this code is only called once, and I think this copy >> failure "shouldn't ever happen", so it won't be filling the logfile. >> But emitting the error here for the truly unexpected case (as >> opposed to the commonplace "out-of-memory" and suchlike) helps >> current and future callers avoid doing the detailed failure >> analysis. > > The message is still there though, it's just a debug message for the > developer. And all it could mean is that the caller passed in a bad > value for the size. > -Chris I'm not sure you could trigger it with a bad size. The copy-target has just been allocated (in this function) and so is known to be the right size. If the size specified is larger than the data source buffer, the copy will likely have OOPSed anyway - or just continued with whatever happens to be next in memory. So AFAICT the bytes vs. size mismatch can only happen if something is broken in the GEM (or SG-list) framework, hence worth announcing. If you reload the driver with debug enabled, it probably won't happen again (and you would have to reload, not just change the debug level, because at present this is only called during driver load). If not an actual ERROR, then maybe a NOTICE is the right level: DRM_NOTICE_IF(bytes != size, "Incomplete copy, wrote %zu of %zu", bytes, size); so the the support engineer who sees the "failed to fetch" message in the log at least gets some hint about what particular weirdness has occurred on this occasion. (Other cases are easier: missing firmware file is obvious, wrong version gets a specific ERROR in the log; and these errors would be persistent. But a copy-fail would be a puzzle.) .Dave. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Soften error messages in i915_gem_object_create_from_data() 2015-07-28 17:09 ` Dave Gordon @ 2015-07-28 17:18 ` Chris Wilson 0 siblings, 0 replies; 6+ messages in thread From: Chris Wilson @ 2015-07-28 17:18 UTC (permalink / raw) To: Dave Gordon; +Cc: intel-gfx, dri-devel, Tom O'Rourke On Tue, Jul 28, 2015 at 06:09:16PM +0100, Dave Gordon wrote: > On 28/07/15 17:34, Chris Wilson wrote: > >On Tue, Jul 28, 2015 at 05:29:09PM +0100, Dave Gordon wrote: > >>On 28/07/15 14:27, Chris Wilson wrote: > >>>Since we already return -EFAULT to the user, emitting an error message > >>>*and* WARN is overkill. If the caller is upset, they can do so, but for > >>>the purposes of debugging we need only log the erroneous values. > >>> > >>>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >>>Cc:: Alex Dai <yu.dai@intel.com> > >>>Cc: Dave Gordon <david.s.gordon@intel.com> > >>>Cc: Tom O'Rourke <Tom.O'Rourke@intel.com> > >>>--- > >>> drivers/gpu/drm/i915/i915_gem.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >>>index c1ded76a6eb4..2039798f4403 100644 > >>>--- a/drivers/gpu/drm/i915/i915_gem.c > >>>+++ b/drivers/gpu/drm/i915/i915_gem.c > >>>@@ -5243,8 +5243,8 @@ i915_gem_object_create_from_data(struct drm_device *dev, > >>> > >>> i915_gem_object_unpin_pages(obj); > >>> > >>>- if (WARN_ON(bytes != size)) { > >>>- DRM_ERROR("Incomplete copy, wrote %zu of %zu", bytes, size); > >>>+ if (bytes != size) { > >>>+ DRM_DEBUG_GEM("Incomplete copy, wrote %zu of %zu", bytes, size); > >>> ret = -EFAULT; > >>> goto fail; > >>> } > >> > >>I agree the WARN_ON() is overkill, but I think maybe the DRM_ERROR() > >>is useful. The (one current) caller will report an error, but at > >>that level it's just: > >> > >> DRM_ERROR("Failed to fetch GuC firmware from %s\n", ...) > >> > >>with no more detail as to whether that was due to file-not-found, > >>bad-file-size, out-of-memory, failed-to-get-pages, or any of the > >>other errors that might arise. > >> > >>At present this code is only called once, and I think this copy > >>failure "shouldn't ever happen", so it won't be filling the logfile. > >>But emitting the error here for the truly unexpected case (as > >>opposed to the commonplace "out-of-memory" and suchlike) helps > >>current and future callers avoid doing the detailed failure > >>analysis. > > > >The message is still there though, it's just a debug message for the > >developer. And all it could mean is that the caller passed in a bad > >value for the size. > >-Chris > > I'm not sure you could trigger it with a bad size. The copy-target > has just been allocated (in this function) and so is known to be the > right size. If the size specified is larger than the data source > buffer, the copy will likely have OOPSed anyway - or just continued > with whatever happens to be next in memory. > > So AFAICT the bytes vs. size mismatch can only happen if something > is broken in the GEM (or SG-list) framework, hence worth announcing. > If you reload the driver with debug enabled, it probably won't > happen again (and you would have to reload, not just change the > debug level, because at present this is only called during driver > load). Hmm, I was thinking that the access of data[size] would be checked and the likely cause of the failure (thinking this was basically just another copy_from_user check). I disagree that this one failure is any more significant that any of the other internal failure cases then. > If not an actual ERROR, then maybe a NOTICE is the right level: No. > DRM_NOTICE_IF(bytes != size, > "Incomplete copy, wrote %zu of %zu", bytes, size); > > so the the support engineer who sees the "failed to fetch" message > in the log at least gets some hint about what particular weirdness > has occurred on this occasion. (Other cases are easier: missing > firmware file is obvious, wrong version gets a specific ERROR in the > log; and these errors would be persistent. But a copy-fail would be > a puzzle.) That equally applies to a debug message. And even more so since EFAULT here is unique. Under my assumption bytes would be recognisable in the context of the caller, however if it ever happens it will be an equally opaque value. Given your arguments here, I think we should just remove it. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-07-28 17:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-07-28 13:27 [PATCH 1/2] drm: Add DRM_DEBUG_GEM Chris Wilson 2015-07-28 13:27 ` [PATCH 2/2] drm/i915: Soften error messages in i915_gem_object_create_from_data() Chris Wilson 2015-07-28 16:29 ` Dave Gordon 2015-07-28 16:34 ` Chris Wilson 2015-07-28 17:09 ` Dave Gordon 2015-07-28 17:18 ` Chris Wilson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.