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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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, 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?

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx