Re: [Intel-gfx] [RFC PATCH v2 1/8] drm/i915: setup bridge for HDMI LPE audio driver

2016-10-17 Thread Daniel Vetter 
On Sat, Oct 01, 2016 at 05:52:35AM +0530, Jerome Anand wrote:
> Enable support for HDMI LPE audio mode on Baytrail and
> Cherrytrail when HDaudio controller is not detected
> 
> Setup minimum required resources during i915_driver_load:
> 1. Create a platform device to share MMIO/IRQ resources
> 2. Make the platform device child of i915 device for runtime PM.
> 3. Create IRQ chip to forward HDMI LPE audio irqs.
> 
> HDMI LPE audio driver (a standalone sound driver) probes the
> LPE audio device and creates a new sound card.
> 
> Signed-off-by: Pierre-Louis Bossart 
> Signed-off-by: Jerome Anand 

You need to add an include-stanza to Documentation/gpu/i915.rst,
otherwise the kerneldoc won't show up. Please also do some docs for the
structures, and then run

$ make htmldocs

to check the output looks reasonable.

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/Makefile  |   3 +
>  drivers/gpu/drm/i915/i915_drv.c|  13 +-
>  drivers/gpu/drm/i915/i915_drv.h|  19 ++
>  drivers/gpu/drm/i915/i915_irq.c|  14 ++
>  drivers/gpu/drm/i915/i915_reg.h|   3 +
>  drivers/gpu/drm/i915/intel_lpe_audio.c | 357 
> +
>  include/drm/intel_lpe_audio.h  |  45 +
>  7 files changed, 452 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_lpe_audio.c
>  create mode 100644 include/drm/intel_lpe_audio.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e6fe004..11f9741 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -115,6 +115,9 @@ i915-y += intel_gvt.o
>  include $(src)/gvt/Makefile
>  endif
>  
> +# LPE Audio for VLV and CHT
> +i915-y += intel_lpe_audio.o
> +
>  obj-$(CONFIG_DRM_I915) += i915.o
>  
>  CFLAGS_i915_trace_points.o := -I$(src)
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 31b2b63..ab1e4768 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1141,7 +1141,13 @@ static void i915_driver_register(struct 
> drm_i915_private *dev_priv)
>   if (IS_GEN5(dev_priv))
>   intel_gpu_ips_init(dev_priv);
>  
> - i915_audio_component_init(dev_priv);
> + if (intel_lpe_audio_detect(dev_priv)) {
> + if (intel_lpe_audio_setup(dev_priv) < 0)
> + DRM_ERROR("failed to setup LPE Audio bridge\n");
> + }
> +
> + if (!IS_LPE_AUDIO_ENABLED(dev_priv))
> + i915_audio_component_init(dev_priv);
>  
>   /*
>* Some ports require correctly set-up hpd registers for detection to
> @@ -1159,7 +1165,10 @@ static void i915_driver_register(struct 
> drm_i915_private *dev_priv)
>   */
>  static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  {
> - i915_audio_component_cleanup(dev_priv);
> + if (IS_LPE_AUDIO_ENABLED(dev_priv))
> + intel_lpe_audio_teardown(dev_priv);
> + else
> + i915_audio_component_cleanup(dev_priv);
>  
>   intel_gpu_ips_teardown();
>   acpi_video_unregister();
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 91ff3d7..399a8ee 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2087,6 +2087,12 @@ struct drm_i915_private {
>   /* Used to save the pipe-to-encoder mapping for audio */
>   struct intel_encoder *av_enc_map[I915_MAX_PIPES];
>  
> + /* necessary resource sharing with HDMI LPE audio driver. */
> + struct {
> + struct platform_device *platdev;
> + int irq;
> + } lpe_audio;
> +
>   /*
>* NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>* will be rejected. Instead look for a better place.
> @@ -2827,6 +2833,13 @@ struct drm_i915_cmd_table {
>  
>  #define HAS_POOLED_EU(dev)   (INTEL_INFO(dev)->has_pooled_eu)
>  
> +#define HAS_LPE_AUDIO(dev)   (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> +#define IS_LPE_AUDIO_ENABLED(dev_priv) \
> + (__I915__(dev_priv)->lpe_audio.platdev != NULL)
> +#define IS_LPE_AUDIO_IRQ_VALID(dev_priv) \
> + (__I915__(dev_priv)->lpe_audio.irq >= 0)
> +
> +
>  #define INTEL_PCH_DEVICE_ID_MASK 0xff00
>  #define INTEL_PCH_IBX_DEVICE_ID_TYPE 0x3b00
>  #define INTEL_PCH_CPT_DEVICE_ID_TYPE 0x1c00
> @@ -3579,6 +3592,12 @@ extern int i915_restore_state(struct drm_device *dev);
>  void i915_setup_sysfs(struct drm_i915_private *dev_priv);
>  void i915_teardown_sysfs(struct drm_i915_private *dev_priv);
>  
> +/* i915_lpe_audio.c */
> +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);
> +int intel_lpe_audio_detect(struct drm_i915_private *dev_priv);
> +
>

Re: [Intel-gfx] [RFC PATCH v2 1/8] drm/i915: setup bridge for HDMI LPE audio driver

2016-10-14 Thread Jani Nikula 
On Fri, 14 Oct 2016, Ville Syrjälä  wrote:
> On Thu, Oct 13, 2016 at 02:38:30PM -0500, Pierre-Louis Bossart wrote:
>> Thanks Ville for the review. A lot of the comments are related to the 
>> initial VED code we took pretty much as is, no issues to clean-up further.
>> 
>> BTW, it looks like Jerome's patches were stuck for 10+ days on the 
>> intel-gfx server for some reason so not everyone saw the initial post?
>
> Did they make it to the list? Jani told me they didn't, nor were they in
> the moderation queue apparently. So no idea where they went. I tried
> bouncing them to the list, but I'm not sure they came through that time
> either.

The patches as bounced by Ville made it to the list, but the original
ones were lost and I have no idea what happened. They were not in
moderation and there was no trace of them.

BR,
Jani.



>
>> 
>> >> @@ -1141,7 +1141,13 @@ static void i915_driver_register(struct 
>> >> drm_i915_private *dev_priv)
>> >>   if (IS_GEN5(dev_priv))
>> >>   intel_gpu_ips_init(dev_priv);
>> >>
>> >> - i915_audio_component_init(dev_priv);
>> >> + if (intel_lpe_audio_detect(dev_priv)) {
>> >> + if (intel_lpe_audio_setup(dev_priv) < 0)
>> >> + DRM_ERROR("failed to setup LPE Audio bridge\n");
>> >> + }
>> >
>> > I'd move all that into the lpe audio code. No need to have anything here
>> > but a single function call IMO.
>> 
>> something like intel_lpe_audio_init() for symmetry with the hda 
>> component stuff, with both detection and setup handled internally?
>> >
>> >> +
>> >> + if (!IS_LPE_AUDIO_ENABLED(dev_priv))
>> >
>> > I don't like that too much. I think I would just make
>> > that HAS_LPE_AUDIO(). The current IS_VLV||IS_CHV check can just be
>> > inlined into the init function.
>> 
>> ok
>> 
>> 
>> >>
>> >>  #define HAS_POOLED_EU(dev)   (INTEL_INFO(dev)->has_pooled_eu)
>> >>
>> >> +#define HAS_LPE_AUDIO(dev)   (IS_VALLEYVIEW(dev) || 
>> >> IS_CHERRYVIEW(dev))
>> >> +#define IS_LPE_AUDIO_ENABLED(dev_priv) \
>> >> + (__I915__(dev_priv)->lpe_audio.platdev != NULL)
>> >> +#define IS_LPE_AUDIO_IRQ_VALID(dev_priv) \
>> >> + (__I915__(dev_priv)->lpe_audio.irq >= 0)
>> >
>> > Seems useless. We should just not register the lpe audio thing if we
>> > have no irq.
>> 
>> ok
>> 
>> >> --- a/drivers/gpu/drm/i915/i915_irq.c
>> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> >> @@ -1827,6 +1827,13 @@ static irqreturn_t valleyview_irq_handler(int irq, 
>> >> void *arg)
>> >>* signalled in iir */
>> >>   valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
>> >>
>> >> + if (IS_LPE_AUDIO_ENABLED(dev_priv))
>> >> + if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
>> >
>> > I think both of these checks can be removed. We won't unmask the
>> > interrupts unless lpe is enabled, so the IIR bits will never be set in
>> > that case.
>> >
>> >> + if (iir & (I915_LPE_PIPE_A_INTERRUPT |
>> >> + I915_LPE_PIPE_B_INTERRUPT |
>> >> + I915_LPE_PIPE_C_INTERRUPT))
>> >> + intel_lpe_audio_irq_handler(dev_priv);
>> >> +
>> 
>> ok.
>> 
>> >>   /*
>> >>* VLV_IIR is single buffered, and reflects the level
>> >>* from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
>> >> @@ -1907,6 +1914,13 @@ static irqreturn_t cherryview_irq_handler(int irq, 
>> >> void *arg)
>> >>* signalled in iir */
>> >>   valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
>> >>
>> >> + if (IS_LPE_AUDIO_ENABLED(dev_priv))
>> >> + if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
>> >> + if (iir & (I915_LPE_PIPE_A_INTERRUPT |
>> >> + I915_LPE_PIPE_B_INTERRUPT |
>> >> + I915_LPE_PIPE_C_INTERRUPT))
>> >> + intel_lpe_audio_irq_handler(dev_priv);
>> >> +
>> >
>> > ditto
>> 
>> ok
>> 
>> 
>> >> + platdev = platform_device_alloc("hdmi-lpe-audio", -1);
>> >> + if (!platdev) {
>> >> + ret = -ENOMEM;
>> >> + DRM_ERROR("Failed to allocate LPE audio platform device\n");
>> >> + goto err;
>> >> + }
>> >> +
>> >> + /* to work-around check_addr in nommu_map_sg() */
>> >
>> > What's this about?
>> 
>> Dunno, this was in the original VED series that we used as a baseline. 
>> Unless someone has memories from that time, i'll just remove this comment.
>> 
>> 
>> >> + DRM_DEBUG("%s: HDMI_AUDIO rsc.start[0] = 0x%x\n",
>> >> + __func__, (int)(rsc[0].start));
>> >
>> > __func__ pointless since DRM_DEBUG alreay adds it. Also saying
>> > "rsc.start[0]" in the message doesn't tell anyone anything useful.
>> > And I think irq numbers are usually printed in decimal.
>> 
>> Same, this was taken from the VED series, no issue to clean-up.
>> 
>> >
>> >> +
>> >> + rsc[1].start=

