Re: [alsa-devel] [PATCH V2 2/7] drm/i915: Add support for audio driver notifications

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@xxxxxxxxxxxxxxx]
> Sent: Saturday, January 7, 2017 1:50 AM
> To: Anand, Jerome <jerome.anand@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx
> Cc: tiwai@xxxxxxx; broonie@xxxxxxxxxx; Ughreja, Rakesh A
> <rakesh.a.ughreja@xxxxxxxxx>; ville.syrjala@xxxxxxxxxxxxxxx
> Subject: Re: [alsa-devel] [PATCH V2 2/7] drm/i915: Add support for audio
> driver notifications
> 
> Same here, missing fixes on agreed comments?
> 

I have left some since it was addressed already. Comments inline.

> On 1/6/17 7:21 PM, Jerome Anand wrote:
> > Notifiations like mode change, hot plug and edid to the audio driver
> > are added. This is inturn used by the audio driver for its
> > functionality.
> >
> > A new interface file capturing the notifications needed by the audio
> > driver is added
> >
> > Signed-off-by: Pierre-Louis Bossart
> > <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> > Signed-off-by: Jerome Anand <jerome.anand@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h        |  3 +++
> >  drivers/gpu/drm/i915/intel_audio.c     |  8 ++++++
> >  drivers/gpu/drm/i915/intel_hdmi.c      |  1 +
> >  drivers/gpu/drm/i915/intel_lpe_audio.c | 46
> ++++++++++++++++++++++++++++++++++
> >  include/drm/intel_lpe_audio.h          |  1 +
> >  5 files changed, 59 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index 2f8165e..263bc48 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3634,6 +3634,9 @@ int  intel_lpe_audio_setup(struct
> > drm_i915_private *dev_priv);  void intel_lpe_audio_teardown(struct
> > drm_i915_private *dev_priv);  void intel_lpe_audio_irq_handler(struct
> > drm_i915_private *dev_priv);  bool intel_lpe_audio_detect(struct
> > drm_i915_private *dev_priv);
> > +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > +			void *eld, int port, int tmds_clk_speed,
> > +			bool connected);
> >
> >  /* intel_i2c.c */
> >  extern int intel_setup_gmbus(struct drm_i915_private *dev_priv); diff
> > --git a/drivers/gpu/drm/i915/intel_audio.c
> > b/drivers/gpu/drm/i915/intel_audio.c
> > index 16c2027..aeb37c2 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/component.h>
> >  #include <drm/i915_component.h>
> > +#include <drm/intel_lpe_audio.h>
> >  #include "intel_drv.h"
> >
> >  #include <drm/drmP.h>
> > @@ -630,6 +631,10 @@ void intel_audio_codec_enable(struct
> intel_encoder *intel_encoder,
> >  	if (acomp && acomp->audio_ops && acomp->audio_ops-
> >pin_eld_notify)
> >  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> >audio_ptr,
> >  						 (int) port, (int) pipe);
> > +
> > +	if (HAS_LPE_AUDIO(dev_priv))
> > +		intel_lpe_audio_notify(dev_priv, connector->eld, port,
> > +			crtc_state->port_clock, true);
> >  }
> >
> >  /**
> > @@ -663,6 +668,9 @@ void intel_audio_codec_disable(struct
> intel_encoder *intel_encoder)
> >  	if (acomp && acomp->audio_ops && acomp->audio_ops-
> >pin_eld_notify)
> >  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops-
> >audio_ptr,
> >  						 (int) port, (int) pipe);
> > +
> > +	if (HAS_LPE_AUDIO(dev_priv))
> > +		intel_lpe_audio_notify(dev_priv, NULL, port, 0, false);
> >  }
> 
> 
>  From Ville: The entire 'connected' parameter seems superfluous. Also why
> aren't we passing 'pipe' along here? How is the audio driver supposed to find
> the right thing to use?
> 

I thought we discussed this already. Audio need not know about the PIPE/ gfx design.
I prefer to keep this 'connected' parameter.

> >
> >  /**
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 0bcfead..377584e1 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -36,6 +36,7 @@
> >  #include <drm/drm_edid.h>
> >  #include "intel_drv.h"
> >  #include <drm/i915_drm.h>
> > +#include <drm/intel_lpe_audio.h>
> >  #include "i915_drv.h"
> >
> >  static struct drm_device *intel_hdmi_to_dev(struct intel_hdmi
> > *intel_hdmi) diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > index 05f5e4e..2a3c1e8 100644
> > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > @@ -353,3 +353,49 @@ void intel_lpe_audio_teardown(struct
> > drm_i915_private *dev_priv)
> >
> >  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);  }
> > +
> > +
> > +/**
> > + * intel_lpe_audio_notify() - notify lpe audio event
> > + * audio driver and i915
> > + * @dev_priv: the i915 drm device private data
> > + * @eld : ELD data
> > + * @port: port id
> > + * @tmds_clk_speed: tmds clock frequency in Hz
> > + * @connected: hdmi connected/disconnected
> > + *
> > + * Notify lpe audio driver of eld change.
> > + */
> > +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> > +			void *eld, int port, int tmds_clk_speed,
> > +			bool connected)
> > +{
> > +	unsigned long irq_flags;
> > +	struct intel_hdmi_lpe_audio_pdata *pdata = NULL;
> > +
> > +	if (!HAS_LPE_AUDIO(dev_priv))
> > +		return;
> > +
> > +	pdata = dev_get_platdata(
> > +		&(dev_priv->lpe_audio.platdev->dev));
> > +
> > +	spin_lock_irqsave(&pdata->lpe_audio_slock, irq_flags);
> > +
> > +	if (eld != NULL) {
> > +		memcpy(pdata->eld.eld_data, eld,
> > +			HDMI_MAX_ELD_BYTES);
> > +		pdata->eld.port_id = port;
> > +
> > +		if (tmds_clk_speed)
> > +			pdata->tmds_clock_speed = tmds_clk_speed;
> > +	}
> 
>  From Takashi: If eld==NULL means that no ELD is found (or disconnected),
> it's better to clear pdata eld as well, so that we don't leak the previous ELD.
> 
> JA: OK
> 

Eld is sent as NULL if unavailable. So leak is not possible. Hence ignored it.

> > +	pdata->hdmi_connected = connected;
> > +	if (pdata->notify_audio_lpe)
> > +		pdata->notify_audio_lpe(
> > +			(eld != NULL) ? &pdata->eld : NULL);
> > +	else
> > +		pdata->notify_pending = true;
> > +
> > +	spin_unlock_irqrestore(&pdata->lpe_audio_slock,
> > +			irq_flags);
> > +}
> > diff --git a/include/drm/intel_lpe_audio.h
> > b/include/drm/intel_lpe_audio.h index a64c449..952de05 100644
> > --- a/include/drm/intel_lpe_audio.h
> > +++ b/include/drm/intel_lpe_audio.h
> > @@ -25,6 +25,7 @@
> >  #define _INTEL_LPE_AUDIO_H_
> >
> >  #include <linux/types.h>
> > +#include <linux/spinlock_types.h>
> 
>  From Takashi: Why do we need to add a header at this point out of sudden?
> 
> JA: It was for some warning removal. I'll try to remove this if not applicable
> now.
> 

This is needed since we use spin lock in the code.

> 
> >
> >  #define HDMI_MAX_ELD_BYTES	128
> >
> >

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux