Hi Ville, > -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > Sent: Wednesday, August 26, 2015 8:27 PM > To: Yang, Libin > Cc: alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; daniel.vetter@xxxxxxxx; > jani.nikula@xxxxxxxxxxxxxxx > Subject: Re: [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@xxxxxxxxxxxxxxx] > > > Sent: Tuesday, August 25, 2015 12:27 AM > > > To: Yang, Libin > > > Cc: alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; intel- > > > gfx@xxxxxxxxxxxxxxxxxxxxx; daniel.vetter@xxxxxxxx; > > > jani.nikula@xxxxxxxxxxxxxxx > > > Subject: Re: [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@xxxxxxxxxxxxxxx] > > > > > Sent: Monday, August 24, 2015 8:53 PM > > > > > To: Yang, Libin > > > > > Cc: alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; intel- > > > > > gfx@xxxxxxxxxxxxxxxxxxxxx; daniel.vetter@xxxxxxxx; > > > > > jani.nikula@xxxxxxxxxxxxxxx > > > > > Subject: Re: [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@xxxxxxxxxxxxxxx] > > > > > > > Sent: Friday, August 21, 2015 11:14 PM > > > > > > > To: Yang, Libin > > > > > > > Cc: alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; intel- > > > > > > > gfx@xxxxxxxxxxxxxxxxxxxxx; daniel.vetter@xxxxxxxx; > > > > > > > jani.nikula@xxxxxxxxxxxxxxx > > > > > > > Subject: Re: [PATCH v5 2/4] drm/i915: implement > > > > > > > sync_audio_rate callback > > > > > > > > > > > > > > On Tue, Aug 18, 2015 at 02:51:52PM +0800, > > > libin.yang@xxxxxxxxx > > > > > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