Re: [Intel-gfx] [RFC PATCH v2 1/8] drm/i915: setup bridge for HDMI LPE audio driver

2016-10-14 Thread Ville Syrjälä 
On Thu, Oct 13, 2016 at 02:38:30PM -0500, Pierre-Louis Bossart wrote:
> Thanks Ville for the review. A lot of the comments are related to the 
> initial VED code we took pretty much as is, no issues to clean-up further.
> 
> BTW, it looks like Jerome's patches were stuck for 10+ days on the 
> intel-gfx server for some reason so not everyone saw the initial post?

Did they make it to the list? Jani told me they didn't, nor were they in
the moderation queue apparently. So no idea where they went. I tried
bouncing them to the list, but I'm not sure they came through that time
either.

> 
> >> @@ -1141,7 +1141,13 @@ static void i915_driver_register(struct 
> >> drm_i915_private *dev_priv)
> >>if (IS_GEN5(dev_priv))
> >>intel_gpu_ips_init(dev_priv);
> >>
> >> -  i915_audio_component_init(dev_priv);
> >> +  if (intel_lpe_audio_detect(dev_priv)) {
> >> +  if (intel_lpe_audio_setup(dev_priv) < 0)
> >> +  DRM_ERROR("failed to setup LPE Audio bridge\n");
> >> +  }
> >
> > I'd move all that into the lpe audio code. No need to have anything here
> > but a single function call IMO.
> 
> something like intel_lpe_audio_init() for symmetry with the hda 
> component stuff, with both detection and setup handled internally?
> >
> >> +
> >> +  if (!IS_LPE_AUDIO_ENABLED(dev_priv))
> >
> > I don't like that too much. I think I would just make
> > that HAS_LPE_AUDIO(). The current IS_VLV||IS_CHV check can just be
> > inlined into the init function.
> 
> ok
> 
> 
> >>
> >>  #define HAS_POOLED_EU(dev)(INTEL_INFO(dev)->has_pooled_eu)
> >>
> >> +#define HAS_LPE_AUDIO(dev)(IS_VALLEYVIEW(dev) || 
> >> IS_CHERRYVIEW(dev))
> >> +#define IS_LPE_AUDIO_ENABLED(dev_priv) \
> >> +  (__I915__(dev_priv)->lpe_audio.platdev != NULL)
> >> +#define IS_LPE_AUDIO_IRQ_VALID(dev_priv) \
> >> +  (__I915__(dev_priv)->lpe_audio.irq >= 0)
> >
> > Seems useless. We should just not register the lpe audio thing if we
> > have no irq.
> 
> ok
> 
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -1827,6 +1827,13 @@ static irqreturn_t valleyview_irq_handler(int irq, 
> >> void *arg)
> >> * signalled in iir */
> >>valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> >>
> >> +  if (IS_LPE_AUDIO_ENABLED(dev_priv))
> >> +  if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
> >
> > I think both of these checks can be removed. We won't unmask the
> > interrupts unless lpe is enabled, so the IIR bits will never be set in
> > that case.
> >
> >> +  if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> >> +  I915_LPE_PIPE_B_INTERRUPT |
> >> +  I915_LPE_PIPE_C_INTERRUPT))
> >> +  intel_lpe_audio_irq_handler(dev_priv);
> >> +
> 
> ok.
> 
> >>/*
> >> * VLV_IIR is single buffered, and reflects the level
> >> * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
> >> @@ -1907,6 +1914,13 @@ static irqreturn_t cherryview_irq_handler(int irq, 
> >> void *arg)
> >> * signalled in iir */
> >>valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);
> >>
> >> +  if (IS_LPE_AUDIO_ENABLED(dev_priv))
> >> +  if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
> >> +  if (iir & (I915_LPE_PIPE_A_INTERRUPT |
> >> +  I915_LPE_PIPE_B_INTERRUPT |
> >> +  I915_LPE_PIPE_C_INTERRUPT))
> >> +  intel_lpe_audio_irq_handler(dev_priv);
> >> +
> >
> > ditto
> 
> ok
> 
> 
> >> +  platdev = platform_device_alloc("hdmi-lpe-audio", -1);
> >> +  if (!platdev) {
> >> +  ret = -ENOMEM;
> >> +  DRM_ERROR("Failed to allocate LPE audio platform device\n");
> >> +  goto err;
> >> +  }
> >> +
> >> +  /* to work-around check_addr in nommu_map_sg() */
> >
> > What's this about?
> 
> Dunno, this was in the original VED series that we used as a baseline. 
> Unless someone has memories from that time, i'll just remove this comment.
> 
> 
> >> +  DRM_DEBUG("%s: HDMI_AUDIO rsc.start[0] = 0x%x\n",
> >> +  __func__, (int)(rsc[0].start));
> >
> > __func__ pointless since DRM_DEBUG alreay adds it. Also saying
> > "rsc.start[0]" in the message doesn't tell anyone anything useful.
> > And I think irq numbers are usually printed in decimal.
> 
> Same, this was taken from the VED series, no issue to clean-up.
> 
> >
> >> +
> >> +  rsc[1].start= pci_resource_start(dev->pdev, 0) +
> >> +  I915_HDMI_LPE_AUDIO_BASE;
> >> +  rsc[1].end  = pci_resource_start(dev->pdev, 0) +
> >> +  I915_HDMI_LPE_AUDIO_BASE + I915_HDMI_LPE_AUDIO_SIZE - 1;
> >> +  rsc[1].flags= IORESOURCE_MEM;
> >> +  rsc[1].name = "hdmi-lpe-audio-mmio";
> >> +
> >> +  DRM_DEBUG("%s: HDMI_AUDIO rsc.start[1] = 0x%x\n",
> >> +

Re: [Intel-gfx] [RFC PATCH v2 1/8] drm/i915: setup bridge for HDMI LPE audio driver

2016-10-13 Thread Pierre-Louis Bossart 
Thanks Ville for the review. A lot of the comments are related to the 
initial VED code we took pretty much as is, no issues to clean-up further.


BTW, it looks like Jerome's patches were stuck for 10+ days on the 
intel-gfx server for some reason so not everyone saw the initial post?



@@ -1141,7 +1141,13 @@ static void i915_driver_register(struct drm_i915_private 
*dev_priv)
if (IS_GEN5(dev_priv))
intel_gpu_ips_init(dev_priv);

-   i915_audio_component_init(dev_priv);
+   if (intel_lpe_audio_detect(dev_priv)) {
+   if (intel_lpe_audio_setup(dev_priv) < 0)
+   DRM_ERROR("failed to setup LPE Audio bridge\n");
+   }


I'd move all that into the lpe audio code. No need to have anything here
but a single function call IMO.


something like intel_lpe_audio_init() for symmetry with the hda 
component stuff, with both detection and setup handled internally?



+
+   if (!IS_LPE_AUDIO_ENABLED(dev_priv))


I don't like that too much. I think I would just make
that HAS_LPE_AUDIO(). The current IS_VLV||IS_CHV check can just be
inlined into the init function.


ok




 #define HAS_POOLED_EU(dev) (INTEL_INFO(dev)->has_pooled_eu)

+#define HAS_LPE_AUDIO(dev) (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
+#define IS_LPE_AUDIO_ENABLED(dev_priv) \
+   (__I915__(dev_priv)->lpe_audio.platdev != NULL)
+#define IS_LPE_AUDIO_IRQ_VALID(dev_priv) \
+   (__I915__(dev_priv)->lpe_audio.irq >= 0)


Seems useless. We should just not register the lpe audio thing if we
have no irq.


ok


--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1827,6 +1827,13 @@ static irqreturn_t valleyview_irq_handler(int irq, void 
*arg)
 * signalled in iir */
valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);

+   if (IS_LPE_AUDIO_ENABLED(dev_priv))
+   if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))


I think both of these checks can be removed. We won't unmask the
interrupts unless lpe is enabled, so the IIR bits will never be set in
that case.


+   if (iir & (I915_LPE_PIPE_A_INTERRUPT |
+   I915_LPE_PIPE_B_INTERRUPT |
+   I915_LPE_PIPE_C_INTERRUPT))
+   intel_lpe_audio_irq_handler(dev_priv);
+


ok.


/*
 * VLV_IIR is single buffered, and reflects the level
 * from PIPESTAT/PORT_HOTPLUG_STAT, hence clear it last.
@@ -1907,6 +1914,13 @@ static irqreturn_t cherryview_irq_handler(int irq, void 
*arg)
 * signalled in iir */
valleyview_pipestat_irq_ack(dev_priv, iir, pipe_stats);

+   if (IS_LPE_AUDIO_ENABLED(dev_priv))
+   if (IS_LPE_AUDIO_IRQ_VALID(dev_priv))
+   if (iir & (I915_LPE_PIPE_A_INTERRUPT |
+   I915_LPE_PIPE_B_INTERRUPT |
+   I915_LPE_PIPE_C_INTERRUPT))
+   intel_lpe_audio_irq_handler(dev_priv);
+


