[v2,1/4] drm/i915: Add audio set_ncts callback
diff mbox

Message ID 1439191931-25705-1-git-send-email-libin.yang@intel.com
State New
Headers show

Commit Message

Yang, Libin Aug. 10, 2015, 7:32 a.m. UTC
From: Libin Yang <libin.yang@intel.com>

Add the set_ncts callback.

With the callback, audio driver can trigger
i915 driver to set the proper N/CTS
based on different sample rates.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 include/drm/i915_component.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

Jani Nikula Aug. 10, 2015, 11:46 a.m. UTC | #1
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@intel.com>
>
> Add the set_ncts callback.
>
> With the callback, audio driver can trigger
> i915 driver to set the proper N/CTS
> based on different sample rates.
>
> Signed-off-by: Libin Yang <libin.yang@intel.com>
> ---
>  include/drm/i915_component.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index c9a8b64..7305881 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -33,6 +33,8 @@ struct i915_audio_component {
>  		void (*put_power)(struct device *);
>  		void (*codec_wake_override)(struct device *, bool enable);
>  		int (*get_cdclk_freq)(struct device *);
> +		int (*set_ncts)(struct device *, int port, int dev_entry,
> +					int rate);

I'd rather call this set_audio_rate or similar, and dropping the
references to N and CTS. The caller does not need to know.

I'm also not fond of adding a dev_entry parameter and leaving it
unused. I do not think we know specifically how we're going to identify
MST sinks, and the interface may need to be changed anyway. Let's force
an update in the caller side as well when there's actually sensible
support in our side.

BR,
Jani.

>  	} *ops;
>  };
>  
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Yang, Libin Aug. 11, 2015, 2:40 a.m. UTC | #2
Hi Jani,

Thanks for reviewing, please see my comments

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Monday, August 10, 2015 7:46 PM
> To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
> Subject: Re: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts
> callback
> 
> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> > From: Libin Yang <libin.yang@intel.com>
> >
> > Add the set_ncts callback.
> >
> > With the callback, audio driver can trigger
> > i915 driver to set the proper N/CTS
> > based on different sample rates.
> >
> > Signed-off-by: Libin Yang <libin.yang@intel.com>
> > ---
> >  include/drm/i915_component.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/include/drm/i915_component.h
> b/include/drm/i915_component.h
> > index c9a8b64..7305881 100644
> > --- a/include/drm/i915_component.h
> > +++ b/include/drm/i915_component.h
> > @@ -33,6 +33,8 @@ struct i915_audio_component {
> >  		void (*put_power)(struct device *);
> >  		void (*codec_wake_override)(struct device *, bool
> enable);
> >  		int (*get_cdclk_freq)(struct device *);
> > +		int (*set_ncts)(struct device *, int port, int dev_entry,
> > +					int rate);
> 
> I'd rather call this set_audio_rate or similar, and dropping the
> references to N and CTS. The caller does not need to know.

But it seems the set_audio_rate will confuse the user. From the
literal meaning, it is setting the rate of audio, such as sample rate,
which will make audio driver developers confused.
And the function is just setting N/CTS which is defined from 
HDMI SPEC.

BTW: your comment reminds me that get_cdclk_freq() seems
to need change the name as cdclk is not in the open spec.

> 
> I'm also not fond of adding a dev_entry parameter and leaving it
> unused. I do not think we know specifically how we're going to identify
> MST sinks, and the interface may need to be changed anyway. Let's
> force
> an update in the caller side as well when there's actually sensible
> support in our side.

The device entry is a concept in HDA spec. For driver implementation,
I think we can use an int variable or a struct device to represent it.
A int variable is an easy way. There is some place in audio driver using
int variable to represent the deivce entry. And audio driver will not
care how gfx driver supports the DP1.2 MST, I mean audio driver will
not know the structures inside gfx driver.

I will remove this parameter in this version if you are thinking the
MST interface is indeterminate. 

BTW: do you know when gfx will normally support MST?

Best Regards,
Libin

> 
> BR,
> Jani.
> 
> >  	} *ops;
> >  };
> >
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Jani Nikula, Intel Open Source Technology Center
Yang, Libin Aug. 11, 2015, 5:51 a.m. UTC | #3
> -----Original Message-----
> From: Yang, Libin
> Sent: Tuesday, August 11, 2015 10:41 AM
> To: 'Jani Nikula'; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
> Subject: RE: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts
> callback
> 
> Hi Jani,
> 
> Thanks for reviewing, please see my comments
> 
> > -----Original Message-----
> > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > Sent: Monday, August 10, 2015 7:46 PM
> > To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> > gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
> > Subject: Re: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts
> > callback
> >
> > On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> > > From: Libin Yang <libin.yang@intel.com>
> > >
> > > Add the set_ncts callback.
> > >
> > > With the callback, audio driver can trigger
> > > i915 driver to set the proper N/CTS
> > > based on different sample rates.
> > >
> > > Signed-off-by: Libin Yang <libin.yang@intel.com>
> > > ---
> > >  include/drm/i915_component.h | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/include/drm/i915_component.h
> > b/include/drm/i915_component.h
> > > index c9a8b64..7305881 100644
> > > --- a/include/drm/i915_component.h
> > > +++ b/include/drm/i915_component.h
> > > @@ -33,6 +33,8 @@ struct i915_audio_component {
> > >  		void (*put_power)(struct device *);
> > >  		void (*codec_wake_override)(struct device *, bool
> > enable);
> > >  		int (*get_cdclk_freq)(struct device *);
> > > +		int (*set_ncts)(struct device *, int port, int dev_entry,
> > > +					int rate);
> >
> > I'd rather call this set_audio_rate or similar, and dropping the
> > references to N and CTS. The caller does not need to know.
> 
> But it seems the set_audio_rate will confuse the user. From the
> literal meaning, it is setting the rate of audio, such as sample rate,
> which will make audio driver developers confused.
> And the function is just setting N/CTS which is defined from
> HDMI SPEC.

After a second thought, set_ncts is not very good, we should
consider DP's naming and handling DP mode together.

> 
> BTW: your comment reminds me that get_cdclk_freq() seems
> to need change the name as cdclk is not in the open spec.
> 
> >
> > I'm also not fond of adding a dev_entry parameter and leaving it
> > unused. I do not think we know specifically how we're going to
> identify
> > MST sinks, and the interface may need to be changed anyway. Let's
> > force
> > an update in the caller side as well when there's actually sensible
> > support in our side.
> 
> The device entry is a concept in HDA spec. For driver implementation,
> I think we can use an int variable or a struct device to represent it.
> A int variable is an easy way. There is some place in audio driver using
> int variable to represent the deivce entry. And audio driver will not
> care how gfx driver supports the DP1.2 MST, I mean audio driver will
> not know the structures inside gfx driver.
> 
> I will remove this parameter in this version if you are thinking the
> MST interface is indeterminate.
> 
> BTW: do you know when gfx will normally support MST?
> 
> Best Regards,
> Libin
> 
> >
> > BR,
> > Jani.
> >
> > >  	} *ops;
> > >  };
> > >
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Jani Nikula, Intel Open Source Technology Center
Yang, Libin Aug. 12, 2015, 3:16 a.m. UTC | #4
Hi,

