[PATCH v5 0/9] drm/msm: make use of the HDMI connector infrastructure

[PATCH v5 0/9] drm/msm: make use of the HDMI connector infrastructure

[Dmitry Baryshkov]

This patchset sits on top Maxime's HDMI connector patchset ([1]).

Currently this is an RFC exploring the interface between HDMI bridges
and HDMI connector code. This has been lightly verified on the Qualcomm
DB820c, which has native HDMI output. If this approach is considered to
be acceptable, I'll finish MSM HDMI bridge conversion (reworking the
Audio Infoframe code). Other bridges can follow the same approach (we
have lt9611 / lt9611uxc / adv7511 on Qualcomm hardware).

[1] https://patchwork.freedesktop.org/series/122421/

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Changes in v5:
- Made drm_bridge_funcs::hdmi_clear_infoframe() callback mandatory for
  DRM_BRIDGE_OP_HDMI bridges (Maxime)
- Added drm_atomic_helper_connector_hdmi_disable_audio_infoframe()
  instead of passing NULL frame to
  drm_atomic_helper_connector_hdmi_update_audio_infoframe() (Maxime)
- Disable Audio Infoframe in MSM HDMI driver on audio shutdown.
- Link to v4: https://lore.kernel.org/r/20240531-bridge-hdmi-connector-v4-0-5110f7943622@linaro.org

Changes in v4:
- Reworked drm_bridge_connector functions to remove ternary operators
  and to reduce indentation level (Maxime)
- Added hdmi_ prefix to all HDMI-related drm_bridge_funcs calls (Maxime)
- Made vendor and product mandatory to the HDMI bridges (Maxime)
- Documented that max_bpc can be 8, 10 or 12 (Maxime)
- Changed hdmi->pixclock to be set using tmds_char_rate instead of
  calculating that manually (Maxime)
- Changed mode_valid to use helper function instead of manually
  doing mode->clock * 1000
- Removed hdmi_mode in favour of connector->display_info.is_hdmi
- Link to v3: https://lore.kernel.org/r/20240530-bridge-hdmi-connector-v3-0-a1d184d68fe3@linaro.org

Changes in v3:
- Rebased on top of the merged HDMI connector patchset.
- Changed drm_bridge_connector to use drmm_connector_init() (Maxime)
- Added a check that write_infoframe callback is present if
  BRIDGE_OP_HDMI is set.
- Moved drm_atomic_helper_connector_hdmi_check() call from
  drm_bridge_connector to the HDMI bridge driver to remove dependency
  from drm_kms_helpers on the optional HDMI state helpers.
- Converted Audio InfoFrame handling code.
- Added support for HDMI Vendor Specific and SPD InfoFrames.
- Link to v2: https://lore.kernel.org/r/20240309-bridge-hdmi-connector-v2-0-1380bea3ee70@linaro.org

Changes in v2:
- Dropped drm_connector_hdmi_setup(). Instead added
  drm_connector_hdmi_init() to be used by drm_bridge_connector.
- Changed the drm_bridge_connector to act as a proxy for the HDMI
  connector  infrastructure. This removes most of the logic from
  the bridge drivers.
- Link to v1: https://lore.kernel.org/r/20240308-bridge-hdmi-connector-v1-0-90b693550260@linaro.org

---
Dmitry Baryshkov (9):
      drm/connector: hdmi: allow disabling Audio Infoframe
      drm/bridge-connector: switch to using drmm allocations
      drm/bridge-connector: implement glue code for HDMI connector
      drm/msm/hdmi: switch to atomic bridge callbacks
      drm/msm/hdmi: turn mode_set into atomic_enable
      drm/msm/hdmi: make use of the drm_connector_hdmi framework
      drm/msm/hdmi: get rid of hdmi_mode
      drm/msm/hdmi: update HDMI_GEN_PKT_CTRL_GENERIC0_UPDATE definition
      drm/msm/hdmi: also send the SPD and HDMI Vendor Specific InfoFrames

 drivers/gpu/drm/display/drm_hdmi_state_helper.c |  36 +++
 drivers/gpu/drm/drm_bridge_connector.c          | 107 ++++++--
 drivers/gpu/drm/drm_debugfs.c                   |   2 +
 drivers/gpu/drm/msm/Kconfig                     |   2 +
 drivers/gpu/drm/msm/hdmi/hdmi.c                 |  47 +---
 drivers/gpu/drm/msm/hdmi/hdmi.h                 |  18 +-
 drivers/gpu/drm/msm/hdmi/hdmi_audio.c           |  74 ++----
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c          | 310 +++++++++++++++++++-----
 drivers/gpu/drm/msm/registers/display/hdmi.xml  |   2 +-
 include/drm/display/drm_hdmi_state_helper.h     |   1 +
 include/drm/drm_bridge.h                        |  81 +++++++
 11 files changed, 496 insertions(+), 184 deletions(-)
---
base-commit: 222d50ef3700aefb694e55a7a1f03d3fe2cb67f9
change-id: 20240307-bridge-hdmi-connector-7e3754e661d0

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

[PATCH v5 1/9] drm/connector: hdmi: allow disabling Audio Infoframe

[Dmitry Baryshkov]

Add drm_atomic_helper_connector_hdmi_disable_audio_infoframe(), an API
to allow the driver disable sending the Audio Infoframe. This is to be
used by the drivers if setup of the infoframes is not tightly coupled
with the audio functionality and just disabling the audio playback
doesn't stop the HDMI hardware from sending the Infoframe.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 36 +++++++++++++++++++++++++
 include/drm/display/drm_hdmi_state_helper.h     |  1 +
 2 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index ce96837eea65..731873b3bdf2 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -714,3 +714,39 @@ drm_atomic_helper_connector_hdmi_update_audio_infoframe(struct drm_connector *co
 	return ret;
 }
 EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_update_audio_infoframe);
