[PATCH v2 0/2] drm: fix two issues related to HDMI Connector implementation

[PATCH v2 0/2] drm: fix two issues related to HDMI Connector implementation

[Dmitry Baryshkov]

Running IGT tests on Qualcomm Dragonboard820c uncovered two issues with
the HDMI Connector implementation and with its integration into the
drm_bridge_connector. Fix those issues.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Changes in v2:
- Actually pass the flags to drm_property_create_range().
- Link to v1: https://lore.kernel.org/r/20240623-drm-bridge-connector-fix-hdmi-reset-v1-0-41e9894dcdec@linaro.org

---
Dmitry Baryshkov (2):
      drm/bridge-connector: reset the HDMI connector state
      drm/connector: automatically set immutable flag for max_bpc property

 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 21 ---------------------
 drivers/gpu/drm/drm_atomic_state_helper.c       | 21 +++++++++++++++++++++
 drivers/gpu/drm/drm_bridge_connector.c          | 13 ++++++++++++-
 drivers/gpu/drm/drm_connector.c                 |  7 ++++++-
 include/drm/display/drm_hdmi_state_helper.h     |  3 ---
 include/drm/drm_atomic_state_helper.h           |  2 ++
 6 files changed, 41 insertions(+), 26 deletions(-)
---
base-commit: 2102cb0d050d34d50b9642a3a50861787527e922
change-id: 20240623-drm-bridge-connector-fix-hdmi-reset-0ce86af053aa

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH v2 1/2] drm/bridge-connector: reset the HDMI connector state

[Dmitry Baryshkov]

On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI
IGT chokes on the max_bpc property in several kms_properties tests due
to the the drm_bridge_connector failing to reset HDMI-related
properties.

Call __drm_atomic_helper_connector_hdmi_reset() if there is a
the drm_bridge_connector has bridge_hdmi.

Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to
drm_atomic_state_helper.c because drm_bridge_connector.c can not depend
on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to
call this function from HDMI bridges, there is is no function that
corresponds to the drm_connector_funcs::reset().

Fixes: 6b4468b0c6ba ("drm/bridge-connector: implement glue code for HDMI connector")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 21 ---------------------
 drivers/gpu/drm/drm_atomic_state_helper.c       | 21 +++++++++++++++++++++
 drivers/gpu/drm/drm_bridge_connector.c          | 13 ++++++++++++-
 include/drm/display/drm_hdmi_state_helper.h     |  3 ---
 include/drm/drm_atomic_state_helper.h           |  2 ++
 5 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index 2dab3ad8ce64..67f39857b0b4 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -8,27 +8,6 @@
 #include <drm/display/drm_hdmi_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
 
