| Message ID | 1457146252-4577-5-git-send-email-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 04, 2016 at 08:50:41PM -0600, Pierre-Louis Bossart wrote: > Add header files for interface available on Baytrail and CherryTrail > > Initial code was downloaded from https://github.com/01org/baytrailaudio/ > ...and had the changes to .config stripped and the revert on sound/init.c > done by David Henningson > > Clean-up, port to 4.4 and intel-drm by Pierre Bossart > > Signed-off-by: David Henningsson <david.henningsson@canonical.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/gpu/drm/i915/hdmi_audio_if.h | 122 +++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 31 +++++++++ > drivers/gpu/drm/i915/i915_reg.h | 7 ++ > drivers/gpu/drm/i915/intel_drv.h | 11 ++++ > 4 files changed, 171 insertions(+) > create mode 100644 drivers/gpu/drm/i915/hdmi_audio_if.h > > diff --git a/drivers/gpu/drm/i915/hdmi_audio_if.h b/drivers/gpu/drm/i915/hdmi_audio_if.h > new file mode 100644 > index 0000000..f968028 > --- /dev/null > +++ b/drivers/gpu/drm/i915/hdmi_audio_if.h > @@ -0,0 +1,122 @@ > +/* > + * Copyright (c) 2010, Intel Corporation. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * Authors: > + * jim liu <jim.liu@intel.com> > + * Uma Shankar <uma.shankar@intel.com> > + */ > + > + > +#ifndef __HDMI_AUDIO_IF_H > +#define __HDMI_AUDIO_IF_H > + > +#include <linux/types.h> > +#include <drm/drmP.h> > + > +/* HDMI AUDIO INTERRUPT TYPE */ > +#define HDMI_AUDIO_UNDERRUN (1UL<<0) > +#define HDMI_AUDIO_BUFFER_DONE (1UL<<1) > + > +/* the monitor type HDMI or DVI */ > +#define MONITOR_TYPE_HDMI 1 > +#define MONITOR_TYPE_DVI 2 > + > +extern int i915_hdmi_state; > +extern int i915_notify_had; > + > +enum had_caps_list { > + HAD_GET_ELD = 1, > + HAD_GET_SAMPLING_FREQ, > + HAD_GET_DISPLAY_RATE, > + HAD_GET_HDCP_STATUS, > + HAD_GET_AUDIO_STATUS, > + HAD_SET_ENABLE_AUDIO, > + HAD_SET_DISABLE_AUDIO, > + HAD_SET_ENABLE_AUDIO_INT, > + HAD_SET_DISABLE_AUDIO_INT, > + OTHERS_TBD, > +}; > + > +enum had_event_type { > + HAD_EVENT_HOT_PLUG = 1, > + HAD_EVENT_HOT_UNPLUG, > + HAD_EVENT_MODE_CHANGING, > + HAD_EVENT_PM_CHANGING, > + HAD_EVENT_AUDIO_BUFFER_DONE, > + HAD_EVENT_AUDIO_BUFFER_UNDERRUN, > + HAD_EVENT_QUERY_IS_AUDIO_BUSY, > + HAD_EVENT_QUERY_IS_AUDIO_SUSPENDED, > +}; > + > +/* > + * HDMI Display Controller Audio Interface > + * > + */ > +typedef int (*had_event_call_back) (enum had_event_type event_type, > + void *ctxt_info); > + > +struct hdmi_audio_registers_ops { > + int (*hdmi_audio_read_register)(uint32_t reg_addr, uint32_t *data); > + int (*hdmi_audio_write_register)(uint32_t reg_addr, uint32_t data); > + int (*hdmi_audio_read_modify)(uint32_t reg_addr, uint32_t data, > + uint32_t mask); > +}; Do all the register actually live within the device 2 mmio bar? If so I think what we might want to do here is go for a proper subdevice approach like we tried to do for the VED on BYT. VED sort of dried up and no one cared (not sure the userspae part was even published), but I think the approach was sane. See here: https://lists.freedesktop.org/archives/intel-gfx/2015-January/058137.html > + > +struct hdmi_audio_query_set_ops { > + int (*hdmi_audio_get_caps)(enum had_caps_list query_element, > + void *capabilties); > + int (*hdmi_audio_set_caps)(enum had_caps_list set_element, > + void *capabilties); > +}; > + > +typedef struct hdmi_audio_event { > + int type; > +} hdmi_audio_event_t; > + > +struct snd_intel_had_interface { > + const char *name; > + int (*query)(void *had_data, hdmi_audio_event_t event); > + int (*suspend)(void *had_data, hdmi_audio_event_t event); > + int (*resume)(void *had_data); > +}; > + > +struct hdmi_audio_priv { > + struct drm_device *dev; > + u32 hdmib_reg; > + > + bool is_hdcp_supported; > + bool hdmi_hpd_connected; > + int monitor_type; > + void *context; > +}; > + > +extern void i915_hdmi_audio_init(struct hdmi_audio_priv *p_hdmi_priv); > + > +extern bool mid_hdmi_audio_is_busy(struct drm_device *dev); > +extern bool mid_hdmi_audio_suspend(struct drm_device *dev); > +extern void mid_hdmi_audio_resume(struct drm_device *dev); > +extern void mid_hdmi_audio_signal_event(struct drm_device *dev, > + enum had_event_type event); > + > +/* Added for HDMI Audio */ > +extern void hdmi_get_eld(uint8_t *eld); > +extern int i915_enable_hdmi_audio_int(struct drm_device *dev); > +extern int i915_disable_hdmi_audio_int(struct drm_device *dev); > +extern int mid_hdmi_audio_setup( > + had_event_call_back audio_callbacks, > + struct hdmi_audio_registers_ops *reg_ops, > + struct hdmi_audio_query_set_ops *query_ops); > +extern int mid_hdmi_audio_register( > + struct snd_intel_had_interface *driver, > + void *had_data); > + > +#endif /* __HDMI_AUDIO_IF_H */ > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2cb0a41..5dceb5b 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -53,6 +53,7 @@ > #include <linux/kref.h> > #include <linux/pm_qos.h> > #include "intel_guc.h" > +#include "hdmi_audio_if.h" > > /* General customization: > */ > @@ -1196,6 +1197,18 @@ struct intel_gen6_power_mgmt { > struct mutex hw_lock; > }; > > +/* Runtime power management related */ > +struct intel_gen7_rpm { > + /* To track (num of get calls - num of put calls) > + * made by procfs > + */ > + atomic_t procfs_count; > + /* To make sure ring get/put are in pair */ > + bool ring_active; > + struct proc_dir_entry *i915_proc_dir; > + struct proc_dir_entry *i915_proc_file; > +}; > + > /* defined intel_pm.c */ > extern spinlock_t mchdev_lock; > > @@ -2018,6 +2031,19 @@ struct drm_i915_private { > > struct intel_encoder *dig_port_map[I915_MAX_PORTS]; > > + /* Added for HDMI Audio */ > + had_event_call_back had_event_callbacks; > + struct snd_intel_had_interface *had_interface; > + void *had_pvt_data; > + int tmds_clock_speed; > + int hdmi_audio_interrupt_mask; > + struct work_struct hdmi_audio_wq; > + > + u32 hotplug_status; > + > + /* Runtime power management related */ > + struct intel_gen7_rpm rpm; > + > /* > * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch > * will be rejected. Instead look for a better place. > @@ -3537,6 +3563,11 @@ int intel_freq_opcode(struct drm_i915_private *dev_priv, int val); > } while (upper != old_upper && loop++ < 2); \ > (u64)upper << 32 | lower; }) > > +int i915_rpm_get_disp(struct drm_device *dev); > +int i915_rpm_put_disp(struct drm_device *dev); > + > +bool i915_is_device_active(struct drm_device *dev); > + > #define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg) > #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 228b22f..370371c 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -2062,7 +2062,12 @@ enum skl_disp_power_wells { > #define I915_WINVALID_INTERRUPT (1<<1) > #define I915_USER_INTERRUPT (1<<1) > #define I915_ASLE_INTERRUPT (1<<0) > +#define I915_LPE_AUDIO_HDMI_STATUS_A _MMIO(dev_priv->info.display_mmio_offset + 0x65064) > +#define I915_LPE_AUDIO_HDMI_STATUS_B _MMIO(dev_priv->info.display_mmio_offset + 0x65864) > +#define I915_HDMI_AUDIO_UNDERRUN (1UL<<31) > +#define I915_HDMI_AUDIO_BUFFER_DONE (1UL<<29) > #define I915_BSD_USER_INTERRUPT (1<<25) > +#define I915_HDMI_AUDIO_UNDERRUN_ENABLE (1UL<<15) > > #define GEN6_BSD_RNCID _MMIO(0x12198) > > @@ -3364,6 +3369,7 @@ enum skl_disp_power_wells { > #define _GEN3_SDVOC 0x61160 > #define GEN3_SDVOB _MMIO(_GEN3_SDVOB) > #define GEN3_SDVOC _MMIO(_GEN3_SDVOC) > +#define HDMIB (dev_priv->info.display_mmio_offset + 0x61140) > #define GEN4_HDMIB GEN3_SDVOB > #define GEN4_HDMIC GEN3_SDVOC > #define VLV_HDMIB _MMIO(VLV_DISPLAY_BASE + 0x61140) > @@ -3373,6 +3379,7 @@ enum skl_disp_power_wells { > #define PCH_HDMIB PCH_SDVOB > #define PCH_HDMIC _MMIO(0xe1150) > #define PCH_HDMID _MMIO(0xe1160) > +#define PORT_ENABLE (1 << 31) > > #define PORT_DFT_I9XX _MMIO(0x61150) > #define DC_BALANCE_RESET (1 << 25) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index cb413e2..adfd27c 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -724,6 +724,14 @@ struct cxsr_latency { > #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base) > #define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL) > > +/* HDMI bits are shared with the DP bits */ > +#define HDMIB_HOTPLUG_LIVE_STATUS (1 << 29) > +#define HDMIC_HOTPLUG_LIVE_STATUS (1 << 28) > +#define HDMID_HOTPLUG_LIVE_STATUS (1 << 27) > +#define HDMI_LIVE_STATUS_BASE 30 > +#define HDMI_LIVE_STATUS_DELAY_STEP 10 > +#define HDMI_EDID_RETRY_COUNT 3 > + > struct intel_hdmi { > i915_reg_t hdmi_reg; > int ddc_bus; > @@ -735,6 +743,9 @@ struct intel_hdmi { > bool rgb_quant_range_selectable; > enum hdmi_picture_aspect aspect_ratio; > struct intel_connector *attached_connector; > + struct edid *edid; > + uint32_t edid_mode_count; > + > void (*write_infoframe)(struct drm_encoder *encoder, > enum hdmi_infoframe_type type, > const void *frame, ssize_t len); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Mar 04, 2016 at 08:50:41PM -0600, Pierre-Louis Bossart wrote: > Add header files for interface available on Baytrail and CherryTrail > > Initial code was downloaded from https://github.com/01org/baytrailaudio/ > ...and had the changes to .config stripped and the revert on sound/init.c > done by David Henningson > > Clean-up, port to 4.4 and intel-drm by Pierre Bossart > > Signed-off-by: David Henningsson <david.henningsson@canonical.com> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/gpu/drm/i915/hdmi_audio_if.h | 122 +++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 31 +++++++++ > drivers/gpu/drm/i915/i915_reg.h | 7 ++ > drivers/gpu/drm/i915/intel_drv.h | 11 ++++ > 4 files changed, 171 insertions(+) > create mode 100644 drivers/gpu/drm/i915/hdmi_audio_if.h > > diff --git a/drivers/gpu/drm/i915/hdmi_audio_if.h b/drivers/gpu/drm/i915/hdmi_audio_if.h > new file mode 100644 > index 0000000..f968028 > --- /dev/null > +++ b/drivers/gpu/drm/i915/hdmi_audio_if.h > @@ -0,0 +1,122 @@ > +/* > + * Copyright (c) 2010, Intel Corporation. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * Authors: > + * jim liu <jim.liu@intel.com> > + * Uma Shankar <uma.shankar@intel.com> > + */ > + > + > +#ifndef __HDMI_AUDIO_IF_H > +#define __HDMI_AUDIO_IF_H > + > +#include <linux/types.h> > +#include <drm/drmP.h> > + > +/* HDMI AUDIO INTERRUPT TYPE */ > +#define HDMI_AUDIO_UNDERRUN (1UL<<0) > +#define HDMI_AUDIO_BUFFER_DONE (1UL<<1) > + > +/* the monitor type HDMI or DVI */ > +#define MONITOR_TYPE_HDMI 1 > +#define MONITOR_TYPE_DVI 2 > + > +extern int i915_hdmi_state; > +extern int i915_notify_had; > + > +enum had_caps_list { > + HAD_GET_ELD = 1, > + HAD_GET_SAMPLING_FREQ, > + HAD_GET_DISPLAY_RATE, > + HAD_GET_HDCP_STATUS, > + HAD_GET_AUDIO_STATUS, > + HAD_SET_ENABLE_AUDIO, > + HAD_SET_DISABLE_AUDIO, > + HAD_SET_ENABLE_AUDIO_INT, > + HAD_SET_DISABLE_AUDIO_INT, > + OTHERS_TBD, > +}; > + > +enum had_event_type { > + HAD_EVENT_HOT_PLUG = 1, > + HAD_EVENT_HOT_UNPLUG, > + HAD_EVENT_MODE_CHANGING, > + HAD_EVENT_PM_CHANGING, > + HAD_EVENT_AUDIO_BUFFER_DONE, > + HAD_EVENT_AUDIO_BUFFER_UNDERRUN, > + HAD_EVENT_QUERY_IS_AUDIO_BUSY, > + HAD_EVENT_QUERY_IS_AUDIO_SUSPENDED, Kinda hard to see where everything gets used due to the way the patches are split up. At least the hotplug/mode change events are not needed. We only have the two points where i915 should inform the audio driver about this stuff, and those are the intel_audio_code_enable/disable(). For that we already have the .pin_eld_notify() hook. The interrupt stuff should mostly vanish from i915 with the subdevice approach. As in i915 would just call the interrupt handler of the audio driver based on the LPE bits in IIR, and the audio driver can then do whatever it wants based on its own status register. > +}; > + > +/* > + * HDMI Display Controller Audio Interface > + * > + */ > +typedef int (*had_event_call_back) (enum had_event_type event_type, > + void *ctxt_info); > + > +struct hdmi_audio_registers_ops { > + int (*hdmi_audio_read_register)(uint32_t reg_addr, uint32_t *data); > + int (*hdmi_audio_write_register)(uint32_t reg_addr, uint32_t data); > + int (*hdmi_audio_read_modify)(uint32_t reg_addr, uint32_t data, > + uint32_t mask); > +}; > + > +struct hdmi_audio_query_set_ops { > + int (*hdmi_audio_get_caps)(enum had_caps_list query_element, > + void *capabilties); > + int (*hdmi_audio_set_caps)(enum had_caps_list set_element, > + void *capabilties); > +}; > + > +typedef struct hdmi_audio_event { > + int type; > +} hdmi_audio_event_t; > + > +struct snd_intel_had_interface { > + const char *name; > + int (*query)(void *had_data, hdmi_audio_event_t event); > + int (*suspend)(void *had_data, hdmi_audio_event_t event); > + int (*resume)(void *had_data); > +}; > + > +struct hdmi_audio_priv { > + struct drm_device *dev; > + u32 hdmib_reg; > + > + bool is_hdcp_supported; > + bool hdmi_hpd_connected; > + int monitor_type; > + void *context; > +}; > + > +extern void i915_hdmi_audio_init(struct hdmi_audio_priv *p_hdmi_priv); > + > +extern bool mid_hdmi_audio_is_busy(struct drm_device *dev); > +extern bool mid_hdmi_audio_suspend(struct drm_device *dev); > +extern void mid_hdmi_audio_resume(struct drm_device *dev); > +extern void mid_hdmi_audio_signal_event(struct drm_device *dev, > + enum had_event_type event); > + > +/* Added for HDMI Audio */ > +extern void hdmi_get_eld(uint8_t *eld); > +extern int i915_enable_hdmi_audio_int(struct drm_device *dev); > +extern int i915_disable_hdmi_audio_int(struct drm_device *dev); > +extern int mid_hdmi_audio_setup( > + had_event_call_back audio_callbacks, > + struct hdmi_audio_registers_ops *reg_ops, > + struct hdmi_audio_query_set_ops *query_ops); > +extern int mid_hdmi_audio_register( > + struct snd_intel_had_interface *driver, > + void *had_data); > + > +#endif /* __HDMI_AUDIO_IF_H */ > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2cb0a41..5dceb5b 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -53,6 +53,7 @@ > #include <linux/kref.h> > #include <linux/pm_qos.h> > #include "intel_guc.h" > +#include "hdmi_audio_if.h" > > /* General customization: > */ > @@ -1196,6 +1197,18 @@ struct intel_gen6_power_mgmt { > struct mutex hw_lock; > }; > > +/* Runtime power management related */ > +struct intel_gen7_rpm { > + /* To track (num of get calls - num of put calls) > + * made by procfs > + */ > + atomic_t procfs_count; > + /* To make sure ring get/put are in pair */ > + bool ring_active; > + struct proc_dir_entry *i915_proc_dir; > + struct proc_dir_entry *i915_proc_file; > +}; > + > /* defined intel_pm.c */ > extern spinlock_t mchdev_lock; > > @@ -2018,6 +2031,19 @@ struct drm_i915_private { > > struct intel_encoder *dig_port_map[I915_MAX_PORTS]; > > + /* Added for HDMI Audio */ > + had_event_call_back had_event_callbacks; > + struct snd_intel_had_interface *had_interface; > + void *had_pvt_data; > + int tmds_clock_speed; > + int hdmi_audio_interrupt_mask; > + struct work_struct hdmi_audio_wq; > + > + u32 hotplug_status; > + > + /* Runtime power management related */ > + struct intel_gen7_rpm rpm; > + > /* > * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch > * will be rejected. Instead look for a better place. > @@ -3537,6 +3563,11 @@ int intel_freq_opcode(struct drm_i915_private *dev_priv, int val); > } while (upper != old_upper && loop++ < 2); \ > (u64)upper << 32 | lower; }) > > +int i915_rpm_get_disp(struct drm_device *dev); > +int i915_rpm_put_disp(struct drm_device *dev); > + > +bool i915_is_device_active(struct drm_device *dev); > + > #define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg) > #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 228b22f..370371c 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -2062,7 +2062,12 @@ enum skl_disp_power_wells { > #define I915_WINVALID_INTERRUPT (1<<1) > #define I915_USER_INTERRUPT (1<<1) > #define I915_ASLE_INTERRUPT (1<<0) > +#define I915_LPE_AUDIO_HDMI_STATUS_A _MMIO(dev_priv->info.display_mmio_offset + 0x65064) > +#define I915_LPE_AUDIO_HDMI_STATUS_B _MMIO(dev_priv->info.display_mmio_offset + 0x65864) > +#define I915_HDMI_AUDIO_UNDERRUN (1UL<<31) > +#define I915_HDMI_AUDIO_BUFFER_DONE (1UL<<29) > #define I915_BSD_USER_INTERRUPT (1<<25) > +#define I915_HDMI_AUDIO_UNDERRUN_ENABLE (1UL<<15) > > #define GEN6_BSD_RNCID _MMIO(0x12198) > > @@ -3364,6 +3369,7 @@ enum skl_disp_power_wells { > #define _GEN3_SDVOC 0x61160 > #define GEN3_SDVOB _MMIO(_GEN3_SDVOB) > #define GEN3_SDVOC _MMIO(_GEN3_SDVOC) > +#define HDMIB (dev_priv->info.display_mmio_offset + 0x61140) > #define GEN4_HDMIB GEN3_SDVOB > #define GEN4_HDMIC GEN3_SDVOC > #define VLV_HDMIB _MMIO(VLV_DISPLAY_BASE + 0x61140) > @@ -3373,6 +3379,7 @@ enum skl_disp_power_wells { > #define PCH_HDMIB PCH_SDVOB > #define PCH_HDMIC _MMIO(0xe1150) > #define PCH_HDMID _MMIO(0xe1160) > +#define PORT_ENABLE (1 << 31) > > #define PORT_DFT_I9XX _MMIO(0x61150) > #define DC_BALANCE_RESET (1 << 25) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index cb413e2..adfd27c 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -724,6 +724,14 @@ struct cxsr_latency { > #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base) > #define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL) > > +/* HDMI bits are shared with the DP bits */ > +#define HDMIB_HOTPLUG_LIVE_STATUS (1 << 29) > +#define HDMIC_HOTPLUG_LIVE_STATUS (1 << 28) > +#define HDMID_HOTPLUG_LIVE_STATUS (1 << 27) > +#define HDMI_LIVE_STATUS_BASE 30 > +#define HDMI_LIVE_STATUS_DELAY_STEP 10 > +#define HDMI_EDID_RETRY_COUNT 3 > + > struct intel_hdmi { > i915_reg_t hdmi_reg; > int ddc_bus; > @@ -735,6 +743,9 @@ struct intel_hdmi { > bool rgb_quant_range_selectable; > enum hdmi_picture_aspect aspect_ratio; > struct intel_connector *attached_connector; > + struct edid *edid; > + uint32_t edid_mode_count; > + > void (*write_infoframe)(struct drm_encoder *encoder, > enum hdmi_infoframe_type type, > const void *frame, ssize_t len); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Thanks for the review Ville [snip] > Kinda hard to see where everything gets used due to the way the patches > are split up. Yes, it's far from great... > At least the hotplug/mode change events are not needed. We only have the > two points where i915 should inform the audio driver about this stuff, > and those are the intel_audio_code_enable/disable(). For that we > already have the .pin_eld_notify() hook. > > The interrupt stuff should mostly vanish from i915 with the subdevice > approach. As in i915 would just call the interrupt handler of the audio > driver based on the LPE bits in IIR, and the audio driver can then do > whatever it wants based on its own status register. Are you saying that the subdevice would provide a read/write interface for the audio driver to look at display registers, and the i915 driver would only provide a notification interface (EDID and interrupts) to the audio driver? If yes, would there be two component framework links, one between i915/audio driver and one between subdevice/audio driver. I am way beyond my comfort zone, bear with me if this is silly. Thanks.
On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > Thanks for the review Ville > > [snip] > > > Kinda hard to see where everything gets used due to the way the patches > > are split up. > > Yes, it's far from great... > > > At least the hotplug/mode change events are not needed. We only have the > > two points where i915 should inform the audio driver about this stuff, > > and those are the intel_audio_code_enable/disable(). For that we > > already have the .pin_eld_notify() hook. > > > > The interrupt stuff should mostly vanish from i915 with the subdevice > > approach. As in i915 would just call the interrupt handler of the audio > > driver based on the LPE bits in IIR, and the audio driver can then do > > whatever it wants based on its own status register. > > Are you saying that the subdevice would provide a read/write interface > for the audio driver to look at display registers, and the i915 driver > would only provide a notification interface (EDID and interrupts) to the > audio driver? The audio driver would simply ioremap the appropriate range of registers itself. > If yes, would there be two component framework links, one between > i915/audio driver and one between subdevice/audio driver. Yeah sort of. i915 registers the platform device for the audio, the audio driver can then bind to the device via the platform driver .probe callback. It can then register with the audio component stuff at some point to get the relevant notifications on the display state. When i915 gets unloaded we remove the platform device, at which point the audio driver's platform driver .remove() callback gets invoked and it should unregister/cleanup everything. I just tried to frob around with the VED code a bit, and got it to load at least. It's not quite happy about reloading i915 while the ipvr driver was loaded though. Not sure what's going on there, but I do think this approach should work. So the VED patch could serve as a decent enough model to follow.
On Fri, Mar 11, 2016 at 09:09:12PM +0200, Ville Syrjälä wrote: > On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > > Thanks for the review Ville > > > > [snip] > > > > > Kinda hard to see where everything gets used due to the way the patches > > > are split up. > > > > Yes, it's far from great... > > > > > At least the hotplug/mode change events are not needed. We only have the > > > two points where i915 should inform the audio driver about this stuff, > > > and those are the intel_audio_code_enable/disable(). For that we > > > already have the .pin_eld_notify() hook. > > > > > > The interrupt stuff should mostly vanish from i915 with the subdevice > > > approach. As in i915 would just call the interrupt handler of the audio > > > driver based on the LPE bits in IIR, and the audio driver can then do > > > whatever it wants based on its own status register. > > > > Are you saying that the subdevice would provide a read/write interface > > for the audio driver to look at display registers, and the i915 driver > > would only provide a notification interface (EDID and interrupts) to the > > audio driver? > > The audio driver would simply ioremap the appropriate range of > registers itself. > > > If yes, would there be two component framework links, one between > > i915/audio driver and one between subdevice/audio driver. > > Yeah sort of. i915 registers the platform device for the audio, the > audio driver can then bind to the device via the platform driver .probe > callback. It can then register with the audio component stuff at some > point to get the relevant notifications on the display state. When > i915 gets unloaded we remove the platform device, at which point the > audio driver's platform driver .remove() callback gets invoked and > it should unregister/cleanup everything. > > I just tried to frob around with the VED code a bit, and got it to load > at least. It's not quite happy about reloading i915 while the ipvr > driver was loaded though. Not sure what's going on there, but I do > think this approach should work. So the VED patch could serve as a > decent enough model to follow. platform devices registerd by modules are apparently inherently racy and in an unfixable way. At least I remember something like that from VED discussion. In short you _must_ unload VED manually before unloading i915, or it all goes boom. If this is the only thing that went boom it's acceptable. Another bit we didn't fully do for VED is abstracting away the dma mapping stuff, because x86 dma abstraction sucks (compared to arm). Not sure, but this might have been fixed meanwhile - if we can set up a dma_ops that the subdevice would use, we should do so (instead of the page_to_pfn hacks VED used). -Daniel
On Mon, Mar 14, 2016 at 10:04:00AM +0100, Daniel Vetter wrote: > On Fri, Mar 11, 2016 at 09:09:12PM +0200, Ville Syrjälä wrote: > > On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > > > Thanks for the review Ville > > > > > > [snip] > > > > > > > Kinda hard to see where everything gets used due to the way the patches > > > > are split up. > > > > > > Yes, it's far from great... > > > > > > > At least the hotplug/mode change events are not needed. We only have the > > > > two points where i915 should inform the audio driver about this stuff, > > > > and those are the intel_audio_code_enable/disable(). For that we > > > > already have the .pin_eld_notify() hook. > > > > > > > > The interrupt stuff should mostly vanish from i915 with the subdevice > > > > approach. As in i915 would just call the interrupt handler of the audio > > > > driver based on the LPE bits in IIR, and the audio driver can then do > > > > whatever it wants based on its own status register. > > > > > > Are you saying that the subdevice would provide a read/write interface > > > for the audio driver to look at display registers, and the i915 driver > > > would only provide a notification interface (EDID and interrupts) to the > > > audio driver? > > > > The audio driver would simply ioremap the appropriate range of > > registers itself. > > > > > If yes, would there be two component framework links, one between > > > i915/audio driver and one between subdevice/audio driver. > > > > Yeah sort of. i915 registers the platform device for the audio, the > > audio driver can then bind to the device via the platform driver .probe > > callback. It can then register with the audio component stuff at some > > point to get the relevant notifications on the display state. When > > i915 gets unloaded we remove the platform device, at which point the > > audio driver's platform driver .remove() callback gets invoked and > > it should unregister/cleanup everything. > > > > I just tried to frob around with the VED code a bit, and got it to load > > at least. It's not quite happy about reloading i915 while the ipvr > > driver was loaded though. Not sure what's going on there, but I do > > think this approach should work. So the VED patch could serve as a > > decent enough model to follow. > > platform devices registerd by modules are apparently inherently racy and > in an unfixable way. At least I remember something like that from VED > discussion. > > In short you _must_ unload VED manually before unloading i915, or it all > goes boom. If this is the only thing that went boom it's acceptable. VED goes boom due drm_global_mutex deadlock at least if you load i915 while ipvr is already loaded. I didn't check to hard if there were any booms on pure unload so far. Anyways, I was a bit worried about the races so I did a pair of small test modules to try out this model, and to me it looked to work so far. I just unregistered the platform device from the parent pci driver .remove() hook, and it got blocked until the platform driver's .remove() hook was done. In any case if this is broken, then I assume mfd must be broken. And that thing is at least used quite extensively.
On 3/11/16 1:09 PM, Ville Syrjälä wrote: > On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: >> Thanks for the review Ville >> >> [snip] >> >>> Kinda hard to see where everything gets used due to the way the patches >>> are split up. >> >> Yes, it's far from great... >> >>> At least the hotplug/mode change events are not needed. We only have the >>> two points where i915 should inform the audio driver about this stuff, >>> and those are the intel_audio_code_enable/disable(). For that we >>> already have the .pin_eld_notify() hook. >>> >>> The interrupt stuff should mostly vanish from i915 with the subdevice >>> approach. As in i915 would just call the interrupt handler of the audio >>> driver based on the LPE bits in IIR, and the audio driver can then do >>> whatever it wants based on its own status register. >> >> Are you saying that the subdevice would provide a read/write interface >> for the audio driver to look at display registers, and the i915 driver >> would only provide a notification interface (EDID and interrupts) to the >> audio driver? > > The audio driver would simply ioremap the appropriate range of > registers itself. > >> If yes, would there be two component framework links, one between >> i915/audio driver and one between subdevice/audio driver. > > Yeah sort of. i915 registers the platform device for the audio, the > audio driver can then bind to the device via the platform driver .probe > callback. It can then register with the audio component stuff at some > point to get the relevant notifications on the display state. When > i915 gets unloaded we remove the platform device, at which point the > audio driver's platform driver .remove() callback gets invoked and > it should unregister/cleanup everything. we don't want to expose this interface when HDAudio is present and enabled so we would need to add a test for this. Also it looks like you want the creation of the platform device done in i915, we were thinking of doing this as part of the audio drivers but in the end this looks equivalent. In both cases we would end-up ignoring the HAD022A8 HID and not use acpi for this extension > I just tried to frob around with the VED code a bit, and got it to load > at least. It's not quite happy about reloading i915 while the ipvr > driver was loaded though. Not sure what's going on there, but I do > think this approach should work. So the VED patch could serve as a > decent enough model to follow. >
On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: > On 3/11/16 1:09 PM, Ville Syrjälä wrote: > > On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > >> Thanks for the review Ville > >> > >> [snip] > >> > >>> Kinda hard to see where everything gets used due to the way the patches > >>> are split up. > >> > >> Yes, it's far from great... > >> > >>> At least the hotplug/mode change events are not needed. We only have the > >>> two points where i915 should inform the audio driver about this stuff, > >>> and those are the intel_audio_code_enable/disable(). For that we > >>> already have the .pin_eld_notify() hook. > >>> > >>> The interrupt stuff should mostly vanish from i915 with the subdevice > >>> approach. As in i915 would just call the interrupt handler of the audio > >>> driver based on the LPE bits in IIR, and the audio driver can then do > >>> whatever it wants based on its own status register. > >> > >> Are you saying that the subdevice would provide a read/write interface > >> for the audio driver to look at display registers, and the i915 driver > >> would only provide a notification interface (EDID and interrupts) to the > >> audio driver? > > > > The audio driver would simply ioremap the appropriate range of > > registers itself. > > > >> If yes, would there be two component framework links, one between > >> i915/audio driver and one between subdevice/audio driver. > > > > Yeah sort of. i915 registers the platform device for the audio, the > > audio driver can then bind to the device via the platform driver .probe > > callback. It can then register with the audio component stuff at some > > point to get the relevant notifications on the display state. When > > i915 gets unloaded we remove the platform device, at which point the > > audio driver's platform driver .remove() callback gets invoked and > > it should unregister/cleanup everything. > > we don't want to expose this interface when HDAudio is present and > enabled so we would need to add a test for this. > Also it looks like you want the creation of the platform device done in > i915, we were thinking of doing this as part of the audio drivers but in > the end this looks equivalent. In both cases we would end-up ignoring > the HAD022A8 HID and not use acpi for this extension Well, if you have a device you can hang off from then i915 should need to register it I suppose. Though that would make the interrupt forwarding perhaps less nice. There's also the suspend/resume order dependency to deal with if there's no parent/child relationship between the devices. > > > I just tried to frob around with the VED code a bit, and got it to load > > at least. It's not quite happy about reloading i915 while the ipvr > > driver was loaded though. Not sure what's going on there, but I do > > think this approach should work. So the VED patch could serve as a > > decent enough model to follow. > >
On Mon, Mar 14, 2016 at 05:21:54PM +0200, Ville Syrjälä wrote: > On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: > > On 3/11/16 1:09 PM, Ville Syrjälä wrote: > > > On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > > >> Thanks for the review Ville > > >> > > >> [snip] > > >> > > >>> Kinda hard to see where everything gets used due to the way the patches > > >>> are split up. > > >> > > >> Yes, it's far from great... > > >> > > >>> At least the hotplug/mode change events are not needed. We only have the > > >>> two points where i915 should inform the audio driver about this stuff, > > >>> and those are the intel_audio_code_enable/disable(). For that we > > >>> already have the .pin_eld_notify() hook. > > >>> > > >>> The interrupt stuff should mostly vanish from i915 with the subdevice > > >>> approach. As in i915 would just call the interrupt handler of the audio > > >>> driver based on the LPE bits in IIR, and the audio driver can then do > > >>> whatever it wants based on its own status register. > > >> > > >> Are you saying that the subdevice would provide a read/write interface > > >> for the audio driver to look at display registers, and the i915 driver > > >> would only provide a notification interface (EDID and interrupts) to the > > >> audio driver? > > > > > > The audio driver would simply ioremap the appropriate range of > > > registers itself. > > > > > >> If yes, would there be two component framework links, one between > > >> i915/audio driver and one between subdevice/audio driver. > > > > > > Yeah sort of. i915 registers the platform device for the audio, the > > > audio driver can then bind to the device via the platform driver .probe > > > callback. It can then register with the audio component stuff at some > > > point to get the relevant notifications on the display state. When > > > i915 gets unloaded we remove the platform device, at which point the > > > audio driver's platform driver .remove() callback gets invoked and > > > it should unregister/cleanup everything. > > > > we don't want to expose this interface when HDAudio is present and > > enabled so we would need to add a test for this. > > Also it looks like you want the creation of the platform device done in > > i915, we were thinking of doing this as part of the audio drivers but in > > the end this looks equivalent. In both cases we would end-up ignoring > > the HAD022A8 HID and not use acpi for this extension > > Well, if you have a device you can hang off from then i915 should need > to register it I suppose. Though that would make the interrupt > forwarding perhaps less nice. There's also the suspend/resume order > dependency to deal with if there's no parent/child relationship between > the devices. Oh and I suppose you'd end up ioremapping part of the pci dev2 mmio bar to get at the registers, which would look a bit funkly at least. > > > > > > I just tried to frob around with the VED code a bit, and got it to load > > > at least. It's not quite happy about reloading i915 while the ipvr > > > driver was loaded though. Not sure what's going on there, but I do > > > think this approach should work. So the VED patch could serve as a > > > decent enough model to follow. > > > > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 3/14/16 10:30 AM, Ville Syrjälä wrote: > On Mon, Mar 14, 2016 at 05:21:54PM +0200, Ville Syrjälä wrote: >> On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: >>> On 3/11/16 1:09 PM, Ville Syrjälä wrote: >>>> On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: >>>>> Thanks for the review Ville >>>>> >>>>> [snip] >>>>> >>>>>> Kinda hard to see where everything gets used due to the way the patches >>>>>> are split up. >>>>> >>>>> Yes, it's far from great... >>>>> >>>>>> At least the hotplug/mode change events are not needed. We only have the >>>>>> two points where i915 should inform the audio driver about this stuff, >>>>>> and those are the intel_audio_code_enable/disable(). For that we >>>>>> already have the .pin_eld_notify() hook. >>>>>> >>>>>> The interrupt stuff should mostly vanish from i915 with the subdevice >>>>>> approach. As in i915 would just call the interrupt handler of the audio >>>>>> driver based on the LPE bits in IIR, and the audio driver can then do >>>>>> whatever it wants based on its own status register. >>>>> >>>>> Are you saying that the subdevice would provide a read/write interface >>>>> for the audio driver to look at display registers, and the i915 driver >>>>> would only provide a notification interface (EDID and interrupts) to the >>>>> audio driver? >>>> >>>> The audio driver would simply ioremap the appropriate range of >>>> registers itself. >>>> >>>>> If yes, would there be two component framework links, one between >>>>> i915/audio driver and one between subdevice/audio driver. >>>> >>>> Yeah sort of. i915 registers the platform device for the audio, the >>>> audio driver can then bind to the device via the platform driver .probe >>>> callback. It can then register with the audio component stuff at some >>>> point to get the relevant notifications on the display state. When >>>> i915 gets unloaded we remove the platform device, at which point the >>>> audio driver's platform driver .remove() callback gets invoked and >>>> it should unregister/cleanup everything. >>> >>> we don't want to expose this interface when HDAudio is present and >>> enabled so we would need to add a test for this. >>> Also it looks like you want the creation of the platform device done in >>> i915, we were thinking of doing this as part of the audio drivers but in >>> the end this looks equivalent. In both cases we would end-up ignoring >>> the HAD022A8 HID and not use acpi for this extension >> >> Well, if you have a device you can hang off from then i915 should need >> to register it I suppose. Though that would make the interrupt >> forwarding perhaps less nice. There's also the suspend/resume order >> dependency to deal with if there's no parent/child relationship between >> the devices. > > Oh and I suppose you'd end up ioremapping part of the pci dev2 mmio bar > to get at the registers, which would look a bit funkly at least. 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?
On Mon, Mar 14, 2016 at 04:20:19PM +0200, Ville Syrjälä wrote: > On Mon, Mar 14, 2016 at 10:04:00AM +0100, Daniel Vetter wrote: > > On Fri, Mar 11, 2016 at 09:09:12PM +0200, Ville Syrjälä wrote: > > > On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > > > > Thanks for the review Ville > > > > > > > > [snip] > > > > > > > > > Kinda hard to see where everything gets used due to the way the patches > > > > > are split up. > > > > > > > > Yes, it's far from great... > > > > > > > > > At least the hotplug/mode change events are not needed. We only have the > > > > > two points where i915 should inform the audio driver about this stuff, > > > > > and those are the intel_audio_code_enable/disable(). For that we > > > > > already have the .pin_eld_notify() hook. > > > > > > > > > > The interrupt stuff should mostly vanish from i915 with the subdevice > > > > > approach. As in i915 would just call the interrupt handler of the audio > > > > > driver based on the LPE bits in IIR, and the audio driver can then do > > > > > whatever it wants based on its own status register. > > > > > > > > Are you saying that the subdevice would provide a read/write interface > > > > for the audio driver to look at display registers, and the i915 driver > > > > would only provide a notification interface (EDID and interrupts) to the > > > > audio driver? > > > > > > The audio driver would simply ioremap the appropriate range of > > > registers itself. > > > > > > > If yes, would there be two component framework links, one between > > > > i915/audio driver and one between subdevice/audio driver. > > > > > > Yeah sort of. i915 registers the platform device for the audio, the > > > audio driver can then bind to the device via the platform driver .probe > > > callback. It can then register with the audio component stuff at some > > > point to get the relevant notifications on the display state. When > > > i915 gets unloaded we remove the platform device, at which point the > > > audio driver's platform driver .remove() callback gets invoked and > > > it should unregister/cleanup everything. > > > > > > I just tried to frob around with the VED code a bit, and got it to load > > > at least. It's not quite happy about reloading i915 while the ipvr > > > driver was loaded though. Not sure what's going on there, but I do > > > think this approach should work. So the VED patch could serve as a > > > decent enough model to follow. > > > > platform devices registerd by modules are apparently inherently racy and > > in an unfixable way. At least I remember something like that from VED > > discussion. > > > > In short you _must_ unload VED manually before unloading i915, or it all > > goes boom. If this is the only thing that went boom it's acceptable. > > VED goes boom due drm_global_mutex deadlock at least if you load > i915 while ipvr is already loaded. I didn't check to hard if there > were any booms on pure unload so far. Oh right another boom that happened, but can be avoided by dropping the ->load callback and directly calling drm_dev_alloc/register. Shouldn't be a problem with a non-drm driver though. > Anyways, I was a bit worried about the races so I did a pair of > small test modules to try out this model, and to me it looked to > work so far. I just unregistered the platform device from the parent > pci driver .remove() hook, and it got blocked until the platform > driver's .remove() hook was done. > > In any case if this is broken, then I assume mfd must be broken. And > that thing is at least used quite extensively. Hm, I don't remember the exact details, but iirc the problem was that the struct device is refcounted, but you can't add a module ref for the vtable is has (specifically the final release method) since that would result in a ref-loop. Usually it works, but if someone keeps the device alive (through sysfs or whatever) and you manage to unload everything before that last ref gets dropped it goes boom. So "works", but not in a safe way. -Daniel
On Mon, Mar 14, 2016 at 10:04:00AM +0100, Daniel Vetter wrote: > On Fri, Mar 11, 2016 at 09:09:12PM +0200, Ville Syrjälä wrote: > > On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > > > Thanks for the review Ville > > > > > > [snip] > > > > > > > Kinda hard to see where everything gets used due to the way the patches > > > > are split up. > > > > > > Yes, it's far from great... > > > > > > > At least the hotplug/mode change events are not needed. We only have the > > > > two points where i915 should inform the audio driver about this stuff, > > > > and those are the intel_audio_code_enable/disable(). For that we > > > > already have the .pin_eld_notify() hook. > > > > > > > > The interrupt stuff should mostly vanish from i915 with the subdevice > > > > approach. As in i915 would just call the interrupt handler of the audio > > > > driver based on the LPE bits in IIR, and the audio driver can then do > > > > whatever it wants based on its own status register. > > > > > > Are you saying that the subdevice would provide a read/write interface > > > for the audio driver to look at display registers, and the i915 driver > > > would only provide a notification interface (EDID and interrupts) to the > > > audio driver? > > > > The audio driver would simply ioremap the appropriate range of > > registers itself. > > > > > If yes, would there be two component framework links, one between > > > i915/audio driver and one between subdevice/audio driver. > > > > Yeah sort of. i915 registers the platform device for the audio, the > > audio driver can then bind to the device via the platform driver .probe > > callback. It can then register with the audio component stuff at some > > point to get the relevant notifications on the display state. When > > i915 gets unloaded we remove the platform device, at which point the > > audio driver's platform driver .remove() callback gets invoked and > > it should unregister/cleanup everything. > > > > I just tried to frob around with the VED code a bit, and got it to load > > at least. It's not quite happy about reloading i915 while the ipvr > > driver was loaded though. Not sure what's going on there, but I do > > think this approach should work. So the VED patch could serve as a > > decent enough model to follow. > > platform devices registerd by modules are apparently inherently racy and > in an unfixable way. At least I remember something like that from VED > discussion. > > In short you _must_ unload VED manually before unloading i915, or it all > goes boom. If this is the only thing that went boom it's acceptable. > > Another bit we didn't fully do for VED is abstracting away the dma mapping > stuff, because x86 dma abstraction sucks (compared to arm). Not sure, but > this might have been fixed meanwhile - if we can set up a dma_ops that the > subdevice would use, we should do so (instead of the page_to_pfn hacks VED > used). This one might be a bit a problem - on byt we got away with pfn_to_page because no iommu at all, but that's not a good idea really. Definitely need to reevaluate this again. Iirc there's been some talk of just walking up a chain of platform devices until the core x86 dma code finds something with dma support, then use that. -Daniel
On Mon, Mar 14, 2016 at 12:27:51PM -0500, Pierre-Louis Bossart wrote: > On 3/14/16 10:30 AM, Ville Syrjälä wrote: > >On Mon, Mar 14, 2016 at 05:21:54PM +0200, Ville Syrjälä wrote: > >>On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: > >>>On 3/11/16 1:09 PM, Ville Syrjälä wrote: > >>>>On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > >>>>>Thanks for the review Ville > >>>>> > >>>>>[snip] > >>>>> > >>>>>>Kinda hard to see where everything gets used due to the way the patches > >>>>>>are split up. > >>>>> > >>>>>Yes, it's far from great... > >>>>> > >>>>>>At least the hotplug/mode change events are not needed. We only have the > >>>>>>two points where i915 should inform the audio driver about this stuff, > >>>>>>and those are the intel_audio_code_enable/disable(). For that we > >>>>>>already have the .pin_eld_notify() hook. > >>>>>> > >>>>>>The interrupt stuff should mostly vanish from i915 with the subdevice > >>>>>>approach. As in i915 would just call the interrupt handler of the audio > >>>>>>driver based on the LPE bits in IIR, and the audio driver can then do > >>>>>>whatever it wants based on its own status register. > >>>>> > >>>>>Are you saying that the subdevice would provide a read/write interface > >>>>>for the audio driver to look at display registers, and the i915 driver > >>>>>would only provide a notification interface (EDID and interrupts) to the > >>>>>audio driver? > >>>> > >>>>The audio driver would simply ioremap the appropriate range of > >>>>registers itself. > >>>> > >>>>>If yes, would there be two component framework links, one between > >>>>>i915/audio driver and one between subdevice/audio driver. > >>>> > >>>>Yeah sort of. i915 registers the platform device for the audio, the > >>>>audio driver can then bind to the device via the platform driver .probe > >>>>callback. It can then register with the audio component stuff at some > >>>>point to get the relevant notifications on the display state. When > >>>>i915 gets unloaded we remove the platform device, at which point the > >>>>audio driver's platform driver .remove() callback gets invoked and > >>>>it should unregister/cleanup everything. > >>> > >>>we don't want to expose this interface when HDAudio is present and > >>>enabled so we would need to add a test for this. > >>>Also it looks like you want the creation of the platform device done in > >>>i915, we were thinking of doing this as part of the audio drivers but in > >>>the end this looks equivalent. In both cases we would end-up ignoring > >>>the HAD022A8 HID and not use acpi for this extension > >> > >>Well, if you have a device you can hang off from then i915 should need > >>to register it I suppose. Though that would make the interrupt > >>forwarding perhaps less nice. There's also the suspend/resume order > >>dependency to deal with if there's no parent/child relationship between > >>the devices. > > > >Oh and I suppose you'd end up ioremapping part of the pci dev2 mmio bar > >to get at the registers, which would look a bit funkly at least. > > 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. -Daniel
On Tue, Mar 15, 2016 at 09:35:26AM +0100, Daniel Vetter wrote: > On Mon, Mar 14, 2016 at 04:20:19PM +0200, Ville Syrjälä wrote: > > On Mon, Mar 14, 2016 at 10:04:00AM +0100, Daniel Vetter wrote: > > > On Fri, Mar 11, 2016 at 09:09:12PM +0200, Ville Syrjälä wrote: > > > > On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > > > > > Thanks for the review Ville > > > > > > > > > > [snip] > > > > > > > > > > > Kinda hard to see where everything gets used due to the way the patches > > > > > > are split up. > > > > > > > > > > Yes, it's far from great... > > > > > > > > > > > At least the hotplug/mode change events are not needed. We only have the > > > > > > two points where i915 should inform the audio driver about this stuff, > > > > > > and those are the intel_audio_code_enable/disable(). For that we > > > > > > already have the .pin_eld_notify() hook. > > > > > > > > > > > > The interrupt stuff should mostly vanish from i915 with the subdevice > > > > > > approach. As in i915 would just call the interrupt handler of the audio > > > > > > driver based on the LPE bits in IIR, and the audio driver can then do > > > > > > whatever it wants based on its own status register. > > > > > > > > > > Are you saying that the subdevice would provide a read/write interface > > > > > for the audio driver to look at display registers, and the i915 driver > > > > > would only provide a notification interface (EDID and interrupts) to the > > > > > audio driver? > > > > > > > > The audio driver would simply ioremap the appropriate range of > > > > registers itself. > > > > > > > > > If yes, would there be two component framework links, one between > > > > > i915/audio driver and one between subdevice/audio driver. > > > > > > > > Yeah sort of. i915 registers the platform device for the audio, the > > > > audio driver can then bind to the device via the platform driver .probe > > > > callback. It can then register with the audio component stuff at some > > > > point to get the relevant notifications on the display state. When > > > > i915 gets unloaded we remove the platform device, at which point the > > > > audio driver's platform driver .remove() callback gets invoked and > > > > it should unregister/cleanup everything. > > > > > > > > I just tried to frob around with the VED code a bit, and got it to load > > > > at least. It's not quite happy about reloading i915 while the ipvr > > > > driver was loaded though. Not sure what's going on there, but I do > > > > think this approach should work. So the VED patch could serve as a > > > > decent enough model to follow. > > > > > > platform devices registerd by modules are apparently inherently racy and > > > in an unfixable way. At least I remember something like that from VED > > > discussion. > > > > > > In short you _must_ unload VED manually before unloading i915, or it all > > > goes boom. If this is the only thing that went boom it's acceptable. > > > > VED goes boom due drm_global_mutex deadlock at least if you load > > i915 while ipvr is already loaded. I didn't check to hard if there > > were any booms on pure unload so far. > > Oh right another boom that happened, but can be avoided by dropping the > ->load callback and directly calling drm_dev_alloc/register. Shouldn't be > a problem with a non-drm driver though. > > > Anyways, I was a bit worried about the races so I did a pair of > > small test modules to try out this model, and to me it looked to > > work so far. I just unregistered the platform device from the parent > > pci driver .remove() hook, and it got blocked until the platform > > driver's .remove() hook was done. > > > > In any case if this is broken, then I assume mfd must be broken. And > > that thing is at least used quite extensively. > > Hm, I don't remember the exact details, but iirc the problem was that the > struct device is refcounted, but you can't add a module ref for the vtable > is has (specifically the final release method) since that would result in > a ref-loop. Usually it works, but if someone keeps the device alive > (through sysfs or whatever) and you manage to unload everything before > that last ref gets dropped it goes boom. > > So "works", but not in a safe way. I was worried that it's something like that. I guess I'll need to try grab a ref on something in my test module and see how it fares.
On Tue, Mar 15, 2016 at 09:39:48AM +0100, Daniel Vetter wrote: > On Mon, Mar 14, 2016 at 12:27:51PM -0500, Pierre-Louis Bossart wrote: > > On 3/14/16 10:30 AM, Ville Syrjälä wrote: > > >On Mon, Mar 14, 2016 at 05:21:54PM +0200, Ville Syrjälä wrote: > > >>On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: > > >>>On 3/11/16 1:09 PM, Ville Syrjälä wrote: > > >>>>On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > > >>>>>Thanks for the review Ville > > >>>>> > > >>>>>[snip] > > >>>>> > > >>>>>>Kinda hard to see where everything gets used due to the way the patches > > >>>>>>are split up. > > >>>>> > > >>>>>Yes, it's far from great... > > >>>>> > > >>>>>>At least the hotplug/mode change events are not needed. We only have the > > >>>>>>two points where i915 should inform the audio driver about this stuff, > > >>>>>>and those are the intel_audio_code_enable/disable(). For that we > > >>>>>>already have the .pin_eld_notify() hook. > > >>>>>> > > >>>>>>The interrupt stuff should mostly vanish from i915 with the subdevice > > >>>>>>approach. As in i915 would just call the interrupt handler of the audio > > >>>>>>driver based on the LPE bits in IIR, and the audio driver can then do > > >>>>>>whatever it wants based on its own status register. > > >>>>> > > >>>>>Are you saying that the subdevice would provide a read/write interface > > >>>>>for the audio driver to look at display registers, and the i915 driver > > >>>>>would only provide a notification interface (EDID and interrupts) to the > > >>>>>audio driver? > > >>>> > > >>>>The audio driver would simply ioremap the appropriate range of > > >>>>registers itself. > > >>>> > > >>>>>If yes, would there be two component framework links, one between > > >>>>>i915/audio driver and one between subdevice/audio driver. > > >>>> > > >>>>Yeah sort of. i915 registers the platform device for the audio, the > > >>>>audio driver can then bind to the device via the platform driver .probe > > >>>>callback. It can then register with the audio component stuff at some > > >>>>point to get the relevant notifications on the display state. When > > >>>>i915 gets unloaded we remove the platform device, at which point the > > >>>>audio driver's platform driver .remove() callback gets invoked and > > >>>>it should unregister/cleanup everything. > > >>> > > >>>we don't want to expose this interface when HDAudio is present and > > >>>enabled so we would need to add a test for this. > > >>>Also it looks like you want the creation of the platform device done in > > >>>i915, we were thinking of doing this as part of the audio drivers but in > > >>>the end this looks equivalent. In both cases we would end-up ignoring > > >>>the HAD022A8 HID and not use acpi for this extension > > >> > > >>Well, if you have a device you can hang off from then i915 should need > > >>to register it I suppose. Though that would make the interrupt > > >>forwarding perhaps less nice. There's also the suspend/resume order > > >>dependency to deal with if there's no parent/child relationship between > > >>the devices. > > > > > >Oh and I suppose you'd end up ioremapping part of the pci dev2 mmio bar > > >to get at the registers, which would look a bit funkly at least. > > > > 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. 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.
On Tue, Mar 15, 2016 at 03:35:45PM +0200, Ville Syrjälä wrote: > On Tue, Mar 15, 2016 at 09:39:48AM +0100, Daniel Vetter wrote: > > On Mon, Mar 14, 2016 at 12:27:51PM -0500, Pierre-Louis Bossart wrote: > > > On 3/14/16 10:30 AM, Ville Syrjälä wrote: > > > >On Mon, Mar 14, 2016 at 05:21:54PM +0200, Ville Syrjälä wrote: > > > >>On Mon, Mar 14, 2016 at 10:13:58AM -0500, Pierre-Louis Bossart wrote: > > > >>>On 3/11/16 1:09 PM, Ville Syrjälä wrote: > > > >>>>On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > > > >>>>>Thanks for the review Ville > > > >>>>> > > > >>>>>[snip] > > > >>>>> > > > >>>>>>Kinda hard to see where everything gets used due to the way the patches > > > >>>>>>are split up. > > > >>>>> > > > >>>>>Yes, it's far from great... > > > >>>>> > > > >>>>>>At least the hotplug/mode change events are not needed. We only have the > > > >>>>>>two points where i915 should inform the audio driver about this stuff, > > > >>>>>>and those are the intel_audio_code_enable/disable(). For that we > > > >>>>>>already have the .pin_eld_notify() hook. > > > >>>>>> > > > >>>>>>The interrupt stuff should mostly vanish from i915 with the subdevice > > > >>>>>>approach. As in i915 would just call the interrupt handler of the audio > > > >>>>>>driver based on the LPE bits in IIR, and the audio driver can then do > > > >>>>>>whatever it wants based on its own status register. > > > >>>>> > > > >>>>>Are you saying that the subdevice would provide a read/write interface > > > >>>>>for the audio driver to look at display registers, and the i915 driver > > > >>>>>would only provide a notification interface (EDID and interrupts) to the > > > >>>>>audio driver? > > > >>>> > > > >>>>The audio driver would simply ioremap the appropriate range of > > > >>>>registers itself. > > > >>>> > > > >>>>>If yes, would there be two component framework links, one between > > > >>>>>i915/audio driver and one between subdevice/audio driver. > > > >>>> > > > >>>>Yeah sort of. i915 registers the platform device for the audio, the > > > >>>>audio driver can then bind to the device via the platform driver .probe > > > >>>>callback. It can then register with the audio component stuff at some > > > >>>>point to get the relevant notifications on the display state. When > > > >>>>i915 gets unloaded we remove the platform device, at which point the > > > >>>>audio driver's platform driver .remove() callback gets invoked and > > > >>>>it should unregister/cleanup everything. > > > >>> > > > >>>we don't want to expose this interface when HDAudio is present and > > > >>>enabled so we would need to add a test for this. > > > >>>Also it looks like you want the creation of the platform device done in > > > >>>i915, we were thinking of doing this as part of the audio drivers but in > > > >>>the end this looks equivalent. In both cases we would end-up ignoring > > > >>>the HAD022A8 HID and not use acpi for this extension > > > >> > > > >>Well, if you have a device you can hang off from then i915 should need > > > >>to register it I suppose. Though that would make the interrupt > > > >>forwarding perhaps less nice. There's also the suspend/resume order > > > >>dependency to deal with if there's no parent/child relationship between > > > >>the devices. > > > > > > > >Oh and I suppose you'd end up ioremapping part of the pci dev2 mmio bar > > > >to get at the registers, which would look a bit funkly at least. > > > > > > 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. > > 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. Hm right. I guess we could stuff it into the platform data stuff maybe instead, so the driver could get at the i915/snd interface directly? -Daniel
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
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?
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
On Tue, Mar 15, 2016 at 03:30:55PM +0200, Ville Syrjälä wrote: > On Tue, Mar 15, 2016 at 09:35:26AM +0100, Daniel Vetter wrote: > > On Mon, Mar 14, 2016 at 04:20:19PM +0200, Ville Syrjälä wrote: > > > On Mon, Mar 14, 2016 at 10:04:00AM +0100, Daniel Vetter wrote: > > > > On Fri, Mar 11, 2016 at 09:09:12PM +0200, Ville Syrjälä wrote: > > > > > On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > > > > > > Thanks for the review Ville > > > > > > > > > > > > [snip] > > > > > > > > > > > > > Kinda hard to see where everything gets used due to the way the patches > > > > > > > are split up. > > > > > > > > > > > > Yes, it's far from great... > > > > > > > > > > > > > At least the hotplug/mode change events are not needed. We only have the > > > > > > > two points where i915 should inform the audio driver about this stuff, > > > > > > > and those are the intel_audio_code_enable/disable(). For that we > > > > > > > already have the .pin_eld_notify() hook. > > > > > > > > > > > > > > The interrupt stuff should mostly vanish from i915 with the subdevice > > > > > > > approach. As in i915 would just call the interrupt handler of the audio > > > > > > > driver based on the LPE bits in IIR, and the audio driver can then do > > > > > > > whatever it wants based on its own status register. > > > > > > > > > > > > Are you saying that the subdevice would provide a read/write interface > > > > > > for the audio driver to look at display registers, and the i915 driver > > > > > > would only provide a notification interface (EDID and interrupts) to the > > > > > > audio driver? > > > > > > > > > > The audio driver would simply ioremap the appropriate range of > > > > > registers itself. > > > > > > > > > > > If yes, would there be two component framework links, one between > > > > > > i915/audio driver and one between subdevice/audio driver. > > > > > > > > > > Yeah sort of. i915 registers the platform device for the audio, the > > > > > audio driver can then bind to the device via the platform driver .probe > > > > > callback. It can then register with the audio component stuff at some > > > > > point to get the relevant notifications on the display state. When > > > > > i915 gets unloaded we remove the platform device, at which point the > > > > > audio driver's platform driver .remove() callback gets invoked and > > > > > it should unregister/cleanup everything. > > > > > > > > > > I just tried to frob around with the VED code a bit, and got it to load > > > > > at least. It's not quite happy about reloading i915 while the ipvr > > > > > driver was loaded though. Not sure what's going on there, but I do > > > > > think this approach should work. So the VED patch could serve as a > > > > > decent enough model to follow. > > > > > > > > platform devices registerd by modules are apparently inherently racy and > > > > in an unfixable way. At least I remember something like that from VED > > > > discussion. > > > > > > > > In short you _must_ unload VED manually before unloading i915, or it all > > > > goes boom. If this is the only thing that went boom it's acceptable. > > > > > > VED goes boom due drm_global_mutex deadlock at least if you load > > > i915 while ipvr is already loaded. I didn't check to hard if there > > > were any booms on pure unload so far. > > > > Oh right another boom that happened, but can be avoided by dropping the > > ->load callback and directly calling drm_dev_alloc/register. Shouldn't be > > a problem with a non-drm driver though. > > > > > Anyways, I was a bit worried about the races so I did a pair of > > > small test modules to try out this model, and to me it looked to > > > work so far. I just unregistered the platform device from the parent > > > pci driver .remove() hook, and it got blocked until the platform > > > driver's .remove() hook was done. > > > > > > In any case if this is broken, then I assume mfd must be broken. And > > > that thing is at least used quite extensively. > > > > Hm, I don't remember the exact details, but iirc the problem was that the > > struct device is refcounted, but you can't add a module ref for the vtable > > is has (specifically the final release method) since that would result in > > a ref-loop. Usually it works, but if someone keeps the device alive > > (through sysfs or whatever) and you manage to unload everything before > > that last ref gets dropped it goes boom. > > > > So "works", but not in a safe way. > > I was worried that it's something like that. I guess I'll need to try > grab a ref on something in my test module and see how it fares. At least a sysfs attribute on the child device didn't cause any explosions in my tests. I slept for a while in the sysfs store function, and while doing that removed the module for the parent device. The platform_device_unregister() in the parent .remove() blocked until the sysfs store was done.
On Wed, Mar 16, 2016 at 06:43:38PM +0200, Ville Syrjälä wrote: > On Tue, Mar 15, 2016 at 03:30:55PM +0200, Ville Syrjälä wrote: > > On Tue, Mar 15, 2016 at 09:35:26AM +0100, Daniel Vetter wrote: > > > On Mon, Mar 14, 2016 at 04:20:19PM +0200, Ville Syrjälä wrote: > > > > On Mon, Mar 14, 2016 at 10:04:00AM +0100, Daniel Vetter wrote: > > > > > On Fri, Mar 11, 2016 at 09:09:12PM +0200, Ville Syrjälä wrote: > > > > > > On Fri, Mar 11, 2016 at 11:27:13AM -0600, Pierre-Louis Bossart wrote: > > > > > > > Thanks for the review Ville > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > Kinda hard to see where everything gets used due to the way the patches > > > > > > > > are split up. > > > > > > > > > > > > > > Yes, it's far from great... > > > > > > > > > > > > > > > At least the hotplug/mode change events are not needed. We only have the > > > > > > > > two points where i915 should inform the audio driver about this stuff, > > > > > > > > and those are the intel_audio_code_enable/disable(). For that we > > > > > > > > already have the .pin_eld_notify() hook. > > > > > > > > > > > > > > > > The interrupt stuff should mostly vanish from i915 with the subdevice > > > > > > > > approach. As in i915 would just call the interrupt handler of the audio > > > > > > > > driver based on the LPE bits in IIR, and the audio driver can then do > > > > > > > > whatever it wants based on its own status register. > > > > > > > > > > > > > > Are you saying that the subdevice would provide a read/write interface > > > > > > > for the audio driver to look at display registers, and the i915 driver > > > > > > > would only provide a notification interface (EDID and interrupts) to the > > > > > > > audio driver? > > > > > > > > > > > > The audio driver would simply ioremap the appropriate range of > > > > > > registers itself. > > > > > > > > > > > > > If yes, would there be two component framework links, one between > > > > > > > i915/audio driver and one between subdevice/audio driver. > > > > > > > > > > > > Yeah sort of. i915 registers the platform device for the audio, the > > > > > > audio driver can then bind to the device via the platform driver .probe > > > > > > callback. It can then register with the audio component stuff at some > > > > > > point to get the relevant notifications on the display state. When > > > > > > i915 gets unloaded we remove the platform device, at which point the > > > > > > audio driver's platform driver .remove() callback gets invoked and > > > > > > it should unregister/cleanup everything. > > > > > > > > > > > > I just tried to frob around with the VED code a bit, and got it to load > > > > > > at least. It's not quite happy about reloading i915 while the ipvr > > > > > > driver was loaded though. Not sure what's going on there, but I do > > > > > > think this approach should work. So the VED patch could serve as a > > > > > > decent enough model to follow. > > > > > > > > > > platform devices registerd by modules are apparently inherently racy and > > > > > in an unfixable way. At least I remember something like that from VED > > > > > discussion. > > > > > > > > > > In short you _must_ unload VED manually before unloading i915, or it all > > > > > goes boom. If this is the only thing that went boom it's acceptable. > > > > > > > > VED goes boom due drm_global_mutex deadlock at least if you load > > > > i915 while ipvr is already loaded. I didn't check to hard if there > > > > were any booms on pure unload so far. > > > > > > Oh right another boom that happened, but can be avoided by dropping the > > > ->load callback and directly calling drm_dev_alloc/register. Shouldn't be > > > a problem with a non-drm driver though. > > > > > > > Anyways, I was a bit worried about the races so I did a pair of > > > > small test modules to try out this model, and to me it looked to > > > > work so far. I just unregistered the platform device from the parent > > > > pci driver .remove() hook, and it got blocked until the platform > > > > driver's .remove() hook was done. > > > > > > > > In any case if this is broken, then I assume mfd must be broken. And > > > > that thing is at least used quite extensively. > > > > > > Hm, I don't remember the exact details, but iirc the problem was that the > > > struct device is refcounted, but you can't add a module ref for the vtable > > > is has (specifically the final release method) since that would result in > > > a ref-loop. Usually it works, but if someone keeps the device alive > > > (through sysfs or whatever) and you manage to unload everything before > > > that last ref gets dropped it goes boom. > > > > > > So "works", but not in a safe way. > > > > I was worried that it's something like that. I guess I'll need to try > > grab a ref on something in my test module and see how it fares. > > At least a sysfs attribute on the child device didn't cause any > explosions in my tests. I slept for a while in the sysfs store function, > and while doing that removed the module for the parent device. The > platform_device_unregister() in the parent .remove() blocked until the > sysfs store was done. Hm, maybe it's been fixed by now, that discussion about platform devices created by modules was a while ago. -Daniel
diff --git a/drivers/gpu/drm/i915/hdmi_audio_if.h b/drivers/gpu/drm/i915/hdmi_audio_if.h new file mode 100644 index 0000000..f968028 --- /dev/null +++ b/drivers/gpu/drm/i915/hdmi_audio_if.h @@ -0,0 +1,122 @@ +/* + * Copyright (c) 2010, Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * Authors: + * jim liu <jim.liu@intel.com> + * Uma Shankar <uma.shankar@intel.com> + */ + + +#ifndef __HDMI_AUDIO_IF_H +#define __HDMI_AUDIO_IF_H + +#include <linux/types.h> +#include <drm/drmP.h> + +/* HDMI AUDIO INTERRUPT TYPE */ +#define HDMI_AUDIO_UNDERRUN (1UL<<0) +#define HDMI_AUDIO_BUFFER_DONE (1UL<<1) + +/* the monitor type HDMI or DVI */ +#define MONITOR_TYPE_HDMI 1 +#define MONITOR_TYPE_DVI 2 + +extern int i915_hdmi_state; +extern int i915_notify_had; + +enum had_caps_list { + HAD_GET_ELD = 1, + HAD_GET_SAMPLING_FREQ, + HAD_GET_DISPLAY_RATE, + HAD_GET_HDCP_STATUS, + HAD_GET_AUDIO_STATUS, + HAD_SET_ENABLE_AUDIO, + HAD_SET_DISABLE_AUDIO, + HAD_SET_ENABLE_AUDIO_INT, + HAD_SET_DISABLE_AUDIO_INT, + OTHERS_TBD, +}; + +enum had_event_type { + HAD_EVENT_HOT_PLUG = 1, + HAD_EVENT_HOT_UNPLUG, + HAD_EVENT_MODE_CHANGING, + HAD_EVENT_PM_CHANGING, + HAD_EVENT_AUDIO_BUFFER_DONE, + HAD_EVENT_AUDIO_BUFFER_UNDERRUN, + HAD_EVENT_QUERY_IS_AUDIO_BUSY, + HAD_EVENT_QUERY_IS_AUDIO_SUSPENDED, +}; + +/* + * HDMI Display Controller Audio Interface + * + */ +typedef int (*had_event_call_back) (enum had_event_type event_type, + void *ctxt_info); + +struct hdmi_audio_registers_ops { + int (*hdmi_audio_read_register)(uint32_t reg_addr, uint32_t *data); + int (*hdmi_audio_write_register)(uint32_t reg_addr, uint32_t data); + int (*hdmi_audio_read_modify)(uint32_t reg_addr, uint32_t data, + uint32_t mask); +}; + +struct hdmi_audio_query_set_ops { + int (*hdmi_audio_get_caps)(enum had_caps_list query_element, + void *capabilties); + int (*hdmi_audio_set_caps)(enum had_caps_list set_element, + void *capabilties); +}; + +typedef struct hdmi_audio_event { + int type; +} hdmi_audio_event_t; + +struct snd_intel_had_interface { + const char *name; + int (*query)(void *had_data, hdmi_audio_event_t event); + int (*suspend)(void *had_data, hdmi_audio_event_t event); + int (*resume)(void *had_data); +}; + +struct hdmi_audio_priv { + struct drm_device *dev; + u32 hdmib_reg; + + bool is_hdcp_supported; + bool hdmi_hpd_connected; + int monitor_type; + void *context; +}; + +extern void i915_hdmi_audio_init(struct hdmi_audio_priv *p_hdmi_priv); + +extern bool mid_hdmi_audio_is_busy(struct drm_device *dev); +extern bool mid_hdmi_audio_suspend(struct drm_device *dev); +extern void mid_hdmi_audio_resume(struct drm_device *dev); +extern void mid_hdmi_audio_signal_event(struct drm_device *dev, + enum had_event_type event); + +/* Added for HDMI Audio */ +extern void hdmi_get_eld(uint8_t *eld); +extern int i915_enable_hdmi_audio_int(struct drm_device *dev); +extern int i915_disable_hdmi_audio_int(struct drm_device *dev); +extern int mid_hdmi_audio_setup( + had_event_call_back audio_callbacks, + struct hdmi_audio_registers_ops *reg_ops, + struct hdmi_audio_query_set_ops *query_ops); +extern int mid_hdmi_audio_register( + struct snd_intel_had_interface *driver, + void *had_data); + +#endif /* __HDMI_AUDIO_IF_H */ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2cb0a41..5dceb5b 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -53,6 +53,7 @@ #include <linux/kref.h> #include <linux/pm_qos.h> #include "intel_guc.h" +#include "hdmi_audio_if.h" /* General customization: */ @@ -1196,6 +1197,18 @@ struct intel_gen6_power_mgmt { struct mutex hw_lock; }; +/* Runtime power management related */ +struct intel_gen7_rpm { + /* To track (num of get calls - num of put calls) + * made by procfs + */ + atomic_t procfs_count; + /* To make sure ring get/put are in pair */ + bool ring_active; + struct proc_dir_entry *i915_proc_dir; + struct proc_dir_entry *i915_proc_file; +}; + /* defined intel_pm.c */ extern spinlock_t mchdev_lock; @@ -2018,6 +2031,19 @@ struct drm_i915_private { struct intel_encoder *dig_port_map[I915_MAX_PORTS]; + /* Added for HDMI Audio */ + had_event_call_back had_event_callbacks; + struct snd_intel_had_interface *had_interface; + void *had_pvt_data; + int tmds_clock_speed; + int hdmi_audio_interrupt_mask; + struct work_struct hdmi_audio_wq; + + u32 hotplug_status; + + /* Runtime power management related */ + struct intel_gen7_rpm rpm; + /* * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch * will be rejected. Instead look for a better place. @@ -3537,6 +3563,11 @@ int intel_freq_opcode(struct drm_i915_private *dev_priv, int val); } while (upper != old_upper && loop++ < 2); \ (u64)upper << 32 | lower; }) +int i915_rpm_get_disp(struct drm_device *dev); +int i915_rpm_put_disp(struct drm_device *dev); + +bool i915_is_device_active(struct drm_device *dev); + #define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg) #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 228b22f..370371c 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2062,7 +2062,12 @@ enum skl_disp_power_wells { #define I915_WINVALID_INTERRUPT (1<<1) #define I915_USER_INTERRUPT (1<<1) #define I915_ASLE_INTERRUPT (1<<0) +#define I915_LPE_AUDIO_HDMI_STATUS_A _MMIO(dev_priv->info.display_mmio_offset + 0x65064) +#define I915_LPE_AUDIO_HDMI_STATUS_B _MMIO(dev_priv->info.display_mmio_offset + 0x65864) +#define I915_HDMI_AUDIO_UNDERRUN (1UL<<31) +#define I915_HDMI_AUDIO_BUFFER_DONE (1UL<<29) #define I915_BSD_USER_INTERRUPT (1<<25) +#define I915_HDMI_AUDIO_UNDERRUN_ENABLE (1UL<<15) #define GEN6_BSD_RNCID _MMIO(0x12198) @@ -3364,6 +3369,7 @@ enum skl_disp_power_wells { #define _GEN3_SDVOC 0x61160 #define GEN3_SDVOB _MMIO(_GEN3_SDVOB) #define GEN3_SDVOC _MMIO(_GEN3_SDVOC) +#define HDMIB (dev_priv->info.display_mmio_offset + 0x61140) #define GEN4_HDMIB GEN3_SDVOB #define GEN4_HDMIC GEN3_SDVOC #define VLV_HDMIB _MMIO(VLV_DISPLAY_BASE + 0x61140) @@ -3373,6 +3379,7 @@ enum skl_disp_power_wells { #define PCH_HDMIB PCH_SDVOB #define PCH_HDMIC _MMIO(0xe1150) #define PCH_HDMID _MMIO(0xe1160) +#define PORT_ENABLE (1 << 31) #define PORT_DFT_I9XX _MMIO(0x61150) #define DC_BALANCE_RESET (1 << 25) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index cb413e2..adfd27c 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -724,6 +724,14 @@ struct cxsr_latency { #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base) #define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL) +/* HDMI bits are shared with the DP bits */ +#define HDMIB_HOTPLUG_LIVE_STATUS (1 << 29) +#define HDMIC_HOTPLUG_LIVE_STATUS (1 << 28) +#define HDMID_HOTPLUG_LIVE_STATUS (1 << 27) +#define HDMI_LIVE_STATUS_BASE 30 +#define HDMI_LIVE_STATUS_DELAY_STEP 10 +#define HDMI_EDID_RETRY_COUNT 3 + struct intel_hdmi { i915_reg_t hdmi_reg; int ddc_bus; @@ -735,6 +743,9 @@ struct intel_hdmi { bool rgb_quant_range_selectable; enum hdmi_picture_aspect aspect_ratio; struct intel_connector *attached_connector; + struct edid *edid; + uint32_t edid_mode_count; + void (*write_infoframe)(struct drm_encoder *encoder, enum hdmi_infoframe_type type, const void *frame, ssize_t len);