From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id B86F310E920 for ; Fri, 11 Mar 2022 11:30:22 +0000 (UTC) Date: Fri, 11 Mar 2022 12:30:18 +0100 From: Zbigniew =?utf-8?Q?Kempczy=C5=84ski?= To: Kamil Konieczny , igt-dev@lists.freedesktop.org, Apoorva Singh , Arjun Melkaveri Message-ID: References: <20220310071529.61930-1-zbigniew.kempczynski@intel.com> <20220310071529.61930-4-zbigniew.kempczynski@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [igt-dev] [PATCH i-g-t 3/6] lib/i915: Introduce library intel_mocs List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On Fri, Mar 11, 2022 at 12:14:52PM +0100, Kamil Konieczny wrote: > Hi Zbigniew, > > Dnia 2022-03-10 at 08:15:26 +0100, Zbigniew Kempczyński napisał(a): > > From: Apoorva Singh > > > > Add new library intel_mocs for mocs settings. > > > > Signed-off-by: Apoorva Singh > > Cc: Zbigniew Kempczyński > > Cc: Arjun Melkaveri > > Reviewed-by: Zbigniew Kempczyński > > --- > > lib/i915/intel_mocs.c | 56 +++++++++++++++++++++++++++++++++++++++++++ > > lib/i915/intel_mocs.h | 25 +++++++++++++++++++ > > lib/meson.build | 1 + > > 3 files changed, 82 insertions(+) > > create mode 100644 lib/i915/intel_mocs.c > > create mode 100644 lib/i915/intel_mocs.h > > > > diff --git a/lib/i915/intel_mocs.c b/lib/i915/intel_mocs.c > > new file mode 100644 > > index 0000000000..63ead1118f > > --- /dev/null > > +++ b/lib/i915/intel_mocs.c > > @@ -0,0 +1,56 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright © 2022 Intel Corporation > > + */ > > + > > +#include "igt.h" > > +#include "i915/gem.h" > > +#include "intel_mocs.h" > > + > > +static void get_mocs_index(int fd, struct drm_i915_mocs_index *mocs) > > +{ > > + uint16_t devid = intel_get_drm_devid(fd); > > + > > + /* > > + * Gen >= 12 onwards don't have a setting for PTE, > > + * so using I915_MOCS_PTE as mocs index may leads to > > + * some undefined MOCS behavior. > > + * This helper function is providing current UC as well > > + * as WB MOCS index based on platform. > > + */ > > + if (IS_DG1(devid)) { > > + mocs->uc_index = DG1_MOCS_UC_IDX; > > + mocs->wb_index = DG1_MOCS_WB_IDX; > > + } else if (IS_DG2(devid)) { > > + mocs->uc_index = DG2_MOCS_UC_IDX; > > + mocs->wb_index = DG2_MOCS_WB_IDX; > > + > > + } else if (IS_GEN12(devid)) { > > + mocs->uc_index = GEN12_MOCS_UC_IDX; > > + mocs->wb_index = GEN12_MOCS_WB_IDX; > > + } else { > > + mocs->uc_index = I915_MOCS_PTE; > > + mocs->wb_index = I915_MOCS_CACHED; > > + igt_info("C1\n"); > > Remove this or make it igt_debug. Eh, thanks for spotting that. > > > + } > > +} > > + > > +/* BitField [6:1] represents index to MOCS Tables > > + * BitField [0] represents Encryption/Decryption > > + */ > > + > > +uint8_t intel_get_wb_mocs(int fd) > > +{ > > + struct drm_i915_mocs_index mocs; > > + > > + get_mocs_index(fd, &mocs); > > + return mocs.wb_index << 1; > > +} > > + > > +uint8_t intel_get_uc_mocs(int fd) > > +{ > > + struct drm_i915_mocs_index mocs; > > + > > + get_mocs_index(fd, &mocs); > > + return mocs.uc_index << 1; > > +} > > diff --git a/lib/i915/intel_mocs.h b/lib/i915/intel_mocs.h > > new file mode 100644 > > index 0000000000..c05569f426 > > --- /dev/null > > +++ b/lib/i915/intel_mocs.h > > @@ -0,0 +1,25 @@ > > +/* SPDX-License-Identifier: MIT */ > > +/* > > + * Copyright © 2022 Intel Corporation > > + */ > > + > > +#ifndef _INTEL_MOCS_H > > +#define _INTEL_MOCS_H > > + > > +#define DG1_MOCS_UC_IDX 1 > > +#define DG1_MOCS_WB_IDX 5 > > +#define DG2_MOCS_UC_IDX 1 > > +#define DG2_MOCS_WB_IDX 3 > > +#define GEN12_MOCS_UC_IDX 3 > > +#define GEN12_MOCS_WB_IDX 2 > > Do we need this here ? Can it be placed in lib implementation > and hided there ? Agree, exporting this is not necessary. > > > +#define XY_BLOCK_COPY_BLT_MOCS_SHIFT 21 > > +#define XY_CTRL_SURF_COPY_BLT_MOCS_SHIFT 25 > > Same here, there is no uses for this constants. They can stay > if you can tell they will be used in near future. Agree. > > > + > > +struct drm_i915_mocs_index { > > + uint8_t uc_index; > > + uint8_t wb_index; > > +}; > > + > > +uint8_t intel_get_wb_mocs(int fd); > > +uint8_t intel_get_uc_mocs(int fd); > > These helpers returns uc and wb index, so maybe struct can be > moved to .c implementation ? So we can keep headers clean. It is > up to you, I do not insist on changes in header file. You're right, if we can hide implementation details we should do this. Will send in v6. > > > +#endif /* _INTEL_MOCS_H */ > > diff --git a/lib/meson.build b/lib/meson.build > > index 3e43316d1e..a5fe91dc99 100644 > > --- a/lib/meson.build > > +++ b/lib/meson.build > > @@ -11,6 +11,7 @@ lib_sources = [ > > 'i915/gem_mman.c', > > 'i915/gem_vm.c', > > 'i915/intel_memory_region.c', > > + 'i915/intel_mocs.c', > > 'igt_collection.c', > > 'igt_color_encoding.c', > > 'igt_debugfs.c', > > -- > > 2.32.0 > > > > With that fixed you can add my r-b tag, Ok, will hide implementation and resend with r-b. Thank you for the review. -- Zbigniew > > Regards, > Kamil