From: Dave Gordon <david.s.gordon@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
Tom O'Rourke <Tom.O'Rourke@intel.com>
Subject: Re: [PATCH 2/2] drm/i915: Soften error messages in i915_gem_object_create_from_data()
Date: Tue, 28 Jul 2015 18:09:16 +0100 [thread overview]
Message-ID: <55B7B73C.4080907@intel.com> (raw)
In-Reply-To: <20150728163434.GG16528@nuc-i3427.alporthouse.com>
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
next prev parent reply other threads:[~2015-07-28 17:09 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2015-07-28 17:18 ` Chris Wilson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55B7B73C.4080907@intel.com \
--to=david.s.gordon@intel.com \
--cc=Tom.O'Rourke@intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.