+
+/**
+ * drm_atomic_helper_connector_hdmi_disable_audio_infoframe - Stop sending the Audio Infoframe
+ * @connector: A pointer to the HDMI connector
+ *
+ * This function is meant for HDMI connector drivers to stop sending their
+ * audio infoframe. It will typically be used in one of the ALSA hooks
+ * (most likely shutdown).
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int
+drm_atomic_helper_connector_hdmi_disable_audio_infoframe(struct drm_connector *connector)
+{
+	struct drm_connector_hdmi_infoframe *infoframe =
+		&connector->hdmi.infoframes.audio;
+	struct drm_display_info *info = &connector->display_info;
+	int ret;
+
+	if (!info->is_hdmi)
+		return 0;
+
+	mutex_lock(&connector->hdmi.infoframes.lock);
+
+	infoframe->set = false;
+
+	ret = clear_infoframe(connector, infoframe);
+
+	memset(&infoframe->data, 0, sizeof(infoframe->data));
+
+	mutex_unlock(&connector->hdmi.infoframes.lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_disable_audio_infoframe);
diff --git a/include/drm/display/drm_hdmi_state_helper.h b/include/drm/display/drm_hdmi_state_helper.h
index fbf86ff9cdfb..c3d23725f0b8 100644
--- a/include/drm/display/drm_hdmi_state_helper.h
+++ b/include/drm/display/drm_hdmi_state_helper.h
@@ -16,6 +16,7 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
 
 int drm_atomic_helper_connector_hdmi_update_audio_infoframe(struct drm_connector *connector,
 							    struct hdmi_audio_infoframe *frame);
+int drm_atomic_helper_connector_hdmi_disable_audio_infoframe(struct drm_connector *connector);
 int drm_atomic_helper_connector_hdmi_update_infoframes(struct drm_connector *connector,
 						       struct drm_atomic_state *state);
 

-- 
2.39.2

Re: [PATCH v5 1/9] drm/connector: hdmi: allow disabling Audio Infoframe

[Maxime Ripard]

On Fri, 7 Jun 2024 16:22:58 +0300, Dmitry Baryshkov wrote:
> Add drm_atomic_helper_connector_hdmi_disable_audio_infoframe(), an API
> to allow the driver disable sending the Audio Infoframe. This is to be
> used by the drivers if setup of the infoframes is not tightly coupled
> with the audio functionality and just disabling the audio playback
> doesn't stop the HDMI hardware from sending the Infoframe.
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v5 2/9] drm/bridge-connector: switch to using drmm allocations

[Dmitry Baryshkov]

Turn drm_bridge_connector to using drmm_kzalloc() and
drmm_connector_init() and drop the custom destroy function. The
drm_connector_unregister() and fwnode_handle_put() are already handled
by the drm_connector_cleanup() and so are safe to be dropped.

Acked-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
index 982552c9f92c..e093fc8928dc 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -15,6 +15,7 @@
 #include <drm/drm_connector.h>
 #include <drm/drm_device.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_probe_helper.h>
 
@@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force)
 	return status;
 }
 
-static void drm_bridge_connector_destroy(struct drm_connector *connector)
-{
-	struct drm_bridge_connector *bridge_connector =
-		to_drm_bridge_connector(connector);
-
-	drm_connector_unregister(connector);
-	drm_connector_cleanup(connector);
-
-	fwnode_handle_put(connector->fwnode);
-
-	kfree(bridge_connector);
-}
-
 static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
 					      struct dentry *root)
 {
@@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
 	.detect = drm_bridge_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = drm_bridge_connector_destroy,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 	.debugfs_init = drm_bridge_connector_debugfs_init,
@@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 	int connector_type;
 	int ret;
 
-	bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
+	bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL);
 	if (!bridge_connector)
 		return ERR_PTR(-ENOMEM);
 
@@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		return ERR_PTR(-EINVAL);
 	}
 
-	ret = drm_connector_init_with_ddc(drm, connector,
-					  &drm_bridge_connector_funcs,
-					  connector_type, ddc);
+	ret = drmm_connector_init(drm, connector,
+				  &drm_bridge_connector_funcs,
+				  connector_type, ddc);
 	if (ret) {
 		kfree(bridge_connector);
 		return ERR_PTR(ret);

-- 
2.39.2

Re: [PATCH v5 2/9] drm/bridge-connector: switch to using drmm allocations

[Maxime Ripard]

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

Hi,

On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote:
> Turn drm_bridge_connector to using drmm_kzalloc() and
> drmm_connector_init() and drop the custom destroy function. The
> drm_connector_unregister() and fwnode_handle_put() are already handled
> by the drm_connector_cleanup() and so are safe to be dropped.
> 
> Acked-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------
>  1 file changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> index 982552c9f92c..e093fc8928dc 100644
> --- a/drivers/gpu/drm/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/drm_bridge_connector.c
> @@ -15,6 +15,7 @@
>  #include <drm/drm_connector.h>
>  #include <drm/drm_device.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_modeset_helper_vtables.h>
>  #include <drm/drm_probe_helper.h>
>  
> @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force)
>  	return status;
>  }
>  
> -static void drm_bridge_connector_destroy(struct drm_connector *connector)
> -{
> -	struct drm_bridge_connector *bridge_connector =
> -		to_drm_bridge_connector(connector);
> -
> -	drm_connector_unregister(connector);
> -	drm_connector_cleanup(connector);
> -
> -	fwnode_handle_put(connector->fwnode);
> -
> -	kfree(bridge_connector);
> -}
> -
>  static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
>  					      struct dentry *root)
>  {
> @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = {
>  	.reset = drm_atomic_helper_connector_reset,
>  	.detect = drm_bridge_connector_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.destroy = drm_bridge_connector_destroy,
>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  	.debugfs_init = drm_bridge_connector_debugfs_init,
> @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  	int connector_type;
>  	int ret;
>  
> -	bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
> +	bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL);

So you make destroy's kfree call unnecessary here ...

>  	if (!bridge_connector)
>  		return ERR_PTR(-ENOMEM);
>  
> @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	ret = drm_connector_init_with_ddc(drm, connector,
> -					  &drm_bridge_connector_funcs,
> -					  connector_type, ddc);
> +	ret = drmm_connector_init(drm, connector,
> +				  &drm_bridge_connector_funcs,
> +				  connector_type, ddc);

... and here of drm_connector_cleanup.

drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to
connector->fwnode since you don't call fwnode_handle_put anymore.

We should register a drmm action right below the call to fwnode_handle_get too.

Maxime

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

Re: [PATCH v5 2/9] drm/bridge-connector: switch to using drmm allocations

[Dmitry Baryshkov]

On Mon, 10 Jun 2024 at 11:04, Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote:
> > Turn drm_bridge_connector to using drmm_kzalloc() and
> > drmm_connector_init() and drop the custom destroy function. The
> > drm_connector_unregister() and fwnode_handle_put() are already handled
> > by the drm_connector_cleanup() and so are safe to be dropped.
> >
> > Acked-by: Maxime Ripard <mripard@kernel.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------
> >  1 file changed, 5 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> > index 982552c9f92c..e093fc8928dc 100644
> > --- a/drivers/gpu/drm/drm_bridge_connector.c
> > +++ b/drivers/gpu/drm/drm_bridge_connector.c
> > @@ -15,6 +15,7 @@
> >  #include <drm/drm_connector.h>
> >  #include <drm/drm_device.h>
> >  #include <drm/drm_edid.h>
> > +#include <drm/drm_managed.h>
> >  #include <drm/drm_modeset_helper_vtables.h>
> >  #include <drm/drm_probe_helper.h>
> >
> > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force)
> >       return status;
> >  }
> >
> > -static void drm_bridge_connector_destroy(struct drm_connector *connector)
> > -{
> > -     struct drm_bridge_connector *bridge_connector =
> > -             to_drm_bridge_connector(connector);
> > -
> > -     drm_connector_unregister(connector);
> > -     drm_connector_cleanup(connector);
> > -
> > -     fwnode_handle_put(connector->fwnode);
> > -
> > -     kfree(bridge_connector);
> > -}
> > -
> >  static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
> >                                             struct dentry *root)
> >  {
> > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = {
> >       .reset = drm_atomic_helper_connector_reset,
> >       .detect = drm_bridge_connector_detect,
> >       .fill_modes = drm_helper_probe_single_connector_modes,
> > -     .destroy = drm_bridge_connector_destroy,
> >       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> >       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> >       .debugfs_init = drm_bridge_connector_debugfs_init,
> > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> >       int connector_type;
> >       int ret;
> >
> > -     bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
> > +     bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL);
>
> So you make destroy's kfree call unnecessary here ...
>
> >       if (!bridge_connector)
> >               return ERR_PTR(-ENOMEM);
> >
> > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> >               return ERR_PTR(-EINVAL);
> >       }
> >
> > -     ret = drm_connector_init_with_ddc(drm, connector,
> > -                                       &drm_bridge_connector_funcs,
> > -                                       connector_type, ddc);
> > +     ret = drmm_connector_init(drm, connector,
> > +                               &drm_bridge_connector_funcs,
> > +                               connector_type, ddc);
>
> ... and here of drm_connector_cleanup.
>
> drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to
> connector->fwnode since you don't call fwnode_handle_put anymore.
>
> We should register a drmm action right below the call to fwnode_handle_get too.

But drm_connector_cleanup() already contains
fwnode_handle_put(connector->fwnode). Isn't that enough?


-- 
With best wishes
Dmitry

Re: [PATCH v5 2/9] drm/bridge-connector: switch to using drmm allocations

[Maxime Ripard]

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

Hi,

+Hans

On Mon, Jun 10, 2024 at 02:46:03PM GMT, Dmitry Baryshkov wrote:
> On Mon, 10 Jun 2024 at 11:04, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > Hi,
> >
> > On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote:
> > > Turn drm_bridge_connector to using drmm_kzalloc() and
> > > drmm_connector_init() and drop the custom destroy function. The
> > > drm_connector_unregister() and fwnode_handle_put() are already handled
> > > by the drm_connector_cleanup() and so are safe to be dropped.
> > >
> > > Acked-by: Maxime Ripard <mripard@kernel.org>
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------
> > >  1 file changed, 5 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> > > index 982552c9f92c..e093fc8928dc 100644
> > > --- a/drivers/gpu/drm/drm_bridge_connector.c
> > > +++ b/drivers/gpu/drm/drm_bridge_connector.c
> > > @@ -15,6 +15,7 @@
> > >  #include <drm/drm_connector.h>
> > >  #include <drm/drm_device.h>
> > >  #include <drm/drm_edid.h>
> > > +#include <drm/drm_managed.h>
> > >  #include <drm/drm_modeset_helper_vtables.h>
> > >  #include <drm/drm_probe_helper.h>
> > >
> > > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force)
> > >       return status;
> > >  }
> > >
> > > -static void drm_bridge_connector_destroy(struct drm_connector *connector)
> > > -{
> > > -     struct drm_bridge_connector *bridge_connector =
> > > -             to_drm_bridge_connector(connector);
> > > -
> > > -     drm_connector_unregister(connector);
> > > -     drm_connector_cleanup(connector);
> > > -
> > > -     fwnode_handle_put(connector->fwnode);
> > > -
> > > -     kfree(bridge_connector);
> > > -}
> > > -
> > >  static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
> > >                                             struct dentry *root)
> > >  {
> > > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = {
> > >       .reset = drm_atomic_helper_connector_reset,
> > >       .detect = drm_bridge_connector_detect,
> > >       .fill_modes = drm_helper_probe_single_connector_modes,
> > > -     .destroy = drm_bridge_connector_destroy,
> > >       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > >       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > >       .debugfs_init = drm_bridge_connector_debugfs_init,
> > > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > >       int connector_type;
> > >       int ret;
> > >
> > > -     bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
> > > +     bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL);
> >
> > So you make destroy's kfree call unnecessary here ...
> >
> > >       if (!bridge_connector)
> > >               return ERR_PTR(-ENOMEM);
> > >
> > > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > >               return ERR_PTR(-EINVAL);
> > >       }
> > >
> > > -     ret = drm_connector_init_with_ddc(drm, connector,
> > > -                                       &drm_bridge_connector_funcs,
> > > -                                       connector_type, ddc);
> > > +     ret = drmm_connector_init(drm, connector,
> > > +                               &drm_bridge_connector_funcs,
> > > +                               connector_type, ddc);
> >
> > ... and here of drm_connector_cleanup.
> >
> > drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to
> > connector->fwnode since you don't call fwnode_handle_put anymore.
> >
> > We should register a drmm action right below the call to fwnode_handle_get too.
> 
> But drm_connector_cleanup() already contains
> fwnode_handle_put(connector->fwnode). Isn't that enough?

It does, but now I'm confused.

drm_bridge_connector_init takes a reference, drm_connector_init doesn't.
It will call drm_bridge_connector_destroy() that gives back its
reference (which makes sense to me), but then why do
drm_connector_cleanup() does? None of the drm_connector code even took
that reference, and we end up with a double-put.

It looks like it was introduced by commit 48c429c6d18d ("drm/connector:
Add a fwnode pointer to drm_connector and register with ACPI (v2)") from
Hans, which does call put, but never gets that reference.

It has nothing to do with this series anymore, but that's super fishy to
me, and the source of bugs as we can see here.

Maxime

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

Re: [PATCH v5 2/9] drm/bridge-connector: switch to using drmm allocations

[Dmitry Baryshkov]

On Mon, Jun 10, 2024 at 02:07:06PM +0200, Maxime Ripard wrote:
> Hi,
> 
> +Hans
> 
> On Mon, Jun 10, 2024 at 02:46:03PM GMT, Dmitry Baryshkov wrote:
> > On Mon, 10 Jun 2024 at 11:04, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote:
> > > > Turn drm_bridge_connector to using drmm_kzalloc() and
> > > > drmm_connector_init() and drop the custom destroy function. The
> > > > drm_connector_unregister() and fwnode_handle_put() are already handled
> > > > by the drm_connector_cleanup() and so are safe to be dropped.
> > > >
> > > > Acked-by: Maxime Ripard <mripard@kernel.org>
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >  drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------
> > > >  1 file changed, 5 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> > > > index 982552c9f92c..e093fc8928dc 100644
> > > > --- a/drivers/gpu/drm/drm_bridge_connector.c
> > > > +++ b/drivers/gpu/drm/drm_bridge_connector.c
> > > > @@ -15,6 +15,7 @@
> > > >  #include <drm/drm_connector.h>
> > > >  #include <drm/drm_device.h>
> > > >  #include <drm/drm_edid.h>
> > > > +#include <drm/drm_managed.h>
> > > >  #include <drm/drm_modeset_helper_vtables.h>
> > > >  #include <drm/drm_probe_helper.h>
> > > >
> > > > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force)
> > > >       return status;
> > > >  }
> > > >
> > > > -static void drm_bridge_connector_destroy(struct drm_connector *connector)
> > > > -{
> > > > -     struct drm_bridge_connector *bridge_connector =
> > > > -             to_drm_bridge_connector(connector);
> > > > -
> > > > -     drm_connector_unregister(connector);
> > > > -     drm_connector_cleanup(connector);
> > > > -
> > > > -     fwnode_handle_put(connector->fwnode);
> > > > -
> > > > -     kfree(bridge_connector);
> > > > -}
> > > > -
> > > >  static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
> > > >                                             struct dentry *root)
> > > >  {
> > > > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = {
> > > >       .reset = drm_atomic_helper_connector_reset,
> > > >       .detect = drm_bridge_connector_detect,
> > > >       .fill_modes = drm_helper_probe_single_connector_modes,
> > > > -     .destroy = drm_bridge_connector_destroy,
> > > >       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > > >       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > > >       .debugfs_init = drm_bridge_connector_debugfs_init,
> > > > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > > >       int connector_type;
> > > >       int ret;
> > > >
> > > > -     bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
> > > > +     bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL);
> > >
> > > So you make destroy's kfree call unnecessary here ...
> > >
> > > >       if (!bridge_connector)
> > > >               return ERR_PTR(-ENOMEM);
> > > >
> > > > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > > >               return ERR_PTR(-EINVAL);
> > > >       }
> > > >
> > > > -     ret = drm_connector_init_with_ddc(drm, connector,
> > > > -                                       &drm_bridge_connector_funcs,
> > > > -                                       connector_type, ddc);
> > > > +     ret = drmm_connector_init(drm, connector,
> > > > +                               &drm_bridge_connector_funcs,
> > > > +                               connector_type, ddc);
> > >
> > > ... and here of drm_connector_cleanup.
> > >
> > > drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to
> > > connector->fwnode since you don't call fwnode_handle_put anymore.
> > >
> > > We should register a drmm action right below the call to fwnode_handle_get too.
> > 
> > But drm_connector_cleanup() already contains
> > fwnode_handle_put(connector->fwnode). Isn't that enough?
> 
> It does, but now I'm confused.
> 
> drm_bridge_connector_init takes a reference, drm_connector_init doesn't.
> It will call drm_bridge_connector_destroy() that gives back its
> reference (which makes sense to me), but then why do
> drm_connector_cleanup() does? None of the drm_connector code even took
> that reference, and we end up with a double-put.
> 
> It looks like it was introduced by commit 48c429c6d18d ("drm/connector:
> Add a fwnode pointer to drm_connector and register with ACPI (v2)") from
> Hans, which does call put, but never gets that reference.

The mentioned patch documents that pretty clearly:

* Drivers can set this to associate a fwnode with a connector, drivers
* are expected to get a reference on the fwnode when setting this.
* drm_connector_cleanup() will call fwnode_handle_put() on this.

This is logical. Whoever sets the drm_connector::fwnode pointer, should
get reference. This way drm_connector_init() doesn't need to play with
the reference counting. The cleanup code drops the reference (so the
driver doesn't need to), because cleanup might be assynchronous..

The drm_bridge_connector follows this approach: it sets
drm_connector->fwnode, so it gets the reference. It uses
drm_connector_cleanup(), so it doesn't need to put it.

> 
> It has nothing to do with this series anymore, but that's super fishy to
> me, and the source of bugs as we can see here.

-- 
With best wishes
Dmitry

Re: [PATCH v5 2/9] drm/bridge-connector: switch to using drmm allocations

[Maxime Ripard]

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

On Mon, Jun 10, 2024 at 08:54:09PM GMT, Dmitry Baryshkov wrote:
> On Mon, Jun 10, 2024 at 02:07:06PM +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > +Hans
> > 
> > On Mon, Jun 10, 2024 at 02:46:03PM GMT, Dmitry Baryshkov wrote:
> > > On Mon, 10 Jun 2024 at 11:04, Maxime Ripard <mripard@kernel.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote:
> > > > > Turn drm_bridge_connector to using drmm_kzalloc() and
> > > > > drmm_connector_init() and drop the custom destroy function. The
> > > > > drm_connector_unregister() and fwnode_handle_put() are already handled
> > > > > by the drm_connector_cleanup() and so are safe to be dropped.
> > > > >
> > > > > Acked-by: Maxime Ripard <mripard@kernel.org>
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------
> > > > >  1 file changed, 5 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> > > > > index 982552c9f92c..e093fc8928dc 100644
> > > > > --- a/drivers/gpu/drm/drm_bridge_connector.c
> > > > > +++ b/drivers/gpu/drm/drm_bridge_connector.c
> > > > > @@ -15,6 +15,7 @@
> > > > >  #include <drm/drm_connector.h>
> > > > >  #include <drm/drm_device.h>
> > > > >  #include <drm/drm_edid.h>
> > > > > +#include <drm/drm_managed.h>
> > > > >  #include <drm/drm_modeset_helper_vtables.h>
> > > > >  #include <drm/drm_probe_helper.h>
> > > > >
> > > > > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force)
> > > > >       return status;
> > > > >  }
> > > > >
> > > > > -static void drm_bridge_connector_destroy(struct drm_connector *connector)
> > > > > -{
> > > > > -     struct drm_bridge_connector *bridge_connector =
> > > > > -             to_drm_bridge_connector(connector);
> > > > > -
> > > > > -     drm_connector_unregister(connector);
> > > > > -     drm_connector_cleanup(connector);
> > > > > -
> > > > > -     fwnode_handle_put(connector->fwnode);
> > > > > -
> > > > > -     kfree(bridge_connector);
> > > > > -}
> > > > > -
> > > > >  static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
> > > > >                                             struct dentry *root)
> > > > >  {
> > > > > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = {
> > > > >       .reset = drm_atomic_helper_connector_reset,
> > > > >       .detect = drm_bridge_connector_detect,
> > > > >       .fill_modes = drm_helper_probe_single_connector_modes,
> > > > > -     .destroy = drm_bridge_connector_destroy,
> > > > >       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > > > >       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > > > >       .debugfs_init = drm_bridge_connector_debugfs_init,
> > > > > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > > > >       int connector_type;
> > > > >       int ret;
> > > > >
> > > > > -     bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
> > > > > +     bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL);
> > > >
> > > > So you make destroy's kfree call unnecessary here ...
> > > >
> > > > >       if (!bridge_connector)
> > > > >               return ERR_PTR(-ENOMEM);
> > > > >
> > > > > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > > > >               return ERR_PTR(-EINVAL);
> > > > >       }
> > > > >
> > > > > -     ret = drm_connector_init_with_ddc(drm, connector,
> > > > > -                                       &drm_bridge_connector_funcs,
> > > > > -                                       connector_type, ddc);
> > > > > +     ret = drmm_connector_init(drm, connector,
> > > > > +                               &drm_bridge_connector_funcs,
> > > > > +                               connector_type, ddc);
> > > >
> > > > ... and here of drm_connector_cleanup.
> > > >
> > > > drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to
> > > > connector->fwnode since you don't call fwnode_handle_put anymore.
> > > >
> > > > We should register a drmm action right below the call to fwnode_handle_get too.
> > > 
> > > But drm_connector_cleanup() already contains
> > > fwnode_handle_put(connector->fwnode). Isn't that enough?
> > 
> > It does, but now I'm confused.
> > 
> > drm_bridge_connector_init takes a reference, drm_connector_init doesn't.
> > It will call drm_bridge_connector_destroy() that gives back its
> > reference (which makes sense to me), but then why do
> > drm_connector_cleanup() does? None of the drm_connector code even took
> > that reference, and we end up with a double-put.
> > 
> > It looks like it was introduced by commit 48c429c6d18d ("drm/connector:
> > Add a fwnode pointer to drm_connector and register with ACPI (v2)") from
> > Hans, which does call put, but never gets that reference.
> 
> The mentioned patch documents that pretty clearly:
> 
> * Drivers can set this to associate a fwnode with a connector, drivers
> * are expected to get a reference on the fwnode when setting this.
> * drm_connector_cleanup() will call fwnode_handle_put() on this.
> 
> This is logical. Whoever sets the drm_connector::fwnode pointer, should
> get reference. This way drm_connector_init() doesn't need to play with
> the reference counting. The cleanup code drops the reference (so the
> driver doesn't need to), because cleanup might be assynchronous..

Right, but it's the cleanup part that isn't logical. It makes total
sense to have the connector that sets connector->fwnode get the
reference itself. It doesn't make sense to have the core give that
reference instead of the driver.

It's confusing, because if the driver is supposed to handle its
reference itself, then it should handle all of it itself. This bug is
the testament for that: the natural approach is buggy.

> The drm_bridge_connector follows this approach: it sets
> drm_connector->fwnode, so it gets the reference. It uses
> drm_connector_cleanup(), so it doesn't need to put it.

Yet, it calls fwnode_handle_put in its destroy path, because it grabbed
a reference before.

Maxime

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

Re: [PATCH v5 2/9] drm/bridge-connector: switch to using drmm allocations

[Dmitry Baryshkov]

On Tue, 11 Jun 2024 at 11:54, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Mon, Jun 10, 2024 at 08:54:09PM GMT, Dmitry Baryshkov wrote:
> > On Mon, Jun 10, 2024 at 02:07:06PM +0200, Maxime Ripard wrote:
> > > Hi,
> > >
> > > +Hans
> > >
> > > On Mon, Jun 10, 2024 at 02:46:03PM GMT, Dmitry Baryshkov wrote:
> > > > On Mon, 10 Jun 2024 at 11:04, Maxime Ripard <mripard@kernel.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote:
> > > > > > Turn drm_bridge_connector to using drmm_kzalloc() and
> > > > > > drmm_connector_init() and drop the custom destroy function. The
> > > > > > drm_connector_unregister() and fwnode_handle_put() are already handled
> > > > > > by the drm_connector_cleanup() and so are safe to be dropped.
> > > > > >
> > > > > > Acked-by: Maxime Ripard <mripard@kernel.org>
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------
> > > > > >  1 file changed, 5 insertions(+), 18 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> > > > > > index 982552c9f92c..e093fc8928dc 100644
> > > > > > --- a/drivers/gpu/drm/drm_bridge_connector.c
> > > > > > +++ b/drivers/gpu/drm/drm_bridge_connector.c
> > > > > > @@ -15,6 +15,7 @@
> > > > > >  #include <drm/drm_connector.h>
> > > > > >  #include <drm/drm_device.h>
> > > > > >  #include <drm/drm_edid.h>
> > > > > > +#include <drm/drm_managed.h>
> > > > > >  #include <drm/drm_modeset_helper_vtables.h>
> > > > > >  #include <drm/drm_probe_helper.h>
> > > > > >
> > > > > > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force)
> > > > > >       return status;
> > > > > >  }
> > > > > >
> > > > > > -static void drm_bridge_connector_destroy(struct drm_connector *connector)
> > > > > > -{
> > > > > > -     struct drm_bridge_connector *bridge_connector =
> > > > > > -             to_drm_bridge_connector(connector);
> > > > > > -
> > > > > > -     drm_connector_unregister(connector);
> > > > > > -     drm_connector_cleanup(connector);
> > > > > > -
> > > > > > -     fwnode_handle_put(connector->fwnode);
> > > > > > -
> > > > > > -     kfree(bridge_connector);
> > > > > > -}
> > > > > > -
> > > > > >  static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
> > > > > >                                             struct dentry *root)
> > > > > >  {
> > > > > > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = {
> > > > > >       .reset = drm_atomic_helper_connector_reset,
> > > > > >       .detect = drm_bridge_connector_detect,
> > > > > >       .fill_modes = drm_helper_probe_single_connector_modes,
> > > > > > -     .destroy = drm_bridge_connector_destroy,
> > > > > >       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > > > > >       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > > > > >       .debugfs_init = drm_bridge_connector_debugfs_init,
> > > > > > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > > > > >       int connector_type;
> > > > > >       int ret;
> > > > > >
> > > > > > -     bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
> > > > > > +     bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL);
> > > > >
> > > > > So you make destroy's kfree call unnecessary here ...
> > > > >
> > > > > >       if (!bridge_connector)
> > > > > >               return ERR_PTR(-ENOMEM);
> > > > > >
> > > > > > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > > > > >               return ERR_PTR(-EINVAL);
> > > > > >       }
> > > > > >
> > > > > > -     ret = drm_connector_init_with_ddc(drm, connector,
> > > > > > -                                       &drm_bridge_connector_funcs,
> > > > > > -                                       connector_type, ddc);
> > > > > > +     ret = drmm_connector_init(drm, connector,
> > > > > > +                               &drm_bridge_connector_funcs,
> > > > > > +                               connector_type, ddc);
> > > > >
> > > > > ... and here of drm_connector_cleanup.
> > > > >
> > > > > drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to
> > > > > connector->fwnode since you don't call fwnode_handle_put anymore.
> > > > >
> > > > > We should register a drmm action right below the call to fwnode_handle_get too.
> > > >
> > > > But drm_connector_cleanup() already contains
> > > > fwnode_handle_put(connector->fwnode). Isn't that enough?
> > >
> > > It does, but now I'm confused.
> > >
> > > drm_bridge_connector_init takes a reference, drm_connector_init doesn't.
> > > It will call drm_bridge_connector_destroy() that gives back its
> > > reference (which makes sense to me), but then why do
> > > drm_connector_cleanup() does? None of the drm_connector code even took
> > > that reference, and we end up with a double-put.
> > >
> > > It looks like it was introduced by commit 48c429c6d18d ("drm/connector:
> > > Add a fwnode pointer to drm_connector and register with ACPI (v2)") from
> > > Hans, which does call put, but never gets that reference.
> >
> > The mentioned patch documents that pretty clearly:
> >
> > * Drivers can set this to associate a fwnode with a connector, drivers
> > * are expected to get a reference on the fwnode when setting this.
> > * drm_connector_cleanup() will call fwnode_handle_put() on this.
> >
> > This is logical. Whoever sets the drm_connector::fwnode pointer, should
> > get reference. This way drm_connector_init() doesn't need to play with
> > the reference counting. The cleanup code drops the reference (so the
> > driver doesn't need to), because cleanup might be assynchronous..
>
> Right, but it's the cleanup part that isn't logical. It makes total
> sense to have the connector that sets connector->fwnode get the
> reference itself. It doesn't make sense to have the core give that
> reference instead of the driver.
>
> It's confusing, because if the driver is supposed to handle its
> reference itself, then it should handle all of it itself. This bug is
> the testament for that: the natural approach is buggy.

I'd say this is the 'transfer of the ownership'. The base driver gets
the reference, and then gives it away to the drm_connecter. But indeed
this is not very intuitive.

I have looked at the original series by Hans/Heikky, but I don't seem
to be able to find a good way to solve that. The fwnode can be set
after initialising the drm_connector. And at the same time it doesn't
make so much sense to put that burden onto the driver. One option
might be to add drm_connector_set_fwnode() that will get the reference
internally, but that looks a bit like an overkill.

>
> > The drm_bridge_connector follows this approach: it sets
> > drm_connector->fwnode, so it gets the reference. It uses
> > drm_connector_cleanup(), so it doesn't need to put it.
>
> Yet, it calls fwnode_handle_put in its destroy path, because it grabbed
> a reference before.
>
> Maxime



-- 
With best wishes
Dmitry

Re: [PATCH v5 2/9] drm/bridge-connector: switch to using drmm allocations

[Maxime Ripard]

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

On Tue, Jun 11, 2024 at 02:26:12PM GMT, Dmitry Baryshkov wrote:
> On Tue, 11 Jun 2024 at 11:54, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Mon, Jun 10, 2024 at 08:54:09PM GMT, Dmitry Baryshkov wrote:
> > > On Mon, Jun 10, 2024 at 02:07:06PM +0200, Maxime Ripard wrote:
> > > > Hi,
> > > >
> > > > +Hans
> > > >
> > > > On Mon, Jun 10, 2024 at 02:46:03PM GMT, Dmitry Baryshkov wrote:
> > > > > On Mon, 10 Jun 2024 at 11:04, Maxime Ripard <mripard@kernel.org> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Fri, Jun 07, 2024 at 04:22:59PM GMT, Dmitry Baryshkov wrote:
> > > > > > > Turn drm_bridge_connector to using drmm_kzalloc() and
> > > > > > > drmm_connector_init() and drop the custom destroy function. The
> > > > > > > drm_connector_unregister() and fwnode_handle_put() are already handled
> > > > > > > by the drm_connector_cleanup() and so are safe to be dropped.
> > > > > > >
> > > > > > > Acked-by: Maxime Ripard <mripard@kernel.org>
> > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/drm_bridge_connector.c | 23 +++++------------------
> > > > > > >  1 file changed, 5 insertions(+), 18 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> > > > > > > index 982552c9f92c..e093fc8928dc 100644
> > > > > > > --- a/drivers/gpu/drm/drm_bridge_connector.c
> > > > > > > +++ b/drivers/gpu/drm/drm_bridge_connector.c
> > > > > > > @@ -15,6 +15,7 @@
> > > > > > >  #include <drm/drm_connector.h>
> > > > > > >  #include <drm/drm_device.h>
> > > > > > >  #include <drm/drm_edid.h>
> > > > > > > +#include <drm/drm_managed.h>
> > > > > > >  #include <drm/drm_modeset_helper_vtables.h>
> > > > > > >  #include <drm/drm_probe_helper.h>
> > > > > > >
> > > > > > > @@ -193,19 +194,6 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force)
> > > > > > >       return status;
> > > > > > >  }
> > > > > > >
> > > > > > > -static void drm_bridge_connector_destroy(struct drm_connector *connector)
> > > > > > > -{
> > > > > > > -     struct drm_bridge_connector *bridge_connector =
> > > > > > > -             to_drm_bridge_connector(connector);
> > > > > > > -
> > > > > > > -     drm_connector_unregister(connector);
> > > > > > > -     drm_connector_cleanup(connector);
> > > > > > > -
> > > > > > > -     fwnode_handle_put(connector->fwnode);
> > > > > > > -
> > > > > > > -     kfree(bridge_connector);
> > > > > > > -}
> > > > > > > -
> > > > > > >  static void drm_bridge_connector_debugfs_init(struct drm_connector *connector,
> > > > > > >                                             struct dentry *root)
> > > > > > >  {
> > > > > > > @@ -224,7 +212,6 @@ static const struct drm_connector_funcs drm_bridge_connector_funcs = {
> > > > > > >       .reset = drm_atomic_helper_connector_reset,
> > > > > > >       .detect = drm_bridge_connector_detect,
> > > > > > >       .fill_modes = drm_helper_probe_single_connector_modes,
> > > > > > > -     .destroy = drm_bridge_connector_destroy,
> > > > > > >       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > > > > > >       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > > > > > >       .debugfs_init = drm_bridge_connector_debugfs_init,
> > > > > > > @@ -328,7 +315,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > > > > > >       int connector_type;
> > > > > > >       int ret;
> > > > > > >
> > > > > > > -     bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
> > > > > > > +     bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL);
> > > > > >
> > > > > > So you make destroy's kfree call unnecessary here ...
> > > > > >
> > > > > > >       if (!bridge_connector)
> > > > > > >               return ERR_PTR(-ENOMEM);
> > > > > > >
> > > > > > > @@ -383,9 +370,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > > > > > >               return ERR_PTR(-EINVAL);
> > > > > > >       }
> > > > > > >
> > > > > > > -     ret = drm_connector_init_with_ddc(drm, connector,
> > > > > > > -                                       &drm_bridge_connector_funcs,
> > > > > > > -                                       connector_type, ddc);
> > > > > > > +     ret = drmm_connector_init(drm, connector,
> > > > > > > +                               &drm_bridge_connector_funcs,
> > > > > > > +                               connector_type, ddc);
> > > > > >
> > > > > > ... and here of drm_connector_cleanup.
> > > > > >
> > > > > > drm_connector_unregister wasn't needed, so can ignore it, but you leak a reference to
> > > > > > connector->fwnode since you don't call fwnode_handle_put anymore.
> > > > > >
> > > > > > We should register a drmm action right below the call to fwnode_handle_get too.
> > > > >
> > > > > But drm_connector_cleanup() already contains
> > > > > fwnode_handle_put(connector->fwnode). Isn't that enough?
> > > >
> > > > It does, but now I'm confused.
> > > >
> > > > drm_bridge_connector_init takes a reference, drm_connector_init doesn't.
> > > > It will call drm_bridge_connector_destroy() that gives back its
> > > > reference (which makes sense to me), but then why do
> > > > drm_connector_cleanup() does? None of the drm_connector code even took
> > > > that reference, and we end up with a double-put.
> > > >
> > > > It looks like it was introduced by commit 48c429c6d18d ("drm/connector:
> > > > Add a fwnode pointer to drm_connector and register with ACPI (v2)") from
> > > > Hans, which does call put, but never gets that reference.
> > >
> > > The mentioned patch documents that pretty clearly:
> > >
> > > * Drivers can set this to associate a fwnode with a connector, drivers
> > > * are expected to get a reference on the fwnode when setting this.
> > > * drm_connector_cleanup() will call fwnode_handle_put() on this.
> > >
> > > This is logical. Whoever sets the drm_connector::fwnode pointer, should
> > > get reference. This way drm_connector_init() doesn't need to play with
> > > the reference counting. The cleanup code drops the reference (so the
> > > driver doesn't need to), because cleanup might be assynchronous..
> >
> > Right, but it's the cleanup part that isn't logical. It makes total
> > sense to have the connector that sets connector->fwnode get the
> > reference itself. It doesn't make sense to have the core give that
> > reference instead of the driver.
> >
> > It's confusing, because if the driver is supposed to handle its
> > reference itself, then it should handle all of it itself. This bug is
> > the testament for that: the natural approach is buggy.
> 
> I'd say this is the 'transfer of the ownership'. The base driver gets
> the reference, and then gives it away to the drm_connecter. But indeed
> this is not very intuitive.
> 
> I have looked at the original series by Hans/Heikky, but I don't seem
> to be able to find a good way to solve that. The fwnode can be set
> after initialising the drm_connector. And at the same time it doesn't
> make so much sense to put that burden onto the driver. One option
> might be to add drm_connector_set_fwnode() that will get the reference
> internally, but that looks a bit like an overkill.

It looks like there's only one driver that actually requires that kind
of hack: i915. So, imo, the hacks should be in i915 and not cripple the
core API.

Maxime

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

[PATCH v5 3/9] drm/bridge-connector: implement glue code for HDMI connector

[Dmitry Baryshkov]

In order to let bridge chains implement HDMI connector infrastructure,
add necessary glue code to the drm_bridge_connector. In case there is a
bridge that sets DRM_BRIDGE_OP_HDMI, drm_bridge_connector will register
itself as a HDMI connector and provide proxy drm_connector_hdmi_funcs
implementation.

Note, to simplify implementation, there can be only one bridge in a
chain that sets DRM_BRIDGE_OP_HDMI. Setting more than one is considered
an error. This limitation can be lifted later, if the need arises.

Acked-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_bridge_connector.c | 94 ++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_debugfs.c          |  2 +
 include/drm/drm_bridge.h               | 81 +++++++++++++++++++++++++++++
 3 files changed, 174 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
index e093fc8928dc..0869b663f17e 100644
--- a/drivers/gpu/drm/drm_bridge_connector.c
+++ b/drivers/gpu/drm/drm_bridge_connector.c
@@ -18,6 +18,7 @@
 #include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/display/drm_hdmi_state_helper.h>
 
 /**
  * DOC: overview
@@ -87,6 +88,13 @@ struct drm_bridge_connector {
 	 * connector modes detection, if any (see &DRM_BRIDGE_OP_MODES).
 	 */
 	struct drm_bridge *bridge_modes;
