Re: [PATCH v2 3/9] drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range

[Hans Verkuil]

On 2/20/23 16:23, Dave Stevenson wrote:
> Hi Hans
> 
> On Sat, 18 Feb 2023 at 11:33, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>
>> Hi Maxime, Dave,
>>
>> On 26/01/2023 14:46, Maxime Ripard wrote:
>>> From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
>>>
>>> Copy Intel's "Broadcast RGB" property semantics to add manual override
>>> of the HDMI pixel range for monitors that don't abide by the content
>>> of the AVI Infoframe.
>>
>> Do we have to copy that property as-is?
> 
> Firstly I'll agree with your later comment that having this control
> allows testing of a range of output modes, and working around HDMI
> sinks that have dodgy implementations.
> (In our vendor kernel we now also have a property to override the
> kernel chosen output format to enable testing of YCbCr4:4:4 and 4:2:2
> output).
> 
> The DRM subsystem has the requirement for an open-source userspace
> project to be using any new property before it will be merged [1].
> This property already exists for i915 and gma-500, therefore avoids
> that requirement.
> 
> There are objections to the UAPI for Broadcast RGB [2], but if it's
> good enough for the existing implementations then there can be no
> objection to it being implemented in the same way for other drivers.
> Otherwise it is a missing feature of the DRM API, and the linked
> discussion is realistically at least a year away from being resolved.
> Why bury our heads in the sand for that period?
> 
> [1] https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements
> [2] https://lists.freedesktop.org/archives/dri-devel/2023-February/391061.html
> 
>> First of all, I think this should really be a drm-level property, rather than
>> a driver property: RGB Quantization Range mismatches are the bane of my life,
>> and I think a way to override this would help everyone.
> 
> Linked to above, if it were the preferred method for controlling this
> then I would expect it to become a drm-level property.
> 
>> Secondly, I hate the name they came up with: 'Broadcast RGB' is pretty meaningless.
>> Can't we stick to something closer to what the CTA-861/HDMI specs use, which is
>> 'RGB Quantization Range'? So either use that, or just 'RGB Range'.
>>
>> In addition, 'Limited 16:235' should just be 'Limited' since the actual range
>> depends on the bits-per-color-component.
> 
> It's documented UAPI with those names[3], therefore any change would
> be a change to user-space's expectation and a regression. At least by
> sticking with the same names then any user space implementation that
> exists for i915 or gma-500 will also work in the same way on vc4.
> 
> [3] https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#existing-kms-properties

Thank you for linking to these threads. It's a terrible name, but it is still
better to have this property than to not have it :-)

