On Tue, Aug 25, 2015 at 01:48:13AM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Tuesday, August 25, 2015 12:27 AM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Mon, Aug 24, 2015 at 03:35:33PM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Monday, August 24, 2015 8:53 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Friday, August 21, 2015 11:14 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Tue, Aug 18, 2015 at 02:51:52PM +0800,
libin.yang@intel.com
wrote: > +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; > + enum pipe pipe = -1; > + u32 tmp; > + int n_low, n_up, n; > + > + /* 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);
This sort of thing would need some locking to safely start
digging
at
the modeset state.
I would probably not do that, and instead add some new lock(s)
for
this.
The modeset code would then fill out some relevant
information
protected by that same lock, and this function could then go look at it
without
having to worry about the rest of modeset locking/state.
From audio side, there is no competition when calling the
function,
I think. For protecting the competition between audio and gfx driver, it seems we need use the lock from gfx side. Do you have
suggested
lock I can use?
To do it like this we'd need pretty much all the modeset locks,
which
to me feels a bit troublesome if the audio driver can call this at any point. So I suggested that we may want to add some kind of
new
audio lock to i915.
If we are using a new audio lock, I believe we still need use the lock when gfx is doing the modeset. Only adding the new lock in the function i915_audio_component_sync_audio_rate() is not enough because gfx driver will still modify it without locking.
I'm not sure where to add the lock in the gfx driver.
The audio enable/disable during modeset would also grab the lock, and consult whatever information the audio driver provided. I would also suggest renaming the .sync_audio_rate() to something more clear like .set_sample_rate()
The sample rate in audio means the how often the audio data is sampled. Actually this function is none of setting sample rate.
Well it's more of sample_rate_change_notify() or something like that. But I don't mind too much what it's called. We can go with your original name if you feel it's more clear.
The sample rate setting is done in audio driver. What the function does is only setting the n/cts based on the audio sample rate.
Anyway, I was thinking of something like this:
intel_audio_codec_enable() { lock(); .. configure n/cts encoder->audio.enabled = true; unlock(); }
intel_audio_codec_disable() { lock(); encoder->audio.enabled = false; unlock(); }
set_sample_rate() { lock(); encoder->audio.sample_rate = rate; if (encoder->audio.enabled) { ... reconfigure n/cts } unlock(); }
I'm still not sure whether it is safe when gfx driver is doing the modeset. Suppose gfx driver will handle the protection, right?
intel_audio_codec_enable/disable are called during the modeset. With my idea as long as you're holding the lock and audio.enabled==true you could be sure there's nothing bad happening at the same time in i915.
If so, I will add spin_lock_irqsave as you suggested.
Could be a mutex I think.
Something like that should protect sample rate changes from racing with an ongoing modeset.
Btw, it is not changing the sample rate. Audio sample rate will never be changed in these functions.
The audio driver can change the sample rate at any point, at which point it'll call into i915 to reconfigure n/cts, no? So we just need to make sure things can't blow up if it does that while there's a modeset happening at the same time.
Regards, Libin
Regards, Libin
> + if (!crtc) { > + DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__); > + continue; > + } > + pipe = crtc->pipe; > + break; > + } > + } > + > + if (pipe == INVALID_PIPE) { > + DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port)); > + return -ENODEV; > + } > + DRM_DEBUG_KMS("pipe %c connects port %c\n", > + pipe_name(pipe),
port_name(port));
> + mode = &crtc->config->base.adjusted_mode; > + > + /* 2. check whether to set the N/CTS/M manually or
not */
> + if (!audio_rate_need_prog(crtc, mode)) { > + tmp = I915_READ(HSW_AUD_CFG(pipe)); > + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; > + I915_WRITE(HSW_AUD_CFG(pipe), tmp);
This stuff would need to be abstracted to work on pre-HSW too.
Right, I need judge the platform firstly.
> + return 0; > + } > + > + n = audio_config_get_n(mode, rate); > + if (n == 0) { > + DRM_DEBUG_KMS("Using automatic mode for
N value
on port %c\n", > + port_name(port)); > + tmp = I915_READ(HSW_AUD_CFG(pipe)); > + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; > + I915_WRITE(HSW_AUD_CFG(pipe), tmp); > + return 0; > + } > + n_low = n & 0xfff; > + n_up = (n >> 12) & 0xff; > + > + /* 4. set the N/CTS/M */ > + tmp = I915_READ(HSW_AUD_CFG(pipe)); > + tmp &= ~(AUD_CONFIG_UPPER_N_MASK | AUD_CONFIG_LOWER_N_MASK); > + tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) | > + (n_low <<
AUD_CONFIG_LOWER_N_SHIFT) |
> + AUD_CONFIG_N_PROG_ENABLE); > + I915_WRITE(HSW_AUD_CFG(pipe), tmp); > + > + return 0; > +} > + > static const struct i915_audio_component_ops i915_audio_component_ops = { > .owner = THIS_MODULE, > .get_power = i915_audio_component_get_power, > .put_power = i915_audio_component_put_power, > .codec_wake_override = i915_audio_component_codec_wake_override, > .get_cdclk_freq =
i915_audio_component_get_cdclk_freq,
> + .sync_audio_rate =
i915_audio_component_sync_audio_rate,
> }; > > static int i915_audio_component_bind(struct device
*i915_dev,
> -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC