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(>->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@xxxxxxxxxxxxxxx>
> 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
