Re: [PATCH v1 1/1] treewide: Align match_string() with sysfs_match_string()
On 02-06-24, 18:57, Andy Shevchenko wrote: > Make two APIs look similar. Hence convert match_string() to be > a 2-argument macro. In order to avoid unneeded churn, convert > all users as well. There is no functional change intended. > .. > drivers/phy/mediatek/phy-mtk-tphy.c | 8 ++--- > drivers/phy/tegra/xusb.c | 4 +-- Acked-by: Vinod Koul -- ~Vinod
Re: [Intel-gfx] [PATCH] ALSA: hda: Use loop counter for hdac_wait_for_cmd_dmas() timeout
On Thu, May 04, 2017 at 12:25:26PM +0200, Takashi Iwai wrote:
> On Thu, 04 May 2017 12:18:29 +0200,
> Chris Wilson wrote:
> >
> > hdac_wait_for_cmd_dmas() uses a jiffie timeout to ensure that we do not
> > wait forever for stuck hardware. However, it is called from an
> > irq-disabled context which prevents jiffie from advancing and so the
> > loop doesn't terminate if the hardware fails. This can then cause NMI
> > watchdog warnings, such as:
> >
> > NMI watchdog: Watchdog detected hard LOCKUP on cpu 3
> > Modules linked in: snd_hda_intel i915 vgem snd_hda_codec_hdmi
> > x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul
> > crc32_pclmul snd_hda_codec_realtek snd_hda_codec_generic
> > ghash_clmulni_intel e1000e snd_hda_codec snd_hwdep snd_hda_core snd_pcm ptp
> > mei_me prime_numbers pps_core mei lpc_ich i2c_hid i2c_designware_platform
> > i2c_designware_core [last unloaded: i915]
> > irq event stamp: 13366
> > hardirqs last enabled at (13365): []
> > _raw_spin_unlock_irq+0x27/0x50
> > hardirqs last disabled at (13366): []
> > _raw_spin_lock_irq+0x12/0x50
> > softirqs last enabled at (12744): []
> > __do_softirq+0x1d9/0x4c0
> > softirqs last disabled at (12721): []
> > irq_exit+0xa9/0xc0
> > CPU: 3 PID: 10443 Comm: kworker/u8:11 Tainted: G U
> > 4.11.0-rc4-CI-CI_DRM_319+ #1
> > Hardware name: /NUC5i5RYB, BIOS
> > RYBDWi35.86A.0362.2017.0118.0940 01/18/2017
> > Workqueue: events_unbound async_run_entry_fn
> > task: 88024cd32740 task.stack: c9000162c000
> > RIP: 0010:preempt_count_add+0xe/0xc0
> > RSP: 0018:c9000162fbd8 EFLAGS: 0082
> > RAX: 8001 RBX: 000704b96558 RCX: 0002
> > RDX: RSI: 81c74f2d RDI: 0001
> > RBP: c9000162fc08 R08: bbcc90cc R09: 23c7b071
> > R10: 827901a8 R11: 88024cd32740 R12: 000704b92baa
> > R13: 3ea0 R14: 0003 R15: a00061f0
> > FS: () GS:880256d8()
> > knlGS:
> > CS: 0010 DS: ES: CR0: 80050033
> > CR2: 7f90f84a5144 CR3: 03e0f000 CR4: 003406e0
> > Call Trace:
> > ? delay_tsc+0x3d/0xc0
> > __delay+0xa/0x10
> > __const_udelay+0x31/0x40
> > snd_hdac_bus_stop_cmd_io+0x96/0xe0 [snd_hda_core]
> > ? azx_dev_disconnect+0x20/0x20 [snd_hda_intel]
> > snd_hdac_bus_stop_chip+0xb1/0x100 [snd_hda_core]
> > azx_stop_chip+0x9/0x10 [snd_hda_codec]
> > azx_suspend+0x72/0x220 [snd_hda_intel]
> > pci_pm_suspend+0x71/0x140
> > dpm_run_callback+0x6f/0x330
> > ? pci_pm_freeze+0xe0/0xe0
> > __device_suspend+0xf9/0x370
> > ? dpm_watchdog_set+0x60/0x60
> > async_suspend+0x1a/0x90
> > async_run_entry_fn+0x34/0x160
> > process_one_work+0x1f4/0x6d0
> > ? process_one_work+0x16e/0x6d0
> > worker_thread+0x49/0x4a0
> > kthread+0x107/0x140
> > ? process_one_work+0x6d0/0x6d0
> > ? kthread_create_on_node+0x40/0x40
> > ret_from_fork+0x2e/0x40
> >
> > Fixes: 38b19ed7f81e ("ALSA: hda: fix to wait for RIRB & CORB DMA to set")
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100419
> > Signed-off-by: Chris Wilson
> > Cc: Jeeja KP
> > Cc: Vinod Koul
> > Cc: Takashi Iwai
> > Cc: # v4.7+
>
> Any reason to submit a different fix from what's attached in the
> bugzilla you mentioned?
probably a race between then :)
Jeeja talked to me earlier today and uploaded the patch where we drop the
locks and still use jiffies.
Takashi,
Do you prefer dropping locks or using loop?
> > ---
> > sound/hda/hdac_controller.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
> > index ee08c389b4d6..7f8806b03982 100644
> > --- a/sound/hda/hdac_controller.c
> > +++ b/sound/hda/hdac_controller.c
> > @@ -85,14 +85,14 @@ static void hdac_wait_for_cmd_dmas(struct hdac_bus *bus)
> > {
> > unsigned long timeout;
> >
> > - timeout = jiffies + msecs_to_jiffies(100);
> > + timeout = 100 * 100; /* 100ms */
> > while ((snd_hdac_chip_readb(bus, RIRBCTL) & AZX_RBCTL_DMA_EN)
> > - && time_before(jiffies, timeout))
> > + && timeout--)
> > udelay(10);
> >
> > - timeout = jiffies + msecs_to_jiffies(100);
> > + timeout = 100 * 100; /* 100ms */
> > while ((snd_hdac_chip_readb(bus, CORBCTL) & AZX_CORBCTL_RUN)
> > - && time_before(jiffies, timeout))
> > + && timeout--)
> > udelay(10);
> > }
> >
> > --
> > 2.11.0
> >
--
~Vinod
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On Tue, Mar 15, 2016 at 04:14:19PM -0500, Pierre-Louis Bossart wrote: > On 3/15/16 11:21 AM, Vinod Koul wrote: > >On Tue, Mar 15, 2016 at 03:35:45PM +0200, Ville Syrjälä wrote: > >>>>I understand the benefits of a parent/child device/subdevice model. What I > >>>>don't see is whether we need the component framework at all here? > >>>>It was used in the case of HDaudio since both i915 and HDaudio controllers > >>>>get probed at different times, here the HDMI interface is just a bunch of > >>>>registers/DMA from the same hardware so the whole master/client interface > >>>>exposed by the component framework to 'bind' independent drivers is a bit > >>>>overkill? > >>> > >>>I haven't read the patches, but using component.c when you instead can > >>>model it with parent/child smells like abuse. Component.c is meant for > >>>when you have multiple devices all over the device tree that in aggregate > >>>constitute the overall logical driver. This doesn't seem to be the case > >>>here. > > > >Right I do think that might be the case. > > > >>We still need the eld notify hooks so that i915 can inform the audio > >>driver about the state of the display. Whether that's best done via > >>the component stuff or something else I don't know. > > > >There is already work ongoing by ARM folks for the interface, should we > >reuse that [1]. I would certainly argue reusing rather than redfeining a > >new one would be better. > > > >Btw this interface seems to rely on display side implemting these ops. > > My understanding is that it's an interface for external encoders > that have an I2S or S/PDIF link. Such external encoders appear as > ASoC codecs that can be bound to the SoC with DT bindings. I don't > see what we could reuse here? That is one of the intended usages. Right now three folks are developing on that, TI which seems to be encoder case, then sti (don't know if that off chip or not) and mediatek. The point here is that we can use the same interface here too if we are not going i915 way. Possibly we might want to modify or add to this and not make it ASoC dependent as this driver won't be an ASoC one. But yes I haven't looked closely enough to say that if this should be the way or not. I wanted to pint our an alternate interface being developed which can be possible reused in non i915 case Thanks -- ~Vinod ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 04/15] drm/i915: Add headers for non-HDAudio HDMI interface
On Tue, Mar 15, 2016 at 03:35:45PM +0200, Ville Syrjälä wrote: > > > I understand the benefits of a parent/child device/subdevice model. What I > > > don't see is whether we need the component framework at all here? > > > It was used in the case of HDaudio since both i915 and HDaudio controllers > > > get probed at different times, here the HDMI interface is just a bunch of > > > registers/DMA from the same hardware so the whole master/client interface > > > exposed by the component framework to 'bind' independent drivers is a bit > > > overkill? > > > > I haven't read the patches, but using component.c when you instead can > > model it with parent/child smells like abuse. Component.c is meant for > > when you have multiple devices all over the device tree that in aggregate > > constitute the overall logical driver. This doesn't seem to be the case > > here. Right I do think that might be the case. > We still need the eld notify hooks so that i915 can inform the audio > driver about the state of the display. Whether that's best done via > the component stuff or something else I don't know. There is already work ongoing by ARM folks for the interface, should we reuse that [1]. I would certainly argue reusing rather than redfeining a new one would be better. Btw this interface seems to rely on display side implemting these ops. Thanks -- ~Vinod [1]: http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105526.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 4/4] ALSA: hda - Move audio component accesses to hdac_i915.c
On Fri, Dec 04, 2015 at 06:12:49PM +0100, Takashi Iwai wrote:
> A couple of i915_audio_component ops have been added and accessed
> directly from patch_hdmi.c. Ideally all these should be factored out
> into hdac_i915.c.
>
> This patch does it, adds two new helper functions for setting N/CTS
> and fetching ELD bytes. One bonus is that the hackish widget vs port
> mapping is also moved to hdac_i915.c, so that it can be fixed /
> enhanced more cleanly.
>
> Signed-off-by: Takashi Iwai
Reviewed-by: Vinod Koul
--
~Vinod
> ---
> include/sound/hda_i915.h | 14 ++
> sound/hda/hdac_i915.c | 66
> sound/pci/hda/patch_hdmi.c | 69
> +-
> 3 files changed, 106 insertions(+), 43 deletions(-)
>
> diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h
> index 930b41e5acf4..fa341fcb5829 100644
> --- a/include/sound/hda_i915.h
> +++ b/include/sound/hda_i915.h
> @@ -10,6 +10,9 @@
> int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable);
> int snd_hdac_display_power(struct hdac_bus *bus, bool enable);
> int snd_hdac_get_display_clk(struct hdac_bus *bus);
> +int snd_hdac_sync_audio_rate(struct hdac_bus *bus, hda_nid_t nid, int rate);
> +int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid,
> +bool *audio_enabled, char *buffer, int max_bytes);
> int snd_hdac_i915_init(struct hdac_bus *bus);
> int snd_hdac_i915_exit(struct hdac_bus *bus);
> int snd_hdac_i915_register_notifier(const struct
> i915_audio_component_audio_ops *);
> @@ -26,6 +29,17 @@ static inline int snd_hdac_get_display_clk(struct hdac_bus
> *bus)
> {
> return 0;
> }
> +static inline int snd_hdac_sync_audio_rate(struct hdac_bus *bus, hda_nid_t
> nid,
> +int rate)
> +{
> + return 0;
> +}
> +static inline int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid,
> + bool *audio_enabled, char *buffer,
> + int max_bytes)
> +{
> + return -ENODEV;
> +}
> static inline int snd_hdac_i915_init(struct hdac_bus *bus)
> {
> return -ENODEV;
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index 8fef1b8d1fd8..c50177fb469f 100644
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -118,6 +118,72 @@ int snd_hdac_get_display_clk(struct hdac_bus *bus)
> }
> EXPORT_SYMBOL_GPL(snd_hdac_get_display_clk);
>
> +/* There is a fixed mapping between audio pin node and display port
> + * on current Intel platforms:
> + * Pin Widget 5 - PORT B (port = 1 in i915 driver)
> + * Pin Widget 6 - PORT C (port = 2 in i915 driver)
> + * Pin Widget 7 - PORT D (port = 3 in i915 driver)
> + */
> +static int pin2port(hda_nid_t pin_nid)
> +{
> + return pin_nid - 4;
> +}
> +
> +/**
> + * snd_hdac_sync_audio_rate - Set N/CTS based on the sample rate
> + * @bus: HDA core bus
> + * @nid: the pin widget NID
> + * @rate: the sample rate to set
> + *
> + * This function is supposed to be used only by a HD-audio controller
> + * driver that needs the interaction with i915 graphics.
> + *
> + * This function sets N/CTS value based on the given sample rate.
> + * Returns zero for success, or a negative error code.
> + */
> +int snd_hdac_sync_audio_rate(struct hdac_bus *bus, hda_nid_t nid, int rate)
> +{
> + struct i915_audio_component *acomp = bus->audio_component;
> +
> + if (!acomp || !acomp->ops || !acomp->ops->sync_audio_rate)
> + return -ENODEV;
> + return acomp->ops->sync_audio_rate(acomp->dev, pin2port(nid), rate);
> +}
> +EXPORT_SYMBOL_GPL(snd_hdac_sync_audio_rate);
> +
> +/**
> + * snd_hdac_acomp_get_eld - Get the audio state and ELD via component
> + * @bus: HDA core bus
> + * @nid: the pin widget NID
> + * @audio_enabled: the pointer to store the current audio state
> + * @buffer: the buffer pointer to store ELD bytes
> + * @max_bytes: the max bytes to be stored on @buffer
> + *
> + * This function is supposed to be used only by a HD-audio controller
> + * driver that needs the interaction with i915 graphics.
> + *
> + * This function queries the current state of the audio on the given
> + * digital port and fetches the ELD bytes onto the given buffer.
> + * It returns the number of bytes for the total ELD data, zero for
> + * invalid ELD, or a negative error code.
> + *
> + * The return size is the total bytes required for the whole ELD bytes,
> + * thus it may be over @max_bytes. If it's over @max_bytes, it implies
> + * that only a part of ELD bytes have been fetched.
Re: [Intel-gfx] [PATCH 5/7] ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling
On Mon, Nov 30, 2015 at 02:37:49PM +0100, Takashi Iwai wrote:
> Since we have a new audio component ops to fetch the current ELD and
> state now, we can reduce the usage of unsol event of HDMI/DP pins.
> The unsol event isn't only unreliable, but it also needs the power
> up/down of the codec and link at each time, which is a significant
> power and time loss.
>
> In this patch, the jack creation and unsol/jack event handling are
> modified to use the audio component for the dedicated Intel chips.
>
> The jack handling got slightly more codes than a simple usage of
> hda_jack layer since we need to deal directly with snd_jack object;
> the hda_jack layer is basically designed for the pin sense read and
> unsol events, both of which aren't used any longer in our case.
>
> Signed-off-by: Takashi Iwai
> ---
> sound/pci/hda/patch_hdmi.c | 84
> --
> 1 file changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 28684aa86408..8378c31e0b4f 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -83,6 +83,7 @@ struct hdmi_spec_per_pin {
> struct mutex lock;
> struct delayed_work work;
> struct snd_kcontrol *eld_ctl;
> + struct snd_jack *acomp_jack; /* jack via audio component */
> int repoll_count;
> bool setup; /* the stream has been set up by prepare callback */
> int channels; /* current number of channels */
> @@ -141,6 +142,7 @@ struct hdmi_spec {
> struct hdmi_ops ops;
>
> bool dyn_pin_out;
> + bool use_acomp; /* use audio component for ELD notify/update */
>
> /*
>* Non-generic VIA/NVIDIA specific
> @@ -1580,6 +1582,9 @@ static void update_eld(struct hda_codec *codec,
> &per_pin->eld_ctl->id);
> }
>
> +static void sync_eld_via_acomp(struct hda_codec *codec,
> +struct hdmi_spec_per_pin *per_pin);
> +
> static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> {
> struct hda_jack_tbl *jack;
> @@ -1599,6 +1604,11 @@ static bool hdmi_present_sense(struct
> hdmi_spec_per_pin *per_pin, int repoll)
> int present;
> bool ret;
>
> + if (spec->use_acomp) {
> + sync_eld_via_acomp(codec, per_pin);
> + return false; /* don't call snd_hda_jack_report_sync() */
> + }
> +
> snd_hda_power_up_pm(codec);
> present = snd_hda_pin_sense(codec, pin_nid);
>
> @@ -2091,6 +2101,68 @@ static int generic_hdmi_build_pcms(struct hda_codec
> *codec)
> return 0;
> }
>
> +/* update ELD and jack state via audio component */
> +static void sync_eld_via_acomp(struct hda_codec *codec,
> +struct hdmi_spec_per_pin *per_pin)
> +{
> + struct i915_audio_component *acomp = codec->bus->core.audio_component;
> + struct hdmi_spec *spec = codec->spec;
> + struct hdmi_eld *eld = &spec->temp_eld;
> + int size;
> +
> + if (acomp && acomp->ops && acomp->ops->get_eld) {
> + mutex_lock(&per_pin->lock);
> + size = acomp->ops->get_eld(acomp->dev,
> +intel_pin2port(per_pin->pin_nid),
> +&eld->monitor_present,
> +eld->eld_buffer,
> +ELD_MAX_SIZE);
> + if (size > 0) {
> + memset(&eld->info, 0, sizeof(eld->info));
> + if (snd_hdmi_parse_eld(codec, &eld->info,
> +eld->eld_buffer, size) < 0)
> + size = -EINVAL;
> + }
> +
> + if (size > 0) {
> + eld->eld_valid = true;
> + eld->eld_size = size;
> + } else {
> + eld->eld_valid = false;
> + eld->eld_size = 0;
> + }
> +
> + update_eld(codec, per_pin, eld);
> + snd_jack_report(per_pin->acomp_jack,
> + eld->monitor_present ? SND_JACK_AVOUT : 0);
> + mutex_unlock(&per_pin->lock);
> + }
> +}
IMO This and the rest can be moved to sound/hda/ so that other can reuse
this code, as the code will be same for other users too..
--
~Vinod
> +
> +static void free_acomp_jack_priv(struct snd_jack *jack)
> +{
> + struct hdmi_spec_per_pin *per_pin = jack->private_data;
> +
> + per_pin->acomp_jack = NULL;
> +}
> +
> +static int add_acomp_jack_kctl(struct hda_codec *codec,
> +struct hdmi_spec_per_pin *per_pin,
> +const char *name)
> +{
> + struct snd_jack *jack;
> + int err;
> +
> + err = snd_jack_new(codec->card, name, SND_JACK_AVOUT, &jack,
> +true, false);
> + if (err < 0)
> + return err;
> + per_pin->ac
Re: [Intel-gfx] [PATCH 4/7] ALSA: hda - Split ELD update code from hdmi_present_sense()
On Mon, Nov 30, 2015 at 02:37:48PM +0100, Takashi Iwai wrote: > > +/* update per_pin ELD from the given new ELD; > + * setup info frame and notification accordingly > + */ nitpick, comment style -- ~Vinod ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Add audio hotplug info struct
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 -- ~Vinod ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Add audio hotplug info struct
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 :)
--
~Vinod
>
>
> > >> +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
--
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] ALSA: hda - Dispatch incoming HDMI hotplug i915 callback
On Wed, Jul 22, 2015 at 10:30:57AM +0200, Takashi Iwai wrote:
> On Tue, 21 Jul 2015 09:57:26 +0200,
> David Henningsson wrote:
> >
> > This lets interested codec(s) be notified of HDMI hotplug
> > events sent from the i915 driver.
> >
> > Signed-off-by: David Henningsson
> > ---
> > include/sound/hdaudio.h |4
> > sound/hda/hdac_i915.c | 24
> > 2 files changed, 28 insertions(+)
> >
> > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> > index 4caf1fd..ce34182 100644
> > --- a/include/sound/hdaudio.h
> > +++ b/include/sound/hdaudio.h
> > @@ -79,6 +79,10 @@ struct hdac_device {
> > int (*exec_verb)(struct hdac_device *dev, unsigned int cmd,
> > unsigned int flags, unsigned int *res);
> >
> > + /* Used for hotplug notification from i915 driver */
> > + void (*hotplug_notify)(struct hdac_device *,
> > + const struct i915_audio_hotplug_info *);
>
> Since this callback is specific to HDMI/DP, a more specific name would
> be better. Otherwise this sounds like a generic hotplug handler.
>
> Or, we may make it really generic, e.g. something like
> void (*hotplug_notify)(struct hdac_device *, int event_type,
> const void *data);
>
> and call it with a specific event_type value
> codec->hotplug_notify(codec, HDAC_NOTIFY_I915_DP, hotplug_info);
or should this be added to unsol_event handler for the device, we already
are have worker thread for that.
For device it will be single notify for both SW and HW events. Yes adding
event types will help
--
~Vinod
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/27] add pm_runtime_last_busy_and_autosuspend() helper
On Thu, Sep 25, 2014 at 09:54:36PM +0200, Rafael J. Wysocki wrote: > On Thursday, September 25, 2014 04:27:58 PM Wolfram Sang wrote: > > > > --Bn2rw/3z4jIqBvZU > > Content-Type: text/plain; charset=us-ascii > > Content-Disposition: inline > > Content-Transfer-Encoding: quoted-printable > > > > On Thu, Sep 25, 2014 at 09:22:01AM -0500, Felipe Balbi wrote: > > > On Thu, Sep 25, 2014 at 01:27:18PM +0530, Vinod Koul wrote: > > > > On Wed, Sep 24, 2014 at 03:32:19PM -0500, Felipe Balbi wrote: > > > > > > > > OK, I guess this is as good as it gets. > > > > > > > >=20 > > > > > > > > What tree would you like it go through? > > > > > > >=20 > > > > > > > Do we really need this new helper ? I mean, the very moment when = > > we > > > > > > > decide to implement ->runtime_idle() we will need to get rid of t= > > his > > > > > > > change. I wonder if it's really valid... > > > > > >=20 > > > > > > I'm not sure I'm following? This seems to simply implement what dr= > > ivers > > > > > > have been doing already as one function. Why would it be invalid t= > > o reduce > > > > > > code duplication? > > > > >=20 > > > > > For two reasons: > > > > >=20 > > > > > 1) the helper has no inteligence whatsoever. It just calls the same > > > > > functions. > > > > >=20 > > > > > 2) the duplication will vanish whenever someone implements > > > > > ->runtime_idle() and have that call pm_runtime_autosuspend() (like PCI > > > > > and USB buses are doing today). This will just be yet another line th= > > at > > > > > needs to change. > > > > >=20 > > > > > Frankly though, no strong feelings, I just think it's a commit that > > > > > doesn't bring that any benefits other than looking like one line was > > > > > removed. > > > > and yes that is what it tries to do nothing more nothing less. If in fu= > > ture > > > > there are no users (today we have quite a few), then we can remove the = > > dead > > > > macro, no harm. But that is not the situation today. > > >=20 > > > as I said, a commit that's bound to be useless. It's not like you're > > > saving 10 lines of code, it's only one. Replacing two simple lines with > > > a function which takes almost as many characters to type . > > >=20 > > > IMO, this is pretty useless and I'd rather not see them in the drivers I > > > maintain, sorry. > > > > It is not a NACK from me; yet from a high-level perspective I agree with > > Felipe. > > OK > > I'd rather not merge something that driver people don't want to use. > > Vinod? There have been quite a few ACKs as well. Either way am okay. If you feel this will get removed as discussed, then there is no point in merging -- ~Vinod ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/27] add pm_runtime_last_busy_and_autosuspend() helper
On Wed, Sep 24, 2014 at 03:32:19PM -0500, Felipe Balbi wrote: > > > > OK, I guess this is as good as it gets. > > > > > > > > What tree would you like it go through? > > > > > > Do we really need this new helper ? I mean, the very moment when we > > > decide to implement ->runtime_idle() we will need to get rid of this > > > change. I wonder if it's really valid... > > > > I'm not sure I'm following? This seems to simply implement what drivers > > have been doing already as one function. Why would it be invalid to reduce > > code duplication? > > For two reasons: > > 1) the helper has no inteligence whatsoever. It just calls the same > functions. > > 2) the duplication will vanish whenever someone implements > ->runtime_idle() and have that call pm_runtime_autosuspend() (like PCI > and USB buses are doing today). This will just be yet another line that > needs to change. > > Frankly though, no strong feelings, I just think it's a commit that > doesn't bring that any benefits other than looking like one line was > removed. and yes that is what it tries to do nothing more nothing less. If in future there are no users (today we have quite a few), then we can remove the dead macro, no harm. But that is not the situation today. Thanks -- ~Vinod signature.asc Description: Digital signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/27] add pm_runtime_last_busy_and_autosuspend() helper
On Wed, Sep 24, 2014 at 10:28:07PM +0200, Rafael J. Wysocki wrote: > > OK, I guess this is as good as it gets. > > What tree would you like it go through? Since rest of the patches are dependent upon 1st patch which should go thru your tree, we should merge this thru your tree Thanks -- ~Vinod ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/27] drm/i915: use pm_runtime_last_busy_and_autosuspend helper
Use the new pm_runtime_last_busy_and_autosuspend helper instead of open coding the same code Signed-off-by: Vinod Koul --- drivers/gpu/drm/i915/intel_pm.c |3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 40c1229..1ec9b8d 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -6729,8 +6729,7 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv) if (!HAS_RUNTIME_PM(dev)) return; - pm_runtime_mark_last_busy(device); - pm_runtime_put_autosuspend(device); + pm_runtime_last_busy_and_autosuspend(device); } void intel_init_runtime_pm(struct drm_i915_private *dev_priv) -- 1.7.0.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 00/27] add pm_runtime_last_busy_and_autosuspend() helper
This patch series adds a simple macro pm_runtime_last_busy_and_autosuspend() which invokes pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() sequentially. Then we do a tree wide update of current patterns which are present. As evident from log below this pattern is frequent in the kernel. This series can be found at git://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git topic/pm_runtime_last_busy_and_autosuspend Fengguang's kbuild has tested it so it shouldn't break things for anyone. Barring one patch (explictyly mentioned in its changelog) rest are simple replacements. If all are okay, this should be merged thru PM tree as it depends on macro addition. Subhransu S. Prusty (1): PM: Add helper pm_runtime_last_busy_and_autosuspend() Vinod Koul (26): dmaengine: ste_dma: use pm_runtime_last_busy_and_autosuspend helper extcon: arizona: use pm_runtime_last_busy_and_autosuspend helper drm/i915: use pm_runtime_last_busy_and_autosuspend helper drm/nouveau: use pm_runtime_last_busy_and_autosuspend helper drm/radeon: use pm_runtime_last_busy_and_autosuspend helper vga_switcheroo: use pm_runtime_last_busy_and_autosuspend helper i2c: designware: use pm_runtime_last_busy_and_autosuspend helper i2c: omap: use pm_runtime_last_busy_and_autosuspend helper i2c: qup: use pm_runtime_last_busy_and_autosuspend helper mfd: ab8500-gpadc: use pm_runtime_last_busy_and_autosuspend helper mfd: arizona: use pm_runtime_last_busy_and_autosuspend helper mei: use pm_runtime_last_busy_and_autosuspend helper mmc: use pm_runtime_last_busy_and_autosuspend helper mmc: mmci: use pm_runtime_last_busy_and_autosuspend helper mmc: omap_hsmmc: use pm_runtime_last_busy_and_autosuspend helper mmc: sdhci-pxav3: use pm_runtime_last_busy_and_autosuspend helper mmc: sdhci: use pm_runtime_last_busy_and_autosuspend helper NFC: trf7970a: use pm_runtime_last_busy_and_autosuspend helper pm2301-charger: use pm_runtime_last_busy_and_autosuspend helper spi: omap2-mcspi: use pm_runtime_last_busy_and_autosuspend helper spi: orion: use pm_runtime_last_busy_and_autosuspend helper spi: ti-qspi: use pm_runtime_last_busy_and_autosuspend helper spi: core: use pm_runtime_last_busy_and_autosuspend helper tty: serial: omap: use pm_runtime_last_busy_and_autosuspend helper usb: musb: omap2430: use pm_runtime_last_busy_and_autosuspend helper video: fbdev: use pm_runtime_last_busy_and_autosuspend helper Documentation/power/runtime_pm.txt |4 ++ drivers/dma/ste_dma40.c | 30 - drivers/extcon/extcon-arizona.c |6 +-- drivers/gpu/drm/i915/intel_pm.c |3 +- drivers/gpu/drm/nouveau/nouveau_connector.c |3 +- drivers/gpu/drm/nouveau/nouveau_drm.c |9 +--- drivers/gpu/drm/radeon/radeon_connectors.c | 15 ++ drivers/gpu/drm/radeon/radeon_drv.c |5 +- drivers/gpu/drm/radeon/radeon_kms.c |6 +-- drivers/gpu/vga/vga_switcheroo.c|7 +-- drivers/i2c/busses/i2c-designware-core.c|3 +- drivers/i2c/busses/i2c-omap.c |6 +-- drivers/i2c/busses/i2c-qup.c|3 +- drivers/mfd/ab8500-gpadc.c |6 +-- drivers/mfd/arizona-irq.c |3 +- drivers/misc/mei/client.c | 12 ++ drivers/mmc/core/core.c |3 +- drivers/mmc/host/mmci.c | 12 ++ drivers/mmc/host/omap_hsmmc.c | 19 ++--- drivers/mmc/host/sdhci-pxav3.c |6 +-- drivers/mmc/host/sdhci.c|3 +- drivers/nfc/trf7970a.c |3 +- drivers/power/pm2301_charger.c |3 +- drivers/spi/spi-omap2-mcspi.c |9 +--- drivers/spi/spi-orion.c |3 +- drivers/spi/spi-ti-qspi.c |5 +- drivers/spi/spi.c |6 +-- drivers/tty/serial/omap-serial.c| 60 +-- drivers/usb/musb/omap2430.c |6 +-- drivers/video/fbdev/auo_k190x.c |9 +--- include/linux/pm_runtime.h |6 +++ 31 files changed, 97 insertions(+), 177 deletions(-) Thanks -- ~Vinod ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] set up an sync channel between audio and display driver (i.e. ALSA and DRM)
On Tue, May 20, 2014 at 05:29:07PM +0300, Imre Deak wrote: > On Tue, 2014-05-20 at 05:52 +0300, Lin, Mengdong wrote: > > This RFC is based on previous discussion to set up a generic > > communication channel between display and audio driver and > > an internal design of Intel MCG/VPG HDMI audio driver. It's still an > > initial draft and your advice would be appreciated > > to improve the design. > > > > The basic idea is to create a new avsink module and let both drm and > > alsa depend on it. > > This new module provides a framework and APIs for synchronization > > between the display and audio driver. > > > > 1. Display/Audio Client > > > > The avsink core provides APIs to create, register and lookup a > > display/audio client. > > A specific display driver (eg. i915) or audio driver (eg. HD-Audio > > driver) can create a client, add some resources > > objects (shared power wells, display outputs, and audio inputs, > > register ops) to the client, and then register this > > client to avisink core. The peer driver can look up a registered > > client by a name or type, or both. If a client gives > > a valid peer client name on registration, avsink core will bind the > > two clients as peer for each other. And we > > expect a display client and an audio client to be peers for each other > > in a system. > > One problem we have at the moment is the order of calling the system > suspend/resume handlers of the display driver wrt. that of the audio > driver. Since the power well control is part of the display HW block, we > need to run the display driver's resume handler first, initialize the > HW, and only then let the audio driver's resume handler run. For similar > reasons we have to call the audio suspend handler first and only then > the display driver resume handler. Currently we solve this using the > display driver's late/early suspend/resume hooks, but we'd need a more > robust solution. > > This seems to be a similar issue to the load time ordering problem that > you describe later. Having a real device for avsync that would be a > child of the display device would solve the ordering issue in both > cases. I admit I haven't looked into it if this is feasible, but I would > like to see some solution to this as part of the plan. If we are able create and mandate that HDMI display controller is parent and audio is child device, then this wouldn't be an issue and PM frameowrk will ensure parent is suspended last. -- ~Vinod signature.asc Description: Digital signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] set up an sync channel between audio and display driver (i.e. ALSA and DRM)
On Tue, May 20, 2014 at 10:10:00AM +0200, Takashi Iwai wrote:
> > struct avsink_client {
> > const char *name; /* client name */
> > int type; /* client type*/
> > void *context;
> > struct module *module; /* top-level module for locking */
> >
> > struct avsink_client *peer; /* peer client */
> >
> > /* shared power wells */
> > struct avsink_power_well *power_well;
> > int num_power_wells;
>
> The "power well" is Intel-specific things. Better to use a more
> generic term. (And, I'm always confused what "power well disable"
> means :)
Given that runtime pm is a prevalent usage, wouldn't it make sense
to say that I am HDMI client so keep the resources on? This can be easily
managed if we are able to create the audio device as child of display
controller. That would be implementation agnostic and controller can do whatever
(power well or not) to keep it on/off
>
> >
> > /* endpoints, display outputs or audio inputs */
> > struct avsink_endpoint * endpoint;
> > int num_endpints;
> >
> > struct avsink_registers_ops *reg_ops; /* ops to access registers
> > of a client */
>
> Use const for ops pointers in general (also other cases below).
>
>
> > void *private_data;
> > ...
> > };
> >
> > On system boots, the avsink module is loaded before the display and audio
> > driver module. And the display and audio
> > driver may be loaded on parallel.
>
> For HD-audio HDMI, both controller and codec drivers would need the
> avsink access. So, both drivers will register the own client?
that sound logical here..
>
>
> > * If a specific display driver (eg. i915) supports avsink, it can create a
> > display client, add power wells and display
> > outputs to the client, and then register the display client to the avsink
> > core. Then it may look up if there is any
> > audio client registered, by name or type, and may find an audio client
> > registered by some audio driver.
> >
> > * If an audio driver supports avsink, it usually should look up a
> > registered display client by name or type at first,
> > because it may need the shared power well in GPU and check the display
> > outputs' name to bind the audio inputs. If
> > the display client is not registered yet, the audio driver can choose to
> > wait (maybe in a work queue) or return
> > -EAGAIN for a deferred probe. After the display client is found, the
> > audio driver can register an audio client with
> > the display client's name as the peer name, the avsink core will bind the
> > display and audio clients to each other.
>
> There is already "component" framework, BTW. Can we integrate it into
> avsink instead?
>
>
> > Open question:
> > If the display or audio driver is disabled by the black list, shall we
> > introduce a time out to avoid waiting for the
> > other client registered endlessly?
>
> Yes, timeout sounds like a sensible option.
>
>
> > 2. Shared power wells (optional)
> >
> > The audio and display devices, maybe only part of them, may share a common
> > power well (e.g. for Intel Haswell and
> > Broadwell). If so, the driver that controls the power well should define a
> > power well object, implement the get/put ops,
> > and add it to its avsink client before registering the client to avsink
> > core. Then the peer client can look up this
> > power well by its name, and get/put this power well as a user.
> >
> > A client can have multiple power well objects.
> >
> > struct avsink_power_well {
> > const char *name; /* name of the power well */
> > void *context; /* parameter of get/put ops, maybe device pointer
> > for this power well */
> > struct avsink_power_well_ops *ops
> > };
> >
> > struct avsink_power_well_ops {
> > int (*get)(void *context);
> > int (*put)(void *context);
> > };
> >
> > API:
> > int avsink_new_power(struct avsink_client *client,
> >const char *power_name,
> >void * power_context,
> >struct avsink_power_well_ops *ops,
> >struct avsink_power_well **power_ret);
> >
> > struct avsink_power_well *avisnk_lookup_power(const char *name);
> >
> > int avsink_get_power(struct avsink_power_well *power); /* Reqesut the
> > power */
> > int avsink_put_power(struct avsink_power_well *power);/* Release the
> > power */
> >
> > For example, the i915 display driver can create a device for the shared
> > power well in Haswell GPU, implement its PM
> > functions, and use the device pointer as the context when creating the
> > power well object, like below
> >
> > struct avsink_power_well_ops i915_power_well_ops = {
> > .get = pm_runtime_get_sync;
> > .put = pm_runtime_put_sync;
> > };
>
> This needs function pointer cast, and it's not portable although it'd
> work practically.
>
>
> > ...
> > avsink