ditto


ok



+   platdev = platform_device_alloc("hdmi-lpe-audio", -1);
+   if (!platdev) {
+   ret = -ENOMEM;
+   DRM_ERROR("Failed to allocate LPE audio platform device\n");
+   goto err;
+   }
+
+   /* to work-around check_addr in nommu_map_sg() */


What's this about?


Dunno, this was in the original VED series that we used as a baseline. 
Unless someone has memories from that time, i'll just remove this comment.




+   DRM_DEBUG("%s: HDMI_AUDIO rsc.start[0] = 0x%x\n",
+   __func__, (int)(rsc[0].start));


__func__ pointless since DRM_DEBUG alreay adds it. Also saying
"rsc.start[0]" in the message doesn't tell anyone anything useful.
And I think irq numbers are usually printed in decimal.


Same, this was taken from the VED series, no issue to clean-up.




+
+   rsc[1].start= pci_resource_start(dev->pdev, 0) +
+   I915_HDMI_LPE_AUDIO_BASE;
+   rsc[1].end  = pci_resource_start(dev->pdev, 0) +
+   I915_HDMI_LPE_AUDIO_BASE + I915_HDMI_LPE_AUDIO_SIZE - 1;
+   rsc[1].flags= IORESOURCE_MEM;
+   rsc[1].name = "hdmi-lpe-audio-mmio";
+
+   DRM_DEBUG("%s: HDMI_AUDIO rsc.start[1] = 0x%x\n",
+   __func__, (int)(rsc[1].start));