> -----Original Message-----
> From: Yang, Libin
> Sent: Tuesday, August 11, 2015 10:41 AM
> To: Jani Nikula; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
> Subject: RE: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts
> callback
> 
> Hi Jani,
> 
> Thanks for reviewing, please see my comments
> 
> > -----Original Message-----
> > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > Sent: Monday, August 10, 2015 7:46 PM
> > To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> > gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
> > Subject: Re: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts
> > callback
> >
> > On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> > > From: Libin Yang <libin.yang@intel.com>
> > >
> > > Add the set_ncts callback.
> > >
> > > With the callback, audio driver can trigger
> > > i915 driver to set the proper N/CTS
> > > based on different sample rates.
> > >
> > > Signed-off-by: Libin Yang <libin.yang@intel.com>
> > > ---
> > >  include/drm/i915_component.h | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/include/drm/i915_component.h
> > b/include/drm/i915_component.h
> > > index c9a8b64..7305881 100644
> > > --- a/include/drm/i915_component.h
> > > +++ b/include/drm/i915_component.h
> > > @@ -33,6 +33,8 @@ struct i915_audio_component {
> > >  		void (*put_power)(struct device *);
> > >  		void (*codec_wake_override)(struct device *, bool
> > enable);
> > >  		int (*get_cdclk_freq)(struct device *);
> > > +		int (*set_ncts)(struct device *, int port, int dev_entry,
> > > +					int rate);
> >
> > I'd rather call this set_audio_rate or similar, and dropping the
> > references to N and CTS. The caller does not need to know.
> 
> But it seems the set_audio_rate will confuse the user. From the
> literal meaning, it is setting the rate of audio, such as sample rate,
> which will make audio driver developers confused.
> And the function is just setting N/CTS which is defined from
> HDMI SPEC.

What about the name sync_audio_rate()?

> 
> BTW: your comment reminds me that get_cdclk_freq() seems
> to need change the name as cdclk is not in the open spec.
> 
> >
> > I'm also not fond of adding a dev_entry parameter and leaving it
> > unused. I do not think we know specifically how we're going to
> identify
> > MST sinks, and the interface may need to be changed anyway. Let's
> > force
> > an update in the caller side as well when there's actually sensible
> > support in our side.
> 
> The device entry is a concept in HDA spec. For driver implementation,
> I think we can use an int variable or a struct device to represent it.
> A int variable is an easy way. There is some place in audio driver using
> int variable to represent the deivce entry. And audio driver will not
> care how gfx driver supports the DP1.2 MST, I mean audio driver will
> not know the structures inside gfx driver.
> 
> I will remove this parameter in this version if you are thinking the
> MST interface is indeterminate.
> 
> BTW: do you know when gfx will normally support MST?
> 
> Best Regards,
> Libin
> 
> >
> > BR,
> > Jani.
> >
> > >  	} *ops;
> > >  };
> > >
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Jani Nikula, Intel Open Source Technology Center
Jani Nikula Aug. 12, 2015, 6:14 a.m. UTC | #5
On Wed, 12 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> Hi,
>
>> -----Original Message-----
>> From: Yang, Libin
>> Sent: Tuesday, August 11, 2015 10:41 AM
>> To: Jani Nikula; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
>> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
>> Subject: RE: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts
>> callback
>> 
>> Hi Jani,
>> 
>> Thanks for reviewing, please see my comments
>> 
>> > -----Original Message-----
>> > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> > Sent: Monday, August 10, 2015 7:46 PM
>> > To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
>> > gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
>> > Subject: Re: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts
>> > callback
>> >
>> > On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
>> > > From: Libin Yang <libin.yang@intel.com>
>> > >
>> > > Add the set_ncts callback.
>> > >
>> > > With the callback, audio driver can trigger
>> > > i915 driver to set the proper N/CTS
>> > > based on different sample rates.
>> > >
>> > > Signed-off-by: Libin Yang <libin.yang@intel.com>
>> > > ---
>> > >  include/drm/i915_component.h | 2 ++
>> > >  1 file changed, 2 insertions(+)
>> > >
>> > > diff --git a/include/drm/i915_component.h
>> > b/include/drm/i915_component.h
>> > > index c9a8b64..7305881 100644
>> > > --- a/include/drm/i915_component.h
>> > > +++ b/include/drm/i915_component.h
>> > > @@ -33,6 +33,8 @@ struct i915_audio_component {
>> > >  		void (*put_power)(struct device *);
>> > >  		void (*codec_wake_override)(struct device *, bool
>> > enable);
>> > >  		int (*get_cdclk_freq)(struct device *);
>> > > +		int (*set_ncts)(struct device *, int port, int dev_entry,
>> > > +					int rate);
>> >
>> > I'd rather call this set_audio_rate or similar, and dropping the
>> > references to N and CTS. The caller does not need to know.
>> 
>> But it seems the set_audio_rate will confuse the user. From the
>> literal meaning, it is setting the rate of audio, such as sample rate,
>> which will make audio driver developers confused.
>> And the function is just setting N/CTS which is defined from
>> HDMI SPEC.
>
> What about the name sync_audio_rate()?

I'm fine with that.

BR,
Jani.

>
>> 
>> BTW: your comment reminds me that get_cdclk_freq() seems
>> to need change the name as cdclk is not in the open spec.
>> 
>> >
>> > I'm also not fond of adding a dev_entry parameter and leaving it
>> > unused. I do not think we know specifically how we're going to
>> identify
>> > MST sinks, and the interface may need to be changed anyway. Let's
>> > force
>> > an update in the caller side as well when there's actually sensible
>> > support in our side.
>> 
>> The device entry is a concept in HDA spec. For driver implementation,
>> I think we can use an int variable or a struct device to represent it.
>> A int variable is an easy way. There is some place in audio driver using
>> int variable to represent the deivce entry. And audio driver will not
>> care how gfx driver supports the DP1.2 MST, I mean audio driver will
>> not know the structures inside gfx driver.
>> 
>> I will remove this parameter in this version if you are thinking the
>> MST interface is indeterminate.
>> 
>> BTW: do you know when gfx will normally support MST?
>> 
>> Best Regards,
>> Libin
>> 
>> >
>> > BR,
>> > Jani.
>> >
>> > >  	} *ops;
>> > >  };
>> > >
>> > > --
>> > > 1.9.1
>> > >
>> > > _______________________________________________
>> > > Intel-gfx mailing list
>> > > Intel-gfx@lists.freedesktop.org
>> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> > --
>> > Jani Nikula, Intel Open Source Technology Center

Patch
diff mbox

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..7305881 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -33,6 +33,8 @@  struct i915_audio_component {
 		void (*put_power)(struct device *);
 		void (*codec_wake_override)(struct device *, bool enable);
 		int (*get_cdclk_freq)(struct device *);
+		int (*set_ncts)(struct device *, int port, int dev_entry,
+					int rate);
 	} *ops;
 };