[1/4] drm/i915: Add audio hotplug info struct
diff mbox

Message ID 1437465447-8974-2-git-send-email-david.henningsson@canonical.com
State New
Headers show

Commit Message

David Henningsson July 21, 2015, 7:57 a.m. UTC
This struct will be used to transfer information from the i915
driver to the hda driver on HDMI hotplug events.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 include/drm/i915_component.h |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Takashi Iwai July 22, 2015, 8:22 a.m. UTC | #1
On Tue, 21 Jul 2015 09:57:24 +0200,
David Henningsson wrote:
> 
> This struct will be used to transfer information from the i915
> driver to the hda driver on HDMI hotplug events.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>

Looks good to me, just a few nitpicking:

> ---
>  include/drm/i915_component.h |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index c9a8b64..4fc0db3 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -24,8 +24,22 @@
>  #ifndef _I915_COMPONENT_H_
>  #define _I915_COMPONENT_H_
>  
> +struct hdac_bus;
> +
> +struct i915_audio_hotplug_info {
> +	int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */
> +	int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type, meant for userspace */
> +	int port; /* Used for mapping to affected nid */
> +	int port_multi_stream_device; /* For DP multi-streaming */
> +
> +	bool plugged_in;
> +	uint8_t *eld;

Use u8 or just unsigned char as it's a in-kernel API.
Also, safer to add const, since this is read-only for audio side.

> +	int eld_size;
> +};
> +
>  struct i915_audio_component {
>  	struct device *dev;
> +	struct hdac_bus *hdac_bus;

If we want to be more generic, using a struct device would be better,
e.g.
	struct device *audio_dev;

>  	const struct i915_audio_component_ops {
>  		struct module *owner;
> @@ -34,6 +48,11 @@ struct i915_audio_component {
>  		void (*codec_wake_override)(struct device *, bool enable);
>  		int (*get_cdclk_freq)(struct device *);
>  	} *ops;
> +
> +	const struct i915_audio_component_cb_ops {
> +		struct module *owner;

Do we need the owner field at all?

> +		void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);
> +	} *cb_ops;

cb_ops doesn't sound intuitive.  Any better name?


thanks,

Takashi
David Henningsson July 22, 2015, 8:50 a.m. UTC | #2
On 2015-07-22 10:22, Takashi Iwai wrote:
> On Tue, 21 Jul 2015 09:57:24 +0200,
> David Henningsson wrote:
>>
>> This struct will be used to transfer information from the i915
>> driver to the hda driver on HDMI hotplug events.
>>
>> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
>
> Looks good to me, just a few nitpicking:
>
>> ---
>>   include/drm/i915_component.h |   19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
>> index c9a8b64..4fc0db3 100644
>> --- a/include/drm/i915_component.h
>> +++ b/include/drm/i915_component.h
>> @@ -24,8 +24,22 @@
>>   #ifndef _I915_COMPONENT_H_
>>   #define _I915_COMPONENT_H_
>>
>> +struct hdac_bus;
>> +
>> +struct i915_audio_hotplug_info {
>> +	int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */
>> +	int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type, meant for userspace */
>> +	int port; /* Used for mapping to affected nid */
>> +	int port_multi_stream_device; /* For DP multi-streaming */
>> +
>> +	bool plugged_in;
>> +	uint8_t *eld;
>
> Use u8 or just unsigned char as it's a in-kernel API.
> Also, safer to add const, since this is read-only for audio side.

Ok.

>
>> +	int eld_size;
>> +};
>> +
>>   struct i915_audio_component {
>>   	struct device *dev;
>> +	struct hdac_bus *hdac_bus;
>
> If we want to be more generic, using a struct device would be better,
> e.g.
> 	struct device *audio_dev;

Does this work? If we want to have the hdac_bus.dev ptr instead of a 
hdac_bus ptr, there does not seem to be an obvious way to go from the 
audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an 
arbitrary dev pointer).

>>   	const struct i915_audio_component_ops {
>>   		struct module *owner;
>> @@ -34,6 +48,11 @@ struct i915_audio_component {
>>   		void (*codec_wake_override)(struct device *, bool enable);
>>   		int (*get_cdclk_freq)(struct device *);
>>   	} *ops;
>> +
>> +	const struct i915_audio_component_cb_ops {
>> +		struct module *owner;
>
> Do we need the owner field at all?

It was merely for symmetry. I'll remove it for v2.

>> +		void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);
>> +	} *cb_ops;
>
> cb_ops doesn't sound intuitive.  Any better name?

I was thinking of it as "callback ops", i e, calls that go in the 
reverse direction compared to the already existing "ops".

But if we call the device "audio_dev" as you suggested above, then maybe 
"audio_ops" would be nice and symmetric?
Takashi Iwai July 22, 2015, 8:55 a.m. UTC | #3
On Wed, 22 Jul 2015 10:50:03 +0200,
David Henningsson wrote:
> 
> >>   struct i915_audio_component {
> >>   	struct device *dev;
> >> +	struct hdac_bus *hdac_bus;
> >
> > If we want to be more generic, using a struct device would be better,
> > e.g.
> > 	struct device *audio_dev;
> 
> Does this work? If we want to have the hdac_bus.dev ptr instead of a 
> hdac_bus ptr, there does not seem to be an obvious way to go from the 
> audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an 
> arbitrary dev pointer).

Hrm, right, currently it's not straightforward.  Scratch the idea,
then.


> >> +		void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);
> >> +	} *cb_ops;
> >
> > cb_ops doesn't sound intuitive.  Any better name?
> 
> I was thinking of it as "callback ops", i e, calls that go in the 
> reverse direction compared to the already existing "ops".
> 
> But if we call the device "audio_dev" as you suggested above, then maybe 
> "audio_ops" would be nice and symmetric?

