diff mbox

[v5,01/14] ASoC: hdac_hdmi: Add hotplug notification and read eld

Message ID 1454161428-7896-2-git-send-email-subhransu.s.prusty@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Subhransu S. Prusty Jan. 30, 2016, 1:43 p.m. UTC
This patch uses i915 component framework to register for hotplug
notification.

In the hotplug notification, driver reads pin sense and ELD by
sending PIN_SENSE and ELD verbs over HDA bus. Once it identifies
valid pin sense and valid eld, store the eld into the
corresponding pin map buffer.

Also read the monitor present sense during resume and ignore the
ELD notify from graphics during PM as is done in legacy hda,
commit 8ae743e82f0b ("ALSA: hda - Skip ELD notification during
system suspend")

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 217 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 211 insertions(+), 6 deletions(-)

Comments

Mark Brown Feb. 9, 2016, 7:42 p.m. UTC | #1
On Sat, Jan 30, 2016 at 07:13:35PM +0530, Subhransu S. Prusty wrote:

> +	if (!pin->eld.monitor_present || !pin->eld.eld_valid) {
> +
> +		dev_info(&edev->hdac.dev, "%s: disconnect for pin %d\n",
> +						__func__, pin->nid);
> +		goto put_hdac_device;

Will this get noisy?

> +		/* TODO: use i915 component for reading ELD later */
> +		if (hdac_hdmi_get_eld(&edev->hdac, pin->nid,
> +				pin->eld.eld_buffer,
> +				&pin->eld.eld_size) == 0) {
> +
> +			print_hex_dump_bytes("Eld: ", DUMP_PREFIX_OFFSET,
> +					pin->eld.eld_buffer, pin->eld.eld_size);

ELD is an acronym, please write it properly (there's some other examples
in the patch).

> +		} else {
> +			dev_err(&edev->hdac.dev, "ELD invalid\n");
> +			pin->eld.monitor_present = false;
> +			pin->eld.eld_valid = false;
> +		}

Is it really invalid or did we fail to read it?  There's a difference as
far as debugging is concerned (one means there's an I/O problem, the
other means that the display is reporting nonsense).  Perhaps this
should be silent and let the read function be more specific?
Vinod Koul Feb. 10, 2016, 4:33 a.m. UTC | #2
On Tue, Feb 09, 2016 at 07:42:06PM +0000, Mark Brown wrote:
> On Sat, Jan 30, 2016 at 07:13:35PM +0530, Subhransu S. Prusty wrote:
> 
> > +	if (!pin->eld.monitor_present || !pin->eld.eld_valid) {
> > +
> > +		dev_info(&edev->hdac.dev, "%s: disconnect for pin %d\n",
> > +						__func__, pin->nid);
> > +		goto put_hdac_device;
> 
> Will this get noisy?

Not really, we print when we get the hotplug notification. Usually that wont
happen too often unless someone is moneky testing :)

> 
> > +		/* TODO: use i915 component for reading ELD later */
> > +		if (hdac_hdmi_get_eld(&edev->hdac, pin->nid,
> > +				pin->eld.eld_buffer,
> > +				&pin->eld.eld_size) == 0) {
> > +
> > +			print_hex_dump_bytes("Eld: ", DUMP_PREFIX_OFFSET,
> > +					pin->eld.eld_buffer, pin->eld.eld_size);
> 
> ELD is an acronym, please write it properly (there's some other examples
> in the patch).

ELD is quite often used acronym in HDMI world, It is already there in
drivers (did a quick grep :)

I will update this and any other acronyms..

> 
> > +		} else {
> > +			dev_err(&edev->hdac.dev, "ELD invalid\n");
> > +			pin->eld.monitor_present = false;
> > +			pin->eld.eld_valid = false;
> > +		}
> 
> Is it really invalid or did we fail to read it?  There's a difference as
> far as debugging is concerned (one means there's an I/O problem, the
> other means that the display is reporting nonsense).  Perhaps this
> should be silent and let the read function be more specific?

There is no way to know if this was IO or garbage from display. So we just
throw up error and retry few times.
Mark Brown Feb. 10, 2016, 7:23 a.m. UTC | #3
On Wed, Feb 10, 2016 at 10:03:58AM +0530, Vinod Koul wrote:
> On Tue, Feb 09, 2016 at 07:42:06PM +0000, Mark Brown wrote:
> > On Sat, Jan 30, 2016 at 07:13:35PM +0530, Subhransu S. Prusty wrote:

> > > +	if (!pin->eld.monitor_present || !pin->eld.eld_valid) {
> > > +
> > > +		dev_info(&edev->hdac.dev, "%s: disconnect for pin %d\n",
> > > +						__func__, pin->nid);
> > > +		goto put_hdac_device;

> > Will this get noisy?

> Not really, we print when we get the hotplug notification. Usually that wont
> happen too often unless someone is moneky testing :)

That sounds like it might get old very quickly, we presumably already
have the graphics stack doing the same thing.

> > > +
> > > +			print_hex_dump_bytes("Eld: ", DUMP_PREFIX_OFFSET,
> > > +					pin->eld.eld_buffer, pin->eld.eld_size);

> > ELD is an acronym, please write it properly (there's some other examples
> > in the patch).

> ELD is quite often used acronym in HDMI world, It is already there in
> drivers (did a quick grep :)

I know it's widely used, I'm saying you should write it properly.

> > > +		} else {
> > > +			dev_err(&edev->hdac.dev, "ELD invalid\n");
> > > +			pin->eld.monitor_present = false;
> > > +			pin->eld.eld_valid = false;
> > > +		}

> > Is it really invalid or did we fail to read it?  There's a difference as
> > far as debugging is concerned (one means there's an I/O problem, the
> > other means that the display is reporting nonsense).  Perhaps this
> > should be silent and let the read function be more specific?

> There is no way to know if this was IO or garbage from display. So we just
> throw up error and retry few times.

It seems implausible that there is no way of identifying I/O errors, and
even if that is the case the point about the message not being terribly
helpful still stands - moving the reporting to the point where we decide
there is an error would be a lot more helpful.
Vinod Koul Feb. 10, 2016, 8:22 a.m. UTC | #4
On Wed, Feb 10, 2016 at 07:23:28AM +0000, Mark Brown wrote:
> On Wed, Feb 10, 2016 at 10:03:58AM +0530, Vinod Koul wrote:
> > On Tue, Feb 09, 2016 at 07:42:06PM +0000, Mark Brown wrote:
> > > On Sat, Jan 30, 2016 at 07:13:35PM +0530, Subhransu S. Prusty wrote:
> 
> > > > +	if (!pin->eld.monitor_present || !pin->eld.eld_valid) {
> > > > +
> > > > +		dev_info(&edev->hdac.dev, "%s: disconnect for pin %d\n",
> > > > +						__func__, pin->nid);
> > > > +		goto put_hdac_device;
> 
> > > Will this get noisy?
> 
> > Not really, we print when we get the hotplug notification. Usually that wont
> > happen too often unless someone is moneky testing :)
> 
> That sounds like it might get old very quickly, we presumably already
> have the graphics stack doing the same thing.

Last I did not see that, but yes lets make it less noisy, will move this to
debug

> 
> > > > +
> > > > +			print_hex_dump_bytes("Eld: ", DUMP_PREFIX_OFFSET,
> > > > +					pin->eld.eld_buffer, pin->eld.eld_size);
> 
> > > ELD is an acronym, please write it properly (there's some other examples
> > > in the patch).
> 
> > ELD is quite often used acronym in HDMI world, It is already there in
> > drivers (did a quick grep :)
> 
> I know it's widely used, I'm saying you should write it properly.

Okay, I think your concern is on writing, sorry I though about the term,
will fix it now :)

