[igt-dev] [i-g-t] tests/kms_plane_alpha_blend: Align width to 256B

Tue Aug 3 17:49:12 UTC 2021

On Tue, Aug 3, 2021 at 7:16 PM Srinivas, Vidya <vidya.srinivas at intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of Juha-
> > Pekka Heikkilä
> > Sent: Tuesday, August 3, 2021 12:49 AM
> > To: Surendrakumar Upadhyay, TejaskumarX
> > <tejaskumarx.surendrakumar.upadhyay at intel.com>
> > Cc: Sharma, Swati2 <swati2.sharma at intel.com>; igt-
> > dev at lists.freedesktop.org
> > Subject: Re: [igt-dev] [i-g-t] tests/kms_plane_alpha_blend: Align width to
> > 256B
> >
> > On Mon, Aug 2, 2021 at 5:16 PM Surendrakumar Upadhyay, TejaskumarX
> > <tejaskumarx.surendrakumar.upadhyay at intel.com> wrote:
> > >
> > > Hi JP,
> > >
> > > You are right i915 driver is giving 64B aligned buffer but test uses raw width
> > and thus it requires width to be aligned.
> >
> > Can you try this on the device which is failing:
> > https://patchwork.freedesktop.org/series/93317/
> >
> > It will put on pixel blend and alpha and try different width linear fbs and
> > compare results with crc. This should tell if alignments somehow can affect
> > the crc on your test machine. On my ICL everything work as expected.
> >
>
> Hi Juha-Pekka
>
> With your https://patchwork.freedesktop.org/series/93317/ the test PASSES.
> But we need to use both planes and exact same sequence as kms_plane_alpha_blend coverage-premultiplied-constant case
> to reproduce the issue.
>
> But with this modification (same as kms_plane_alpha_blend) done here https://patchwork.freedesktop.org/patch/448369/?series=93317&rev=3
> It PASS with ALIGN in draw_squares and draw_squares_coverage but fails without the ALIGNMENT.
>
> Kindly have a check. Thank you. Apologies, I pushed rev3 on top of yours to try-bot to maintain context.
>

Hi Vidya,

thanks for showing this. Your change should segfault because with your
alignment that memory drawing function will draw past reserved memory
but that's not the interesting part. That original bug was included on
the cut'n'pasted lines, it has nothing to do with strides but just
simple rounding issue.

Where there's this function:

+static void draw_squares(struct igt_fb *fb, int w, int h, double a)
+{
+ cairo_t *cr = igt_get_cairo_ctx(fb->fd, fb);
+
+ cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE);
+ igt_paint_color_alpha(cr, 0, 0, w / 2, h / 2, 1., 0., 0., a);
+ igt_paint_color_alpha(cr, w / 2, 0, w / 2, h / 2, 0., 1., 0., a);
+ igt_paint_color_alpha(cr, 0, h / 2, w / 2, h / 2, 0., 0., 1., a);
+ igt_paint_color_alpha(cr, w / 2, h / 2, w / 4, h / 2, 1., 1., 1., a);
+ igt_paint_color_alpha(cr, 3 * w / 4, h / 2, w / 4, h / 2, 0., 0., 0., a);
+
+ igt_put_cairo_ctx(cr);
+}

That last igt_paint_color_alpha(..) will need to say:

igt_paint_color_alpha(cr, w * 3 / 4, h / 2, w - (w * 3 / 4), h / 2,
0., 0., 0., a);

Otherwise it can draw one pixel too narrow box.

/Juha-Pekka

> >
> > >
> > > Thanks,
> > > Tejas
> > >
> > > > -----Original Message-----
> > > > From: Sharma, Swati2 <swati2.sharma at intel.com>
> > > > Sent: 02 August 2021 18:35
> > > > To: juhapekka.heikkila at gmail.com; Surendrakumar Upadhyay,
> > > > TejaskumarX <tejaskumarx.surendrakumar.upadhyay at intel.com>; igt-
> > > > dev at lists.freedesktop.org
> > > > Subject: Re: [igt-dev] [i-g-t] tests/kms_plane_alpha_blend: Align
> > > > width to 256B
> > > >
> > > > Hi JP,
> > > >
> > > > Tejas reapplied the same fix as in
> > > > https://patchwork.freedesktop.org/patch/445010/?series=92751&rev=2
> > > > which was reviewed by Daniel.
> > > >
> > > > On 02-Aug-21 5:42 PM, Juha-Pekka Heikkila wrote:
> > > > > 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:
> > > > >> Reviewed-by:
> > > > >> Swati Sharma <swati2.sharma at intel.com>
> > > > >>
> > > > >> On 28-Jul-21 10:25 AM, Tejas Upadhyay wrote:
> > > > >>> 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.
> > > > >
> > > > >>>
> > > > >>> 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?
> > > > >
> > > > >>>
> > > > >>> 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
> > > > >>> <tejaskumarx.surendrakumar.upadhyay at intel.com>
> > > > >>> ---
> > > > >>>   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.
> > > > >
> > > > >>>       /* 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
> > > >
> > > > --
> > > > ~Swati Sharma