[alsa-devel] [RFC PATCH v3 2/7] drm/i915: Add support for audio driver notifications

[Anand, Jerome]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Tuesday, November 29, 2016 1:58 PM
> To: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>; Anand, Jerome
> <jerome.anand at intel.com>; intel-gfx at lists.freedesktop.org; alsa-
> devel at alsa-project.org; broonie at kernel.org; Ughreja, Rakesh A
> <rakesh.a.ughreja at 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