> > > > +		} else {
> > > > +			dev_err(&edev->hdac.dev, "ELD invalid\n");
> > > > +			pin->eld.monitor_present = false;
> > > > +			pin->eld.eld_valid = false;
> > > > +		}
> 
> > > Is it really invalid or did we fail to read it?  There's a difference as
> > > far as debugging is concerned (one means there's an I/O problem, the
> > > other means that the display is reporting nonsense).  Perhaps this
> > > should be silent and let the read function be more specific?
> 
> > There is no way to know if this was IO or garbage from display. So we just
> > throw up error and retry few times.
> 
> It seems implausible that there is no way of identifying I/O errors, and
> even if that is the case the point about the message not being terribly
> helpful still stands - moving the reporting to the point where we decide
> there is an error would be a lot more helpful.

Hrmmm, looking back and checking the get_eld does print more verbose
information on the failure, so this one is really not required, we will
remove this

Thanks
diff mbox

Patch

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 5a1ec0f..59abacd 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -34,6 +34,9 @@ 
 
 #define HDA_MAX_CONNECTIONS     32
 
+#define ELD_MAX_SIZE    256
+#define ELD_FIXED_BYTES	20
+
 struct hdac_hdmi_cvt_params {
 	unsigned int channels_min;
 	unsigned int channels_max;
@@ -48,11 +51,22 @@  struct hdac_hdmi_cvt {
 	struct hdac_hdmi_cvt_params params;
 };
 
+struct hdac_hdmi_eld {
+	bool	monitor_present;
+	bool	eld_valid;
+	int	eld_size;
+	char    eld_buffer[ELD_MAX_SIZE];
+};
+
 struct hdac_hdmi_pin {
 	struct list_head head;
 	hda_nid_t nid;
 	int num_mux_nids;
 	hda_nid_t mux_nids[HDA_MAX_CONNECTIONS];
+	struct hdac_hdmi_eld eld;
+	struct hdac_ext_device *edev;
+	int repoll_count;
+	struct delayed_work work;
 };
 
 struct hdac_hdmi_dai_pin_map {
@@ -76,6 +90,80 @@  static inline struct hdac_ext_device *to_hda_ext_device(struct device *dev)
 	return to_ehdac_device(hdac);
 }
 
+ /* HDMI Eld routines */
+static unsigned int hdac_hdmi_get_eld_data(struct hdac_device *codec,
+				hda_nid_t nid, int byte_index)
+{
+	unsigned int val;
+
+	val = snd_hdac_codec_read(codec, nid, 0, AC_VERB_GET_HDMI_ELDD,
+							byte_index);
+
+	dev_dbg(&codec->dev, "HDMI: ELD data byte %d: 0x%x\n",
+					byte_index, val);
+
+	return val;
+}
+
+static int hdac_hdmi_get_eld_size(struct hdac_device *codec, hda_nid_t nid)
+{
+	return snd_hdac_codec_read(codec, nid, 0, AC_VERB_GET_HDMI_DIP_SIZE,
+						 AC_DIPSIZE_ELD_BUF);
+}
+
+/*
+ * This function queries the eld size and eld data and fills in the buffer
+ * passed by user
+ */
+static int hdac_hdmi_get_eld(struct hdac_device *codec, hda_nid_t nid,
+			     unsigned char *buf, int *eld_size)
+{
+	int i, size, ret = 0;
+
+	/*
+	 * ELD size is initialized to zero in caller function. If no errors and
+	 * ELD is valid, actual eld_size is assigned.
+	 */
+
+	size = hdac_hdmi_get_eld_size(codec, nid);
+	if (size < ELD_FIXED_BYTES || size > ELD_MAX_SIZE) {
+		dev_info(&codec->dev, "HDMI: invalid ELD buf size %d\n", size);
+		return -ERANGE;
+	}
+
+	/* set ELD buffer */
+	for (i = 0; i < size; i++) {
+		unsigned int val = hdac_hdmi_get_eld_data(codec, nid, i);
+		/*
+		 * Graphics driver might be writing to ELD buffer right now.
+		 * Just abort. The caller will repoll after a while.
+		 */
+		if (!(val & AC_ELDD_ELD_VALID)) {
+			dev_info(&codec->dev,
+				"HDMI: invalid ELD data byte %d\n", i);
+			ret = -EINVAL;
+			goto error;
+		}
+		val &= AC_ELDD_ELD_DATA;
+		/*
+		 * The first byte cannot be zero. This can happen on some DVI
+		 * connections. Some Intel chips may also need some 250ms delay
+		 * to return non-zero ELD data, even when the graphics driver
+		 * correctly writes ELD content before setting ELD_valid bit.
+		 */
+		if (!val && !i) {
+			dev_dbg(&codec->dev, "HDMI: 0 ELD data\n");
+			ret = -EINVAL;
+			goto error;
+		}
+		buf[i] = val;
+	}
+
+	*eld_size = size;
+error:
+	return ret;
+}
+
 static int hdac_hdmi_setup_stream(struct hdac_ext_device *hdac,
 				hda_nid_t cvt_nid, hda_nid_t pin_nid,
 				u32 stream_tag, int format)
@@ -246,7 +334,6 @@  static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
 	struct hdac_ext_device *hdac = snd_soc_dai_get_drvdata(dai);
 	struct hdac_hdmi_priv *hdmi = hdac->private_data;
 	struct hdac_hdmi_dai_pin_map *dai_map;
-	int val;
 
 	if (dai->id > 0) {
 		dev_err(&hdac->hdac.dev, "Only one dai supported as of now\n");
@@ -255,12 +342,13 @@  static int hdac_hdmi_pcm_open(struct snd_pcm_substream *substream,
 
 	dai_map = &hdmi->dai_map[dai->id];
 
-	val = snd_hdac_codec_read(&hdac->hdac, dai_map->pin->nid, 0,
-					AC_VERB_GET_PIN_SENSE, 0);
-	dev_info(&hdac->hdac.dev, "Val for AC_VERB_GET_PIN_SENSE: %x\n", val);
+	if ((!dai_map->pin->eld.monitor_present) ||
+			(!dai_map->pin->eld.eld_valid)) {
+
+		dev_err(&hdac->hdac.dev, "Failed: montior present? %d eld valid?: %d\n",
+				dai_map->pin->eld.monitor_present,
+				dai_map->pin->eld.eld_valid);
 
-	if ((!(val & AC_PINSENSE_PRESENCE)) || (!(val & AC_PINSENSE_ELDV))) {
-		dev_err(&hdac->hdac.dev, "Monitor presence invalid with val: %x\n", val);
 		return -ENODEV;
 	}
 
@@ -410,6 +498,72 @@  static int hdac_hdmi_add_cvt(struct hdac_ext_device *edev, hda_nid_t nid)
 	return hdac_hdmi_query_cvt_params(&edev->hdac, cvt);
 }
 
+static void hdac_hdmi_present_sense(struct hdac_hdmi_pin *pin, int repoll)
+{
+	struct hdac_ext_device *edev = pin->edev;
+	int val;
+
+	if (!edev)
+		return;
+
+	pin->repoll_count = repoll;
+
+	pm_runtime_get_sync(&edev->hdac.dev);
+	val = snd_hdac_codec_read(&edev->hdac, pin->nid, 0,
+					AC_VERB_GET_PIN_SENSE, 0);
+
+	dev_dbg(&edev->hdac.dev, "Pin sense val %x for pin: %d\n",
+						val, pin->nid);
+
+	pin->eld.monitor_present = !!(val & AC_PINSENSE_PRESENCE);
+	pin->eld.eld_valid = !!(val & AC_PINSENSE_ELDV);
+
+	if (!pin->eld.monitor_present || !pin->eld.eld_valid) {
+
+		dev_info(&edev->hdac.dev, "%s: disconnect for pin %d\n",
+						__func__, pin->nid);
+		goto put_hdac_device;
+	}
+
+	if (pin->eld.monitor_present && pin->eld.eld_valid) {
+		/* TODO: use i915 component for reading ELD later */
+		if (hdac_hdmi_get_eld(&edev->hdac, pin->nid,
+				pin->eld.eld_buffer,
+				&pin->eld.eld_size) == 0) {
+
+			print_hex_dump_bytes("Eld: ", DUMP_PREFIX_OFFSET,
+					pin->eld.eld_buffer, pin->eld.eld_size);
+		} else {
+			dev_err(&edev->hdac.dev, "ELD invalid\n");
+			pin->eld.monitor_present = false;
+			pin->eld.eld_valid = false;
+		}
+	}
+
+	/*
+	 * Sometimes the pin_sense may present invalid monitor
+	 * present and eld_valid. If eld data is not valid loop, few
+	 * more times to get correct pin sense and valid eld.
+	 */
+	if ((!pin->eld.monitor_present || !pin->eld.eld_valid) && repoll)
+		schedule_delayed_work(&pin->work, msecs_to_jiffies(300));
+
+put_hdac_device:
+	pm_runtime_put_sync(&edev->hdac.dev);
+}
+
+static void hdac_hdmi_repoll_eld(struct work_struct *work)
+{
+	struct hdac_hdmi_pin *pin =
+		container_of(to_delayed_work(work), struct hdac_hdmi_pin, work);
+
+	/* picked from legacy HDA driver */
+	if (pin->repoll_count++ > 6)
+		pin->repoll_count = 0;
+
+	hdac_hdmi_present_sense(pin, pin->repoll_count);
+}
+
 static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
 {
 	struct hdac_hdmi_priv *hdmi = edev->private_data;
@@ -424,6 +578,9 @@  static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
 	list_add_tail(&pin->head, &hdmi->pin_list);
 	hdmi->num_pin++;
 
+	pin->edev = edev;
+	INIT_DELAYED_WORK(&pin->work, hdac_hdmi_repoll_eld);
+
 	return 0;
 }
 
@@ -482,17 +639,65 @@  static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
 	return hdac_hdmi_init_dai_map(edev);
 }
 
+static void hdac_hdmi_eld_notify_cb(void *aptr, int port)
+{
+	struct hdac_ext_device *edev = aptr;
+	struct hdac_hdmi_priv *hdmi = edev->private_data;
+	struct hdac_hdmi_pin *pin;
+	struct snd_soc_codec *codec = edev->scodec;
+
+	/* Don't know how this mapping is derived */
+	hda_nid_t pin_nid = port + 0x04;
+
+	dev_dbg(&edev->hdac.dev, "%s: for pin: %d\n", __func__, pin_nid);
+
+	/*
+	 * skip notification during system suspend (but not in runtime PM);
+	 * the state will be updated at resume. Also since the ELD and
+	 * connection states are updated in anyway at the end of the resume,
+	 * we can skip it when received during PM process.
+	 */
+	if (snd_power_get_state(codec->component.card->snd_card) !=
+			SNDRV_CTL_POWER_D0)
+		return;
+
+	if (atomic_read(&edev->hdac.in_pm))
+		return;
+
+	list_for_each_entry(pin, &hdmi->pin_list, head) {
+		if (pin->nid == pin_nid)
+			hdac_hdmi_present_sense(pin, 1);
+	}
+}
+
+static struct i915_audio_component_audio_ops aops = {
+	.pin_eld_notify	= hdac_hdmi_eld_notify_cb,
+};
+
 static int hdmi_codec_probe(struct snd_soc_codec *codec)
 {
 	struct hdac_ext_device *edev = snd_soc_codec_get_drvdata(codec);
 	struct hdac_hdmi_priv *hdmi = edev->private_data;
 	struct snd_soc_dapm_context *dapm =
 		snd_soc_component_get_dapm(&codec->component);
+	struct hdac_hdmi_pin *pin;
+	int ret;
 
 	edev->scodec = codec;
 
 	create_fill_widget_route_map(dapm, &hdmi->dai_map[0]);
 
+	aops.audio_ptr = edev;
+	ret = snd_hdac_i915_register_notifier(&aops);
+	if (ret < 0) {
+		dev_err(&edev->hdac.dev, "notifier register failed: err: %d\n",
+				ret);
+		return ret;
+	}
+
+	list_for_each_entry(pin, &hdmi->pin_list, head)
+		hdac_hdmi_present_sense(pin, 1);
+
 	/* Imp: Store the card pointer in hda_codec */
 	edev->card = dapm->card->snd_card;