| Message ID | 1409661367-19047-4-git-send-email-subhransu.s.prusty@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 02, 2014 at 06:05:58PM +0530, Subhransu S. Prusty wrote: > Some controls need to initialize stuffs like pvt data, so they need a > callback if the control creation is successful. Adding Takashi - this is ALSA core code so he needs to review it. > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > --- > include/sound/control.h | 3 +++ > sound/core/control.c | 7 +++++++ > 2 files changed, 10 insertions(+) > > diff --git a/include/sound/control.h b/include/sound/control.h > index 0426139..1389f69 100644 > --- a/include/sound/control.h > +++ b/include/sound/control.h > @@ -30,6 +30,7 @@ struct snd_kcontrol; > typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_info * uinfo); > typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol); > typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol); > +typedef int (snd_kcontrol_init_t) (struct snd_kcontrol * kcontrol); > typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol, > int op_flag, /* SNDRV_CTL_TLV_OP_XXX */ > unsigned int size, > @@ -52,6 +53,7 @@ struct snd_kcontrol_new { > snd_kcontrol_info_t *info; > snd_kcontrol_get_t *get; > snd_kcontrol_put_t *put; > + snd_kcontrol_init_t *init; > union { > snd_kcontrol_tlv_rw_t *c; > const unsigned int *p; > @@ -71,6 +73,7 @@ struct snd_kcontrol { > snd_kcontrol_info_t *info; > snd_kcontrol_get_t *get; > snd_kcontrol_put_t *put; > + snd_kcontrol_init_t *init; > union { > snd_kcontrol_tlv_rw_t *c; > const unsigned int *p; > diff --git a/sound/core/control.c b/sound/core/control.c > index b961134..9d30663 100644 > --- a/sound/core/control.c > +++ b/sound/core/control.c > @@ -256,6 +256,7 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, > kctl.info = ncontrol->info; > kctl.get = ncontrol->get; > kctl.put = ncontrol->put; > + kctl.init = ncontrol->init; > kctl.tlv.p = ncontrol->tlv.p; > kctl.private_value = ncontrol->private_value; > kctl.private_data = private_data; > @@ -362,6 +363,12 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) > err = -ENOMEM; > goto error; > } > + if (kcontrol->init) { > + err = kcontrol->init(kcontrol); > + if (err < 0) > + goto error; > + } > + > list_add_tail(&kcontrol->list, &card->controls); > card->controls_count += kcontrol->count; > kcontrol->id.numid = card->last_numid + 1; > -- > 1.9.0 > >
At Sat, 6 Sep 2014 15:21:24 +0100, Mark Brown wrote: > > On Tue, Sep 02, 2014 at 06:05:58PM +0530, Subhransu S. Prusty wrote: > > Some controls need to initialize stuffs like pvt data, so they need a > > callback if the control creation is successful. > > Adding Takashi - this is ALSA core code so he needs to review it. This would bloat effectively the data size of all sound drivers, so I can't ack it without more proper reasoning. That is, please convince me why this change must be taken for the cost of size bloat of all sound drivers. Can't you do it in the caller side of snd_ctl_add() like many other drivers already do? thanks, Takashi > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > --- > > include/sound/control.h | 3 +++ > > sound/core/control.c | 7 +++++++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/include/sound/control.h b/include/sound/control.h > > index 0426139..1389f69 100644 > > --- a/include/sound/control.h > > +++ b/include/sound/control.h > > @@ -30,6 +30,7 @@ struct snd_kcontrol; > > typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_info * uinfo); > > typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol); > > typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol); > > +typedef int (snd_kcontrol_init_t) (struct snd_kcontrol * kcontrol); > > typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol, > > int op_flag, /* SNDRV_CTL_TLV_OP_XXX */ > > unsigned int size, > > @@ -52,6 +53,7 @@ struct snd_kcontrol_new { > > snd_kcontrol_info_t *info; > > snd_kcontrol_get_t *get; > > snd_kcontrol_put_t *put; > > + snd_kcontrol_init_t *init; > > union { > > snd_kcontrol_tlv_rw_t *c; > > const unsigned int *p; > > @@ -71,6 +73,7 @@ struct snd_kcontrol { > > snd_kcontrol_info_t *info; > > snd_kcontrol_get_t *get; > > snd_kcontrol_put_t *put; > > + snd_kcontrol_init_t *init; > > union { > > snd_kcontrol_tlv_rw_t *c; > > const unsigned int *p; > > diff --git a/sound/core/control.c b/sound/core/control.c > > index b961134..9d30663 100644 > > --- a/sound/core/control.c > > +++ b/sound/core/control.c > > @@ -256,6 +256,7 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, > > kctl.info = ncontrol->info; > > kctl.get = ncontrol->get; > > kctl.put = ncontrol->put; > > + kctl.init = ncontrol->init; > > kctl.tlv.p = ncontrol->tlv.p; > > kctl.private_value = ncontrol->private_value; > > kctl.private_data = private_data; > > @@ -362,6 +363,12 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) > > err = -ENOMEM; > > goto error; > > } > > + if (kcontrol->init) { > > + err = kcontrol->init(kcontrol); > > + if (err < 0) > > + goto error; > > + } > > + > > list_add_tail(&kcontrol->list, &card->controls); > > card->controls_count += kcontrol->count; > > kcontrol->id.numid = card->last_numid + 1; > > -- > > 1.9.0 > > > > > [2 Digital signature <application/pgp-signature (7bit)>] >
On Sat, Sep 06, 2014 at 05:56:02PM +0200, Takashi Iwai wrote: > At Sat, 6 Sep 2014 15:21:24 +0100, > Mark Brown wrote: > > > > On Tue, Sep 02, 2014 at 06:05:58PM +0530, Subhransu S. Prusty wrote: > > > Some controls need to initialize stuffs like pvt data, so they need a > > > callback if the control creation is successful. > > > > Adding Takashi - this is ALSA core code so he needs to review it. > > This would bloat effectively the data size of all sound drivers, so I > can't ack it without more proper reasoning. That is, please convince > me why this change must be taken for the cost of size bloat of all > sound drivers. Can't you do it in the caller side of snd_ctl_add() > like many other drivers already do? Okay lets step back and see why we need this :) In our case after the control creation we need to allocate memory which will hold the data for the byte controls. This can be done only after the controls are created (by asoc). For that we need a callback into driver so that we can allocate the memory. Thats why we added .init() method. If you have any other way to do this, we are all ears :)
At Mon, 8 Sep 2014 09:44:21 +0530, Vinod Koul wrote: > > On Sat, Sep 06, 2014 at 05:56:02PM +0200, Takashi Iwai wrote: > > At Sat, 6 Sep 2014 15:21:24 +0100, > > Mark Brown wrote: > > > > > > On Tue, Sep 02, 2014 at 06:05:58PM +0530, Subhransu S. Prusty wrote: > > > > Some controls need to initialize stuffs like pvt data, so they need a > > > > callback if the control creation is successful. > > > > > > Adding Takashi - this is ALSA core code so he needs to review it. > > > > This would bloat effectively the data size of all sound drivers, so I > > can't ack it without more proper reasoning. That is, please convince > > me why this change must be taken for the cost of size bloat of all > > sound drivers. Can't you do it in the caller side of snd_ctl_add() > > like many other drivers already do? > Okay lets step back and see why we need this :) > > In our case after the control creation we need to allocate memory which will > hold the data for the byte controls. This can be done only after the > controls are created (by asoc). Why? Because you don't need how many bytes to allocate? > For that we need a callback into driver so that we can allocate the memory. > Thats why we added .init() method. If you have any other way to do this, we > are all ears :) For example, you can embed an init flag into your record and call the initializer in get/put callback if not called yet. Takashi > > -- > ~Vinod > > > > > > > thanks, > > > > Takashi > > > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com> > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > > --- > > > > include/sound/control.h | 3 +++ > > > > sound/core/control.c | 7 +++++++ > > > > 2 files changed, 10 insertions(+) > > > > > > > > diff --git a/include/sound/control.h b/include/sound/control.h > > > > index 0426139..1389f69 100644 > > > > --- a/include/sound/control.h > > > > +++ b/include/sound/control.h > > > > @@ -30,6 +30,7 @@ struct snd_kcontrol; > > > > typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_info * uinfo); > > > > typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol); > > > > typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol); > > > > +typedef int (snd_kcontrol_init_t) (struct snd_kcontrol * kcontrol); > > > > typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol, > > > > int op_flag, /* SNDRV_CTL_TLV_OP_XXX */ > > > > unsigned int size, > > > > @@ -52,6 +53,7 @@ struct snd_kcontrol_new { > > > > snd_kcontrol_info_t *info; > > > > snd_kcontrol_get_t *get; > > > > snd_kcontrol_put_t *put; > > > > + snd_kcontrol_init_t *init; > > > > union { > > > > snd_kcontrol_tlv_rw_t *c; > > > > const unsigned int *p; > > > > @@ -71,6 +73,7 @@ struct snd_kcontrol { > > > > snd_kcontrol_info_t *info; > > > > snd_kcontrol_get_t *get; > > > > snd_kcontrol_put_t *put; > > > > + snd_kcontrol_init_t *init; > > > > union { > > > > snd_kcontrol_tlv_rw_t *c; > > > > const unsigned int *p; > > > > diff --git a/sound/core/control.c b/sound/core/control.c > > > > index b961134..9d30663 100644 > > > > --- a/sound/core/control.c > > > > +++ b/sound/core/control.c > > > > @@ -256,6 +256,7 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, > > > > kctl.info = ncontrol->info; > > > > kctl.get = ncontrol->get; > > > > kctl.put = ncontrol->put; > > > > + kctl.init = ncontrol->init; > > > > kctl.tlv.p = ncontrol->tlv.p; > > > > kctl.private_value = ncontrol->private_value; > > > > kctl.private_data = private_data; > > > > @@ -362,6 +363,12 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) > > > > err = -ENOMEM; > > > > goto error; > > > > } > > > > + if (kcontrol->init) { > > > > + err = kcontrol->init(kcontrol); > > > > + if (err < 0) > > > > + goto error; > > > > + } > > > > + > > > > list_add_tail(&kcontrol->list, &card->controls); > > > > card->controls_count += kcontrol->count; > > > > kcontrol->id.numid = card->last_numid + 1; > > > > -- > > > > 1.9.0 > > > > > > > > > > > [2 Digital signature <application/pgp-signature (7bit)>] > > > > > -- >
On Mon, Sep 08, 2014 at 10:04:42AM +0200, Takashi Iwai wrote: > At Mon, 8 Sep 2014 09:44:21 +0530, > Vinod Koul wrote: > > > > On Sat, Sep 06, 2014 at 05:56:02PM +0200, Takashi Iwai wrote: > > > At Sat, 6 Sep 2014 15:21:24 +0100, > > > Mark Brown wrote: > > > > > > > > On Tue, Sep 02, 2014 at 06:05:58PM +0530, Subhransu S. Prusty wrote: > > > > > Some controls need to initialize stuffs like pvt data, so they need a > > > > > callback if the control creation is successful. > > > > > > > > Adding Takashi - this is ALSA core code so he needs to review it. > > > > > > This would bloat effectively the data size of all sound drivers, so I > > > can't ack it without more proper reasoning. That is, please convince > > > me why this change must be taken for the cost of size bloat of all > > > sound drivers. Can't you do it in the caller side of snd_ctl_add() > > > like many other drivers already do? > > Okay lets step back and see why we need this :) > > > > In our case after the control creation we need to allocate memory which will > > hold the data for the byte controls. This can be done only after the > > controls are created (by asoc). > > Why? Because you don't need how many bytes to allocate? > > > For that we need a callback into driver so that we can allocate the memory. > > Thats why we added .init() method. If you have any other way to do this, we > > are all ears :) > > For example, you can embed an init flag into your record and call the > initializer in get/put callback if not called yet. Yes but that would involve open coding in all control hanlers for the driver : if (!initialized) allocate_control_mem(); This approach was done earlier (checking in info) and flaged off by Mark. It make better to have these handled in the framework. Either ways we are okay with whatever you two decide :)
At Mon, 8 Sep 2014 13:38:07 +0530, Vinod Koul wrote: > > On Mon, Sep 08, 2014 at 10:04:42AM +0200, Takashi Iwai wrote: > > At Mon, 8 Sep 2014 09:44:21 +0530, > > Vinod Koul wrote: > > > > > > On Sat, Sep 06, 2014 at 05:56:02PM +0200, Takashi Iwai wrote: > > > > At Sat, 6 Sep 2014 15:21:24 +0100, > > > > Mark Brown wrote: > > > > > > > > > > On Tue, Sep 02, 2014 at 06:05:58PM +0530, Subhransu S. Prusty wrote: > > > > > > Some controls need to initialize stuffs like pvt data, so they need a > > > > > > callback if the control creation is successful. > > > > > > > > > > Adding Takashi - this is ALSA core code so he needs to review it. > > > > > > > > This would bloat effectively the data size of all sound drivers, so I > > > > can't ack it without more proper reasoning. That is, please convince > > > > me why this change must be taken for the cost of size bloat of all > > > > sound drivers. Can't you do it in the caller side of snd_ctl_add() > > > > like many other drivers already do? > > > Okay lets step back and see why we need this :) > > > > > > In our case after the control creation we need to allocate memory which will > > > hold the data for the byte controls. This can be done only after the > > > controls are created (by asoc). > > > > Why? Because you don't need how many bytes to allocate? > > > > > For that we need a callback into driver so that we can allocate the memory. > > > Thats why we added .init() method. If you have any other way to do this, we > > > are all ears :) > > > > For example, you can embed an init flag into your record and call the > > initializer in get/put callback if not called yet. > Yes but that would involve open coding in all control hanlers for the driver : > > if (!initialized) > allocate_control_mem(); > > This approach was done earlier (checking in info) and flaged off by Mark. It make better to > have these handled in the framework. For the cost of bloating all sound drivers with every control element? No. You can extend the framework, but not in this way. For example, it's nonsense to keep this permanently in each control element, and both in kcontrol and kcontrol_new. Remember that this is a oneshot callback that is called only after creation... Takashi
On Mon, Sep 08, 2014 at 10:36:57AM +0200, Takashi Iwai wrote: > At Mon, 8 Sep 2014 13:38:07 +0530, > Vinod Koul wrote: > > > > On Mon, Sep 08, 2014 at 10:04:42AM +0200, Takashi Iwai wrote: > > > At Mon, 8 Sep 2014 09:44:21 +0530, > > > Vinod Koul wrote: > > > > > > > > On Sat, Sep 06, 2014 at 05:56:02PM +0200, Takashi Iwai wrote: > > > > > At Sat, 6 Sep 2014 15:21:24 +0100, > > > > > Mark Brown wrote: > > > > > > > > > > > > On Tue, Sep 02, 2014 at 06:05:58PM +0530, Subhransu S. Prusty wrote: > > > > > > > Some controls need to initialize stuffs like pvt data, so they need a > > > > > > > callback if the control creation is successful. > > > > > > > > > > > > Adding Takashi - this is ALSA core code so he needs to review it. > > > > > > > > > > This would bloat effectively the data size of all sound drivers, so I > > > > > can't ack it without more proper reasoning. That is, please convince > > > > > me why this change must be taken for the cost of size bloat of all > > > > > sound drivers. Can't you do it in the caller side of snd_ctl_add() > > > > > like many other drivers already do? > > > > Okay lets step back and see why we need this :) > > > > > > > > In our case after the control creation we need to allocate memory which will > > > > hold the data for the byte controls. This can be done only after the > > > > controls are created (by asoc). > > > > > > Why? Because you don't need how many bytes to allocate? > > > > > > > For that we need a callback into driver so that we can allocate the memory. > > > > Thats why we added .init() method. If you have any other way to do this, we > > > > are all ears :) > > > > > > For example, you can embed an init flag into your record and call the > > > initializer in get/put callback if not called yet. > > Yes but that would involve open coding in all control hanlers for the driver : > > > > if (!initialized) > > allocate_control_mem(); > > > > This approach was done earlier (checking in info) and flaged off by Mark. It make better to > > have these handled in the framework. > > For the cost of bloating all sound drivers with every control element? > No. You can extend the framework, but not in this way. Yes makes sense. > For example, it's nonsense to keep this permanently in each control > element, and both in kcontrol and kcontrol_new. Remember that this is > a oneshot callback that is called only after creation... Yes indeed, so we are going to rely on the falg in driver records and initialize at first invocation. Updated series spinning now
On Mon, Sep 08, 2014 at 01:38:07PM +0530, Vinod Koul wrote: Please (as I'm sure has been pointed out before) fix your mailer to word wrap within paragraphs. > On Mon, Sep 08, 2014 at 10:04:42AM +0200, Takashi Iwai wrote: > > For example, you can embed an init flag into your record and call the > > initializer in get/put callback if not called yet. > Yes but that would involve open coding in all control hanlers for the driver : > if (!initialized) > allocate_control_mem(); > This approach was done earlier (checking in info) and flaged off by Mark. It make better to > have these handled in the framework. No, that's not the problem I identified. The bug you had was that the driver was initialising the data in only the info callback and would just fail if someone decided to call another callback first.
On Tue, Sep 09, 2014 at 12:44:25PM +0100, Mark Brown wrote: > On Mon, Sep 08, 2014 at 01:38:07PM +0530, Vinod Koul wrote: > > Please (as I'm sure has been pointed out before) fix your mailer to word > wrap within paragraphs. Oops, i though I had set mutt to fix it after you pointed out last time, will check again. > > > On Mon, Sep 08, 2014 at 10:04:42AM +0200, Takashi Iwai wrote: > > > > For example, you can embed an init flag into your record and call the > > > initializer in get/put callback if not called yet. > > > Yes but that would involve open coding in all control hanlers for the > > driver : > > > if (!initialized) allocate_control_mem(); > > > This approach was done earlier (checking in info) and flaged off by > > Mark. It make better to have these handled in the framework. > > No, that's not the problem I identified. The bug you had was that the > driver was initialising the data in only the info callback and would just > fail if someone decided to call another callback first. Yes and during the discussed we wanted to handle it in framework leading to init callback. Nevertheless we are moving it back to driver while fixing the bug you pointed. Thanks
diff --git a/include/sound/control.h b/include/sound/control.h index 0426139..1389f69 100644 --- a/include/sound/control.h +++ b/include/sound/control.h @@ -30,6 +30,7 @@ struct snd_kcontrol; typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_info * uinfo); typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol); typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol); +typedef int (snd_kcontrol_init_t) (struct snd_kcontrol * kcontrol); typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol, int op_flag, /* SNDRV_CTL_TLV_OP_XXX */ unsigned int size, @@ -52,6 +53,7 @@ struct snd_kcontrol_new { snd_kcontrol_info_t *info; snd_kcontrol_get_t *get; snd_kcontrol_put_t *put; + snd_kcontrol_init_t *init; union { snd_kcontrol_tlv_rw_t *c; const unsigned int *p; @@ -71,6 +73,7 @@ struct snd_kcontrol { snd_kcontrol_info_t *info; snd_kcontrol_get_t *get; snd_kcontrol_put_t *put; + snd_kcontrol_init_t *init; union { snd_kcontrol_tlv_rw_t *c; const unsigned int *p; diff --git a/sound/core/control.c b/sound/core/control.c index b961134..9d30663 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -256,6 +256,7 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, kctl.info = ncontrol->info; kctl.get = ncontrol->get; kctl.put = ncontrol->put; + kctl.init = ncontrol->init; kctl.tlv.p = ncontrol->tlv.p; kctl.private_value = ncontrol->private_value; kctl.private_data = private_data; @@ -362,6 +363,12 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol) err = -ENOMEM; goto error; } + if (kcontrol->init) { + err = kcontrol->init(kcontrol); + if (err < 0) + goto error; + } + list_add_tail(&kcontrol->list, &card->controls); card->controls_count += kcontrol->count; kcontrol->id.numid = card->last_numid + 1;