-/**
- * __drm_atomic_helper_connector_hdmi_reset() - Initializes all HDMI @drm_connector_state resources
- * @connector: DRM connector
- * @new_conn_state: connector state to reset
- *
- * Initializes all HDMI resources from a @drm_connector_state without
- * actually allocating it. This is useful for HDMI drivers, in
- * combination with __drm_atomic_helper_connector_reset() or
- * drm_atomic_helper_connector_reset().
- */
-void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector,
-					      struct drm_connector_state *new_conn_state)
-{
-	unsigned int max_bpc = connector->max_bpc;
-
-	new_conn_state->max_bpc = max_bpc;
-	new_conn_state->max_requested_bpc = max_bpc;
-	new_conn_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO;
-}
-EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset);
-
 static const struct drm_display_mode *
 connector_state_get_mode(const struct drm_connector_state *conn_state)
 {
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 519228eb1095..1518ada81b45 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -478,6 +478,27 @@ void drm_atomic_helper_connector_reset(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
 
+/**
+ * __drm_atomic_helper_connector_hdmi_reset() - Initializes all HDMI @drm_connector_state resources
+ * @connector: DRM connector
+ * @new_conn_state: connector state to reset
+ *
+ * Initializes all HDMI resources from a @drm_connector_state without
+ * actually allocating it. This is useful for HDMI drivers, in
+ * combination with __drm_atomic_helper_connector_reset() or
+ * drm_atomic_helper_connector_reset().
+ */
+void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector,
+					      struct drm_connector_state *new_conn_state)
+{
+	unsigned int max_bpc = connector->max_bpc;
+
+	new_conn_state->max_bpc = max_bpc;
+	new_conn_state->max_requested_bpc = max_bpc;
+	new_conn_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO;
+}
+EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset);
+
 /**
  * drm_atomic_helper_connector_tv_margins_reset - Resets TV connector properties
  * @connector: DRM connector
diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
index 0869b663f17e..7ebb35438459 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -216,8 +216,19 @@ static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
 	}
 }
 
+static void drm_bridge_connector_reset(struct drm_connector *connector)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+
+	drm_atomic_helper_connector_reset(connector);
+	if (bridge_connector->bridge_hdmi)
+		__drm_atomic_helper_connector_hdmi_reset(connector,
+							 connector->state);
+}
+
 static const struct drm_connector_funcs drm_bridge_connector_funcs = {
-	.reset = drm_atomic_helper_connector_reset,
+	.reset = drm_bridge_connector_reset,
 	.detect = drm_bridge_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
diff --git a/include/drm/display/drm_hdmi_state_helper.h b/include/drm/display/drm_hdmi_state_helper.h
index 285f366cf716..454319b40f17 100644
--- a/include/drm/display/drm_hdmi_state_helper.h
+++ b/include/drm/display/drm_hdmi_state_helper.h
@@ -8,9 +8,6 @@ struct drm_connector;
 struct drm_connector_state;
 struct hdmi_audio_infoframe;
 
-void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector,
-					      struct drm_connector_state *new_conn_state);
-
 int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
 					   struct drm_atomic_state *state);
 
diff --git a/include/drm/drm_atomic_state_helper.h b/include/drm/drm_atomic_state_helper.h
index b9740edb2658..adf300e6b4eb 100644
--- a/include/drm/drm_atomic_state_helper.h
+++ b/include/drm/drm_atomic_state_helper.h
@@ -71,6 +71,8 @@ void __drm_atomic_helper_connector_state_reset(struct drm_connector_state *conn_
 void __drm_atomic_helper_connector_reset(struct drm_connector *connector,
 					 struct drm_connector_state *conn_state);
 void drm_atomic_helper_connector_reset(struct drm_connector *connector);
+void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector,
+					      struct drm_connector_state *new_conn_state);
 void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector);
 int drm_atomic_helper_connector_tv_check(struct drm_connector *connector,
 					 struct drm_atomic_state *state);

-- 
2.39.2

Re: [PATCH v2 1/2] drm/bridge-connector: reset the HDMI connector state

[Abhinav Kumar]

On 6/22/2024 10:40 PM, Dmitry Baryshkov wrote:
> On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI
> IGT chokes on the max_bpc property in several kms_properties tests due
> to the the drm_bridge_connector failing to reset HDMI-related
> properties.
> 
> Call __drm_atomic_helper_connector_hdmi_reset() if there is a
> the drm_bridge_connector has bridge_hdmi.
> 
> Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to
> drm_atomic_state_helper.c because drm_bridge_connector.c can not depend
> on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to
> call this function from HDMI bridges, there is is no function that
> corresponds to the drm_connector_funcs::reset().
> 
> Fixes: 6b4468b0c6ba ("drm/bridge-connector: implement glue code for HDMI connector")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/display/drm_hdmi_state_helper.c | 21 ---------------------
>   drivers/gpu/drm/drm_atomic_state_helper.c       | 21 +++++++++++++++++++++
>   drivers/gpu/drm/drm_bridge_connector.c          | 13 ++++++++++++-
>   include/drm/display/drm_hdmi_state_helper.h     |  3 ---
>   include/drm/drm_atomic_state_helper.h           |  2 ++
>   5 files changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 2dab3ad8ce64..67f39857b0b4 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -8,27 +8,6 @@
>   #include <drm/display/drm_hdmi_helper.h>
>   #include <drm/display/drm_hdmi_state_helper.h>
>   
> -/**
> - * __drm_atomic_helper_connector_hdmi_reset() - Initializes all HDMI @drm_connector_state resources
> - * @connector: DRM connector
> - * @new_conn_state: connector state to reset
> - *
> - * Initializes all HDMI resources from a @drm_connector_state without
> - * actually allocating it. This is useful for HDMI drivers, in
> - * combination with __drm_atomic_helper_connector_reset() or
> - * drm_atomic_helper_connector_reset().
> - */
> -void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector,
> -					      struct drm_connector_state *new_conn_state)
> -{
> -	unsigned int max_bpc = connector->max_bpc;
> -
> -	new_conn_state->max_bpc = max_bpc;
> -	new_conn_state->max_requested_bpc = max_bpc;
> -	new_conn_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO;
> -}
> -EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset);
> -
>   static const struct drm_display_mode *
>   connector_state_get_mode(const struct drm_connector_state *conn_state)
>   {
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 519228eb1095..1518ada81b45 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -478,6 +478,27 @@ void drm_atomic_helper_connector_reset(struct drm_connector *connector)
>   }
>   EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
>   
> +/**
> + * __drm_atomic_helper_connector_hdmi_reset() - Initializes all HDMI @drm_connector_state resources
> + * @connector: DRM connector
> + * @new_conn_state: connector state to reset
> + *
> + * Initializes all HDMI resources from a @drm_connector_state without
> + * actually allocating it. This is useful for HDMI drivers, in
> + * combination with __drm_atomic_helper_connector_reset() or
> + * drm_atomic_helper_connector_reset().
> + */
> +void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector,
> +					      struct drm_connector_state *new_conn_state)
> +{
> +	unsigned int max_bpc = connector->max_bpc;
> +
> +	new_conn_state->max_bpc = max_bpc;
> +	new_conn_state->max_requested_bpc = max_bpc;

I understand this is just code propagation but do we need a max_bpc 
local variable?

We can just do

new_conn_state->max_bpc = connector->max_bpc;
new_conn_state->max_requested_bpc = connector->max_bpc;

But apart from that nit, this LGTM.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH v2 1/2] drm/bridge-connector: reset the HDMI connector state

[Dmitry Baryshkov]

On Tue, 25 Jun 2024 at 01:28, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/22/2024 10:40 PM, Dmitry Baryshkov wrote:
> > On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI
> > IGT chokes on the max_bpc property in several kms_properties tests due
> > to the the drm_bridge_connector failing to reset HDMI-related
> > properties.
> >
> > Call __drm_atomic_helper_connector_hdmi_reset() if there is a
> > the drm_bridge_connector has bridge_hdmi.
> >
> > Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to
> > drm_atomic_state_helper.c because drm_bridge_connector.c can not depend
> > on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to
> > call this function from HDMI bridges, there is is no function that
> > corresponds to the drm_connector_funcs::reset().
> >
> > Fixes: 6b4468b0c6ba ("drm/bridge-connector: implement glue code for HDMI connector")
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/display/drm_hdmi_state_helper.c | 21 ---------------------
> >   drivers/gpu/drm/drm_atomic_state_helper.c       | 21 +++++++++++++++++++++
> >   drivers/gpu/drm/drm_bridge_connector.c          | 13 ++++++++++++-
> >   include/drm/display/drm_hdmi_state_helper.h     |  3 ---
> >   include/drm/drm_atomic_state_helper.h           |  2 ++
> >   5 files changed, 35 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > index 2dab3ad8ce64..67f39857b0b4 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > @@ -8,27 +8,6 @@
> >   #include <drm/display/drm_hdmi_helper.h>
> >   #include <drm/display/drm_hdmi_state_helper.h>
> >
> > -/**
> > - * __drm_atomic_helper_connector_hdmi_reset() - Initializes all HDMI @drm_connector_state resources
> > - * @connector: DRM connector
> > - * @new_conn_state: connector state to reset
> > - *
> > - * Initializes all HDMI resources from a @drm_connector_state without
> > - * actually allocating it. This is useful for HDMI drivers, in
> > - * combination with __drm_atomic_helper_connector_reset() or
> > - * drm_atomic_helper_connector_reset().
> > - */
> > -void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector,
> > -                                           struct drm_connector_state *new_conn_state)
> > -{
> > -     unsigned int max_bpc = connector->max_bpc;
> > -
> > -     new_conn_state->max_bpc = max_bpc;
> > -     new_conn_state->max_requested_bpc = max_bpc;
> > -     new_conn_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO;
> > -}
> > -EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset);
> > -
> >   static const struct drm_display_mode *
> >   connector_state_get_mode(const struct drm_connector_state *conn_state)
> >   {
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index 519228eb1095..1518ada81b45 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -478,6 +478,27 @@ void drm_atomic_helper_connector_reset(struct drm_connector *connector)
> >   }
> >   EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
> >
> > +/**
> > + * __drm_atomic_helper_connector_hdmi_reset() - Initializes all HDMI @drm_connector_state resources
> > + * @connector: DRM connector
> > + * @new_conn_state: connector state to reset
> > + *
> > + * Initializes all HDMI resources from a @drm_connector_state without
> > + * actually allocating it. This is useful for HDMI drivers, in
> > + * combination with __drm_atomic_helper_connector_reset() or
> > + * drm_atomic_helper_connector_reset().
> > + */
> > +void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector,
> > +                                           struct drm_connector_state *new_conn_state)
> > +{
> > +     unsigned int max_bpc = connector->max_bpc;
> > +
> > +     new_conn_state->max_bpc = max_bpc;
> > +     new_conn_state->max_requested_bpc = max_bpc;
>
> I understand this is just code propagation but do we need a max_bpc
> local variable?
>
> We can just do
>
> new_conn_state->max_bpc = connector->max_bpc;
> new_conn_state->max_requested_bpc = connector->max_bpc;

Possible implementation, but I was really just doing code move.

>
> But apart from that nit, this LGTM.
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>



-- 
With best wishes
Dmitry

Re: [PATCH v2 1/2] drm/bridge-connector: reset the HDMI connector state

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

Hi,

On Sun, Jun 23, 2024 at 08:40:12AM GMT, Dmitry Baryshkov wrote:
> On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI
> IGT chokes on the max_bpc property in several kms_properties tests due
> to the the drm_bridge_connector failing to reset HDMI-related
> properties.
> 
> Call __drm_atomic_helper_connector_hdmi_reset() if there is a
> the drm_bridge_connector has bridge_hdmi.
> 
> Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to
> drm_atomic_state_helper.c because drm_bridge_connector.c can not depend
> on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to
> call this function from HDMI bridges, there is is no function that
> corresponds to the drm_connector_funcs::reset().

Why can't it depend on DRM_DISPLAY_HDMI_STATE_HELPER?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] drm/bridge-connector: reset the HDMI connector state

[Dmitry Baryshkov]

On Tue, 25 Jun 2024 at 18:02, Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Sun, Jun 23, 2024 at 08:40:12AM GMT, Dmitry Baryshkov wrote:
> > On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI
> > IGT chokes on the max_bpc property in several kms_properties tests due
> > to the the drm_bridge_connector failing to reset HDMI-related
> > properties.
> >
> > Call __drm_atomic_helper_connector_hdmi_reset() if there is a
> > the drm_bridge_connector has bridge_hdmi.
> >
> > Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to
> > drm_atomic_state_helper.c because drm_bridge_connector.c can not depend
> > on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to
> > call this function from HDMI bridges, there is is no function that
> > corresponds to the drm_connector_funcs::reset().
>
> Why can't it depend on DRM_DISPLAY_HDMI_STATE_HELPER?

Is it okay to have DRM_KMS_HELPER to depend unconditionally or select
DRM_DISPLAY_HDMI_STATE_HELPER?

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/2] drm/bridge-connector: reset the HDMI connector state

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

