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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx