[Intel-gfx] [PATCH] drm/i915/hwmon: Silence "mailbox access failed" warning in snb_pcode_read

[Gupta, Anshuman]

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit at intel.com>
> Sent: Monday, December 5, 2022 7:10 AM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Nilawar, Badal
> <badal.nilawar at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Subject: Re: [PATCH] drm/i915/hwmon: Silence "mailbox access failed"
> warning in snb_pcode_read
> 
> On Sat, 03 Dec 2022 01:47:06 -0800, Gupta, Anshuman wrote:
> >
> 
> Hi Anshuman,
> 
> > > > hwm_pcode_read_i1 is called during i915 load. This results in the
> > > > following warning from snb_pcode_read because
> > > > POWER_SETUP_SUBCOMMAND_READ_I1 is unsupported on DG1/DG2.
> > > >
> > > > [drm:snb_pcode_read [i915]] warning: pcode (read from mbox 47c) \
> > > >				mailbox access failed for snb_pcode_read_p
> > > > [i915]: -6
> > > >
> > > > The code handles the unsupported command but the warning in dmesg
> > > > is a red herring which has resulted in a couple of bugs being filed.
> > > > Therefore silence the warning by avoiding calling snb_pcode_read_p
> > > > for > > DG1/DG2.
> > > >
> > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_hwmon.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> > > > b/drivers/gpu/drm/i915/i915_hwmon.c
> > > > index c588a17f97e98..cca7a4350ec8f 100644
> > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > > > @@ -293,6 +293,10 @@ static const struct hwmon_channel_info
> > > > *hwm_gt_info[] = {
> > > >  /* I1 is exposed as power_crit or as curr_crit depending on bit
> > > > 31 */ static int hwm_pcode_read_i1(struct drm_i915_private *i915,
> > > > u32 *uval) {
> > > > +	/* Avoid ILLEGAL_SUBCOMMAND "mailbox access failed" warning in
> > > > snb_pcode_read */
> > > > +	if (IS_DG1(i915) || IS_DG2(i915))
> > > > +		return -ENXIO;
> > >
> > > AFAIK it is specific to client specific parts,
> 
> No this is not true, see c8939848f7e4 where it says I1 power is exposed as
> power1_crit for client products and as curr1_crit for server. Also I know this is
> available on future client products. So it appears only DG1 and DG2 were an
> exception because of lack of HW support, this will available in the future for
> client. Therefore the patch looks correct to me.
> 
> > > how about declaring a is_client intel_runtime_info flag to
> > > distinguish between client and server part. That *intel_device_info*
> > > will also cover any future platform as well.
Thanks for explanation.
Reviewed-by: Anshuman Gupta <anshuman.gupta at intel.com>
> 
> Thanks.
> --
> Ashutosh