diff mbox series

[RFC,v2,5/5] drm/msm/hdmi: make use of the drm_connector_hdmi framework

Message ID 20240309-bridge-hdmi-connector-v2-5-1380bea3ee70@linaro.org (mailing list archive)
State New, archived
Headers show
Series drm/msm: make use of the HDMI connector infrastructure | expand

Commit Message

Dmitry Baryshkov March 9, 2024, 10:31 a.m. UTC 
Setup the HDMI connector on the MSM HDMI outputs. Make use of
atomic_check hook and of the provided Infoframe infrastructure.

Note: for now only AVI Infoframes are enabled. Audio Infoframes are
currenly handled separately. This will be fixed for the final version.

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

Comments

Maxime Ripard March 11, 2024, 3:46 p.m. UTC | #1 
Hi,

On Sat, Mar 09, 2024 at 12:31:32PM +0200, Dmitry Baryshkov wrote:
> Setup the HDMI connector on the MSM HDMI outputs. Make use of
> atomic_check hook and of the provided Infoframe infrastructure.
> 
> Note: for now only AVI Infoframes are enabled. Audio Infoframes are
> currenly handled separately. This will be fixed for the final version.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I had a look at the driver, and it looks like mode_set and mode_valid
could use the connector_state tmds_char_rate instead of pixclock and
drm_connector_hdmi_compute_mode_clock respectively instead of
calculating it by themselves.

We can probably remove hdmi->pixclock entirely if we manage to pass the
connector state to msm_hdmi_power_on.

And that's unrelated to this series, but we can also remove
hdmi->hdmi_mode for drm_display_info.is_hdmi.

Maxime
Dmitry Baryshkov March 11, 2024, 3:55 p.m. UTC | #2 
On Mon, 11 Mar 2024 at 17:46, Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Sat, Mar 09, 2024 at 12:31:32PM +0200, Dmitry Baryshkov wrote:
> > Setup the HDMI connector on the MSM HDMI outputs. Make use of
> > atomic_check hook and of the provided Infoframe infrastructure.
> >
> > Note: for now only AVI Infoframes are enabled. Audio Infoframes are
> > currenly handled separately. This will be fixed for the final version.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> I had a look at the driver, and it looks like mode_set and mode_valid
> could use the connector_state tmds_char_rate instead of pixclock and
> drm_connector_hdmi_compute_mode_clock respectively instead of
> calculating it by themselves.

Ack, I'll take a look.b

>
> We can probably remove hdmi->pixclock entirely if we manage to pass the
> connector state to msm_hdmi_power_on.

I'd like to defer this for a moment, I have a pending series moving
MSM HDMI PHY drivers to generic PHY subsystem. However that patchset
reworks the way the PHY is setup, so it doesn't make sense to rework
msm_hdmi_power_on().

>
> And that's unrelated to this series, but we can also remove
> hdmi->hdmi_mode for drm_display_info.is_hdmi.

Yes, that's the plan, once I rework the audio infoframe handling.
Maxime Ripard March 11, 2024, 5:06 p.m. UTC | #3 
On Mon, Mar 11, 2024 at 05:55:36PM +0200, Dmitry Baryshkov wrote:
> On Mon, 11 Mar 2024 at 17:46, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > Hi,
> >
> > On Sat, Mar 09, 2024 at 12:31:32PM +0200, Dmitry Baryshkov wrote:
> > > Setup the HDMI connector on the MSM HDMI outputs. Make use of
> > > atomic_check hook and of the provided Infoframe infrastructure.
> > >
> > > Note: for now only AVI Infoframes are enabled. Audio Infoframes are
> > > currenly handled separately. This will be fixed for the final version.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >
> > I had a look at the driver, and it looks like mode_set and mode_valid
> > could use the connector_state tmds_char_rate instead of pixclock and
> > drm_connector_hdmi_compute_mode_clock respectively instead of
> > calculating it by themselves.
> 
> Ack, I'll take a look.b
> 
> >
> > We can probably remove hdmi->pixclock entirely if we manage to pass the
> > connector state to msm_hdmi_power_on.
> 
> I'd like to defer this for a moment, I have a pending series moving
> MSM HDMI PHY drivers to generic PHY subsystem. However that patchset
> reworks the way the PHY is setup, so it doesn't make sense to rework
> msm_hdmi_power_on().
> 
> >
> > And that's unrelated to this series, but we can also remove
> > hdmi->hdmi_mode for drm_display_info.is_hdmi.
> 
> Yes, that's the plan, once I rework the audio infoframe handling.

