Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
Quoting Anusha Srivatsa (2017-11-02 20:28:10)
> On Wed, Nov 01, 2017 at 01:24:15PM +, Chris Wilson wrote:
> > Quoting Michal Wajdeczko (2017-11-01 13:14:33)
> > > On Wed, 01 Nov 2017 01:11:20 +0100, Anusha Srivatsa
> > > > @@ -172,13 +174,18 @@ static int guc_ucode_xfer_dma(struct
> > > > drm_i915_private *dev_priv,
> > > >*/
> > > > ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
> > > > + load_time = ktime_ms_delta(ktime_get(), start_load);
> > > > +
> > > > DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
> > > > I915_READ(DMA_CTRL), status);
> > > > if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
> > > > DRM_ERROR("GuC firmware signature verification failed\n");
> > > > ret = -ENOEXEC;
> > > > - }
> > > > + } else if (load_time > 20)
> > > > + DRM_NOTE("GuC load takes more than acceptable
> > > > threshold\n");
> > >
> > > Please add threshold and actual load time to the message to let user
> > > know that times
> >
> > The more important question is why are we telling the user this at all;
> > a significant but normal condition. What do we expect them to do? It
> > doesn't impair any functionality of the driver, it just took longer than
> > you expected -- which may be simply because the system was doing
> > something else and we slept for longer.
>
> Chris, I am inclining to have this info more for us than the user. It is more
> of
> a debug print to give us some data. We can see if firmware takes peculiarly
> long time to load. We know its normal to be within 20ms range. So, alert
> if it takes longer than that...
Sure, but the impact is that this is a user facing message, even marked
as a significant message. We are quite capable of parsing debug
messages; even capable of setting up ftrace to extract this timing info
without adding the dmesg in the first place...
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
On Wed, Nov 01, 2017 at 01:24:15PM +, Chris Wilson wrote:
> Quoting Michal Wajdeczko (2017-11-01 13:14:33)
> > On Wed, 01 Nov 2017 01:11:20 +0100, Anusha Srivatsa
> > > @@ -172,13 +174,18 @@ static int guc_ucode_xfer_dma(struct
> > > drm_i915_private *dev_priv,
> > >*/
> > > ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
> > > + load_time = ktime_ms_delta(ktime_get(), start_load);
> > > +
> > > DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
> > > I915_READ(DMA_CTRL), status);
> > > if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
> > > DRM_ERROR("GuC firmware signature verification failed\n");
> > > ret = -ENOEXEC;
> > > - }
> > > + } else if (load_time > 20)
> > > + DRM_NOTE("GuC load takes more than acceptable threshold\n");
> >
> > Please add threshold and actual load time to the message to let user
> > know that times
>
> The more important question is why are we telling the user this at all;
> a significant but normal condition. What do we expect them to do? It
> doesn't impair any functionality of the driver, it just took longer than
> you expected -- which may be simply because the system was doing
> something else and we slept for longer.
Chris, I am inclining to have this info more for us than the user. It is more of
a debug print to give us some data. We can see if firmware takes peculiarly
long time to load. We know its normal to be within 20ms range. So, alert
if it takes longer than that...
> -Chris
--
Anusha Srivatsa
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
On Wed, Nov 01, 2017 at 02:14:33PM +0100, Michal Wajdeczko wrote:
> On Wed, 01 Nov 2017 01:11:20 +0100, Anusha Srivatsa
> wrote:
>
> >Calculate the time that GuC takes to load using
> >jiffies. This information could be very useful in
> ^^^
> This is no longer true.
True. Will sending an all new patch with updated
approach(using ktime instead of jiffies) be good?
Or stick to this with change in commit message?
> >determining if GuC is taking unreasonably long time
> >to load in a certain platforms.
> >
> >v2: Calculate time before logs are collected.
> >Move the guc_load_time variable as a part of
> >intel_uc_fw struct. Store only final result
> >which is to be exported to debugfs. (Michal)
> >Add the load time in the print message as well.
> >
> >v3: Remove debugfs entry. Remove local variable
> >guc_finish_load. (Daniel, Tvrtko)
> >
> >v4: Use ktime_get() instead of jiffies. Use DRM_NOTE
> >if time taken to load is more than the threshold. On
> >load times within acceptable range, use DRM_DEBUG_DRIVER
> >(Tvrtko)
> >
> >v5: Rebased. Do not expose the load time variable in a global
> >struct (Tvrtko, Joonas)
> >
> >Cc: Chris Wilson
> >Cc: Daniel Vetter
> >Cc: Michal Wajdeczko
> >Cc: Oscar Mateo
> >Cc: Sujaritha Sundaresan
> >Cc: Tvrtko Ursulin
> >Signed-off-by: Anusha Srivatsa
> >---
> > drivers/gpu/drm/i915/intel_guc_fw.c | 11 +--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c
> >b/drivers/gpu/drm/i915/intel_guc_fw.c
> >index ef67a36..4ce9a30 100644
> >--- a/drivers/gpu/drm/i915/intel_guc_fw.c
> >+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
> >@@ -133,7 +133,8 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
> >*dev_priv,
> > unsigned long offset;
> > struct sg_table *sg = vma->pages;
> > u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
> >-int i, ret = 0;
> >+int i, ret = 0, load_time;
>
> Note that ktime_ms_delta() return type is s64 not int.
>
> >+ktime_t start_load;
>
> s/start_load/now ?
I think start_load is verbose.
> > /* where RSA signature starts */
> > offset = guc_fw->rsa_offset;
> >@@ -160,6 +161,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
> >*dev_priv,
> > I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
> > /* Finally start the DMA */
> >+start_load = ktime_get();
>
> Maybe we should either update comment with note about taking start time
> or move ktime_get call before that comment to avoid confusion..
I prefer the latter.
> > I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
> > /*
> >@@ -172,13 +174,18 @@ static int guc_ucode_xfer_dma(struct
> >drm_i915_private *dev_priv,
> > */
> > ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
> >+load_time = ktime_ms_delta(ktime_get(), start_load);
> >+
> > DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
> > I915_READ(DMA_CTRL), status);
> > if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
> > DRM_ERROR("GuC firmware signature verification failed\n");
> > ret = -ENOEXEC;
> >-}
> >+} else if (load_time > 20)
> >+DRM_NOTE("GuC load takes more than acceptable threshold\n");
>
> Please add threshold and actual load time to the message to let user
> know that times
> >+else
> >+DRM_DEBUG_DRIVER("GuC loaded in %d ms\n", load_time);
>
> And I'm not sure that we can rely on 'load_time' on timeout in wait_for.
Hmm we are checking the DMA status right after that which means
the firmware load should have happened by then thats why I
am calculating it there
> > DRM_DEBUG_DRIVER("returning %d\n", ret);
--
Anusha Srivatsa
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
Quoting David Weinehall (2017-11-01 13:38:48) > On Tue, Oct 31, 2017 at 05:11:20PM -0700, Anusha Srivatsa wrote: > > Calculate the time that GuC takes to load using > > jiffies. This information could be very useful in > > determining if GuC is taking unreasonably long time > > to load in a certain platforms. > > > > v2: Calculate time before logs are collected. > > Move the guc_load_time variable as a part of > > intel_uc_fw struct. Store only final result > > which is to be exported to debugfs. (Michal) > > Add the load time in the print message as well. > > > > v3: Remove debugfs entry. Remove local variable > > guc_finish_load. (Daniel, Tvrtko) > > > > v4: Use ktime_get() instead of jiffies. Use DRM_NOTE > > if time taken to load is more than the threshold. On > > load times within acceptable range, use DRM_DEBUG_DRIVER > > (Tvrtko) > > > > v5: Rebased. Do not expose the load time variable in a global > > struct (Tvrtko, Joonas) > > From my point of view (measuring suspend/resume times) knowing > how much time is spent loading GuC & HuC is really useful, > so it's definitely useful information. This information could be gleaned from ftrace... -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
On Tue, Oct 31, 2017 at 05:11:20PM -0700, Anusha Srivatsa wrote: > Calculate the time that GuC takes to load using > jiffies. This information could be very useful in > determining if GuC is taking unreasonably long time > to load in a certain platforms. > > v2: Calculate time before logs are collected. > Move the guc_load_time variable as a part of > intel_uc_fw struct. Store only final result > which is to be exported to debugfs. (Michal) > Add the load time in the print message as well. > > v3: Remove debugfs entry. Remove local variable > guc_finish_load. (Daniel, Tvrtko) > > v4: Use ktime_get() instead of jiffies. Use DRM_NOTE > if time taken to load is more than the threshold. On > load times within acceptable range, use DRM_DEBUG_DRIVER > (Tvrtko) > > v5: Rebased. Do not expose the load time variable in a global > struct (Tvrtko, Joonas) From my point of view (measuring suspend/resume times) knowing how much time is spent loading GuC & HuC is really useful, so it's definitely useful information. Kind regards, David Weinehall ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
Quoting Michal Wajdeczko (2017-11-01 13:14:33)
> On Wed, 01 Nov 2017 01:11:20 +0100, Anusha Srivatsa
> > @@ -172,13 +174,18 @@ static int guc_ucode_xfer_dma(struct
> > drm_i915_private *dev_priv,
> >*/
> > ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
> > + load_time = ktime_ms_delta(ktime_get(), start_load);
> > +
> > DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
> > I915_READ(DMA_CTRL), status);
> > if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
> > DRM_ERROR("GuC firmware signature verification failed\n");
> > ret = -ENOEXEC;
> > - }
> > + } else if (load_time > 20)
> > + DRM_NOTE("GuC load takes more than acceptable threshold\n");
>
> Please add threshold and actual load time to the message to let user
> know that times
The more important question is why are we telling the user this at all;
a significant but normal condition. What do we expect them to do? It
doesn't impair any functionality of the driver, it just took longer than
you expected -- which may be simply because the system was doing
something else and we slept for longer.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
On Wed, 01 Nov 2017 01:11:20 +0100, Anusha Srivatsa
wrote:
Calculate the time that GuC takes to load using
jiffies. This information could be very useful in
^^^
This is no longer true.
determining if GuC is taking unreasonably long time
to load in a certain platforms.
v2: Calculate time before logs are collected.
Move the guc_load_time variable as a part of
intel_uc_fw struct. Store only final result
which is to be exported to debugfs. (Michal)
Add the load time in the print message as well.
v3: Remove debugfs entry. Remove local variable
guc_finish_load. (Daniel, Tvrtko)
v4: Use ktime_get() instead of jiffies. Use DRM_NOTE
if time taken to load is more than the threshold. On
load times within acceptable range, use DRM_DEBUG_DRIVER
(Tvrtko)
v5: Rebased. Do not expose the load time variable in a global
struct (Tvrtko, Joonas)
Cc: Chris Wilson
Cc: Daniel Vetter
Cc: Michal Wajdeczko
Cc: Oscar Mateo
Cc: Sujaritha Sundaresan
Cc: Tvrtko Ursulin
Signed-off-by: Anusha Srivatsa
---
drivers/gpu/drm/i915/intel_guc_fw.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c
b/drivers/gpu/drm/i915/intel_guc_fw.c
index ef67a36..4ce9a30 100644
--- a/drivers/gpu/drm/i915/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/intel_guc_fw.c
@@ -133,7 +133,8 @@ static int guc_ucode_xfer_dma(struct
drm_i915_private *dev_priv,
unsigned long offset;
struct sg_table *sg = vma->pages;
u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
- int i, ret = 0;
+ int i, ret = 0, load_time;
Note that ktime_ms_delta() return type is s64 not int.
+ ktime_t start_load;
s/start_load/now ?
/* where RSA signature starts */
offset = guc_fw->rsa_offset;
@@ -160,6 +161,7 @@ static int guc_ucode_xfer_dma(struct
drm_i915_private *dev_priv,
I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
/* Finally start the DMA */
+ start_load = ktime_get();
Maybe we should either update comment with note about taking start time
or move ktime_get call before that comment to avoid confusion..
I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
/*
@@ -172,13 +174,18 @@ static int guc_ucode_xfer_dma(struct
drm_i915_private *dev_priv,
*/
ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
+ load_time = ktime_ms_delta(ktime_get(), start_load);
+
DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
I915_READ(DMA_CTRL), status);
if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
DRM_ERROR("GuC firmware signature verification failed\n");
ret = -ENOEXEC;
- }
+ } else if (load_time > 20)
+ DRM_NOTE("GuC load takes more than acceptable threshold\n");
Please add threshold and actual load time to the message to let user
know that times
+ else
+ DRM_DEBUG_DRIVER("GuC loaded in %d ms\n", load_time);
And I'm not sure that we can rely on 'load_time' on timeout in wait_for.
DRM_DEBUG_DRIVER("returning %d\n", ret);
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
>-Original Message-
>From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com]
>Sent: Thursday, October 5, 2017 11:30 PM
>To: Srivatsa, Anusha ; Tvrtko Ursulin
>; intel-gfx@lists.freedesktop.org
>Cc: Daniel Vetter ; Sundaresan, Sujaritha
>
>Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg
>log.
>
>On Fri, 2017-10-06 at 00:56 +, Srivatsa, Anusha wrote:
>> > -Original Message-
>> > From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com]
>> > Sent: Wednesday, October 4, 2017 5:52 AM
>> > To: Tvrtko Ursulin ; Srivatsa,
>> > Anusha ; intel-gfx@lists.freedesktop.org
>> > Cc: Daniel Vetter ; Sundaresan, Sujaritha
>> >
>> > Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time
>> > to dmesg log.
>> >
>> > On Wed, 2017-10-04 at 09:16 +0100, Tvrtko Ursulin wrote:
>> > > On 04/10/2017 01:41, Anusha Srivatsa wrote:
>> > > > Calculate the time that GuC takes to load using jiffies. This
>> > > > information could be very useful in determining if GuC is taking
>> > > > unreasonably long time to load in a certain platforms.
>> > > >
>> > > > v2: Calculate time before logs are collected.
>> > > > Move the guc_load_time variable as a part of intel_uc_fw struct.
>> > > > Store only final result which is to be exported to debugfs.
>> > > > (Michal) Add the load time in the print message as well.
>> > > >
>> > > > v3: Remove debugfs entry. Remove local variable guc_finish_load.
>> > > > (Daniel, Tvrtko)
>> > > >
>> > > > v4: Use ktime_get() instead of jiffies. Use DRM_NOTE if time
>> > > > taken to load is more than the threshold. On load times within
>> > > > acceptable range, use DRM_DEBUG_DRIVER
>> > > > (Tvrtko)
>> > > >
>> > > > Cc: Chris Wilson
>> > > > Cc: Tvrtko ursulin
>> > > > Cc: Daniel Vetter
>> > > > Cc: Sujaritha Sundaresan
>> > > > Cc: Oscar Mateo
>> > > > Cc: Michal Wajdeczko
>> > > > Signed-off-by: Anusha Srivatsa
>> >
>> >
>> >
>> > > > @@ -373,6 +379,8 @@ int intel_guc_init_hw(struct intel_guc *guc)
>> > > > guc->fw.path,
>> > > > guc->fw.major_ver_found, guc->fw.minor_ver_found);
>> > > >
>> > > > + DRM_DEBUG_DRIVER("GuC is loaded in: %lld
>> > > > +ms\n",guc->fw.load_time);
>> > > > +
>> > >
>> > > If you move this debug to where the DRM_NOTE is you don't have to
>> > > store the load time in the global structure. Unless there will be
>> > > a reason in the future to have the value stored?
>> >
>> > We decided not to expose it through , so let's try to avoid a variable,
>> > too.
>>
>> Hi Joonas,
>>
>> The order of prints will then be:
>> GuC firmware status: fetch SUCCESS load PENDING GuC loaded in x ms GuC
>> loaded/submission enabled firmware _major.minor.bin
>> version major.minor
>
>But is not this the more logical one, too? Say we would enable the asynchronous
>loading of GuC firmware, and we could simultaneously fallback to ELSP and start
>using the system. So there could be considerable time between GuC being loaded
>and GuC submission being enabled. So I might tie the firmware path information
>to the GuC loaded message.
>
>Also, if we have printk timestamping available, would not the information be
>readable from there, just calculate delta between firmware status changing from
>PENDING to LOADED? It won't hurt to explicitly compute it for the user, of
>course.
Agreed. Will remove the variable altogether.
Thanks a lot Joonas.
Anusha
>Regards, Joonas
>
>>
>> Compared to
>>
>> GuC firmware status: fetch SUCCESS load PENDING GuC loaded/submission
>> enabled firmware _platform_major.minor.bin version
>> major.minor GuC loaded in x ms
>>
>> I felt the second one looked a better sequence, but I will change if that’s
>> the
>majority.
>>
>> Thanks,
>> Anusha
>> > Regards, Joonas
>> >
>> > PS. Also, lets try to get to the habit of having the S-o-b: and Cc:s
>> > in chronological order. Less fixing for committers when applying patches.
>>
>> Will keep in mind, thanks
>>
>> Anusha
>> > Joonas Lahtinen
>> > Open Source Technology Center
>> > Intel Corporation
>--
>Joonas Lahtinen
>Open Source Technology Center
>Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
On Fri, 2017-10-06 at 00:56 +, Srivatsa, Anusha wrote:
> > -Original Message-
> > From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com]
> > Sent: Wednesday, October 4, 2017 5:52 AM
> > To: Tvrtko Ursulin ; Srivatsa, Anusha
> > ; intel-gfx@lists.freedesktop.org
> > Cc: Daniel Vetter ; Sundaresan, Sujaritha
> >
> > Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to
> > dmesg
> > log.
> >
> > On Wed, 2017-10-04 at 09:16 +0100, Tvrtko Ursulin wrote:
> > > On 04/10/2017 01:41, Anusha Srivatsa wrote:
> > > > Calculate the time that GuC takes to load using jiffies. This
> > > > information could be very useful in determining if GuC is taking
> > > > unreasonably long time to load in a certain platforms.
> > > >
> > > > v2: Calculate time before logs are collected.
> > > > Move the guc_load_time variable as a part of intel_uc_fw struct.
> > > > Store only final result which is to be exported to debugfs. (Michal)
> > > > Add the load time in the print message as well.
> > > >
> > > > v3: Remove debugfs entry. Remove local variable guc_finish_load.
> > > > (Daniel, Tvrtko)
> > > >
> > > > v4: Use ktime_get() instead of jiffies. Use DRM_NOTE if time taken
> > > > to load is more than the threshold. On load times within acceptable
> > > > range, use DRM_DEBUG_DRIVER
> > > > (Tvrtko)
> > > >
> > > > Cc: Chris Wilson
> > > > Cc: Tvrtko ursulin
> > > > Cc: Daniel Vetter
> > > > Cc: Sujaritha Sundaresan
> > > > Cc: Oscar Mateo
> > > > Cc: Michal Wajdeczko
> > > > Signed-off-by: Anusha Srivatsa
> >
> >
> >
> > > > @@ -373,6 +379,8 @@ int intel_guc_init_hw(struct intel_guc *guc)
> > > > guc->fw.path,
> > > > guc->fw.major_ver_found, guc->fw.minor_ver_found);
> > > >
> > > > + DRM_DEBUG_DRIVER("GuC is loaded in: %lld
> > > > ms\n",guc->fw.load_time);
> > > > +
> > >
> > > If you move this debug to where the DRM_NOTE is you don't have to
> > > store the load time in the global structure. Unless there will be a
> > > reason in the future to have the value stored?
> >
> > We decided not to expose it through , so let's try to avoid a variable, too.
>
> Hi Joonas,
>
> The order of prints will then be:
> GuC firmware status: fetch SUCCESS load PENDING
> GuC loaded in x ms
> GuC loaded/submission enabled firmware _major.minor.bin
> version major.minor
But is not this the more logical one, too? Say we would enable the
asynchronous loading of GuC firmware, and we could simultaneously
fallback to ELSP and start using the system. So there could be
considerable time between GuC being loaded and GuC submission being
enabled. So I might tie the firmware path information to the GuC loaded
message.
Also, if we have printk timestamping available, would not the
information be readable from there, just calculate delta between
firmware status changing from PENDING to LOADED? It won't hurt to
explicitly compute it for the user, of course.
Regards, Joonas
>
> Compared to
>
> GuC firmware status: fetch SUCCESS load PENDING
> GuC loaded/submission enabled firmware
> _platform_major.minor.bin version major.minor
> GuC loaded in x ms
>
> I felt the second one looked a better sequence, but I will change if that’s
> the majority.
>
> Thanks,
> Anusha
> > Regards, Joonas
> >
> > PS. Also, lets try to get to the habit of having the S-o-b: and Cc:s in
> > chronological
> > order. Less fixing for committers when applying patches.
>
> Will keep in mind, thanks
>
> Anusha
> > Joonas Lahtinen
> > Open Source Technology Center
> > Intel Corporation
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
>-Original Message-
>From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com]
>Sent: Wednesday, October 4, 2017 5:52 AM
>To: Tvrtko Ursulin ; Srivatsa, Anusha
>; intel-gfx@lists.freedesktop.org
>Cc: Daniel Vetter ; Sundaresan, Sujaritha
>
>Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg
>log.
>
>On Wed, 2017-10-04 at 09:16 +0100, Tvrtko Ursulin wrote:
>> On 04/10/2017 01:41, Anusha Srivatsa wrote:
>> > Calculate the time that GuC takes to load using jiffies. This
>> > information could be very useful in determining if GuC is taking
>> > unreasonably long time to load in a certain platforms.
>> >
>> > v2: Calculate time before logs are collected.
>> > Move the guc_load_time variable as a part of intel_uc_fw struct.
>> > Store only final result which is to be exported to debugfs. (Michal)
>> > Add the load time in the print message as well.
>> >
>> > v3: Remove debugfs entry. Remove local variable guc_finish_load.
>> > (Daniel, Tvrtko)
>> >
>> > v4: Use ktime_get() instead of jiffies. Use DRM_NOTE if time taken
>> > to load is more than the threshold. On load times within acceptable
>> > range, use DRM_DEBUG_DRIVER
>> > (Tvrtko)
>> >
>> > Cc: Chris Wilson
>> > Cc: Tvrtko ursulin
>> > Cc: Daniel Vetter
>> > Cc: Sujaritha Sundaresan
>> > Cc: Oscar Mateo
>> > Cc: Michal Wajdeczko
>> > Signed-off-by: Anusha Srivatsa
>
>
>
>> > @@ -373,6 +379,8 @@ int intel_guc_init_hw(struct intel_guc *guc)
>> > guc->fw.path,
>> > guc->fw.major_ver_found, guc->fw.minor_ver_found);
>> >
>> > + DRM_DEBUG_DRIVER("GuC is loaded in: %lld ms\n",guc->fw.load_time);
>> > +
>>
>> If you move this debug to where the DRM_NOTE is you don't have to
>> store the load time in the global structure. Unless there will be a
>> reason in the future to have the value stored?
>
>We decided not to expose it through , so let's try to avoid a variable, too.
Hi Joonas,
The order of prints will then be:
GuC firmware status: fetch SUCCESS load PENDING
GuC loaded in x ms
GuC loaded/submission enabled firmware _major.minor.bin
version major.minor
Compared to
GuC firmware status: fetch SUCCESS load PENDING
GuC loaded/submission enabled firmware
_platform_major.minor.bin version major.minor
GuC loaded in x ms
I felt the second one looked a better sequence, but I will change if that’s the
majority.
Thanks,
Anusha
>Regards, Joonas
>
>PS. Also, lets try to get to the habit of having the S-o-b: and Cc:s in
>chronological
>order. Less fixing for committers when applying patches.
Will keep in mind, thanks
Anusha
>Joonas Lahtinen
>Open Source Technology Center
>Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
>-Original Message-
>From: Tvrtko Ursulin [mailto:tvrtko.ursu...@linux.intel.com]
>Sent: Wednesday, October 4, 2017 1:17 AM
>To: Srivatsa, Anusha ; intel-
>g...@lists.freedesktop.org
>Cc: Sundaresan, Sujaritha ; Daniel Vetter
>
>Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg
>log.
>
>
>On 04/10/2017 01:41, Anusha Srivatsa wrote:
>> Calculate the time that GuC takes to load using jiffies. This
>> information could be very useful in determining if GuC is taking
>> unreasonably long time to load in a certain platforms.
>>
>> v2: Calculate time before logs are collected.
>> Move the guc_load_time variable as a part of intel_uc_fw struct. Store
>> only final result which is to be exported to debugfs. (Michal) Add the
>> load time in the print message as well.
>>
>> v3: Remove debugfs entry. Remove local variable guc_finish_load.
>> (Daniel, Tvrtko)
>>
>> v4: Use ktime_get() instead of jiffies. Use DRM_NOTE if time taken to
>> load is more than the threshold. On load times within acceptable
>> range, use DRM_DEBUG_DRIVER
>> (Tvrtko)
>>
>> Cc: Chris Wilson
>> Cc: Tvrtko ursulin
>> Cc: Daniel Vetter
>> Cc: Sujaritha Sundaresan
>> Cc: Oscar Mateo
>> Cc: Michal Wajdeczko
>> Signed-off-by: Anusha Srivatsa
>> ---
>> drivers/gpu/drm/i915/intel_guc_loader.c | 10 +-
>> drivers/gpu/drm/i915/intel_uc.h | 1 +
>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>> index c9e25be..a0b562c 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -199,6 +199,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
>*dev_priv,
>> struct sg_table *sg = vma->pages;
>> u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
>> int i, ret = 0;
>> +ktime_t start_load;
>>
>> /* where RSA signature starts */
>> offset = guc_fw->rsa_offset;
>> @@ -225,6 +226,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
>*dev_priv,
>> I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
>>
>> /* Finally start the DMA */
>> +start_load = ktime_get();
>> I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE |
>START_DMA));
>>
>> /*
>> @@ -237,13 +239,17 @@ static int guc_ucode_xfer_dma(struct
>drm_i915_private *dev_priv,
>> */
>> ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
>>
>> +guc_fw->load_time = ktime_ms_delta(ktime_get(), start_load);
>> +
>> DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
>> I915_READ(DMA_CTRL), status);
>>
>> if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
>> DRM_ERROR("GuC firmware signature verification failed\n");
>> ret = -ENOEXEC;
>> -}
>> +} else if (guc_fw->load_time > 20)
>> +DRM_NOTE("Time taken to load GuC is more than the
>acceptable \
>> + threshold\n");
>>
>> DRM_DEBUG_DRIVER("returning %d\n", ret);
>>
>> @@ -373,6 +379,8 @@ int intel_guc_init_hw(struct intel_guc *guc)
>> guc->fw.path,
>> guc->fw.major_ver_found, guc->fw.minor_ver_found);
>>
>> +DRM_DEBUG_DRIVER("GuC is loaded in: %lld ms\n",guc->fw.load_time);
>> +
>
>If you move this debug to where the DRM_NOTE is you don't have to store the
>load time in the global structure. Unless there will be a reason in the future
>to
>have the value stored?
We can have an IGT that checks the load time and fails if its greater than the
threshold for that it could be useful to have load time this way?
>Also, where ever it is (global or local) unsigned int is enough since you
>picked ms
>as the resolution.
Unsigned int is a better approach.
Will change to unsigned int Tvrtko, thanks for suggestion.
Anusha
>Regards,
>
>Tvrtko
>
>> return 0;
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h
>> b/drivers/gpu/drm/i915/intel_uc.h index 6966349..65b9674 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -136,6 +136,7 @@ struct intel_uc_fw {
>> uint32_t rsa_offset;
>> uint32_t ucode_size;
>> uint32_t ucode_offset;
>> +unsigned long long load_time;
>> };
>>
>> struct intel_guc_log {
>>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
On Wed, 2017-10-04 at 09:16 +0100, Tvrtko Ursulin wrote:
> On 04/10/2017 01:41, Anusha Srivatsa wrote:
> > Calculate the time that GuC takes to load using
> > jiffies. This information could be very useful in
> > determining if GuC is taking unreasonably long time
> > to load in a certain platforms.
> >
> > v2: Calculate time before logs are collected.
> > Move the guc_load_time variable as a part of
> > intel_uc_fw struct. Store only final result
> > which is to be exported to debugfs. (Michal)
> > Add the load time in the print message as well.
> >
> > v3: Remove debugfs entry. Remove local variable
> > guc_finish_load. (Daniel, Tvrtko)
> >
> > v4: Use ktime_get() instead of jiffies. Use DRM_NOTE
> > if time taken to load is more than the threshold. On
> > load times within acceptable range, use DRM_DEBUG_DRIVER
> > (Tvrtko)
> >
> > Cc: Chris Wilson
> > Cc: Tvrtko ursulin
> > Cc: Daniel Vetter
> > Cc: Sujaritha Sundaresan
> > Cc: Oscar Mateo
> > Cc: Michal Wajdeczko
> > Signed-off-by: Anusha Srivatsa
> > @@ -373,6 +379,8 @@ int intel_guc_init_hw(struct intel_guc *guc)
> > guc->fw.path,
> > guc->fw.major_ver_found, guc->fw.minor_ver_found);
> >
> > + DRM_DEBUG_DRIVER("GuC is loaded in: %lld ms\n",guc->fw.load_time);
> > +
>
> If you move this debug to where the DRM_NOTE is you don't have to store
> the load time in the global structure. Unless there will be a reason in
> the future to have the value stored?
We decided not to expose it through debugfs, so let's try to avoid a
variable, too.
Regards, Joonas
PS. Also, lets try to get to the habit of having the S-o-b: and Cc:s in
chronological order. Less fixing for committers when applying patches.
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
On 04/10/2017 01:41, Anusha Srivatsa wrote:
Calculate the time that GuC takes to load using
jiffies. This information could be very useful in
determining if GuC is taking unreasonably long time
to load in a certain platforms.
v2: Calculate time before logs are collected.
Move the guc_load_time variable as a part of
intel_uc_fw struct. Store only final result
which is to be exported to debugfs. (Michal)
Add the load time in the print message as well.
v3: Remove debugfs entry. Remove local variable
guc_finish_load. (Daniel, Tvrtko)
v4: Use ktime_get() instead of jiffies. Use DRM_NOTE
if time taken to load is more than the threshold. On
load times within acceptable range, use DRM_DEBUG_DRIVER
(Tvrtko)
Cc: Chris Wilson
Cc: Tvrtko ursulin
Cc: Daniel Vetter
Cc: Sujaritha Sundaresan
Cc: Oscar Mateo
Cc: Michal Wajdeczko
Signed-off-by: Anusha Srivatsa
---
drivers/gpu/drm/i915/intel_guc_loader.c | 10 +-
drivers/gpu/drm/i915/intel_uc.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
b/drivers/gpu/drm/i915/intel_guc_loader.c
index c9e25be..a0b562c 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -199,6 +199,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
*dev_priv,
struct sg_table *sg = vma->pages;
u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
int i, ret = 0;
+ ktime_t start_load;
/* where RSA signature starts */
offset = guc_fw->rsa_offset;
@@ -225,6 +226,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
*dev_priv,
I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
/* Finally start the DMA */
+ start_load = ktime_get();
I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
/*
@@ -237,13 +239,17 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
*dev_priv,
*/
ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
+ guc_fw->load_time = ktime_ms_delta(ktime_get(), start_load);
+
DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
I915_READ(DMA_CTRL), status);
if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
DRM_ERROR("GuC firmware signature verification failed\n");
ret = -ENOEXEC;
- }
+ } else if (guc_fw->load_time > 20)
+ DRM_NOTE("Time taken to load GuC is more than the acceptable \
+threshold\n");
DRM_DEBUG_DRIVER("returning %d\n", ret);
@@ -373,6 +379,8 @@ int intel_guc_init_hw(struct intel_guc *guc)
guc->fw.path,
guc->fw.major_ver_found, guc->fw.minor_ver_found);
+ DRM_DEBUG_DRIVER("GuC is loaded in: %lld ms\n",guc->fw.load_time);
+
If you move this debug to where the DRM_NOTE is you don't have to store
the load time in the global structure. Unless there will be a reason in
the future to have the value stored?
Also, where ever it is (global or local) unsigned int is enough since
you picked ms as the resolution.
Regards,
Tvrtko
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 6966349..65b9674 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -136,6 +136,7 @@ struct intel_uc_fw {
uint32_t rsa_offset;
uint32_t ucode_size;
uint32_t ucode_offset;
+ unsigned long long load_time;
};
struct intel_guc_log {
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
On 22/09/2017 21:12, Srivatsa, Anusha wrote: Sending to intel-gfx. -Original Message- From: Ursulin, Tvrtko Sent: Thursday, September 21, 2017 8:16 AM To: Srivatsa, Anusha ; intel- g...@lists.freedektop.org Cc: Chris Wilson ; Vetter, Daniel ; Sundaresan, Sujaritha ; Mateo Lozano, Oscar ; Wajdeczko, Michal Subject: RE: [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log. Hi, For some reason this email hasn't appeared on the mailing list so apologies for a lame Outlook reply. Thanks Tvrtko. I corrected the address. I thought we agreed to use a better time source than jiffies (ktime_get()) and also that DRM_NOTE would get emitted only in the case of load time being over some threshold. If it is in realm of normal it should be a normal DRM_DEBUG_DRIVER. If it is over 20 ms (the threshold) wont DRM_ERROR be a better option? If it is within that limit, then the info will be in DRM_DEBUG_DRIVER from which the QA can pick it. Error only if it fails to load I think. DRM_NOTE was for cases when it succeeds but after taking unexpectedly long. Just so it is a level about informational. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
Sending to intel-gfx.
>-Original Message-
>From: Ursulin, Tvrtko
>Sent: Thursday, September 21, 2017 8:16 AM
>To: Srivatsa, Anusha ; intel-
>g...@lists.freedektop.org
>Cc: Chris Wilson ; Vetter, Daniel
>; Sundaresan, Sujaritha
>; Mateo Lozano, Oscar
>; Wajdeczko, Michal
>Subject: RE: [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
>
>
>Hi,
>
>For some reason this email hasn't appeared on the mailing list so apologies
>for a
>lame Outlook reply.
Thanks Tvrtko. I corrected the address.
>I thought we agreed to use a better time source than jiffies (ktime_get()) and
>also
>that DRM_NOTE would get emitted only in the case of load time being over some
>threshold. If it is in realm of normal it should be a normal DRM_DEBUG_DRIVER.
If it is over 20 ms (the threshold) wont DRM_ERROR be a better option? If it is
within that limit, then the info will be in DRM_DEBUG_DRIVER from which the QA
can pick it.
Anusha
>Tvrtko
>
>-Original Message-
>From: Srivatsa, Anusha
>Sent: Thursday, September 21, 2017 1:44 AM
>To: intel-...@lists.freedektop.org
>Cc: Srivatsa, Anusha ; Chris Wilson wilson.co.uk>; Ursulin, Tvrtko ; Vetter, Daniel
>; Sundaresan, Sujaritha
>; Mateo Lozano, Oscar
>; Wajdeczko, Michal
>Subject: [PATCH 1/2] drm/i915/guc: Add GuC Load time to dmesg log.
>
>Calculate the time that GuC takes to load using jiffies. This information
>could be
>very useful in determining if GuC is taking unreasonably long time to load in a
>certain platforms.
>
>v2: Calculate time before logs are collected.
>Move the guc_load_time variable as a part of intel_uc_fw struct. Store only
>final
>result which is to be exported to debugfs. (Michal) Add the load time in the
>print
>message as well.
>
>v3: Remove debugfs entry. Remove local variable guc_finish_load. (Daniel,
>Tvrtko)
>
>Cc: Chris Wilson
>Cc: Tvrtko ursulin
>Cc: Daniel Vetter
>Cc: Sujaritha Sundaresan
>Cc: Oscar Mateo
>Cc: Michal Wajdeczko
>Signed-off-by: Anusha Srivatsa
>---
> drivers/gpu/drm/i915/intel_guc_loader.c | 7 +++
> drivers/gpu/drm/i915/intel_uc.h | 1 +
> 2 files changed, 8 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>b/drivers/gpu/drm/i915/intel_guc_loader.c
>index 8b0ae7f..4b1fc55 100644
>--- a/drivers/gpu/drm/i915/intel_guc_loader.c
>+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>@@ -199,6 +199,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
>*dev_priv,
> struct sg_table *sg = vma->pages;
> u32 status, rsa[UOS_RSA_SCRATCH_MAX_COUNT];
> int i, ret = 0;
>+ unsigned long guc_start_load;
>
> /* where RSA signature starts */
> offset = guc_fw->rsa_offset;
>@@ -226,6 +227,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
>*dev_priv,
>
> /* Finally start the DMA */
> I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE |
>START_DMA));
>+ guc_start_load = jiffies;
>
> /*
>* Wait for the DMA to complete & the GuC to start up.
>@@ -237,6 +239,8 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
>*dev_priv,
>*/
> ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
>
>+ guc_fw->load_time = jiffies_to_msecs(jiffies - guc_start_load);
>+
> DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
> I915_READ(DMA_CTRL), status);
>
>@@ -372,6 +376,9 @@ int intel_guc_init_hw(struct intel_guc *guc)
>guc->fw.path,
>guc->fw.major_ver_found, guc->fw.minor_ver_found);
>
>+ DRM_NOTE("Time taken to load GuC is %lu ms\n",
>+ guc->fw.load_time);
>+
> return 0;
> }
>
>diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
>index 7703c9a..749b069 100644
>--- a/drivers/gpu/drm/i915/intel_uc.h
>+++ b/drivers/gpu/drm/i915/intel_uc.h
>@@ -136,6 +136,7 @@ struct intel_uc_fw {
> uint32_t rsa_offset;
> uint32_t ucode_size;
> uint32_t ucode_offset;
>+ unsigned long load_time;
> };
>
> struct intel_guc_log {
>--
>2.7.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
