From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751708AbdAPNYL (ORCPT ); Mon, 16 Jan 2017 08:24:11 -0500 Received: from smtprelay4.synopsys.com ([198.182.47.9]:53164 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751667AbdAPNYH (ORCPT ); Mon, 16 Jan 2017 08:24:07 -0500 Subject: Re: [RFC] drm: Parse HDMI 2.0 YCbCr 4:2:0 VDB and VCB To: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , "Jose Abreu" References: <9df9f649-d3e9-7fd6-3dda-1d47799f50f7@synopsys.com> <20170105114640.GO31595@intel.com> <2700086a-00b9-e8c7-f502-df132fbbe2dc@synopsys.com> <20170110111601.GY31595@intel.com> <4233e877-8ca6-b3d4-e986-a2bf3e17f1e1@synopsys.com> <20170110153315.GC31595@intel.com> <20170110155244.GD31595@intel.com> <20170110172141.GG31595@intel.com> <20170111113602.GI31595@intel.com> CC: , Carlos Palminha , , Daniel Vetter From: Jose Abreu Message-ID: <1a8fd854-3943-1818-1a65-4d39830bc897@synopsys.com> Date: Mon, 16 Jan 2017 13:24:01 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170111113602.GI31595@intel.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.107.19.90] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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