Hi Ville,
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Wednesday, August 26, 2015 8:27 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 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.
OK, I will keep the original name as it will be more clear for audio driver developer.
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.
OK, mutex is OK for audio driver, too.
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.
The function is called before playing audio.
Regards, Libin