-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, November 29, 2016 1:58 PM To: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com; Anand, Jerome jerome.anand@intel.com; intel-gfx@lists.freedesktop.org; alsa- devel@alsa-project.org; broonie@kernel.org; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications
On Mon, 28 Nov 2016 22:56:24 +0100, Pierre-Louis Bossart wrote:
On 11/28/16 1:30 PM, Ville Syrjälä wrote:
On Mon, Nov 28, 2016 at 01:13:31PM -0600, Pierre-Louis Bossart wrote:
On 11/28/16 11:01 AM, Ville Syrjälä wrote:
>>>> + if (pdata->notify_audio_lpe) >>>> + pdata->notify_audio_lpe( >>>> + (eld != NULL) ? &pdata->eld :
NULL);
>>>> + else >>>> + pdata->notify_pending = true; >>> Still not sure why the "pending" thing is useful. Can't the >>> audio driver just do its thing (whatever it is) unconditionally? >>> >> This is added to avoid race when audio driver loads late and >> the notification > from display has already passed. > > You keep saying that but I can't see it. > I have seen this happen - before audio driver is loaded, codec enable
completes and notification is sent to the audio driver.
Since the audio callbacks are not initialized, notification gets missed.
Sure. But what does the extra notification_pending flag buy us? The audio driver could just check the eld/tmds_clock/port directly.
>>> When disabling just clear the port to INVALID, eld to zero, >>> and tmds clock to 0, and it should all be fine no? >>> >> Yes, that's what is being done. > Where? > Notify callback will have eld to NULL and tmds to zero sent in codec_disable
But the driver can look those thigns up directly as well it seems. So this whole thing is a bit of a mess on account of sharing the platform as a communication channel and also trying to pass the things as paraameters to the notify hook. I think we need to pick one or the other approach, not some mismash of both.
Indeed it looks weird to have both a parameter for tmds_clock in the pdata AND the notify parameter, this can probably be cleaned-up.
That said, I am not sure I completely understand the feedback that the audio driver can get all the eld/tmds/port information directly. We are trying to avoid accessing the data structures of the i915
driver.
IIRC what I proposed originally didn't even expose the same structure to both sides, but that's not what we seem to have atm.
Are you suggesting a scheme where the i915 driver would just provide a door-bell like notification and the audio driver would use a get_eld/tmds/port interface exposed by the i915 driver on startup and upon receiving this notification?
Well, we could do it that way, or we'd do it the other way that the audio driver just calls i915 to triggers a single i915->audio notify call after the audio driver has finished its probe. Or we could do whatever we seem to have now is where the audio driver can just root around directly in the structure (at which point passing any parameters in the notify calls seems redundant as well).
Looking at some older emails, i think you recommended a 'register' callback to let the audio driver signal to the i915 side it completed its initialization, with a single notify generated if needed (what you described just above as 'the other way')
If you look at the path 4 of the series,
I seem to have missed the whole series by some reason, both to my inbox and to ML. Could you resubmit later?
Yes - I will resend rfc v4 after addressing some of Ville's comment.
Jerome was trying to implement this with a 'notify_pending' field in the platform data set by the i915 side and used during the audio driver probe
Yes - thanks Pierre. I believe Ville's comment was addressed with this change. Am not sure if there is anything critically missed out leading to a flaw
- if (pdata->notify_pending) {
pr_debug("%s: handle pending notification\n", __func__);notify_audio_lpe(&pdata->eld);pdata->notify_pending = false;- }
Maybe an explicit handshake is more self-explanatory and safer?
IMO yes, the pending notification flag isn't intrusive.
Yes - I thought it was simpler to handle the notification in this manner
There are cases where the audio driver wants to inquire the status from gfx side; at least, the HD-audio driver needed to recheck after PM.
Am not sure if there is any case like that to be handled here. Hence a simpler interface was created and implemented
Also, isn't the code above racy without a proper mutex?
No
thanks,
Takashi