+	/**
+	 * @bridge_hdmi:
+	 *
+	 * The bridge in the chain that implements necessary support for the
+	 * HDMI connector infrastructure, if any (see &DRM_BRIDGE_OP_HDMI).
+	 */
+	struct drm_bridge *bridge_hdmi;
 };
 
 #define to_drm_bridge_connector(x) \
@@ -287,6 +295,60 @@ static const struct drm_connector_helper_funcs drm_bridge_connector_helper_funcs
 	.disable_hpd = drm_bridge_connector_disable_hpd,
 };
 
+static enum drm_mode_status
+drm_bridge_connector_tmds_char_rate_valid(const struct drm_connector *connector,
+					  const struct drm_display_mode *mode,
+					  unsigned long long tmds_rate)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi;
+	if (!bridge)
+		return MODE_ERROR;
+
+	if (bridge->funcs->hdmi_tmds_char_rate_valid)
+		return bridge->funcs->hdmi_tmds_char_rate_valid(bridge, mode, tmds_rate);
+	else
+		return MODE_OK;
+}
+
+static int drm_bridge_connector_clear_infoframe(struct drm_connector *connector,
+						enum hdmi_infoframe_type type)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi;
+	if (!bridge)
+		return -EINVAL;
+
+	return bridge->funcs->hdmi_clear_infoframe(bridge, type);
+}
+
+static int drm_bridge_connector_write_infoframe(struct drm_connector *connector,
+						enum hdmi_infoframe_type type,
+						const u8 *buffer, size_t len)
+{
+	struct drm_bridge_connector *bridge_connector =
+		to_drm_bridge_connector(connector);
+	struct drm_bridge *bridge;
+
+	bridge = bridge_connector->bridge_hdmi;
+	if (!bridge)
+		return -EINVAL;
+
+	return bridge->funcs->hdmi_write_infoframe(bridge, type, buffer, len);
+}
+
+static const struct drm_connector_hdmi_funcs drm_bridge_connector_hdmi_funcs = {
+	.tmds_char_rate_valid = drm_bridge_connector_tmds_char_rate_valid,
+	.clear_infoframe = drm_bridge_connector_clear_infoframe,
+	.write_infoframe = drm_bridge_connector_write_infoframe,
+};
+
 /* -----------------------------------------------------------------------------
  * Bridge Connector Initialisation
  */
@@ -312,6 +374,8 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 	struct drm_connector *connector;
 	struct i2c_adapter *ddc = NULL;
 	struct drm_bridge *bridge, *panel_bridge = NULL;
+	unsigned int supported_formats = BIT(HDMI_COLORSPACE_RGB);
+	unsigned int max_bpc = 8;
 	int connector_type;
 	int ret;
 
@@ -348,6 +412,20 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			bridge_connector->bridge_detect = bridge;
 		if (bridge->ops & DRM_BRIDGE_OP_MODES)
 			bridge_connector->bridge_modes = bridge;
+		if (bridge->ops & DRM_BRIDGE_OP_HDMI) {
+			if (bridge_connector->bridge_hdmi)
+				return ERR_PTR(-EBUSY);
+			if (!bridge->funcs->hdmi_write_infoframe ||
+			    !bridge->funcs->hdmi_clear_infoframe)
+				return ERR_PTR(-EINVAL);
+
+			bridge_connector->bridge_hdmi = bridge;
+
+			if (bridge->supported_formats)
+				supported_formats = bridge->supported_formats;
+			if (bridge->max_bpc)
+				max_bpc = bridge->max_bpc;
+		}
 
 		if (!drm_bridge_get_next_bridge(bridge))
 			connector_type = bridge->type;
@@ -370,9 +448,19 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 		return ERR_PTR(-EINVAL);
 	}
 
-	ret = drmm_connector_init(drm, connector,
-				  &drm_bridge_connector_funcs,
-				  connector_type, ddc);
+	if (bridge_connector->bridge_hdmi)
+		ret = drmm_connector_hdmi_init(drm, connector,
+					       bridge_connector->bridge_hdmi->vendor,
+					       bridge_connector->bridge_hdmi->product,
+					       &drm_bridge_connector_funcs,
+					       &drm_bridge_connector_hdmi_funcs,
+					       connector_type, ddc,
+					       supported_formats,
+					       max_bpc);
+	else
+		ret = drmm_connector_init(drm, connector,
+					  &drm_bridge_connector_funcs,
+					  connector_type, ddc);
 	if (ret) {
 		kfree(bridge_connector);
 		return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index dd39a5b7a711..e385d90ef893 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -762,6 +762,8 @@ static int bridges_show(struct seq_file *m, void *data)
 			drm_puts(&p, " hpd");
 		if (bridge->ops & DRM_BRIDGE_OP_MODES)
 			drm_puts(&p, " modes");
+		if (bridge->ops & DRM_BRIDGE_OP_HDMI)
+			drm_puts(&p, " hdmi");
 		drm_puts(&p, "\n");
 	}
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4baca0d9107b..9f5d7bef41c5 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -630,6 +630,52 @@ struct drm_bridge_funcs {
 	 */
 	void (*hpd_disable)(struct drm_bridge *bridge);
 
+	/**
+	 * @hdmi_tmds_char_rate_valid:
+	 *
+	 * Check whether a particular TMDS character rate is supported by the
+	 * driver.
+	 *
+	 * This callback is optional and should only be implemented by the
+	 * bridges that take part in the HDMI connector implementation. Bridges
+	 * that implement it shall set set the DRM_BRIDGE_OP_HDMI flag in their
+	 * &drm_bridge->ops.
+	 *
+	 * Returns:
+	 *
+	 * Either &drm_mode_status.MODE_OK or one of the failure reasons
+	 * in &enum drm_mode_status.
+	 */
+	enum drm_mode_status
+	(*hdmi_tmds_char_rate_valid)(const struct drm_bridge *bridge,
+				     const struct drm_display_mode *mode,
+				     unsigned long long tmds_rate);
+
+	/**
+	 * @hdmi_clear_infoframe:
+	 *
+	 * This callback clears the infoframes in the hardware during commit.
+	 * It will be called multiple times, once for every disabled infoframe
+	 * type.
+	 *
+	 * This callback is optional but it must be implemented by bridges that
+	 * set the DRM_BRIDGE_OP_HDMI flag in their &drm_bridge->ops.
+	 */
+	int (*hdmi_clear_infoframe)(struct drm_bridge *bridge,
+				    enum hdmi_infoframe_type type);
+	/**
+	 * @hdmi_write_infoframe:
+	 *
+	 * Program the infoframe into the hardware. It will be called multiple
+	 * times, once for every updated infoframe type.
+	 *
+	 * This callback is optional but it must be implemented by bridges that
+	 * set the DRM_BRIDGE_OP_HDMI flag in their &drm_bridge->ops.
+	 */
+	int (*hdmi_write_infoframe)(struct drm_bridge *bridge,
+				    enum hdmi_infoframe_type type,
+				    const u8 *buffer, size_t len);
+
 	/**
 	 * @debugfs_init:
 	 *
@@ -705,6 +751,16 @@ enum drm_bridge_ops {
 	 * this flag shall implement the &drm_bridge_funcs->get_modes callback.
 	 */
 	DRM_BRIDGE_OP_MODES = BIT(3),
+	/**
+	 * @DRM_BRIDGE_OP_HDMI: The bridge provides HDMI connector operations,
+	 * including infoframes support. Bridges that set this flag must
+	 * implement the &drm_bridge_funcs->write_infoframe callback.
+	 *
+	 * Note: currently there can be at most one bridge in a chain that sets
+	 * this bit. This is to simplify corresponding glue code in connector
+	 * drivers.
+	 */
+	DRM_BRIDGE_OP_HDMI = BIT(4),
 };
 
 /**
@@ -773,6 +829,31 @@ struct drm_bridge {
 	 * @hpd_cb.
 	 */
 	void *hpd_data;
+
+	/**
+	 * @vendor: Vendor of the product to be used for the SPD InfoFrame
+	 * generation. This is required if @DRM_BRIDGE_OP_HDMI is set.
+	 */
+	const char *vendor;
+
+	/**
+	 * @product: Name of the product to be used for the SPD InfoFrame
+	 * generation. This is required if @DRM_BRIDGE_OP_HDMI is set.
+	 */
+	const char *product;
+
+	/**
+	 * @supported_formats: Bitmask of @hdmi_colorspace listing supported
+	 * output formats. This is only relevant if @DRM_BRIDGE_OP_HDMI is set.
+	 */
+	unsigned int supported_formats;
+
+	/**
+	 * @max_bpc: Maximum bits per char the HDMI bridge supports. Allowed
+	 * values are 8, 10 and 12. This is only relevant if
+	 * @DRM_BRIDGE_OP_HDMI is set.
+	 */
+	unsigned int max_bpc;
 };
 
 static inline struct drm_bridge *

-- 
2.39.2

[PATCH v5 4/9] drm/msm/hdmi: switch to atomic bridge callbacks

[Dmitry Baryshkov]

Change MSM HDMI bridge to use atomic_* callbacks in preparation to
enablign the HDMI connector support.

Acked-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 4a5b5112227f..d839c71091dc 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -126,7 +126,8 @@ static void msm_hdmi_config_avi_infoframe(struct hdmi *hdmi)
 	hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL1, val);
 }
 
-static void msm_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
+static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					      struct drm_bridge_state *old_bridge_state)
 {
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
@@ -152,7 +153,8 @@ static void msm_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
 		msm_hdmi_hdcp_on(hdmi->hdcp_ctrl);
 }
 