On Tue, Jun 25, 2024 at 06:05:33PM GMT, Dmitry Baryshkov wrote:
> On Tue, 25 Jun 2024 at 18:02, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > Hi,
> >
> > On Sun, Jun 23, 2024 at 08:40:12AM GMT, Dmitry Baryshkov wrote:
> > > On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI
> > > IGT chokes on the max_bpc property in several kms_properties tests due
> > > to the the drm_bridge_connector failing to reset HDMI-related
> > > properties.
> > >
> > > Call __drm_atomic_helper_connector_hdmi_reset() if there is a
> > > the drm_bridge_connector has bridge_hdmi.
> > >
> > > Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to
> > > drm_atomic_state_helper.c because drm_bridge_connector.c can not depend
> > > on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to
> > > call this function from HDMI bridges, there is is no function that
> > > corresponds to the drm_connector_funcs::reset().
> >
> > Why can't it depend on DRM_DISPLAY_HDMI_STATE_HELPER?
> 
> Is it okay to have DRM_KMS_HELPER to depend unconditionally or select
> DRM_DISPLAY_HDMI_STATE_HELPER?

No, but it's not clear to me why drm_bridge_connector is part of
DRM_KMS_HELPER? It doesn't seem to be called from the core but only
drivers, just like DRM_PANEL_BRIDGE which has a separate config option

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] drm/bridge-connector: reset the HDMI connector state

[Dmitry Baryshkov]

