Re: [igt-dev] [PATCH i-g-t 2/2] lib/igt_draw: Use device coherent and cpu coherent mappings for draw methods.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 19, 2021 at 11:40:30AM +0200, Maarten Lankhorst wrote:
> This will allow kms_frontbuffer_tracking to draw buffers for frontbuffer tracking on dg1.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  lib/igt_draw.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/igt_draw.c b/lib/igt_draw.c
> index da1d39fcba95..62ca4761a9f2 100644
> --- a/lib/igt_draw.c
> +++ b/lib/igt_draw.c
> @@ -341,8 +341,8 @@ static void draw_rect_mmap_cpu(int fd, struct buf_data *buf, struct rect *rect,
>  	if (tiling != I915_TILING_NONE)
>  		igt_require(intel_display_ver(intel_get_drm_devid(fd)) >= 5);
>  
> -	ptr = gem_mmap__cpu(fd, buf->handle, 0, PAGE_ALIGN(buf->size),
> -			    PROT_READ | PROT_WRITE);
> +	ptr = gem_mmap__cpu_coherent(fd, buf->handle, 0, PAGE_ALIGN(buf->size),
> +				     PROT_READ | PROT_WRITE);
>  
>  	switch (tiling) {
>  	case I915_TILING_NONE:
> @@ -391,8 +391,8 @@ static void draw_rect_mmap_wc(int fd, struct buf_data *buf, struct rect *rect,
>  	if (tiling != I915_TILING_NONE)
>  		igt_require(intel_display_ver(intel_get_drm_devid(fd)) >= 5);
>  
> -	ptr = gem_mmap__wc(fd, buf->handle, 0, PAGE_ALIGN(buf->size),
> -			   PROT_READ | PROT_WRITE);
> +	ptr = gem_mmap__device_coherent(fd, buf->handle, 0, PAGE_ALIGN(buf->size),
> +					PROT_READ | PROT_WRITE);

That seems to have a silent fallback to mmap_gtt, so on
first blush this does not seem to have 100% identical
behaviour anymore.

Then again gem_mmap__wc() should assert if you attempt to
use it on a machine that doesn't support WC mmaps. So I guess
it ends up being the same in practice? Maybe we should have an
assert in draw_rect_mmap_wc() that either lmem or wc mmap is
available?

Or maybe we don't want to conflate mmap_wc and mmap_fixed
(which I guess is some code for "lmem"?) and instead igt_draw
should have separate methods for each?

>  
>  	switch (tiling) {
>  	case I915_TILING_NONE:
> -- 
> 2.33.0

-- 
Ville Syrjälä
Intel