-static void msm_hdmi_bridge_post_disable(struct drm_bridge *bridge)
+static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+						struct drm_bridge_state *old_bridge_state)
 {
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
@@ -299,8 +301,11 @@ static enum drm_mode_status msm_hdmi_bridge_mode_valid(struct drm_bridge *bridge
 }
 
 static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = {
-	.pre_enable = msm_hdmi_bridge_pre_enable,
-	.post_disable = msm_hdmi_bridge_post_disable,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.atomic_pre_enable = msm_hdmi_bridge_atomic_pre_enable,
+	.atomic_post_disable = msm_hdmi_bridge_atomic_post_disable,
 	.mode_set = msm_hdmi_bridge_mode_set,
 	.mode_valid = msm_hdmi_bridge_mode_valid,
 	.edid_read = msm_hdmi_bridge_edid_read,

-- 
2.39.2

Re: [PATCH v5 4/9] drm/msm/hdmi: switch to atomic bridge callbacks

[Abhinav Kumar]

On 6/7/2024 6:23 AM, Dmitry Baryshkov wrote:
> Change MSM HDMI bridge to use atomic_* callbacks in preparation to
> enablign the HDMI connector support.
> 
> Acked-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 


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

[PATCH v5 5/9] drm/msm/hdmi: turn mode_set into atomic_enable

[Dmitry Baryshkov]

The mode_set callback is deprecated, it doesn't get the
drm_bridge_state, just mode-related argumetns. Turn it into the
atomic_enable callback as suggested by the documentation.

Acked-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index d839c71091dc..f259d6268c0f 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -129,12 +129,25 @@ static void msm_hdmi_config_avi_infoframe(struct hdmi *hdmi)
 static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 					      struct drm_bridge_state *old_bridge_state)
 {
+	struct drm_atomic_state *state = old_bridge_state->base.state;
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
 	struct hdmi_phy *phy = hdmi->phy;
+	struct drm_encoder *encoder = bridge->encoder;
+	struct drm_connector *connector;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc_state *crtc_state;
+	const struct drm_display_mode *mode;
 
 	DBG("power up");
 
+	connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+	mode = &crtc_state->adjusted_mode;
+
+	hdmi->pixclock = mode->clock * 1000;
+
 	if (!hdmi->power_on) {
 		msm_hdmi_phy_resource_enable(phy);
 		msm_hdmi_power_on(bridge);
@@ -177,18 +190,24 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
 	}
 }
 
-static void msm_hdmi_bridge_mode_set(struct drm_bridge *bridge,
-		 const struct drm_display_mode *mode,
-		 const struct drm_display_mode *adjusted_mode)
+static void msm_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
+					  struct drm_bridge_state *old_bridge_state)
 {
+	struct drm_atomic_state *state = old_bridge_state->base.state;
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
+	struct drm_encoder *encoder = bridge->encoder;
+	struct drm_connector *connector;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc_state *crtc_state;
+	const struct drm_display_mode *mode;
 	int hstart, hend, vstart, vend;
 	uint32_t frame_ctrl;
 
-	mode = adjusted_mode;
-
-	hdmi->pixclock = mode->clock * 1000;
+	connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+	mode = &crtc_state->adjusted_mode;
 
 	hstart = mode->htotal - mode->hsync_start;
 	hend   = mode->htotal - mode->hsync_start + mode->hdisplay;
@@ -305,8 +324,8 @@ static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
 	.atomic_pre_enable = msm_hdmi_bridge_atomic_pre_enable,
+	.atomic_enable = msm_hdmi_bridge_atomic_enable,
 	.atomic_post_disable = msm_hdmi_bridge_atomic_post_disable,
-	.mode_set = msm_hdmi_bridge_mode_set,
 	.mode_valid = msm_hdmi_bridge_mode_valid,
 	.edid_read = msm_hdmi_bridge_edid_read,
 	.detect = msm_hdmi_bridge_detect,

-- 
2.39.2

Re: [PATCH v5 5/9] drm/msm/hdmi: turn mode_set into atomic_enable

[Abhinav Kumar]

On 6/7/2024 6:23 AM, Dmitry Baryshkov wrote:
> The mode_set callback is deprecated, it doesn't get the
> drm_bridge_state, just mode-related argumetns. Turn it into the
> atomic_enable callback as suggested by the documentation.
> 

mode_set is deprecated but atomic_mode_set is not.

I would rather use atomic_mode_set because moving to atomic_enable() 
would be incorrect.

That would be called after encoder's enable and hence changes the 
sequence. That was not the intention of this patch.

NAK.

> Acked-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 33 ++++++++++++++++++++++++++-------
>   1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index d839c71091dc..f259d6268c0f 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -129,12 +129,25 @@ static void msm_hdmi_config_avi_infoframe(struct hdmi *hdmi)
>   static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>   					      struct drm_bridge_state *old_bridge_state)
>   {
> +	struct drm_atomic_state *state = old_bridge_state->base.state;
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
>   	struct hdmi_phy *phy = hdmi->phy;
> +	struct drm_encoder *encoder = bridge->encoder;
> +	struct drm_connector *connector;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc_state *crtc_state;
> +	const struct drm_display_mode *mode;
>   
>   	DBG("power up");
>   
> +	connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
> +	conn_state = drm_atomic_get_new_connector_state(state, connector);
> +	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
> +	mode = &crtc_state->adjusted_mode;
> +
> +	hdmi->pixclock = mode->clock * 1000;
> +
>   	if (!hdmi->power_on) {
>   		msm_hdmi_phy_resource_enable(phy);
>   		msm_hdmi_power_on(bridge);
> @@ -177,18 +190,24 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
>   	}
>   }
>   
> -static void msm_hdmi_bridge_mode_set(struct drm_bridge *bridge,
> -		 const struct drm_display_mode *mode,
> -		 const struct drm_display_mode *adjusted_mode)
> +static void msm_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
> +					  struct drm_bridge_state *old_bridge_state)
>   {
> +	struct drm_atomic_state *state = old_bridge_state->base.state;
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
> +	struct drm_encoder *encoder = bridge->encoder;
> +	struct drm_connector *connector;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc_state *crtc_state;
> +	const struct drm_display_mode *mode;
>   	int hstart, hend, vstart, vend;
>   	uint32_t frame_ctrl;
>   
> -	mode = adjusted_mode;
> -
> -	hdmi->pixclock = mode->clock * 1000;
> +	connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
> +	conn_state = drm_atomic_get_new_connector_state(state, connector);
> +	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
> +	mode = &crtc_state->adjusted_mode;
>   
>   	hstart = mode->htotal - mode->hsync_start;
>   	hend   = mode->htotal - mode->hsync_start + mode->hdisplay;
> @@ -305,8 +324,8 @@ static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = {
>   	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>   	.atomic_reset = drm_atomic_helper_bridge_reset,
>   	.atomic_pre_enable = msm_hdmi_bridge_atomic_pre_enable,
> +	.atomic_enable = msm_hdmi_bridge_atomic_enable,
>   	.atomic_post_disable = msm_hdmi_bridge_atomic_post_disable,
> -	.mode_set = msm_hdmi_bridge_mode_set,
>   	.mode_valid = msm_hdmi_bridge_mode_valid,
>   	.edid_read = msm_hdmi_bridge_edid_read,
>   	.detect = msm_hdmi_bridge_detect,
> 

Re: [PATCH v5 5/9] drm/msm/hdmi: turn mode_set into atomic_enable

[Dmitry Baryshkov]

On Thu, Jun 20, 2024 at 01:27:15PM GMT, Abhinav Kumar wrote:
> 
> 
> On 6/7/2024 6:23 AM, Dmitry Baryshkov wrote:
> > The mode_set callback is deprecated, it doesn't get the
> > drm_bridge_state, just mode-related argumetns. Turn it into the
> > atomic_enable callback as suggested by the documentation.
> > 
> 
> mode_set is deprecated but atomic_mode_set is not.

There is no atomic_mode_set() in drm_bridge_funcs. Also:

* This is deprecated, do not use!
* New drivers shall set their mode in the
* &drm_bridge_funcs.atomic_enable operation.

> 
> I would rather use atomic_mode_set because moving to atomic_enable() would
> be incorrect.
> 
> That would be called after encoder's enable and hence changes the sequence.
> That was not the intention of this patch.
> 
> NAK.
> 
> > Acked-by: Maxime Ripard <mripard@kernel.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 33 ++++++++++++++++++++++++++-------
> >   1 file changed, 26 insertions(+), 7 deletions(-)


-- 
With best wishes
Dmitry

Re: [PATCH v5 5/9] drm/msm/hdmi: turn mode_set into atomic_enable

[Abhinav Kumar]

On 6/20/2024 1:32 PM, Dmitry Baryshkov wrote:
> On Thu, Jun 20, 2024 at 01:27:15PM GMT, Abhinav Kumar wrote:
>>
>>
>> On 6/7/2024 6:23 AM, Dmitry Baryshkov wrote:
>>> The mode_set callback is deprecated, it doesn't get the
>>> drm_bridge_state, just mode-related argumetns. Turn it into the
>>> atomic_enable callback as suggested by the documentation.
>>>
>>
>> mode_set is deprecated but atomic_mode_set is not.
> 
> There is no atomic_mode_set() in drm_bridge_funcs. Also:
> 

Please excuse me. I thought since encoder has atomic_mode_set(), bridge 
has one too.

> * This is deprecated, do not use!
> * New drivers shall set their mode in the
> * &drm_bridge_funcs.atomic_enable operation.
>

Yes I saw this note but it also says "new drivers" and not really 
enforcing migrating existing ones which are using modeset to atomic_enable.

My concern is that today the timing engine setup happens in encoder's 
enable() and the hdmi's timing is programmed in mode_set().

Ideally, we should program hdmi's timing registers first before the 
encoder's timing.

Although timing engine is not enabled yet, till post_kickoff, this is 
changing the sequence.

If this really required for rest of this series?

>>
>> I would rather use atomic_mode_set because moving to atomic_enable() would
>> be incorrect.
>>
>> That would be called after encoder's enable and hence changes the sequence.
>> That was not the intention of this patch.
>>
>> NAK.
>>
>>> Acked-by: Maxime Ripard <mripard@kernel.org>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 33 ++++++++++++++++++++++++++-------
>>>    1 file changed, 26 insertions(+), 7 deletions(-)
> 
> 

Re: [PATCH v5 5/9] drm/msm/hdmi: turn mode_set into atomic_enable

[Dmitry Baryshkov]

On Thu, Jun 20, 2024 at 01:49:33PM GMT, Abhinav Kumar wrote:
> 
> 
> On 6/20/2024 1:32 PM, Dmitry Baryshkov wrote:
> > On Thu, Jun 20, 2024 at 01:27:15PM GMT, Abhinav Kumar wrote:
> > > 
> > > 
> > > On 6/7/2024 6:23 AM, Dmitry Baryshkov wrote:
> > > > The mode_set callback is deprecated, it doesn't get the
> > > > drm_bridge_state, just mode-related argumetns. Turn it into the
> > > > atomic_enable callback as suggested by the documentation.
> > > > 
> > > 
> > > mode_set is deprecated but atomic_mode_set is not.
> > 
> > There is no atomic_mode_set() in drm_bridge_funcs. Also:
> > 
> 
> Please excuse me. I thought since encoder has atomic_mode_set(), bridge has
> one too.
> 
> > * This is deprecated, do not use!
> > * New drivers shall set their mode in the
> > * &drm_bridge_funcs.atomic_enable operation.
> > 
> 
> Yes I saw this note but it also says "new drivers" and not really enforcing
> migrating existing ones which are using modeset to atomic_enable.

Well, deprecated functions are supposed to be migrated.

> My concern is that today the timing engine setup happens in encoder's
> enable() and the hdmi's timing is programmed in mode_set().
> 
> Ideally, we should program hdmi's timing registers first before the
> encoder's timing.
> 
> Although timing engine is not enabled yet, till post_kickoff, this is
> changing the sequence.
> 
> If this really required for rest of this series?

No, it was just worth doing it as I was doing conversion to atomic_*
anyway. I can delay this patch for now.

Two questions:

1) Are you sure regarding the HDMI timing registers vs INTF order? I
observe the underrun issues while changing modes on db820c.

2) What should be the order between programming of the HDMI timing
engine and HDMI PHY?

What would be your opinion on moving HDMI timing programming to
atomic_pre_enable? (the exact place depends on the answer on the second
question).

> 
> > > 
> > > I would rather use atomic_mode_set because moving to atomic_enable() would
> > > be incorrect.
> > > 
> > > That would be called after encoder's enable and hence changes the sequence.
> > > That was not the intention of this patch.
> > > 
> > > NAK.
> > > 
> > > > Acked-by: Maxime Ripard <mripard@kernel.org>
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >    drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 33 ++++++++++++++++++++++++++-------
> > > >    1 file changed, 26 insertions(+), 7 deletions(-)
> > 
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v5 5/9] drm/msm/hdmi: turn mode_set into atomic_enable

[Abhinav Kumar]

On 6/20/2024 2:24 PM, Dmitry Baryshkov wrote:
> On Thu, Jun 20, 2024 at 01:49:33PM GMT, Abhinav Kumar wrote:
>>
>>
>> On 6/20/2024 1:32 PM, Dmitry Baryshkov wrote:
>>> On Thu, Jun 20, 2024 at 01:27:15PM GMT, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/7/2024 6:23 AM, Dmitry Baryshkov wrote:
>>>>> The mode_set callback is deprecated, it doesn't get the
>>>>> drm_bridge_state, just mode-related argumetns. Turn it into the
>>>>> atomic_enable callback as suggested by the documentation.
>>>>>
>>>>
>>>> mode_set is deprecated but atomic_mode_set is not.
>>>
>>> There is no atomic_mode_set() in drm_bridge_funcs. Also:
>>>
>>
>> Please excuse me. I thought since encoder has atomic_mode_set(), bridge has
>> one too.
>>
>>> * This is deprecated, do not use!
>>> * New drivers shall set their mode in the
>>> * &drm_bridge_funcs.atomic_enable operation.
>>>
>>
>> Yes I saw this note but it also says "new drivers" and not really enforcing
>> migrating existing ones which are using modeset to atomic_enable.
> 
> Well, deprecated functions are supposed to be migrated.
> 

Along with rest of the pieces of the driver :)

>> My concern is that today the timing engine setup happens in encoder's
>> enable() and the hdmi's timing is programmed in mode_set().
>>
>> Ideally, we should program hdmi's timing registers first before the
>> encoder's timing.
>>
>> Although timing engine is not enabled yet, till post_kickoff, this is
>> changing the sequence.
>>
>> If this really required for rest of this series?
> 
> No, it was just worth doing it as I was doing conversion to atomic_*
> anyway. I can delay this patch for now.
> 
> Two questions:
> 
> 1) Are you sure regarding the HDMI timing registers vs INTF order? I
> observe the underrun issues while changing modes on db820c.
> 

Yes this is the order I see in the docs:

1) setup HDMI PHY and PLL
2) setup HDMI video path correctly (HDMI timing registers)
3) setup timing generator to match the HDMI video in (2)
4) Enable timing engine

This change is mixing up the order of (2) and (3).

> 2) What should be the order between programming of the HDMI timing
> engine and HDMI PHY?
> 

Mentioned above.

> What would be your opinion on moving HDMI timing programming to
> atomic_pre_enable? (the exact place depends on the answer on the second
> question).
> 

So

-> bridge->pre_enable() : Do HDMI timing programming
-> encoder->atomic_enable() : Do timing engine programming
-> post_kickoff() does timing engine enable

This matches the steps I wrote above.

Hence this is fine from my PoV.

>>
>>>>
>>>> I would rather use atomic_mode_set because moving to atomic_enable() would
>>>> be incorrect.
>>>>
>>>> That would be called after encoder's enable and hence changes the sequence.
>>>> That was not the intention of this patch.
>>>>
>>>> NAK.
>>>>
>>>>> Acked-by: Maxime Ripard <mripard@kernel.org>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>     drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 33 ++++++++++++++++++++++++++-------
>>>>>     1 file changed, 26 insertions(+), 7 deletions(-)
>>>
>>>
> 

Re: [PATCH v5 5/9] drm/msm/hdmi: turn mode_set into atomic_enable

[Dmitry Baryshkov]

On Thu, Jun 20, 2024 at 03:05:16PM GMT, Abhinav Kumar wrote:
> 
> 
> On 6/20/2024 2:24 PM, Dmitry Baryshkov wrote:
> > On Thu, Jun 20, 2024 at 01:49:33PM GMT, Abhinav Kumar wrote:
> > > 
> > > 
> > > On 6/20/2024 1:32 PM, Dmitry Baryshkov wrote:
> > > > On Thu, Jun 20, 2024 at 01:27:15PM GMT, Abhinav Kumar wrote:
> > > > > 
> > > > > 
> > > > > On 6/7/2024 6:23 AM, Dmitry Baryshkov wrote:
> > > > > > The mode_set callback is deprecated, it doesn't get the
> > > > > > drm_bridge_state, just mode-related argumetns. Turn it into the
> > > > > > atomic_enable callback as suggested by the documentation.
> > > > > > 
> > > > > 
> > > > > mode_set is deprecated but atomic_mode_set is not.
> > > > 
> > > > There is no atomic_mode_set() in drm_bridge_funcs. Also:
> > > > 
> > > 
> > > Please excuse me. I thought since encoder has atomic_mode_set(), bridge has
> > > one too.
> > > 
> > > > * This is deprecated, do not use!
> > > > * New drivers shall set their mode in the
> > > > * &drm_bridge_funcs.atomic_enable operation.
> > > > 
> > > 
> > > Yes I saw this note but it also says "new drivers" and not really enforcing
> > > migrating existing ones which are using modeset to atomic_enable.
> > 
> > Well, deprecated functions are supposed to be migrated.
> > 
> 
> Along with rest of the pieces of the driver :)

Step by step :-)

> 
> > > My concern is that today the timing engine setup happens in encoder's
> > > enable() and the hdmi's timing is programmed in mode_set().
> > > 
> > > Ideally, we should program hdmi's timing registers first before the
> > > encoder's timing.
> > > 
> > > Although timing engine is not enabled yet, till post_kickoff, this is
> > > changing the sequence.
> > > 
> > > If this really required for rest of this series?
> > 
> > No, it was just worth doing it as I was doing conversion to atomic_*
> > anyway. I can delay this patch for now.
> > 
> > Two questions:
> > 
> > 1) Are you sure regarding the HDMI timing registers vs INTF order? I
> > observe the underrun issues while changing modes on db820c.
> > 
> 
> Yes this is the order I see in the docs:
> 
> 1) setup HDMI PHY and PLL
> 2) setup HDMI video path correctly (HDMI timing registers)
> 3) setup timing generator to match the HDMI video in (2)
> 4) Enable timing engine

Thanks!

> 
> This change is mixing up the order of (2) and (3).
> 
> > 2) What should be the order between programming of the HDMI timing
> > engine and HDMI PHY?
> > 
> 
> Mentioned above.
> 
> > What would be your opinion on moving HDMI timing programming to
> > atomic_pre_enable? (the exact place depends on the answer on the second
> > question).
> > 
> 
> So
> 
> -> bridge->pre_enable() : Do HDMI timing programming
> -> encoder->atomic_enable() : Do timing engine programming
> -> post_kickoff() does timing engine enable
> 
> This matches the steps I wrote above.
> 
> Hence this is fine from my PoV.

Ack, I'll skip this patch for now and repost it separately (moving the
code to pre_enable function).

> 
> > > 
> > > > > 
> > > > > I would rather use atomic_mode_set because moving to atomic_enable() would
> > > > > be incorrect.
> > > > > 
> > > > > That would be called after encoder's enable and hence changes the sequence.
> > > > > That was not the intention of this patch.
> > > > > 
> > > > > NAK.
> > > > > 
> > > > > > Acked-by: Maxime Ripard <mripard@kernel.org>
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > ---
> > > > > >     drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 33 ++++++++++++++++++++++++++-------
> > > > > >     1 file changed, 26 insertions(+), 7 deletions(-)
> > > > 
> > > > 
> > 

-- 
With best wishes
Dmitry

[PATCH v5 6/9] drm/msm/hdmi: make use of the drm_connector_hdmi framework

[Dmitry Baryshkov]

Setup the HDMI connector on the MSM HDMI outputs. Make use of
atomic_check hook and of the provided Infoframe infrastructure.

Acked-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/Kconfig            |   2 +
 drivers/gpu/drm/msm/hdmi/hdmi.c        |  45 ++-------
 drivers/gpu/drm/msm/hdmi/hdmi.h        |  16 +---
 drivers/gpu/drm/msm/hdmi/hdmi_audio.c  |  74 ++++----------
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 170 ++++++++++++++++++++++++---------
 5 files changed, 160 insertions(+), 147 deletions(-)

diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index 1931ecf73e32..b5c78c1dd744 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -164,6 +164,8 @@ config DRM_MSM_HDMI
 	bool "Enable HDMI support in MSM DRM driver"
 	depends on DRM_MSM
 	default y
+	select DRM_DISPLAY_HDMI_HELPER
+	select DRM_DISPLAY_HDMI_STATE_HELPER
 	help
 	  Compile in support for the HDMI output MSM DRM driver. It can
 	  be a primary or a secondary display on device. Note that this is used
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 24abcb7254cc..2279e09fd2de 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -12,6 +12,7 @@
 
 #include <drm/drm_bridge_connector.h>
 #include <drm/drm_of.h>
+#include <drm/display/drm_hdmi_state_helper.h>
 
 #include <sound/hdmi-codec.h>
 #include "hdmi.h"
@@ -165,8 +166,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 	hdmi->dev = dev;
 	hdmi->encoder = encoder;
 
-	hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
-
 	ret = msm_hdmi_bridge_init(hdmi);
 	if (ret) {
 		DRM_DEV_ERROR(dev->dev, "failed to create HDMI bridge: %d\n", ret);
@@ -254,40 +253,12 @@ static int msm_hdmi_audio_hw_params(struct device *dev, void *data,
 				    struct hdmi_codec_params *params)
 {
 	struct hdmi *hdmi = dev_get_drvdata(dev);
-	unsigned int chan;
-	unsigned int channel_allocation = 0;
 	unsigned int rate;
-	unsigned int level_shift  = 0; /* 0dB */
-	bool down_mix = false;
+	int ret;
 
 	DRM_DEV_DEBUG(dev, "%u Hz, %d bit, %d channels\n", params->sample_rate,
 		 params->sample_width, params->cea.channels);
 
-	switch (params->cea.channels) {
-	case 2:
-		/* FR and FL speakers */
-		channel_allocation  = 0;
-		chan = MSM_HDMI_AUDIO_CHANNEL_2;
-		break;
-	case 4:
-		/* FC, LFE, FR and FL speakers */
-		channel_allocation  = 0x3;
-		chan = MSM_HDMI_AUDIO_CHANNEL_4;
-		break;
-	case 6:
-		/* RR, RL, FC, LFE, FR and FL speakers */
-		channel_allocation  = 0x0B;
-		chan = MSM_HDMI_AUDIO_CHANNEL_6;
-		break;
-	case 8:
-		/* FRC, FLC, RR, RL, FC, LFE, FR and FL speakers */
-		channel_allocation  = 0x1F;
-		chan = MSM_HDMI_AUDIO_CHANNEL_8;
-		break;
-	default:
-		return -EINVAL;
-	}
-
 	switch (params->sample_rate) {
 	case 32000:
 		rate = HDMI_SAMPLE_RATE_32KHZ;
@@ -316,9 +287,12 @@ static int msm_hdmi_audio_hw_params(struct device *dev, void *data,
 		return -EINVAL;
 	}
 
-	msm_hdmi_audio_set_sample_rate(hdmi, rate);
-	msm_hdmi_audio_info_setup(hdmi, 1, chan, channel_allocation,
-			      level_shift, down_mix);
+	ret = drm_atomic_helper_connector_hdmi_update_audio_infoframe(hdmi->connector,
+								      &params->cea);
+	if (ret)
+		return ret;
+
+	msm_hdmi_audio_info_setup(hdmi, rate, params->cea.channels);
 
 	return 0;
 }
@@ -327,7 +301,8 @@ static void msm_hdmi_audio_shutdown(struct device *dev, void *data)
 {
 	struct hdmi *hdmi = dev_get_drvdata(dev);
 
-	msm_hdmi_audio_info_setup(hdmi, 0, 0, 0, 0, 0);
+	drm_atomic_helper_connector_hdmi_disable_audio_infoframe(hdmi->connector);
+	msm_hdmi_audio_disable(hdmi);
 }
 
 static const struct hdmi_codec_ops msm_hdmi_audio_codec_ops = {
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 4586baf36415..0ac034eaaf0f 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -24,8 +24,8 @@ struct hdmi_platform_config;
 
 struct hdmi_audio {
 	bool enabled;
-	struct hdmi_audio_infoframe infoframe;
 	int rate;
+	int channels;
 };
 
 struct hdmi_hdcp_ctrl;
@@ -199,12 +199,6 @@ static inline int msm_hdmi_pll_8996_init(struct platform_device *pdev)
 /*
  * audio:
  */
-/* Supported HDMI Audio channels and rates */
-#define	MSM_HDMI_AUDIO_CHANNEL_2	0
-#define	MSM_HDMI_AUDIO_CHANNEL_4	1
-#define	MSM_HDMI_AUDIO_CHANNEL_6	2
-#define	MSM_HDMI_AUDIO_CHANNEL_8	3
-
 #define	HDMI_SAMPLE_RATE_32KHZ		0
 #define	HDMI_SAMPLE_RATE_44_1KHZ	1
 #define	HDMI_SAMPLE_RATE_48KHZ		2
@@ -213,12 +207,8 @@ static inline int msm_hdmi_pll_8996_init(struct platform_device *pdev)
 #define	HDMI_SAMPLE_RATE_176_4KHZ	5
 #define	HDMI_SAMPLE_RATE_192KHZ		6
 
-int msm_hdmi_audio_update(struct hdmi *hdmi);
-int msm_hdmi_audio_info_setup(struct hdmi *hdmi, bool enabled,
-	uint32_t num_of_channels, uint32_t channel_allocation,
-	uint32_t level_shift, bool down_mix);
-void msm_hdmi_audio_set_sample_rate(struct hdmi *hdmi, int rate);
-
+int msm_hdmi_audio_info_setup(struct hdmi *hdmi, int rate, int channels);
+int msm_hdmi_audio_disable(struct hdmi *hdmi);
 
 /*
  * hdmi bridge:
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_audio.c b/drivers/gpu/drm/msm/hdmi/hdmi_audio.c
index 4c2058c4adc1..924654bfb48c 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_audio.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_audio.c
@@ -7,9 +7,6 @@
 #include <linux/hdmi.h>
 #include "hdmi.h"
 
-/* maps MSM_HDMI_AUDIO_CHANNEL_n consts used by audio driver to # of channels: */
-static int nchannels[] = { 2, 4, 6, 8 };
-
 /* Supported HDMI Audio sample rates */
 #define MSM_HDMI_SAMPLE_RATE_32KHZ		0
 #define MSM_HDMI_SAMPLE_RATE_44_1KHZ		1
@@ -71,19 +68,20 @@ static const struct hdmi_msm_audio_arcs *get_arcs(unsigned long int pixclock)
 	return NULL;
 }
 
-int msm_hdmi_audio_update(struct hdmi *hdmi)
+static int msm_hdmi_audio_update(struct hdmi *hdmi)
 {
 	struct hdmi_audio *audio = &hdmi->audio;
-	struct hdmi_audio_infoframe *info = &audio->infoframe;
 	const struct hdmi_msm_audio_arcs *arcs = NULL;
 	bool enabled = audio->enabled;
 	uint32_t acr_pkt_ctrl, vbi_pkt_ctrl, aud_pkt_ctrl;
-	uint32_t infofrm_ctrl, audio_config;
+	uint32_t audio_config;
+
+	if (!hdmi->connector->display_info.is_hdmi)
+		return -EINVAL;
+
+	DBG("audio: enabled=%d, channels=%d, rate=%d",
+	    audio->enabled, audio->channels, audio->rate);
 
-	DBG("audio: enabled=%d, channels=%d, channel_allocation=0x%x, "
-		"level_shift_value=%d, downmix_inhibit=%d, rate=%d",
-		audio->enabled, info->channels,  info->channel_allocation,
-		info->level_shift_value, info->downmix_inhibit, audio->rate);
 	DBG("video: power_on=%d, pixclock=%lu", hdmi->power_on, hdmi->pixclock);
 
 	if (enabled && !(hdmi->power_on && hdmi->pixclock)) {
@@ -104,7 +102,6 @@ int msm_hdmi_audio_update(struct hdmi *hdmi)
 	acr_pkt_ctrl = hdmi_read(hdmi, REG_HDMI_ACR_PKT_CTRL);
 	vbi_pkt_ctrl = hdmi_read(hdmi, REG_HDMI_VBI_PKT_CTRL);
 	aud_pkt_ctrl = hdmi_read(hdmi, REG_HDMI_AUDIO_PKT_CTRL1);
-	infofrm_ctrl = hdmi_read(hdmi, REG_HDMI_INFOFRAME_CTRL0);
 	audio_config = hdmi_read(hdmi, REG_HDMI_AUDIO_CFG);
 
 	/* Clear N/CTS selection bits */
@@ -113,7 +110,6 @@ int msm_hdmi_audio_update(struct hdmi *hdmi)
 	if (enabled) {
 		uint32_t n, cts, multiplier;
 		enum hdmi_acr_cts select;
-		uint8_t buf[14];
 
 		n   = arcs->lut[audio->rate].n;
 		cts = arcs->lut[audio->rate].cts;
@@ -155,20 +151,12 @@ int msm_hdmi_audio_update(struct hdmi *hdmi)
 				HDMI_ACR_1_N(n));
 
 		hdmi_write(hdmi, REG_HDMI_AUDIO_PKT_CTRL2,
-				COND(info->channels != 2, HDMI_AUDIO_PKT_CTRL2_LAYOUT) |
+				COND(audio->channels != 2, HDMI_AUDIO_PKT_CTRL2_LAYOUT) |
 				HDMI_AUDIO_PKT_CTRL2_OVERRIDE);
 
 		acr_pkt_ctrl |= HDMI_ACR_PKT_CTRL_CONT;
 		acr_pkt_ctrl |= HDMI_ACR_PKT_CTRL_SEND;
 
-		/* configure infoframe: */
-		hdmi_audio_infoframe_pack(info, buf, sizeof(buf));
-		hdmi_write(hdmi, REG_HDMI_AUDIO_INFO0,
-				(buf[3] <<  0) | (buf[4] <<  8) |
-				(buf[5] << 16) | (buf[6] << 24));
-		hdmi_write(hdmi, REG_HDMI_AUDIO_INFO1,
-				(buf[7] <<  0) | (buf[8] << 8));
-
 		hdmi_write(hdmi, REG_HDMI_GC, 0);
 
 		vbi_pkt_ctrl |= HDMI_VBI_PKT_CTRL_GC_ENABLE;
@@ -176,11 +164,6 @@ int msm_hdmi_audio_update(struct hdmi *hdmi)
 
 		aud_pkt_ctrl |= HDMI_AUDIO_PKT_CTRL1_AUDIO_SAMPLE_SEND;
 
-		infofrm_ctrl |= HDMI_INFOFRAME_CTRL0_AUDIO_INFO_SEND;
-		infofrm_ctrl |= HDMI_INFOFRAME_CTRL0_AUDIO_INFO_CONT;
-		infofrm_ctrl |= HDMI_INFOFRAME_CTRL0_AUDIO_INFO_SOURCE;
-		infofrm_ctrl |= HDMI_INFOFRAME_CTRL0_AUDIO_INFO_UPDATE;
-
 		audio_config &= ~HDMI_AUDIO_CFG_FIFO_WATERMARK__MASK;
 		audio_config |= HDMI_AUDIO_CFG_FIFO_WATERMARK(4);
 		audio_config |= HDMI_AUDIO_CFG_ENGINE_ENABLE;
@@ -190,17 +173,12 @@ int msm_hdmi_audio_update(struct hdmi *hdmi)
 		vbi_pkt_ctrl &= ~HDMI_VBI_PKT_CTRL_GC_ENABLE;
 		vbi_pkt_ctrl &= ~HDMI_VBI_PKT_CTRL_GC_EVERY_FRAME;
 		aud_pkt_ctrl &= ~HDMI_AUDIO_PKT_CTRL1_AUDIO_SAMPLE_SEND;
-		infofrm_ctrl &= ~HDMI_INFOFRAME_CTRL0_AUDIO_INFO_SEND;
-		infofrm_ctrl &= ~HDMI_INFOFRAME_CTRL0_AUDIO_INFO_CONT;
-		infofrm_ctrl &= ~HDMI_INFOFRAME_CTRL0_AUDIO_INFO_SOURCE;
-		infofrm_ctrl &= ~HDMI_INFOFRAME_CTRL0_AUDIO_INFO_UPDATE;
 		audio_config &= ~HDMI_AUDIO_CFG_ENGINE_ENABLE;
 	}
 
 	hdmi_write(hdmi, REG_HDMI_ACR_PKT_CTRL, acr_pkt_ctrl);
 	hdmi_write(hdmi, REG_HDMI_VBI_PKT_CTRL, vbi_pkt_ctrl);
 	hdmi_write(hdmi, REG_HDMI_AUDIO_PKT_CTRL1, aud_pkt_ctrl);
-	hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL0, infofrm_ctrl);
 
 	hdmi_write(hdmi, REG_HDMI_AUD_INT,
 			COND(enabled, HDMI_AUD_INT_AUD_FIFO_URUN_INT) |
@@ -214,41 +192,29 @@ int msm_hdmi_audio_update(struct hdmi *hdmi)
 	return 0;
 }
 
-int msm_hdmi_audio_info_setup(struct hdmi *hdmi, bool enabled,
-	uint32_t num_of_channels, uint32_t channel_allocation,
-	uint32_t level_shift, bool down_mix)
+int msm_hdmi_audio_info_setup(struct hdmi *hdmi, int rate, int channels)
 {
-	struct hdmi_audio *audio;
-
 	if (!hdmi)
 		return -ENXIO;
 
-	audio = &hdmi->audio;
-
-	if (num_of_channels >= ARRAY_SIZE(nchannels))
+	if ((rate < 0) || (rate >= MSM_HDMI_SAMPLE_RATE_MAX))
 		return -EINVAL;
 
-	audio->enabled = enabled;
-	audio->infoframe.channels = nchannels[num_of_channels];
-	audio->infoframe.channel_allocation = channel_allocation;
-	audio->infoframe.level_shift_value = level_shift;
-	audio->infoframe.downmix_inhibit = down_mix;
+	hdmi->audio.rate = rate;
+	hdmi->audio.channels = channels;
+	hdmi->audio.enabled = true;
 
 	return msm_hdmi_audio_update(hdmi);
 }
 
-void msm_hdmi_audio_set_sample_rate(struct hdmi *hdmi, int rate)
+int msm_hdmi_audio_disable(struct hdmi *hdmi)
 {
-	struct hdmi_audio *audio;
-
 	if (!hdmi)
-		return;
-
-	audio = &hdmi->audio;
+		return -ENXIO;
 
-	if ((rate < 0) || (rate >= MSM_HDMI_SAMPLE_RATE_MAX))
-		return;
+	hdmi->audio.rate = 0;
+	hdmi->audio.channels = 2;
+	hdmi->audio.enabled = false;
 
-	audio->rate = rate;
-	msm_hdmi_audio_update(hdmi);
+	return msm_hdmi_audio_update(hdmi);
 }
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index f259d6268c0f..9eecc9960e75 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -7,6 +7,8 @@
 #include <linux/delay.h>
 #include <drm/drm_bridge_connector.h>
 #include <drm/drm_edid.h>
+#include <drm/display/drm_hdmi_helper.h>
+#include <drm/display/drm_hdmi_state_helper.h>
 
 #include "msm_kms.h"
 #include "hdmi.h"
@@ -68,23 +70,17 @@ static void power_off(struct drm_bridge *bridge)
 
 #define AVI_IFRAME_LINE_NUMBER 1
 
-static void msm_hdmi_config_avi_infoframe(struct hdmi *hdmi)
+static int msm_hdmi_config_avi_infoframe(struct hdmi *hdmi,
+					 const u8 *buffer, size_t len)
 {
-	struct drm_crtc *crtc = hdmi->encoder->crtc;
-	const struct drm_display_mode *mode = &crtc->state->adjusted_mode;
-	union hdmi_infoframe frame;
-	u8 buffer[HDMI_INFOFRAME_SIZE(AVI)];
+	u32 buf[4] = {};
 	u32 val;
-	int len;
+	int i;
 
-	drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
-						 hdmi->connector, mode);
-
-	len = hdmi_infoframe_pack(&frame, buffer, sizeof(buffer));
-	if (len < 0) {
+	if (len != HDMI_INFOFRAME_SIZE(AVI) || len - 3 > sizeof(buf)) {
 		DRM_DEV_ERROR(&hdmi->pdev->dev,
 			"failed to configure avi infoframe\n");
-		return;
+		return -EINVAL;
 	}
 
 	/*
@@ -93,37 +89,126 @@ static void msm_hdmi_config_avi_infoframe(struct hdmi *hdmi)
 	 * written to the LSB byte of AVI_INFO0 and the version is written to
 	 * the third byte from the LSB of AVI_INFO3
 	 */
-	hdmi_write(hdmi, REG_HDMI_AVI_INFO(0),
+	memcpy(buf, &buffer[3], len - 3);
+
+	buf[3] |= buffer[1] << 24;
+
+	for (i = 0; i < ARRAY_SIZE(buf); i++)
+		hdmi_write(hdmi, REG_HDMI_AVI_INFO(i), buf[i]);
+
+	val = hdmi_read(hdmi, REG_HDMI_INFOFRAME_CTRL1);
+	val |= HDMI_INFOFRAME_CTRL0_AVI_SEND |
+		HDMI_INFOFRAME_CTRL0_AVI_CONT;
+	hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL0, val);
+
+	val = hdmi_read(hdmi, REG_HDMI_INFOFRAME_CTRL1);
+	val &= ~HDMI_INFOFRAME_CTRL1_AVI_INFO_LINE__MASK;
+	val |= HDMI_INFOFRAME_CTRL1_AVI_INFO_LINE(AVI_IFRAME_LINE_NUMBER);
+	hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL1, val);
+
+	return 0;
+}
+
+static int msm_hdmi_config_audio_infoframe(struct hdmi *hdmi,
+					   const u8 *buffer, size_t len)
+{
+	u32 val;
+
+	if (len != HDMI_INFOFRAME_SIZE(AUDIO)) {
+		DRM_DEV_ERROR(&hdmi->pdev->dev,
+			"failed to configure audio infoframe\n");
+		return -EINVAL;
+	}
+
+	hdmi_write(hdmi, REG_HDMI_AUDIO_INFO0,
 		   buffer[3] |
 		   buffer[4] << 8 |
 		   buffer[5] << 16 |
 		   buffer[6] << 24);
 
-	hdmi_write(hdmi, REG_HDMI_AVI_INFO(1),
+	hdmi_write(hdmi, REG_HDMI_AUDIO_INFO1,
 		   buffer[7] |
 		   buffer[8] << 8 |
 		   buffer[9] << 16 |
 		   buffer[10] << 24);
 
-	hdmi_write(hdmi, REG_HDMI_AVI_INFO(2),
-		   buffer[11] |
-		   buffer[12] << 8 |
-		   buffer[13] << 16 |
-		   buffer[14] << 24);
+	val = hdmi_read(hdmi, REG_HDMI_INFOFRAME_CTRL1);
+	val |= HDMI_INFOFRAME_CTRL0_AUDIO_INFO_SEND |
+		HDMI_INFOFRAME_CTRL0_AUDIO_INFO_CONT |
+		HDMI_INFOFRAME_CTRL0_AUDIO_INFO_SOURCE |
+		HDMI_INFOFRAME_CTRL0_AUDIO_INFO_UPDATE;
+	hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL0, val);
 
-	hdmi_write(hdmi, REG_HDMI_AVI_INFO(3),
-		   buffer[15] |
-		   buffer[16] << 8 |
-		   buffer[1] << 24);
+	return 0;
+}
 
-	hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL0,
-		   HDMI_INFOFRAME_CTRL0_AVI_SEND |
-		   HDMI_INFOFRAME_CTRL0_AVI_CONT);
+static int msm_hdmi_bridge_clear_infoframe(struct drm_bridge *bridge,
+					   enum hdmi_infoframe_type type)
+{
+	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
+	struct hdmi *hdmi = hdmi_bridge->hdmi;
+	u32 val;
 
-	val = hdmi_read(hdmi, REG_HDMI_INFOFRAME_CTRL1);
-	val &= ~HDMI_INFOFRAME_CTRL1_AVI_INFO_LINE__MASK;
-	val |= HDMI_INFOFRAME_CTRL1_AVI_INFO_LINE(AVI_IFRAME_LINE_NUMBER);
-	hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL1, val);
+	switch (type) {
+	case HDMI_INFOFRAME_TYPE_AVI:
+		val = hdmi_read(hdmi, REG_HDMI_INFOFRAME_CTRL0);
+		val &= ~(HDMI_INFOFRAME_CTRL0_AVI_SEND |
+			 HDMI_INFOFRAME_CTRL0_AVI_CONT);
+		hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL0, val);
+
+		val = hdmi_read(hdmi, REG_HDMI_INFOFRAME_CTRL1);
+		val &= ~HDMI_INFOFRAME_CTRL1_AVI_INFO_LINE__MASK;
+		hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL1, val);
+
+		break;
+
+	case HDMI_INFOFRAME_TYPE_AUDIO:
+		val = hdmi_read(hdmi, REG_HDMI_INFOFRAME_CTRL0);
+		val &= ~(HDMI_INFOFRAME_CTRL0_AUDIO_INFO_SEND |
+			 HDMI_INFOFRAME_CTRL0_AUDIO_INFO_CONT |
+			 HDMI_INFOFRAME_CTRL0_AUDIO_INFO_SOURCE |
+			 HDMI_INFOFRAME_CTRL0_AUDIO_INFO_UPDATE);
+		hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL0, val);
+
+		val = hdmi_read(hdmi, REG_HDMI_INFOFRAME_CTRL1);
+		val &= ~HDMI_INFOFRAME_CTRL1_AUDIO_INFO_LINE__MASK;
+		hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL1, val);
+
+		break;
+
+	default:
+		drm_dbg_driver(hdmi_bridge->base.dev, "Unsupported infoframe type %x\n", type);
+	}
+
+	return 0;
+}
+
+static int msm_hdmi_bridge_write_infoframe(struct drm_bridge *bridge,
+					   enum hdmi_infoframe_type type,
+					   const u8 *buffer, size_t len)
+{
+	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
+	struct hdmi *hdmi = hdmi_bridge->hdmi;
+
+	msm_hdmi_bridge_clear_infoframe(bridge, type);
+
+	switch (type) {
+	case HDMI_INFOFRAME_TYPE_AVI:
+		return msm_hdmi_config_avi_infoframe(hdmi, buffer, len);
+	case HDMI_INFOFRAME_TYPE_AUDIO:
+		return msm_hdmi_config_audio_infoframe(hdmi, buffer, len);
+	default:
+		drm_dbg_driver(hdmi_bridge->base.dev, "Unsupported infoframe type %x\n", type);
+		return 0;
+	}
+}
+
+static int msm_hdmi_bridge_atomic_check(struct drm_bridge *bridge,
+			    struct drm_bridge_state *bridge_state,
+			    struct drm_crtc_state *crtc_state,
+			    struct drm_connector_state *conn_state)
+{
+	return drm_atomic_helper_connector_hdmi_check(conn_state->connector, conn_state->state);
 }
 
 static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
