Re: [alsa-devel] [Intel-gfx] [PATCH v3 2/4] drm/i915: Add reverse mapping between port and intel_encoder

On Thu, Dec 10, 2015 at 10:47:50AM +0100, Takashi Iwai wrote:

On Thu, 10 Dec 2015 10:38:14 +0100, Daniel Vetter wrote:

...

On Tue, Dec 08, 2015 at 06:42:09PM +0100, Takashi Iwai wrote:

...

On Mon, 07 Dec 2015 10:41:51 +0100, Takashi Iwai wrote:

...

On Mon, 07 Dec 2015 09:44:45 +0100, Daniel Vetter wrote:

...

On Fri, Dec 04, 2015 at 06:12:47PM +0100, Takashi Iwai wrote:

...

This patch adds a reverse mapping from a digital port number to intel_encoder object containing the corresponding intel_digital_port. It simplifies the query of the encoder a lot.

Signed-off-by: Takashi Iwai tiwai@suse.de

v2->v3:

• Squashed previously two cleanup patches to a single patch

drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_audio.c | 39 +++++++++++--------------------------- drivers/gpu/drm/i915/intel_ddi.c | 1 + drivers/gpu/drm/i915/intel_dp.c | 1 + drivers/gpu/drm/i915/intel_hdmi.c | 2 ++ 5 files changed, 17 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 15c6dc0b4f37..9dbc143cac27 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1944,6 +1944,8 @@ struct drm_i915_private { /* perform PHY state sanity checks? */ bool chv_phy_assert[2];

• struct intel_encoder *dig_port_map[I915_MAX_PORTS];
• /*
  • NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
  • will be rejected. Instead look for a better place.

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index eeac9f763110..05f08b445dd6 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -636,13 +636,11 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, int port, int rate) { struct drm_i915_private *dev_priv = dev_to_i915(dev);

• struct drm_device *drm_dev = dev_priv->dev; struct intel_encoder *intel_encoder;
• struct intel_digital_port *intel_dig_port; struct intel_crtc *crtc; struct drm_display_mode *mode; struct i915_audio_component *acomp = dev_priv->audio_component;
• enum pipe pipe = -1;

• enum pipe pipe = INVALID_PIPE; u32 tmp; int n;

@@ -655,21 +653,12 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,

mutex_lock(&dev_priv->av_mutex); /* 1. get the pipe */

• for_each_intel_encoder(drm_dev, intel_encoder) {

• if (intel_encoder->type != INTEL_OUTPUT_HDMI)
  

• 	continue;
  

• intel_dig_port = enc_to_dig_port(&intel_encoder->base);
  

• if (port == intel_dig_port->port) {
  

• 	crtc = to_intel_crtc(intel_encoder->base.crtc);
  

• 	if (!crtc) {
  

• 		DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
  

• 		continue;
  

• 	}
  

• 	pipe = crtc->pipe;
  

• 	break;
  

• }
  

• intel_encoder = dev_priv->dig_port_map[port];
• if (intel_encoder && intel_encoder->type == INTEL_OUTPUT_HDMI &&

Is it really legit to call these functions with a port for which we don't have an encoder? WARN_ON(!encoder) here and in the other places?

Currently there are little checks in the caller side. So I guess only a few of them deserve WARN_ON(). The empty encoder and empty crtc would be good with WARN_ON(), as the port is supposed to be real. The HDMI check may be silent as is.

...

Or would that require some function for snd-hda to inquire i915 which ports are enabled (which we probably should have to avoid registering audio ports that aren't there)?

Otherwise lgtm.

OK, I'll add WARN_ON() to them.

It turned out that the encoder might be NULL for MST, as currently it's not set properly yet. So WARN_ON() will splat too much unnecessarily.

Though, I rewrote a bit about the check and give a bit more messages. Below is the revised patch. So far, I have only this change, so I hesitate to resubmit the whole patchset. If a full patchset is still preferred, let me know.

Imo this note about mst should be in the commit message. lgtm otherwise.

OK, I'll update the changelog.

...

Do you plan to prep a topic branch with all the patches that we could pull into both snd and drm-intel trees?

Yes, currently test/hdmi-jack branch contains these based on drm-next and some sound updates.

...

Note that for 4.5 features drm-intel closes next week, so not that much time left.

Yeah, I'd like to merge this stuff soonishly, too.

Great. When it's all ready can you pls send me a pull req?

Thanks, Daniel

thanks,

Takashi

...

-Daniel

...

thanks,

Takashi

-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH v4] drm/i915: Add reverse mapping between port and intel_encoder

This patch adds a reverse mapping from a digital port number to intel_encoder object containing the corresponding intel_digital_port. It simplifies the query of the encoder a lot.

Signed-off-by: Takashi Iwai tiwai@suse.de

v3->v4: add a bit more verbose check for NULL encoder, etc.

drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_audio.c | 57 +++++++++++++++----------------------- drivers/gpu/drm/i915/intel_ddi.c | 1 + drivers/gpu/drm/i915/intel_dp.c | 1 + drivers/gpu/drm/i915/intel_hdmi.c | 2 ++ 5 files changed, 28 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 15c6dc0b4f37..9dbc143cac27 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1944,6 +1944,8 @@ struct drm_i915_private { /* perform PHY state sanity checks? */ bool chv_phy_assert[2];

• struct intel_encoder *dig_port_map[I915_MAX_PORTS];
• /*
  • NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
  • will be rejected. Instead look for a better place.

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index eeac9f763110..6380b2400448 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -636,15 +636,14 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, int port, int rate) { struct drm_i915_private *dev_priv = dev_to_i915(dev);

• struct drm_device *drm_dev = dev_priv->dev; struct intel_encoder *intel_encoder;
• struct intel_digital_port *intel_dig_port; struct intel_crtc *crtc; struct drm_display_mode *mode; struct i915_audio_component *acomp = dev_priv->audio_component;
• enum pipe pipe = -1;

• enum pipe pipe = INVALID_PIPE; u32 tmp; int n;

• int err = 0;

  /* HSW, BDW, SKL, KBL need this fix */ if (!IS_SKYLAKE(dev_priv) &&

@@ -655,26 +654,21 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,

mutex_lock(&dev_priv->av_mutex); /* 1. get the pipe */

• for_each_intel_encoder(drm_dev, intel_encoder) {

• if (intel_encoder->type != INTEL_OUTPUT_HDMI)
  

• 	continue;
  

• intel_dig_port = enc_to_dig_port(&intel_encoder->base);
  

• if (port == intel_dig_port->port) {
  

• 	crtc = to_intel_crtc(intel_encoder->base.crtc);
  

• 	if (!crtc) {
  

• 		DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
  

• 		continue;
  

• 	}
  

• 	pipe = crtc->pipe;
  

• 	break;
  

• }
  

• intel_encoder = dev_priv->dig_port_map[port];
• if (!intel_encoder || !intel_encoder->base.crtc ||

•    intel_encoder->type != INTEL_OUTPUT_HDMI) {
  

• DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
  

• err = -ENODEV;
  

• goto unlock;
  

  }

• crtc = to_intel_crtc(intel_encoder->base.crtc);
• pipe = crtc->pipe; if (pipe == INVALID_PIPE) { DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));

• mutex_unlock(&dev_priv->av_mutex);
  

• return -ENODEV;
  

• err = -ENODEV;
  

• goto unlock;
  

  }
• DRM_DEBUG_KMS("pipe %c connects port %c\n", pipe_name(pipe), port_name(port)); mode = &crtc->config->base.adjusted_mode;

@@ -687,8 +681,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_PROG_ENABLE; I915_WRITE(HSW_AUD_CFG(pipe), tmp);

• mutex_unlock(&dev_priv->av_mutex);
  

• return 0;
  

• goto unlock;
  

  }

  n = audio_config_get_n(mode, rate);

@@ -698,8 +691,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_PROG_ENABLE; I915_WRITE(HSW_AUD_CFG(pipe), tmp);

• mutex_unlock(&dev_priv->av_mutex);
  

• return 0;
  

• goto unlock;
  

  }

  /* 3. set the N/CTS/M */

@@ -707,8 +699,9 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, tmp = audio_config_setup_n_reg(n, tmp); I915_WRITE(HSW_AUD_CFG(pipe), tmp);

• unlock: mutex_unlock(&dev_priv->av_mutex);

• return 0;

• return err;

}

static int i915_audio_component_get_eld(struct device *dev, int port, @@ -716,27 +709,21 @@ static int i915_audio_component_get_eld(struct device *dev, int port, unsigned char *buf, int max_bytes) { struct drm_i915_private *dev_priv = dev_to_i915(dev);

• struct drm_device *drm_dev = dev_priv->dev; struct intel_encoder *intel_encoder; struct intel_digital_port *intel_dig_port; const u8 *eld; int ret = -EINVAL;

  mutex_lock(&dev_priv->av_mutex);

• for_each_intel_encoder(drm_dev, intel_encoder) {

• if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&
  

•     intel_encoder->type != INTEL_OUTPUT_HDMI)
  

• 	continue;
  

• intel_encoder = dev_priv->dig_port_map[port];
• if (intel_encoder) {

• ret = 0;
  

  intel_dig_port = enc_to_dig_port(&intel_encoder->base);

• if (port == intel_dig_port->port) {
  

• 	ret = 0;
  

• 	*enabled = intel_dig_port->audio_connector != NULL;
  

• 	if (!*enabled)
  

• 		break;
  

• *enabled = intel_dig_port->audio_connector != NULL;
  

• if (*enabled) {
  eld = intel_dig_port->audio_connector->eld;
  ret = drm_eld_size(eld);
  memcpy(buf, eld, min(max_bytes, ret));
  

• 	break;
  

  } }

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 76ce7c2960b6..59deb0d85533 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -3295,6 +3295,7 @@ void intel_ddi_init(struct drm_device *dev, enum port port) intel_encoder->get_config = intel_ddi_get_config;

intel_dig_port->port = port;

• dev_priv->dig_port_map[port] = intel_encoder; intel_dig_port->saved_port_bits = I915_READ(DDI_BUF_CTL(port)) & (DDI_BUF_PORT_REVERSAL | DDI_A_4_LANES);

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index e1ceff7ab265..e1456ead5c53 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -6003,6 +6003,7 @@ intel_dp_init(struct drm_device *dev, }

intel_dig_port->port = port;

• dev_priv->dig_port_map[port] = intel_encoder; intel_dig_port->dp.output_reg = output_reg;

  intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index bdd462e7c690..c046017be786 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -2148,6 +2148,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port, void intel_hdmi_init(struct drm_device *dev, i915_reg_t hdmi_reg, enum port port) {

• struct drm_i915_private *dev_priv = dev->dev_private; struct intel_digital_port *intel_dig_port; struct intel_encoder *intel_encoder; struct intel_connector *intel_connector;

@@ -2216,6 +2217,7 @@ void intel_hdmi_init(struct drm_device *dev, intel_encoder->cloneable |= 1 << INTEL_OUTPUT_HDMI;

intel_dig_port->port = port;

• dev_priv->dig_port_map[port] = intel_encoder; intel_dig_port->hdmi.hdmi_reg = hdmi_reg; intel_dig_port->dp.output_reg = INVALID_MMIO_REG;

-- 2.6.3

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch