Message-ID: <20170116135754.GM31595@intel.com>
Date:   Mon, 16 Jan 2017 15:57:54 +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 Mon, Jan 16, 2017 at 01:24:01PM +0000, Jose Abreu wrote:
> Hi Ville,
> 
> 
> Sorry for the late reply.
> 
> 
> On 11-01-2017 11:36, Ville Syrjälä wrote:
> > 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. 
> 
> RGB Full Range is the default for IT modes. As for CE modes it
> says it depends on vactive, which I am not quite understanding
> why (pg. 34, CEA-861-F).

I think that vactive note is just referring to the SD->BT.601 and
HD->BT.709 rule.

> 
> > 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 think we can start by only RGB vs. YCbCr vs. automatic.
> 
> > 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.
> >
> 
> Agreed. I will start by that but in order for this to work in a
> real scenario (I got it working by changing EDID manually) the
> list of VIC's must be expanded to the new HDMI 2.0 VIC's. A patch
> was sent here a while ago (not by me) and I think you commented
> on that. I don't know whats the status of the patch now but it
> wasn't merged.

The new VICs were reverted due to the "exposing aspect ratio as mode
flags broke userspace" thing. What we need to do is add the new VICs
without changing the userspace API. And I don't see any reason why
we can't do just that. But no such patch has materialized AFAIK.

> 
> Anyway, regarding this I think we could:
>     - Reuse this existing RFC where it concerns EDID parsing
>     - Add new flags (which will not be exported to userspace) to
> signal YCbCr 4:2:0'only and 'able modes
>     - Add a helper function to compute best colorspace which will
> always return RGB (for now) unless the mode is 4:2:0 only or
> unless the mode can't use RGB because of max clock limitations.
> 
> What do you think?

Sounds reasonable to me.

-- 
Ville Syrjälä
Intel OTC

Powered by blists - more mailing lists