diff mbox series

[v2,2/2] drm/connector: automatically set immutable flag for max_bpc property

Message ID 20240623-drm-bridge-connector-fix-hdmi-reset-v2-2-8590d44912ce@linaro.org (mailing list archive)
State New, archived
Headers show
Series drm: fix two issues related to HDMI Connector implementation | expand

Commit Message

Dmitry Baryshkov June 23, 2024, 5:40 a.m. UTC
With the introduction of the HDMI Connector framework the driver might
end up creating the max_bpc property with min = max = 8. IGT insists
that such properties carry the 'immutable' flag. Automatically set the
flag if the driver asks for the max_bpc property with min == max.

Fixes: aadb3e16b8f3 ("drm/connector: hdmi: Add output BPC to the connector state")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_connector.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Abhinav Kumar June 24, 2024, 10:39 p.m. UTC | #1
+ IGT dev

On 6/22/2024 10:40 PM, Dmitry Baryshkov wrote:
> With the introduction of the HDMI Connector framework the driver might
> end up creating the max_bpc property with min = max = 8. IGT insists
> that such properties carry the 'immutable' flag. Automatically set the
> flag if the driver asks for the max_bpc property with min == max.
> 

This change does not look right to me.

I wonder why we need this check because DRM_MODE_PROP_IMMUTABLE means 
that as per the doc, userspace cannot change the property.

          * DRM_MODE_PROP_IMMUTABLE
          *     Set for properties whose values cannot be changed by
          *     userspace. The kernel is allowed to update the value of 
these
          *     properties. This is generally used to expose probe state to
          *     userspace, e.g. the EDID, or the connector path property 
on DP
          *     MST sinks. Kernel can update the value of an immutable 
property
          *     by calling drm_object_property_set_value().
          */

Here we are allowing userspace to change max_bpc


drm_atomic_connector_set_property()
{
	**********

         } else if (property == connector->max_bpc_property) {
                 state->max_requested_bpc = val;

	**********
}

I believe you are referring to this IGT check right?

https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_properties.c#L428

I think we should fix IGT in this case unless there is some reason we 
are missing. Because just because it has the same min and max does not 
mean its immutable by the doc of the IMMUTABLE flag.


> Fixes: aadb3e16b8f3 ("drm/connector: hdmi: Add output BPC to the connector state")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/drm_connector.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index ab6ab7ff7ea8..33847fd63628 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2610,7 +2610,12 @@ int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>   
>   	prop = connector->max_bpc_property;
>   	if (!prop) {
> -		prop = drm_property_create_range(dev, 0, "max bpc", min, max);
> +		u32 flags = 0;
> +
> +		if (min == max)
> +			flags |= DRM_MODE_PROP_IMMUTABLE;
> +
> +		prop = drm_property_create_range(dev, flags, "max bpc", min, max);
>   		if (!prop)
>   			return -ENOMEM;
>   
>
Dmitry Baryshkov June 24, 2024, 10:46 p.m. UTC | #2
On Tue, 25 Jun 2024 at 01:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> + IGT dev
>
> On 6/22/2024 10:40 PM, Dmitry Baryshkov wrote:
> > With the introduction of the HDMI Connector framework the driver might
> > end up creating the max_bpc property with min = max = 8. IGT insists
> > that such properties carry the 'immutable' flag. Automatically set the
> > flag if the driver asks for the max_bpc property with min == max.
> >
>
> This change does not look right to me.
>
> I wonder why we need this check because DRM_MODE_PROP_IMMUTABLE means
> that as per the doc, userspace cannot change the property.
>
>           * DRM_MODE_PROP_IMMUTABLE
>           *     Set for properties whose values cannot be changed by
>           *     userspace. The kernel is allowed to update the value of
> these
>           *     properties. This is generally used to expose probe state to
>           *     userspace, e.g. the EDID, or the connector path property
> on DP
>           *     MST sinks. Kernel can update the value of an immutable
> property
>           *     by calling drm_object_property_set_value().
>           */
>
> Here we are allowing userspace to change max_bpc
>
>
> drm_atomic_connector_set_property()
> {
>         **********
>
>          } else if (property == connector->max_bpc_property) {
>                  state->max_requested_bpc = val;
>
>         **********
> }
>
> I believe you are referring to this IGT check right?
>
> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_properties.c#L428

Yes

>
> I think we should fix IGT in this case unless there is some reason we
> are missing. Because just because it has the same min and max does not
> mean its immutable by the doc of the IMMUTABLE flag.

Well, having the same min and max means that it is impossible to
change the property. So the property is immutable, but doesn't have
the flag.

>
>
> > Fixes: aadb3e16b8f3 ("drm/connector: hdmi: Add output BPC to the connector state")
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/drm_connector.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)

