Re: [Intel-gfx] [PATCH 3/8] drm/i915/pcode: Extend pcode functions for multiple gt's

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Wed, 11 May 2022 01:18:07 -0700, Tvrtko Ursulin wrote:
> On 11/05/2022 06:26, Dixit, Ashutosh wrote:
> > On Tue, 10 May 2022 00:43:29 -0700, Jani Nikula wrote:
> >> On Tue, 10 May 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> >>> On 29/04/2022 20:56, Ashutosh Dixit wrote:
> >>>> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> >>>> index 90b0ce5051af..bc49eff38c6a 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_driver.c
> >>>> +++ b/drivers/gpu/drm/i915/i915_driver.c
> >>>> @@ -520,6 +520,22 @@ static int i915_set_dma_info(struct drm_i915_private *i915)
> >>>>	return ret;
> >>>>    }
> >>>>
> >>>> +static int i915_pcode_init(struct drm_i915_private *i915)
> >>>> +{
> >>>> +	struct intel_gt *gt;
> >>>> +	int id, ret;
> >>>> +
> >>>> +	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);
> >>>
> >>> A few nits..
> >>>
> >>> 1) All other/current logs use "gt%d" (no space).

Fixed.

> >>>
> >>> 2) intel_pcode_init also logs a drm_err - do we need two? I suggest
> >>> leaving this one only since it has more information.

Remove duplicated drm_err and also drm_dbg from intel_pcode_init() in Patch
v4 (Series v5).

> >>>
> >>> 3) It would have been nicer to have refactoring of intel_pcode_ to work
> >>> on uncore separate from adding for_each_gt.
> >>
> >> Yeah.
> >>
> >> Also the obvious first patch would've been to convert intel_pcode.c
> >> functions from struct drm_i915_private * to intel_uncore *.
> >
> > Will fix up the first 2 points but about this last point, to not break
> > incremental compile all callers of the pcode functions also need to be
> > converted to i915->uncore or gt->uncore (so it's not possible to convert
> > just intel_pcode.c functions without also converting all callers, if that
> > was the intent of this comment, unless I am missing something).
>
> Yes the implication is to convert the callers when doing such conversion,
> we never do broken commits.
>
> > But yes the i915_pcode_init() above can be separated out to a separate
> > patch so I can do that.
>
> AFAIR that will achieve what is suggested, thanks!

Move out i915_pcode_init() to a separate patch "drm/i915/pcode: Init pcode
on different gt's" in Patch v4 (Series v5).

Thanks.
--
Ashutosh