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

[Dixit, Ashutosh]

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.
--
Ashutosh