Sure, if it makes more sense to defer it for now, then let's postpone it
:)

Maxime
Dmitry Baryshkov March 11, 2024, 5:12 p.m. UTC | #4 
On Mon, 11 Mar 2024 at 19:06, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Mon, Mar 11, 2024 at 05:55:36PM +0200, Dmitry Baryshkov wrote:
> > On Mon, 11 Mar 2024 at 17:46, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > Hi,
> > >
> > > On Sat, Mar 09, 2024 at 12:31:32PM +0200, Dmitry Baryshkov wrote:
> > > > Setup the HDMI connector on the MSM HDMI outputs. Make use of
> > > > atomic_check hook and of the provided Infoframe infrastructure.
> > > >
> > > > Note: for now only AVI Infoframes are enabled. Audio Infoframes are
> > > > currenly handled separately. This will be fixed for the final version.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >
> > > I had a look at the driver, and it looks like mode_set and mode_valid
> > > could use the connector_state tmds_char_rate instead of pixclock and
> > > drm_connector_hdmi_compute_mode_clock respectively instead of
> > > calculating it by themselves.
> >
> > Ack, I'll take a look.b
> >
> > >
> > > We can probably remove hdmi->pixclock entirely if we manage to pass the
> > > connector state to msm_hdmi_power_on.
> >
> > I'd like to defer this for a moment, I have a pending series moving
> > MSM HDMI PHY drivers to generic PHY subsystem. However that patchset
> > reworks the way the PHY is setup, so it doesn't make sense to rework
> > msm_hdmi_power_on().
> >
> > >
> > > And that's unrelated to this series, but we can also remove
> > > hdmi->hdmi_mode for drm_display_info.is_hdmi.
> >
> > Yes, that's the plan, once I rework the audio infoframe handling.
>
> Sure, if it makes more sense to defer it for now, then let's postpone it
> :)

I hope to fix this one for v3. Audio InfoFrame should be converted too.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index d839c71091dc..26c847f42522 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -68,23 +68,15 @@  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 val;
-	int len;
 
-	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)) {
 		DRM_DEV_ERROR(&hdmi->pdev->dev,
 			"failed to configure avi infoframe\n");
-		return;
+		return -EINVAL;
 	}
 
 	/*
@@ -124,6 +116,55 @@  static void msm_hdmi_config_avi_infoframe(struct hdmi *hdmi)
 	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_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_CTRL0);
+
+	switch (type) {
+	case HDMI_INFOFRAME_TYPE_AVI:
+		val &= ~(HDMI_INFOFRAME_CTRL0_AVI_SEND |
+			 HDMI_INFOFRAME_CTRL0_AVI_CONT);
+		break;
+	case HDMI_INFOFRAME_TYPE_AUDIO:
+		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);
+		break;
+	default:
+		drm_dbg_driver(hdmi_bridge->base.dev, "Unsupported infoframe type %x\n", type);
+	}
+
+	hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL0, val);
+
+	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);
+	default:
+		drm_dbg_driver(hdmi_bridge->base.dev, "Unsupported infoframe type %x\n", type);
+		return 0;
+	}
 }
 
 static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
@@ -132,6 +173,10 @@  static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 	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_atomic_state *state = old_bridge_state->base.state;
+	struct drm_connector *connector =
+		drm_atomic_get_new_connector_for_encoder(state, encoder);
 
 	DBG("power up");
 
@@ -139,12 +184,13 @@  static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 		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);
+
+	if (hdmi->hdmi_mode)
+		msm_hdmi_audio_update(hdmi);
+
 	msm_hdmi_phy_powerup(phy, hdmi->pixclock);
 
 	msm_hdmi_set_mode(hdmi, true);
@@ -310,6 +356,8 @@  static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = {
 	.mode_valid = msm_hdmi_bridge_mode_valid,
 	.edid_read = msm_hdmi_bridge_edid_read,
 	.detect = msm_hdmi_bridge_detect,
+	.clear_infoframe = msm_hdmi_bridge_clear_infoframe,
+	.write_infoframe = msm_hdmi_bridge_write_infoframe,
 };
 
 static void
@@ -341,8 +389,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);