linux-kernel - Re: [PATCH 5/9] drm/vc4: hdmi: Rework the CSC matrices organization

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 26 Jan 2023 14:40:03 +0100
From:   Maxime Ripard <maxime@...no.tech>
To:     Dave Stevenson <dave.stevenson@...pberrypi.com>
Cc:     Thomas Zimmermann <tzimmermann@...e.de>,
        Emma Anholt <emma@...olt.net>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 5/9] drm/vc4: hdmi: Rework the CSC matrices organization

Hi,

On Wed, Jan 11, 2023 at 05:00:41PM +0000, Dave Stevenson wrote:
> Hi Thomas
> 
> Thanks for the review
> 
> On Wed, 11 Jan 2023 at 15:03, Thomas Zimmermann <tzimmermann@...e.de> wrote:
> >
> > Hi
> >
> > Am 07.12.22 um 17:07 schrieb Maxime Ripard:
> > > From: Dave Stevenson <dave.stevenson@...pberrypi.com>
> > >
> > > The CSC matrices were stored as separate matrix for each colorspace, and
> > > if we wanted a limited or full RGB output.
> > >
> > > This created some gaps in our support and we would not always pick the
> > > relevant matrix.
> > >
> > > Let's rework our data structure to store one per colorspace, and then a
> > > matrix for limited range and one for full range. This makes us add a new
> > > matrix to support full range BT709 YUV output, and drops the redundant
> > > (but somehow different) BT709 YUV444 vs YUV422 matrix.
> >
> > The final sentence is confusing and I didn't understand how two
> > different matrices can now be just one.
> 
> Two changes to accommodate the hardware requirements:
> 
> Firstly the driver was only defining
> vc5_hdmi_csc_full_rgb_to_limited_yuv444_bt709 and
> vc5_hdmi_csc_full_rgb_to_limited_yuv422_bt709. There was no matrix for
> full_rgb_to_full_yuv_bt709, so that had to be added.
> 
> Secondly, for some reason when in 444 mode the hardware wants the
> matrix rows in a different order. That is why
> vc5_hdmi_csc_full_rgb_to_limited_yuv444_bt709 differed from
> vc5_hdmi_csc_full_rgb_to_limited_yuv422_bt709 - it was a simple
> reordering of the rows.
> 
> This patch dropped the special handling for 444, and in the process
> programmed the wrong coefficients into the hardware :-(
> Patch 6/9 then reintroduces this reordering, so really should be
> squashed into the one patch.

Thanks to both of you for that feedback. I've chosen a slightly
different solution since I believe it still makes sens to have the swap
patch separate. I've move the swap function introduction earlier and
removed the two redundant matrices in that patch. And now, that patch
doesn't drop any matrix anymore so I've removed the confusing part of
the commit log.

> Looking at my more recent work, it looks like I messed up on 6/9 too.
> One of the patches on [1] corrects that row swapping for yuv444 - I
> was obviously having a bad day.
> 
> Maxime: Are you OK to fix that up?

I've squashed it in for the next revision

Thanks!
maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)