> -----Original Message----- > From: Takashi Iwai [mailto:tiwai@xxxxxxx] > Sent: Tuesday, November 29, 2016 1:58 PM > To: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; Anand, Jerome > <jerome.anand@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; alsa- > devel@xxxxxxxxxxxxxxxx; broonie@xxxxxxxxxx; Ughreja, Rakesh A > <rakesh.a.ughreja@xxxxxxxxx> > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
