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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