@@ -136,28 +221,22 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	struct drm_encoder *encoder = bridge->encoder;
 	struct drm_connector *connector;
 	struct drm_connector_state *conn_state;
-	struct drm_crtc_state *crtc_state;
-	const struct drm_display_mode *mode;
 
 	DBG("power up");
 
 	connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
 	conn_state = drm_atomic_get_new_connector_state(state, connector);
-	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
-	mode = &crtc_state->adjusted_mode;
 
-	hdmi->pixclock = mode->clock * 1000;
+	hdmi->pixclock = conn_state->hdmi.tmds_char_rate;
 
 	if (!hdmi->power_on) {
 		msm_hdmi_phy_resource_enable(phy);
 		msm_hdmi_power_on(bridge);
 		hdmi->power_on = true;
-		if (hdmi->hdmi_mode) {
-			msm_hdmi_config_avi_infoframe(hdmi);
-			msm_hdmi_audio_update(hdmi);
-		}
 	}
 
+	drm_atomic_helper_connector_hdmi_update_infoframes(connector, state);
+
 	msm_hdmi_phy_powerup(phy, hdmi->pixclock);
 
 	msm_hdmi_set_mode(hdmi, true);
@@ -184,8 +263,6 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
 	if (hdmi->power_on) {
 		power_off(bridge);
 		hdmi->power_on = false;
-		if (hdmi->hdmi_mode)
-			msm_hdmi_audio_update(hdmi);
 		msm_hdmi_phy_resource_disable(phy);
 	}
 }
@@ -252,9 +329,6 @@ static void msm_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
 		frame_ctrl |= HDMI_FRAME_CTRL_INTERLACED_EN;
 	DBG("frame_ctrl=%08x", frame_ctrl);
 	hdmi_write(hdmi, REG_HDMI_FRAME_CTRL, frame_ctrl);