Again "rsc[0].start" doesn't seem like anything useful to print in a
debug message. Don't see much point in the whole debug message TBH since
the start/end are fixed.


so are you saying we should remove the code or move the level to 
DRM_INFO - for extra verbose debug?





+
+   ret = platform_device_add_resources(platdev, rsc, 2);
+   if (ret) {
+   DRM_ERROR("Failed to add resource for platform device: %d\n",
+   ret);
+   goto

[Intel-gfx] [RFC PATCH v2 1/8] drm/i915: setup bridge for HDMI LPE audio driver

2016-10-12 Thread Jerome Anand 
Enable support for HDMI LPE audio mode on Baytrail and
Cherrytrail when HDaudio controller is not detected

Setup minimum required resources during i915_driver_load:
1. Create a platform device to share MMIO/IRQ resources
2. Make the platform device child of i915 device for runtime PM.
3. Create IRQ chip to forward HDMI LPE audio irqs.

HDMI LPE audio driver (a standalone sound driver) probes the
LPE audio device and creates a new sound card.

Signed-off-by: Pierre-Louis Bossart 
Signed-off-by: Jerome Anand 
---
 drivers/gpu/drm/i915/Makefile  |   3 +
 drivers/gpu/drm/i915/i915_drv.c|  13 +-
 drivers/gpu/drm/i915/i915_drv.h|  19 ++
 drivers/gpu/drm/i915/i915_irq.c|  14 ++
 drivers/gpu/drm/i915/i915_reg.h|   3 +
 drivers/gpu/drm/i915/intel_lpe_audio.c | 357 +
 include/drm/intel_lpe_audio.h  |  45 +
 7 files changed, 452 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_lpe_audio.c
 create mode 100644 include/drm/intel_lpe_audio.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index e6fe004..11f9741 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -115,6 +115,9 @@ i915-y += intel_gvt.o
 include $(src)/gvt/Makefile
 endif
 
+# LPE Audio for VLV and CHT
+i915-y += intel_lpe_audio.o
+
 obj-$(CONFIG_DRM_I915) += i915.o
 
 CFLAGS_i915_trace_points.o := -I$(src)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 31b2b63..ab1e4768 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1141,7 +1141,13 @@ static void i915_driver_register(struct drm_i915_private 
*dev_priv)
if (IS_GEN5(dev_priv))
intel_gpu_ips_init(dev_priv);
 
-   i915_audio_component_init(dev_priv);
+   if (intel_lpe_audio_detect(dev_priv)) {
+   if (intel_lpe_audio_setup(dev_priv) < 0)
+   DRM_ERROR("failed to setup LPE Audio bridge\n");
+   }
+
+   if (!IS_LPE_AUDIO_ENABLED(dev_priv))
+   i915_audio_component_init(dev_priv);
 
/*
 * Some ports require correctly set-up hpd registers for detection to
@@ -1159,7 +1165,10 @@ static void i915_driver_register(struct drm_i915_private 
*dev_priv)
  */
 static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 {
-   i915_audio_component_cleanup(dev_priv);
+   if (IS_LPE_AUDIO_ENABLED(dev_priv))
+   intel_lpe_audio_teardown(dev_priv);
+   else
+   i915_audio_component_cleanup(dev_priv);
 
intel_gpu_ips_teardown();
acpi_video_unregister();
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 91ff3d7..399a8ee 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2087,6 +2087,12 @@ struct drm_i915_private {
/* Used to save the pipe-to-encoder mapping for audio */
struct intel_encoder *av_enc_map[I915_MAX_PIPES];
 
+   /* necessary resource sharing with HDMI LPE audio driver. */
+   struct {
+   struct platform_device *platdev;
+   int irq;
+   } lpe_audio;
+
/*
 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 * will be rejected. Instead look for a better place.
@@ -2827,6 +2833,13 @@ struct drm_i915_cmd_table {
 
 #define HAS_POOLED_EU(dev) (INTEL_INFO(dev)->has_pooled_eu)
 
+#define HAS_LPE_AUDIO(dev) (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
+#define IS_LPE_AUDIO_ENABLED(dev_priv) \
+   (__I915__(dev_priv)->lpe_audio.platdev != NULL)
+#define IS_LPE_AUDIO_IRQ_VALID(dev_priv) \
+   (__I915__(dev_priv)->lpe_audio.irq >= 0)
+
+
 #define INTEL_PCH_DEVICE_ID_MASK   0xff00
 #define INTEL_PCH_IBX_DEVICE_ID_TYPE   0x3b00
 #define INTEL_PCH_CPT_DEVICE_ID_TYPE   0x1c00
@@ -3579,6 +3592,12 @@ extern int i915_restore_state(struct drm_device *dev);
 void i915_setup_sysfs(struct drm_i915_private *dev_priv);
 void i915_teardown_sysfs(struct drm_i915_private *dev_priv);
 
+/* i915_lpe_audio.c */
+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);
+int intel_lpe_audio_detect(struct drm_i915_private *dev_priv);
+
 /* intel_i2c.c */
 extern int intel_setup_gmbus(struct drm_device *dev);
 extern void intel_teardown_gmbus(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6ed5b24..d8f515f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1827,6 +1827,13 @@ static irqreturn_t valleyview_irq_handler(int irq, void 
*arg)
 * signalled in iir */

Re: [Intel-gfx] [RFC PATCH v2 1/8] drm/i915: setup bridge for HDMI LPE audio driver

2016-10-11 Thread Ville Syrjälä 
On Sat, Oct 01, 2016 at 05:52:35AM +0530, Jerome Anand wrote:
> Enable support for HDMI LPE audio mode on Baytrail and
> Cherrytrail when HDaudio controller is not detected
> 
> Setup minimum required resources during i915_driver_load:
> 1. Create a platform device to share MMIO/IRQ resources
> 2. Make the platform device child of i915 device for runtime PM.
> 3. Create IRQ chip to forward HDMI LPE audio irqs.
> 
> HDMI LPE audio driver (a standalone sound driver) probes the
> LPE audio device and creates a new sound card.
> 
> Signed-off-by: Pierre-Louis Bossart 
> Signed-off-by: Jerome Anand 
> ---
>  drivers/gpu/drm/i915/Makefile  |   3 +
>  drivers/gpu/drm/i915/i915_drv.c|  13 +-
>  drivers/gpu/drm/i915/i915_drv.h|  19 ++
>  drivers/gpu/drm/i915/i915_irq.c|  14 ++
>  drivers/gpu/drm/i915/i915_reg.h|   3 +
>  drivers/gpu/drm/i915/intel_lpe_audio.c | 357 
> +
>  include/drm/intel_lpe_audio.h  |  45 +
>  7 files changed, 452 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_lpe_audio.c
>  create mode 100644 include/drm/intel_lpe_audio.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index e6fe004..11f9741 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -115,6 +115,9 @@ i915-y += intel_gvt.o
>  include $(src)/gvt/Makefile
>  endif
>  
> +# LPE Audio for VLV and CHT
> +i915-y += intel_lpe_audio.o
> +
>  obj-$(CONFIG_DRM_I915) += i915.o
>  
>  CFLAGS_i915_trace_points.o := -I$(src)
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 31b2b63..ab1e4768 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1141,7 +1141,13 @@ static void i915_driver_register(struct 
> drm_i915_private *dev_priv)
>   if (IS_GEN5(dev_priv))
>   intel_gpu_ips_init(dev_priv);
>  
> - i915_audio_component_init(dev_priv);
> + if (intel_lpe_audio_detect(dev_priv)) {
> + if (intel_lpe_audio_setup(dev_priv) < 0)
> + DRM_ERROR("failed to setup LPE Audio bridge\n");
> + }

I'd move all that into the lpe audio code. No need to have anything here
but a single function call IMO.

> +
> + if (!IS_LPE_AUDIO_ENABLED(dev_priv))

I don't like that too much. I think I would just make
that HAS_LPE_AUDIO(). The current IS_VLV||IS_CHV check can just be
inlined into the init function.

> + i915_audio_component_init(dev_priv);
>  
>   /*
>* Some ports require correctly set-up hpd registers for detection to
> @@ -1159,7 +1165,10 @@ static void i915_driver_register(struct 
> drm_i915_private *dev_priv)
>   */
>  static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  {
> - i915_audio_component_cleanup(dev_priv);
> + if (IS_LPE_AUDIO_ENABLED(dev_priv))
> + intel_lpe_audio_teardown(dev_priv);
> + else
> + i915_audio_component_cleanup(dev_priv);
>  
>   intel_gpu_ips_teardown();
>   acpi_video_unregister();
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 91ff3d7..399a8ee 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2087,6 +2087,12 @@ struct drm_i915_private {
>   /* Used to save the pipe-to-encoder mapping for audio */
>   struct intel_encoder *av_enc_map[I915_MAX_PIPES];
>  
> + /* necessary resource sharing with HDMI LPE audio driver. */
> + struct {
> + struct platform_device *platdev;
> + int irq;
> + } lpe_audio;
> +
>   /*
>* NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>* will be rejected. Instead look for a better place.
> @@ -2827,6 +2833,13 @@ struct drm_i915_cmd_table {
>  
>  #define HAS_POOLED_EU(dev)   (INTEL_INFO(dev)->has_pooled_eu)
>  
> +#define HAS_LPE_AUDIO(dev)   (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> +#define IS_LPE_AUDIO_ENABLED(dev_priv) \
> + (__I915__(dev_priv)->lpe_audio.platdev != NULL)
> +#define IS_LPE_AUDIO_IRQ_VALID(dev_priv) \
> + (__I915__(dev_priv)->lpe_audio.irq >= 0)

Seems useless. We should just not register the lpe audio thing if we
have no irq.

> +
> +
>  #define INTEL_PCH_DEVICE_ID_MASK 0xff00
>  #define INTEL_PCH_IBX_DEVICE_ID_TYPE 0x3b00
>  #define INTEL_PCH_CPT_DEVICE_ID_TYPE 0x1c00
> @@ -3579,6 +3592,12 @@ extern int i915_restore_state(struct drm_device *dev);
>  void i915_setup_sysfs(struct drm_i915_private *dev_priv);
>  void i915_teardown_sysfs(struct drm_i915_private *dev_priv);
>  
> +/* i915_lpe_audio.c */
> +int  intel_lpe_audio_setup(struct drm_i915_private *dev_priv);
> +void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv);
> +void