On 2.8.2021 16.04, Sharma, Swati2 wrote:

Hi JP,

Tejas reapplied the same fix as in 
https://patchwork.freedesktop.org/patch/445010/?series=92751&rev=2 which 
was reviewed by Daniel.

I didn't notice anyone having reviewed that patch but it's different 
story. That patch anyway is about vgem driver.

Those comments about alignments apply to that kms_prime patch too, fix 
there is not aligning to 256 bytes but 256 pixels. kms_prime use 32bpp 
hence it's aligning to 1024B, not to 256B

Fix there is to be able to move buffer memory between devices..and not 
trying to be too complex with it. Real fix there would be to do fb stuff 
correctly, not like kms_prime now does:

	for (size_t idx = 0; idx < scratch->size / sizeof(*ptr); ++idx)
		ptr[idx] = color;

On i915 driver itself does agree with itself about alignments. I still 
think intel hw will be able to show varying size fb's, not just those 
where width is divisible by 256. We have all the juggling with strides 
to take care of fb alignments.

As is I see this patch was only hiding some potential real problem and 
kms_prime would need proper fix too.

/Juha-Pekka

On 02-Aug-21 5:42 PM, Juha-Pekka Heikkila wrote:

quoted

Hi Tejas, Swati,

I was confused when I read this patch, mind you tell what happen here? 
To me it look like nothing matches with each other with the patch.

Even subject is telling other things than what this patch actually does.

On 30.7.2021 8.50, Sharma, Swati2 wrote:

quoted

Reviewed-by:
Swati Sharma [off-list ref]

On 28-Jul-21 10:25 AM, Tejas Upadhyay wrote:

quoted

some display resolutions like 1366x768 6bpc which does not
have 64B aligned width are creating crc mismatch in
kms_plane_alpha_blend test on Intel platforms.

hm. You are saying none of Intel hw is able to show 1366 wide 
framebuffers correctly? And it is fixed by hiding it from being tested?

Memory given from kernel will have 64B alignment. You can easily see 
by yourself.

quoted

quoted

Also having different alignment requirement by different drivers,
256B aligned width should work for all drm drivers.

You are saying you are fixing some problem with Intel hw, what does 
all this other stuff have to do with it? None of those other drivers 
are able to show 1366 pixels wide framebuffers either?

quoted

quoted

amdgpu and radeon, amdgpu_align_pitch: 256B
armada, armada_pitch: 128B
exynos_drm_gem_dumb_create: No alignment required
drm_gem_shmem_dumb_create: 8B
drm_gem_vram_fill_create_dumb: 8B

Thus 256B covers everything we see in the kernel drm drivers.
Signed-off-by: Tejas Upadhyay 
[off-list ref]
---
  tests/kms_plane_alpha_blend.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/tests/kms_plane_alpha_blend.c 

b/tests/kms_plane_alpha_blend.c
index d649a09f..864e83f9 100644

--- a/tests/kms_plane_alpha_blend.c
+++ b/tests/kms_plane_alpha_blend.c

@@ -168,6 +168,7 @@ static void prepare_crtc(data_t *data, 

igt_output_t *output, enum pipe pipe)
      w = mode->hdisplay;
      h = mode->vdisplay;
+    w = ALIGN(w, 256);

This doesn't cause anything to align with 256 bytes. This makes fb 
width in pixels divisible by 256. For anything to do with fb 
alignments..this has very little to do. Kernel will do viewport 
clipping hence for actual intended test this does nothing. FB 
alignments are handled otherwise as in this case with with linear fb 
will have 64 bytes per stride.

quoted

quoted

      /* recreate all fbs if incompatible */
      if (data->xrgb_fb.width != w || data->xrgb_fb.height != h) {
          cairo_t *cr;

If this patch actually fixed anything you'll need to create hw wa and 
this need to be fixed in kernel. Not testing is not fixing. Imo there 
should be no problem for Intel hw to show varying fb sizes, including 
1366 wide.

/Juha-Pekka