Re: [Freedreno] [PATCH v2 00/13] drm/hdcp: Pull HDCP auth/exchange/check into helpers
Hi Sean On 2021-09-15 13:38, Sean Paul wrote: From: Sean Paul Hello again, This is the second version of the HDCP helper patchset. See version 1 here: https://patchwork.freedesktop.org/series/94623/ In this second version, I've fixed up the oopsies exposed by 0-day and yamllint and incorporated early review feedback from the dt/dts reviews. Please take a look, Sean One question overall on the series: 1) Regarding validation, did you run any secure video to check the transitions? 2) Is running HDCP 1x compliance also part of the validation efforts? Thanks Abhinav Sean Paul (13): drm/hdcp: Add drm_hdcp_atomic_check() drm/hdcp: Avoid changing crtc state in hdcp atomic check drm/hdcp: Update property value on content type and user changes drm/hdcp: Expand HDCP helper library for enable/disable/check drm/i915/hdcp: Consolidate HDCP setup/state cache drm/i915/hdcp: Retain hdcp_capable return codes drm/i915/hdcp: Use HDCP helpers for i915 drm/msm/dpu_kms: Re-order dpu includes drm/msm/dpu: Remove useless checks in dpu_encoder drm/msm/dpu: Remove encoder->enable() hack drm/msm/dp: Re-order dp_audio_put in deinit_sub_modules dt-bindings: msm/dp: Add bindings for HDCP registers drm/msm: Implement HDCP 1.x using the new drm HDCP helpers .../bindings/display/msm/dp-controller.yaml |7 +- arch/arm64/boot/dts/qcom/sc7180.dtsi |4 +- drivers/gpu/drm/drm_hdcp.c| 1197 - drivers/gpu/drm/i915/display/intel_atomic.c |7 +- drivers/gpu/drm/i915/display/intel_ddi.c | 29 +- .../drm/i915/display/intel_display_debugfs.c | 11 +- .../drm/i915/display/intel_display_types.h| 58 +- drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 345 ++--- drivers/gpu/drm/i915/display/intel_dp_mst.c | 17 +- drivers/gpu/drm/i915/display/intel_hdcp.c | 1011 +++--- drivers/gpu/drm/i915/display/intel_hdcp.h | 35 +- drivers/gpu/drm/i915/display/intel_hdmi.c | 256 ++-- drivers/gpu/drm/msm/Makefile |1 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 30 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |2 - drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h |4 - drivers/gpu/drm/msm/dp/dp_debug.c | 49 +- drivers/gpu/drm/msm/dp/dp_debug.h |6 +- drivers/gpu/drm/msm/dp/dp_display.c | 47 +- drivers/gpu/drm/msm/dp/dp_display.h |5 + drivers/gpu/drm/msm/dp/dp_drm.c | 68 +- drivers/gpu/drm/msm/dp/dp_drm.h |5 + drivers/gpu/drm/msm/dp/dp_hdcp.c | 433 ++ drivers/gpu/drm/msm/dp/dp_hdcp.h | 27 + drivers/gpu/drm/msm/dp/dp_parser.c| 22 +- drivers/gpu/drm/msm/dp/dp_parser.h|4 + drivers/gpu/drm/msm/dp/dp_reg.h | 44 +- drivers/gpu/drm/msm/msm_atomic.c | 15 + include/drm/drm_hdcp.h| 194 +++ 30 files changed, 2561 insertions(+), 1389 deletions(-) create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.c create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.h
Re: [Freedreno] [PATCH v2 08/13] drm/msm/dpu_kms: Re-order dpu includes
On 2021-09-15 13:38, Sean Paul wrote: From: Sean Paul Make includes alphabetical in dpu_kms.c Signed-off-by: Sean Paul Reviewed-by: Abhinav Kumar Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-9-s...@poorly.run #v1 Changes in v2: -None --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index ae48f41821cf..fb0d9f781c66 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -21,14 +21,14 @@ #include "msm_gem.h" #include "disp/msm_disp_snapshot.h" -#include "dpu_kms.h" #include "dpu_core_irq.h" +#include "dpu_crtc.h" +#include "dpu_encoder.h" #include "dpu_formats.h" #include "dpu_hw_vbif.h" -#include "dpu_vbif.h" -#include "dpu_encoder.h" +#include "dpu_kms.h" #include "dpu_plane.h" -#include "dpu_crtc.h" +#include "dpu_vbif.h" #define CREATE_TRACE_POINTS #include "dpu_trace.h"
Re: [Freedreno] [PATCH v2 13/13] drm/msm: Implement HDCP 1.x using the new drm HDCP helpers
On 2021-09-15 13:38, Sean Paul wrote:
From: Sean Paul
This patch adds HDCP 1.x support to msm DP connectors using the new
HDCP
helpers.
Cc: Stephen Boyd
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-15-s...@poorly.run
#v1
Changes in v2:
-Squash [1] into this patch with the following changes (Stephen)
-Update the sc7180 dtsi file
-Remove resource names and just use index (Stephen)
[1]
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-14-s...@poorly.run
---
arch/arm64/boot/dts/qcom/sc7180.dtsi | 4 +-
drivers/gpu/drm/msm/Makefile | 1 +
drivers/gpu/drm/msm/dp/dp_debug.c| 49 ++-
drivers/gpu/drm/msm/dp/dp_debug.h| 6 +-
drivers/gpu/drm/msm/dp/dp_display.c | 45 ++-
drivers/gpu/drm/msm/dp/dp_display.h | 5 +
drivers/gpu/drm/msm/dp/dp_drm.c | 68 -
drivers/gpu/drm/msm/dp/dp_drm.h | 5 +
drivers/gpu/drm/msm/dp/dp_hdcp.c | 433 +++
drivers/gpu/drm/msm/dp/dp_hdcp.h | 27 ++
drivers/gpu/drm/msm/dp/dp_parser.c | 22 +-
drivers/gpu/drm/msm/dp/dp_parser.h | 4 +
drivers/gpu/drm/msm/dp/dp_reg.h | 44 ++-
drivers/gpu/drm/msm/msm_atomic.c | 15 +
14 files changed, 709 insertions(+), 19 deletions(-)
create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.c
create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.h
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index c8921e2d6480..3ae6fc7a2c01 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -3088,7 +3088,9 @@ mdss_dp: displayport-controller@ae9 {
compatible = "qcom,sc7180-dp";
status = "disabled";
- reg = <0 0x0ae9 0 0x1400>;
+ reg = <0 0x0ae9 0 0x1400>,
+ <0 0x0aed1000 0 0x174>,
+ <0 0x0aee1000 0 0x2c>;
interrupt-parent = <>;
interrupts = <12>;
diff --git a/drivers/gpu/drm/msm/Makefile
b/drivers/gpu/drm/msm/Makefile
index 904535eda0c4..98731fd262d6 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -109,6 +109,7 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \
dp/dp_ctrl.o \
dp/dp_display.o \
dp/dp_drm.o \
+ dp/dp_hdcp.o \
dp/dp_hpd.o \
dp/dp_link.o \
dp/dp_panel.o \
diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c
b/drivers/gpu/drm/msm/dp/dp_debug.c
index 2f6247e80e9d..de16fca8782a 100644
--- a/drivers/gpu/drm/msm/dp/dp_debug.c
+++ b/drivers/gpu/drm/msm/dp/dp_debug.c
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
#include "dp_parser.h"
#include "dp_catalog.h"
@@ -15,6 +16,7 @@
#include "dp_ctrl.h"
#include "dp_debug.h"
#include "dp_display.h"
+#include "dp_hdcp.h"
#define DEBUG_NAME "msm_dp"
@@ -24,6 +26,7 @@ struct dp_debug_private {
struct dp_usbpd *usbpd;
struct dp_link *link;
struct dp_panel *panel;
+ struct dp_hdcp *hdcp;
struct drm_connector **connector;
struct device *dev;
struct drm_device *drm_dev;
@@ -349,6 +352,38 @@ static int dp_test_active_open(struct inode
*inode,
inode->i_private);
}
+static ssize_t dp_hdcp_key_write(struct file *file, const char __user
*ubuf,
+size_t len, loff_t *offp)
+{
+ char *input_buffer;
+ int ret = 0;
+ struct dp_debug_private *debug = file->private_data;
+ struct drm_device *dev;
+
+ dev = debug->drm_dev;
+
+ if (len != (DRM_HDCP_KSV_LEN + DP_HDCP_NUM_KEYS * DP_HDCP_KEY_LEN))
+ return -EINVAL;
+
+ if (!debug->hdcp)
+ return -ENOENT;
+
+ input_buffer = memdup_user_nul(ubuf, len);
+ if (IS_ERR(input_buffer))
+ return PTR_ERR(input_buffer);
+
+ ret = dp_hdcp_ingest_key(debug->hdcp, input_buffer, len);
+
+ kfree(input_buffer);
+ if (ret < 0) {
+ DRM_ERROR("Could not ingest HDCP key, ret=%d\n", ret);
+ return ret;
+ }
+
+ *offp += len;
+ return len;
+}
It seems like the HDCP keys written using debugfs, just for my
understanding,
are you storing this in some secure partition and the usermode reads
from it
and writes them here?
+
static const struct file_operations dp_debug_fops = {
.open = simple_open,
.read = dp_debug_read_info,
@@ -363,6 +398,12 @@ static const struct file_operations
test_active_fops = {
.write = dp_test_active_write
};
+static const struct file_operations dp_hdcp_key_fops = {
+ .owner = THIS_MODULE,
+ .open = simple_open,
+ .write = dp_hdcp_key_write,
+};
+
static int dp_debug_init(struct dp_debug *dp_debug, struct drm_minor
*minor)
{
int rc = 0;
@@
Re: [Freedreno] [PATCH v2 04/13] drm/hdcp: Expand HDCP helper library for enable/disable/check
On 2021-09-15 13:38, Sean Paul wrote:
From: Sean Paul
This patch expands upon the HDCP helper library to manage HDCP
enable, disable, and check.
Previous to this patch, the majority of the state management and sink
interaction is tucked inside the Intel driver with the understanding
that once a new platform supported HDCP we could make good decisions
about what should be centralized. With the addition of HDCP support
for Qualcomm, it's time to migrate the protocol-specific bits of HDCP
authentication, key exchange, and link checks to the HDCP helper.
In terms of functionality, this migration is 1:1 with the Intel driver,
however things are laid out a bit differently than with intel_hdcp.c,
which is why this is a separate patch from the i915 transition to the
helper. On i915, the "shim" vtable is used to account for HDMI vs. DP
vs. DP-MST differences whereas the helper library uses a LUT to
account for the register offsets and a remote read function to route
the messages. On i915, storing the sink information in the source is
done inline whereas now we use the new drm_hdcp_helper_funcs vtable
to store and fetch information to/from source hw. Finally, instead of
calling enable/disable directly from the driver, we'll leave that
decision to the helper and by calling drm_hdcp_helper_atomic_commit()
from the driver. All told, this will centralize the protocol and state
handling in the helper, ensuring we collect all of our bugs^Wlogic
in one place.
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-5-s...@poorly.run
#v1
Changes in v2:
-Fixed set-but-unused variable identified by 0-day
---
drivers/gpu/drm/drm_hdcp.c | 1103
include/drm/drm_hdcp.h | 191 +++
2 files changed, 1294 insertions(+)
diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
index 742313ce8f6f..47c6e6923a76 100644
--- a/drivers/gpu/drm/drm_hdcp.c
+++ b/drivers/gpu/drm/drm_hdcp.c
@@ -6,15 +6,20 @@
* Ramalingam C
*/
+#include
#include
#include
#include
+#include
+#include
#include
#include
#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -513,3 +518,1101 @@ bool drm_hdcp_atomic_check(struct drm_connector
*connector,
return old_hdcp != new_hdcp;
}
EXPORT_SYMBOL(drm_hdcp_atomic_check);
+
+struct drm_hdcp_helper_data {
+ struct mutex mutex;
+ struct mutex *driver_mutex;
+
+ struct drm_connector *connector;
+ const struct drm_hdcp_helper_funcs *funcs;
+
+ u64 value;
+ unsigned int enabled_type;
+
+ struct delayed_work check_work;
+ struct work_struct prop_work;
+
+ struct drm_dp_aux *aux;
+ const struct drm_hdcp_hdcp1_receiver_reg_lut *hdcp1_lut;
+};
+
+struct drm_hdcp_hdcp1_receiver_reg_lut {
+ unsigned int bksv;
+ unsigned int ri;
+ unsigned int aksv;
+ unsigned int an;
+ unsigned int ainfo;
+ unsigned int v[5];
+ unsigned int bcaps;
+ unsigned int bcaps_mask_repeater_present;
+ unsigned int bstatus;
+};
+
+static const struct drm_hdcp_hdcp1_receiver_reg_lut
drm_hdcp_hdcp1_ddc_lut = {
+ .bksv = DRM_HDCP_DDC_BKSV,
+ .ri = DRM_HDCP_DDC_RI_PRIME,
+ .aksv = DRM_HDCP_DDC_AKSV,
+ .an = DRM_HDCP_DDC_AN,
+ .ainfo = DRM_HDCP_DDC_AINFO,
+ .v = { DRM_HDCP_DDC_V_PRIME(0), DRM_HDCP_DDC_V_PRIME(1),
+ DRM_HDCP_DDC_V_PRIME(2), DRM_HDCP_DDC_V_PRIME(3),
+ DRM_HDCP_DDC_V_PRIME(4) },
+ .bcaps = DRM_HDCP_DDC_BCAPS,
+ .bcaps_mask_repeater_present = DRM_HDCP_DDC_BCAPS_REPEATER_PRESENT,
+ .bstatus = DRM_HDCP_DDC_BSTATUS,
+};
+
+static const struct drm_hdcp_hdcp1_receiver_reg_lut
drm_hdcp_hdcp1_dpcd_lut = {
+ .bksv = DP_AUX_HDCP_BKSV,
+ .ri = DP_AUX_HDCP_RI_PRIME,
+ .aksv = DP_AUX_HDCP_AKSV,
+ .an = DP_AUX_HDCP_AN,
+ .ainfo = DP_AUX_HDCP_AINFO,
+ .v = { DP_AUX_HDCP_V_PRIME(0), DP_AUX_HDCP_V_PRIME(1),
+ DP_AUX_HDCP_V_PRIME(2), DP_AUX_HDCP_V_PRIME(3),
+ DP_AUX_HDCP_V_PRIME(4) },
+ .bcaps = DP_AUX_HDCP_BCAPS,
+ .bcaps_mask_repeater_present = DP_BCAPS_REPEATER_PRESENT,
+
+ /*
+* For some reason the HDMI and DP HDCP specs call this register
+ * definition by different names. In the HDMI spec, it's called
BSTATUS,
+* but in DP it's called BINFO.
+*/
+ .bstatus = DP_AUX_HDCP_BINFO,
+};
+
+static int drm_hdcp_remote_ddc_read(struct i2c_adapter *i2c,
+ unsigned int offset, u8 *value, size_t len)
+{
+ int ret;
+ u8 start = offset & 0xff;
+ struct i2c_msg msgs[] = {
+ {
+ .addr = DRM_HDCP_DDC_ADDR,
+ .flags = 0,
+ .len = 1,
+ .buf = ,
+ },
+ {
+ .addr = DRM_HDCP_DDC_ADDR,
+
Re: [Freedreno] [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support
On Tue, Sep 21, 2021 at 3:20 PM Doug Anderson wrote: > > Hi, > > On Mon, Sep 20, 2021 at 3:53 PM Rob Clark wrote: > > > > From: Rob Clark > > > > Slightly awkward to fish out the display_info when we aren't creating > > own connector. But I don't see an obvious better way. > > > > v2: Remove error return with NO_CONNECTOR flag > > > > Signed-off-by: Rob Clark > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 --- > > 1 file changed, 29 insertions(+), 10 deletions(-) > > This seems fine to me: > > Reviewed-by: Douglas Anderson > > ...if you would like me to apply patch #2 / #3 to drm-misc-next then > please yell. Thanks.. I think we can give it a few days for Laurent to have a look. This will conflict some with Maxime's bridge vs dsi-host ordering series.. not sure how close that one is to the finish line, but I can rebase either patch on top of the other depending on which order they are applied BR, -R
Re: [Freedreno] [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support
Hi, On Mon, Sep 20, 2021 at 3:53 PM Rob Clark wrote: > > From: Rob Clark > > Slightly awkward to fish out the display_info when we aren't creating > own connector. But I don't see an obvious better way. > > v2: Remove error return with NO_CONNECTOR flag > > Signed-off-by: Rob Clark > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 --- > 1 file changed, 29 insertions(+), 10 deletions(-) This seems fine to me: Reviewed-by: Douglas Anderson ...if you would like me to apply patch #2 / #3 to drm-misc-next then please yell. -Doug
Re: [Freedreno] [PATCH] drm/msm/dsi: do not install irq handler before power up the host
On 2021-09-21 10:47, Dmitry Baryshkov wrote:
Hi,
On Tue, 21 Sept 2021 at 20:01, wrote:
On 2021-09-21 09:22, Dmitry Baryshkov wrote:
> The DSI host might be left in some state by the bootloader. If this
> state generates an IRQ, it might hang the system by holding the
> interrupt line before the driver sets up the DSI host to the known
> state.
>
> Move the request/free_irq calls into msm_dsi_host_power_on/_off calls,
> so that we can be sure that the interrupt is delivered when the host is
> in the known state.
>
> Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support")
> Signed-off-by: Dmitry Baryshkov
This is a valid change and we have seen interrupt storms in downstream
happening
when like you said the bootloader leaves the DSI host in unknown
state.
Just one question below.
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 21 -
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index e269df285136..cd842347a6b1 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1951,15 +1951,6 @@ int msm_dsi_host_modeset_init(struct
> mipi_dsi_host *host,
> return ret;
> }
>
> - ret = devm_request_irq(>dev, msm_host->irq,
> - dsi_host_irq, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> - "dsi_isr", msm_host);
> - if (ret < 0) {
> - DRM_DEV_ERROR(>dev, "failed to request IRQ%u: %d\n",
> - msm_host->irq, ret);
> - return ret;
> - }
> -
> msm_host->dev = dev;
> ret = cfg_hnd->ops->tx_buf_alloc(msm_host, SZ_4K);
> if (ret) {
> @@ -2413,6 +2404,16 @@ int msm_dsi_host_power_on(struct mipi_dsi_host
> *host,
> if (msm_host->disp_en_gpio)
> gpiod_set_value(msm_host->disp_en_gpio, 1);
>
> + ret = devm_request_irq(_host->pdev->dev, msm_host->irq,
> + dsi_host_irq, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> + "dsi_isr", msm_host);
> + if (ret < 0) {
> + DRM_DEV_ERROR(_host->pdev->dev, "failed to request IRQ%u:
%d\n",
> + msm_host->irq, ret);
> + return ret;
> + }
> +
> +
Do you want to move this to msm_dsi_host_enable()?
So without the controller being enabled it is still in unknown state?
msm_dsi_host_power_on() reconfigures the host registers, so the state
is known at the end of the power_on().
Also do you want to do this after dsi0 and dsi1 are initialized to
account for
dual dsi cases?
I don't think this should matter. The host won't generate 'extra'
interrupts in such case, will it?
We have seen cases where misconfiguration has caused interrupts to storm
only
on one DSI in some cases. So yes, I would prefer this is done after both
are
configured.
> msm_host->power_on = true;
> mutex_unlock(_host->dev_mutex);
>
> @@ -2439,6 +2440,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host
> *host)
> goto unlock_ret;
> }
>
> + devm_free_irq(_host->pdev->dev, msm_host->irq, msm_host);
> +
> dsi_ctrl_config(msm_host, false, NULL, NULL);
>
> if (msm_host->disp_en_gpio)
[Freedreno] [PATCH v2] drm/msm/dsi: do not enable irq handler before powering up the host
The DSI host might be left in some state by the bootloader. If this
state generates an IRQ, it might hang the system by holding the
interrupt line before the driver sets up the DSI host to the known
state.
Move the request_irq into msm_dsi_host_init and pass IRQF_NO_AUTOEN to
it. Call enable/disable_irq from msm_dsi_host_power_on/_off() functions,
so that we can be sure that the interrupt is delivered when the host is
in the known state.
Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 38 +-
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e269df285136..b47708305f5c 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1898,6 +1898,23 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
return ret;
}
+ msm_host->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+ if (msm_host->irq < 0) {
+ ret = msm_host->irq;
+ dev_err(>dev, "failed to get irq: %d\n", ret);
+ return ret;
+ }
+
+ /* do not autoenable, will be enabled later, in msm_dsi_host_power_on */
+ ret = devm_request_irq(>dev, msm_host->irq, dsi_host_irq,
+ IRQF_TRIGGER_HIGH | IRQF_ONESHOT | IRQF_NO_AUTOEN,
+ "dsi_isr", msm_host);
+ if (ret < 0) {
+ dev_err(>dev, "failed to request IRQ%u: %d\n",
+ msm_host->irq, ret);
+ return ret;
+ }
+
init_completion(_host->dma_comp);
init_completion(_host->video_comp);
mutex_init(_host->dev_mutex);
@@ -1941,25 +1958,8 @@ int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
{
struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
- struct platform_device *pdev = msm_host->pdev;
int ret;
- msm_host->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
- if (msm_host->irq < 0) {
- ret = msm_host->irq;
- DRM_DEV_ERROR(dev->dev, "failed to get irq: %d\n", ret);
- return ret;
- }
-
- ret = devm_request_irq(>dev, msm_host->irq,
- dsi_host_irq, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
- "dsi_isr", msm_host);
- if (ret < 0) {
- DRM_DEV_ERROR(>dev, "failed to request IRQ%u: %d\n",
- msm_host->irq, ret);
- return ret;
- }
-
msm_host->dev = dev;
ret = cfg_hnd->ops->tx_buf_alloc(msm_host, SZ_4K);
if (ret) {
@@ -2413,6 +2413,8 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
if (msm_host->disp_en_gpio)
gpiod_set_value(msm_host->disp_en_gpio, 1);
+ enable_irq(msm_host->irq);
+
msm_host->power_on = true;
mutex_unlock(_host->dev_mutex);
@@ -2439,6 +2441,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host)
goto unlock_ret;
}
+ disable_irq(msm_host->irq);
+
dsi_ctrl_config(msm_host, false, NULL, NULL);
if (msm_host->disp_en_gpio)
--
2.30.2
Re: [Freedreno] [PATCH] drm/msm/dsi: do not install irq handler before power up the host
Hi,
On Tue, 21 Sept 2021 at 20:01, wrote:
>
> On 2021-09-21 09:22, Dmitry Baryshkov wrote:
> > The DSI host might be left in some state by the bootloader. If this
> > state generates an IRQ, it might hang the system by holding the
> > interrupt line before the driver sets up the DSI host to the known
> > state.
> >
> > Move the request/free_irq calls into msm_dsi_host_power_on/_off calls,
> > so that we can be sure that the interrupt is delivered when the host is
> > in the known state.
> >
> > Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support")
> > Signed-off-by: Dmitry Baryshkov
>
> This is a valid change and we have seen interrupt storms in downstream
> happening
> when like you said the bootloader leaves the DSI host in unknown state.
> Just one question below.
>
> > ---
> > drivers/gpu/drm/msm/dsi/dsi_host.c | 21 -
> > 1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index e269df285136..cd842347a6b1 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -1951,15 +1951,6 @@ int msm_dsi_host_modeset_init(struct
> > mipi_dsi_host *host,
> > return ret;
> > }
> >
> > - ret = devm_request_irq(>dev, msm_host->irq,
> > - dsi_host_irq, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > - "dsi_isr", msm_host);
> > - if (ret < 0) {
> > - DRM_DEV_ERROR(>dev, "failed to request IRQ%u: %d\n",
> > - msm_host->irq, ret);
> > - return ret;
> > - }
> > -
> > msm_host->dev = dev;
> > ret = cfg_hnd->ops->tx_buf_alloc(msm_host, SZ_4K);
> > if (ret) {
> > @@ -2413,6 +2404,16 @@ int msm_dsi_host_power_on(struct mipi_dsi_host
> > *host,
> > if (msm_host->disp_en_gpio)
> > gpiod_set_value(msm_host->disp_en_gpio, 1);
> >
> > + ret = devm_request_irq(_host->pdev->dev, msm_host->irq,
> > + dsi_host_irq, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > + "dsi_isr", msm_host);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(_host->pdev->dev, "failed to request IRQ%u:
> > %d\n",
> > + msm_host->irq, ret);
> > + return ret;
> > + }
> > +
> > +
>
> Do you want to move this to msm_dsi_host_enable()?
> So without the controller being enabled it is still in unknown state?
msm_dsi_host_power_on() reconfigures the host registers, so the state
is known at the end of the power_on().
> Also do you want to do this after dsi0 and dsi1 are initialized to
> account for
> dual dsi cases?
I don't think this should matter. The host won't generate 'extra'
interrupts in such case, will it?
>
> > msm_host->power_on = true;
> > mutex_unlock(_host->dev_mutex);
> >
> > @@ -2439,6 +2440,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host
> > *host)
> > goto unlock_ret;
> > }
> >
> > + devm_free_irq(_host->pdev->dev, msm_host->irq, msm_host);
> > +
> > dsi_ctrl_config(msm_host, false, NULL, NULL);
> >
> > if (msm_host->disp_en_gpio)
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] drm/msm/dsi: do not install irq handler before power up the host
On 2021-09-21 09:22, Dmitry Baryshkov wrote:
The DSI host might be left in some state by the bootloader. If this
state generates an IRQ, it might hang the system by holding the
interrupt line before the driver sets up the DSI host to the known
state.
Move the request/free_irq calls into msm_dsi_host_power_on/_off calls,
so that we can be sure that the interrupt is delivered when the host is
in the known state.
Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support")
Signed-off-by: Dmitry Baryshkov
This is a valid change and we have seen interrupt storms in downstream
happening
when like you said the bootloader leaves the DSI host in unknown state.
Just one question below.
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e269df285136..cd842347a6b1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1951,15 +1951,6 @@ int msm_dsi_host_modeset_init(struct
mipi_dsi_host *host,
return ret;
}
- ret = devm_request_irq(>dev, msm_host->irq,
- dsi_host_irq, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
- "dsi_isr", msm_host);
- if (ret < 0) {
- DRM_DEV_ERROR(>dev, "failed to request IRQ%u: %d\n",
- msm_host->irq, ret);
- return ret;
- }
-
msm_host->dev = dev;
ret = cfg_hnd->ops->tx_buf_alloc(msm_host, SZ_4K);
if (ret) {
@@ -2413,6 +2404,16 @@ int msm_dsi_host_power_on(struct mipi_dsi_host
*host,
if (msm_host->disp_en_gpio)
gpiod_set_value(msm_host->disp_en_gpio, 1);
+ ret = devm_request_irq(_host->pdev->dev, msm_host->irq,
+ dsi_host_irq, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+ "dsi_isr", msm_host);
+ if (ret < 0) {
+ DRM_DEV_ERROR(_host->pdev->dev, "failed to request IRQ%u:
%d\n",
+ msm_host->irq, ret);
+ return ret;
+ }
+
+
Do you want to move this to msm_dsi_host_enable()?
So without the controller being enabled it is still in unknown state?
Also do you want to do this after dsi0 and dsi1 are initialized to
account for
dual dsi cases?
msm_host->power_on = true;
mutex_unlock(_host->dev_mutex);
@@ -2439,6 +2440,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host
*host)
goto unlock_ret;
}
+ devm_free_irq(_host->pdev->dev, msm_host->irq, msm_host);
+
dsi_ctrl_config(msm_host, false, NULL, NULL);
if (msm_host->disp_en_gpio)
Re: [Freedreno] [PATCH] drm/msm/dsi/phy: fix clock names in 28nm_8960 phy
On 2021-09-21 09:22, Dmitry Baryshkov wrote:
The commit 9f91f22aafcd ("drm/msm/dsi: remove duplicate fields from
dsi_pll_Nnm instances") mistakenly changed registered clock names.
While
the platform is in progress of migration to using clock properties in
the dts rather than the global clock names, we should provide backwards
compatibility. Thus restore registerd global clock names.
Fixes: 9f91f22aafcd ("drm/msm/dsi: remove duplicate fields from
dsi_pll_Nnm instances")
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
index aaa37456f4ee..71ed4aa0dc67 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
@@ -428,7 +428,7 @@ static int pll_28nm_register(struct dsi_pll_28nm
*pll_28nm, struct clk_hw **prov
bytediv->reg = pll_28nm->phy->pll_base +
REG_DSI_28nm_8960_PHY_PLL_CTRL_9;
snprintf(parent_name, 32, "dsi%dvco_clk", pll_28nm->phy->id);
- snprintf(clk_name, 32, "dsi%dpllbyte", pll_28nm->phy->id);
+ snprintf(clk_name, 32, "dsi%dpllbyte", pll_28nm->phy->id + 1);
bytediv_init.name = clk_name;
bytediv_init.ops = _bytediv_ops;
@@ -442,7 +442,7 @@ static int pll_28nm_register(struct dsi_pll_28nm
*pll_28nm, struct clk_hw **prov
return ret;
provided_clocks[DSI_BYTE_PLL_CLK] = >hw;
- snprintf(clk_name, 32, "dsi%dpll", pll_28nm->phy->id);
+ snprintf(clk_name, 32, "dsi%dpll", pll_28nm->phy->id + 1);
/* DIV3 */
hw = devm_clk_hw_register_divider(dev, clk_name,
parent_name, 0, pll_28nm->phy->pll_base +
[Freedreno] [PATCH] drm/msm/dsi: do not install irq handler before power up the host
The DSI host might be left in some state by the bootloader. If this
state generates an IRQ, it might hang the system by holding the
interrupt line before the driver sets up the DSI host to the known
state.
Move the request/free_irq calls into msm_dsi_host_power_on/_off calls,
so that we can be sure that the interrupt is delivered when the host is
in the known state.
Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e269df285136..cd842347a6b1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1951,15 +1951,6 @@ int msm_dsi_host_modeset_init(struct mipi_dsi_host *host,
return ret;
}
- ret = devm_request_irq(>dev, msm_host->irq,
- dsi_host_irq, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
- "dsi_isr", msm_host);
- if (ret < 0) {
- DRM_DEV_ERROR(>dev, "failed to request IRQ%u: %d\n",
- msm_host->irq, ret);
- return ret;
- }
-
msm_host->dev = dev;
ret = cfg_hnd->ops->tx_buf_alloc(msm_host, SZ_4K);
if (ret) {
@@ -2413,6 +2404,16 @@ int msm_dsi_host_power_on(struct mipi_dsi_host *host,
if (msm_host->disp_en_gpio)
gpiod_set_value(msm_host->disp_en_gpio, 1);
+ ret = devm_request_irq(_host->pdev->dev, msm_host->irq,
+ dsi_host_irq, IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+ "dsi_isr", msm_host);
+ if (ret < 0) {
+ DRM_DEV_ERROR(_host->pdev->dev, "failed to request IRQ%u:
%d\n",
+ msm_host->irq, ret);
+ return ret;
+ }
+
+
msm_host->power_on = true;
mutex_unlock(_host->dev_mutex);
@@ -2439,6 +2440,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host *host)
goto unlock_ret;
}
+ devm_free_irq(_host->pdev->dev, msm_host->irq, msm_host);
+
dsi_ctrl_config(msm_host, false, NULL, NULL);
if (msm_host->disp_en_gpio)
--
2.30.2
[Freedreno] [PATCH] drm/msm/dsi/phy: fix clock names in 28nm_8960 phy
The commit 9f91f22aafcd ("drm/msm/dsi: remove duplicate fields from
dsi_pll_Nnm instances") mistakenly changed registered clock names. While
the platform is in progress of migration to using clock properties in
the dts rather than the global clock names, we should provide backwards
compatibility. Thus restore registerd global clock names.
Fixes: 9f91f22aafcd ("drm/msm/dsi: remove duplicate fields from dsi_pll_Nnm
instances")
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
index aaa37456f4ee..71ed4aa0dc67 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm_8960.c
@@ -428,7 +428,7 @@ static int pll_28nm_register(struct dsi_pll_28nm *pll_28nm,
struct clk_hw **prov
bytediv->reg = pll_28nm->phy->pll_base +
REG_DSI_28nm_8960_PHY_PLL_CTRL_9;
snprintf(parent_name, 32, "dsi%dvco_clk", pll_28nm->phy->id);
- snprintf(clk_name, 32, "dsi%dpllbyte", pll_28nm->phy->id);
+ snprintf(clk_name, 32, "dsi%dpllbyte", pll_28nm->phy->id + 1);
bytediv_init.name = clk_name;
bytediv_init.ops = _bytediv_ops;
@@ -442,7 +442,7 @@ static int pll_28nm_register(struct dsi_pll_28nm *pll_28nm,
struct clk_hw **prov
return ret;
provided_clocks[DSI_BYTE_PLL_CLK] = >hw;
- snprintf(clk_name, 32, "dsi%dpll", pll_28nm->phy->id);
+ snprintf(clk_name, 32, "dsi%dpll", pll_28nm->phy->id + 1);
/* DIV3 */
hw = devm_clk_hw_register_divider(dev, clk_name,
parent_name, 0, pll_28nm->phy->pll_base +
--
2.30.2