With best wishes
Dmitry
Abhinav Kumar June 24, 2024, 10:55 p.m. UTC | #3
On 6/24/2024 3:46 PM, Dmitry Baryshkov wrote:
> On Tue, 25 Jun 2024 at 01:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> + IGT dev
>>
>> On 6/22/2024 10:40 PM, Dmitry Baryshkov wrote:
>>> With the introduction of the HDMI Connector framework the driver might
>>> end up creating the max_bpc property with min = max = 8. IGT insists
>>> that such properties carry the 'immutable' flag. Automatically set the
>>> flag if the driver asks for the max_bpc property with min == max.
>>>
>>
>> This change does not look right to me.
>>
>> I wonder why we need this check because DRM_MODE_PROP_IMMUTABLE means
>> that as per the doc, userspace cannot change the property.
>>
>>            * DRM_MODE_PROP_IMMUTABLE
>>            *     Set for properties whose values cannot be changed by
>>            *     userspace. The kernel is allowed to update the value of
>> these
>>            *     properties. This is generally used to expose probe state to
>>            *     userspace, e.g. the EDID, or the connector path property
>> on DP
>>            *     MST sinks. Kernel can update the value of an immutable
>> property
>>            *     by calling drm_object_property_set_value().
>>            */
>>
>> Here we are allowing userspace to change max_bpc
>>
>>
>> drm_atomic_connector_set_property()
>> {
>>          **********
>>
>>           } else if (property == connector->max_bpc_property) {
>>                   state->max_requested_bpc = val;
>>
>>          **********
>> }
>>
>> I believe you are referring to this IGT check right?
>>
>> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_properties.c#L428
> 
> Yes
> 
>>
>> I think we should fix IGT in this case unless there is some reason we
>> are missing. Because just because it has the same min and max does not
>> mean its immutable by the doc of the IMMUTABLE flag.
> 
> Well, having the same min and max means that it is impossible to
> change the property. So the property is immutable, but doesn't have
> the flag.
> 

True, then does DRM_MODE_PROP_IMMUTABLE need a doc update too indicating 
that even if the min and max is same, property will be interpreted as 
immutable.

