linux-kernel - Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170111113602.GI31595@intel.com>
Date:   Wed, 11 Jan 2017 13:36:02 +0200
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     dri-devel@...ts.freedesktop.org,
        Carlos Palminha <CARLOS.PALMINHA@...opsys.com>,
        linux-kernel@...r.kernel.org,
        Daniel Vetter <daniel.vetter@...el.com>
Subject: Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB

On Wed, Jan 11, 2017 at 10:27:03AM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> On 10-01-2017 17:21, Ville Syrjälä wrote:
> 
> [snip]
> 
> >> But we already have color_formats field in drm_display_info
> >> struct, right? Shouldn't we instead create for example a helper
> >> which returns the best output colorspace? According to what you
> >> said it would be something like:
> >>
> >> if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
> >>     return YCBCR444;
> >> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
> >>     return YCBCR422;
> >> else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR420
> >> && vic_is_420)
> >>     return YCBCR420;
> >> else
> >>     return RGB444; /* Mandatory by spec */
> > Perhaps. But it would have to be more involved than that since there
> > might limitations on eg. the max TMDS clock imposed by the source or
> > cable/dongle which presumably might require that we pick 4:2:0 over
> > 4:4:4.
> >
> > It would also need to account which formats are actually supported by
> > the source.
> >
> > I guess for bpc it would be enough to just consider 8bpc in such a
> > function, and then the driver can bump it up afterwards if possible.
> 
> But the max tmds clock will probably be involved in deep color
> modes, as 24bpp has always a 1x factor in every YCbCr, except
> 4:2:0. So, the sink has a max tmds but this gets into account
> when the vic list present in the EDID is built, but not
> considered in deep color modes, unless the EDID is broken.
> 
> >
> > As for the RGB vs. YCbCr question, I guess we should just prefer RGB444
> > for now. And fall back to YCbCr 4:2:2 or 4:2:0 if necessary. And that 
> > leaves YCbCr 4:4:4 unsed since it has the same requirements as RGB
> > 4:4:4 and thus doesn't provide any benefit as such. We could later add
> > a property to let the user choose between RGB vs. YCbCr more explicitly.
> >
> 
> Hmm, I am trying to implement this but I am facing a difficulty:
> how will I fallback to YCbCr? RGB is always supported and the max
> tmds only enters in deep color modes. For reference here is a
> simple struct i created with the different tmds character rate
> factors for the different encodings (I think they are correct,
> but cross check please):
> 
> #define DRM_CS_DESC(cs, f, b) \
>     .colorspace = (cs), .factor_to_khz = (f), .bpc = (b)
> 
> static const struct drm_mode_colorspace_desc {
>     u32 colorspace;
>     u32 factor_to_khz;
>     u32 bpc;
> } drm_mode_colorspace_factors = { /* Ordered by descending
> preference */
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 2000, 48) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 2000, 48) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1500, 36) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1500, 36) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1250, 30) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1250, 30) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_RGB444, 1000, 24) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB444, 1000, 24) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 24) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 30) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB422, 1000, 36) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 1000, 48) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 750, 36) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 625, 30) },
>     { DRM_CS_DESC(DRM_COLOR_FORMAT_YCRCB420, 500, 24) },
> 
> Notice how YCbCr 4:4:4 will never get picked: it has the same
> factor of RGB 4:4:4 for every bpc. So, the sink must support RGB
> 4:4:4 and may support YCbCr. What I didn't check was that if it
> is possible to have support for deep color YCbCr without having
> support for deep color RGB 4:4:4. Do you know if this can happen?

I don't think that's possible. So the 4:4:4 RGB vs. YCbCr choice is
probably something we have to leave up to the user. Although I have
a vague recollection that CEA-861 says that you should prefer YCbCr
for CE modes and RGB for IT modes. If we want to follow that I think we
want a property similar to the "Broadcast RGB" thing that allows you to
select between "Automatic", "RGB", and "YCbCr". Not sure if we should
also allow the user to explicitly select the subsampling mode for YCbCr.
I also think we should probably still fall back to YCbCr 4:2:0
automagically even if the user explicitly asked for RGB if we can't
light up the mode with RGB 4:4:4.

-- 
Ville Syrjälä
Intel OTC