> 
>>>
>>> Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/vc4/vc4_hdmi.c | 97 ++++++++++++++++++++++++++++++++++++++++--
>>>  drivers/gpu/drm/vc4/vc4_hdmi.h |  9 ++++
>>>  2 files changed, 102 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> index 4b3bf77bb5cd..78749c6fa837 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> @@ -150,10 +150,16 @@ static bool vc4_hdmi_mode_needs_scrambling(const struct drm_display_mode *mode,
>>>  }
>>>
>>>  static bool vc4_hdmi_is_full_range_rgb(struct vc4_hdmi *vc4_hdmi,
>>> -                                    const struct drm_display_mode *mode)
>>> +                                    struct vc4_hdmi_connector_state *vc4_state)
>>>  {
>>> +     const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
>>>       struct drm_display_info *display = &vc4_hdmi->connector.display_info;
>>>
>>> +     if (vc4_state->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_LIMITED)
>>> +             return false;
>>> +     else if (vc4_state->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_FULL)
>>> +             return true;
>>> +
>>>       return !display->is_hdmi ||
>>>               drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_FULL;
>>>  }
>>> @@ -524,8 +530,12 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
>>>  {
>>>       struct drm_connector_state *old_state =
>>>               drm_atomic_get_old_connector_state(state, connector);
>>> +     struct vc4_hdmi_connector_state *old_vc4_state =
>>> +             conn_state_to_vc4_hdmi_conn_state(old_state);
>>>       struct drm_connector_state *new_state =
>>>               drm_atomic_get_new_connector_state(state, connector);
>>> +     struct vc4_hdmi_connector_state *new_vc4_state =
>>> +             conn_state_to_vc4_hdmi_conn_state(new_state);
>>>       struct drm_crtc *crtc = new_state->crtc;
>>>
>>>       if (!crtc)
>>> @@ -558,6 +568,7 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
>>>       }
>>>
>>>       if (old_state->colorspace != new_state->colorspace ||
>>> +         old_vc4_state->broadcast_rgb != new_vc4_state->broadcast_rgb ||
>>
>> The problem with this is that this will cause a mode change, even though all
>> that is necessary is to update the csc matrix and AVI InfoFrame.
>>
>> I used this code (added just before the 'return 0;' at the end of this function):
>>
>>         if (old_vc4_state->broadcast_rgb != new_vc4_state->broadcast_rgb) {
>>                 const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
>>
>>                 old_vc4_state->broadcast_rgb = new_vc4_state->broadcast_rgb;
>>                 vc4_hdmi_set_avi_infoframe(encoder);
>>                 if (vc4_hdmi->variant->csc_setup)
>>                         vc4_hdmi->variant->csc_setup(vc4_hdmi, old_state, mode);
>>         }
>>
>> I'm certain this is in the wrong place, but I'm not familiar enough with the drm API
>> to determine where this should go.
> 
> atomic_check is meant to validate the state, but must never touch the
> hardware [4].
> atomic_commit updates the hardware, although for struct
> drm_connector_helper_funcs atomic_commit is documented as only being
> applicable for writeback connectors [5]. Hence our only sensible
> options are to trigger a mode change which reprograms everything, or
> ignore the helpers.
> There may be scope for improving this, but it'll need significant
> driver (and possibly framework) rework in order to be done properly.
> 
> [4] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L1083
> [5] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L1101
> 
>> This approach probably applies to the hdr_metadata metadata as well, that too
>> doesn't need a mode change.
>>
>> I see that the i915 driver has a 'fastset' mechanism for changes like this, but
>> it is not clear to me how that interacts with the drm API.
>>
>> I've been playing around with this vc4 driver and it is proving to be very useful
>> for debugging all sorts of quantization range bugs in other equipment.
> 
> Glad it's proving useful.
> 
> We're also needing it as users have misbehaving monitors, and telling
> them to replace the monitor as it breaks the HDMI spec generally
> doesn't go down well! Likewise I've had users needing YCbCr444 output
> because of some limitation in their receiving device, hence overriding
> the kernel choice of output mode. It'd be nice if there were consensus
> on the correct way to achieve that.

In any case, given this background you can add my:

Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>

Regards,

	Hans

> 
> Cheers.
>   Dave
> 
>> Regards,
>>
>>         Hans
>>
>>>           !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
>>>               struct drm_crtc_state *crtc_state;
>>>
>>> @@ -571,6 +582,49 @@ static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
>>>       return 0;
>>>  }
>>>
>>> +static int vc4_hdmi_connector_get_property(struct drm_connector *connector,
>>> +                                        const struct drm_connector_state *state,
>>> +                                        struct drm_property *property,
>>> +                                        uint64_t *val)
>>> +{
>>> +     struct drm_device *drm = connector->dev;
>>> +     struct vc4_hdmi *vc4_hdmi =
>>> +             connector_to_vc4_hdmi(connector);
>>> +     const struct vc4_hdmi_connector_state *vc4_conn_state =
>>> +             conn_state_to_vc4_hdmi_conn_state(state);
>>> +
>>> +     if (property == vc4_hdmi->broadcast_rgb_property) {
>>> +             *val = vc4_conn_state->broadcast_rgb;
>>> +     } else {
>>> +             drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
>>> +                     property->base.id, property->name);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int vc4_hdmi_connector_set_property(struct drm_connector *connector,
>>> +                                        struct drm_connector_state *state,
>>> +                                        struct drm_property *property,
>>> +                                        uint64_t val)
>>> +{
>>> +     struct drm_device *drm = connector->dev;
>>> +     struct vc4_hdmi *vc4_hdmi =
>>> +             connector_to_vc4_hdmi(connector);
>>> +     struct vc4_hdmi_connector_state *vc4_conn_state =
>>> +             conn_state_to_vc4_hdmi_conn_state(state);
>>> +
>>> +     if (property == vc4_hdmi->broadcast_rgb_property) {
>>> +             vc4_conn_state->broadcast_rgb = val;
>>> +             return 0;
>>> +     }
>>> +
>>> +     drm_dbg(drm, "Unknown property [PROP:%d:%s]\n",
>>> +             property->base.id, property->name);
>>> +     return -EINVAL;
>>> +}
>>> +
>>>  static void vc4_hdmi_connector_reset(struct drm_connector *connector)
>>>  {
>>>       struct vc4_hdmi_connector_state *old_state =
>>> @@ -590,6 +644,7 @@ static void vc4_hdmi_connector_reset(struct drm_connector *connector)
>>>       new_state->base.max_bpc = 8;
>>>       new_state->base.max_requested_bpc = 8;
>>>       new_state->output_format = VC4_HDMI_OUTPUT_RGB;
>>> +     new_state->broadcast_rgb = VC4_HDMI_BROADCAST_RGB_AUTO;
>>>       drm_atomic_helper_connector_tv_margins_reset(connector);
>>>  }
>>>
>>> @@ -607,6 +662,7 @@ vc4_hdmi_connector_duplicate_state(struct drm_connector *connector)
>>>       new_state->tmds_char_rate = vc4_state->tmds_char_rate;
>>>       new_state->output_bpc = vc4_state->output_bpc;
>>>       new_state->output_format = vc4_state->output_format;
>>> +     new_state->broadcast_rgb = vc4_state->broadcast_rgb;
>>>       __drm_atomic_helper_connector_duplicate_state(connector, &new_state->base);
>>>
>>>       return &new_state->base;
>>> @@ -617,6 +673,8 @@ static const struct drm_connector_funcs vc4_hdmi_connector_funcs = {
>>>       .reset = vc4_hdmi_connector_reset,
>>>       .atomic_duplicate_state = vc4_hdmi_connector_duplicate_state,
>>>       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>> +     .atomic_get_property = vc4_hdmi_connector_get_property,
>>> +     .atomic_set_property = vc4_hdmi_connector_set_property,
>>>  };
>>>
>>>  static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs = {
>>> @@ -625,6 +683,33 @@ static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs =
>>>       .atomic_check = vc4_hdmi_connector_atomic_check,
>>>  };
>>>
>>> +static const struct drm_prop_enum_list broadcast_rgb_names[] = {
>>> +     { VC4_HDMI_BROADCAST_RGB_AUTO, "Automatic" },
>>> +     { VC4_HDMI_BROADCAST_RGB_FULL, "Full" },
>>> +     { VC4_HDMI_BROADCAST_RGB_LIMITED, "Limited 16:235" },
>>> +};
>>> +
>>> +static void
>>> +vc4_hdmi_attach_broadcast_rgb_property(struct drm_device *dev,
>>> +                                    struct vc4_hdmi *vc4_hdmi)
>>> +{
>>> +     struct drm_property *prop = vc4_hdmi->broadcast_rgb_property;
>>> +
>>> +     if (!prop) {
>>> +             prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
>>> +                                             "Broadcast RGB",
>>> +                                             broadcast_rgb_names,
>>> +                                             ARRAY_SIZE(broadcast_rgb_names));
>>> +             if (!prop)
>>> +                     return;
>>> +
>>> +             vc4_hdmi->broadcast_rgb_property = prop;
>>> +     }
>>> +
>>> +     drm_object_attach_property(&vc4_hdmi->connector.base, prop,
>>> +                                VC4_HDMI_BROADCAST_RGB_AUTO);
>>> +}
>>> +
>>>  static int vc4_hdmi_connector_init(struct drm_device *dev,
>>>                                  struct vc4_hdmi *vc4_hdmi)
>>>  {
>>> @@ -671,6 +756,8 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
>>>       if (vc4_hdmi->variant->supports_hdr)
>>>               drm_connector_attach_hdr_output_metadata_property(connector);
>>>
>>> +     vc4_hdmi_attach_broadcast_rgb_property(dev, vc4_hdmi);
>>> +
>>>       drm_connector_attach_encoder(connector, encoder);
>>>
>>>       return 0;
>>> @@ -825,7 +912,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder *encoder)
>>>
>>>       drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>>>                                          connector, mode,
>>> -                                        vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode) ?
>>> +                                        vc4_hdmi_is_full_range_rgb(vc4_hdmi, vc4_state) ?
>>>                                          HDMI_QUANTIZATION_RANGE_FULL :
>>>                                          HDMI_QUANTIZATION_RANGE_LIMITED);
>>>       drm_hdmi_avi_infoframe_colorimetry(&frame.avi, cstate);
>>> @@ -1066,6 +1153,8 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
>>>                              struct drm_connector_state *state,
>>>                              const struct drm_display_mode *mode)
>>>  {
>>> +     struct vc4_hdmi_connector_state *vc4_state =
>>> +             conn_state_to_vc4_hdmi_conn_state(state);
>>>       struct drm_device *drm = vc4_hdmi->connector.dev;
>>>       unsigned long flags;
>>>       u32 csc_ctl;
>>> @@ -1079,7 +1168,7 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
>>>       csc_ctl = VC4_SET_FIELD(VC4_HD_CSC_CTL_ORDER_BGR,
>>>                               VC4_HD_CSC_CTL_ORDER);
>>>
>>> -     if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode)) {
>>> +     if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, vc4_state)) {
>>>               /* CEA VICs other than #1 requre limited range RGB
>>>                * output unless overridden by an AVI infoframe.
>>>                * Apply a colorspace conversion to squash 0-255 down
>>> @@ -1232,7 +1321,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi,
>>>       case VC4_HDMI_OUTPUT_RGB:
>>>               if_xbar = 0x354021;
>>>
>>> -             if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, mode))
>>> +             if (!vc4_hdmi_is_full_range_rgb(vc4_hdmi, vc4_state))
>>>                       vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_to_limited_rgb);
>>>               else
>>>                       vc5_hdmi_set_csc_coeffs(vc4_hdmi, vc5_hdmi_csc_full_rgb_unity);
>>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
>>> index 5d249ac54cd1..89800c48aa24 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
>>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
>>> @@ -117,6 +117,12 @@ enum vc4_hdmi_output_format {
>>>       VC4_HDMI_OUTPUT_YUV420,
>>>  };
>>>
>>> +enum vc4_hdmi_broadcast_rgb {
>>> +     VC4_HDMI_BROADCAST_RGB_AUTO,
>>> +     VC4_HDMI_BROADCAST_RGB_FULL,
>>> +     VC4_HDMI_BROADCAST_RGB_LIMITED,
>>> +};
>>> +
>>>  /* General HDMI hardware state. */
>>>  struct vc4_hdmi {
>>>       struct vc4_hdmi_audio audio;
>>> @@ -129,6 +135,8 @@ struct vc4_hdmi {
>>>
>>>       struct delayed_work scrambling_work;
>>>
>>> +     struct drm_property *broadcast_rgb_property;
>>> +
>>>       struct i2c_adapter *ddc;
>>>       void __iomem *hdmicore_regs;
>>>       void __iomem *hd_regs;
>>> @@ -238,6 +246,7 @@ struct vc4_hdmi_connector_state {
>>>       unsigned long long              tmds_char_rate;
>>>       unsigned int                    output_bpc;
>>>       enum vc4_hdmi_output_format     output_format;
>>> +     enum vc4_hdmi_broadcast_rgb     broadcast_rgb;
>>>  };
>>>
>>>  #define conn_state_to_vc4_hdmi_conn_state(_state)                    \
>>>
>>