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. > 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@xxxxxxxxxxxxxxxxxxxxx > > > > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > > > > > -- > > > > > > Ville Syrjälä > > > > > > Intel OTC > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel OTC > > > > -- > > Ville Syrjälä > > Intel OTC -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
