Message-ID: <1a8fd854-3943-1818-1a65-4d39830bc897@synopsys.com>
Date:   Mon, 16 Jan 2017 13:24:01 +0000
From:   Jose Abreu <Jose.Abreu@...opsys.com>
To:     Ville Syrjälä <ville.syrjala@...ux.intel.com>,
        "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

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).

> 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.

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?

Best regards,
Jose Miguel Abreu

Powered by blists - more mailing lists