diff mbox series

[2/2] drm/i915: More use of GT specific print helpers

Message ID 20231009183802.673882-3-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show
Series More print message helper updates | expand

Commit Message

John Harrison Oct. 9, 2023, 6:38 p.m. UTC 
From: John Harrison <John.C.Harrison@Intel.com>

Update a bunch of GT related print messages in non-GT files to use the
GT specific helpers.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 8 +++-----
 drivers/gpu/drm/i915/i915_driver.c        | 3 ++-
 drivers/gpu/drm/i915/i915_perf.c          | 8 ++++----
 3 files changed, 9 insertions(+), 10 deletions(-)

Comments

Andi Shyti Oct. 9, 2023, 7:54 p.m. UTC | #1 
Hi John,

...

> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -71,6 +71,7 @@
>  #include "gem/i915_gem_pm.h"
>  #include "gt/intel_gt.h"
>  #include "gt/intel_gt_pm.h"
> +#include "gt/intel_gt_print.h"
>  #include "gt/intel_rc6.h"
>  
>  #include "pxp/intel_pxp.h"
> @@ -429,7 +430,7 @@ static int i915_pcode_init(struct drm_i915_private *i915)
>  	for_each_gt(gt, i915, id) {
>  		ret = intel_pcode_init(gt->uncore);
>  		if (ret) {
> -			drm_err(&gt->i915->drm, "gt%d: intel_pcode_init failed %d\n", id, ret);
> +			gt_err(gt, "intel_pcode_init failed %d\n", ret);

using gt_*() print functions in the upper layers looks a bit
wrong to me. If we need GT printing, the prints need to be done
inside the function called, in this case would be
intel_pcode_init().

Andi
John Harrison Oct. 9, 2023, 7:57 p.m. UTC | #2 
On 10/9/2023 12:54, Andi Shyti wrote:
> Hi John,
>
> ...
>
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -71,6 +71,7 @@
>>   #include "gem/i915_gem_pm.h"
>>   #include "gt/intel_gt.h"
>>   #include "gt/intel_gt_pm.h"
>> +#include "gt/intel_gt_print.h"
>>   #include "gt/intel_rc6.h"
>>   
>>   #include "pxp/intel_pxp.h"
>> @@ -429,7 +430,7 @@ static int i915_pcode_init(struct drm_i915_private *i915)
>>   	for_each_gt(gt, i915, id) {
>>   		ret = intel_pcode_init(gt->uncore);
>>   		if (ret) {
>> -			drm_err(&gt->i915->drm, "gt%d: intel_pcode_init failed %d\n", id, ret);
>> +			gt_err(gt, "intel_pcode_init failed %d\n", ret);
> using gt_*() print functions in the upper layers looks a bit
> wrong to me. If we need GT printing, the prints need to be done
> inside the function called, in this case would be
> intel_pcode_init().
It is less wrong that using gt->i915->drm as a parameter and 'gt%d' in 
the format string. That is the whole point of the helper. The code has 
access to a gt object so it should use the gt helper to make use of that 
object rather than unrolling it and diving in to the gt internals.

As for moving the error message inside the init function itself. That is 
maybe a valid change but that potentially counts as a functional change 
and should be done by someone who actually knows the code. All I'm doing 
is improving the code layering by using the correct helper to hide the 
internal details of an object this layer should not know about.

John.

>
> Andi
Andi Shyti Oct. 10, 2023, 8:22 p.m. UTC | #3 
Hi John,

On Mon, Oct 09, 2023 at 12:57:55PM -0700, John Harrison wrote:
> On 10/9/2023 12:54, Andi Shyti wrote:
> > Hi John,
> > 
> > ...
> > 
> > > --- a/drivers/gpu/drm/i915/i915_driver.c
> > > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > > @@ -71,6 +71,7 @@
> > >   #include "gem/i915_gem_pm.h"
> > >   #include "gt/intel_gt.h"
> > >   #include "gt/intel_gt_pm.h"
> > > +#include "gt/intel_gt_print.h"
> > >   #include "gt/intel_rc6.h"
> > >   #include "pxp/intel_pxp.h"
> > > @@ -429,7 +430,7 @@ static int i915_pcode_init(struct drm_i915_private *i915)
> > >   	for_each_gt(gt, i915, id) {
> > >   		ret = intel_pcode_init(gt->uncore);
> > >   		if (ret) {
> > > -			drm_err(&gt->i915->drm, "gt%d: intel_pcode_init failed %d\n", id, ret);
> > > +			gt_err(gt, "intel_pcode_init failed %d\n", ret);
> > using gt_*() print functions in the upper layers looks a bit
> > wrong to me. If we need GT printing, the prints need to be done
> > inside the function called, in this case would be
> > intel_pcode_init().
> It is less wrong that using gt->i915->drm as a parameter and 'gt%d' in the
> format string. That is the whole point of the helper. The code has access to
> a gt object so it should use the gt helper to make use of that object rather
> than unrolling it and diving in to the gt internals.

yes, it's an improvement

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

> As for moving the error message inside the init function itself. That is
> maybe a valid change but that potentially counts as a functional change and
> should be done by someone who actually knows the code. All I'm doing is
> improving the code layering by using the correct helper to hide the internal
> details of an object this layer should not know about.

maybe one day we need to revisit all the gt dependency in the
higher levels and the i915 dependencies in gt.

Thanks,
Andi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
index 0d3b22a743659..453d855dd1de7 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -68,8 +68,7 @@  static void gsc_work(struct work_struct *work)
 				 * A proxy failure right after firmware load means the proxy-init
 				 * step has failed so mark GSC as not usable after this
 				 */
-				drm_err(&gt->i915->drm,
-					"GSC proxy handler failed to init\n");
+				gt_err(gt, "GSC proxy handler failed to init\n");
 				intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_LOAD_FAIL);
 			}
 			goto out_put;
@@ -83,11 +82,10 @@  static void gsc_work(struct work_struct *work)
 			 * status register to check if the proxy init was actually successful
 			 */
 			if (intel_gsc_uc_fw_proxy_init_done(gsc, false)) {
-				drm_dbg(&gt->i915->drm, "GSC Proxy initialized\n");
+				gt_dbg(gt, "GSC Proxy initialized\n");
 				intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_RUNNING);
 			} else {
-				drm_err(&gt->i915->drm,
-					"GSC status reports proxy init not complete\n");
+				gt_err(gt, "GSC status reports proxy init not complete\n");
 				intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_LOAD_FAIL);
 			}
 		}
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index e5a94b08d5efe..944ab895da72e 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -71,6 +71,7 @@ 
 #include "gem/i915_gem_pm.h"
 #include "gt/intel_gt.h"
 #include "gt/intel_gt_pm.h"
+#include "gt/intel_gt_print.h"
 #include "gt/intel_rc6.h"
 
 #include "pxp/intel_pxp.h"
@@ -429,7 +430,7 @@  static int i915_pcode_init(struct drm_i915_private *i915)
 	for_each_gt(gt, i915, id) {
 		ret = intel_pcode_init(gt->uncore);
 		if (ret) {
-			drm_err(&gt->i915->drm, "gt%d: intel_pcode_init failed %d\n", id, ret);
+			gt_err(gt, "intel_pcode_init failed %d\n", ret);
 			return ret;
 		}
 	}
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 1347e4ec9dd5a..8f7ab64feec0d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -206,6 +206,7 @@ 
 #include "gt/intel_gt.h"
 #include "gt/intel_gt_clock_utils.h"
 #include "gt/intel_gt_mcr.h"
+#include "gt/intel_gt_print.h"
 #include "gt/intel_gt_regs.h"
 #include "gt/intel_lrc.h"
 #include "gt/intel_lrc_reg.h"
@@ -1659,9 +1660,8 @@  static void i915_oa_stream_destroy(struct i915_perf_stream *stream)
 	free_noa_wait(stream);
 
 	if (perf->spurious_report_rs.missed) {
-		drm_notice(&gt->i915->drm,
-			   "%d spurious OA report notices suppressed due to ratelimiting\n",
-			   perf->spurious_report_rs.missed);
+		gt_notice(gt, "%d spurious OA report notices suppressed due to ratelimiting\n",
+			  perf->spurious_report_rs.missed);
 	}
 }
 
@@ -1852,7 +1852,7 @@  static int alloc_oa_buffer(struct i915_perf_stream *stream)
 	 */
 	ret = i915_vma_pin(vma, 0, SZ_16M, PIN_GLOBAL | PIN_HIGH);
 	if (ret) {
-		drm_err(&gt->i915->drm, "Failed to pin OA buffer %d\n", ret);
+		gt_err(gt, "Failed to pin OA buffer %d\n", ret);
 		goto err_unref;
 	}