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

[Takashi Iwai]

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?


> 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
> 
> +	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.

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.

Also, isn't the code above racy without a proper mutex?


thanks,

Takashi