From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1465E6EDC5 for ; Thu, 16 Jan 2020 21:47:08 +0000 (UTC) Date: Thu, 16 Jan 2020 13:47:07 -0800 Message-ID: <87muanvztw.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <157900049765.27314.15743110141892570457@skylake-alporthouse-com> References: <20200114052728.13313-1-zbigniew.kempczynski@intel.com> <87eew2zig5.wl-ashutosh.dixit@intel.com> <157900049765.27314.15743110141892570457@skylake-alporthouse-com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Subject: Re: [igt-dev] [PATCH i-g-t] lib/i915/gem_mman.c: add cpu coherency mapping wrapper List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-2" Content-Transfer-Encoding: quoted-printable Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Chris Wilson Cc: igt-dev@lists.freedesktop.org List-ID: On Tue, 14 Jan 2020 03:14:57 -0800, Chris Wilson wrote: > > Quoting Dixit, Ashutosh (2020-01-14 05:59:54) > > On Mon, 13 Jan 2020 21:27:28 -0800, Zbigniew Kempczy=F1ski wrote: > > > > > > For reduce code redundancy adding a wrapper for cpu memory mapping. > > > > > > +void *__gem_mmap__cpu_coherent(int fd, uint32_t handle, uint64_t off= set, > > > + uint64_t size, unsigned prot) > > > +{ > > > + void *ptr =3D __gem_mmap_offset__cpu(fd, handle, offset, size, = prot); > > > + > > > + if (!ptr) > > > + ptr =3D __gem_mmap__cpu(fd, handle, offset, size, prot); > > > + > > > + return ptr; > > > +} > > > > We need similar wrappers for WC and GTT too. So why don't we put this c= ode > > That's gem_mmap__device_coherent. > > > in __gem_mmap__cpu() itself and we can do the same for __gem_mmap__wc()= and > > __gem_mmap__gtt() too? Otherwise what are we going to call those functi= ons? > > Something like: > > > > void *__gem_mmap__cpu(int fd, uint32_t handle, uint64_t offset, uint64_= t size, unsigned prot) > > { > > if (gem_has_mmap_offset(fd)) > > return __gem_mmap_offset(fd, handle, offset, size, prot= , I915_MMAP_OFFSET_WB); > > else > > return __gem_mmap(fd, handle, offset, size, prot, 0); > > } > > > > So I am not sure of the point of introducing new wrappers, this code co= uld > > just be put in the old existing wrappers. > > The point is that we are separating intent from implementation. Where a > test is not looking at the mmap ioctl, but just wants access to a > buffer, how it wants to access that buffer is the important bit of > information. After Chris' explanation: Acked-by: Ashutosh Dixit _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev