Thanks for reviewing.
On Wed, 2023-01-18 at 16:09 -0800, Ceraolo Spurio, Daniele wrote:
> >
> >
Alan: [snip]
> > > > -/* KCR register definitions */
> > > > -#define KCR_INIT _MMIO(0x320f0)
> > > > -/* Setting KCR Init bit is required after system boot */
> > > > -#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES REG_BIT(14)
> > > > +static i915_reg_t get_kcr_reg(const struct intel_pxp *pxp)
> > > > +{
> > > > + if (pxp->gsccs_priv)
> >
> > IMO here a better check would be:
> >
> > if (pxp->ctrl_gt->type == GT_MEDIA)
> >
> > It's equivalent, but IMO from a readability perspective it highlights
> > the fact that this is a change due to us moving to the media GT model
> > and has nothing to do with the GSC CS itself.
> >
Alan: Yes agreed - in alignment with to your next comment, this readiblity
improvement shall therefore go into pxp_init (when we initialize value for kcr_base offset)
Alan: [snip]
> > > > +#ifndef __INTEL_PXP_REGS_H__
> > > > +#define __INTEL_PXP_REGS_H__
> > > > +
> > > > +#include "i915_reg_defs.h"
> > > > +
> > > > +/* KCR enable/disable control */
> > > > +#define GEN12_KCR_INIT _MMIO(0x320f0)
> > > > +#define MTL_KCR_INIT _MMIO(0x3860f0)
> > > > +
> > > > +/* Setting KCR Init bit is required after system boot */
> > > > +#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES REG_BIT(14)
> > > > +
> > > > +/* KCR hwdrm session in play status 0-31 */
> > > > +#define GEN12_KCR_SIP _MMIO(0x32260)
> > > > +#define MTL_KCR_SIP _MMIO(0x386260)
> > > > +
> > > > +/* PXP global terminate register for session termination */
> > > > +#define GEN12_KCR_GLOBAL_TERMINATE _MMIO(0x320f8)
> > > > +#define MTL_KCR_GLOBAL_TERMINATE _MMIO(0x3860f8)
> >
> > Non blocking suggestion:
> > it looks like all KCR regs are at the same relative offsets, just from a
> > different base (which makes, sense, because the HW just got moved to the
> > media tile as-is). So we could possibly have something like what we do
> > for the engines:
> >
> > #define GEN12_KCR_BASE 0x32000
> > #define MTL_KCR_BASE 0x386000
> >
> > #define KCR_INIT(base) _MMIO(base + 0xf0)
> > #define KCR_GLOBAL_TERMINATE(base) _MMIO(base + 0xf8)
> > #define KCR_SIP(base) _MMIO(base + 0x260)
> >
> > Then if we store the correct base in the pxp structure we can just pass
> > it in the define when we need to access a register and remove the if
> > conditions at each access.
> >
Alan: sounds good - will do this.
Alan: [snip]