>>
>>
>>> Fixes: aadb3e16b8f3 ("drm/connector: hdmi: Add output BPC to the connector state")
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/drm_connector.c | 7 ++++++-
>>>    1 file changed, 6 insertions(+), 1 deletion(-)
> 
> With best wishes
> Dmitry
Dmitry Baryshkov June 25, 2024, 6:21 a.m. UTC | #4
On Tue, 25 Jun 2024 at 01:56, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/24/2024 3:46 PM, Dmitry Baryshkov wrote:
> > On Tue, 25 Jun 2024 at 01:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> + IGT dev
> >>
> >> On 6/22/2024 10:40 PM, Dmitry Baryshkov wrote:
> >>> With the introduction of the HDMI Connector framework the driver might
> >>> end up creating the max_bpc property with min = max = 8. IGT insists
> >>> that such properties carry the 'immutable' flag. Automatically set the
> >>> flag if the driver asks for the max_bpc property with min == max.
> >>>
> >>
> >> This change does not look right to me.
> >>
> >> I wonder why we need this check because DRM_MODE_PROP_IMMUTABLE means
> >> that as per the doc, userspace cannot change the property.
> >>
> >>            * DRM_MODE_PROP_IMMUTABLE
> >>            *     Set for properties whose values cannot be changed by
> >>            *     userspace. The kernel is allowed to update the value of
> >> these
> >>            *     properties. This is generally used to expose probe state to
> >>            *     userspace, e.g. the EDID, or the connector path property
> >> on DP
> >>            *     MST sinks. Kernel can update the value of an immutable
> >> property
> >>            *     by calling drm_object_property_set_value().
> >>            */
> >>
> >> Here we are allowing userspace to change max_bpc
> >>
> >>
> >> drm_atomic_connector_set_property()
> >> {
> >>          **********
> >>
> >>           } else if (property == connector->max_bpc_property) {
> >>                   state->max_requested_bpc = val;
> >>
> >>          **********
> >> }
> >>
> >> I believe you are referring to this IGT check right?
> >>
> >> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_properties.c#L428
> >
> > Yes
> >
> >>
> >> I think we should fix IGT in this case unless there is some reason we
> >> are missing. Because just because it has the same min and max does not
> >> mean its immutable by the doc of the IMMUTABLE flag.
> >
> > Well, having the same min and max means that it is impossible to
> > change the property. So the property is immutable, but doesn't have
> > the flag.
> >
>
> True, then does DRM_MODE_PROP_IMMUTABLE need a doc update too indicating
> that even if the min and max is same, property will be interpreted as
> immutable.

Granted that I'm only doing it for max_bpc property I don't think so.

>
> >>
> >>
> >>> Fixes: aadb3e16b8f3 ("drm/connector: hdmi: Add output BPC to the connector state")
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>>    drivers/gpu/drm/drm_connector.c | 7 ++++++-
> >>>    1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > With best wishes
> > Dmitry
Maxime Ripard June 25, 2024, 7:19 a.m. UTC | #5
Hi,

On Tue, Jun 25, 2024 at 09:21:27AM GMT, Dmitry Baryshkov wrote:
> On Tue, 25 Jun 2024 at 01:56, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >
> >
> >
> > On 6/24/2024 3:46 PM, Dmitry Baryshkov wrote:
> > > On Tue, 25 Jun 2024 at 01:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >>
> > >> + IGT dev
> > >>
> > >> On 6/22/2024 10:40 PM, Dmitry Baryshkov wrote:
> > >>> With the introduction of the HDMI Connector framework the driver might
> > >>> end up creating the max_bpc property with min = max = 8. IGT insists
> > >>> that such properties carry the 'immutable' flag. Automatically set the
> > >>> flag if the driver asks for the max_bpc property with min == max.
> > >>>
> > >>
> > >> This change does not look right to me.
> > >>
> > >> I wonder why we need this check because DRM_MODE_PROP_IMMUTABLE means
> > >> that as per the doc, userspace cannot change the property.
> > >>
> > >>            * DRM_MODE_PROP_IMMUTABLE
> > >>            *     Set for properties whose values cannot be changed by
> > >>            *     userspace. The kernel is allowed to update the value of
> > >> these
> > >>            *     properties. This is generally used to expose probe state to
> > >>            *     userspace, e.g. the EDID, or the connector path property
> > >> on DP
> > >>            *     MST sinks. Kernel can update the value of an immutable
> > >> property
> > >>            *     by calling drm_object_property_set_value().
> > >>            */
> > >>
> > >> Here we are allowing userspace to change max_bpc
> > >>
> > >>
> > >> drm_atomic_connector_set_property()
> > >> {
> > >>          **********
> > >>
> > >>           } else if (property == connector->max_bpc_property) {
> > >>                   state->max_requested_bpc = val;
> > >>
> > >>          **********
> > >> }
> > >>
> > >> I believe you are referring to this IGT check right?
> > >>
> > >> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_properties.c#L428
> > >
> > > Yes
> > >
> > >>
> > >> I think we should fix IGT in this case unless there is some reason we
> > >> are missing. Because just because it has the same min and max does not
> > >> mean its immutable by the doc of the IMMUTABLE flag.
> > >
> > > Well, having the same min and max means that it is impossible to
> > > change the property. So the property is immutable, but doesn't have
> > > the flag.
> > >
> >
> > True, then does DRM_MODE_PROP_IMMUTABLE need a doc update too indicating
> > that even if the min and max is same, property will be interpreted as
> > immutable.
> 
> Granted that I'm only doing it for max_bpc property I don't think so.

