Re: [PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy
Le lun. 22 févr. 2021 à 17:36, Shuah Khan a écrit : > > Cool. A quick check shows me 1031 strscpy() calls with no return > checks. All or some of these probably need to be reviewed and add > return checks. Is this something that is in the plan to address as > part of this work? > > thanks, > -- Shuah > Hi, Initially, what we planned with Kees is to firstly replace all calls with error handling codes (like this series does), and then replace all other simple calls (without error handling). However, we can also start a discussion about this topic, all suggestions are welcome. I am not sure that it does make sense to check all returns code in all cases (for example in arch/alpha/kernel/setup.c, there are a ton of other examples in the kernel). But a general review (as you suggest), would make sense. Regards, Romain ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 04/20] dma-buf: Manual replacement of the deprecated strlcpy() with return values
The strlcpy() reads the entire source buffer first, it is dangerous if
the source buffer lenght is unbounded or possibility non NULL-terminated.
It can lead to linear read overflows, crashes, etc...
As recommended in the deprecated interfaces [1], it should be replaced
by strscpy.
This commit replaces all calls to strlcpy that handle the return values
by the corresponding strscpy calls with new handling of the return
values (as it is quite different between the two functions).
[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
Signed-off-by: Romain Perier
---
drivers/dma-buf/dma-buf.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index f264b70c383e..515192f2f404 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -42,12 +42,12 @@ static char *dmabuffs_dname(struct dentry *dentry, char
*buffer, int buflen)
{
struct dma_buf *dmabuf;
char name[DMA_BUF_NAME_LEN];
- size_t ret = 0;
+ ssize_t ret = 0;
dmabuf = dentry->d_fsdata;
spin_lock(&dmabuf->name_lock);
if (dmabuf->name)
- ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN);
+ ret = strscpy(name, dmabuf->name, DMA_BUF_NAME_LEN);
spin_unlock(&dmabuf->name_lock);
return dynamic_dname(dentry, buffer, buflen, "/%s:%s",
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 00/20] Manual replacement of all strlcpy in favor of strscpy
strlcpy() copy a C-String into a sized buffer, the result is always a valid NULL-terminated that fits in the buffer, howerver it has severals issues. It reads the source buffer first, which is dangerous if it is non NULL-terminated or if the corresponding buffer is unbounded. Its safe replacement is strscpy(), as suggested in the deprecated interface [1]. We plan to make this contribution in two steps: - Firsly all cases of strlcpy's return value are manually replaced by the corresponding calls of strscpy() with the new handling of the return value (as the return code is different in case of error). - Then all other cases are automatically replaced by using coccinelle. This series covers manual replacements. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy Romain Perier (20): cgroup: Manual replacement of the deprecated strlcpy() with return values crypto: Manual replacement of the deprecated strlcpy() with return values devlink: Manual replacement of the deprecated strlcpy() with return values dma-buf: Manual replacement of the deprecated strlcpy() with return values kobject: Manual replacement of the deprecated strlcpy() with return values ima: Manual replacement of the deprecated strlcpy() with return values SUNRPC: Manual replacement of the deprecated strlcpy() with return values kernfs: Manual replacement of the deprecated strlcpy() with return values m68k/atari: Manual replacement of the deprecated strlcpy() with return values module: Manual replacement of the deprecated strlcpy() with return values hwmon: Manual replacement of the deprecated strlcpy() with return values s390/hmcdrv: Manual replacement of the deprecated strlcpy() with return values scsi: zfcp: Manual replacement of the deprecated strlcpy() with return values target: Manual replacement of the deprecated strlcpy() with return values ALSA: usb-audio: Manual replacement of the deprecated strlcpy() with return values tracing/probe: Manual replacement of the deprecated strlcpy() with return values vt: Manual replacement of the deprecated strlcpy() with return values usb: gadget: f_midi: Manual replacement of the deprecated strlcpy() with return values usbip: usbip_host: Manual replacement of the deprecated strlcpy() with return values s390/watchdog: Manual replacement of the deprecated strlcpy() with return values arch/m68k/emu/natfeat.c | 6 +-- crypto/lrw.c| 6 +-- crypto/xts.c| 6 +-- drivers/dma-buf/dma-buf.c | 4 +- drivers/hwmon/pmbus/max20730.c | 66 + drivers/s390/char/diag_ftp.c| 4 +- drivers/s390/char/sclp_ftp.c| 6 +-- drivers/s390/scsi/zfcp_fc.c | 8 +-- drivers/target/target_core_configfs.c | 33 - drivers/tty/vt/keyboard.c | 5 +- drivers/usb/gadget/function/f_midi.c| 4 +- drivers/usb/gadget/function/f_printer.c | 8 +-- drivers/usb/usbip/stub_main.c | 6 +-- drivers/watchdog/diag288_wdt.c | 12 +++-- fs/kernfs/dir.c | 27 +- kernel/cgroup/cgroup.c | 2 +- kernel/module.c | 4 +- kernel/trace/trace_uprobe.c | 11 ++--- lib/kobject_uevent.c| 6 +-- net/core/devlink.c | 6 +-- net/sunrpc/clnt.c | 6 ++- security/integrity/ima/ima_policy.c | 8 ++- sound/usb/card.c| 4 +- 23 files changed, 129 insertions(+), 119 deletions(-) -- 2.20.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] drm: dw-hdmi: gate audio clock from the I2S enablement callbacks
Hello,
Le 19/04/2017 à 06:51, Archit Taneja a écrit :
>
>
> On 04/14/2017 02:01 PM, Romain Perier wrote:
>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>> step E. and is kept enabled for later use. This clock should be enabled
>> and disabled along with the actual audio stream and not always on (that
>> is bad for PM). Futhermore, as described by the datasheet, the I2S
>
> s/Futhermore/Furthermore
>
>> variant need to gate/ungate the clock when the stream is
>
> s/need/needs
>
>> enabled/disabled.
>>
>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>> when the audio sample clock must be enabled or disabled. Then, it adds
>> the call to this function from dw_hdmi_i2s_audio_enable() and
>> dw_hdmi_i2s_audio_disable().
>>
>> Signed-off-by: Romain Perier
>> ---
>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 20 ++--
>> 1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index 5b328c0..a6da634 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -544,6 +544,12 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi
>> *hdmi, unsigned int rate)
>> }
>> EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
>>
>> +static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi, bool enable)
>> +{
>> +hdmi_modb(hdmi, enable ? 0 : HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>> + HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
>> +}
>> +
>> void dw_hdmi_ahb_audio_enable(struct dw_hdmi *hdmi)
>> {
>> hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
>> @@ -557,6 +563,12 @@ void dw_hdmi_ahb_audio_disable(struct dw_hdmi
>> *hdmi)
>> void dw_hdmi_i2s_audio_enable(struct dw_hdmi *hdmi)
>> {
>> hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
>> +hdmi_enable_audio_clk(hdmi, true);
>> +}
>> +
>> +void dw_hdmi_i2s_audio_disable(struct dw_hdmi *hdmi)
>> +{
>> +hdmi_enable_audio_clk(hdmi, false);
>> }
>
> This should be static too.
>
> If you're okay with the suggestions, I can fix these myself and push. Let
> me know if that's okay.
>
> Thanks,
> Archit
>
Yes, I completely agree.
Thanks,
Romain
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] drm: dw-hdmi: gate audio clock from the I2S enablement callbacks
Hello, > Took the liberty of going ahead with the fixes. Queued both patches to > drm-misc-next. > > Thanks, > Archit You were right. Sorry for the delay, I am back from vacations :) Regards, Romain ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/2] drm: dw-hdmi: gate audio clock from the I2S enablement callbacks
Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
step E. and is kept enabled for later use. This clock should be enabled
and disabled along with the actual audio stream and not always on (that
is bad for PM). Futhermore, as described by the datasheet, the I2S
variant need to gate/ungate the clock when the stream is
enabled/disabled.
This commit adds a parameter to hdmi_audio_enable_clk() that controls
when the audio sample clock must be enabled or disabled. Then, it adds
the call to this function from dw_hdmi_i2s_audio_enable() and
dw_hdmi_i2s_audio_disable().
Signed-off-by: Romain Perier
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 20 ++--
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 5b328c0..a6da634 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -544,6 +544,12 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi,
unsigned int rate)
}
EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
+static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi, bool enable)
+{
+ hdmi_modb(hdmi, enable ? 0 : HDMI_MC_CLKDIS_AUDCLK_DISABLE,
+ HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
+}
+
void dw_hdmi_ahb_audio_enable(struct dw_hdmi *hdmi)
{
hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
@@ -557,6 +563,12 @@ void dw_hdmi_ahb_audio_disable(struct dw_hdmi *hdmi)
void dw_hdmi_i2s_audio_enable(struct dw_hdmi *hdmi)
{
hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+ hdmi_enable_audio_clk(hdmi, true);
+}
+
+void dw_hdmi_i2s_audio_disable(struct dw_hdmi *hdmi)
+{
+ hdmi_enable_audio_clk(hdmi, false);
}
void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
@@ -1592,11 +1604,6 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi
*hdmi)
HDMI_MC_FLOWCTRL);
}
-static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi)
-{
- hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
-}
-
/* Workaround to clear the overflow condition */
static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi)
{
@@ -1710,7 +1717,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct
drm_display_mode *mode)
/* HDMI Initialization Step E - Configure audio */
hdmi_clk_regenerator_update_pixel_clock(hdmi);
- hdmi_enable_audio_clk(hdmi);
+ hdmi_enable_audio_clk(hdmi, true);
}
/* not for DVI mode */
@@ -2438,6 +2445,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
audio.write = hdmi_writeb;
audio.read = hdmi_readb;
hdmi->enable_audio = dw_hdmi_i2s_audio_enable;
+ hdmi->disable_audio = dw_hdmi_i2s_audio_disable;
pdevinfo.name = "dw-hdmi-i2s-audio";
pdevinfo.data = &audio;
--
2.9.3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/2] drm: dw-hdmi: various improvements
This set of patches split the stream handling functions in two parts. It introduces new callbacks that are specific to each variant, one for I2S and one for AHB. Then, as requested by the datasheet for the I2S variant, it adds support for gating the audio sampler clock when the audio stream is enabled and disabled. This patches series is the continuity of the following discussion: http://lists.infradead.org/pipermail/linux-arm-kernel/2017-March/493550.html Changes in v2: - Add NULL check in dw_hdmi_audio_enable/dw_hdmi_audio_disable - Don't define dw_hdmi_i2s_audio_disable in PATCH 1/2 - Rebased onto drm-misc-next Romain Perier (2): drm: dw-hdmi: add specific I2S and AHB functions for stream handling drm: dw-hdmi: gate audio clock from the I2S enablement callbacks drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 46 +-- 1 file changed, 38 insertions(+), 8 deletions(-) -- 2.9.3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/2] drm: dw-hdmi: add specific I2S and AHB functions for stream handling
Currently, CTS+N is forced to zero as a workaround of the IP block for
i.MX platforms. This is requested in the datasheet of the corresponding
IP for AHB mode only. However, we have seen that it introduces glitches
or delays when playing a sound on HDMI for I2S mode. This proves that we
cannot keep the current functions for handling audio stream as-is if
these contain workaround that are specific to a mode.
This commit introduces two callbacks, one for each variant.
dw_hdmi_setup defines the right function depending on the detected
variant. Then, the exported functions dw_hdmi_audio_enable and
dw_hdmi_audio_disable calls the corresponding callbacks
Signed-off-by: Romain Perier
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 26 --
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 4b6f216..5b328c0 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -173,6 +173,8 @@ struct dw_hdmi {
unsigned int reg_shift;
struct regmap *regm;
+ void (*enable_audio)(struct dw_hdmi *hdmi);
+ void (*disable_audio)(struct dw_hdmi *hdmi);
};
#define HDMI_IH_PHY_STAT0_RX_SENSE \
@@ -542,13 +544,29 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi,
unsigned int rate)
}
EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
+void dw_hdmi_ahb_audio_enable(struct dw_hdmi *hdmi)
+{
+ hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+}
+
+void dw_hdmi_ahb_audio_disable(struct dw_hdmi *hdmi)
+{
+ hdmi_set_cts_n(hdmi, hdmi->audio_cts, 0);
+}
+
+void dw_hdmi_i2s_audio_enable(struct dw_hdmi *hdmi)
+{
+ hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+}
+
void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
{
unsigned long flags;
spin_lock_irqsave(&hdmi->audio_lock, flags);
hdmi->audio_enable = true;
- hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+ if (hdmi->enable_audio)
+ hdmi->enable_audio(hdmi);
spin_unlock_irqrestore(&hdmi->audio_lock, flags);
}
EXPORT_SYMBOL_GPL(dw_hdmi_audio_enable);
@@ -559,7 +577,8 @@ void dw_hdmi_audio_disable(struct dw_hdmi *hdmi)
spin_lock_irqsave(&hdmi->audio_lock, flags);
hdmi->audio_enable = false;
- hdmi_set_cts_n(hdmi, hdmi->audio_cts, 0);
+ if (hdmi->disable_audio)
+ hdmi->disable_audio(hdmi);
spin_unlock_irqrestore(&hdmi->audio_lock, flags);
}
EXPORT_SYMBOL_GPL(dw_hdmi_audio_disable);
@@ -2404,6 +2423,8 @@ __dw_hdmi_probe(struct platform_device *pdev,
audio.irq = irq;
audio.hdmi = hdmi;
audio.eld = hdmi->connector.eld;
+ hdmi->enable_audio = dw_hdmi_ahb_audio_enable;
+ hdmi->disable_audio = dw_hdmi_ahb_audio_disable;
pdevinfo.name = "dw-hdmi-ahb-audio";
pdevinfo.data = &audio;
@@ -2416,6 +2437,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
audio.hdmi = hdmi;
audio.write = hdmi_writeb;
audio.read = hdmi_readb;
+ hdmi->enable_audio = dw_hdmi_i2s_audio_enable;
pdevinfo.name = "dw-hdmi-i2s-audio";
pdevinfo.data = &audio;
--
2.9.3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: dw-hdmi: Implement the mode_fixup drm helper
This helper is supposed to validate or reject the modeline before it
applied by the mode setting. Currently this function has been dropped,
it was previously set to a dummy function that always returned true. For
both cases, this means that userspace can ask for a bad modeline that
will be always accepted.
On some platforms, like Rockchip, the drm dw_hdmi-rockchip variant driver
already implements the atomic_check drm helper, so mode_fixup cannot be
handled and implemented there (as drm_atomic_helper relies on either
atomic_check or mode_fixup).
This commit implements this helper. It only checks that this mode is
correct from the connector point of view
Signed-off-by: Romain Perier
---
drivers/gpu/drm/bridge/dw-hdmi.c | 16
1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 22211ff..3bd0807 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1740,6 +1740,21 @@ static int dw_hdmi_bridge_attach(struct drm_bridge
*bridge)
return 0;
}
+
+static bool dw_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
+ const struct drm_display_mode *orig_mode,
+ struct drm_display_mode *mode)
+{
+ struct dw_hdmi *hdmi = bridge->driver_private;
+ struct drm_connector *connector = &hdmi->connector;
+ enum drm_mode_status status;
+
+ status = dw_hdmi_connector_mode_valid(connector, mode);
+ if (status != MODE_OK)
+ return false;
+ return true;
+}
+
static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge,
struct drm_display_mode *orig_mode,
struct drm_display_mode *mode)
@@ -1781,6 +1796,7 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs
= {
.enable = dw_hdmi_bridge_enable,
.disable = dw_hdmi_bridge_disable,
.mode_set = dw_hdmi_bridge_mode_set,
+ .mode_fixup = dw_hdmi_bridge_mode_fixup,
};
static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi)
--
2.9.3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm: dw-hdmi: add specific I2S and AHB functions for stream handling
Currently, CTS+N is forced to zero as a workaround of the IP block for
i.MX platforms. This is requested in the datasheet of the corresponding
IP for AHB mode only. However, we have seen that it introduces glitches
or delays when playing a sound on HDMI for I2S mode. This proves that we
cannot keep the current functions for handling audio stream as-is if
these contain workaround that are specific to a mode.
This commit introduces two callbacks, one for each variant.
dw_hdmi_setup defines the right function depending on the detected
variant. Then, the exported functions dw_hdmi_audio_enable and
dw_hdmi_audio_disable calls the corresponding callbacks
Signed-off-by: Romain Perier
---
drivers/gpu/drm/bridge/dw-hdmi.c | 31 +--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 542d198..d34e0a5 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -166,6 +166,8 @@ struct dw_hdmi {
void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
u8 (*read)(struct dw_hdmi *hdmi, int offset);
+ void (*enable_audio)(struct dw_hdmi *hdmi);
+ void (*disable_audio)(struct dw_hdmi *hdmi);
};
#define HDMI_IH_PHY_STAT0_RX_SENSE \
@@ -557,13 +559,34 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi,
unsigned int rate)
}
EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
+void dw_hdmi_ahb_audio_enable(struct dw_hdmi *hdmi)
+{
+ hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+}
+
+void dw_hdmi_ahb_audio_disable(struct dw_hdmi *hdmi)
+{
+ hdmi_set_cts_n(hdmi, hdmi->audio_cts, 0);
+}
+
+void dw_hdmi_i2s_audio_enable(struct dw_hdmi *hdmi)
+{
+ hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+}
+
+
+void dw_hdmi_i2s_audio_disable(struct dw_hdmi *hdmi)
+{
+
+}
+
void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
{
unsigned long flags;
spin_lock_irqsave(&hdmi->audio_lock, flags);
hdmi->audio_enable = true;
- hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+ hdmi->enable_audio(hdmi);
spin_unlock_irqrestore(&hdmi->audio_lock, flags);
}
EXPORT_SYMBOL_GPL(dw_hdmi_audio_enable);
@@ -574,7 +597,7 @@ void dw_hdmi_audio_disable(struct dw_hdmi *hdmi)
spin_lock_irqsave(&hdmi->audio_lock, flags);
hdmi->audio_enable = false;
- hdmi_set_cts_n(hdmi, hdmi->audio_cts, 0);
+ hdmi->disable_audio(hdmi);
spin_unlock_irqrestore(&hdmi->audio_lock, flags);
}
EXPORT_SYMBOL_GPL(dw_hdmi_audio_disable);
@@ -2114,6 +2137,8 @@ __dw_hdmi_probe(struct platform_device *pdev,
audio.irq = irq;
audio.hdmi = hdmi;
audio.eld = hdmi->connector.eld;
+ hdmi->enable_audio = dw_hdmi_ahb_audio_enable;
+ hdmi->disable_audio = dw_hdmi_ahb_audio_disable;
pdevinfo.name = "dw-hdmi-ahb-audio";
pdevinfo.data = &audio;
@@ -2126,6 +2151,8 @@ __dw_hdmi_probe(struct platform_device *pdev,
audio.hdmi = hdmi;
audio.write = hdmi_writeb;
audio.read = hdmi_readb;
+ hdmi->enable_audio = dw_hdmi_i2s_audio_enable;
+ hdmi->disable_audio = dw_hdmi_i2s_audio_disable;
pdevinfo.name = "dw-hdmi-i2s-audio";
pdevinfo.data = &audio;
--
2.9.3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm: dw-hdmi: Gate audio clock from the I2S enablement callbacks
Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
step E. and is kept enabled for later use. This clock should be enabled
and disabled along with the actual audio stream and not always on (that
is bad for PM). Futhermore, as described by the datasheet, the I2S
variant need to gate/ungate the clock when the stream is
enabled/disabled.
This commit adds a parameter to hdmi_audio_enable_clk() that controls
when the audio sample clock must be enabled or disabled. Then, it adds
the call to this function from dw_hdmi_i2s_audio_enable() and
dw_hdmi_i2s_audio_disable().
Signed-off-by: Romain Perier
---
drivers/gpu/drm/bridge/dw-hdmi.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index d34e0a5..3bd0807 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -559,6 +559,12 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi,
unsigned int rate)
}
EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
+static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi, bool enable)
+{
+ hdmi_modb(hdmi, enable ? 0 : HDMI_MC_CLKDIS_AUDCLK_DISABLE,
+ HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
+}
+
void dw_hdmi_ahb_audio_enable(struct dw_hdmi *hdmi)
{
hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
@@ -572,12 +578,13 @@ void dw_hdmi_ahb_audio_disable(struct dw_hdmi *hdmi)
void dw_hdmi_i2s_audio_enable(struct dw_hdmi *hdmi)
{
hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+ hdmi_enable_audio_clk(hdmi, true);
}
void dw_hdmi_i2s_audio_disable(struct dw_hdmi *hdmi)
{
-
+ hdmi_enable_audio_clk(hdmi, false);
}
void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
@@ -1365,11 +1372,6 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi
*hdmi)
}
}
-static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi)
-{
- hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
-}
-
/* Workaround to clear the overflow condition */
static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi)
{
@@ -1471,7 +1473,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct
drm_display_mode *mode)
/* HDMI Initialization Step E - Configure audio */
hdmi_clk_regenerator_update_pixel_clock(hdmi);
- hdmi_enable_audio_clk(hdmi);
+ hdmi_enable_audio_clk(hdmi, true);
}
/* not for DVI mode */
--
2.9.3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/2] drm: dw-hdmi: various improvements
This set of patches split the stream handling functions in two parts. It introduces new callbacks that are specific to each variant, one for I2S and one for AHB. Then, as requested by the datasheet for the I2S variant, it adds support for gating the audio sampler clock when the audio stream is enabled and disabled. This patches series is the continuity of the following discussion: http://lists.infradead.org/pipermail/linux-arm-kernel/2017-March/493550.html Romain Perier (2): drm: dw-hdmi: add specific I2S and AHB functions for stream handling drm: dw-hdmi: Gate audio clock from the I2S enablement callbacks drivers/gpu/drm/bridge/dw-hdmi.c | 45 +--- 1 file changed, 37 insertions(+), 8 deletions(-) -- 2.9.3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm: dw-hdmi: Gate audio clock from the I2S enablement callbacks
Hello,
Le 07/04/2017 à 16:23, Neil Armstrong a écrit :
> On 04/07/2017 04:19 PM, Romain Perier wrote:
>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>> step E. and is kept enabled for later use. This clock should be enabled
>> and disabled along with the actual audio stream and not always on (that
>> is bad for PM). Futhermore, as described by the datasheet, the I2S
>> variant need to gate/ungate the clock when the stream is
>> enabled/disabled.
>>
>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>> when the audio sample clock must be enabled or disabled. Then, it adds
>> the call to this function from dw_hdmi_i2s_audio_enable() and
>> dw_hdmi_i2s_audio_disable().
>>
>> Signed-off-by: Romain Perier
>> ---
>> drivers/gpu/drm/bridge/dw-hdmi.c | 16 +---
>> 1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
>> b/drivers/gpu/drm/bridge/dw-hdmi.c
>> index d34e0a5..3bd0807 100644
>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>> @@ -559,6 +559,12 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi,
>> unsigned int rate)
>> }
>> EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
>>
>> +static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi, bool enable)
>> +{
>> +hdmi_modb(hdmi, enable ? 0 : HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>> + HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
>> +}
>> +
>> void dw_hdmi_ahb_audio_enable(struct dw_hdmi *hdmi)
>> {
>> hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
>> @@ -572,12 +578,13 @@ void dw_hdmi_ahb_audio_disable(struct dw_hdmi *hdmi)
>> void dw_hdmi_i2s_audio_enable(struct dw_hdmi *hdmi)
>> {
>> hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
>> +hdmi_enable_audio_clk(hdmi, true);
>> }
>>
>>
>> void dw_hdmi_i2s_audio_disable(struct dw_hdmi *hdmi)
>> {
>> -
>> +hdmi_enable_audio_clk(hdmi, false);
>> }
> I think the NULL check is still valid if you fill this function, because the
> IP also
> support other modes (SPDIF and GPA).
Ah, good point!
Thanks,
Romain
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions
Hi, Le 13/03/2017 à 19:49, Jose Abreu a écrit : > Hi Russell, > > > On 13-03-2017 13:10, Russell King - ARM Linux wrote: >> On Mon, Mar 13, 2017 at 12:55:58PM +, Jose Abreu wrote: >>> Hi, >>> >>> >>> On 13-03-2017 09:36, Russell King - ARM Linux wrote: >>>> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote: >>>>> On 03/10/2017 10:35 AM, Romain Perier wrote: >>>>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at >>>>>> step E. and is kept enabled for later use. This clock should be enabled >>>>>> and disabled along with the actual audio stream and not always on (that >>>>>> is bad for PM). Futhermore, this might cause sound glitches with some >>>>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled >>>>>> while the audio clock is still running. >>>>>> >>>>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls >>>>>> when the audio sample clock must be enabled or disabled. Then, it moves >>>>>> the call to this function into dw_hdmi_audio_enable() and >>>>>> dw_hdmi_audio_disable(). >>>>>> >>>>>> Signed-off-by: Romain Perier >>>>>> --- >>>>>> drivers/gpu/drm/bridge/dw-hdmi.c | 15 +-- >>>>>> 1 file changed, 9 insertions(+), 6 deletions(-) >>>>>> >>>>> Hi Romain, Russell, Jose, >>>>> >>>>> This is a little out of scope, but I was wondering why the CTS calculation >>>>> was not left in AUTO mode in the dw-hdmi driver ? >>>> There is no indication in the iMX6 manuals that the iMX6 supports >>>> automatic CTS calculation. Bits 7:4 of the AUD_CTS3 register are >>>> marked as "reserved". >>>> >>>> We're reliant on the information in the iMX6 manuals as we don't have >>>> access to Synopsis' databooks for these parts (I understand you have >>>> to be a customer to have access to that.) >>>> >>> (Synopsis -> Synopsys :)) >>> >>> Trying to catch up with the conversation: >>> >>> 1) In AHB audio mode the bits are always reserved. >>> 2) I think we should enable/disable clock instead of just forcing >>> N/CTS, though, I don't know what could be the implications for >>> iMX platform. I remember at the time I tested this using I2S >>> (I've never used AHB), and HDMI protocol analyzers were >>> complaining about the N/CTS being forced to zero. >> What you're recommending is to basically ignore the published workaround, >> which, presumably, was arrived at by proper analysis of the issue both >> by Freescale engineers and Synopsys engineers, and instead try some other >> solution. >> >> I'm not sure that's a good idea. Only those with knowledge of the IP can >> say what effect disabling the audio clock will have: will it still allow >> the FIFO to be loaded, as required by the errata? If not, it's not a >> solution that AHB can use. I would imagine that _if_ it was as simple as >> disabling the audio clock, that simple approach would have been documented >> in Freescale's errata documents, rather than the rather more complex >> method of zeroing the CTS/N values. > I'm just following what the user guide says: the final step of > configuration is enabling the audio clock. There is no reference > neither in datasheet neither in user guide about this behavior > but, as I said, I've never used AHB so I can't prove what is the > best solution. Could it be platform specific? And that's precisely why I am enabling it > >> I think there are two choices here: >> >> 1) get some definitive information about the IP and the errata for AHB, >>and determine whether the solution you propose would work. (That's >>out of scope for me, I've no contacts with FSL/NXP and I'm not a >>Synopsys customer.) > This is the right thing to do but if you can't test in the > Freescale platform then we have not much else to do. > > Best regards, > Jose Miguel Abreu > >> 2) the I2S audio support stops re-using the AHB audio support functions, >>switching to their own implementation that behaves correctly for them. >>(Remember, it was I2S's choice to re-use the AHB audio support functions >>I added which we're now discussing - they didn't have to do that, and >>regressing AHB audio just for I2S is not something we should ever >>consider doing.) >> The workaround looks AHB specific in any cases and does not seem to work with I2S. The I2S variant should not used the same functions, at least for enabling/disabling audio stream. Regards, Romain ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions
Hello,
Le 10/03/2017 à 12:15, Russell King - ARM Linux a écrit :
> On Fri, Mar 10, 2017 at 11:58:19AM +0100, Romain Perier wrote:
>> Hello,
>>
>> Le 10/03/2017 à 11:27, Russell King - ARM Linux a écrit :
>>> I also would not think that it's platform specific - remember that
>>> this is Designware IP, and it's likely that other platforms with
>>> exactly the same IP would suffer from the same problem. It's
>>> probably revision specific, but we need Synopsis' input on that.
>>>
>>> However, I do believe that your patch probably doesn't have much
>>> merit in any case: on a mode set, you enable the audio clock, and
>>> it will remain enabled until the audio device is first opened and
>>> then closed. If another mode set comes along, then exactly the
>>> same situation happens again. So, really it isn't achieving all
>>> that much.
>> The fact is we still have sound glitches caused by this workaround on
>> Rockchip, we probably have a revision
>> of the IP without this issue. If CTS+N is not forced to zero we no
>> longer have the glitches :/ (with or without touching the clock)
> Are you sure that removing the workaround just doesn't result in the
> same bug as on iMX6 appearing? The bug concerned is the ordering of
> the channels, so unless you're (eg0 monitoring left/right separately
> or directing audio to just one channel, you may not (with modern TVs)
> realise that the channels are swapped. I'll include the errata
> description and impact below.
>
> There are occasional issues on iMX6 as well despite this work-around,
> but I don't clearly remember what devmem2 tweaks I've done in the past
> to get it to resolve itself, nor could I describe them from memory
> any better than "burbling audio".
Well, I have reverted locally your patch (I did it quickly by hand
because it did not work via git revert... anyway...),
And I no longer have the issue at all. It's working fine all the time
and it's truly deterministic (see attachment)
Also, I have found that the version of the IP is not exactly the same.
On Rockchip it's 0x20:0xa:0xa0:0xc1, on IMX.6 it's 0x13:0xa:0xa0:0xc1.
>
>
> When the AHB Audio DMA is started, by setting to 1'b1 for the first
> time the register field AHB_DMA_START.data_buffer_ready, the AHB
> Audio DMA will request data from the AHB bus to fill its internal
> AHB DMA FIFO. It is possible that a AHB DMA FIFO read action occurs
> during the time window between the first sample stored on the AHB
> DMA FIFO and when the AHB DMA FIFO has stored, at least, the number
> of configured audio channels in samples. If this happens, the AHB
> DMA FIFO will reply with samples that are currently on the AHB
> Audio FIFO and will repeat the last sample after the empty condition
> is reached.
>
> Projected Impact:
> This will miss-align the audio stream, causing a shift in the
> distribution of audio on the channels on the HDMI sink side, with no
> knowledge of the DWC_hdmi_tx enabled system.
Thanks for this, it helps!
It looks specific to AHB audio
>
>
> If you know that this definitely does not apply to your version, then
> we need to split the audio enable/disable functions between the AHB
> and I2S variants.
Mhhh, yeah both seem to work differently.
Romain
diff --git a/drivers/gpu/drm/bridge/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/dw-hdmi-i2s-audio.c
index aaf287d..ce589402 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi-i2s-audio.c
@@ -67,8 +67,6 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void *data,
hdmi_write(audio, conf0, HDMI_AUD_CONF0);
hdmi_write(audio, conf1, HDMI_AUD_CONF1);
- dw_hdmi_audio_enable(hdmi);
-
return 0;
}
@@ -77,8 +75,6 @@ static void dw_hdmi_i2s_audio_shutdown(struct device *dev, void *data)
struct dw_hdmi_i2s_audio_data *audio = data;
struct dw_hdmi *hdmi = audio->hdmi;
- dw_hdmi_audio_disable(hdmi);
-
hdmi_write(audio, HDMI_AUD_CONF0_SW_RESET, HDMI_AUD_CONF0);
}
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index b621fc7..7529cb9 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -19,7 +19,6 @@
#include
#include
#include
-#include
#include
#include
@@ -148,12 +147,8 @@ struct dw_hdmi {
bool rxsense; /* rxsense state */
u8 phy_mask; /* desired phy int mask settings */
- spinlock_t audio_lock;
struct mutex audio_mutex;
unsigned int sample_rate;
- unsigned int audio_cts;
- unsigned int audio_n;
- bool audio_enable;
void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
u8 (*read)(struct dw_hdmi *hdmi, int offset)
Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions
Hello, Le 10/03/2017 à 10:46, Russell King - ARM Linux a écrit : > On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote: >> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at >> step E. and is kept enabled for later use. This clock should be enabled >> and disabled along with the actual audio stream and not always on (that >> is bad for PM). Futhermore, this might cause sound glitches with some >> HDMI devices, as the CTS+N is forced to zero when the stream is disabled >> while the audio clock is still running. >> >> This commit adds a parameter to hdmi_audio_enable_clk() that controls >> when the audio sample clock must be enabled or disabled. Then, it moves >> the call to this function into dw_hdmi_audio_enable() and >> dw_hdmi_audio_disable(). > How does this interact with the workaround given in my commit introducing > these functions? (Commit b90120a96608). > > Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be > loaded with data prior to setting N non-zero. If disabling the audio > clock prevents the audio FIFO being loaded, your patch will break iMX6. > Mhhh, the fact is I have no IMX6 devices here (only Rockchip). So I only tested on Rockchip devices. An approach might be to introduce an option for handling this errata, because that's platform specific and other platforms (like Rockchip) are in conflict with this. Romain ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions
Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
step E. and is kept enabled for later use. This clock should be enabled
and disabled along with the actual audio stream and not always on (that
is bad for PM). Futhermore, this might cause sound glitches with some
HDMI devices, as the CTS+N is forced to zero when the stream is disabled
while the audio clock is still running.
This commit adds a parameter to hdmi_audio_enable_clk() that controls
when the audio sample clock must be enabled or disabled. Then, it moves
the call to this function into dw_hdmi_audio_enable() and
dw_hdmi_audio_disable().
Signed-off-by: Romain Perier
---
drivers/gpu/drm/bridge/dw-hdmi.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index b621fc7..5b6090c 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -537,6 +537,12 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi,
unsigned int rate)
}
EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
+static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi, bool enable)
+{
+ hdmi_modb(hdmi, enable ? 0 : HDMI_MC_CLKDIS_AUDCLK_DISABLE,
+ HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
+}
+
void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
{
unsigned long flags;
@@ -544,6 +550,7 @@ void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
spin_lock_irqsave(&hdmi->audio_lock, flags);
hdmi->audio_enable = true;
hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+ hdmi_enable_audio_clk(hdmi, true);
spin_unlock_irqrestore(&hdmi->audio_lock, flags);
}
EXPORT_SYMBOL_GPL(dw_hdmi_audio_enable);
@@ -554,6 +561,7 @@ void dw_hdmi_audio_disable(struct dw_hdmi *hdmi)
spin_lock_irqsave(&hdmi->audio_lock, flags);
hdmi->audio_enable = false;
+ hdmi_enable_audio_clk(hdmi, false);
hdmi_set_cts_n(hdmi, hdmi->audio_cts, 0);
spin_unlock_irqrestore(&hdmi->audio_lock, flags);
}
@@ -1343,11 +1351,6 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi
*hdmi)
}
}
-static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi)
-{
- hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
-}
-
/* Workaround to clear the overflow condition */
static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi)
{
@@ -1430,7 +1433,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct
drm_display_mode *mode)
/* HDMI Initialization Step E - Configure audio */
hdmi_clk_regenerator_update_pixel_clock(hdmi);
- hdmi_enable_audio_clk(hdmi);
+ hdmi_enable_audio_clk(hdmi, true);
}
/* not for DVI mode */
--
2.9.3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm: dw_hdmi: Don't rely on the status of the bridge for updating HPD
Currently, the irq handler that monitores changes for HPD anx RX_SENSE
relies on the status of the bridge for updating the status of the HPD.
The update is done only when the bridge is enabled.
However, on Rockchip platforms we have found use cases where it could be
a problem. When HDMI is being used, turning off/on the screen or
unplugging/re-plugging the cable, the following simplified code path
will happen:
- dw_hdmi_irq() will be triggered by an HPD event, as the bridge is on
hdmi->disabled is false, then the handler will update the rxsense flag
accordingly.
- dw_hdmi_update_power() will be invoked with the mode
DRM_FORCE_UNSPECIFIED and rxsense == 1, so dw_hdmi_poweroff() will be
called and the PHY will be desactivated (its pixel clocks and TMDS)
[...]
- dw_hdmi_bridge_disable() will be invoked, the bridge will be marked as
disabled.
- dw_hdmi_irq() will be triggered by an HPD event, as the bridge is
currently disabled the HPD status won't be updated, so hdmi->rxsense
won't be changed. Even if the data part of the PHY is disabled, this
information coming from the HDMI Transmitter is correct and should be
saved.
[...]
- dw_hdmi_bridge_enable() will be invoked, the bridge will be marked as
enabled.
- dw_hdmi_update_power() will be called. When hdmi->force is equal to
DRM_FORCE_UNSPECIFIED the function will rely on hdmi->rxsense. If this
field has not been updated by the irq handler, it will be false and
DRM_FORCE_ON won't be put to hdmi->force.
Consequently, most of the time dw_hdmi_poweron() won't be called in this
use case, TMDS won't be re-enabled the PHY won't be re-initialized,
resulting in a "Signal not found".
This commit fixes the issue by removing the check for "!hdmi->disabled".
As already explained, even if the PHY is partially disabled, information
coming from HDMI Transmitter about HPD should be saved for a later use.
Signed-off-by: Romain Perier
---
Changes in v2:
- I have re-worded the commit message, as suggested by Russel
drivers/gpu/drm/bridge/dw-hdmi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 3a70016..5b6090c 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1793,7 +1793,7 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
if (intr_stat &
(HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) {
mutex_lock(&hdmi->mutex);
- if (!hdmi->disabled && !hdmi->force) {
+ if (!hdmi->force) {
/*
* If the RX sense status indicates we're disconnected,
* clear the software rxsense status.
--
2.9.3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions
Hello, Le 10/03/2017 à 11:27, Russell King - ARM Linux a écrit : > On Fri, Mar 10, 2017 at 11:21:53AM +0100, Romain Perier wrote: >> Hello, >> >> Le 10/03/2017 à 10:46, Russell King - ARM Linux a écrit : >>> On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote: >>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at >>>> step E. and is kept enabled for later use. This clock should be enabled >>>> and disabled along with the actual audio stream and not always on (that >>>> is bad for PM). Futhermore, this might cause sound glitches with some >>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled >>>> while the audio clock is still running. >>>> >>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls >>>> when the audio sample clock must be enabled or disabled. Then, it moves >>>> the call to this function into dw_hdmi_audio_enable() and >>>> dw_hdmi_audio_disable(). >>> How does this interact with the workaround given in my commit introducing >>> these functions? (Commit b90120a96608). >>> >>> Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be >>> loaded with data prior to setting N non-zero. If disabling the audio >>> clock prevents the audio FIFO being loaded, your patch will break iMX6. >>> >> Mhhh, the fact is I have no IMX6 devices here (only Rockchip). So >> I only tested on Rockchip devices. An approach might be to introduce an >> option for handling this errata, because that's platform specific and >> other platforms (like Rockchip) are in conflict with this. > or are you saying > that the I2S driver was never functionally tested as working? No ^^ > > I also would not think that it's platform specific - remember that > this is Designware IP, and it's likely that other platforms with > exactly the same IP would suffer from the same problem. It's > probably revision specific, but we need Synopsis' input on that. > > However, I do believe that your patch probably doesn't have much > merit in any case: on a mode set, you enable the audio clock, and > it will remain enabled until the audio device is first opened and > then closed. If another mode set comes along, then exactly the > same situation happens again. So, really it isn't achieving all > that much. > The fact is we still have sound glitches caused by this workaround on Rockchip, we probably have a revision of the IP without this issue. If CTS+N is not forced to zero we no longer have the glitches :/ (with or without touching the clock) Romain ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RESEND] drm: dw_hdmi: Don't rely on the status of the bridge for updating HPD
Hello,
Le 09/03/2017 à 15:28, Jose Abreu a écrit :
> Hi Romain,
>
>
> On 08-03-2017 08:15, Romain Perier wrote:
>> Currently, the irq handler that monitores changes for HPD anx RX_SENSE
>> relies on the status of the bridge for updating the status of the HPD.
>> The update is done only when the bridge is enabled.
>>
>> However, on Rockchip platforms we have found use cases where it could be
>> a problem. When HDMI is being used, turning off/on the screen or
>> unplugging/re-plugging the cable, the following simplified code path
>> will happen:
>>
>> - dw_hdmi_irq() will be triggered by an HPD event, as the bridge is on
>> hdmi->disabled is false, then the handler will update the rxsense flag
>> accordingly.
>> - dw_hdmi_update_power() will be invoked with the mode
>> DRM_FORCE_UNSPECIFIED and rxsense == 1, so dw_hdmi_poweroff() will be
>> called and the PHY will be desactivated (its pixel clocks and TMDS)
>>
>> [...]
>>
>> - dw_hdmi_bridge_disable() will be invoked, the bridge will be marked as
>> disabled.
>>
>> - dw_hdmi_irq() will be triggered by an HPD event, as the bridge is
>> currently disabled the HPD status won't be updated, so hdmi->rxsense
>> won't be changed. Even if the data part of the PHY is disabled, this
>> information coming from the HDMI Transmitter is correct and should be
>> saved.
>>
>> [...]
>>
>> - dw_hdmi_bridge_enable() will be invoked, the bridge will be marked as
>> enabled.
>> - dw_hdmi_update_power() will be called. As hdmi->force will be equal to
>> DRM_FORCE_UNSPECIFIED the function will rely on hdmi->rxsense. This
>> field has not been updated by the irq handler, so it will be false and
>> DRM_FORCE_ON won't be put to hdmi->force.
>>
>> Consequently, most of the time dw_hdmi_poweron() won't be called in this
>> use case, TMDS won't be re-enabled the PHY won't be re-initialized,
>> resulting in a "Signal not found".
>>
>> This commit fixes the issue by removing the check for "!hdmi->disabled".
>> As already explained, even if the PHY is partially disabled, information
>> coming from HDMI Transmitter about HPD should be saved for a later use.
>>
>> Signed-off-by: Romain Perier
>> ---
>>
>> Note: Due to an email configuration issue, some of my patches were not
>> received on infradead.org or vger.kernel.org. It is now fixed, so I
>> resend this patch for this reason.
>>
>> drivers/gpu/drm/bridge/dw-hdmi.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
>> b/drivers/gpu/drm/bridge/dw-hdmi.c
>> index 235ce7d..b621fc7 100644
>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>> @@ -1790,7 +1790,7 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>> if (intr_stat &
>> (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) {
>> mutex_lock(&hdmi->mutex);
>> -if (!hdmi->disabled && !hdmi->force) {
>> +if (!hdmi->force) {
>> /*
>> * If the RX sense status indicates we're disconnected,
>> * clear the software rxsense status.
> Makes sense but I think you need to make sure that phy will not
> be enabled when bridge is disabled i.e. you should update rxsense
> only.
>
> Best regards,
> Jose Miguel Abreu
I agree with Russel, dw_hdmi_update_phy_mask already checks that. So it
should not be enabled, imho.
Regards,
Romain
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH RESEND] drm: dw_hdmi: Don't rely on the status of the bridge for updating HPD
Currently, the irq handler that monitores changes for HPD anx RX_SENSE
relies on the status of the bridge for updating the status of the HPD.
The update is done only when the bridge is enabled.
However, on Rockchip platforms we have found use cases where it could be
a problem. When HDMI is being used, turning off/on the screen or
unplugging/re-plugging the cable, the following simplified code path
will happen:
- dw_hdmi_irq() will be triggered by an HPD event, as the bridge is on
hdmi->disabled is false, then the handler will update the rxsense flag
accordingly.
- dw_hdmi_update_power() will be invoked with the mode
DRM_FORCE_UNSPECIFIED and rxsense == 1, so dw_hdmi_poweroff() will be
called and the PHY will be desactivated (its pixel clocks and TMDS)
[...]
- dw_hdmi_bridge_disable() will be invoked, the bridge will be marked as
disabled.
- dw_hdmi_irq() will be triggered by an HPD event, as the bridge is
currently disabled the HPD status won't be updated, so hdmi->rxsense
won't be changed. Even if the data part of the PHY is disabled, this
information coming from the HDMI Transmitter is correct and should be
saved.
[...]
- dw_hdmi_bridge_enable() will be invoked, the bridge will be marked as
enabled.
- dw_hdmi_update_power() will be called. As hdmi->force will be equal to
DRM_FORCE_UNSPECIFIED the function will rely on hdmi->rxsense. This
field has not been updated by the irq handler, so it will be false and
DRM_FORCE_ON won't be put to hdmi->force.
Consequently, most of the time dw_hdmi_poweron() won't be called in this
use case, TMDS won't be re-enabled the PHY won't be re-initialized,
resulting in a "Signal not found".
This commit fixes the issue by removing the check for "!hdmi->disabled".
As already explained, even if the PHY is partially disabled, information
coming from HDMI Transmitter about HPD should be saved for a later use.
Signed-off-by: Romain Perier
---
Note: Due to an email configuration issue, some of my patches were not
received on infradead.org or vger.kernel.org. It is now fixed, so I
resend this patch for this reason.
drivers/gpu/drm/bridge/dw-hdmi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 235ce7d..b621fc7 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1790,7 +1790,7 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
if (intr_stat &
(HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) {
mutex_lock(&hdmi->mutex);
- if (!hdmi->disabled && !hdmi->force) {
+ if (!hdmi->force) {
/*
* If the RX sense status indicates we're disconnected,
* clear the software rxsense status.
--
2.9.3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/rockchip: vop: Correct enabled clocks during setup
Hi all,
I had an issue with the drm driver, when I start the kernel via kexec,
the drm driver freezes the platform. it is caused by the function
vop_initial (when copying registers via ahb). This issue happens with
the chromeos kernel or with the mainline kernel. When I investigated a
bit, I found that it had something to do with the hclk clock.
This patch fixes the issue. Everything works like a charm now.
Tested-by: Romain Perier
2015-09-29 13:58 GMT+02:00 Sjoerd Simons :
> On Tue, 2015-09-29 at 18:58 +0800, Yakir Yang wrote:
>>
>> On 09/29/2015 05:55 PM, Yakir Yang wrote:
>> >
>> >
>> > On 09/29/2015 05:28 PM, Sjoerd Simons wrote:
>> > > When doing the initial setup both the hclk and the aclk need to
>> > > be
>> > > enabled otherwise the board will simply hang. This only occurs
>> > > when
>> > > building the vop driver as a module, when its built-in the
>> > > initial setup
>>
>> Hmm... My previous test was built-in the vop driver, and just notice
>> that
>> you say problem only occurred when building the vop driver as module.
>> That's to say my test was wrong, so I try to do the right things.
>>
>> But I found that vop driver module and rockchipdrm driver module in
>> dependency cycles, here are the build message:
>> depmod: ERROR: Found 2 modules in dependency cycles!
>
>
> I've only tested with mainline which doesn't seem to have that issue?
> So can't easily help you there unfortunately.
>
>
>> Thanks,
>> - Yakir
>>
>> > > happens to run before the clock framework shuts of unused clocks
>> > > (including the aclk).
>> > >
>> > > While there also switch to doing prepare and enable in one step
>> > > rather
>> > > then separate steps to reduce the amount of code required.
>> > >
>> > > Signed-off-by: Sjoerd Simons
>> >
>> > Looks good and test on chromeos-3.14 tree, no problem, so
>> >
>> > Tested-by: Yakir Yang
>> >
>> > > ---
>> > >
>> > > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 36
>> > > +++--
>> > > 1 file changed, 14 insertions(+), 22 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> > > b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> > > index 5d8ae5e..48719df 100644
>> > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> > > @@ -1575,32 +1575,25 @@ static int vop_initial(struct vop *vop)
>> > > return PTR_ERR(vop->dclk);
>> > > }
>> > > -ret = clk_prepare(vop->hclk);
>> > > -if (ret < 0) {
>> > > -dev_err(vop->dev, "failed to prepare hclk\n");
>> > > -return ret;
>> > > -}
>> > > -
>> > > ret = clk_prepare(vop->dclk);
>> > > if (ret < 0) {
>> > > dev_err(vop->dev, "failed to prepare dclk\n");
>> > > -goto err_unprepare_hclk;
>> > > +return ret;
>> > > }
>> > > -ret = clk_prepare(vop->aclk);
>> > > +/* Enable both the hclk and aclk to setup the vop */
>> > > +ret = clk_prepare_enable(vop->hclk);
>> > > if (ret < 0) {
>> > > -dev_err(vop->dev, "failed to prepare aclk\n");
>> > > +dev_err(vop->dev, "failed to prepare/enable hclk\n");
>> > > goto err_unprepare_dclk;
>> > > }
>> > > -/*
>> > > - * enable hclk, so that we can config vop register.
>> > > - */
>> > > -ret = clk_enable(vop->hclk);
>> > > +ret = clk_prepare_enable(vop->aclk);
>> > > if (ret < 0) {
>> > > -dev_err(vop->dev, "failed to prepare aclk\n");
>> > > -goto err_unprepare_aclk;
>> > > +dev_err(vop->dev, "failed to prepare/enable aclk\n");
>> > > +goto err_disable_hclk;
>> > > }
>> > > +
>> > > /*
>> > >* do hclk_reset, reset all vop registers.
>> > >*/
>> > > @@ -1608,7 +1601,7 @@ static int vop_initial(struct vop *vop)
>> > > if (IS_ERR(ahb_rst)) {
>> > > dev_e
[PATCH v3 0/14] Add Analogix Core Display Port Driver
Hi, Could you rebase your serie onto linux-next or 4.2-rc8 ? it does not apply here... Regards, Romain 2015-08-21 15:16 GMT+02:00 Thierry Reding : > On Fri, Aug 21, 2015 at 08:24:16PM +0900, Jingoo Han wrote: >> On 2015. 8. 21., at PM 7:01, Yakir Yang wrote: >> > >> > Hi Jingoo, >> > >> >> å¨ 2015/8/21 16:20, Jingoo Han åé: >> >>> On 2015. 8. 19., at PM 11:48, Yakir Yang wrote: >> >> >> >> . >> >> >> >>> .../bindings/video/analogix_dp-rockchip.txt| 83 ++ >> >>> .../devicetree/bindings/video/exynos_dp.txt| 51 +- >> >>> arch/arm/boot/dts/exynos5250-arndale.dts | 10 +- >> >>> arch/arm/boot/dts/exynos5250-smdk5250.dts | 10 +- >> >>> arch/arm/boot/dts/exynos5250-snow.dts | 12 +- >> >>> arch/arm/boot/dts/exynos5250-spring.dts| 12 +- >> >>> arch/arm/boot/dts/exynos5420-peach-pit.dts | 12 +- >> >>> arch/arm/boot/dts/exynos5420-smdk5420.dts | 10 +- >> >>> arch/arm/boot/dts/exynos5800-peach-pi.dts | 12 +- >> >>> drivers/gpu/drm/bridge/Kconfig |5 + >> >>> drivers/gpu/drm/bridge/Makefile|1 + >> >>> drivers/gpu/drm/bridge/analogix_dp_core.c | 1382 >> >>> +++ >> >>> drivers/gpu/drm/bridge/analogix_dp_core.h | 286 >> >>> drivers/gpu/drm/bridge/analogix_dp_reg.c | 1294 >> >>> ++ >> >>> .../exynos_dp_reg.h => bridge/analogix_dp_reg.h} | 270 ++-- >> >>> drivers/gpu/drm/exynos/Kconfig |5 +- >> >>> drivers/gpu/drm/exynos/Makefile|2 +- >> >>> drivers/gpu/drm/exynos/analogix_dp-exynos.c| 347 + >> >> Would you change this file name to "exynos_dp.c"? >> > >> > Sorry, I don't think so ;( >> > >> > I think IP_name+Soc_name would be better in this re-use case. >> >> So? Is there the naming rule such as "IP_name+SoC_name"? >> >> > Beside I see >> > there are lots of driver named with this format in kernel, such as dw_hdmi >> > & dw_mmc >> >> Please look at other dw cases. >> For example, look at dw_pcie. >> >> drivers/pci/host/ >> pcie-designware.c >> pci-spear13xx.c >> pci-exynos.c >> >> In this case, pci-spear13xx.c and pci-exynos.c do not use >> "IP_name+SoC_name", even though these are dw IPs. >> >> Also, naming consistency is more important. >> Now, Exynos DRM files are using "exynos_drm_" prefix. >> >> drivers/gpu/drm/exynos/ >> exynos_drm_buf.c >> exynos_drm_core.c >> >> >> However, "analogix_dp-exynos.c" looks very inconsistent. >> >> If there is no strict naming rule, please use "exynos_dp.c" >> or "exynos_drm_dp.c". > > Exynos DRM maintainers get to pick their filenames, so Yakir, please > rename as Jingoo suggested. > > Even if you didn't the first thing that would go into the Exynos DRM > driver tree after this is merged is a rename patch anyway. > > Thierry
