From: Daniel Vetter <daniel@ffwll.ch>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org,
Mengdong Lin <mengdong.lin@linux.intel.com>,
Vinod Koul <vinod.koul@intel.com>,
intel-gfx@lists.freedesktop.org,
Daniel Vetter <daniel.vetter@intel.com>,
David Henningsson <david.henningsson@canonical.com>
Subject: Re: [PATCH v3 2/4] drm/i915: Add reverse mapping between port and intel_encoder
Date: Thu, 10 Dec 2015 10:38:14 +0100 [thread overview]
Message-ID: <20151210093814.GJ20822@phenom.ffwll.local> (raw)
In-Reply-To: <s5h7fko6dn2.wl-tiwai@suse.de>
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.
Do you plan to prep a topic branch with all the patches that we could pull
into both snd and drm-intel trees?
Note that for 4.5 features drm-intel closes next week, so not that much
time left.
-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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2015-12-10 9:38 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-04 17:12 [PATCH v3 0/4] Add get_eld audio component for i915/HD-audio Takashi Iwai
2015-12-04 17:12 ` [PATCH v3 1/4] drm/i915: Add get_eld audio component Takashi Iwai
2015-12-07 8:42 ` Daniel Vetter
2015-12-04 17:12 ` [PATCH v3 2/4] drm/i915: Add reverse mapping between port and intel_encoder Takashi Iwai
2015-12-07 8:44 ` Daniel Vetter
2015-12-07 9:41 ` Takashi Iwai
2015-12-08 17:42 ` Takashi Iwai
2015-12-10 9:38 ` Daniel Vetter [this message]
2015-12-10 9:47 ` [Intel-gfx] " Takashi Iwai
2015-12-10 10:06 ` Daniel Vetter
2015-12-07 10:30 ` Jani Nikula
2015-12-07 10:37 ` Takashi Iwai
2015-12-04 17:12 ` [PATCH v3 3/4] ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling Takashi Iwai
2015-12-04 17:12 ` [PATCH v3 4/4] ALSA: hda - Move audio component accesses to hdac_i915.c Takashi Iwai
2015-12-07 4:48 ` Vinod Koul
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151210093814.GJ20822@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=alsa-devel@alsa-project.org \
--cc=daniel.vetter@intel.com \
--cc=david.henningsson@canonical.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=mengdong.lin@linux.intel.com \
--cc=tiwai@suse.de \
--cc=vinod.koul@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.