On Tue, Jun 25, 2024 at 05:49:54PM GMT, Maxime Ripard wrote:
> On Tue, Jun 25, 2024 at 06:05:33PM GMT, Dmitry Baryshkov wrote:
> > On Tue, 25 Jun 2024 at 18:02, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > Hi,
> > >
> > > On Sun, Jun 23, 2024 at 08:40:12AM GMT, Dmitry Baryshkov wrote:
> > > > On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI
> > > > IGT chokes on the max_bpc property in several kms_properties tests due
> > > > to the the drm_bridge_connector failing to reset HDMI-related
> > > > properties.
> > > >
> > > > Call __drm_atomic_helper_connector_hdmi_reset() if there is a
> > > > the drm_bridge_connector has bridge_hdmi.
> > > >
> > > > Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to
> > > > drm_atomic_state_helper.c because drm_bridge_connector.c can not depend
> > > > on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to
> > > > call this function from HDMI bridges, there is is no function that
> > > > corresponds to the drm_connector_funcs::reset().
> > >
> > > Why can't it depend on DRM_DISPLAY_HDMI_STATE_HELPER?
> > 
> > Is it okay to have DRM_KMS_HELPER to depend unconditionally or select
> > DRM_DISPLAY_HDMI_STATE_HELPER?
> 
> No, but it's not clear to me why drm_bridge_connector is part of
> DRM_KMS_HELPER? It doesn't seem to be called from the core but only
> drivers, just like DRM_PANEL_BRIDGE which has a separate config option

But then DRM_PANEL_BRIDGE is also linked into drm_kms_helper.ko. I can't
really say that I understand the definition of various helper modules,
but my taste tells me that code from drm/*.c should not depend directly
on the code from drm/display/*.c.

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/2] drm/bridge-connector: reset the HDMI connector state

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2123 bytes --]

On Tue, Jun 25, 2024 at 07:38:22PM GMT, Dmitry Baryshkov wrote:
> On Tue, Jun 25, 2024 at 05:49:54PM GMT, Maxime Ripard wrote:
> > On Tue, Jun 25, 2024 at 06:05:33PM GMT, Dmitry Baryshkov wrote:
> > > On Tue, 25 Jun 2024 at 18:02, Maxime Ripard <mripard@kernel.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Sun, Jun 23, 2024 at 08:40:12AM GMT, Dmitry Baryshkov wrote:
> > > > > On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI
> > > > > IGT chokes on the max_bpc property in several kms_properties tests due
> > > > > to the the drm_bridge_connector failing to reset HDMI-related
> > > > > properties.
> > > > >
> > > > > Call __drm_atomic_helper_connector_hdmi_reset() if there is a
> > > > > the drm_bridge_connector has bridge_hdmi.
> > > > >
> > > > > Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to
> > > > > drm_atomic_state_helper.c because drm_bridge_connector.c can not depend
> > > > > on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to
> > > > > call this function from HDMI bridges, there is is no function that
> > > > > corresponds to the drm_connector_funcs::reset().
> > > >
> > > > Why can't it depend on DRM_DISPLAY_HDMI_STATE_HELPER?
> > > 
> > > Is it okay to have DRM_KMS_HELPER to depend unconditionally or select
> > > DRM_DISPLAY_HDMI_STATE_HELPER?
> > 
> > No, but it's not clear to me why drm_bridge_connector is part of
> > DRM_KMS_HELPER? It doesn't seem to be called from the core but only
> > drivers, just like DRM_PANEL_BRIDGE which has a separate config option
> 
> But then DRM_PANEL_BRIDGE is also linked into drm_kms_helper.ko.

And we don't seem to have any reason for that, it could be a separate
module afaik.

> I can't really say that I understand the definition of various helper
> modules, but my taste tells me that code from drm/*.c should not
> depend directly on the code from drm/display/*.c.

Sure, but the bridge connector could be under bridge just as well, or
part of the display helpers.

It's really a "leaf" driver as far as helpers are concerned.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Dmitry Baryshkov]

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(-)

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;
 

-- 
2.39.2

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

[Abhinav Kumar]

+ 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;
>   
> 

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

[Dmitry Baryshkov]

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

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

[Abhinav Kumar]

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

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

[Dmitry Baryshkov]

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



-- 
With best wishes
Dmitry

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3062 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Dmitry Baryshkov]

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?

-- 
With best wishes
Dmitry

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3671 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Dmitry Baryshkov]

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?

-- 
With best wishes
Dmitry