This commit implements the "Broadcast RGB" drm property for the AMD GPU
driver.
Signed-off-by: Werner Sembach <[email protected]>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 22 ++++++++++++++-----
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++++
2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 9ffd2f9d3d75..c5dbf948a47a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5252,7 +5252,8 @@ get_aspect_ratio(const struct drm_display_mode *mode_in)
}
static enum dc_color_space
-get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
+get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
+ enum drm_mode_color_range preferred_color_range)
{
enum dc_color_space color_space = COLOR_SPACE_SRGB;
@@ -5267,13 +5268,17 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
* respectively
*/
if (dc_crtc_timing->pix_clk_100hz > 270300) {
- if (dc_crtc_timing->flags.Y_ONLY)
+ if (dc_crtc_timing->flags.Y_ONLY
+ || preferred_color_range ==
+ DRM_MODE_COLOR_RANGE_LIMITED_16_235)
color_space =
COLOR_SPACE_YCBCR709_LIMITED;
else
color_space = COLOR_SPACE_YCBCR709;
} else {
- if (dc_crtc_timing->flags.Y_ONLY)
+ if (dc_crtc_timing->flags.Y_ONLY
+ || preferred_color_range ==
+ DRM_MODE_COLOR_RANGE_LIMITED_16_235)
color_space =
COLOR_SPACE_YCBCR601_LIMITED;
else
@@ -5283,7 +5288,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
}
break;
case PIXEL_ENCODING_RGB:
- color_space = COLOR_SPACE_SRGB;
+ if (preferred_color_range == DRM_MODE_COLOR_RANGE_LIMITED_16_235)
+ color_space = COLOR_SPACE_SRGB_LIMITED;
+ else
+ color_space = COLOR_SPACE_SRGB;
break;
default:
@@ -5429,7 +5437,10 @@ static void fill_stream_properties_from_drm_display_mode(
timing_out->aspect_ratio = get_aspect_ratio(mode_in);
- stream->output_color_space = get_output_color_space(timing_out);
+ stream->output_color_space = get_output_color_space(timing_out,
+ connector_state ?
+ connector_state->preferred_color_range :
+ DRM_MODE_COLOR_RANGE_UNSET);
stream->out_transfer_func->type = TF_TYPE_PREDEFINED;
stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;
@@ -7780,6 +7791,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
drm_connector_attach_preferred_color_format_property(&aconnector->base);
drm_connector_attach_active_color_format_property(&aconnector->base);
+ drm_connector_attach_preferred_color_range_property(&aconnector->base);
drm_connector_attach_active_color_range_property(&aconnector->base);
}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 2563788ba95a..80e1389fd0ec 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -421,6 +421,10 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
if (connector->active_color_format_property)
drm_connector_attach_active_color_format_property(&aconnector->base);
+ connector->preferred_color_range_property = master->base.preferred_color_range_property;
+ if (connector->preferred_color_range_property)
+ drm_connector_attach_preferred_color_range_property(&aconnector->base);
+
connector->active_color_range_property = master->base.active_color_range_property;
if (connector->active_color_range_property)
drm_connector_attach_active_color_range_property(&aconnector->base);
--
2.25.1
On Fri, 18 Jun 2021 11:11:16 +0200
Werner Sembach <[email protected]> wrote:
> This commit implements the "Broadcast RGB" drm property for the AMD GPU
> driver.
>
> Signed-off-by: Werner Sembach <[email protected]>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 22 ++++++++++++++-----
> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++++
> 2 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 9ffd2f9d3d75..c5dbf948a47a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5252,7 +5252,8 @@ get_aspect_ratio(const struct drm_display_mode *mode_in)
> }
>
> static enum dc_color_space
> -get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
> +get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
> + enum drm_mode_color_range preferred_color_range)
> {
> enum dc_color_space color_space = COLOR_SPACE_SRGB;
>
> @@ -5267,13 +5268,17 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
> * respectively
> */
> if (dc_crtc_timing->pix_clk_100hz > 270300) {
> - if (dc_crtc_timing->flags.Y_ONLY)
> + if (dc_crtc_timing->flags.Y_ONLY
> + || preferred_color_range ==
> + DRM_MODE_COLOR_RANGE_LIMITED_16_235)
> color_space =
> COLOR_SPACE_YCBCR709_LIMITED;
> else
> color_space = COLOR_SPACE_YCBCR709;
Hi,
does this mean that amdgpu would be using a property named "Broadcast
RGB" to control the range of YCbCr too?
That is surprising. If this is truly wanted, then the documentation of
"Broadcast RGB" must say that it applies to YCbCr too.
Does amdgpu do the same as intel wrt. to the question about whose
responsibility it is to make the pixels at the connector to match the
set range?
Thanks,
pq
> } else {
> - if (dc_crtc_timing->flags.Y_ONLY)
> + if (dc_crtc_timing->flags.Y_ONLY
> + || preferred_color_range ==
> + DRM_MODE_COLOR_RANGE_LIMITED_16_235)
> color_space =
> COLOR_SPACE_YCBCR601_LIMITED;
> else
> @@ -5283,7 +5288,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
> }
> break;
> case PIXEL_ENCODING_RGB:
> - color_space = COLOR_SPACE_SRGB;
> + if (preferred_color_range == DRM_MODE_COLOR_RANGE_LIMITED_16_235)
> + color_space = COLOR_SPACE_SRGB_LIMITED;
> + else
> + color_space = COLOR_SPACE_SRGB;
> break;
>
> default:
> @@ -5429,7 +5437,10 @@ static void fill_stream_properties_from_drm_display_mode(
>
> timing_out->aspect_ratio = get_aspect_ratio(mode_in);
>
> - stream->output_color_space = get_output_color_space(timing_out);
> + stream->output_color_space = get_output_color_space(timing_out,
> + connector_state ?
> + connector_state->preferred_color_range :
> + DRM_MODE_COLOR_RANGE_UNSET);
>
> stream->out_transfer_func->type = TF_TYPE_PREDEFINED;
> stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;
> @@ -7780,6 +7791,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
> drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
> drm_connector_attach_preferred_color_format_property(&aconnector->base);
> drm_connector_attach_active_color_format_property(&aconnector->base);
> + drm_connector_attach_preferred_color_range_property(&aconnector->base);
> drm_connector_attach_active_color_range_property(&aconnector->base);
> }
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 2563788ba95a..80e1389fd0ec 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -421,6 +421,10 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
> if (connector->active_color_format_property)
> drm_connector_attach_active_color_format_property(&aconnector->base);
>
> + connector->preferred_color_range_property = master->base.preferred_color_range_property;
> + if (connector->preferred_color_range_property)
> + drm_connector_attach_preferred_color_range_property(&aconnector->base);
> +
> connector->active_color_range_property = master->base.active_color_range_property;
> if (connector->active_color_range_property)
> drm_connector_attach_active_color_range_property(&aconnector->base);
Am 22.06.21 um 09:29 schrieb Pekka Paalanen:
> On Fri, 18 Jun 2021 11:11:16 +0200
> Werner Sembach <[email protected]> wrote:
>
>> This commit implements the "Broadcast RGB" drm property for the AMD GPU
>> driver.
>>
>> Signed-off-by: Werner Sembach <[email protected]>
>> ---
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 22 ++++++++++++++-----
>> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++++
>> 2 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 9ffd2f9d3d75..c5dbf948a47a 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -5252,7 +5252,8 @@ get_aspect_ratio(const struct drm_display_mode *mode_in)
>> }
>>
>> static enum dc_color_space
>> -get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
>> +get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
>> + enum drm_mode_color_range preferred_color_range)
>> {
>> enum dc_color_space color_space = COLOR_SPACE_SRGB;
>>
>> @@ -5267,13 +5268,17 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
>> * respectively
>> */
>> if (dc_crtc_timing->pix_clk_100hz > 270300) {
>> - if (dc_crtc_timing->flags.Y_ONLY)
>> + if (dc_crtc_timing->flags.Y_ONLY
>> + || preferred_color_range ==
>> + DRM_MODE_COLOR_RANGE_LIMITED_16_235)
>> color_space =
>> COLOR_SPACE_YCBCR709_LIMITED;
>> else
>> color_space = COLOR_SPACE_YCBCR709;
> Hi,
>
> does this mean that amdgpu would be using a property named "Broadcast
> RGB" to control the range of YCbCr too?
Yes, because I avoided creating a new property, but I'm not really happy with it either.
Possibility 1: Use "Broadcast RGB" for Y'CbCr too and clarify in documentation
??? - still confusing name
??? - limited does not mean something a little bit different for Y'CbCr and not strictly 16-235:
https://www.kernel.org/doc/html/v5.12/userspace-api/media/v4l/colorspaces-defs.html#c.V4L.v4l2_quantization , but name
of option is given by preexisting property
Possibility 2: Deprecate "Broadcast RGB" and a a more neutral sounding "preferred color range", with the more neutral
sounding "limited" option instead of "Limited 16:235"
??? - What's the relation between the 2? pq mentioned on the amdgpu gitlab that there is a posibility for userspace to
have only the new or the old one shown
??? - Alternatively ignore "Broadcast RGB" when "preferred color range" is set and have them coexist?
>
> That is surprising. If this is truly wanted, then the documentation of
> "Broadcast RGB" must say that it applies to YCbCr too.
>
> Does amdgpu do the same as intel wrt. to the question about whose
> responsibility it is to make the pixels at the connector to match the
> set range?
I guess the kernel driver does the conversion, but i have to check for both.
For Intel I did not change the behavior of Boradcast RGB, but i think it's not clearly specified in the docs where the
conversion happens.
>
>
> Thanks,
> pq
>
>> } else {
>> - if (dc_crtc_timing->flags.Y_ONLY)
>> + if (dc_crtc_timing->flags.Y_ONLY
>> + || preferred_color_range ==
>> + DRM_MODE_COLOR_RANGE_LIMITED_16_235)
>> color_space =
>> COLOR_SPACE_YCBCR601_LIMITED;
>> else
>> @@ -5283,7 +5288,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
>> }
>> break;
>> case PIXEL_ENCODING_RGB:
>> - color_space = COLOR_SPACE_SRGB;
>> + if (preferred_color_range == DRM_MODE_COLOR_RANGE_LIMITED_16_235)
>> + color_space = COLOR_SPACE_SRGB_LIMITED;
>> + else
>> + color_space = COLOR_SPACE_SRGB;
>> break;
>>
>> default:
>> @@ -5429,7 +5437,10 @@ static void fill_stream_properties_from_drm_display_mode(
>>
>> timing_out->aspect_ratio = get_aspect_ratio(mode_in);
>>
>> - stream->output_color_space = get_output_color_space(timing_out);
>> + stream->output_color_space = get_output_color_space(timing_out,
>> + connector_state ?
>> + connector_state->preferred_color_range :
>> + DRM_MODE_COLOR_RANGE_UNSET);
>>
>> stream->out_transfer_func->type = TF_TYPE_PREDEFINED;
>> stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;
>> @@ -7780,6 +7791,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
>> drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16);
>> drm_connector_attach_preferred_color_format_property(&aconnector->base);
>> drm_connector_attach_active_color_format_property(&aconnector->base);
>> + drm_connector_attach_preferred_color_range_property(&aconnector->base);
>> drm_connector_attach_active_color_range_property(&aconnector->base);
>> }
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> index 2563788ba95a..80e1389fd0ec 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> @@ -421,6 +421,10 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>> if (connector->active_color_format_property)
>> drm_connector_attach_active_color_format_property(&aconnector->base);
>>
>> + connector->preferred_color_range_property = master->base.preferred_color_range_property;
>> + if (connector->preferred_color_range_property)
>> + drm_connector_attach_preferred_color_range_property(&aconnector->base);
>> +
>> connector->active_color_range_property = master->base.active_color_range_property;
>> if (connector->active_color_range_property)
>> drm_connector_attach_active_color_range_property(&aconnector->base);
> _______________________________________________
> amd-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Tue, 22 Jun 2021 11:28:57 +0200
Werner Sembach <[email protected]> wrote:
> Am 22.06.21 um 09:29 schrieb Pekka Paalanen:
> > On Fri, 18 Jun 2021 11:11:16 +0200
> > Werner Sembach <[email protected]> wrote:
> >
> >> This commit implements the "Broadcast RGB" drm property for the AMD GPU
> >> driver.
> >>
> >> Signed-off-by: Werner Sembach <[email protected]>
> >> ---
> >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 22 ++++++++++++++-----
> >> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++++
> >> 2 files changed, 21 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> index 9ffd2f9d3d75..c5dbf948a47a 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> @@ -5252,7 +5252,8 @@ get_aspect_ratio(const struct drm_display_mode *mode_in)
> >> }
> >>
> >> static enum dc_color_space
> >> -get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
> >> +get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
> >> + enum drm_mode_color_range preferred_color_range)
> >> {
> >> enum dc_color_space color_space = COLOR_SPACE_SRGB;
> >>
> >> @@ -5267,13 +5268,17 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
> >> * respectively
> >> */
> >> if (dc_crtc_timing->pix_clk_100hz > 270300) {
> >> - if (dc_crtc_timing->flags.Y_ONLY)
> >> + if (dc_crtc_timing->flags.Y_ONLY
> >> + || preferred_color_range ==
> >> + DRM_MODE_COLOR_RANGE_LIMITED_16_235)
> >> color_space =
> >> COLOR_SPACE_YCBCR709_LIMITED;
> >> else
> >> color_space = COLOR_SPACE_YCBCR709;
> > Hi,
> >
> > does this mean that amdgpu would be using a property named "Broadcast
> > RGB" to control the range of YCbCr too?
>
> Yes, because I avoided creating a new property, but I'm not really happy with it either.
>
> Possibility 1: Use "Broadcast RGB" for Y'CbCr too and clarify in documentation
> - still confusing name
> - limited does not mean something a little bit different for Y'CbCr and not strictly 16-235:
> https://www.kernel.org/doc/html/v5.12/userspace-api/media/v4l/colorspaces-defs.html#c.V4L.v4l2_quantization , but name
> of option is given by preexisting property
>
> Possibility 2: Deprecate "Broadcast RGB" and a a more neutral sounding "preferred color range", with the more neutral
> sounding "limited" option instead of "Limited 16:235"
> - What's the relation between the 2? pq mentioned on the amdgpu
> gitlab that there is a posibility for userspace to have only the new
> or the old one shown
It's just an idea that we could decide to expose only one or the other
property. It would need to be engineered in code, go through the UAPI
validation with userspace etc. I'm not aware of this being done before
exactly like this, but DRM client caps exist.
> - Alternatively ignore "Broadcast RGB" when "preferred color range" is set and have them coexist?
Determining "is set" means we would need "unset" value for "preferred
color range". But there is no notion of who set it. If some KMS client
decides to set it, then it will likely remain set, even if you next
start another KMS client who does not use this property - it would just
confuse users when "Broadcast RGB" silently stopped working while it
still exists.
So I don't think this is a good solution.
When considering a new property, what I wrote just earlier fit here:
https://lists.freedesktop.org/archives/dri-devel/2021-June/312248.html
There are more questions that just what does the limited range actually
mean.
> > That is surprising. If this is truly wanted, then the documentation of
> > "Broadcast RGB" must say that it applies to YCbCr too.
> >
> > Does amdgpu do the same as intel wrt. to the question about whose
> > responsibility it is to make the pixels at the connector to match the
> > set range?
>
> I guess the kernel driver does the conversion, but i have to check
> for both.
>
> For Intel I did not change the behavior of Boradcast RGB, but i think
> it's not clearly specified in the docs where the conversion happens.
Right, at the very least the current behaviour needs to be documented
before enrolling this property to any more drivers, so that those
drivers can then be reviewed to work the same way.
You notice I didn't actually answer your question 1 or 2. I don't know.
Going with 1 is easy compared to 2, even if the names are awkward but
it technically shouldn't cause any problems. 2 may or may not be
better, and until we have answers to which design is better, it's maybe
best to leave option 2 alone?
Thanks,
pq
Am 23.06.21 um 10:01 schrieb Pekka Paalanen:
> On Tue, 22 Jun 2021 11:28:57 +0200
> Werner Sembach <[email protected]> wrote:
>
>> Am 22.06.21 um 09:29 schrieb Pekka Paalanen:
>>> On Fri, 18 Jun 2021 11:11:16 +0200
>>> Werner Sembach <[email protected]> wrote:
>>>
>>>> This commit implements the "Broadcast RGB" drm property for the AMD GPU
>>>> driver.
>>>>
>>>> Signed-off-by: Werner Sembach <[email protected]>
>>>> ---
>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 22 ++++++++++++++-----
>>>> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++++
>>>> 2 files changed, 21 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> index 9ffd2f9d3d75..c5dbf948a47a 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -5252,7 +5252,8 @@ get_aspect_ratio(const struct drm_display_mode *mode_in)
>>>> }
>>>>
>>>> static enum dc_color_space
>>>> -get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
>>>> +get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
>>>> + enum drm_mode_color_range preferred_color_range)
>>>> {
>>>> enum dc_color_space color_space = COLOR_SPACE_SRGB;
>>>>
>>>> @@ -5267,13 +5268,17 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
>>>> * respectively
>>>> */
>>>> if (dc_crtc_timing->pix_clk_100hz > 270300) {
>>>> - if (dc_crtc_timing->flags.Y_ONLY)
>>>> + if (dc_crtc_timing->flags.Y_ONLY
>>>> + || preferred_color_range ==
>>>> + DRM_MODE_COLOR_RANGE_LIMITED_16_235)
>>>> color_space =
>>>> COLOR_SPACE_YCBCR709_LIMITED;
>>>> else
>>>> color_space = COLOR_SPACE_YCBCR709;
>>> Hi,
>>>
>>> does this mean that amdgpu would be using a property named "Broadcast
>>> RGB" to control the range of YCbCr too?
>> Yes, because I avoided creating a new property, but I'm not really happy with it either.
>>
>> Possibility 1: Use "Broadcast RGB" for Y'CbCr too and clarify in documentation
>> - still confusing name
>> - limited does not mean something a little bit different for Y'CbCr and not strictly 16-235:
>> https://www.kernel.org/doc/html/v5.12/userspace-api/media/v4l/colorspaces-defs.html#c.V4L.v4l2_quantization , but name
>> of option is given by preexisting property
>>
>> Possibility 2: Deprecate "Broadcast RGB" and a a more neutral sounding "preferred color range", with the more neutral
>> sounding "limited" option instead of "Limited 16:235"
>> - What's the relation between the 2? pq mentioned on the amdgpu
>> gitlab that there is a posibility for userspace to have only the new
>> or the old one shown
> It's just an idea that we could decide to expose only one or the other
> property. It would need to be engineered in code, go through the UAPI
> validation with userspace etc. I'm not aware of this being done before
> exactly like this, but DRM client caps exist.
>
>> - Alternatively ignore "Broadcast RGB" when "preferred color range" is set and have them coexist?
> Determining "is set" means we would need "unset" value for "preferred
> color range". But there is no notion of who set it. If some KMS client
> decides to set it, then it will likely remain set, even if you next
> start another KMS client who does not use this property - it would just
> confuse users when "Broadcast RGB" silently stopped working while it
> still exists.
Unset would be the "auto" option. But i think this problem exists already e.g. a KMS client is unaware of the new
"preferred color format" property and sets "Broadcast RGB" on intel which does nothing in the case of Y'CbCr.
Since the properties affecting each other, having only one not at default, potentially breaks KMS clients unaware of
them regardless.
>
> So I don't think this is a good solution.
>
> When considering a new property, what I wrote just earlier fit here:
> https://lists.freedesktop.org/archives/dri-devel/2021-June/312248.html
>
> There are more questions that just what does the limited range actually
> mean.
>
>>> That is surprising. If this is truly wanted, then the documentation of
>>> "Broadcast RGB" must say that it applies to YCbCr too.
>>>
>>> Does amdgpu do the same as intel wrt. to the question about whose
>>> responsibility it is to make the pixels at the connector to match the
>>> set range?
>> I guess the kernel driver does the conversion, but i have to check
>> for both.
>>
>> For Intel I did not change the behavior of Boradcast RGB, but i think
>> it's not clearly specified in the docs where the conversion happens.
> Right, at the very least the current behaviour needs to be documented
> before enrolling this property to any more drivers, so that those
> drivers can then be reviewed to work the same way.
>
> You notice I didn't actually answer your question 1 or 2. I don't know.
> Going with 1 is easy compared to 2, even if the names are awkward but
> it technically shouldn't cause any problems. 2 may or may not be
> better, and until we have answers to which design is better, it's maybe
> best to leave option 2 alone?
OK, I will try to validate the behavior of the Intel driver and be detailed in the documentation, but keep the
implementation like this now (unless the intel driver does something different then I thought ofc).
>
>
> Thanks,
> pq