>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >Sent: Friday, February 8, 2019 9:06 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >Cc: Sharma, Shashank <shashank.sharma@xxxxxxxxx>; intel- >gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville <ville.syrjala@xxxxxxxxx>; dri- >devel@xxxxxxxxxxxxxxxxxxxxx; Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx> >Subject: Re: [Intel-gfx] [v11 1/4] drm: Add HDMI colorspace property > >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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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. Ok, so then I will have all the enum values with explicit information whether its RGB or planar formats. Currently I feel only BT709 and 170M are the ones with ambiguity. Hope we are aligned on this .. Regards, Uma Shankar -- >Ville Syrjälä >Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
