On Fri, Feb 08, 2019 at 03:03:34PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > >Sent: Friday, February 8, 2019 8:08 PM > >To: Sharma, Shashank <shashank.sharma@xxxxxxxxx> > >Cc: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; > >Syrjala, Ville <ville.syrjala@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Lankhorst, > >Maarten <maarten.lankhorst@xxxxxxxxx> > >Subject: Re: [v11 1/4] drm: Add HDMI colorspace property > > > >On Fri, Feb 08, 2019 at 07:36:24PM +0530, Sharma, Shashank wrote: > >> Regards > >> > >> Shashank > >> > >> On 2/8/2019 7:00 PM, Ville Syrjälä wrote: > >> > On Fri, Feb 08, 2019 at 06:36:39PM +0530, Sharma, Shashank wrote: > >> >> Regards > >> >> > >> >> Shashank > >> >> > >> >> On 2/8/2019 6:22 PM, Ville Syrjälä wrote: > >> >>> On Fri, Feb 08, 2019 at 12:36:25PM +0000, Sharma, Shashank wrote: > >> >>>> Regards > >> >>>> Shashank > >> >>>> > >> >>>>> -----Original Message----- > >> >>>>> From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] > >> >>>>> On Behalf Of Shankar, Uma > >> >>>>> Sent: Friday, February 8, 2019 5:45 PM > >> >>>>> To: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >> >>>>> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville > >> >>>>> <ville.syrjala@xxxxxxxxx>; dri- devel@xxxxxxxxxxxxxxxxxxxxx; > >> >>>>> Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx> > >> >>>>> Subject: Re: [v11 1/4] drm: Add HDMI colorspace > >> >>>>> property > >> >>>>> > >> >>>>>>>>>> -----Original Message----- > >> >>>>>>>>>> From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > >> >>>>>>>>>> Sent: Tuesday, February 5, 2019 10:02 PM > >> >>>>>>>>>> To: Shankar, Uma <uma.shankar@xxxxxxxxx> > >> >>>>>>>>>> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; > >> >>>>>>>>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville > >> >>>>>>>>>> <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten > >> >>>>>>>>>> <maarten.lankhorst@xxxxxxxxx> > >> >>>>>>>>>> Subject: Re: [v11 1/4] drm: Add HDMI colorspace > >> >>>>>>>>>> property > >> >>>>>>>>>> > >> >>>>>>>>>> On Tue, Feb 05, 2019 at 09:33:34PM +0530, Uma Shankar wrote: > >> >>>>>>>>>>> Create a new connector property to program colorspace to sink > >devices. > >> >>>>>>>>>>> Modern sink devices support more than 1 type of colorspace > >> >>>>>>>>>>> like 601, 709, BT2020 etc. This helps to switch based on > >> >>>>>>>>>>> content type which is to be displayed. The decision lies > >> >>>>>>>>>>> with compositors as to in which scenarios, a particular colorspace will > >be picked. > >> >>>>>>>>>>> > >> >>>>>>>>>>> This will be helpful mostly to switch to higher gamut > >> >>>>>>>>>>> colorspaces like > >> >>>>>>>>>>> BT2020 when the media content is encoded as BT2020. > >> >>>>>>>>>>> Thereby giving a good visual experience to users. > >> >>>>>>>>>>> > >> >>>>>>>>>>> The expectation from userspace is that it should parse the > >> >>>>>>>>>>> EDID and get supported colorspaces. Use this property and > >> >>>>>>>>>>> switch to the one supported. Sink supported colorspaces > >> >>>>>>>>>>> should be retrieved by userspace from EDID and driver will > >> >>>>>>>>>>> not explicitly expose > >> >>>>>> them. > >> >>>>>>>>>>> Basically the expectation from userspace is: > >> >>>>>>>>>>> - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some sink > >> >>>>>>>>>>> colorspace > >> >>>>>>>>>>> - Set this new property to let the sink know what it > >> >>>>>>>>>>> converted the CRTC output to. > >> >>>>>>>>>>> > >> >>>>>>>>>>> v2: Addressed Maarten and Ville's review comments. > >> >>>>>>>>>>> Enhanced the colorspace enum to incorporate both HDMI and > >> >>>>>>>>>>> DP supported > >> >>>>>> colorspaces. > >> >>>>>>>>>>> Also, added a default option for colorspace. > >> >>>>>>>>>>> > >> >>>>>>>>>>> v3: Removed Adobe references from enum definitions as per > >> >>>>>>>>>>> Ville, Hans Verkuil and Jonas Karlman suggestions. Changed > >> >>>>>>>>>>> Default to an unset state where driver will assign the > >> >>>>>>>>>>> colorspace is not chosen by user, suggested by Ville and > >> >>>>>>>>>>> Maarten. Addressed other misc review comments from Maarten. > >> >>>>>>>>>>> Split the changes to have separate colorspace property for DP and > >HDMI. > >> >>>>>>>>>>> > >> >>>>>>>>>>> v4: Addressed Chris and Ville's review comments, and > >> >>>>>>>>>>> created a common colorspace property for DP and HDMI, > >> >>>>>>>>>>> filtered the list based on the colorspaces supported by the respective > >protocol standard. > >> >>>>>>>>>>> > >> >>>>>>>>>>> v5: Made the property creation helper accept enum list > >> >>>>>>>>>>> based on platform capabilties as suggested by Shashank. > >> >>>>>>>>>>> Consolidated HDMI and DP property creation in the common helper. > >> >>>>>>>>>>> > >> >>>>>>>>>>> v6: Addressed Shashank's review comments. > >> >>>>>>>>>>> > >> >>>>>>>>>>> v7: Added defines instead of enum in uapi as per Brian > >> >>>>>>>>>>> Starkey's suggestion in order to go with string matching at userspace. > >> >>>>>>>>>>> Updated the commit message to add more details as well kernel docs. > >> >>>>>>>>>>> > >> >>>>>>>>>>> v8: Addressed Maarten's review comments. > >> >>>>>>>>>>> > >> >>>>>>>>>>> v9: Removed macro defines from uapi as per Brian Starkey > >> >>>>>>>>>>> and Daniel Stone's comments and moved to drm include file. > >> >>>>>>>>>>> Moved back to older design with exposing all HDMI > >> >>>>>>>>>>> colorspaces to userspace since infoframe capability is > >> >>>>>>>>>>> there even on legacy platforms, as per Ville's review comments. > >> >>>>>>>>>>> > >> >>>>>>>>>>> v10: Fixed sparse warnings, updated the RB from Maarten and Jani's > >ack. > >> >>>>>>>>>>> > >> >>>>>>>>>>> v11: Addressed Ville's review comments. Updated the Macro > >> >>>>>>>>>>> naming and added DCI-P3 colorspace as well defined in CEA 861.G > >spec. > >> >>>>>>>>>>> > >> >>>>>>>>>>> Signed-off-by: Uma Shankar <uma.shankar@xxxxxxxxx> > >> >>>>>>>>>>> Acked-by: Jani Nikula <jani.nikula@xxxxxxxxx> > >> >>>>>>>>>>> Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> > >> >>>>>>>>>>> Reviewed-by: Maarten Lankhorst > >> >>>>>>>>>>> <maarten.lankhorst@xxxxxxxxxxxxxxx> > >> >>>>>>>>>>> --- > >> >>>>>>>>>>> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ > >> >>>>>>>>>>> drivers/gpu/drm/drm_connector.c | 78 > >> >>>>>>>>>> +++++++++++++++++++++++++++++++++++++++ > >> >>>>>>>>>>> include/drm/drm_connector.h | 50 > >+++++++++++++++++++++++++ > >> >>>>>>>>>>> 3 files changed, 132 insertions(+) > >> >>>>>>>>>>> > >> >>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c > >> >>>>>>>>>>> b/drivers/gpu/drm/drm_atomic_uapi.c > >> >>>>>>>>>>> index 9a1f41a..9b5d44f 100644 > >> >>>>>>>>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c > >> >>>>>>>>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c > >> >>>>>>>>>>> @@ -746,6 +746,8 @@ static int > >> >>>>>>>>>>> drm_atomic_connector_set_property(struct > >> >>>>>>>>>> drm_connector *connector, > >> >>>>>>>>>>> return -EINVAL; > >> >>>>>>>>>>> } > >> >>>>>>>>>>> state->content_protection = val; > >> >>>>>>>>>>> + } else if (property == connector->colorspace_property) { > >> >>>>>>>>>>> + state->colorspace = val; > >> >>>>>>>>>>> } else if (property == config->writeback_fb_id_property) { > >> >>>>>>>>>>> struct drm_framebuffer *fb = > >> >>>>>> drm_framebuffer_lookup(dev, > >> >>>>>>>>>> NULL, val); > >> >>>>>>>>>>> int ret = > >> >>>>>> drm_atomic_set_writeback_fb_for_connector(state, > >> >>>>>>>>>> fb); @@ > >> >>>>>>>>>>> -814,6 +816,8 @@ static int > >> >>>>>>>>>>> drm_atomic_connector_set_property(struct > >> >>>>>>>>>> drm_connector *connector, > >> >>>>>>>>>>> *val = state->picture_aspect_ratio; > >> >>>>>>>>>>> } else if (property == config->content_type_property) { > >> >>>>>>>>>>> *val = state->content_type; > >> >>>>>>>>>>> + } else if (property == connector->colorspace_property) { > >> >>>>>>>>>>> + *val = state->colorspace; > >> >>>>>>>>>>> } else if (property == connector->scaling_mode_property) { > >> >>>>>>>>>>> *val = state->scaling_mode; > >> >>>>>>>>>>> } else if (property == > >> >>>>>>>>>>> connector->content_protection_property) > >> >>>>>>>>>>> { diff --git a/drivers/gpu/drm/drm_connector.c > >> >>>>>>>>>>> b/drivers/gpu/drm/drm_connector.c index 8475396..4309873 > >> >>>>>>>>>>> 100644 > >> >>>>>>>>>>> --- a/drivers/gpu/drm/drm_connector.c > >> >>>>>>>>>>> +++ b/drivers/gpu/drm/drm_connector.c > >> >>>>>>>>>>> @@ -826,6 +826,33 @@ int > >> >>>>>>>>>>> drm_display_info_set_bus_formats(struct > >> >>>>>>>>>>> drm_display_info *info, }; > >> >>>>>>>>>>> DRM_ENUM_NAME_FN(drm_get_content_protection_name, > >> >>>>>>>>>> drm_cp_enum_list) > >> >>>>>>>>>>> +static const struct drm_prop_enum_list hdmi_colorspaces[] = { > >> >>>>>>>>>>> + /* For Default case, driver will set the colorspace */ > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_DEFAULT, "Default" }, > >> >>>>>>>>>>> + /* Standard Definition Colorimetry based on CEA 861 */ > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_SMPTE_170M, > >"SMPTE_170M" }, > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_BT709, "BT709" }, > >> >>>>>>>>>>> + /* Standard Definition Colorimetry based on IEC 61966-2-4 > >*/ > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" }, > >> >>>>>>>>>>> + /* High Definition Colorimetry based on IEC 61966-2-4 */ > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" }, > >> >>>>>>>>>>> + /* Colorimetry based on IEC 61966-2-1/Amendment 1 */ > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" }, > >> >>>>>>>>>>> + /* Colorimetry based on IEC 61966-2-5 [33] */ > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" }, > >> >>>>>>>>>>> + /* Colorimetry based on IEC 61966-2-5 */ > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" }, > >> >>>>>>>>>>> + /* Colorimetry based on ITU-R BT.2020 */ > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" > >}, > >> >>>>>>>>>>> + /* Colorimetry based on ITU-R BT.2020 */ > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" > >}, > >> >>>>>>>>>>> + /* Colorimetry based on ITU-R BT.2020 */ > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_BT2020_CYCC, > >"BT2020_CYCC" }, > >> >>>>>>>>>>> + /* Added as part of Additional Colorimetry Extension in > >861.G */ > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI- > >> >>>>>> P3_RGB_D65" }, > >> >>>>>>>>>>> + { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI- > >> >>>>>>>>>> P3_RGB_Theater" }, > >> >>>>>>>>>>> +}; > >> >>>>>>>>>>> + > >> >>>>>>>>>>> /** > >> >>>>>>>>>>> * DOC: standard connector properties > >> >>>>>>>>>>> * > >> >>>>>>>>>>> @@ -1548,6 +1575,57 @@ int > >> >>>>>>>>>>> drm_mode_create_aspect_ratio_property(struct drm_device > >> >>>>>>>>>>> *dev) > >> >>>>>>>>>>> EXPORT_SYMBOL(drm_mode_create_aspect_ratio_property); > >> >>>>>>>>>>> > >> >>>>>>>>>>> /** > >> >>>>>>>>>>> + * DOC: standard connector properties > >> >>>>>>>>>>> + * > >> >>>>>>>>>>> + * Colorspace: > >> >>>>>>>>>>> + * drm_mode_create_colorspace_property - create colorspace > >> >>>>>> property > >> >>>>>>>>>>> + * This property helps select a suitable colorspace based on the > >sink > >> >>>>>>>>>>> + * capability. Modern sink devices support wider gamut like > >BT2020. > >> >>>>>>>>>>> + * This helps switch to BT2020 mode if the BT2020 encoded video > >> >>>>>> stream > >> >>>>>>>>>>> + * is being played by the user, same for any other colorspace. > >Thereby > >> >>>>>>>>>>> + * giving a good visual experience to users. > >> >>>>>>>>>>> + * > >> >>>>>>>>>>> + * The expectation from userspace is that it should parse the EDID > >> >>>>>>>>>>> + * and get supported colorspaces. Use this property and switch to > >the > >> >>>>>>>>>>> + * one supported. Sink supported colorspaces should be retrieved > >by > >> >>>>>>>>>>> + * userspace from EDID and driver will not explicitly expose them. > >> >>>>>>>>>>> + * > >> >>>>>>>>>>> + * Basically the expectation from userspace is: > >> >>>>>>>>>>> + * - Set up CRTC DEGAMMA/CTM/GAMMA to convert to some > >sink > >> >>>>>>>>>>> + * colorspace > >> >>>>>>>>>>> + * - Set this new property to let the sink know what it > >> >>>>>>>>>>> + * converted the CRTC output to. > >> >>>>>>>>>>> + * - This property is just to inform sink what colorspace > >> >>>>>>>>>>> + * source is trying to drive. > >> >>>>>>>>>>> + * > >> >>>>>>>>>>> + * Called by a driver the first time it's needed, must be > >> >>>>>>>>>>> +attached to desired > >> >>>>>>>>>>> + * connectors. > >> >>>>>>>>>>> + */ > >> >>>>>>>>>>> +int drm_mode_create_colorspace_property(struct > >> >>>>>>>>>>> +drm_connector > >> >>>>>>>>>>> +*connector) { > >> >>>>>>>>>>> + struct drm_device *dev = connector->dev; > >> >>>>>>>>>>> + struct drm_property *prop; > >> >>>>>>>>>>> + > >> >>>>>>>>>>> + if (connector->connector_type == > >> >>>>>> DRM_MODE_CONNECTOR_HDMIA || > >> >>>>>>>>>>> + connector->connector_type == > >> >>>>>> DRM_MODE_CONNECTOR_HDMIB) { > >> >>>>>>>>>>> + prop = drm_property_create_enum(dev, > >> >>>>>>>>>> DRM_MODE_PROP_ENUM, > >> >>>>>>>>>>> + "Colorspace", > >> >>>>>>>>>>> + hdmi_colorspaces, > >> >>>>>>>>>>> + > >> >>>>>>>>>> ARRAY_SIZE(hdmi_colorspaces)); > >> >>>>>>>>>>> + if (!prop) > >> >>>>>>>>>>> + return -ENOMEM; > >> >>>>>>>>>>> + } else { > >> >>>>>>>>>>> + DRM_DEBUG_KMS("Colorspace property not > >> >>>>>> supported\n"); > >> >>>>>>>>>>> + return 0; > >> >>>>>>>>>>> + } > >> >>>>>>>>>>> + > >> >>>>>>>>>>> + connector->colorspace_property = prop; > >> >>>>>>>>>>> + > >> >>>>>>>>>>> + return 0; > >> >>>>>>>>>>> +} > >> >>>>>>>>>>> +EXPORT_SYMBOL(drm_mode_create_colorspace_property); > >> >>>>>>>>>>> + > >> >>>>>>>>>>> +/** > >> >>>>>>>>>>> * drm_mode_create_content_type_property - create > >> >>>>>>>>>>> content type > >> >>>>>> property > >> >>>>>>>>>>> * @dev: DRM device > >> >>>>>>>>>>> * > >> >>>>>>>>>>> diff --git a/include/drm/drm_connector.h > >> >>>>>>>>>>> b/include/drm/drm_connector.h index 9941613..58db66e > >> >>>>>>>>>>> 100644 > >> >>>>>>>>>>> --- a/include/drm/drm_connector.h > >> >>>>>>>>>>> +++ b/include/drm/drm_connector.h > >> >>>>>>>>>>> @@ -253,6 +253,42 @@ enum drm_panel_orientation { > >> >>>>>>>>>>> DRM_MODE_PANEL_ORIENTATION_RIGHT_UP, > >> >>>>>>>>>>> }; > >> >>>>>>>>>>> > >> >>>>>>>>>>> +/* > >> >>>>>>>>>>> + * This is a consolidated colorimetry list supported by > >> >>>>>>>>>>> +HDMI and > >> >>>>>>>>>>> + * DP protocol standard. The respective connectors will > >> >>>>>>>>>>> +register > >> >>>>>>>>>>> + * a property with the subset of this list (supported by > >> >>>>>>>>>>> +that > >> >>>>>>>>>>> + * respective protocol). Userspace will set the > >> >>>>>>>>>>> +colorspace through > >> >>>>>>>>>>> + * a colorspace property which will be created and > >> >>>>>>>>>>> +exposed to > >> >>>>>>>>>>> + * userspace. > >> >>>>>>>>>>> + */ > >> >>>>>>>>>>> + > >> >>>>>>>>>>> +/* For Default case, driver will set the colorspace */ > >> >>>>>>>>>>> +#define DRM_MODE_COLORIMETRY_DEFAULT 0 > >> >>>>>>>>>>> +/* CEA 861 Normal Colorimetry options */ > >> >>>>>>>>>>> +#define DRM_MODE_COLORIMETRY_NO_DATA 0 > >> >>>>>>>>>>> +#define DRM_MODE_COLORIMETRY_SMPTE_170M > >> >>>>>>>> 1 > >> >>>>>>>>>>> +#define DRM_MODE_COLORIMETRY_BT709 2 > >> >>>>>>>>>> Still missing the YCbCr information in these two. > >> >>>>>>>>> As per CTA 861.G spec, we have these as only SMPTE_170M and > >> >>>>>>>>> ITU-R BT709, with no specific mention of RGB or YCbCr. > >> >>>>>>>>> Hence, kept it as per spec. We seem to have a common field for both as > >per CTA spec. > >> >>>>>>>>> Correct me if my understanding is wrong. > >> >>>>>>>> Check > >> >>>>>>>> "Table 14 Picture Colorimetry Indicated by the RGB or YC B C > >> >>>>>>>> R (Y), Colorimetry > >> >>>>>>>> (C) and Extended Colorimetry (EC) Field Settings" > >> >>>>>>> These Y2 Y1 Y0 values should be programmed separately and not > >> >>>>>>> through the colorspace as they are data formats isn't it. I > >> >>>>>>> feel this should get controlled separately independent of > >> >>>>>>> colorimetry, or should we add all the YCbCr and RGB versions > >> >>>>>>> and program them as part of this property itself > >> >>>>>> ? > >> >>>>>> > >> >>>>>> I don't think we can separate them. The values of the > >> >>>>>> colorimetry bits doesn't mean anything without the Y bits. It > >> >>>>>> would make the uapi kinda crazy if we allow the user to specify > >> >>>>>> BT.2020 RGB but then we actually signal BT.2020 YCbCr in the > >> >>>>>> infoframe, or vice versa. Or we just signal a totally invalid value for all the > >other cases. > >> >>>>> I agree we need data format also to be send along with > >> >>>>> colorspace, but they are 2 different things. To handle YCbCr and > >> >>>>> variants (YUV 444, 420 or 422) and RGB I feel we should expose a > >> >>>>> different API instead of using this one. We can create an output csc property > >which does that job for us. > >> >>>>> > >> >>>>> So from a user perspective if we wants to set a colorspace we > >> >>>>> should use the one in this series and set the data format > >> >>>>> separately using the other interface. Both will be received in the state > >variables and later Infoframe packet will be created accordingly. > >> >>>>> Clubbing both in one may lead to lot of possible combinations > >> >>>>> exposed as enum which may not look too clean. > >> >>>>> > >> >>>>> What do you say about handling that in the output csc property. > >> >>>>> I will go with what you recommend . > >> >>>> Programming Y2Y1Y0 is already taken care of, when we added support for > >YCBCR outputs (4:2:0 implementation). > >> >>>> In intel_hdmi.c:intel_hdmi_set_avi_infoframe(): > >> >>>> > >> >>>> if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420) > >> >>>> frame.avi.colorspace = HDMI_COLORSPACE_YUV420; else if > >> >>>> (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) > >> >>>> frame.avi.colorspace = HDMI_COLORSPACE_YUV444; else > >> >>>> frame.avi.colorspace = HDMI_COLORSPACE_RGB; > >> >>>> > >> >>>> IMO This looks like a better way to handle this, ie while adding support for a > >new format, add support for corresponding AVI IF fileds. This will also make scope of > >the property under discussion less complicated. > >> >>> That's an exceptional case. We're programming the CSC in that case > >> >>> to do the RGB->YCbCr conversion without telling userspace. So we > >> >>> must also configure the Y bits automagically. > >> >>> > >> >>> What we want is a check like so: > >> >>> > >> >>> if (prop.colorspace != default && output_format != RGB) > >> >>> return -INVAL; > >> >>> > >> >>> because we can't set up the CSC to make a mess of the user's > >> >>> carefully crafted pixels. The pixels might not even contain RGB > >> >>> data in that case. > >> >>> > >> >>> We'll need to extend that a little bit once we add the explicit > >> >>> control of the output CSC via another prop. But the same principle > >> >>> should hold. > >> >> In fact, I remember the reason why we created infrastructure like > >> >> this, so that, going forward, we could just add a output_format > >> >> property, for which, we already have the backed and the state > >> >> variables ready. The consume of this backend can be this > >> >> drm_property (like output_format), or internal modeset like > >> >> YCBCR4:2:0 only modes in EDID. It will also be inline with your suggestion of CSC > >property. > >> >> > >> >> So we can have two properties colorspace (BT2020/SRGB/REC709) and > >> >> color_model/color_format(RGB/YCBCR444/420), and a combination of > >> >> both should program the output AVI info-frames. Does it appeal you ? > >> > I'm not sure it's feasible to split it like that. The combinations > >> > supported by the infoframe are rather specific so trying to split it > >> > up leads to either super limited uapi, or one that exposes a lot of > >> > illegal combinations. Also DP has slightly different set of things > >> > it supports which adds more complications. > >> > > >> > So I think the best option is still to include the Y=yes/no > >> > information in the prop value alognside the C/EC/ACE bits. > >> > >> Yes, you are right, this could be a simpler way of doing things, but > >> with a limited set of combination. > >> > >> Consider this example: > >> > >> - If a HDMI monitor supports BT2020, but doesn't support YCBCR > >> formatting, how to handle this option ? This means we have to > >> selectively show BT2020_RGB enum value only not BT2020_YCBCR_444/420, > >> > >> - This also means we have to probe the EDID first, and then create > >> this property, which means we will be dependent on the hot-plug / > >> detect to create this property, instead of doing this in hdmi_init. > > > >IMO we just ignore this problem and expose all the legal options the spec has. > > > >If userspace wants to know what's actually supported it will have to parse the EDID. > >We could think about parsing that in the kernel too and exposing an immutable prop > >to expose that infromation. But I'm not sure this is such a good idea since we'd > >basically drown in properties if we try to expose all the information from the EDID. A > >better long term plan would be a common EDID parsing library for everyone. > > > >> > >> Even more complex scenario would be when the output support depends on > >> monitor + platform both. This means we have to provide this > >> combination upfront while creating this property. > >> > >> On the other hand, the invalid combination of two property, it would > >> be easy to detect at runtime at atomic_check() and fail the commit. > >> > >> > >> This is just a thought, honestly I am also not sure what would be the > >> most appropriate solution for this, but seems its simpler to create > >> property with two properties, might be difficult to manage at runtime. > >> > >> With one property, its very difficult to create, but once created, it > >> would work sure shot. > > > >I don't think it's that difficult. Just expose all the things in the > >CTA-861 big table. Which means including the Y bit too. > > Hi Ville, > Ok So will try to do this way then: > > Have a separate enum option like > DRM_MODE_COLORIMETRY_BT709_YUV444 > DRM_MODE_COLORIMETRY_BT709_YUV420 > DRM_MODE_COLORIMETRY_BT709_YUV422 Hmm. I'm not quite convinced that we want the subsampling information included here. That will make it difficult to do the output csc property in a way that leaves the driver free to select the subsampling automagically, which we proably still want because of those annoying HDMI 2.0 monitors. -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