Yeah, I have to agree with Abhinav here, it does look fishy to me too,
even more so that it's only ever "documented" through an igt routine
that has never documented why we want that.

I'm fine with the change if it's indeed what we expect, and it might
very well be, but I'd like to clear that up and document it first.

Maxime
Dmitry Baryshkov June 25, 2024, 7:23 a.m. UTC | #6
On Tue, 25 Jun 2024 at 10:19, Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Tue, Jun 25, 2024 at 09:21:27AM GMT, Dmitry Baryshkov wrote:
> > On Tue, 25 Jun 2024 at 01:56, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >
> > >
> > >
> > > On 6/24/2024 3:46 PM, Dmitry Baryshkov wrote:
> > > > On Tue, 25 Jun 2024 at 01:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > > >>
> > > >> + IGT dev
> > > >>
> > > >> On 6/22/2024 10:40 PM, Dmitry Baryshkov wrote:
> > > >>> With the introduction of the HDMI Connector framework the driver might
> > > >>> end up creating the max_bpc property with min = max = 8. IGT insists
> > > >>> that such properties carry the 'immutable' flag. Automatically set the
> > > >>> flag if the driver asks for the max_bpc property with min == max.
> > > >>>
> > > >>
> > > >> This change does not look right to me.
> > > >>
> > > >> I wonder why we need this check because DRM_MODE_PROP_IMMUTABLE means
> > > >> that as per the doc, userspace cannot change the property.
> > > >>
> > > >>            * DRM_MODE_PROP_IMMUTABLE
> > > >>            *     Set for properties whose values cannot be changed by
> > > >>            *     userspace. The kernel is allowed to update the value of
> > > >> these
> > > >>            *     properties. This is generally used to expose probe state to
> > > >>            *     userspace, e.g. the EDID, or the connector path property
> > > >> on DP
> > > >>            *     MST sinks. Kernel can update the value of an immutable
> > > >> property
> > > >>            *     by calling drm_object_property_set_value().
> > > >>            */
> > > >>
> > > >> Here we are allowing userspace to change max_bpc
> > > >>
> > > >>
> > > >> drm_atomic_connector_set_property()
> > > >> {
> > > >>          **********
> > > >>
> > > >>           } else if (property == connector->max_bpc_property) {
> > > >>                   state->max_requested_bpc = val;
> > > >>
> > > >>          **********
> > > >> }
> > > >>
> > > >> I believe you are referring to this IGT check right?
> > > >>
> > > >> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_properties.c#L428
> > > >
> > > > Yes
> > > >
> > > >>
> > > >> I think we should fix IGT in this case unless there is some reason we
> > > >> are missing. Because just because it has the same min and max does not
> > > >> mean its immutable by the doc of the IMMUTABLE flag.
> > > >
> > > > Well, having the same min and max means that it is impossible to
> > > > change the property. So the property is immutable, but doesn't have
> > > > the flag.
> > > >
> > >
> > > True, then does DRM_MODE_PROP_IMMUTABLE need a doc update too indicating
> > > that even if the min and max is same, property will be interpreted as
> > > immutable.
> >
> > Granted that I'm only doing it for max_bpc property I don't think so.
>
> Yeah, I have to agree with Abhinav here, it does look fishy to me too,
> even more so that it's only ever "documented" through an igt routine
> that has never documented why we want that.
>
> I'm fine with the change if it's indeed what we expect, and it might
> very well be, but I'd like to clear that up and document it first.

