[Intel-gfx] [PATCH v2 3/9] drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation

[Teres Alexis, Alan Previn]

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]