Yes, it sounds better.


Takashi
Vinod Koul July 22, 2015, 2:13 p.m. UTC | #4
On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
> On Wed, 22 Jul 2015 10:50:03 +0200,
> David Henningsson wrote:
> > 
> > >>   struct i915_audio_component {
> > >>   	struct device *dev;
> > >> +	struct hdac_bus *hdac_bus;
> > >
> > > If we want to be more generic, using a struct device would be better,
> > > e.g.
> > > 	struct device *audio_dev;
> > 
> > Does this work? If we want to have the hdac_bus.dev ptr instead of a 
> > hdac_bus ptr, there does not seem to be an obvious way to go from the 
> > audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an 
> > arbitrary dev pointer).
> 
> Hrm, right, currently it's not straightforward.  Scratch the idea,
> then.

That depends on the device we register this with. Actually this makes more
sense to me :)

If we register with struct device *audio_dev, which in this case would be
the codec device we create while probing the bus. This way you are linking i915
ops to the codec device. Ofcourse hdac_device has bus pointer but you can
invoke device callback without even searching for the device :)
David Henningsson July 22, 2015, 5:52 p.m. UTC | #5
On 2015-07-22 16:13, Vinod Koul wrote:
> On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
>> On Wed, 22 Jul 2015 10:50:03 +0200,
>> David Henningsson wrote:
>>>
>>>>>    struct i915_audio_component {
>>>>>    	struct device *dev;
>>>>> +	struct hdac_bus *hdac_bus;
>>>>
>>>> If we want to be more generic, using a struct device would be better,
>>>> e.g.
>>>> 	struct device *audio_dev;
>>>
>>> Does this work? If we want to have the hdac_bus.dev ptr instead of a
>>> hdac_bus ptr, there does not seem to be an obvious way to go from the
>>> audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an
>>> arbitrary dev pointer).
>>
>> Hrm, right, currently it's not straightforward.  Scratch the idea,
>> then.
>
> That depends on the device we register this with. Actually this makes more
> sense to me :)
>
> If we register with struct device *audio_dev, which in this case would be
> the codec device we create while probing the bus. This way you are linking i915
> ops to the codec device. Ofcourse hdac_device has bus pointer but you can
> invoke device callback without even searching for the device :)

It would require some extra setup, so I skipped it (at least for now).

I e, in order to detect codecs, we need to call hdac_i915 functions to 
turn the power well on. And in order to connect the codec to the 
i915_audio_component, we need to have detected a codec.

Which, now that I think of it, actually gives an interesting potential 
race condition, in case the following happens:

1) Monitor is plugged in at boot time
2) i915 initializes
3) hda starts initializing and sets up the audio component
4) i915 finishes initialization and as part of that, calls the hotplug 
notify to let hda know that the monitor is plugged in. However, at this 
point, hda has not finished initialization yet, so there are no codecs 
that listen for this information.

