| Message ID | 1425072502-3204-8-git-send-email-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/27/2015 10:28 PM, Takashi Iwai wrote: > For reducing the number of verbs performed during the initialization > and the resume of the generic parser, this patch (re-)introduces the > mechanism to cache everything in init and sync later. > > However, it became a bit tricky in the end: we can't use regcache's > cache_only flag straightforwardly here because we do want the actual > hardware access for reads if needed, but forbids only the writes. > So, instead, the regmap reg_write callback checks the own flag > (reusing the existing codec->cached_write) and skips the actual > access. This works by assumption of regmap dumping the whole > registers via regcache_sync() at a later point. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/pci/hda/hda_generic.c | 4 ++++ > sound/pci/hda/hda_regmap.c | 6 ++++++ > 2 files changed, 10 insertions(+) > > diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c > index 33de31c83d6e..bcaed93f8795 100644 > --- a/sound/pci/hda/hda_generic.c > +++ b/sound/pci/hda/hda_generic.c > @@ -5411,6 +5411,8 @@ int snd_hda_gen_init(struct hda_codec *codec) > > snd_hda_apply_verbs(codec); > > + codec->cached_write = 1; > + How about using regcache_cache_only(), this handles this internally in regmap and will also make sure that the cache gets marked as dirty so that a regcache_sync() will properly write out the dirty registers. > init_multi_out(codec); > init_extra_out(codec); > init_multi_io(codec); [...] >
At Sat, 28 Feb 2015 10:51:29 +0100, Lars-Peter Clausen wrote: > > On 02/27/2015 10:28 PM, Takashi Iwai wrote: > > For reducing the number of verbs performed during the initialization > > and the resume of the generic parser, this patch (re-)introduces the > > mechanism to cache everything in init and sync later. > > > > However, it became a bit tricky in the end: we can't use regcache's > > cache_only flag straightforwardly here because we do want the actual > > hardware access for reads if needed, but forbids only the writes. > > So, instead, the regmap reg_write callback checks the own flag > > (reusing the existing codec->cached_write) and skips the actual > > access. This works by assumption of regmap dumping the whole > > registers via regcache_sync() at a later point. > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > sound/pci/hda/hda_generic.c | 4 ++++ > > sound/pci/hda/hda_regmap.c | 6 ++++++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c > > index 33de31c83d6e..bcaed93f8795 100644 > > --- a/sound/pci/hda/hda_generic.c > > +++ b/sound/pci/hda/hda_generic.c > > @@ -5411,6 +5411,8 @@ int snd_hda_gen_init(struct hda_codec *codec) > > > > snd_hda_apply_verbs(codec); > > > > + codec->cached_write = 1; > > + > > How about using regcache_cache_only(), this handles this internally in > regmap and will also make sure that the cache gets marked as dirty so that a > regcache_sync() will properly write out the dirty registers. I tried this as a first option, too, but it doesn't work because we still want reading the value from hardware if not read beforehand. regcache_cache_only() prevents also from reading a h/w value. It's the point I mentioned this being tricky. thanks, Takashi
On 02/28/2015 11:01 AM, Takashi Iwai wrote: > At Sat, 28 Feb 2015 10:51:29 +0100, > Lars-Peter Clausen wrote: >> >> On 02/27/2015 10:28 PM, Takashi Iwai wrote: >>> For reducing the number of verbs performed during the initialization >>> and the resume of the generic parser, this patch (re-)introduces the >>> mechanism to cache everything in init and sync later. >>> >>> However, it became a bit tricky in the end: we can't use regcache's >>> cache_only flag straightforwardly here because we do want the actual >>> hardware access for reads if needed, but forbids only the writes. >>> So, instead, the regmap reg_write callback checks the own flag >>> (reusing the existing codec->cached_write) and skips the actual >>> access. This works by assumption of regmap dumping the whole >>> registers via regcache_sync() at a later point. >>> >>> Signed-off-by: Takashi Iwai <tiwai@suse.de> >>> --- >>> sound/pci/hda/hda_generic.c | 4 ++++ >>> sound/pci/hda/hda_regmap.c | 6 ++++++ >>> 2 files changed, 10 insertions(+) >>> >>> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c >>> index 33de31c83d6e..bcaed93f8795 100644 >>> --- a/sound/pci/hda/hda_generic.c >>> +++ b/sound/pci/hda/hda_generic.c >>> @@ -5411,6 +5411,8 @@ int snd_hda_gen_init(struct hda_codec *codec) >>> >>> snd_hda_apply_verbs(codec); >>> >>> + codec->cached_write = 1; >>> + >> >> How about using regcache_cache_only(), this handles this internally in >> regmap and will also make sure that the cache gets marked as dirty so that a >> regcache_sync() will properly write out the dirty registers. > > I tried this as a first option, too, but it doesn't work because we > still want reading the value from hardware if not read beforehand. > regcache_cache_only() prevents also from reading a h/w value. It's > the point I mentioned this being tricky. Hm, right, I skipped right past that. But I think it will still be better to implement this write-only caching in the regmap layer. - Lars
At Sat, 28 Feb 2015 11:27:42 +0100, Lars-Peter Clausen wrote: > > On 02/28/2015 11:01 AM, Takashi Iwai wrote: > > At Sat, 28 Feb 2015 10:51:29 +0100, > > Lars-Peter Clausen wrote: > >> > >> On 02/27/2015 10:28 PM, Takashi Iwai wrote: > >>> For reducing the number of verbs performed during the initialization > >>> and the resume of the generic parser, this patch (re-)introduces the > >>> mechanism to cache everything in init and sync later. > >>> > >>> However, it became a bit tricky in the end: we can't use regcache's > >>> cache_only flag straightforwardly here because we do want the actual > >>> hardware access for reads if needed, but forbids only the writes. > >>> So, instead, the regmap reg_write callback checks the own flag > >>> (reusing the existing codec->cached_write) and skips the actual > >>> access. This works by assumption of regmap dumping the whole > >>> registers via regcache_sync() at a later point. > >>> > >>> Signed-off-by: Takashi Iwai <tiwai@suse.de> > >>> --- > >>> sound/pci/hda/hda_generic.c | 4 ++++ > >>> sound/pci/hda/hda_regmap.c | 6 ++++++ > >>> 2 files changed, 10 insertions(+) > >>> > >>> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c > >>> index 33de31c83d6e..bcaed93f8795 100644 > >>> --- a/sound/pci/hda/hda_generic.c > >>> +++ b/sound/pci/hda/hda_generic.c > >>> @@ -5411,6 +5411,8 @@ int snd_hda_gen_init(struct hda_codec *codec) > >>> > >>> snd_hda_apply_verbs(codec); > >>> > >>> + codec->cached_write = 1; > >>> + > >> > >> How about using regcache_cache_only(), this handles this internally in > >> regmap and will also make sure that the cache gets marked as dirty so that a > >> regcache_sync() will properly write out the dirty registers. > > > > I tried this as a first option, too, but it doesn't work because we > > still want reading the value from hardware if not read beforehand. > > regcache_cache_only() prevents also from reading a h/w value. It's > > the point I mentioned this being tricky. > > Hm, right, I skipped right past that. But I think it will still be better to > implement this write-only caching in the regmap layer. Yeah, that would make sense, I'm not against it :) Takashi
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c index 33de31c83d6e..bcaed93f8795 100644 --- a/sound/pci/hda/hda_generic.c +++ b/sound/pci/hda/hda_generic.c @@ -5411,6 +5411,8 @@ int snd_hda_gen_init(struct hda_codec *codec) snd_hda_apply_verbs(codec); + codec->cached_write = 1; + init_multi_out(codec); init_extra_out(codec); init_multi_io(codec); @@ -5424,6 +5426,8 @@ int snd_hda_gen_init(struct hda_codec *codec) /* call init functions of standard auto-mute helpers */ update_automute_all(codec); + codec->cached_write = 0; + regcache_sync(codec->regmap); if (spec->vmaster_mute.sw_kctl && spec->vmaster_mute.hook) diff --git a/sound/pci/hda/hda_regmap.c b/sound/pci/hda/hda_regmap.c index 38934c06a813..79da06da286d 100644 --- a/sound/pci/hda/hda_regmap.c +++ b/sound/pci/hda/hda_regmap.c @@ -116,6 +116,12 @@ static int hda_reg_write(void *context, unsigned int reg, unsigned int val) unsigned int verb; int i, bytes; + /* skip h/w write when cached_write flag is set, assuming that + * regcache_sync() will dump the all registers at a later point + */ + if (codec->cached_write) + return 0; + if (!codec_is_running(codec)) return 0; /* skip the h/w write, it'll be synced later */
For reducing the number of verbs performed during the initialization and the resume of the generic parser, this patch (re-)introduces the mechanism to cache everything in init and sync later. However, it became a bit tricky in the end: we can't use regcache's cache_only flag straightforwardly here because we do want the actual hardware access for reads if needed, but forbids only the writes. So, instead, the regmap reg_write callback checks the own flag (reusing the existing codec->cached_write) and skips the actual access. This works by assumption of regmap dumping the whole registers via regcache_sync() at a later point. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/pci/hda/hda_generic.c | 4 ++++ sound/pci/hda/hda_regmap.c | 6 ++++++ 2 files changed, 10 insertions(+)