-
-	if (hdmi->hdmi_mode)
-		msm_hdmi_audio_update(hdmi);
 }
 
 static const struct drm_edid *msm_hdmi_bridge_edid_read(struct drm_bridge *bridge,
@@ -297,7 +371,7 @@ static enum drm_mode_status msm_hdmi_bridge_mode_valid(struct drm_bridge *bridge
 	struct msm_kms *kms = priv->kms;
 	long actual, requested;
 
-	requested = 1000 * mode->clock;
+	requested = drm_hdmi_compute_mode_clock(mode, 8, HDMI_COLORSPACE_RGB);
 
 	/* for mdp5/apq8074, we manage our own pixel clk (as opposed to
 	 * mdp4/dtv stuff where pixel clk is assigned to mdp/encoder
@@ -323,12 +397,15 @@ static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.atomic_check = msm_hdmi_bridge_atomic_check,
 	.atomic_pre_enable = msm_hdmi_bridge_atomic_pre_enable,
 	.atomic_enable = msm_hdmi_bridge_atomic_enable,
 	.atomic_post_disable = msm_hdmi_bridge_atomic_post_disable,
 	.mode_valid = msm_hdmi_bridge_mode_valid,
 	.edid_read = msm_hdmi_bridge_edid_read,
 	.detect = msm_hdmi_bridge_detect,
+	.hdmi_clear_infoframe = msm_hdmi_bridge_clear_infoframe,
+	.hdmi_write_infoframe = msm_hdmi_bridge_write_infoframe,
 };
 
 static void
@@ -360,8 +437,11 @@ int msm_hdmi_bridge_init(struct hdmi *hdmi)
 	bridge->funcs = &msm_hdmi_bridge_funcs;
 	bridge->ddc = hdmi->i2c;
 	bridge->type = DRM_MODE_CONNECTOR_HDMIA;
+	bridge->vendor = "Qualcomm";
+	bridge->product = "Snapdragon";
 	bridge->ops = DRM_BRIDGE_OP_HPD |
 		DRM_BRIDGE_OP_DETECT |
+		DRM_BRIDGE_OP_HDMI |
 		DRM_BRIDGE_OP_EDID;
 
 	ret = devm_drm_bridge_add(hdmi->dev->dev, bridge);

-- 
2.39.2

[PATCH v5 7/9] drm/msm/hdmi: get rid of hdmi_mode

[Dmitry Baryshkov]

Use connector->display_info.is_hdmi instead of manually using
drm_detect_hdmi_monitor().

Acked-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c        |  2 +-
 drivers/gpu/drm/msm/hdmi/hdmi.h        |  2 --
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 11 -----------
 3 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 2279e09fd2de..8c6c9dffffd6 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -25,7 +25,7 @@ void msm_hdmi_set_mode(struct hdmi *hdmi, bool power_on)
 	spin_lock_irqsave(&hdmi->reg_lock, flags);
 	if (power_on) {
 		ctrl |= HDMI_CTRL_ENABLE;
-		if (!hdmi->hdmi_mode) {
+		if (!hdmi->connector->display_info.is_hdmi) {
 			ctrl |= HDMI_CTRL_HDMI;
 			hdmi_write(hdmi, REG_HDMI_CTRL, ctrl);
 			ctrl &= ~HDMI_CTRL_HDMI;
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 0ac034eaaf0f..b7fc1c5f1d1e 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -67,8 +67,6 @@ struct hdmi {
 	/* the encoder we are hooked to (outside of hdmi block) */
 	struct drm_encoder *encoder;
 
-	bool hdmi_mode;               /* are we in hdmi mode? */
-
 	int irq;
 	struct workqueue_struct *workq;
 
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 9eecc9960e75..9258d3100042 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -346,17 +346,6 @@ static const struct drm_edid *msm_hdmi_bridge_edid_read(struct drm_bridge *bridg
 
 	hdmi_write(hdmi, REG_HDMI_CTRL, hdmi_ctrl);
 
-	if (drm_edid) {
-		/*
-		 * FIXME: This should use connector->display_info.is_hdmi from a
-		 * path that has read the EDID and called
-		 * drm_edid_connector_update().
-		 */
-		const struct edid *edid = drm_edid_raw(drm_edid);
-
-		hdmi->hdmi_mode = drm_detect_hdmi_monitor(edid);
-	}
-
 	return drm_edid;
 }
 

-- 
2.39.2

Re: [PATCH v5 7/9] drm/msm/hdmi: get rid of hdmi_mode

[Abhinav Kumar]

On 6/7/2024 6:23 AM, Dmitry Baryshkov wrote:
> Use connector->display_info.is_hdmi instead of manually using
> drm_detect_hdmi_monitor().
> 
> Acked-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c        |  2 +-
>   drivers/gpu/drm/msm/hdmi/hdmi.h        |  2 --
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 11 -----------
>   3 files changed, 1 insertion(+), 14 deletions(-)
> 

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

[PATCH v5 8/9] drm/msm/hdmi: update HDMI_GEN_PKT_CTRL_GENERIC0_UPDATE definition

[Dmitry Baryshkov]

The GENERIC0_UPDATE field is a single bit. Redefine it as boolean to
simplify its usage in the driver.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/registers/display/hdmi.xml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/registers/display/hdmi.xml b/drivers/gpu/drm/msm/registers/display/hdmi.xml
index 6c81581016c7..fc711a842363 100644
--- a/drivers/gpu/drm/msm/registers/display/hdmi.xml
+++ b/drivers/gpu/drm/msm/registers/display/hdmi.xml
@@ -131,7 +131,7 @@ xsi:schemaLocation="https://gitlab.freedesktop.org/freedreno/ rules-fd.xsd">
 		 -->
 		<bitfield name="GENERIC0_SEND" pos="0" type="boolean"/>
 		<bitfield name="GENERIC0_CONT" pos="1" type="boolean"/>
-		<bitfield name="GENERIC0_UPDATE" low="2" high="3" type="uint"/> <!-- ??? -->
+		<bitfield name="GENERIC0_UPDATE" pos="2" type="boolean"/>
 		<bitfield name="GENERIC1_SEND" pos="4" type="boolean"/>
 		<bitfield name="GENERIC1_CONT" pos="5" type="boolean"/>
 		<bitfield name="GENERIC0_LINE" low="16" high="21" type="uint"/>

-- 
2.39.2

Re: [PATCH v5 8/9] drm/msm/hdmi: update HDMI_GEN_PKT_CTRL_GENERIC0_UPDATE definition

[Abhinav Kumar]

On 6/7/2024 6:23 AM, Dmitry Baryshkov wrote:
> The GENERIC0_UPDATE field is a single bit. Redefine it as boolean to
> simplify its usage in the driver.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/registers/display/hdmi.xml | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 


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

[PATCH v5 9/9] drm/msm/hdmi: also send the SPD and HDMI Vendor Specific InfoFrames

[Dmitry Baryshkov]

Extend the driver to send SPD and HDMI Vendor Specific InfoFrames.

While the HDMI block has special block to send HVS InfoFrame, use
GENERIC0 block instead. VENSPEC_INFO registers pack frame data in a way
that requires manual repacking in the driver, while GENERIC0 doesn't
have such format requirements. The msm-4.4 kernel uses GENERIC0 to send
HDR InfoFrame which we do not at this point anyway.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 93 ++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 9258d3100042..ad6258a2017a 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -69,6 +69,8 @@ static void power_off(struct drm_bridge *bridge)
 }
 
 #define AVI_IFRAME_LINE_NUMBER 1
+#define SPD_IFRAME_LINE_NUMBER 1
+#define VENSPEC_IFRAME_LINE_NUMBER 3
 
 static int msm_hdmi_config_avi_infoframe(struct hdmi *hdmi,
 					 const u8 *buffer, size_t len)
@@ -142,6 +144,74 @@ static int msm_hdmi_config_audio_infoframe(struct hdmi *hdmi,
 	return 0;
 }
 
+static int msm_hdmi_config_spd_infoframe(struct hdmi *hdmi,
+					 const u8 *buffer, size_t len)
+{
+	u32 buf[7] = {};
+	u32 val;
+	int i;
+
+	if (len != HDMI_INFOFRAME_SIZE(SPD) || len - 3 > sizeof(buf)) {
+		DRM_DEV_ERROR(&hdmi->pdev->dev,
+			"failed to configure SPD infoframe\n");
+		return -EINVAL;
+	}
+
+	/* checksum gets written together with the body of the frame */
+	hdmi_write(hdmi, REG_HDMI_GENERIC1_HDR,
+		   buffer[0] |
+		   buffer[1] << 8 |
+		   buffer[2] << 16);
+
+	memcpy(buf, &buffer[3], len - 3);
+
+	for (i = 0; i < ARRAY_SIZE(buf); i++)
+		hdmi_write(hdmi, REG_HDMI_GENERIC1(i), buf[i]);
+
+	val = hdmi_read(hdmi, REG_HDMI_GEN_PKT_CTRL);
+	val |= HDMI_GEN_PKT_CTRL_GENERIC1_SEND |
+		 HDMI_GEN_PKT_CTRL_GENERIC1_CONT |
+		 HDMI_GEN_PKT_CTRL_GENERIC1_LINE(SPD_IFRAME_LINE_NUMBER);
+	hdmi_write(hdmi, REG_HDMI_GEN_PKT_CTRL, val);
+
+	return 0;
+}
+
+static int msm_hdmi_config_hdmi_infoframe(struct hdmi *hdmi,
+					  const u8 *buffer, size_t len)
+{
+	u32 buf[7] = {};
+	u32 val;
+	int i;
+
+	if (len < HDMI_INFOFRAME_HEADER_SIZE + HDMI_VENDOR_INFOFRAME_SIZE ||
+	    len - 3 > sizeof(buf)) {
+		DRM_DEV_ERROR(&hdmi->pdev->dev,
+			"failed to configure HDMI infoframe\n");
+		return -EINVAL;
+	}
+
+	/* checksum gets written together with the body of the frame */
+	hdmi_write(hdmi, REG_HDMI_GENERIC0_HDR,
+		   buffer[0] |
+		   buffer[1] << 8 |
+		   buffer[2] << 16);
+
+	memcpy(buf, &buffer[3], len - 3);
+
+	for (i = 0; i < ARRAY_SIZE(buf); i++)
+		hdmi_write(hdmi, REG_HDMI_GENERIC0(i), buf[i]);
+
+	val = hdmi_read(hdmi, REG_HDMI_GEN_PKT_CTRL);
+	val |= HDMI_GEN_PKT_CTRL_GENERIC0_SEND |
+		 HDMI_GEN_PKT_CTRL_GENERIC0_CONT |
+		 HDMI_GEN_PKT_CTRL_GENERIC0_UPDATE |
+		 HDMI_GEN_PKT_CTRL_GENERIC0_LINE(VENSPEC_IFRAME_LINE_NUMBER);
+	hdmi_write(hdmi, REG_HDMI_GEN_PKT_CTRL, val);
+
+	return 0;
+}
+
 static int msm_hdmi_bridge_clear_infoframe(struct drm_bridge *bridge,
 					   enum hdmi_infoframe_type type)
 {
@@ -176,6 +246,25 @@ static int msm_hdmi_bridge_clear_infoframe(struct drm_bridge *bridge,
 
 		break;
 
+	case HDMI_INFOFRAME_TYPE_SPD:
+		val = hdmi_read(hdmi, REG_HDMI_GEN_PKT_CTRL);
+		val &= ~(HDMI_GEN_PKT_CTRL_GENERIC1_SEND |
+			 HDMI_GEN_PKT_CTRL_GENERIC1_CONT |
+			 HDMI_GEN_PKT_CTRL_GENERIC1_LINE__MASK);
+		hdmi_write(hdmi, REG_HDMI_GEN_PKT_CTRL, val);
+
+		break;
+
+	case HDMI_INFOFRAME_TYPE_VENDOR:
+		val = hdmi_read(hdmi, REG_HDMI_GEN_PKT_CTRL);
+		val &= ~(HDMI_GEN_PKT_CTRL_GENERIC0_SEND |
+			 HDMI_GEN_PKT_CTRL_GENERIC0_CONT |
+			 HDMI_GEN_PKT_CTRL_GENERIC0_UPDATE |
+			 HDMI_GEN_PKT_CTRL_GENERIC0_LINE__MASK);
+		hdmi_write(hdmi, REG_HDMI_GEN_PKT_CTRL, val);
+
+		break;
+
 	default:
 		drm_dbg_driver(hdmi_bridge->base.dev, "Unsupported infoframe type %x\n", type);
 	}
@@ -197,6 +286,10 @@ static int msm_hdmi_bridge_write_infoframe(struct drm_bridge *bridge,
 		return msm_hdmi_config_avi_infoframe(hdmi, buffer, len);
 	case HDMI_INFOFRAME_TYPE_AUDIO:
 		return msm_hdmi_config_audio_infoframe(hdmi, buffer, len);
+	case HDMI_INFOFRAME_TYPE_SPD:
+		return msm_hdmi_config_spd_infoframe(hdmi, buffer, len);
+	case HDMI_INFOFRAME_TYPE_VENDOR:
+		return msm_hdmi_config_hdmi_infoframe(hdmi, buffer, len);
 	default:
 		drm_dbg_driver(hdmi_bridge->base.dev, "Unsupported infoframe type %x\n", type);
 		return 0;

-- 
2.39.2

Re: [PATCH v5 9/9] drm/msm/hdmi: also send the SPD and HDMI Vendor Specific InfoFrames

[Maxime Ripard]

On Fri, 7 Jun 2024 16:23:06 +0300, Dmitry Baryshkov wrote:
> Extend the driver to send SPD and HDMI Vendor Specific InfoFrames.
> 
> While the HDMI block has special block to send HVS InfoFrame, use
> GENERIC0 block instead. VENSPEC_INFO registers pack frame data in a way
> that requires manual repacking in the driver, while GENERIC0 doesn't
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: (subset) [PATCH v5 0/9] drm/msm: make use of the HDMI connector infrastructure

[Dmitry Baryshkov]

On Fri, 07 Jun 2024 16:22:57 +0300, Dmitry Baryshkov wrote:
> This patchset sits on top Maxime's HDMI connector patchset ([1]).
> 
> Currently this is an RFC exploring the interface between HDMI bridges
> and HDMI connector code. This has been lightly verified on the Qualcomm
> DB820c, which has native HDMI output. If this approach is considered to
> be acceptable, I'll finish MSM HDMI bridge conversion (reworking the
> Audio Infoframe code). Other bridges can follow the same approach (we
> have lt9611 / lt9611uxc / adv7511 on Qualcomm hardware).
> 
> [...]

Applied to drm-misc-next, thanks!

[1/9] drm/connector: hdmi: allow disabling Audio Infoframe
      commit: 000d1940c90984a9a2af9c02bc17e3ca0d87f71d
[2/9] drm/bridge-connector: switch to using drmm allocations
      commit: c12907be57b16eed5a73f75a44ebea8f30629c85
[3/9] drm/bridge-connector: implement glue code for HDMI connector
      commit: 6b4468b0c6ba37a16795da567b58dc80bc7fb439

Best regards,
-- 
With best wishes
Dmitry

Re: [PATCH v5 0/9] drm/msm: make use of the HDMI connector infrastructure

[Dmitry Baryshkov]

On Fri, 07 Jun 2024 16:22:57 +0300, Dmitry Baryshkov wrote:
> This patchset sits on top Maxime's HDMI connector patchset ([1]).
> 
> Currently this is an RFC exploring the interface between HDMI bridges
> and HDMI connector code. This has been lightly verified on the Qualcomm
> DB820c, which has native HDMI output. If this approach is considered to
> be acceptable, I'll finish MSM HDMI bridge conversion (reworking the
> Audio Infoframe code). Other bridges can follow the same approach (we
> have lt9611 / lt9611uxc / adv7511 on Qualcomm hardware).
> 
> [...]

Applied, thanks!

[4/9] drm/msm/hdmi: switch to atomic bridge callbacks
      https://gitlab.freedesktop.org/lumag/msm/-/commit/4fd10fa0b573
[6/9] drm/msm/hdmi: make use of the drm_connector_hdmi framework
      https://gitlab.freedesktop.org/lumag/msm/-/commit/aaa38235b5fe
[7/9] drm/msm/hdmi: get rid of hdmi_mode
      https://gitlab.freedesktop.org/lumag/msm/-/commit/97d6442e8f9e
[8/9] drm/msm/hdmi: update HDMI_GEN_PKT_CTRL_GENERIC0_UPDATE definition
      https://gitlab.freedesktop.org/lumag/msm/-/commit/917921a20294
[9/9] drm/msm/hdmi: also send the SPD and HDMI Vendor Specific InfoFrames
      https://gitlab.freedesktop.org/lumag/msm/-/commit/bf28d52a20b1

Per discussion on the mailing list, 5/9 is deferred.

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