Anyhow, this is not a problem really yet, but it might be if we ever 
decide to not write the ELD to the hardware.
Takashi Iwai July 22, 2015, 8:31 p.m. UTC | #6
On Wed, 22 Jul 2015 19:52:23 +0200,
David Henningsson wrote:
> 
> 
> 
> On 2015-07-22 16:13, Vinod Koul wrote:
> > On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
> >> On Wed, 22 Jul 2015 10:50:03 +0200,
> >> David Henningsson wrote:
> >>>
> >>>>>    struct i915_audio_component {
> >>>>>    	struct device *dev;
> >>>>> +	struct hdac_bus *hdac_bus;
> >>>>
> >>>> If we want to be more generic, using a struct device would be better,
> >>>> e.g.
> >>>> 	struct device *audio_dev;
> >>>
> >>> Does this work? If we want to have the hdac_bus.dev ptr instead of a
> >>> hdac_bus ptr, there does not seem to be an obvious way to go from the
> >>> audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an
> >>> arbitrary dev pointer).
> >>
> >> Hrm, right, currently it's not straightforward.  Scratch the idea,
> >> then.
> >
> > That depends on the device we register this with. Actually this makes more
> > sense to me :)
> >
> > If we register with struct device *audio_dev, which in this case would be
> > the codec device we create while probing the bus. This way you are linking i915
> > ops to the codec device. Ofcourse hdac_device has bus pointer but you can
> > invoke device callback without even searching for the device :)
> 
> It would require some extra setup, so I skipped it (at least for now).
> 
> I e, in order to detect codecs, we need to call hdac_i915 functions to 
> turn the power well on. And in order to connect the codec to the 
> i915_audio_component, we need to have detected a codec.
> 
> Which, now that I think of it, actually gives an interesting potential 
> race condition, in case the following happens:
> 
> 1) Monitor is plugged in at boot time
> 2) i915 initializes
> 3) hda starts initializing and sets up the audio component
> 4) i915 finishes initialization and as part of that, calls the hotplug 
> notify to let hda know that the monitor is plugged in. However, at this 
> point, hda has not finished initialization yet, so there are no codecs 
> that listen for this information.
> 
> Anyhow, this is not a problem really yet, but it might be if we ever 
> decide to not write the ELD to the hardware.

For avoid such a problem, maybe we need two ops, one for notification
and one for getting the assigned data.  At the initialization time,
the audio driver queries the assigned status and data.  When a hotplug
happens, it's just notified.  That is, it simply replaces the current
unsol event and the ELD data read via two ops.


Takashi
Vinod Koul July 23, 2015, 3:43 a.m. UTC | #7
On Wed, Jul 22, 2015 at 10:31:45PM +0200, Takashi Iwai wrote:
> > > That depends on the device we register this with. Actually this makes more
> > > sense to me :)
> > >
> > > If we register with struct device *audio_dev, which in this case would be
> > > the codec device we create while probing the bus. This way you are linking i915
> > > ops to the codec device. Ofcourse hdac_device has bus pointer but you can
> > > invoke device callback without even searching for the device :)
> > 
> > It would require some extra setup, so I skipped it (at least for now).
> > 
> > I e, in order to detect codecs, we need to call hdac_i915 functions to 
> > turn the power well on. And in order to connect the codec to the 
> > i915_audio_component, we need to have detected a codec.
Yes that is true but when driver registers the callback you can assign the
callback to i915 component, so afterwards the call from i915 lands up in
codec. If not registered we have default bus handler :)

> > Which, now that I think of it, actually gives an interesting potential 
> > race condition, in case the following happens:
> > 
> > 1) Monitor is plugged in at boot time
> > 2) i915 initializes
> > 3) hda starts initializing and sets up the audio component
> > 4) i915 finishes initialization and as part of that, calls the hotplug 
> > notify to let hda know that the monitor is plugged in. However, at this 
> > point, hda has not finished initialization yet, so there are no codecs 
> > that listen for this information.
> > 
> > Anyhow, this is not a problem really yet, but it might be if we ever 
> > decide to not write the ELD to the hardware.
> 
> For avoid such a problem, maybe we need two ops, one for notification
> and one for getting the assigned data.  At the initialization time,
> the audio driver queries the assigned status and data.  When a hotplug
> happens, it's just notified.  That is, it simply replaces the current
> unsol event and the ELD data read via two ops.
Yeah, I do favour adding new ops so that we can query the current values and
setup whenever driver probes
David Henningsson July 23, 2015, 6:17 a.m. UTC | #8
On 2015-07-22 22:31, Takashi Iwai wrote:
> On Wed, 22 Jul 2015 19:52:23 +0200,
> David Henningsson wrote:
>>
>>
>>
>> On 2015-07-22 16:13, Vinod Koul wrote:
>>> On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
>>>> On Wed, 22 Jul 2015 10:50:03 +0200,
>>>> David Henningsson wrote:
>>>>>
>>>>>>>     struct i915_audio_component {
>>>>>>>     	struct device *dev;
>>>>>>> +	struct hdac_bus *hdac_bus;
>>>>>>
>>>>>> If we want to be more generic, using a struct device would be better,
>>>>>> e.g.
>>>>>> 	struct device *audio_dev;
>>>>>
>>>>> Does this work? If we want to have the hdac_bus.dev ptr instead of a
>>>>> hdac_bus ptr, there does not seem to be an obvious way to go from the
>>>>> audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an
>>>>> arbitrary dev pointer).
>>>>
>>>> Hrm, right, currently it's not straightforward.  Scratch the idea,
>>>> then.
>>>
>>> That depends on the device we register this with. Actually this makes more
>>> sense to me :)
>>>
>>> If we register with struct device *audio_dev, which in this case would be
>>> the codec device we create while probing the bus. This way you are linking i915
>>> ops to the codec device. Ofcourse hdac_device has bus pointer but you can
>>> invoke device callback without even searching for the device :)
>>
>> It would require some extra setup, so I skipped it (at least for now).
>>
>> I e, in order to detect codecs, we need to call hdac_i915 functions to
>> turn the power well on. And in order to connect the codec to the
>> i915_audio_component, we need to have detected a codec.
>>
>> Which, now that I think of it, actually gives an interesting potential
>> race condition, in case the following happens:
>>
>> 1) Monitor is plugged in at boot time
>> 2) i915 initializes
>> 3) hda starts initializing and sets up the audio component
>> 4) i915 finishes initialization and as part of that, calls the hotplug
>> notify to let hda know that the monitor is plugged in. However, at this
>> point, hda has not finished initialization yet, so there are no codecs
>> that listen for this information.
>>
>> Anyhow, this is not a problem really yet, but it might be if we ever
>> decide to not write the ELD to the hardware.
>
> For avoid such a problem, maybe we need two ops, one for notification
> and one for getting the assigned data.  At the initialization time,
> the audio driver queries the assigned status and data.  When a hotplug
> happens, it's just notified.  That is, it simply replaces the current
> unsol event and the ELD data read via two ops.

I'm about to go on vacation so it would be good to get some closure 
here. If you both prefer this setup, how about I remove "struct 
i915_audio_hotplug_info" for now? We will then have:

	const struct i915_audio_component_audio_ops {
		void (*hotplug_notify)(struct hdac_bus *);
	}

...which will make the hda driver read from the hardware. Asking the 
i915 driver for ELD, connector and hotplug status can then be done in a 
later patch.
David Henningsson July 23, 2015, 6:25 a.m. UTC | #9
On 2015-07-23 08:17, David Henningsson wrote:
> I'm about to go on vacation so it would be good to get some closure
> here. If you both prefer this setup, how about I remove "struct
> i915_audio_hotplug_info" for now? We will then have:
>
>      const struct i915_audio_component_audio_ops {
>          void (*hotplug_notify)(struct hdac_bus *);
>      }

Sorry, it would look like this:

       const struct i915_audio_component_audio_ops {
           void (*hotplug_notify)(struct hdac_bus *, int port, int 
port_mst_index);
       }

...to indicate what port needs updating.
Takashi Iwai July 23, 2015, 10:02 a.m. UTC | #10
On Thu, 23 Jul 2015 08:25:21 +0200,
David Henningsson wrote:
> 
> 
> 
> On 2015-07-23 08:17, David Henningsson wrote:
> > I'm about to go on vacation so it would be good to get some closure
> > here. If you both prefer this setup, how about I remove "struct
> > i915_audio_hotplug_info" for now? We will then have:
> >
> >      const struct i915_audio_component_audio_ops {
> >          void (*hotplug_notify)(struct hdac_bus *);
> >      }
> 
> Sorry, it would look like this:
> 
>        const struct i915_audio_component_audio_ops {
>            void (*hotplug_notify)(struct hdac_bus *, int port, int 
> port_mst_index);
>        }
> 
> ...to indicate what port needs updating.

Yes, I think this is simpler.

A remaining question is whether it should be notified to bus or
codec.  In the latter case, we need to allow registering the audio
codec after binding the component.

I find the codec can be a bit better, as this is directly targeted
(while for bus you need to look through the codec list).  But it's
just a minor difference, and I don't mind so much about this, if there
is any other difficulty by that move.


thanks,

Takashi

Patch
diff mbox

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..4fc0db3 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -24,8 +24,22 @@ 
 #ifndef _I915_COMPONENT_H_
 #define _I915_COMPONENT_H_
 
+struct hdac_bus;
+
+struct i915_audio_hotplug_info {
+	int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */
+	int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type, meant for userspace */
+	int port; /* Used for mapping to affected nid */
+	int port_multi_stream_device; /* For DP multi-streaming */
+
+	bool plugged_in;
+	uint8_t *eld;
+	int eld_size;
+};
+
 struct i915_audio_component {
 	struct device *dev;
+	struct hdac_bus *hdac_bus;
 
 	const struct i915_audio_component_ops {
 		struct module *owner;
@@ -34,6 +48,11 @@  struct i915_audio_component {
 		void (*codec_wake_override)(struct device *, bool enable);
 		int (*get_cdclk_freq)(struct device *);
 	} *ops;
+
+	const struct i915_audio_component_cb_ops {
+		struct module *owner;
+		void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);
+	} *cb_ops;
 };
 
 #endif /* _I915_COMPONENT_H_ */