Should I also move the setting of the IMMUTABLE flag to a more generic code?
Maxime Ripard June 25, 2024, 1:58 p.m. UTC | #7
On Tue, Jun 25, 2024 at 10:23:14AM GMT, Dmitry Baryshkov wrote:
> On Tue, 25 Jun 2024 at 10:19, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > Hi,
> >
> > On Tue, Jun 25, 2024 at 09:21:27AM GMT, Dmitry Baryshkov wrote:
> > > On Tue, 25 Jun 2024 at 01:56, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > > >
> > > >
> > > >
> > > > On 6/24/2024 3:46 PM, Dmitry Baryshkov wrote:
> > > > > On Tue, 25 Jun 2024 at 01:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > > > >>
> > > > >> + IGT dev
> > > > >>
> > > > >> On 6/22/2024 10:40 PM, Dmitry Baryshkov wrote:
> > > > >>> With the introduction of the HDMI Connector framework the driver might
> > > > >>> end up creating the max_bpc property with min = max = 8. IGT insists
> > > > >>> that such properties carry the 'immutable' flag. Automatically set the
> > > > >>> flag if the driver asks for the max_bpc property with min == max.
> > > > >>>
> > > > >>
> > > > >> This change does not look right to me.
> > > > >>
> > > > >> I wonder why we need this check because DRM_MODE_PROP_IMMUTABLE means
> > > > >> that as per the doc, userspace cannot change the property.
> > > > >>
> > > > >>            * DRM_MODE_PROP_IMMUTABLE
> > > > >>            *     Set for properties whose values cannot be changed by
> > > > >>            *     userspace. The kernel is allowed to update the value of
> > > > >> these
> > > > >>            *     properties. This is generally used to expose probe state to
> > > > >>            *     userspace, e.g. the EDID, or the connector path property
> > > > >> on DP
> > > > >>            *     MST sinks. Kernel can update the value of an immutable
> > > > >> property
> > > > >>            *     by calling drm_object_property_set_value().
> > > > >>            */
> > > > >>
> > > > >> Here we are allowing userspace to change max_bpc
> > > > >>
> > > > >>
> > > > >> drm_atomic_connector_set_property()
> > > > >> {
> > > > >>          **********
> > > > >>
> > > > >>           } else if (property == connector->max_bpc_property) {
> > > > >>                   state->max_requested_bpc = val;
> > > > >>
> > > > >>          **********
> > > > >> }
> > > > >>
> > > > >> I believe you are referring to this IGT check right?
> > > > >>
> > > > >> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_properties.c#L428
> > > > >
> > > > > Yes
> > > > >
> > > > >>
> > > > >> I think we should fix IGT in this case unless there is some reason we
> > > > >> are missing. Because just because it has the same min and max does not
> > > > >> mean its immutable by the doc of the IMMUTABLE flag.
> > > > >
> > > > > Well, having the same min and max means that it is impossible to
> > > > > change the property. So the property is immutable, but doesn't have
> > > > > the flag.
> > > > >
> > > >
> > > > True, then does DRM_MODE_PROP_IMMUTABLE need a doc update too indicating
> > > > that even if the min and max is same, property will be interpreted as
> > > > immutable.
> > >
> > > Granted that I'm only doing it for max_bpc property I don't think so.
> >
> > Yeah, I have to agree with Abhinav here, it does look fishy to me too,
> > even more so that it's only ever "documented" through an igt routine
> > that has never documented why we want that.
> >
> > I'm fine with the change if it's indeed what we expect, and it might
> > very well be, but I'd like to clear that up and document it first.
> 
> Should I also move the setting of the IMMUTABLE flag to a more generic code?

Possibly, but I guess that will depend on the outcome of that discussion

Maxime
Dmitry Baryshkov June 25, 2024, 2:31 p.m. UTC | #8
On Tue, Jun 25, 2024 at 03:58:25PM GMT, Maxime Ripard wrote:
> On Tue, Jun 25, 2024 at 10:23:14AM GMT, Dmitry Baryshkov wrote:
> > On Tue, 25 Jun 2024 at 10:19, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Jun 25, 2024 at 09:21:27AM GMT, Dmitry Baryshkov wrote:
> > > > On Tue, 25 Jun 2024 at 01:56, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 6/24/2024 3:46 PM, Dmitry Baryshkov wrote:
> > > > > > On Tue, 25 Jun 2024 at 01:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > > > > >>
> > > > > >> + IGT dev
> > > > > >>
> > > > > >> On 6/22/2024 10:40 PM, Dmitry Baryshkov wrote:
> > > > > >>> With the introduction of the HDMI Connector framework the driver might
> > > > > >>> end up creating the max_bpc property with min = max = 8. IGT insists
> > > > > >>> that such properties carry the 'immutable' flag. Automatically set the
> > > > > >>> flag if the driver asks for the max_bpc property with min == max.
> > > > > >>>
> > > > > >>
> > > > > >> This change does not look right to me.
> > > > > >>
> > > > > >> I wonder why we need this check because DRM_MODE_PROP_IMMUTABLE means
> > > > > >> that as per the doc, userspace cannot change the property.
> > > > > >>
> > > > > >>            * DRM_MODE_PROP_IMMUTABLE
> > > > > >>            *     Set for properties whose values cannot be changed by
> > > > > >>            *     userspace. The kernel is allowed to update the value of
> > > > > >> these
> > > > > >>            *     properties. This is generally used to expose probe state to
> > > > > >>            *     userspace, e.g. the EDID, or the connector path property
> > > > > >> on DP
> > > > > >>            *     MST sinks. Kernel can update the value of an immutable
> > > > > >> property
> > > > > >>            *     by calling drm_object_property_set_value().
> > > > > >>            */
> > > > > >>
> > > > > >> Here we are allowing userspace to change max_bpc
> > > > > >>
> > > > > >>
> > > > > >> drm_atomic_connector_set_property()
> > > > > >> {
> > > > > >>          **********
> > > > > >>
> > > > > >>           } else if (property == connector->max_bpc_property) {
> > > > > >>                   state->max_requested_bpc = val;
> > > > > >>
> > > > > >>          **********
> > > > > >> }
> > > > > >>
> > > > > >> I believe you are referring to this IGT check right?
> > > > > >>
> > > > > >> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_properties.c#L428
> > > > > >
> > > > > > Yes
> > > > > >
> > > > > >>
> > > > > >> I think we should fix IGT in this case unless there is some reason we
> > > > > >> are missing. Because just because it has the same min and max does not
> > > > > >> mean its immutable by the doc of the IMMUTABLE flag.
> > > > > >
> > > > > > Well, having the same min and max means that it is impossible to
> > > > > > change the property. So the property is immutable, but doesn't have
> > > > > > the flag.
> > > > > >
> > > > >
> > > > > True, then does DRM_MODE_PROP_IMMUTABLE need a doc update too indicating
> > > > > that even if the min and max is same, property will be interpreted as
> > > > > immutable.
> > > >
> > > > Granted that I'm only doing it for max_bpc property I don't think so.
> > >
> > > Yeah, I have to agree with Abhinav here, it does look fishy to me too,
> > > even more so that it's only ever "documented" through an igt routine
> > > that has never documented why we want that.
> > >
> > > I'm fine with the change if it's indeed what we expect, and it might
> > > very well be, but I'd like to clear that up and document it first.
> > 
> > Should I also move the setting of the IMMUTABLE flag to a more generic code?
> 
> Possibly, but I guess that will depend on the outcome of that discussion

Agreed, I'll post it later today. Could you please ack or comment the first patch?
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index ab6ab7ff7ea8..33847fd63628 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2610,7 +2610,12 @@  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 
 	prop = connector->max_bpc_property;
 	if (!prop) {
-		prop = drm_property_create_range(dev, 0, "max bpc", min, max);
+		u32 flags = 0;
+
+		if (min == max)
+			flags |= DRM_MODE_PROP_IMMUTABLE;
+
+		prop = drm_property_create_range(dev, flags, "max bpc", min, max);
 		if (!prop)
 			return -ENOMEM;