Re: [Freedreno] [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Sat, Oct 02, 2021 at 01:05:47AM +0300, Ville Syrjälä wrote:
> On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote:
> > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > > > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> > > > > Hi all,
> > > > >
> > > > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst")
> > > > > was to
> > > > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's
> > > > > what this
> > > > > patch series is about.
> > > > >
> > > > > You will find two types of changes here:
> > > > >
> > > > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding
> > > > > boilerplate) with
> > > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as
> > > > > it has
> > > > > already been done in previous commits such as b7ea04d2)
> > > > >
> > > > > - Replacing "drm_modeset_lock_all()" with
> > > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> > > > > in the remaining places (as it has already been done in previous
> > > > > commits
> > > > > such as 57037094)
> > > > >
> > > > > Most of the changes are straight forward, except for a few cases in
> > > > > the "amd"
> > > > > and "i915" drivers where some extra dancing was needed to overcome the
> > > > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can
> > > > > only be used
> > > > > once inside the same function (the reason being that the macro
> > > > > expansion
> > > > > includes *labels*, and you can not have two labels named the same
> > > > > inside one
> > > > > function)
> > > > >
> > > > > Notice that, even after this patch series, some places remain where
> > > > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still
> > > > > present,
> > > > > all inside drm core (which makes sense), except for two (in "amd" and
> > > > > "i915")
> > > > > which cannot be replaced due to the way they are being used.
> > > > >
> > > > > Changes in v2:
> > > > >
> > > > > - Fix commit message typo
> > > > > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> > > > > - Split drm/i915 patch into two simpler ones
> > > > > - Remove drm_modeset_(un)lock_all()
> > > > > - Fix build problems in non-x86 platforms
> > > > >
> > > > > Fernando Ramos (17):
> > > > > drm: cleanup: drm_modeset_lock_all_ctx() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/i915: cleanup: drm_modeset_lock_all_ctx() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/msm: cleanup: drm_modeset_lock_all_ctx() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN() drm/vmwgfx: cleanup:
> > > > > drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/tegra: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/shmobile: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/radeon: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/omapdrm: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/nouveau: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/msm: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN() part 2
> > > > > drm/gma500: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm/amd: cleanup: drm_modeset_lock_all() -->
> > > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > > drm: cleanup: remove drm_modeset_(un)lock_all()
> > > > > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
> > > > >
> > > >
> > > > Thank you for revising, Fernando! I've pushed the set to drm-misc-next
> > > > (along
> > > > with the necessary drm-tip conflict resolutions).
> > >
> > > Ugh. Did anyone actually review the locking changes this does?
> > > I shot the previous i915 stuff down because the commit messages
> > > did not address any of it.
> >
> > I reviewed the set on 9/17, I didn't see your feedback on that thread.
>
> It was much earlir than that.
> https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html
>
> And I think I might have also shot down a similar thing earlier.
>
> I was actually half considering sending a patch to nuke that
> misleading TODO item. I don't think anything which changes
> which locks are taken should be considred a starter level task.
> And the commit messages here don't seem to address any of it.
And i915 is now broken :(
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10680/fi-bwr-2160/boot.html
--
Re: [Freedreno] [PATCH v3] drm/msm/dsi: do not enable irq handler before powering up the host
On 2021-10-01 18:08, 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_irq into msm_dsi_host_init and pass IRQF_NO_AUTOEN to
it. Call enable/disable_irq after 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.
It is not possible to defer the interrupt enablement to a later point,
because drm_panel_prepare might need to communicate with the panel over
the DSI link and that requires working interrupt.
Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support")
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dsi/dsi.h | 2 ++
drivers/gpu/drm/msm/dsi/dsi_host.c| 48 +--
drivers/gpu/drm/msm/dsi/dsi_manager.c | 16 +
3 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h
b/drivers/gpu/drm/msm/dsi/dsi.h
index b50db91cb8a7..569c8ff062ba 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -107,6 +107,8 @@ void msm_dsi_host_cmd_xfer_commit(struct
mipi_dsi_host *host,
u32 dma_base, u32 len);
int msm_dsi_host_enable(struct mipi_dsi_host *host);
int msm_dsi_host_disable(struct mipi_dsi_host *host);
+void msm_dsi_host_enable_irq(struct mipi_dsi_host *host);
+void msm_dsi_host_disable_irq(struct mipi_dsi_host *host);
int msm_dsi_host_power_on(struct mipi_dsi_host *host,
struct msm_dsi_phy_shared_timings *phy_shared_timings,
bool is_bonded_dsi, struct msm_dsi_phy *phy);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e269df285136..ce26eb78cb6c 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 */
+ 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) {
@@ -2315,6 +2315,20 @@ void msm_dsi_host_get_phy_clk_req(struct
mipi_dsi_host *host,
clk_req->escclk_rate = msm_host->esc_clk_rate;
}
+void msm_dsi_host_enable_irq(struct mipi_dsi_host *host)
+{
+ struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+ enable_irq(msm_host->irq);
+}
+
+void msm_dsi_host_disable_irq(struct mipi_dsi_host *host)
+{
+ struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
+
+ disable_irq(msm_host->irq);
+}
+
int msm_dsi_host_enable(struct mipi_dsi_host *host)
{
struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index c41d39f5b7cf..fb4ccffdcfe1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -377,6 +377,14 @@ static void dsi_mgr_bridge_pre_enable(struct
drm_bridge *bridge)
}
}
+ /*
+* Enable before preparing the panel, disable after unpreparing, so
+* that the panel can communicate over the DSI
[Freedreno] [PATCH] drm/msm/dsi: use bulk clk API
Use clk_bulk_* API instead of hand-coding them. Note, this drops support
for legacy clk naming (e.g. "iface_clk" instead of just "iface"),
however all in-kernel device trees were converted long long ago. The
warning is present there since 2017.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 59 ++
1 file changed, 12 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e269df285136..3b81f40bba2e 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -106,7 +106,8 @@ struct msm_dsi_host {
phys_addr_t ctrl_size;
struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
- struct clk *bus_clks[DSI_BUS_CLK_MAX];
+ int num_bus_clks;
+ struct clk_bulk_data bus_clks[DSI_BUS_CLK_MAX];
struct clk *byte_clk;
struct clk *esc_clk;
@@ -374,15 +375,14 @@ static int dsi_clk_init(struct msm_dsi_host *msm_host)
int i, ret = 0;
/* get bus clocks */
- for (i = 0; i < cfg->num_bus_clks; i++) {
- msm_host->bus_clks[i] = msm_clk_get(pdev,
- cfg->bus_clk_names[i]);
- if (IS_ERR(msm_host->bus_clks[i])) {
- ret = PTR_ERR(msm_host->bus_clks[i]);
- pr_err("%s: Unable to get %s clock, ret = %d\n",
- __func__, cfg->bus_clk_names[i], ret);
- goto exit;
- }
+ for (i = 0; i < cfg->num_bus_clks; i++)
+ msm_host->bus_clks[i].id = cfg->bus_clk_names[i];
+ msm_host->num_bus_clks = cfg->num_bus_clks;
+
+ ret = devm_clk_bulk_get(>dev, msm_host->num_bus_clks,
msm_host->bus_clks);
+ if (ret < 0) {
+ dev_err(>dev, "Unable to get clocks, ret = %d\n", ret);
+ goto exit;
}
/* get link and source clocks */
@@ -433,41 +433,6 @@ static int dsi_clk_init(struct msm_dsi_host *msm_host)
return ret;
}
-static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)
-{
- const struct msm_dsi_config *cfg = msm_host->cfg_hnd->cfg;
- int i, ret;
-
- DBG("id=%d", msm_host->id);
-
- for (i = 0; i < cfg->num_bus_clks; i++) {
- ret = clk_prepare_enable(msm_host->bus_clks[i]);
- if (ret) {
- pr_err("%s: failed to enable bus clock %d ret %d\n",
- __func__, i, ret);
- goto err;
- }
- }
-
- return 0;
-err:
- for (; i > 0; i--)
- clk_disable_unprepare(msm_host->bus_clks[i]);
-
- return ret;
-}
-
-static void dsi_bus_clk_disable(struct msm_dsi_host *msm_host)
-{
- const struct msm_dsi_config *cfg = msm_host->cfg_hnd->cfg;
- int i;
-
- DBG("");
-
- for (i = cfg->num_bus_clks - 1; i >= 0; i--)
- clk_disable_unprepare(msm_host->bus_clks[i]);
-}
-
int msm_dsi_runtime_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
@@ -478,7 +443,7 @@ int msm_dsi_runtime_suspend(struct device *dev)
if (!msm_host->cfg_hnd)
return 0;
- dsi_bus_clk_disable(msm_host);
+ clk_bulk_disable_unprepare(msm_host->num_bus_clks, msm_host->bus_clks);
return 0;
}
@@ -493,7 +458,7 @@ int msm_dsi_runtime_resume(struct device *dev)
if (!msm_host->cfg_hnd)
return 0;
- return dsi_bus_clk_enable(msm_host);
+ return clk_bulk_prepare_enable(msm_host->num_bus_clks,
msm_host->bus_clks);
}
int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host)
--
2.33.0
Re: [Freedreno] [PATCH] drm/msm/dsi: do not install irq handler before power up the host
On 28/09/2021 04:40, Dmitry Baryshkov wrote:
On 28/09/2021 04:33, abhin...@codeaurora.org wrote:
On 2021-09-27 18:29, Dmitry Baryshkov wrote:
On 28/09/2021 04:19, abhin...@codeaurora.org wrote:
On 2021-09-27 18:06, Dmitry Baryshkov wrote:
On Tue, 28 Sept 2021 at 03:22, wrote:
On 2021-09-25 12:43, Dmitry Baryshkov wrote:
> On 21/09/2021 23:52, abhin...@codeaurora.org wrote:
>> 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.
>
> I've checked. The power_on is called from
dsi_mgr_bridge_pre_enable()
> when both DSI hosts should be bound.
DSI being bound is enough? I thought the issue we are trying to
address
is that
we need to have called msm_dsi_host_power_on() for both the hosts so
that both are
put in the known state before requesting the irq.
OR in other words move the irq_enable() to below location.
341 static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
342 {
364 ret = msm_dsi_host_power_on(host, _shared_timings[id],
is_bonded_dsi, msm_dsi->phy);
365 if (ret) {
366 pr_err("%s: power on host %d failed, %d\n",
__func__, id, ret);
367 goto host_on_fail;
368 }
369
370 if (is_bonded_dsi && msm_dsi1) {
371 ret = msm_dsi_host_power_on(msm_dsi1->host,
372 _shared_timings[DSI_1],
is_bonded_dsi, msm_dsi1->phy);
373 if (ret) {
374 pr_err("%s: power on host1 failed, %d\n",
375 __func__,
ret);
376 goto host1_on_fail;
377 }
378 }
< move the irq enable here >
**
Ah, I see your point. What about moving to
Re: [Freedreno] [PATCH 1/3] mfd: qcom-pm8xxx: switch away from using chained IRQ handlers
On 02/10/2021 04:08, Dmitry Baryshkov wrote: PM8xxx PMIC family uses GPIO as parent IRQ. Using it together with the irq_set_chained_handler_and_data() results in warnings from the GPIOLIB as in this path the IRQ resources are not allocated (and thus the corresponding GPIO is not marked as used for the IRQ. Use request_irq so that the IRQ resources are proprely setup. Signed-off-by: Dmitry Baryshkov Please ignore this duplicate, it was sent by mistake. -- With best wishes Dmitry
[Freedreno] [PATCH 1/3] mfd: qcom-pm8xxx: switch away from using chained IRQ handlers
PM8xxx PMIC family uses GPIO as parent IRQ. Using it together with the
irq_set_chained_handler_and_data() results in warnings from the GPIOLIB
as in this path the IRQ resources are not allocated (and thus the
corresponding GPIO is not marked as used for the IRQ. Use request_irq so
that the IRQ resources are proprely setup.
Signed-off-by: Dmitry Baryshkov
---
drivers/mfd/qcom-pm8xxx.c | 39 ---
1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
index ec18a04de355..2f2734ba5273 100644
--- a/drivers/mfd/qcom-pm8xxx.c
+++ b/drivers/mfd/qcom-pm8xxx.c
@@ -65,7 +65,7 @@
struct pm_irq_data {
int num_irqs;
struct irq_chip *irq_chip;
- void (*irq_handler)(struct irq_desc *desc);
+ irq_handler_t irq_handler;
};
struct pm_irq_chip {
@@ -169,19 +169,16 @@ static int pm8xxx_irq_master_handler(struct pm_irq_chip
*chip, int master)
return ret;
}
-static void pm8xxx_irq_handler(struct irq_desc *desc)
+static irqreturn_t pm8xxx_irq_handler(int irq, void *data)
{
- struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
- struct irq_chip *irq_chip = irq_desc_get_chip(desc);
+ struct pm_irq_chip *chip = data;
unsigned int root;
int i, ret, masters = 0;
- chained_irq_enter(irq_chip, desc);
-
ret = regmap_read(chip->regmap, SSBI_REG_ADDR_IRQ_ROOT, );
if (ret) {
pr_err("Can't read root status ret=%d\n", ret);
- return;
+ return IRQ_NONE;
}
/* on pm8xxx series masters start from bit 1 of the root */
@@ -192,7 +189,7 @@ static void pm8xxx_irq_handler(struct irq_desc *desc)
if (masters & (1 << i))
pm8xxx_irq_master_handler(chip, i);
- chained_irq_exit(irq_chip, desc);
+ return IRQ_HANDLED;
}
static void pm8821_irq_block_handler(struct pm_irq_chip *chip,
@@ -230,19 +227,17 @@ static inline void pm8821_irq_master_handler(struct
pm_irq_chip *chip,
pm8821_irq_block_handler(chip, master, block);
}
-static void pm8821_irq_handler(struct irq_desc *desc)
+static irqreturn_t pm8821_irq_handler(int irq, void *data)
{
- struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
- struct irq_chip *irq_chip = irq_desc_get_chip(desc);
+ struct pm_irq_chip *chip = data;
unsigned int master;
int ret;
- chained_irq_enter(irq_chip, desc);
ret = regmap_read(chip->regmap,
PM8821_SSBI_REG_ADDR_IRQ_MASTER0, );
if (ret) {
pr_err("Failed to read master 0 ret=%d\n", ret);
- goto done;
+ return IRQ_NONE;
}
/* bits 1 through 7 marks the first 7 blocks in master 0 */
@@ -251,19 +246,18 @@ static void pm8821_irq_handler(struct irq_desc *desc)
/* bit 0 marks if master 1 contains any bits */
if (!(master & BIT(0)))
- goto done;
+ return IRQ_NONE;
ret = regmap_read(chip->regmap,
PM8821_SSBI_REG_ADDR_IRQ_MASTER1, );
if (ret) {
pr_err("Failed to read master 1 ret=%d\n", ret);
- goto done;
+ return IRQ_NONE;
}
pm8821_irq_master_handler(chip, 1, master);
-done:
- chained_irq_exit(irq_chip, desc);
+ return IRQ_HANDLED;
}
static void pm8xxx_irq_mask_ack(struct irq_data *d)
@@ -574,14 +568,15 @@ static int pm8xxx_probe(struct platform_device *pdev)
if (!chip->irqdomain)
return -ENODEV;
- irq_set_chained_handler_and_data(irq, data->irq_handler, chip);
+ rc = devm_request_irq(>dev, irq, data->irq_handler, 0,
dev_name(>dev), chip);
+ if (rc)
+ return rc;
+
irq_set_irq_wake(irq, 1);
rc = of_platform_populate(pdev->dev.of_node, NULL, NULL, >dev);
- if (rc) {
- irq_set_chained_handler_and_data(irq, NULL, NULL);
+ if (rc)
irq_domain_remove(chip->irqdomain);
- }
return rc;
}
@@ -594,11 +589,9 @@ static int pm8xxx_remove_child(struct device *dev, void
*unused)
static int pm8xxx_remove(struct platform_device *pdev)
{
- int irq = platform_get_irq(pdev, 0);
struct pm_irq_chip *chip = platform_get_drvdata(pdev);
device_for_each_child(>dev, NULL, pm8xxx_remove_child);
- irq_set_chained_handler_and_data(irq, NULL, NULL);
irq_domain_remove(chip->irqdomain);
return 0;
--
2.33.0
Re: [Freedreno] [PATCH 3/3] drm/msm/dsi: fix signedness bug in msm_dsi_host_cmd_rx()
On 01/10/2021 15:36, Dan Carpenter wrote:
The "msg->tx_len" variable is type size_t so if dsi_cmds2buf_tx()
returns a negative error code that it type promoted to a high positive
value and treat as a success. The second problem with this code is
that it can return meaningless positive values on error.
It looks to me that this piece of code is not fully correct at all.
dsi_cmds2bus_tx would return the size of DSI packet, not the size of the
DSI buffer.
Could you please be more specific, which 'meaningless positive values'
were you receiving?
Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index c86b5090fae6..42073a562072 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -2133,8 +2133,10 @@ int msm_dsi_host_cmd_rx(struct mipi_dsi_host *host,
}
ret = dsi_cmds2buf_tx(msm_host, msg);
- if (ret < msg->tx_len) {
+ if (ret < 0 || ret < msg->tx_len) {
pr_err("%s: Read cmd Tx failed, %d\n", __func__, ret);
+ if (ret >= 0)
+ ret = -EIO;
return ret;
}
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH 2/3] drm/msm/dsi: fix off by one in dsi_bus_clk_enable error handling
On 01/10/2021 15:34, Dan Carpenter wrote:
This disables a lock which wasn't enabled and it does not disable
the first lock in the array.
Fixes: 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")
Signed-off-by: Dan Carpenter
Reviewed-by: Dmitry Baryshkov
We should probably switch this to bulk clk api.
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e269df285136..c86b5090fae6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -451,7 +451,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)
return 0;
err:
- for (; i > 0; i--)
+ while (--i >= 0)
clk_disable_unprepare(msm_host->bus_clks[i]);
return ret;
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH 1/3] drm/msm/dsi: Fix an error code in msm_dsi_modeset_init()
On 01/10/2021 15:33, Dan Carpenter wrote:
Return an error code if msm_dsi_manager_validate_current_config().
Don't return success.
Fixes: 8b03ad30e314 ("drm/msm/dsi: Use one connector for dual DSI mode")
Signed-off-by: Dan Carpenter
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/dsi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 614dc7f26f2c..75ae3008b68f 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -215,8 +215,10 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct
drm_device *dev,
goto fail;
}
- if (!msm_dsi_manager_validate_current_config(msm_dsi->id))
+ if (!msm_dsi_manager_validate_current_config(msm_dsi->id)) {
+ ret = -EINVAL;
goto fail;
+ }
msm_dsi->encoder = encoder;
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] drm/msm/dsi: prevent unintentional integer overflow in dsi_pll_28nm_clk_recalc_rate()
On 29/09/2021 20:51, Tim Gardner wrote:
Coverity warns of an unintentional integer overflow
CID 120715 (#1 of 1): Unintentional integer overflow (OVERFLOW_BEFORE_WIDEN)
overflow_before_widen: Potentially overflowing expression ref_clk * sdm_byp_div
with type unsigned int (32 bits, unsigned) is evaluated using 32-bit
arithmetic,
and then used in a context that expects an expression of type unsigned long
(64 bits, unsigned).
To avoid overflow, cast either ref_clk or sdm_byp_div to type unsigned long.
263vco_rate = ref_clk * sdm_byp_div;
Fix this and another possible overflow by casting ref_clk to unsigned long.
Changing ref_clk from u32 to unsigned long would be a more simple and
elegant way of fixing this issue. Could you please update your patch?
Cc: Rob Clark
Cc: Sean Paul
Cc: David Airlie
Cc: Daniel Vetter
Cc: Dmitry Baryshkov
Cc: Abhinav Kumar
Cc: linux-arm-...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc: freedreno@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Tim Gardner
---
drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
index 2da673a2add6..cfe4b30eb96d 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_28nm.c
@@ -260,7 +260,7 @@ static unsigned long dsi_pll_28nm_clk_recalc_rate(struct
clk_hw *hw,
sdm_byp_div = FIELD(
dsi_phy_read(base +
REG_DSI_28nm_PHY_PLL_SDM_CFG0),
DSI_28nm_PHY_PLL_SDM_CFG0_BYP_DIV) + 1;
- vco_rate = ref_clk * sdm_byp_div;
+ vco_rate = (unsigned long)ref_clk * sdm_byp_div;
} else {
/* sdm mode */
sdm_dc_off = FIELD(
@@ -274,7 +274,7 @@ static unsigned long dsi_pll_28nm_clk_recalc_rate(struct
clk_hw *hw,
sdm_freq_seed = (sdm3 << 8) | sdm2;
DBG("sdm_freq_seed = %d", sdm_freq_seed);
- vco_rate = (ref_clk * (sdm_dc_off + 1)) +
+ vco_rate = ((unsigned long)ref_clk * (sdm_dc_off + 1)) +
mult_frac(ref_clk, sdm_freq_seed, BIT(16));
DBG("vco rate = %lu", vco_rate);
}
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] drm/msm: prevent NULL dereference in msm_gpu_crashstate_capture()
On 29/09/2021 19:25, Tim Gardner wrote:
Coverity complains of a possible NULL dereference:
CID 120718 (#1 of 1): Dereference null return value (NULL_RETURNS)
23. dereference: Dereferencing a pointer that might be NULL state->bos when
calling msm_gpu_crashstate_get_bo. [show details]
301msm_gpu_crashstate_get_bo(state, submit->bos[i].obj,
302submit->bos[i].iova, submit->bos[i].flags);
Fix this by employing the same state->bos NULL check as is used in the next
for loop.
Cc: Rob Clark
Cc: Sean Paul
Cc: David Airlie
Cc: Daniel Vetter
Cc: linux-arm-...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc: freedreno@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Tim Gardner
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/msm_gpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 8a3a592da3a4..2c46cd968ac4 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -296,7 +296,7 @@ static void msm_gpu_crashstate_capture(struct msm_gpu *gpu,
state->bos = kcalloc(nr,
sizeof(struct msm_gpu_state_bo), GFP_KERNEL);
- for (i = 0; i < submit->nr_bos; i++) {
+ for (i = 0; state->bos && i < submit->nr_bos; i++) {
if (should_dump(submit, i)) {
msm_gpu_crashstate_get_bo(state,
submit->bos[i].obj,
submit->bos[i].iova,
submit->bos[i].flags);
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v2] drm: msm: adreno_gpu.c: Add and use pr_fmt(fmt)
On 26/08/2021 05:23, zhaoxiao wrote:
Use a more common logging style.
Signed-off-by: zhaoxiao
Your subject tells about pr_fmt(), while the patch itself changs
printk()s to pr_info(). Could you please fix the commit subject and
expand/correct commit message?
---
v2:Remove the line: #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
drivers/gpu/drm/msm/adreno/adreno_gpu.c:23:9: warning: 'pr_fmt' macro
redefined [-Wmacro-redefined]
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
^
include/linux/printk.h:348:9: note: previous definition is here
#define pr_fmt(fmt) fmt
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 9f5a30234b33..f10e9e04c13b 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -753,7 +753,7 @@ void adreno_dump_info(struct msm_gpu *gpu)
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
int i;
- printk("revision: %d (%d.%d.%d.%d)\n",
+ pr_info("revision: %d (%d.%d.%d.%d)\n",
adreno_gpu->info->revn, adreno_gpu->rev.core,
adreno_gpu->rev.major, adreno_gpu->rev.minor,
adreno_gpu->rev.patchid);
@@ -761,12 +761,12 @@ void adreno_dump_info(struct msm_gpu *gpu)
for (i = 0; i < gpu->nr_rings; i++) {
struct msm_ringbuffer *ring = gpu->rb[i];
- printk("rb %d: fence:%d/%d\n", i,
+ pr_info("rb %d: fence:%d/%d\n", i,
ring->memptrs->fence,
ring->seqno);
- printk("rptr: %d\n", get_rptr(adreno_gpu, ring));
- printk("rb wptr: %d\n", get_wptr(ring));
+ pr_info("rptr: %d\n", get_rptr(adreno_gpu, ring));
+ pr_info("rb wptr: %d\n", get_wptr(ring));
}
}
@@ -780,7 +780,7 @@ void adreno_dump(struct msm_gpu *gpu)
return;
/* dump these out in a form that can be parsed by demsm: */
- printk("IO:region %s 0002\n", gpu->name);
+ pr_info("IO:region %s 0002\n", gpu->name);
for (i = 0; adreno_gpu->registers[i] != ~0; i += 2) {
uint32_t start = adreno_gpu->registers[i];
uint32_t end = adreno_gpu->registers[i+1];
@@ -788,7 +788,7 @@ void adreno_dump(struct msm_gpu *gpu)
for (addr = start; addr <= end; addr++) {
uint32_t val = gpu_read(gpu, addr);
- printk("IO:R %08x %08x\n", addr<<2, val);
+ pr_info("IO:R %08x %08x\n", addr<<2, val);
}
}
}
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v2 2/2] drm/msm/dpu: Fix timeout issues on command mode panels
On 11/09/2021 19:39, AngeloGioacchino Del Regno wrote: In function dpu_encoder_phys_cmd_wait_for_commit_done we are always checking if the relative CTL is started by waiting for an interrupt to fire: it is fine to do that, but then sometimes we call this function while the CTL is up and has never been put down, but that interrupt gets raised only when the CTL gets a state change from 0 to 1 (disabled to enabled), so we're going to wait for something that will never happen on its own. Solving this while avoiding to restart the CTL is actually possible and can be done by just checking if it is already up and running when the wait_for_commit_done function is called: in this case, so, if the CTL was already running, we can say that the commit is done if the command transmission is complete (in other terms, if the interface has been flushed). I've compared this with the MDP5 driver, where we always wait for PP_DONE interrupt. Would it be enough to always wait for it (= always call dpu_encoder_phys_cmd_wait_for_tx_complete())? Signed-off-by: AngeloGioacchino Del Regno --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index aa01698d6b25..aa5d3b3cef15 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -682,6 +682,9 @@ static int dpu_encoder_phys_cmd_wait_for_commit_done( if (!dpu_encoder_phys_cmd_is_master(phys_enc)) return 0; + if (phys_enc->hw_ctl->ops.is_started(phys_enc->hw_ctl)) + return dpu_encoder_phys_cmd_wait_for_tx_complete(phys_enc); + return _dpu_encoder_phys_cmd_wait_for_ctl_start(phys_enc); } -- With best wishes Dmitry
Re: [Freedreno] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote:
> On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> > > > Hi all,
> > > >
> > > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst")
> > > > was to
> > > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's
> > > > what this
> > > > patch series is about.
> > > >
> > > > You will find two types of changes here:
> > > >
> > > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding
> > > > boilerplate) with
> > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it
> > > > has
> > > > already been done in previous commits such as b7ea04d2)
> > > >
> > > > - Replacing "drm_modeset_lock_all()" with
> > > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> > > > in the remaining places (as it has already been done in previous
> > > > commits
> > > > such as 57037094)
> > > >
> > > > Most of the changes are straight forward, except for a few cases in the
> > > > "amd"
> > > > and "i915" drivers where some extra dancing was needed to overcome the
> > > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only
> > > > be used
> > > > once inside the same function (the reason being that the macro expansion
> > > > includes *labels*, and you can not have two labels named the same
> > > > inside one
> > > > function)
> > > >
> > > > Notice that, even after this patch series, some places remain where
> > > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still
> > > > present,
> > > > all inside drm core (which makes sense), except for two (in "amd" and
> > > > "i915")
> > > > which cannot be replaced due to the way they are being used.
> > > >
> > > > Changes in v2:
> > > >
> > > > - Fix commit message typo
> > > > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> > > > - Split drm/i915 patch into two simpler ones
> > > > - Remove drm_modeset_(un)lock_all()
> > > > - Fix build problems in non-x86 platforms
> > > >
> > > > Fernando Ramos (17):
> > > > drm: cleanup: drm_modeset_lock_all_ctx() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/i915: cleanup: drm_modeset_lock_all_ctx() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/msm: cleanup: drm_modeset_lock_all_ctx() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/vmwgfx: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/tegra: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/shmobile: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/radeon: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/omapdrm: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/nouveau: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/msm: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN() part 2
> > > > drm/gma500: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm/amd: cleanup: drm_modeset_lock_all() -->
> > > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > > drm: cleanup: remove drm_modeset_(un)lock_all()
> > > > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
> > > >
> > >
> > > Thank you for revising, Fernando! I've pushed the set to drm-misc-next
> > > (along
> > > with the necessary drm-tip conflict resolutions).
> >
> > Ugh. Did anyone actually review the locking changes this does?
> > I shot the previous i915 stuff down because the commit messages
> > did not address any of it.
>
> I reviewed the set on 9/17, I didn't see your feedback on that thread.
It was much earlir than that.
https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html
And I think I might have also shot down a similar thing earlier.
I was actually half considering sending a patch to nuke that
misleading TODO item. I don't think anything which changes
which locks are taken should be considred a starter level task.
And the commit messages here don't seem to address any of it.
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH][V2] drm/msm: Fix potential integer overflow on 32 bit multiply
On 29/09/2021 14:53, Colin King wrote:
From: Colin Ian King
In the case where clock is 2147485 or greater the 32 bit multiplication
by 1000 will cause an integer overflow. Fix this by making the constant
1000 an unsigned long to ensure a long multiply occurs to avoid the
You are talking about 'unsigned long' here, however in the patch you've
used just 'unsigned' suffix. So, which one should be used?
I suspect that wanted to use UL here, since mode->clock is int, so it is
int * unsigned.
Also I'd suggest to define a helper function macro in the drm_modes.h(?)
that would take struct drm_display_mode pointer and return proper clock.
See icc_units_to_bps() for the inspiration.
overflow before assigning the result to the long result in variable
requested. Most probably a theoretical overflow issue, but worth fixing
to clear up static analysis warnings.
Addresses-Coverity: ("Unintentional integer overflow")
Fixes: c8afe684c95c ("drm/msm: basic KMS driver for snapdragon")
Fixes: 3e87599b68e7 ("drm/msm/mdp4: add LVDS panel support")
Fixes: 937f941ca06f ("drm/msm/dp: Use qmp phy for DP PLL and PHY")
Fixes: ab5b0107ccf3 ("drm/msm: Initial add eDP support in msm drm driver (v5)")
Fixes: a3376e3ec81c ("drm/msm: convert to drm_bridge")
Signed-off-by: Colin Ian King
---
V2: Find and fix all unintentional integer overflows that match this
overflow pattern.
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c| 2 +-
drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 2 +-
drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 2 +-
drivers/gpu/drm/msm/dp/dp_ctrl.c| 4 ++--
drivers/gpu/drm/msm/edp/edp_connector.c | 2 +-
drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 2 +-
drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 2 +-
7 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c
index 88645dbc3785..83140066441e 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c
@@ -50,7 +50,7 @@ static void mdp4_dtv_encoder_mode_set(struct drm_encoder
*encoder,
DBG("set mode: " DRM_MODE_FMT, DRM_MODE_ARG(mode));
- mdp4_dtv_encoder->pixclock = mode->clock * 1000;
+ mdp4_dtv_encoder->pixclock = mode->clock * 1000U;
DBG("pixclock=%lu", mdp4_dtv_encoder->pixclock);
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
index 10eb3e5b218e..d90dc0a39855 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
@@ -225,7 +225,7 @@ static void mdp4_lcdc_encoder_mode_set(struct drm_encoder
*encoder,
DBG("set mode: " DRM_MODE_FMT, DRM_MODE_ARG(mode));
- mdp4_lcdc_encoder->pixclock = mode->clock * 1000;
+ mdp4_lcdc_encoder->pixclock = mode->clock * 1000U;
DBG("pixclock=%lu", mdp4_lcdc_encoder->pixclock);
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
index 7288041dd86a..a965e7962a7f 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
@@ -64,7 +64,7 @@ static int mdp4_lvds_connector_mode_valid(struct
drm_connector *connector,
struct drm_encoder *encoder = mdp4_lvds_connector->encoder;
long actual, requested;
- requested = 1000 * mode->clock;
+ requested = 1000U * mode->clock;
actual = mdp4_lcdc_round_pixclk(encoder, requested);
DBG("requested=%ld, actual=%ld", requested, actual);
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 62e75dc8afc6..6babeb79aeb0 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1316,7 +1316,7 @@ static int dp_ctrl_enable_mainlink_clocks(struct
dp_ctrl_private *ctrl)
opts_dp->lanes = ctrl->link->link_params.num_lanes;
opts_dp->link_rate = ctrl->link->link_params.rate / 100;
dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link",
- ctrl->link->link_params.rate * 1000);
+ ctrl->link->link_params.rate * 1000U);
phy_configure(phy, _io->phy_opts);
phy_power_on(phy);
@@ -1336,7 +1336,7 @@ static int dp_ctrl_enable_stream_clocks(struct
dp_ctrl_private *ctrl)
int ret = 0;
dp_ctrl_set_clock_rate(ctrl, DP_STREAM_PM, "stream_pixel",
- ctrl->dp_ctrl.pixel_rate * 1000);
+ ctrl->dp_ctrl.pixel_rate * 1000U);
ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, true);
if (ret)
diff --git a/drivers/gpu/drm/msm/edp/edp_connector.c
b/drivers/gpu/drm/msm/edp/edp_connector.c
index 73cb5fd97a5a..837e7873141f 100644
---
Re: [Freedreno] [PATCH v3 3/3] drm/msm/mdp5: Add configuration for MDP v1.16
On 28/09/2021 16:19, Sireesh Kodali wrote:
From: Vladimir Lypak
MDP version v1.16 is almost identical to v1.15 with most significant
difference being presence of second DSI interface. MDP v1.16 is found on
SoCs such as MSM8x53, SDM450, SDM632 (All with Adreno 506).
Signed-off-by: Vladimir Lypak
Signed-off-by: Sireesh Kodali
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 89
1 file changed, 89 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
index 9741544ffc35..0d28c8ff4009 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c
@@ -752,6 +752,94 @@ const struct mdp5_cfg_hw msm8x76_config = {
.max_clk = 36000,
};
+static const struct mdp5_cfg_hw msm8x53_config = {
+ .name = "msm8x53",
+ .mdp = {
+ .count = 1,
+ .caps = MDP_CAP_CDM |
+ MDP_CAP_SRC_SPLIT,
+ },
+ .ctl = {
+ .count = 3,
+ .base = { 0x01000, 0x01200, 0x01400 },
+ .flush_hw_mask = 0x,
+ },
+ .pipe_vig = {
+ .count = 1,
+ .base = { 0x04000 },
+ .caps = MDP_PIPE_CAP_HFLIP |
+ MDP_PIPE_CAP_VFLIP |
+ MDP_PIPE_CAP_SCALE |
+ MDP_PIPE_CAP_CSC|
+ MDP_PIPE_CAP_DECIMATION |
+ MDP_PIPE_CAP_SW_PIX_EXT |
+ 0,
+ },
+ .pipe_rgb = {
+ .count = 2,
+ .base = { 0x14000, 0x16000 },
+ .caps = MDP_PIPE_CAP_HFLIP |
+ MDP_PIPE_CAP_VFLIP |
+ MDP_PIPE_CAP_DECIMATION |
+ MDP_PIPE_CAP_SW_PIX_EXT |
+ 0,
+ },
+ .pipe_dma = {
+ .count = 1,
+ .base = { 0x24000 },
+ .caps = MDP_PIPE_CAP_HFLIP |
+ MDP_PIPE_CAP_VFLIP |
+ MDP_PIPE_CAP_SW_PIX_EXT |
+ 0,
+ },
+ .pipe_cursor = {
+ .count = 1,
+ .base = { 0x34000 },
+ .caps = MDP_PIPE_CAP_HFLIP |
+ MDP_PIPE_CAP_VFLIP |
+ MDP_PIPE_CAP_SW_PIX_EXT |
+ MDP_PIPE_CAP_CURSOR |
+ 0,
+ },
+
+ .lm = {
+ .count = 3,
+ .base = { 0x44000, 0x45000 },
+ .instances = {
+ { .id = 0, .pp = 0, .dspp = 0,
+ .caps = MDP_LM_CAP_DISPLAY |
+ MDP_LM_CAP_PAIR },
+ { .id = 1, .pp = 1, .dspp = -1,
+ .caps = MDP_LM_CAP_DISPLAY },
+},
+ .nb_stages = 5,
+ .max_width = 2048,
+ .max_height = 0x,
+ },
+ .dspp = {
+ .count = 1,
+ .base = { 0x54000 },
+
+ },
+ .pp = {
+ .count = 2,
+ .base = { 0x7, 0x70800 },
+ },
+ .cdm = {
+ .count = 1,
+ .base = { 0x79200 },
+ },
+ .intf = {
+ .base = { 0x6a000, 0x6a800, 0x6b000 },
+ .connect = {
+ [0] = INTF_DISABLED,
+ [1] = INTF_DSI,
+ [2] = INTF_DSI,
+ },
+ },
+ .max_clk = 4,
+};
+
static const struct mdp5_cfg_hw msm8917_config = {
.name = "msm8917",
.mdp = {
@@ -1151,6 +1239,7 @@ static const struct mdp5_cfg_handler cfg_handlers_v1[] = {
{ .revision = 7, .config = { .hw = _config } },
{ .revision = 11, .config = { .hw = _config } },
{ .revision = 15, .config = { .hw = _config } },
+ { .revision = 16, .config = { .hw = _config } },
};
static const struct mdp5_cfg_handler cfg_handlers_v3[] = {
--
With best wishes
Dmitry
Re: [Freedreno] [RFC] drm/msm/dp: Allow attaching a drm_panel
On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> Hi,
>
> On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> wrote:
> >
> > +static int dp_parser_find_panel(struct dp_parser *parser)
> > +{
> > + struct device_node *np = parser->pdev->dev.of_node;
> > + int rc;
> > +
> > + rc = drm_of_find_panel_or_bridge(np, 2, 0, >drm_panel,
> > NULL);
>
> Why port 2? Shouldn't this just be port 1 always? The yaml says that
> port 1 is "Output endpoint of the controller". We should just use port
> 1 here, right?
>
Finally got back to this, changed it to 1 and figured out why I left it
at 2.
drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
reference to the USB-C controller, scan through the registered panels
and conclude that the of_node of the USB-C controller isn't a registered
panel and return -EPROBE_DEFER.
So I picked 2, because I'm not able to figure out a way to distinguish
between a not yet probed panel and the USB-C controller...
Any suggestions?
Regards,
Bjorn
Re: [Freedreno] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> > > Hi all,
> > >
> > > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was
> > > to
> > > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what
> > > this
> > > patch series is about.
> > >
> > > You will find two types of changes here:
> > >
> > > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate)
> > > with
> > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it
> > > has
> > > already been done in previous commits such as b7ea04d2)
> > >
> > > - Replacing "drm_modeset_lock_all()" with
> > > "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> > > in the remaining places (as it has already been done in previous
> > > commits
> > > such as 57037094)
> > >
> > > Most of the changes are straight forward, except for a few cases in the
> > > "amd"
> > > and "i915" drivers where some extra dancing was needed to overcome the
> > > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be
> > > used
> > > once inside the same function (the reason being that the macro expansion
> > > includes *labels*, and you can not have two labels named the same inside
> > > one
> > > function)
> > >
> > > Notice that, even after this patch series, some places remain where
> > > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still
> > > present,
> > > all inside drm core (which makes sense), except for two (in "amd" and
> > > "i915")
> > > which cannot be replaced due to the way they are being used.
> > >
> > > Changes in v2:
> > >
> > > - Fix commit message typo
> > > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> > > - Split drm/i915 patch into two simpler ones
> > > - Remove drm_modeset_(un)lock_all()
> > > - Fix build problems in non-x86 platforms
> > >
> > > Fernando Ramos (17):
> > > drm: cleanup: drm_modeset_lock_all_ctx() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/i915: cleanup: drm_modeset_lock_all_ctx() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/msm: cleanup: drm_modeset_lock_all_ctx() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/vmwgfx: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/tegra: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/shmobile: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/radeon: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/omapdrm: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/nouveau: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/msm: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/i915: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN() part 2
> > > drm/gma500: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm/amd: cleanup: drm_modeset_lock_all() -->
> > > DRM_MODESET_LOCK_ALL_BEGIN()
> > > drm: cleanup: remove drm_modeset_(un)lock_all()
> > > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
> > >
> >
> > Thank you for revising, Fernando! I've pushed the set to drm-misc-next
> > (along
> > with the necessary drm-tip conflict resolutions).
>
> Ugh. Did anyone actually review the locking changes this does?
> I shot the previous i915 stuff down because the commit messages
> did not address any of it.
I reviewed the set on 9/17, I didn't see your feedback on that thread.
Sean
>
> --
> Ville Syrjälä
> Intel
--
Sean Paul, Software Engineer, Google / Chromium OS
[Freedreno] [PATCH] drm/msm/a6xx: Serialize GMU communication
From: Rob Clark
I've seen some crashes in our crash reporting that *look* like multiple
threads stomping on each other while communicating with GMU. So wrap
all those paths in a lock.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 3 ++
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 40 +++
3 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index a7c58018959f..8b73f70766a4 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -296,6 +296,8 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
u32 val;
int request, ack;
+ WARN_ON_ONCE(!mutex_is_locked(>lock));
+
if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
return -EINVAL;
@@ -337,6 +339,8 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
{
int bit;
+ WARN_ON_ONCE(!mutex_is_locked(>lock));
+
if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
return;
@@ -1482,6 +1486,8 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
if (!pdev)
return -ENODEV;
+ mutex_init(>lock);
+
gmu->dev = >dev;
of_dma_configure(gmu->dev, node, true);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index 3c74f64e3126..84bd516f01e8 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -44,6 +44,9 @@ struct a6xx_gmu_bo {
struct a6xx_gmu {
struct device *dev;
+ /* For serializing communication with the GMU: */
+ struct mutex lock;
+
struct msm_gem_address_space *aspace;
void * __iomem mmio;
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index f6a4dbef796b..bd7bdeff5d6f 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -881,7 +881,7 @@ static int a6xx_zap_shader_init(struct msm_gpu *gpu)
A6XX_RBBM_INT_0_MASK_UCHE_OOB_ACCESS | \
A6XX_RBBM_INT_0_MASK_UCHE_TRAP_INTR)
-static int a6xx_hw_init(struct msm_gpu *gpu)
+static int hw_init(struct msm_gpu *gpu)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
@@ -1135,6 +1135,19 @@ static int a6xx_hw_init(struct msm_gpu *gpu)
return ret;
}
+static int a6xx_hw_init(struct msm_gpu *gpu)
+{
+ struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+ struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+ int ret;
+
+ mutex_lock(_gpu->gmu.lock);
+ ret = hw_init(gpu);
+ mutex_unlock(_gpu->gmu.lock);
+
+ return ret;
+}
+
static void a6xx_dump(struct msm_gpu *gpu)
{
DRM_DEV_INFO(>pdev->dev, "status: %08x\n",
@@ -1509,7 +1522,9 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
trace_msm_gpu_resume(0);
+ mutex_lock(_gpu->gmu.lock);
ret = a6xx_gmu_resume(a6xx_gpu);
+ mutex_unlock(_gpu->gmu.lock);
if (ret)
return ret;
@@ -1532,7 +1547,9 @@ static int a6xx_pm_suspend(struct msm_gpu *gpu)
msm_devfreq_suspend(gpu);
+ mutex_lock(_gpu->gmu.lock);
ret = a6xx_gmu_stop(a6xx_gpu);
+ mutex_unlock(_gpu->gmu.lock);
if (ret)
return ret;
@@ -1547,18 +1564,19 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu,
uint64_t *value)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
- static DEFINE_MUTEX(perfcounter_oob);
- mutex_lock(_oob);
+ mutex_lock(_gpu->gmu.lock);
/* Force the GPU power on so we can read this register */
a6xx_gmu_set_oob(_gpu->gmu, GMU_OOB_PERFCOUNTER_SET);
*value = gpu_read64(gpu, REG_A6XX_CP_ALWAYS_ON_COUNTER_LO,
- REG_A6XX_CP_ALWAYS_ON_COUNTER_HI);
+ REG_A6XX_CP_ALWAYS_ON_COUNTER_HI);
a6xx_gmu_clear_oob(_gpu->gmu, GMU_OOB_PERFCOUNTER_SET);
- mutex_unlock(_oob);
+
+ mutex_unlock(_gpu->gmu.lock);
+
return 0;
}
@@ -1622,6 +1640,16 @@ static unsigned long a6xx_gpu_busy(struct msm_gpu *gpu)
return (unsigned long)busy_time;
}
+void a6xx_gpu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
+{
+ struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+ struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+
+ mutex_lock(_gpu->gmu.lock);
+ a6xx_gmu_set_freq(gpu, opp);
+ mutex_unlock(_gpu->gmu.lock);
+}
+
static struct msm_gem_address_space *
a6xx_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev)
{
@@ -1766,7 +1794,7 @@ static const struct adreno_gpu_funcs funcs = {
#endif
Re: [Freedreno] [PATCH v3 12/14] dt-bindings: msm/dp: Add bindings for HDCP registers
On Fri, 01 Oct 2021 11:11:41 -0400, Sean Paul wrote: > From: Sean Paul > > This patch adds the bindings for the MSM DisplayPort HDCP registers > which are required to write the HDCP key into the display controller as > well as the registers to enable HDCP authentication/key > exchange/encryption. > > We'll use a new compatible string for this since the fields are optional. > > Cc: Rob Herring > Cc: Stephen Boyd > Signed-off-by: Sean Paul > Link: > https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-13-s...@poorly.run > #v1 > Link: > https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-13-s...@poorly.run > #v2 > > Changes in v2: > -Drop register range names (Stephen) > -Fix yaml errors (Rob) > Changes in v3: > -Add new compatible string for dp-hdcp > -Add descriptions to reg > -Add minItems/maxItems to reg > -Make reg depend on the new hdcp compatible string > --- > > Disclaimer: I really don't know if this is the right way to approach > this. I tried using examples from other bindings, but feedback would be > very much welcome on how I could add the optional register ranges. > > > .../bindings/display/msm/dp-controller.yaml | 34 --- > 1 file changed, 30 insertions(+), 4 deletions(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/msm/dp-controller.example.dt.yaml: example-0: displayport-controller@ae9:reg:0: [0, 183042048, 0, 5120] is too long From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/msm/dp-controller.example.dt.yaml: example-0: displayport-controller@ae9:reg:1: [0, 183308288, 0, 372] is too long From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/msm/dp-controller.example.dt.yaml: example-0: displayport-controller@ae9:reg:2: [0, 183373824, 0, 44] is too long From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1535361 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
Re: [Freedreno] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> > Hi all,
> >
> > One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was to
> > "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what
> > this
> > patch series is about.
> >
> > You will find two types of changes here:
> >
> > - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate)
> > with
> > "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has
> > already been done in previous commits such as b7ea04d2)
> >
> > - Replacing "drm_modeset_lock_all()" with
> > "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> > in the remaining places (as it has already been done in previous commits
> > such as 57037094)
> >
> > Most of the changes are straight forward, except for a few cases in the
> > "amd"
> > and "i915" drivers where some extra dancing was needed to overcome the
> > limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be
> > used
> > once inside the same function (the reason being that the macro expansion
> > includes *labels*, and you can not have two labels named the same inside one
> > function)
> >
> > Notice that, even after this patch series, some places remain where
> > "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still present,
> > all inside drm core (which makes sense), except for two (in "amd" and
> > "i915")
> > which cannot be replaced due to the way they are being used.
> >
> > Changes in v2:
> >
> > - Fix commit message typo
> > - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> > - Split drm/i915 patch into two simpler ones
> > - Remove drm_modeset_(un)lock_all()
> > - Fix build problems in non-x86 platforms
> >
> > Fernando Ramos (17):
> > drm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/i915: cleanup: drm_modeset_lock_all_ctx() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/msm: cleanup: drm_modeset_lock_all_ctx() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/tegra: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/shmobile: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/radeon: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/omapdrm: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/nouveau: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/i915: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN() part 2
> > drm/gma500: cleanup: drm_modeset_lock_all() -->
> > DRM_MODESET_LOCK_ALL_BEGIN()
> > drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> > drm: cleanup: remove drm_modeset_(un)lock_all()
> > doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
> >
>
> Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along
> with the necessary drm-tip conflict resolutions).
Ugh. Did anyone actually review the locking changes this does?
I shot the previous i915 stuff down because the commit messages
did not address any of it.
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Fri, Sep 24, 2021 at 08:43:07AM +0200, Fernando Ramos wrote:
> Hi all,
>
> One of the things in the DRM TODO list ("Documentation/gpu/todo.rst") was to
> "use DRM_MODESET_LOCAL_ALL_* helpers instead of boilerplate". That's what this
> patch series is about.
>
> You will find two types of changes here:
>
> - Replacing "drm_modeset_lock_all_ctx()" (and surrounding boilerplate) with
> "DRM_MODESET_LOCK_ALL_BEGIN()/END()" in the remaining places (as it has
> already been done in previous commits such as b7ea04d2)
>
> - Replacing "drm_modeset_lock_all()" with
> "DRM_MODESET_LOCK_ALL_BEGIN()/END()"
> in the remaining places (as it has already been done in previous commits
> such as 57037094)
>
> Most of the changes are straight forward, except for a few cases in the "amd"
> and "i915" drivers where some extra dancing was needed to overcome the
> limitation that the DRM_MODESET_LOCK_ALL_BEGIN()/END() macros can only be used
> once inside the same function (the reason being that the macro expansion
> includes *labels*, and you can not have two labels named the same inside one
> function)
>
> Notice that, even after this patch series, some places remain where
> "drm_modeset_lock_all()" and "drm_modeset_lock_all_ctx()" are still present,
> all inside drm core (which makes sense), except for two (in "amd" and "i915")
> which cannot be replaced due to the way they are being used.
>
> Changes in v2:
>
> - Fix commit message typo
> - Use the value returned by DRM_MODESET_LOCK_ALL_END when possible
> - Split drm/i915 patch into two simpler ones
> - Remove drm_modeset_(un)lock_all()
> - Fix build problems in non-x86 platforms
>
> Fernando Ramos (17):
> drm: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/i915: cleanup: drm_modeset_lock_all_ctx() -->
> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/msm: cleanup: drm_modeset_lock_all_ctx() -->
> DRM_MODESET_LOCK_ALL_BEGIN()
> drm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/vmwgfx: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/tegra: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/shmobile: cleanup: drm_modeset_lock_all() -->
> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/radeon: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/omapdrm: cleanup: drm_modeset_lock_all() -->
> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/nouveau: cleanup: drm_modeset_lock_all() -->
> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/msm: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/i915: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> part 2
> drm/gma500: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN()
> drm: cleanup: remove drm_modeset_(un)lock_all()
> doc: drm: remove TODO entry regarding DRM_MODSET_LOCK_ALL cleanup
>
Thank you for revising, Fernando! I've pushed the set to drm-misc-next (along
with the necessary drm-tip conflict resolutions).
Sean
> Documentation/gpu/todo.rst| 17
> Documentation/locking/ww-mutex-design.rst | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 21 +++--
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 50 +-
> .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 25 ++---
> drivers/gpu/drm/drm_client_modeset.c | 14 ++-
> drivers/gpu/drm/drm_crtc_helper.c | 18 ++--
> drivers/gpu/drm/drm_fb_helper.c | 10 +-
> drivers/gpu/drm/drm_framebuffer.c | 6 +-
> drivers/gpu/drm/drm_modeset_lock.c| 94 +--
> drivers/gpu/drm/gma500/psb_device.c | 18 ++--
> drivers/gpu/drm/i915/display/intel_audio.c| 16 ++--
> drivers/gpu/drm/i915/display/intel_display.c | 23 ++---
> .../drm/i915/display/intel_display_debugfs.c | 46 +
> drivers/gpu/drm/i915/display/intel_overlay.c | 46 -
> drivers/gpu/drm/i915/display/intel_pipe_crc.c | 7 +-
> drivers/gpu/drm/i915/i915_drv.c | 13 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 10 +-
> .../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 12 +--
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 15 ++-
> drivers/gpu/drm/omapdrm/omap_fb.c | 9 +-
> drivers/gpu/drm/radeon/radeon_device.c| 21 +++--
> drivers/gpu/drm/radeon/radeon_dp_mst.c| 10 +-
> drivers/gpu/drm/shmobile/shmob_drm_drv.c | 6 +-
> drivers/gpu/drm/tegra/dsi.c | 6 +-
> drivers/gpu/drm/tegra/hdmi.c | 6 +-
> drivers/gpu/drm/tegra/sor.c | 11 ++-
> drivers/gpu/drm/vmwgfx/vmwgfx_ioctl.c | 11 ++-
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 12 ++-
> include/drm/drm_modeset_lock.h| 2 -
> 30 files changed, 265
Re: [Freedreno] [PATCH] drm/msm: Fix null pointer dereference on pointer edp
On 29/09/2021 15:18, Colin King wrote:
From: Colin Ian King
The initialization of pointer dev dereferences pointer edp before
edp is null checked, so there is a potential null pointer deference
issue. Fix this by only dereferencing edp after edp has been null
checked.
Addresses-Coverity: ("Dereference before null check")
Fixes: ab5b0107ccf3 ("drm/msm: Initial add eDP support in msm drm driver (v5)")
Signed-off-by: Colin Ian King
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/edp/edp_ctrl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/edp/edp_ctrl.c
b/drivers/gpu/drm/msm/edp/edp_ctrl.c
index 4fb397ee7c84..fe1366b4c49f 100644
--- a/drivers/gpu/drm/msm/edp/edp_ctrl.c
+++ b/drivers/gpu/drm/msm/edp/edp_ctrl.c
@@ -1116,7 +1116,7 @@ void msm_edp_ctrl_power(struct edp_ctrl *ctrl, bool on)
int msm_edp_ctrl_init(struct msm_edp *edp)
{
struct edp_ctrl *ctrl = NULL;
- struct device *dev = >pdev->dev;
+ struct device *dev;
int ret;
if (!edp) {
@@ -1124,6 +1124,7 @@ int msm_edp_ctrl_init(struct msm_edp *edp)
return -EINVAL;
}
+ dev = >pdev->dev;
ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
if (!ctrl)
return -ENOMEM;
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v2 2/3] drm/bridge: ti-sn65dsi86: Implement bridge->mode_valid()
Hi, On Wed, Sep 22, 2021 at 5:31 PM Laurent Pinchart wrote: > > Hi Rob, > > Thank you for the patch. > > On Mon, Sep 20, 2021 at 03:57:59PM -0700, Rob Clark wrote: > > From: Rob Clark > > > > For the brave new world of bridges not creating their own connectors, we > > need to implement the max clock limitation via bridge->mode_valid() > > instead of connector->mode_valid(). > > > > v2: Drop unneeded connector->mode_valid() > > > > Signed-off-by: Rob Clark > > Reviewed-by: Douglas Anderson > > Reviewed-by: Laurent Pinchart > > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 25 + > > 1 file changed, 13 insertions(+), 12 deletions(-) There's no reason to wait on this patch. Landed to drm-misc-next. 77d40e0176a5 drm/bridge: ti-sn65dsi86: Implement bridge->mode_valid() -Doug
Re: [Freedreno] [PATCH v2 3/3] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support
Hi, On Thu, Sep 23, 2021 at 7:26 PM Laurent Pinchart wrote: > > > > > err_conn_init: > > > > drm_dp_aux_unregister(>aux); > > > > return ret; > > > > @@ -792,9 +790,30 @@ static void ti_sn_bridge_set_dsi_rate(struct > > > > ti_sn65dsi86 *pdata) > > > > regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val); > > > > } > > > > > > > > +/* > > > > + * Find the connector and fish out the bpc from display_info. It would > > > > + * be nice if we could get this instead from drm_bridge_state, but that > > > > + * doesn't yet appear to be the case. > > > > > > You already have a bus format in the bridge state, from which you can > > > derive the bpp. Could you give it a try ? > > > > Possibly the bridge should be converted to ->atomic_enable(), etc.. > > I'll leave that for another time > > It should be fairly straightforward, and would avoid the hack below. Given this point of controversy, my inclination is to wait and not apply this patch now. I don't think there's anything urgent here, right? Worst case eventually Laurent might pick it up in his patch series? At least we know it will work with the MSM driver once patch #1 lands. :-) -Doug
Re: [Freedreno] [RFC] drm/msm/a6xx: Serialize GMU communication
On 01/10/2021 21:05, Rob Clark wrote:
On Fri, Oct 1, 2021 at 10:39 AM Dmitry Baryshkov
wrote:
On 27/09/2021 21:03, Rob Clark wrote:
From: Rob Clark
I've seen some crashes in our crash reporting that *look* like multiple
threads stomping on each other while communicating with GMU. So wrap
all those paths in a lock.
Signed-off-by: Rob Clark
---
Are we allowed to use c99/gnu99 yet?
drivers/gpu/drm/msm/Makefile | 2 +-
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 9 +
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 50 ---
4 files changed, 54 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 904535eda0c4..57283bbad3f0 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
-ccflags-y := -I $(srctree)/$(src)
+ccflags-y := -I $(srctree)/$(src) -std=gnu99
ccflags-y += -I $(srctree)/$(src)/disp/dpu1
ccflags-$(CONFIG_DRM_MSM_DSI) += -I $(srctree)/$(src)/dsi
ccflags-$(CONFIG_DRM_MSM_DP) += -I $(srctree)/$(src)/dp
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index a7c58018959f..8b73f70766a4 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -296,6 +296,8 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
u32 val;
int request, ack;
+ WARN_ON_ONCE(!mutex_is_locked(>lock));
+
if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
return -EINVAL;
@@ -337,6 +339,8 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
{
int bit;
+ WARN_ON_ONCE(!mutex_is_locked(>lock));
+
if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
return;
@@ -1482,6 +1486,8 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
device_node *node)
if (!pdev)
return -ENODEV;
+ mutex_init(>lock);
+
gmu->dev = >dev;
of_dma_configure(gmu->dev, node, true);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index 3c74f64e3126..f05a00c0afd0 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -44,6 +44,9 @@ struct a6xx_gmu_bo {
struct a6xx_gmu {
struct device *dev;
+ /* For serializing communication with the GMU: */
+ struct mutex lock;
+
struct msm_gem_address_space *aspace;
void * __iomem mmio;
@@ -88,6 +91,12 @@ struct a6xx_gmu {
bool legacy; /* a618 or a630 */
};
+/* Helper macro for serializing GMU access: */
+#define with_gmu_lock(gmu) \
+ for (bool done = ({ mutex_lock(&(gmu)->lock); false; }); \
+ !done; \
+ done = ({ mutex_unlock(&(gmu)->lock); true; }))
The intent is good, but I'm not sure this kind of syntax sugar would be
a good approach. What about calling lock/unlock explicitly, like we
typically do? Then we won't have to use c99.
Yeah, I was planning to resend without the sugar.. but it was a good
excuse to bring up c99. Ie. I want c99 regardless ;-)
(The sugar was useful initially before I'd sorted thru all the code
paths and settled on using a mutex vs spinlock)
We can always have GMU_LOCK/GMU_UNLOCK macros.
BR,
-R
+
static inline u32 gmu_read(struct a6xx_gmu *gmu, u32 offset)
{
return msm_readl(gmu->mmio + (offset << 2));
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index f6a4dbef796b..5e1ae3df42ba 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -881,7 +881,7 @@ static int a6xx_zap_shader_init(struct msm_gpu *gpu)
A6XX_RBBM_INT_0_MASK_UCHE_OOB_ACCESS | \
A6XX_RBBM_INT_0_MASK_UCHE_TRAP_INTR)
-static int a6xx_hw_init(struct msm_gpu *gpu)
+static int hw_init(struct msm_gpu *gpu)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
@@ -1135,6 +1135,19 @@ static int a6xx_hw_init(struct msm_gpu *gpu)
return ret;
}
+static int a6xx_hw_init(struct msm_gpu *gpu)
+{
+ struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+ struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+ int ret;
+
+ with_gmu_lock(_gpu->gmu) {
+ ret = hw_init(gpu);
+ }
+
+ return ret;
+}
+
static void a6xx_dump(struct msm_gpu *gpu)
{
DRM_DEV_INFO(>pdev->dev, "status: %08x\n",
@@ -1509,7 +1522,9 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
trace_msm_gpu_resume(0);
- ret = a6xx_gmu_resume(a6xx_gpu);
+ with_gmu_lock(_gpu->gmu) {
+ ret = a6xx_gmu_resume(a6xx_gpu);
+ }
if (ret)
return ret;
@@ -1532,7 +1547,9 @@ static int a6xx_pm_suspend(struct msm_gpu *gpu)
msm_devfreq_suspend(gpu);
- ret =
Re: [Freedreno] [RFC] drm/msm/a6xx: Serialize GMU communication
On Fri, Oct 1, 2021 at 10:39 AM Dmitry Baryshkov
wrote:
>
> On 27/09/2021 21:03, Rob Clark wrote:
> > From: Rob Clark
> >
> > I've seen some crashes in our crash reporting that *look* like multiple
> > threads stomping on each other while communicating with GMU. So wrap
> > all those paths in a lock.
> >
> > Signed-off-by: Rob Clark
> > ---
> > Are we allowed to use c99/gnu99 yet?
> >
> > drivers/gpu/drm/msm/Makefile | 2 +-
> > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6
> > drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 9 +
> > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 50 ---
> > 4 files changed, 54 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> > index 904535eda0c4..57283bbad3f0 100644
> > --- a/drivers/gpu/drm/msm/Makefile
> > +++ b/drivers/gpu/drm/msm/Makefile
> > @@ -1,5 +1,5 @@
> > # SPDX-License-Identifier: GPL-2.0
> > -ccflags-y := -I $(srctree)/$(src)
> > +ccflags-y := -I $(srctree)/$(src) -std=gnu99
> > ccflags-y += -I $(srctree)/$(src)/disp/dpu1
> > ccflags-$(CONFIG_DRM_MSM_DSI) += -I $(srctree)/$(src)/dsi
> > ccflags-$(CONFIG_DRM_MSM_DP) += -I $(srctree)/$(src)/dp
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > index a7c58018959f..8b73f70766a4 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > @@ -296,6 +296,8 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
> > a6xx_gmu_oob_state state)
> > u32 val;
> > int request, ack;
> >
> > + WARN_ON_ONCE(!mutex_is_locked(>lock));
> > +
> > if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
> > return -EINVAL;
> >
> > @@ -337,6 +339,8 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum
> > a6xx_gmu_oob_state state)
> > {
> > int bit;
> >
> > + WARN_ON_ONCE(!mutex_is_locked(>lock));
> > +
> > if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
> > return;
> >
> > @@ -1482,6 +1486,8 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct
> > device_node *node)
> > if (!pdev)
> > return -ENODEV;
> >
> > + mutex_init(>lock);
> > +
> > gmu->dev = >dev;
> >
> > of_dma_configure(gmu->dev, node, true);
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> > b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> > index 3c74f64e3126..f05a00c0afd0 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> > @@ -44,6 +44,9 @@ struct a6xx_gmu_bo {
> > struct a6xx_gmu {
> > struct device *dev;
> >
> > + /* For serializing communication with the GMU: */
> > + struct mutex lock;
> > +
> > struct msm_gem_address_space *aspace;
> >
> > void * __iomem mmio;
> > @@ -88,6 +91,12 @@ struct a6xx_gmu {
> > bool legacy; /* a618 or a630 */
> > };
> >
> > +/* Helper macro for serializing GMU access: */
> > +#define with_gmu_lock(gmu) \
> > + for (bool done = ({ mutex_lock(&(gmu)->lock); false; }); \
> > + !done; \
> > + done = ({ mutex_unlock(&(gmu)->lock); true; }))
>
> The intent is good, but I'm not sure this kind of syntax sugar would be
> a good approach. What about calling lock/unlock explicitly, like we
> typically do? Then we won't have to use c99.
Yeah, I was planning to resend without the sugar.. but it was a good
excuse to bring up c99. Ie. I want c99 regardless ;-)
(The sugar was useful initially before I'd sorted thru all the code
paths and settled on using a mutex vs spinlock)
BR,
-R
> > +
> > static inline u32 gmu_read(struct a6xx_gmu *gmu, u32 offset)
> > {
> > return msm_readl(gmu->mmio + (offset << 2));
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > index f6a4dbef796b..5e1ae3df42ba 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> > @@ -881,7 +881,7 @@ static int a6xx_zap_shader_init(struct msm_gpu *gpu)
> > A6XX_RBBM_INT_0_MASK_UCHE_OOB_ACCESS | \
> > A6XX_RBBM_INT_0_MASK_UCHE_TRAP_INTR)
> >
> > -static int a6xx_hw_init(struct msm_gpu *gpu)
> > +static int hw_init(struct msm_gpu *gpu)
> > {
> > struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> > struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> > @@ -1135,6 +1135,19 @@ static int a6xx_hw_init(struct msm_gpu *gpu)
> > return ret;
> > }
> >
> > +static int a6xx_hw_init(struct msm_gpu *gpu)
> > +{
> > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> > + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> > + int ret;
> > +
> > + with_gmu_lock(_gpu->gmu) {
> > + ret = hw_init(gpu);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static void a6xx_dump(struct msm_gpu *gpu)
> > {
> > DRM_DEV_INFO(>pdev->dev, "status: %08x\n",
> > @@ -1509,7 +1522,9 @@ static int
[Freedreno] [PATCH v3 2/5] drm/msm/dp: Modify prototype of encoder based API
Functions in the DisplayPort code that relates to individual instances
(encoders) are passed both the struct msm_dp and the struct drm_encoder. But
in a situation where multiple DP instances would exist this means that
the caller need to resolve which struct msm_dp relates to the struct
drm_encoder at hand.
Store a reference to the struct msm_dp associated with each
dpu_encoder_virt to allow the particular instance to be associate with
the encoder in the following patch.
Reviewed-by: Stephen Boyd
Signed-off-by: Bjorn Andersson
---
Changes since v2:
- None
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 23 -
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0e9d3fa1544b..b7f33da2799c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -168,6 +168,7 @@ enum dpu_enc_rc_states {
* @vsync_event_work: worker to handle vsync event for autorefresh
* @topology: topology of the display
* @idle_timeout: idle timeout duration in milliseconds
+ * @dp:msm_dp pointer, for DP encoders
*/
struct dpu_encoder_virt {
struct drm_encoder base;
@@ -206,6 +207,8 @@ struct dpu_encoder_virt {
struct msm_display_topology topology;
u32 idle_timeout;
+
+ struct msm_dp *dp;
};
#define to_dpu_encoder_virt(x) container_of(x, struct dpu_encoder_virt, base)
@@ -1000,8 +1003,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder
*drm_enc,
trace_dpu_enc_mode_set(DRMID(drm_enc));
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp)
- msm_dp_display_mode_set(priv->dp, drm_enc, mode, adj_mode);
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)
+ msm_dp_display_mode_set(dpu_enc->dp, drm_enc, mode, adj_mode);
list_for_each_entry(conn_iter, connector_list, head)
if (conn_iter->encoder == drm_enc)
@@ -1182,9 +1185,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder
*drm_enc)
_dpu_encoder_virt_enable_helper(drm_enc);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- ret = msm_dp_display_enable(priv->dp,
- drm_enc);
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ ret = msm_dp_display_enable(dpu_enc->dp, drm_enc);
if (ret) {
DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
ret);
@@ -1224,8 +1226,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
/* wait for idle */
dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- if (msm_dp_display_pre_disable(priv->dp, drm_enc))
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ if (msm_dp_display_pre_disable(dpu_enc->dp, drm_enc))
DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
}
@@ -1253,8 +1255,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
- if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
- if (msm_dp_display_disable(priv->dp, drm_enc))
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+ if (msm_dp_display_disable(dpu_enc->dp, drm_enc))
DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
}
@@ -2170,7 +2172,8 @@ int dpu_encoder_setup(struct drm_device *dev, struct
drm_encoder *enc,
timer_setup(_enc->vsync_event_timer,
dpu_encoder_vsync_event_handler,
0);
-
+ else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+ dpu_enc->dp = priv->dp;
INIT_DELAYED_WORK(_enc->delayed_off_work,
dpu_encoder_off_work);
--
2.29.2
[Freedreno] [PATCH v3 3/5] drm/msm/dp: Support up to 3 DP controllers
Based on the removal of the g_dp_display and the movement of the
priv->dp lookup into the DP code it's now possible to have multiple
DP instances.
In line with the other controllers in the MSM driver, introduce a
per-compatible list of base addresses which is used to resolve the
"instance id" for the given DP controller. This instance id is used as
index in the priv->dp[] array.
Then extend the initialization code to initialize struct drm_encoder for
each of the registered priv->dp[] and update the logic for associating
each struct msm_dp with the struct dpu_encoder_virt.
Lastly, bump the number of struct msm_dp instances carries by priv->dp
to 3, the currently known maximum number of controllers found in a
Qualcomm SoC.
Signed-off-by: Bjorn Andersson
---
Changes since v2:
- Added MSM_DRM_DP_COUNT to link the two 3s
- Moved NULL check for msm_dp_debugfs_init() to the call site
- Made struct dp_display_private->id unsigned
I also implemented added connector_type to each of the DP instances and
propagated this to dp_drm_connector_init() but later dropped this again per
Doug's suggestion that we'll base this on the presence/absence of a associated
drm bridge or panel.
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 66 +++
.../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 8 ++-
drivers/gpu/drm/msm/dp/dp_display.c | 44 -
drivers/gpu/drm/msm/msm_drv.h | 4 +-
5 files changed, 90 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index b7f33da2799c..9cd9539a1504 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2173,7 +2173,7 @@ int dpu_encoder_setup(struct drm_device *dev, struct
drm_encoder *enc,
dpu_encoder_vsync_event_handler,
0);
else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
- dpu_enc->dp = priv->dp;
+ dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
INIT_DELAYED_WORK(_enc->delayed_off_work,
dpu_encoder_off_work);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index f655adbc2421..875b07e7183d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -188,6 +188,7 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms, struct
drm_minor *minor)
struct dentry *entry;
struct drm_device *dev;
struct msm_drm_private *priv;
+ int i;
if (!p)
return -EINVAL;
@@ -203,8 +204,10 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms,
struct drm_minor *minor)
dpu_debugfs_vbif_init(dpu_kms, entry);
dpu_debugfs_core_irq_init(dpu_kms, entry);
- if (priv->dp)
- msm_dp_debugfs_init(priv->dp, minor);
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+ if (priv->dp[i])
+ msm_dp_debugfs_init(priv->dp[i], minor);
+ }
return dpu_core_perf_debugfs_init(dpu_kms, entry);
}
@@ -544,35 +547,42 @@ static int _dpu_kms_initialize_displayport(struct
drm_device *dev,
{
struct drm_encoder *encoder = NULL;
struct msm_display_info info;
- int rc = 0;
+ int rc;
+ int i;
- if (!priv->dp)
- return rc;
+ for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+ if (!priv->dp[i])
+ continue;
- encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
- if (IS_ERR(encoder)) {
- DPU_ERROR("encoder init failed for dsi display\n");
- return PTR_ERR(encoder);
- }
+ encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
+ if (IS_ERR(encoder)) {
+ DPU_ERROR("encoder init failed for dsi display\n");
+ return PTR_ERR(encoder);
+ }
- memset(, 0, sizeof(info));
- rc = msm_dp_modeset_init(priv->dp, dev, encoder);
- if (rc) {
- DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
- drm_encoder_cleanup(encoder);
- return rc;
- }
+ memset(, 0, sizeof(info));
+ rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
+ if (rc) {
+ DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
+ drm_encoder_cleanup(encoder);
+ return rc;
+ }
- priv->encoders[priv->num_encoders++] = encoder;
+ priv->encoders[priv->num_encoders++] = encoder;
- info.num_of_h_tiles = 1;
- info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
- info.intf_type = encoder->encoder_type;
- rc =
[Freedreno] [PATCH v3 5/5] drm/msm/dp: Add sc8180x DP controllers
The sc8180x has 2 DP and 1 eDP controllers, add support for these to the
DP driver.
Reviewed-by: Stephen Boyd
Signed-off-by: Bjorn Andersson
---
Changes since v2:
- None
drivers/gpu/drm/msm/dp/dp_display.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index ff3477474c5d..56a79aeffed4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -127,8 +127,15 @@ static const struct msm_dp_config sc7180_dp_cfg = {
.num_descs = 1,
};
+static const struct msm_dp_config sc8180x_dp_cfg = {
+ .io_start = { 0xae9, 0xae98000, 0xae9a000 },
+ .num_descs = 3,
+};
+
static const struct of_device_id dp_dt_match[] = {
{ .compatible = "qcom,sc7180-dp", .data = _dp_cfg },
+ { .compatible = "qcom,sc8180x-dp", .data = _dp_cfg },
+ { .compatible = "qcom,sc8180x-edp", .data = _dp_cfg },
{}
};
--
2.29.2
[Freedreno] [PATCH v3 4/5] dt-bindings: msm/dp: Add SC8180x compatibles
The Qualcomm SC8180x has 2 DP controllers and 1 eDP controller, add compatibles for these to the msm/dp binding. Reviewed-by: Stephen Boyd Signed-off-by: Bjorn Andersson --- Changes since v2: - None .../devicetree/bindings/display/msm/dp-controller.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml index 6bb424c21340..63e585f48789 100644 --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml @@ -17,6 +17,8 @@ properties: compatible: enum: - qcom,sc7180-dp + - qcom,sc8180x-dp + - qcom,sc8180x-edp reg: items: -- 2.29.2
[Freedreno] [PATCH v3 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x
The current implementation supports a single DP instance and the DPU code will only match it against INTF_DP instance 0. These patches extends this to allow multiple DP instances and support for matching against DP instances beyond 0. With that in place add SC8180x DP and eDP controllers. Bjorn Andersson (5): drm/msm/dp: Remove global g_dp_display variable drm/msm/dp: Modify prototype of encoder based API drm/msm/dp: Support up to 3 DP controllers dt-bindings: msm/dp: Add SC8180x compatibles drm/msm/dp: Add sc8180x DP controllers .../bindings/display/msm/dp-controller.yaml | 2 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 23 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 66 + .../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 8 +- drivers/gpu/drm/msm/dp/dp_display.c | 131 +- drivers/gpu/drm/msm/msm_drv.h | 4 +- 6 files changed, 132 insertions(+), 102 deletions(-) -- 2.29.2
[Freedreno] [PATCH v3 1/5] drm/msm/dp: Remove global g_dp_display variable
As the Qualcomm DisplayPort driver only supports a single instance of
the driver the commonly used struct dp_display is kept in a global
variable. As we introduce additional instances this obviously doesn't
work.
Replace this with a combination of existing references to adjacent
objects and drvdata.
Reviewed-by: Stephen Boyd
Signed-off-by: Bjorn Andersson
---
Changes since v2:
- None
drivers/gpu/drm/msm/dp/dp_display.c | 80 -
1 file changed, 21 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index fbe4c2cd52a3..5d3ee5ef07c2 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -27,7 +27,6 @@
#include "dp_audio.h"
#include "dp_debug.h"
-static struct msm_dp *g_dp_display;
#define HPD_STRING_SIZE 30
enum {
@@ -121,6 +120,13 @@ static const struct of_device_id dp_dt_match[] = {
{}
};
+static struct dp_display_private *dev_get_dp_display_private(struct device
*dev)
+{
+ struct msm_dp *dp = dev_get_drvdata(dev);
+
+ return container_of(dp, struct dp_display_private, dp_display);
+}
+
static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
u32 data, u32 delay)
{
@@ -197,15 +203,12 @@ static int dp_display_bind(struct device *dev, struct
device *master,
void *data)
{
int rc = 0;
- struct dp_display_private *dp;
- struct drm_device *drm;
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
struct msm_drm_private *priv;
+ struct drm_device *drm;
drm = dev_get_drvdata(master);
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
-
dp->dp_display.drm_dev = drm;
priv = drm->dev_private;
priv->dp = &(dp->dp_display);
@@ -240,13 +243,10 @@ static int dp_display_bind(struct device *dev, struct
device *master,
static void dp_display_unbind(struct device *dev, struct device *master,
void *data)
{
- struct dp_display_private *dp;
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
struct drm_device *drm = dev_get_drvdata(master);
struct msm_drm_private *priv = drm->dev_private;
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
-
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
priv->dp = NULL;
@@ -379,38 +379,17 @@ static void dp_display_host_deinit(struct
dp_display_private *dp)
static int dp_display_usbpd_configure_cb(struct device *dev)
{
- int rc = 0;
- struct dp_display_private *dp;
-
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- rc = -EINVAL;
- goto end;
- }
-
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
dp_display_host_init(dp, false);
- rc = dp_display_process_hpd_high(dp);
-end:
- return rc;
+ return dp_display_process_hpd_high(dp);
}
static int dp_display_usbpd_disconnect_cb(struct device *dev)
{
int rc = 0;
- struct dp_display_private *dp;
-
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- rc = -EINVAL;
- return rc;
- }
-
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
@@ -472,15 +451,7 @@ static int dp_display_usbpd_attention_cb(struct device
*dev)
{
int rc = 0;
u32 sink_request;
- struct dp_display_private *dp;
-
- if (!dev) {
- DRM_ERROR("invalid dev\n");
- return -EINVAL;
- }
-
- dp = container_of(g_dp_display,
- struct dp_display_private, dp_display);
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
/* check for any test request issued by sink */
rc = dp_link_process_request(dp->link);
@@ -647,7 +618,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private
*dp, u32 data)
DRM_DEBUG_DP("hpd_state=%d\n", state);
/* signal the disconnect event early to ensure proper teardown */
- dp_display_handle_plugged_change(g_dp_display, false);
+ dp_display_handle_plugged_change(>dp_display, false);
/* enable HDP plug interrupt to prepare for next plugin */
dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
@@ -842,9 +813,7 @@ static int dp_display_prepare(struct msm_dp *dp)
static int dp_display_enable(struct dp_display_private *dp, u32 data)
{
Re: [Freedreno] [PATCH v2 1/3] drm/msm/dsi: Support NO_CONNECTOR bridges
On Fri, Oct 1, 2021 at 10:28 AM Dmitry Baryshkov
wrote:
>
> On 21/09/2021 01:57, Rob Clark wrote:
> > From: Rob Clark
> >
> > For now, since we have a mix of bridges which support this flag, which
> > which do *not* support this flag, or work both ways, try it once with
> > NO_CONNECTOR and then fall back to the old way if that doesn't work.
> > Eventually we can drop the fallback path.
> >
> > v2: Add missing drm_connector_attach_encoder() so display actually comes
> > up when the bridge properly handles the NO_CONNECTOR flag
> >
> > Signed-off-by: Rob Clark
> > Reviewed-by: Laurent Pinchart
>
> Reviewed-by: Dmitry Baryshkov
>
> I think this patch can go through the drm/msm, while two other patches
> would need to through the drm-misc. Is it correct?
Correct, I made sure things worked in either order (ie. with msm patch
but without bridge patches, and visa versa), so they can land through
different trees
BR,
-R
>
> > ---
> > drivers/gpu/drm/msm/Kconfig | 2 ++
> > drivers/gpu/drm/msm/dsi/dsi_manager.c | 50 ---
> > 2 files changed, 39 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> > index e9c6af78b1d7..36e5ba3ccc28 100644
> > --- a/drivers/gpu/drm/msm/Kconfig
> > +++ b/drivers/gpu/drm/msm/Kconfig
> > @@ -14,6 +14,8 @@ config DRM_MSM
> > select REGULATOR
> > select DRM_KMS_HELPER
> > select DRM_PANEL
> > + select DRM_BRIDGE
> > + select DRM_PANEL_BRIDGE
> > select DRM_SCHED
> > select SHMEM
> > select TMPFS
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > index c41d39f5b7cf..e25877073d31 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> > @@ -3,6 +3,8 @@
> >* Copyright (c) 2015, The Linux Foundation. All rights reserved.
> >*/
> >
> > +#include "drm/drm_bridge_connector.h"
> > +
> > #include "msm_kms.h"
> > #include "dsi.h"
> >
> > @@ -688,10 +690,10 @@ struct drm_connector
> > *msm_dsi_manager_ext_bridge_init(u8 id)
> > {
> > struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> > struct drm_device *dev = msm_dsi->dev;
> > + struct drm_connector *connector;
> > struct drm_encoder *encoder;
> > struct drm_bridge *int_bridge, *ext_bridge;
> > - struct drm_connector *connector;
> > - struct list_head *connector_list;
> > + int ret;
> >
> > int_bridge = msm_dsi->bridge;
> > ext_bridge = msm_dsi->external_bridge =
> > @@ -699,22 +701,44 @@ struct drm_connector
> > *msm_dsi_manager_ext_bridge_init(u8 id)
> >
> > encoder = msm_dsi->encoder;
> >
> > - /* link the internal dsi bridge to the external bridge */
> > - drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
> > -
> > /*
> > - * we need the drm_connector created by the external bridge
> > - * driver (or someone else) to feed it to our driver's
> > - * priv->connector[] list, mainly for msm_fbdev_init()
> > + * Try first to create the bridge without it creating its own
> > + * connector.. currently some bridges support this, and others
> > + * do not (and some support both modes)
> >*/
> > - connector_list = >mode_config.connector_list;
> > + ret = drm_bridge_attach(encoder, ext_bridge, int_bridge,
> > + DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > + if (ret == -EINVAL) {
> > + struct drm_connector *connector;
> > + struct list_head *connector_list;
> > +
> > + /* link the internal dsi bridge to the external bridge */
> > + drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
> > +
> > + /*
> > + * we need the drm_connector created by the external bridge
> > + * driver (or someone else) to feed it to our driver's
> > + * priv->connector[] list, mainly for msm_fbdev_init()
> > + */
> > + connector_list = >mode_config.connector_list;
> >
> > - list_for_each_entry(connector, connector_list, head) {
> > - if (drm_connector_has_possible_encoder(connector, encoder))
> > - return connector;
> > + list_for_each_entry(connector, connector_list, head) {
> > + if (drm_connector_has_possible_encoder(connector,
> > encoder))
> > + return connector;
> > + }
> > +
> > + return ERR_PTR(-ENODEV);
> > + }
> > +
> > + connector = drm_bridge_connector_init(dev, encoder);
> > + if (IS_ERR(connector)) {
> > + DRM_ERROR("Unable to create bridge connector\n");
> > + return ERR_CAST(connector);
> > }
> >
> > - return ERR_PTR(-ENODEV);
> > + drm_connector_attach_encoder(connector, encoder);
> > +
> > + return connector;
> > }
> >
> > void
[Freedreno] [PATCH 2/2] drm/msm: One sched entity per process per priority
From: Rob Clark
Some userspace apps make assumptions that rendering against multiple
contexts within the same process (from the same thread, with appropriate
MakeCurrent() calls) provides sufficient synchronization without any
external synchronization (ie. glFenceSync()/glWaitSync()). Since a
submitqueue maps to a gl/vk context, having multiple sched entities of
the same priority only works with implicit sync enabled.
To fix this, limit things to a single sched entity per priority level
per process.
An alternative would be sharing submitqueues between contexts in
userspace, but tracking of per-context faults (ie. GL_EXT_robustness)
is already done at the submitqueue level, so this is not an option.
Signed-off-by: Rob Clark
---
Unfortunately, due to a finch experiment (a sort of A/B experiment)
all my testing of the drm/scheduler with chrome(ium) was using
SkiaRenderer which does not trigger this bug. It wasn't until folks
started reporting misrendering on dev channel, and I tracked it down
to legacy GLRenderer vs SkiaRenderer, that I realized the problem :-(
drivers/gpu/drm/msm/msm_gem_submit.c | 2 +-
drivers/gpu/drm/msm/msm_gpu.h | 24 ++
drivers/gpu/drm/msm/msm_submitqueue.c | 68 +++
3 files changed, 74 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 924b01b9c105..34ed56b24224 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -46,7 +46,7 @@ static struct msm_gem_submit *submit_create(struct drm_device
*dev,
if (!submit)
return ERR_PTR(-ENOMEM);
- ret = drm_sched_job_init(>base, >entity, queue);
+ ret = drm_sched_job_init(>base, queue->entity, queue);
if (ret) {
kfree(submit);
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 592334cb9a0b..d72b1de3cb1f 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -290,6 +290,19 @@ struct msm_file_private {
struct msm_gem_address_space *aspace;
struct kref ref;
int seqno;
+
+ /**
+* entities:
+*
+* Table of per-priority-level sched entities used by submitqueues
+* associated with this _file. Because some userspace apps
+* make assumptions about rendering from multiple gl contexts
+* (of the same priority) within the process happening in FIFO
+* order without requiring any fencing beyond MakeCurrent(), we
+* create at most one _sched_entity per-process per-priority-
+* level.
+*/
+ struct drm_sched_entity *entities[NR_SCHED_PRIORITIES *
MSM_GPU_MAX_RINGS];
};
/**
@@ -370,7 +383,7 @@ struct msm_gpu_submitqueue {
struct idr fence_idr;
struct mutex lock;
struct kref ref;
- struct drm_sched_entity entity;
+ struct drm_sched_entity *entity;
};
struct msm_gpu_state_bo {
@@ -471,14 +484,7 @@ void msm_submitqueue_close(struct msm_file_private *ctx);
void msm_submitqueue_destroy(struct kref *kref);
-static inline void __msm_file_private_destroy(struct kref *kref)
-{
- struct msm_file_private *ctx = container_of(kref,
- struct msm_file_private, ref);
-
- msm_gem_address_space_put(ctx->aspace);
- kfree(ctx);
-}
+void __msm_file_private_destroy(struct kref *kref);
static inline void msm_file_private_put(struct msm_file_private *ctx)
{
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c
b/drivers/gpu/drm/msm/msm_submitqueue.c
index 7ce0771b5582..b8621c6e0554 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -7,6 +7,24 @@
#include "msm_gpu.h"
+void __msm_file_private_destroy(struct kref *kref)
+{
+ struct msm_file_private *ctx = container_of(kref,
+ struct msm_file_private, ref);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(ctx->entities); i++) {
+ if (!ctx->entities[i])
+ continue;
+
+ drm_sched_entity_destroy(ctx->entities[i]);
+ kfree(ctx->entities[i]);
+ }
+
+ msm_gem_address_space_put(ctx->aspace);
+ kfree(ctx);
+}
+
void msm_submitqueue_destroy(struct kref *kref)
{
struct msm_gpu_submitqueue *queue = container_of(kref,
@@ -14,8 +32,6 @@ void msm_submitqueue_destroy(struct kref *kref)
idr_destroy(>fence_idr);
- drm_sched_entity_destroy(>entity);
-
msm_file_private_put(queue->ctx);
kfree(queue);
@@ -61,13 +77,47 @@ void msm_submitqueue_close(struct msm_file_private *ctx)
}
}
+static struct drm_sched_entity *
+get_sched_entity(struct msm_file_private *ctx, struct msm_ringbuffer *ring,
+unsigned ring_nr, enum drm_sched_priority sched_prio)
+{
+ static DEFINE_MUTEX(entity_lock);
+ unsigned idx =
[Freedreno] [PATCH 1/2] drm/msm: A bit more docs + cleanup
From: Rob Clark
msm_file_private is more gpu related, and in the next commit it will
need access to other GPU specific #defines. While we're at it, add
some comments.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_drv.h | 44 --
drivers/gpu/drm/msm/msm_gpu.h | 58 ++-
2 files changed, 57 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 8633d0059a3e..31b39c27156d 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -53,15 +53,6 @@ struct msm_disp_state;
#define FRAC_16_16(mult, div)(((mult) << 16) / (div))
-struct msm_file_private {
- rwlock_t queuelock;
- struct list_head submitqueues;
- int queueid;
- struct msm_gem_address_space *aspace;
- struct kref ref;
- int seqno;
-};
-
enum msm_mdp_plane_property {
PLANE_PROP_ZPOS,
PLANE_PROP_ALPHA,
@@ -511,41 +502,6 @@ void msm_hrtimer_work_init(struct msm_hrtimer_work *work,
clockid_t clock_id,
enum hrtimer_mode mode);
-struct msm_gpu_submitqueue;
-int msm_submitqueue_init(struct drm_device *drm, struct msm_file_private *ctx);
-struct msm_gpu_submitqueue *msm_submitqueue_get(struct msm_file_private *ctx,
- u32 id);
-int msm_submitqueue_create(struct drm_device *drm,
- struct msm_file_private *ctx,
- u32 prio, u32 flags, u32 *id);
-int msm_submitqueue_query(struct drm_device *drm, struct msm_file_private *ctx,
- struct drm_msm_submitqueue_query *args);
-int msm_submitqueue_remove(struct msm_file_private *ctx, u32 id);
-void msm_submitqueue_close(struct msm_file_private *ctx);
-
-void msm_submitqueue_destroy(struct kref *kref);
-
-static inline void __msm_file_private_destroy(struct kref *kref)
-{
- struct msm_file_private *ctx = container_of(kref,
- struct msm_file_private, ref);
-
- msm_gem_address_space_put(ctx->aspace);
- kfree(ctx);
-}
-
-static inline void msm_file_private_put(struct msm_file_private *ctx)
-{
- kref_put(>ref, __msm_file_private_destroy);
-}
-
-static inline struct msm_file_private *msm_file_private_get(
- struct msm_file_private *ctx)
-{
- kref_get(>ref);
- return ctx;
-}
-
#define DBG(fmt, ...) DRM_DEBUG_DRIVER(fmt"\n", ##__VA_ARGS__)
#define VERB(fmt, ...) if (0) DRM_DEBUG_DRIVER(fmt"\n", ##__VA_ARGS__)
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 2fcb6c195865..592334cb9a0b 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -272,6 +272,26 @@ struct msm_gpu_perfcntr {
*/
#define NR_SCHED_PRIORITIES (1 + DRM_SCHED_PRIORITY_HIGH -
DRM_SCHED_PRIORITY_MIN)
+/**
+ * struct msm_file_private - per-drm_file context
+ *
+ * @queuelock:synchronizes access to submitqueues list
+ * @submitqueues: list of _gpu_submitqueue created by userspace
+ * @queueid: counter incremented each time a submitqueue is created,
+ *used to assign _gpu_submitqueue.id
+ * @aspace: the per-process GPU address-space
+ * @ref: reference count
+ * @seqno:unique per process seqno
+ */
+struct msm_file_private {
+ rwlock_t queuelock;
+ struct list_head submitqueues;
+ int queueid;
+ struct msm_gem_address_space *aspace;
+ struct kref ref;
+ int seqno;
+};
+
/**
* msm_gpu_convert_priority - Map userspace priority to ring # and sched
priority
*
@@ -319,6 +339,8 @@ static inline int msm_gpu_convert_priority(struct msm_gpu
*gpu, int prio,
}
/**
+ * struct msm_gpu_submitqueues - Userspace created context.
+ *
* A submitqueue is associated with a gl context or vk queue (or equiv)
* in userspace.
*
@@ -336,7 +358,7 @@ static inline int msm_gpu_convert_priority(struct msm_gpu
*gpu, int prio,
* seqno, protected by submitqueue lock
* @lock: submitqueue lock
* @ref: reference count
- * @entity: the submit job-queue
+ * @entity:the submit job-queue
*/
struct msm_gpu_submitqueue {
int id;
@@ -436,6 +458,40 @@ static inline void gpu_write64(struct msm_gpu *gpu, u32
lo, u32 hi, u64 val)
int msm_gpu_pm_suspend(struct msm_gpu *gpu);
int msm_gpu_pm_resume(struct msm_gpu *gpu);
+int msm_submitqueue_init(struct drm_device *drm, struct msm_file_private *ctx);
+struct msm_gpu_submitqueue *msm_submitqueue_get(struct msm_file_private *ctx,
+ u32 id);
+int msm_submitqueue_create(struct drm_device *drm,
+ struct msm_file_private *ctx,
+ u32 prio, u32 flags, u32 *id);
+int msm_submitqueue_query(struct drm_device *drm, struct msm_file_private *ctx,
+ struct drm_msm_submitqueue_query *args);
+int msm_submitqueue_remove(struct msm_file_private *ctx, u32 id);
+void msm_submitqueue_close(struct msm_file_private *ctx);
+
+void
[Freedreno] [PATCH 0/2] drm/msm: Un-break multi-context gl
From: Rob Clark Userspace is expecting that a single thread doing rendering against multiple contexts does not need additional synchronization between those contexts beyond ensuring work is flushed to the kernel in the correct order. But if we have a sched-entity per-context, and are not using implicit sync, GPU jobs from different contexts can execute in a different order than they were flushed to the kernel. To solve that, share sched-entities for a given priority level between submitqueues (which map to gl contexts). Rob Clark (2): drm/msm: A bit more docs + cleanup drm/msm: One sched entity per process per priority drivers/gpu/drm/msm/msm_drv.h | 44 - drivers/gpu/drm/msm/msm_gem_submit.c | 2 +- drivers/gpu/drm/msm/msm_gpu.h | 66 +- drivers/gpu/drm/msm/msm_submitqueue.c | 68 +++ 4 files changed, 123 insertions(+), 57 deletions(-) -- 2.31.1
Re: [Freedreno] [PATCH] drm/msm/dp: Remove redundant initialization of variable bpp
On 29/09/2021 13:54, Colin King wrote:
From: Colin Ian King
The variable bpp is being initialized with a value that is never
read, it is being updated later on in both paths of an if statement.
The assignment is redundant and can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_panel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 2181b60e1d1d..71db10c0f262 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -234,7 +234,7 @@ u32 dp_panel_get_mode_bpp(struct dp_panel *dp_panel,
u32 mode_edid_bpp, u32 mode_pclk_khz)
{
struct dp_panel_private *panel;
- u32 bpp = mode_edid_bpp;
+ u32 bpp;
if (!dp_panel || !mode_edid_bpp || !mode_pclk_khz) {
DRM_ERROR("invalid input\n");
--
With best wishes
Dmitry
[Freedreno] [PATCH v3 5/5] drm/msm/dp: Allow sub-regions to be specified in DT
Not all platforms has P0 at an offset of 0x1000 from the base address,
so add support for specifying each sub-region in DT. The code falls back
to the predefined offsets in the case that only a single reg is
specified, in order to support existing DT.
Reviewed-by: Stephen Boyd
Reviewed-by: Abhinav Kumar
Signed-off-by: Bjorn Andersson
---
Changes since v2:
- None
drivers/gpu/drm/msm/dp/dp_parser.c | 49 +++---
1 file changed, 38 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
b/drivers/gpu/drm/msm/dp/dp_parser.c
index 1f084b2b5bd3..4d6e047f803d 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -50,18 +50,45 @@ static int dp_parser_ctrl_res(struct dp_parser *parser)
if (IS_ERR(dss->ahb.base))
return PTR_ERR(dss->ahb.base);
- if (dss->ahb.len < DP_DEFAULT_P0_OFFSET + DP_DEFAULT_P0_SIZE) {
- DRM_ERROR("legacy memory region not large enough\n");
- return -EINVAL;
- }
+ dss->aux.base = dp_ioremap(pdev, 1, >aux.len);
+ if (IS_ERR(dss->aux.base)) {
+ /*
+* The initial binding had a single reg, but in order to
+* support variation in the sub-region sizes this was split.
+* dp_ioremap() will fail with -ENODEV here if only a single
+* reg is specified, so fill in the sub-region offsets and
+* lengths based on this single region.
+*/
+ if (PTR_ERR(dss->aux.base) == -ENODEV) {
+ if (dss->ahb.len < DP_DEFAULT_P0_OFFSET +
DP_DEFAULT_P0_SIZE) {
+ DRM_ERROR("legacy memory region not large
enough\n");
+ return -EINVAL;
+ }
+
+ dss->ahb.len = DP_DEFAULT_AHB_SIZE;
+ dss->aux.base = dss->ahb.base + DP_DEFAULT_AUX_OFFSET;
+ dss->aux.len = DP_DEFAULT_AUX_SIZE;
+ dss->link.base = dss->ahb.base + DP_DEFAULT_LINK_OFFSET;
+ dss->link.len = DP_DEFAULT_LINK_SIZE;
+ dss->p0.base = dss->ahb.base + DP_DEFAULT_P0_OFFSET;
+ dss->p0.len = DP_DEFAULT_P0_SIZE;
+ } else {
+ DRM_ERROR("unable to remap aux region: %pe\n",
dss->aux.base);
+ return PTR_ERR(dss->aux.base);
+ }
+ } else {
+ dss->link.base = dp_ioremap(pdev, 2, >link.len);
+ if (IS_ERR(dss->link.base)) {
+ DRM_ERROR("unable to remap link region: %pe\n",
dss->link.base);
+ return PTR_ERR(dss->link.base);
+ }
- dss->ahb.len = DP_DEFAULT_AHB_SIZE;
- dss->aux.base = dss->ahb.base + DP_DEFAULT_AUX_OFFSET;
- dss->aux.len = DP_DEFAULT_AUX_SIZE;
- dss->link.base = dss->ahb.base + DP_DEFAULT_LINK_OFFSET;
- dss->link.len = DP_DEFAULT_LINK_SIZE;
- dss->p0.base = dss->ahb.base + DP_DEFAULT_P0_OFFSET;
- dss->p0.len = DP_DEFAULT_P0_SIZE;
+ dss->p0.base = dp_ioremap(pdev, 3, >p0.len);
+ if (IS_ERR(dss->p0.base)) {
+ DRM_ERROR("unable to remap p0 region: %pe\n",
dss->p0.base);
+ return PTR_ERR(dss->p0.base);
+ }
+ }
io->phy = devm_phy_get(>dev, "dp");
if (IS_ERR(io->phy))
--
2.29.2
[Freedreno] [PATCH v3 4/5] drm/msm/dp: Store each subblock in the io region
Not all platforms has DP_P0 at offset 0x1000 from the beginning of the
DP block. So split the dss_io_data memory region into a set of
sub-regions, to make it possible in the next patch to specify each of
the sub-regions individually.
Reviewed-by: Stephen Boyd
Signed-off-by: Bjorn Andersson
---
Changes since v2:
- Skipped the unnecessary reorder in struct dss_io_region
drivers/gpu/drm/msm/dp/dp_catalog.c | 64 +
drivers/gpu/drm/msm/dp/dp_parser.c | 28 +++--
drivers/gpu/drm/msm/dp/dp_parser.h | 9 +++-
3 files changed, 53 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index cc2bb8295329..6ae9b29044b6 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -24,15 +24,6 @@
#define DP_INTERRUPT_STATUS_ACK_SHIFT 1
#define DP_INTERRUPT_STATUS_MASK_SHIFT 2
-#define MSM_DP_CONTROLLER_AHB_OFFSET 0x
-#define MSM_DP_CONTROLLER_AHB_SIZE 0x0200
-#define MSM_DP_CONTROLLER_AUX_OFFSET 0x0200
-#define MSM_DP_CONTROLLER_AUX_SIZE 0x0200
-#define MSM_DP_CONTROLLER_LINK_OFFSET 0x0400
-#define MSM_DP_CONTROLLER_LINK_SIZE0x0C00
-#define MSM_DP_CONTROLLER_P0_OFFSET0x1000
-#define MSM_DP_CONTROLLER_P0_SIZE 0x0400
-
#define DP_INTERRUPT_STATUS1 \
(DP_INTR_AUX_I2C_DONE| \
DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \
@@ -66,82 +57,77 @@ void dp_catalog_snapshot(struct dp_catalog *dp_catalog,
struct msm_disp_state *d
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
+ struct dss_io_data *dss = >io->dp_controller;
- msm_disp_snapshot_add_block(disp_state, catalog->io->dp_controller.len,
- catalog->io->dp_controller.base, "dp_ctrl");
+ msm_disp_snapshot_add_block(disp_state, dss->ahb.len, dss->ahb.base,
"dp_ahb");
+ msm_disp_snapshot_add_block(disp_state, dss->aux.len, dss->aux.base,
"dp_aux");
+ msm_disp_snapshot_add_block(disp_state, dss->link.len, dss->link.base,
"dp_link");
+ msm_disp_snapshot_add_block(disp_state, dss->p0.len, dss->p0.base,
"dp_p0");
}
static inline u32 dp_read_aux(struct dp_catalog_private *catalog, u32 offset)
{
- offset += MSM_DP_CONTROLLER_AUX_OFFSET;
- return readl_relaxed(catalog->io->dp_controller.base + offset);
+ return readl_relaxed(catalog->io->dp_controller.aux.base + offset);
}
static inline void dp_write_aux(struct dp_catalog_private *catalog,
u32 offset, u32 data)
{
- offset += MSM_DP_CONTROLLER_AUX_OFFSET;
/*
* To make sure aux reg writes happens before any other operation,
* this function uses writel() instread of writel_relaxed()
*/
- writel(data, catalog->io->dp_controller.base + offset);
+ writel(data, catalog->io->dp_controller.aux.base + offset);
}
static inline u32 dp_read_ahb(struct dp_catalog_private *catalog, u32 offset)
{
- offset += MSM_DP_CONTROLLER_AHB_OFFSET;
- return readl_relaxed(catalog->io->dp_controller.base + offset);
+ return readl_relaxed(catalog->io->dp_controller.ahb.base + offset);
}
static inline void dp_write_ahb(struct dp_catalog_private *catalog,
u32 offset, u32 data)
{
- offset += MSM_DP_CONTROLLER_AHB_OFFSET;
/*
* To make sure phy reg writes happens before any other operation,
* this function uses writel() instread of writel_relaxed()
*/
- writel(data, catalog->io->dp_controller.base + offset);
+ writel(data, catalog->io->dp_controller.ahb.base + offset);
}
static inline void dp_write_p0(struct dp_catalog_private *catalog,
u32 offset, u32 data)
{
- offset += MSM_DP_CONTROLLER_P0_OFFSET;
/*
* To make sure interface reg writes happens before any other operation,
* this function uses writel() instread of writel_relaxed()
*/
- writel(data, catalog->io->dp_controller.base + offset);
+ writel(data, catalog->io->dp_controller.p0.base + offset);
}
static inline u32 dp_read_p0(struct dp_catalog_private *catalog,
u32 offset)
{
- offset += MSM_DP_CONTROLLER_P0_OFFSET;
/*
* To make sure interface reg writes happens before any other operation,
* this function uses writel() instread of writel_relaxed()
*/
- return readl_relaxed(catalog->io->dp_controller.base + offset);
+ return readl_relaxed(catalog->io->dp_controller.p0.base + offset);
}
static inline u32 dp_read_link(struct dp_catalog_private *catalog, u32 offset)
{
- offset += MSM_DP_CONTROLLER_LINK_OFFSET;
- return readl_relaxed(catalog->io->dp_controller.base + offset);
+ return readl_relaxed(catalog->io->dp_controller.link.base + offset);
}
static
[Freedreno] [PATCH v3 3/5] drm/msm/dp: Refactor ioremap wrapper
In order to deal with multiple memory ranges in the following commit
change the ioremap wrapper to not poke directly into the dss_io_data
struct.
While at it, devm_ioremap_resource() already prints useful error
messages on failure, so omit the unnecessary prints from the caller.
Signed-off-by: Bjorn Andersson
---
Changes since v2:
- Switched to devm_platform_get_and_ioremap_resource()
drivers/gpu/drm/msm/dp/dp_parser.c | 35 ++
drivers/gpu/drm/msm/dp/dp_parser.h | 2 +-
2 files changed, 12 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
b/drivers/gpu/drm/msm/dp/dp_parser.c
index c064ced78278..c05ba1990218 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -19,40 +19,27 @@ static const struct dp_regulator_cfg sdm845_dp_reg_cfg = {
},
};
-static int msm_dss_ioremap(struct platform_device *pdev,
- struct dss_io_data *io_data)
+static void __iomem *dp_ioremap(struct platform_device *pdev, int idx, size_t
*len)
{
- struct resource *res = NULL;
+ struct resource *res;
+ void __iomem *base;
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- DRM_ERROR("%pS->%s: msm_dss_get_res failed\n",
- __builtin_return_address(0), __func__);
- return -ENODEV;
- }
-
- io_data->len = (u32)resource_size(res);
- io_data->base = devm_ioremap(>dev, res->start, io_data->len);
- if (!io_data->base) {
- DRM_ERROR("%pS->%s: ioremap failed\n",
- __builtin_return_address(0), __func__);
- return -EIO;
- }
+ base = devm_platform_get_and_ioremap_resource(pdev, idx, );
+ if (!IS_ERR(base))
+ *len = resource_size(res);
- return 0;
+ return base;
}
static int dp_parser_ctrl_res(struct dp_parser *parser)
{
- int rc = 0;
struct platform_device *pdev = parser->pdev;
struct dp_io *io = >io;
+ struct dss_io_data *dss = >dp_controller;
- rc = msm_dss_ioremap(pdev, >dp_controller);
- if (rc) {
- DRM_ERROR("unable to remap dp io resources, rc=%d\n", rc);
- return rc;
- }
+ dss->base = dp_ioremap(pdev, 0, >len);
+ if (IS_ERR(dss->base))
+ return PTR_ERR(dss->base);
io->phy = devm_phy_get(>dev, "dp");
if (IS_ERR(io->phy))
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h
b/drivers/gpu/drm/msm/dp/dp_parser.h
index 34b49628bbaf..dc62e70b1640 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -26,7 +26,7 @@ enum dp_pm_type {
};
struct dss_io_data {
- u32 len;
+ size_t len;
void __iomem *base;
};
--
2.29.2
[Freedreno] [PATCH v3 2/5] drm/msm/dp: Use devres for ioremap()
The non-devres version of ioremap is used, which requires manual
cleanup. But the code paths leading here is mixed with other devres
users, so rely on this for ioremap as well to simplify the code.
Reviewed-by: Abhinav Kumar
Reviewed-by: Stephen Boyd
Signed-off-by: Bjorn Andersson
---
Changes since v2:
- None
drivers/gpu/drm/msm/dp/dp_parser.c | 29 -
1 file changed, 4 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
b/drivers/gpu/drm/msm/dp/dp_parser.c
index 0519dd3ac3c3..c064ced78278 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -32,7 +32,7 @@ static int msm_dss_ioremap(struct platform_device *pdev,
}
io_data->len = (u32)resource_size(res);
- io_data->base = ioremap(res->start, io_data->len);
+ io_data->base = devm_ioremap(>dev, res->start, io_data->len);
if (!io_data->base) {
DRM_ERROR("%pS->%s: ioremap failed\n",
__builtin_return_address(0), __func__);
@@ -42,22 +42,6 @@ static int msm_dss_ioremap(struct platform_device *pdev,
return 0;
}
-static void msm_dss_iounmap(struct dss_io_data *io_data)
-{
- if (io_data->base) {
- iounmap(io_data->base);
- io_data->base = NULL;
- }
- io_data->len = 0;
-}
-
-static void dp_parser_unmap_io_resources(struct dp_parser *parser)
-{
- struct dp_io *io = >io;
-
- msm_dss_iounmap(>dp_controller);
-}
-
static int dp_parser_ctrl_res(struct dp_parser *parser)
{
int rc = 0;
@@ -67,19 +51,14 @@ static int dp_parser_ctrl_res(struct dp_parser *parser)
rc = msm_dss_ioremap(pdev, >dp_controller);
if (rc) {
DRM_ERROR("unable to remap dp io resources, rc=%d\n", rc);
- goto err;
+ return rc;
}
io->phy = devm_phy_get(>dev, "dp");
- if (IS_ERR(io->phy)) {
- rc = PTR_ERR(io->phy);
- goto err;
- }
+ if (IS_ERR(io->phy))
+ return PTR_ERR(io->phy);
return 0;
-err:
- dp_parser_unmap_io_resources(parser);
- return rc;
}
static int dp_parser_misc(struct dp_parser *parser)
--
2.29.2
[Freedreno] [PATCH v3 1/5] dt-bindings: msm/dp: Change reg definition
reg was defined as one region covering the entire DP block, but the
memory map is actually split in 4 regions and obviously the size of
these regions differs between platforms.
Switch the reg to require that all four regions are specified instead.
It is expected that the implementation will handle existing DTBs, even
though the schema defines the new layout.
Reviewed-by: Stephen Boyd
Reviewed-by: Rob Herring
Signed-off-by: Bjorn Andersson
---
Changes since v2:
- None
.../bindings/display/msm/dp-controller.yaml | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index d89b3c510c27..6bb424c21340 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -19,7 +19,12 @@ properties:
- qcom,sc7180-dp
reg:
-maxItems: 1
+items:
+ - description: ahb register block
+ - description: aux register block
+ - description: link register block
+ - description: p0 register block
+ - description: p1 register block
interrupts:
maxItems: 1
@@ -99,7 +104,11 @@ examples:
displayport-controller@ae9 {
compatible = "qcom,sc7180-dp";
-reg = <0xae9 0x1400>;
+reg = <0xae9 0x200>,
+ <0xae90200 0x200>,
+ <0xae90400 0xc00>,
+ <0xae91000 0x400>,
+ <0xae91400 0x400>;
interrupt-parent = <>;
interrupts = <12>;
clocks = < DISP_CC_MDSS_AHB_CLK>,
--
2.29.2
[Freedreno] [PATCH v3 0/5] drm/msm/dp: Allow variation in register regions
It turns out that sc8180x (among others) doesn't have the same internal layout of the 4 subblocks. This series therefor modifies the binding to require all four regions to be described individually and then extends the driver to read these four regions. The driver will fall back to read the old single-reg format and apply the original offsets and sizes. Bjorn Andersson (5): dt-bindings: msm/dp: Change reg definition drm/msm/dp: Use devres for ioremap() drm/msm/dp: Refactor ioremap wrapper drm/msm/dp: Store each subblock in the io region drm/msm/dp: Allow sub-regions to be specified in DT .../bindings/display/msm/dp-controller.yaml | 13 ++- drivers/gpu/drm/msm/dp/dp_catalog.c | 64 --- drivers/gpu/drm/msm/dp/dp_parser.c| 102 ++ drivers/gpu/drm/msm/dp/dp_parser.h| 11 +- 4 files changed, 100 insertions(+), 90 deletions(-) -- 2.29.2
Re: [Freedreno] [RFC] drm/msm/a6xx: Serialize GMU communication
On 27/09/2021 21:03, Rob Clark wrote:
From: Rob Clark
I've seen some crashes in our crash reporting that *look* like multiple
threads stomping on each other while communicating with GMU. So wrap
all those paths in a lock.
Signed-off-by: Rob Clark
---
Are we allowed to use c99/gnu99 yet?
drivers/gpu/drm/msm/Makefile | 2 +-
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 9 +
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 50 ---
4 files changed, 54 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 904535eda0c4..57283bbad3f0 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
-ccflags-y := -I $(srctree)/$(src)
+ccflags-y := -I $(srctree)/$(src) -std=gnu99
ccflags-y += -I $(srctree)/$(src)/disp/dpu1
ccflags-$(CONFIG_DRM_MSM_DSI) += -I $(srctree)/$(src)/dsi
ccflags-$(CONFIG_DRM_MSM_DP) += -I $(srctree)/$(src)/dp
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index a7c58018959f..8b73f70766a4 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -296,6 +296,8 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
u32 val;
int request, ack;
+ WARN_ON_ONCE(!mutex_is_locked(>lock));
+
if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
return -EINVAL;
@@ -337,6 +339,8 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
{
int bit;
+ WARN_ON_ONCE(!mutex_is_locked(>lock));
+
if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
return;
@@ -1482,6 +1486,8 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
if (!pdev)
return -ENODEV;
+ mutex_init(>lock);
+
gmu->dev = >dev;
of_dma_configure(gmu->dev, node, true);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index 3c74f64e3126..f05a00c0afd0 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -44,6 +44,9 @@ struct a6xx_gmu_bo {
struct a6xx_gmu {
struct device *dev;
+ /* For serializing communication with the GMU: */
+ struct mutex lock;
+
struct msm_gem_address_space *aspace;
void * __iomem mmio;
@@ -88,6 +91,12 @@ struct a6xx_gmu {
bool legacy; /* a618 or a630 */
};
+/* Helper macro for serializing GMU access: */
+#define with_gmu_lock(gmu) \
+ for (bool done = ({ mutex_lock(&(gmu)->lock); false; }); \
+ !done; \
+ done = ({ mutex_unlock(&(gmu)->lock); true; }))
The intent is good, but I'm not sure this kind of syntax sugar would be
a good approach. What about calling lock/unlock explicitly, like we
typically do? Then we won't have to use c99.
+
static inline u32 gmu_read(struct a6xx_gmu *gmu, u32 offset)
{
return msm_readl(gmu->mmio + (offset << 2));
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index f6a4dbef796b..5e1ae3df42ba 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -881,7 +881,7 @@ static int a6xx_zap_shader_init(struct msm_gpu *gpu)
A6XX_RBBM_INT_0_MASK_UCHE_OOB_ACCESS | \
A6XX_RBBM_INT_0_MASK_UCHE_TRAP_INTR)
-static int a6xx_hw_init(struct msm_gpu *gpu)
+static int hw_init(struct msm_gpu *gpu)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
@@ -1135,6 +1135,19 @@ static int a6xx_hw_init(struct msm_gpu *gpu)
return ret;
}
+static int a6xx_hw_init(struct msm_gpu *gpu)
+{
+ struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+ struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+ int ret;
+
+ with_gmu_lock(_gpu->gmu) {
+ ret = hw_init(gpu);
+ }
+
+ return ret;
+}
+
static void a6xx_dump(struct msm_gpu *gpu)
{
DRM_DEV_INFO(>pdev->dev, "status: %08x\n",
@@ -1509,7 +1522,9 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
trace_msm_gpu_resume(0);
- ret = a6xx_gmu_resume(a6xx_gpu);
+ with_gmu_lock(_gpu->gmu) {
+ ret = a6xx_gmu_resume(a6xx_gpu);
+ }
if (ret)
return ret;
@@ -1532,7 +1547,9 @@ static int a6xx_pm_suspend(struct msm_gpu *gpu)
msm_devfreq_suspend(gpu);
- ret = a6xx_gmu_stop(a6xx_gpu);
+ with_gmu_lock(_gpu->gmu) {
+ ret = a6xx_gmu_stop(a6xx_gpu);
+ }
if (ret)
return ret;
@@ -1547,18 +1564,17 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu, uint64_t *value)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
struct a6xx_gpu *a6xx_gpu =
Re: [Freedreno] [PATCH v2 1/3] drm/msm/dsi: Support NO_CONNECTOR bridges
On 21/09/2021 01:57, Rob Clark wrote:
From: Rob Clark
For now, since we have a mix of bridges which support this flag, which
which do *not* support this flag, or work both ways, try it once with
NO_CONNECTOR and then fall back to the old way if that doesn't work.
Eventually we can drop the fallback path.
v2: Add missing drm_connector_attach_encoder() so display actually comes
up when the bridge properly handles the NO_CONNECTOR flag
Signed-off-by: Rob Clark
Reviewed-by: Laurent Pinchart
Reviewed-by: Dmitry Baryshkov
I think this patch can go through the drm/msm, while two other patches
would need to through the drm-misc. Is it correct?
---
drivers/gpu/drm/msm/Kconfig | 2 ++
drivers/gpu/drm/msm/dsi/dsi_manager.c | 50 ---
2 files changed, 39 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index e9c6af78b1d7..36e5ba3ccc28 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -14,6 +14,8 @@ config DRM_MSM
select REGULATOR
select DRM_KMS_HELPER
select DRM_PANEL
+ select DRM_BRIDGE
+ select DRM_PANEL_BRIDGE
select DRM_SCHED
select SHMEM
select TMPFS
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index c41d39f5b7cf..e25877073d31 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -3,6 +3,8 @@
* Copyright (c) 2015, The Linux Foundation. All rights reserved.
*/
+#include "drm/drm_bridge_connector.h"
+
#include "msm_kms.h"
#include "dsi.h"
@@ -688,10 +690,10 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
{
struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
struct drm_device *dev = msm_dsi->dev;
+ struct drm_connector *connector;
struct drm_encoder *encoder;
struct drm_bridge *int_bridge, *ext_bridge;
- struct drm_connector *connector;
- struct list_head *connector_list;
+ int ret;
int_bridge = msm_dsi->bridge;
ext_bridge = msm_dsi->external_bridge =
@@ -699,22 +701,44 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8
id)
encoder = msm_dsi->encoder;
- /* link the internal dsi bridge to the external bridge */
- drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
-
/*
-* we need the drm_connector created by the external bridge
-* driver (or someone else) to feed it to our driver's
-* priv->connector[] list, mainly for msm_fbdev_init()
+* Try first to create the bridge without it creating its own
+* connector.. currently some bridges support this, and others
+* do not (and some support both modes)
*/
- connector_list = >mode_config.connector_list;
+ ret = drm_bridge_attach(encoder, ext_bridge, int_bridge,
+ DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+ if (ret == -EINVAL) {
+ struct drm_connector *connector;
+ struct list_head *connector_list;
+
+ /* link the internal dsi bridge to the external bridge */
+ drm_bridge_attach(encoder, ext_bridge, int_bridge, 0);
+
+ /*
+* we need the drm_connector created by the external bridge
+* driver (or someone else) to feed it to our driver's
+* priv->connector[] list, mainly for msm_fbdev_init()
+*/
+ connector_list = >mode_config.connector_list;
- list_for_each_entry(connector, connector_list, head) {
- if (drm_connector_has_possible_encoder(connector, encoder))
- return connector;
+ list_for_each_entry(connector, connector_list, head) {
+ if (drm_connector_has_possible_encoder(connector,
encoder))
+ return connector;
+ }
+
+ return ERR_PTR(-ENODEV);
+ }
+
+ connector = drm_bridge_connector_init(dev, encoder);
+ if (IS_ERR(connector)) {
+ DRM_ERROR("Unable to create bridge connector\n");
+ return ERR_CAST(connector);
}
- return ERR_PTR(-ENODEV);
+ drm_connector_attach_encoder(connector, encoder);
+
+ return connector;
}
void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] drm: msm: hdmi: Constify static structs
On 21/09/2021 00:20, Rikard Falkeborn wrote:
The only usage of hdmi_8996_pll_ops is to assign its address to the ops
field in the clk_init_data struct, and the only usage of pll_init is to
assign its address to the init field in the clk_hw struct, both which
are pointers to const. Make them const to allow the compiler to put them
in read-only memory.
Signed-off-by: Rikard Falkeborn
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/hdmi/hdmi_phy_8996.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_phy_8996.c
b/drivers/gpu/drm/msm/hdmi/hdmi_phy_8996.c
index a8f3b2cbfdc5..99c7853353fd 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_phy_8996.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_phy_8996.c
@@ -682,7 +682,7 @@ static int hdmi_8996_pll_is_enabled(struct clk_hw *hw)
return pll_locked;
}
-static struct clk_ops hdmi_8996_pll_ops = {
+static const struct clk_ops hdmi_8996_pll_ops = {
.set_rate = hdmi_8996_pll_set_clk_rate,
.round_rate = hdmi_8996_pll_round_rate,
.recalc_rate = hdmi_8996_pll_recalc_rate,
@@ -695,7 +695,7 @@ static const char * const hdmi_pll_parents[] = {
"xo",
};
-static struct clk_init_data pll_init = {
+static const struct clk_init_data pll_init = {
.name = "hdmipll",
.ops = _8996_pll_ops,
.parent_names = hdmi_pll_parents,
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] drm/msm: Avoid potential overflow in timeout_to_jiffies()
On 17/09/2021 03:59, Marek Vasut wrote:
The return type of ktime_divns() is s64. The timeout_to_jiffies() currently
assigns the result of this ktime_divns() to unsigned long, which on 32 bit
systems may overflow. Furthermore, the result of this function is sometimes
also passed to functions which expect signed long, dma_fence_wait_timeout()
is one such example.
Fix this by adjusting the type of remaining_jiffies to s64, so we do not
suffer overflow there, and return a value limited to range of 0..INT_MAX,
which is safe for all usecases of this timeout.
The above overflow can be triggered if userspace passes in too large timeout
value, larger than INT_MAX / HZ seconds. The kernel detects it and complains
about "schedule_timeout: wrong timeout value %lx" and generates a warning
backtrace.
Note that this fixes commit 6cedb8b377bb ("drm/msm: avoid using 'timespec'"),
because the previously used timespec_to_jiffies() function returned unsigned
long instead of s64:
static inline unsigned long timespec_to_jiffies(const struct timespec *value)
Fixes: 6cedb8b377bb ("drm/msm: avoid using 'timespec'")
Signed-off-by: Marek Vasut
Cc: Arnd Bergmann
Cc: Jordan Crouse
Cc: Rob Clark
Cc: sta...@vger.kernel.org # 5.6+
Reviewed-by: Dmitry Baryshkov
---
NOTE: This is related to Mesa MR
https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12886
---
drivers/gpu/drm/msm/msm_drv.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 0b2686b060c73..d96b254b8aa46 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -543,7 +543,7 @@ static inline int align_pitch(int width, int bpp)
static inline unsigned long timeout_to_jiffies(const ktime_t *timeout)
{
ktime_t now = ktime_get();
- unsigned long remaining_jiffies;
+ s64 remaining_jiffies;
if (ktime_compare(*timeout, now) < 0) {
remaining_jiffies = 0;
@@ -552,7 +552,7 @@ static inline unsigned long timeout_to_jiffies(const
ktime_t *timeout)
remaining_jiffies = ktime_divns(rem, NSEC_PER_SEC / HZ);
}
- return remaining_jiffies;
+ return clamp(remaining_jiffies, 0LL, (s64)INT_MAX);
}
#endif /* __MSM_DRV_H__ */
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] drm/msm/dsi: dsi_phy_14nm: Take ready-bit into account in poll_for_ready
On 06/09/2021 23:25, Marijn Suijten wrote:
The downstream driver models this PLL lock check as an if-elseif-else.
The only way to reach the else case where pll_locked=true [1] is by
succeeding both readl_poll_timeout_atomic calls (which return zero on
success) in the if _and_ elseif condition. Hence both the "lock" and
"ready" bit need to be tested in the SM_READY_STATUS register before
considering the PLL locked and ready to go.
Tested on the Sony Xperia XA2 Ultra (nile-discovery, sdm630).
[1]:
https://source.codeaurora.org/quic/la/kernel/msm-4.19/tree/drivers/clk/qcom/mdss/mdss-dsi-pll-14nm-util.c?h=LA.UM.9.2.1.r1-08000-sdm660.0#n302
Fixes: f079f6d999cb ("drm/msm/dsi: Add PHY/PLL for 8x96")
Signed-off-by: Marijn Suijten
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 30 +++---
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
index 8905f365c932..789b08c24d25 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
@@ -110,14 +110,13 @@ static struct dsi_pll_14nm *pll_14nm_list[DSI_MAX];
static bool pll_14nm_poll_for_ready(struct dsi_pll_14nm *pll_14nm,
u32 nb_tries, u32 timeout_us)
{
- bool pll_locked = false;
+ bool pll_locked = false, pll_ready = false;
void __iomem *base = pll_14nm->phy->pll_base;
u32 tries, val;
tries = nb_tries;
while (tries--) {
- val = dsi_phy_read(base +
- REG_DSI_14nm_PHY_PLL_RESET_SM_READY_STATUS);
+ val = dsi_phy_read(base +
REG_DSI_14nm_PHY_PLL_RESET_SM_READY_STATUS);
pll_locked = !!(val & BIT(5));
if (pll_locked)
@@ -126,23 +125,24 @@ static bool pll_14nm_poll_for_ready(struct dsi_pll_14nm
*pll_14nm,
udelay(timeout_us);
}
- if (!pll_locked) {
- tries = nb_tries;
- while (tries--) {
- val = dsi_phy_read(base +
- REG_DSI_14nm_PHY_PLL_RESET_SM_READY_STATUS);
- pll_locked = !!(val & BIT(0));
+ if (!pll_locked)
+ goto out;
- if (pll_locked)
- break;
+ tries = nb_tries;
+ while (tries--) {
+ val = dsi_phy_read(base +
REG_DSI_14nm_PHY_PLL_RESET_SM_READY_STATUS);
+ pll_ready = !!(val & BIT(0));
- udelay(timeout_us);
- }
+ if (pll_ready)
+ break;
+
+ udelay(timeout_us);
}
- DBG("DSI PLL is %slocked", pll_locked ? "" : "*not* ");
+out:
+ DBG("DSI PLL is %slocked, %sready", pll_locked ? "" : "*not* ", pll_ready ? "" :
"*not* ");
- return pll_locked;
+ return pll_locked && pll_ready;
}
static void dsi_pll_14nm_config_init(struct dsi_pll_config *pconf)
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH 0/3] drm/msm: drop old eDP support
On Fri 01 Oct 09:50 PDT 2021, Dmitry Baryshkov wrote: > MSM DRM driver has support for eDP block present on MSM 8x74/8x84 SoC > families. However since addition back in 2015 this driver received only > generic fixes. No actual devices with these SoCs supported upstream (or > by the community) seem to support eDP panels. Judging from downstream > kernels the eDP was present only on MSM8974 LIQUID or on APQ8084 CDP. > Remove this driver. > Nice! Reviewed-by: Bjorn Andersson Regards, Bjorn > > Dmitry Baryshkov (3): > drm/msm/mdp5: drop eDP support > drm/msm/edp: drop old eDP support > dt-bindings: display/msm: remove edp.txt > > .../devicetree/bindings/display/msm/edp.txt| 56 - > drivers/gpu/drm/msm/Makefile |6 - > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 17 +- > drivers/gpu/drm/msm/edp/edp.c | 198 --- > drivers/gpu/drm/msm/edp/edp.h | 77 -- > drivers/gpu/drm/msm/edp/edp.xml.h | 388 -- > drivers/gpu/drm/msm/edp/edp_aux.c | 265 > drivers/gpu/drm/msm/edp/edp_bridge.c | 111 -- > drivers/gpu/drm/msm/edp/edp_connector.c| 132 -- > drivers/gpu/drm/msm/edp/edp_ctrl.c | 1375 > > drivers/gpu/drm/msm/edp/edp_phy.c | 98 -- > drivers/gpu/drm/msm/msm_drv.c |2 - > drivers/gpu/drm/msm/msm_drv.h | 12 - > 13 files changed, 1 insertion(+), 2736 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/display/msm/edp.txt > delete mode 100644 drivers/gpu/drm/msm/edp/edp.c > delete mode 100644 drivers/gpu/drm/msm/edp/edp.h > delete mode 100644 drivers/gpu/drm/msm/edp/edp.xml.h > delete mode 100644 drivers/gpu/drm/msm/edp/edp_aux.c > delete mode 100644 drivers/gpu/drm/msm/edp/edp_bridge.c > delete mode 100644 drivers/gpu/drm/msm/edp/edp_connector.c > delete mode 100644 drivers/gpu/drm/msm/edp/edp_ctrl.c > delete mode 100644 drivers/gpu/drm/msm/edp/edp_phy.c >
[Freedreno] [PATCH 2/3] drm/msm/edp: drop old eDP support
MSM DRM driver has support for eDP block present on MSM 8x74/8x84 SoC
families. However since addition back in 2015 this driver received only
generic fixes. No actual devices with these SoCs supported upstream (or
by the community) seem to support eDP panels. Judging from downstream
kernels the eDP was present only on MSM8974 LIQUID or on APQ8084 CDP.
Remove this driver.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/Makefile|6 -
drivers/gpu/drm/msm/edp/edp.c | 198
drivers/gpu/drm/msm/edp/edp.h | 77 --
drivers/gpu/drm/msm/edp/edp.xml.h | 388 ---
drivers/gpu/drm/msm/edp/edp_aux.c | 265 -
drivers/gpu/drm/msm/edp/edp_bridge.c| 111 --
drivers/gpu/drm/msm/edp/edp_connector.c | 132 ---
drivers/gpu/drm/msm/edp/edp_ctrl.c | 1375 ---
drivers/gpu/drm/msm/edp/edp_phy.c | 98 --
drivers/gpu/drm/msm/msm_drv.c |2 -
drivers/gpu/drm/msm/msm_drv.h | 12 -
11 files changed, 2664 deletions(-)
delete mode 100644 drivers/gpu/drm/msm/edp/edp.c
delete mode 100644 drivers/gpu/drm/msm/edp/edp.h
delete mode 100644 drivers/gpu/drm/msm/edp/edp.xml.h
delete mode 100644 drivers/gpu/drm/msm/edp/edp_aux.c
delete mode 100644 drivers/gpu/drm/msm/edp/edp_bridge.c
delete mode 100644 drivers/gpu/drm/msm/edp/edp_connector.c
delete mode 100644 drivers/gpu/drm/msm/edp/edp_ctrl.c
delete mode 100644 drivers/gpu/drm/msm/edp/edp_phy.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 904535eda0c4..a6f67c8a5105 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -25,12 +25,6 @@ msm-y := \
hdmi/hdmi_phy_8960.o \
hdmi/hdmi_phy_8x60.o \
hdmi/hdmi_phy_8x74.o \
- edp/edp.o \
- edp/edp_aux.o \
- edp/edp_bridge.o \
- edp/edp_connector.o \
- edp/edp_ctrl.o \
- edp/edp_phy.o \
disp/mdp_format.o \
disp/mdp_kms.o \
disp/mdp4/mdp4_crtc.o \
diff --git a/drivers/gpu/drm/msm/edp/edp.c b/drivers/gpu/drm/msm/edp/edp.c
deleted file mode 100644
index 106a67473af5..
--- a/drivers/gpu/drm/msm/edp/edp.c
+++ /dev/null
@@ -1,198 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright (c) 2014-2015, The Linux Foundation. All rights reserved.
- */
-
-#include
-#include "edp.h"
-
-static irqreturn_t edp_irq(int irq, void *dev_id)
-{
- struct msm_edp *edp = dev_id;
-
- /* Process eDP irq */
- return msm_edp_ctrl_irq(edp->ctrl);
-}
-
-static void edp_destroy(struct platform_device *pdev)
-{
- struct msm_edp *edp = platform_get_drvdata(pdev);
-
- if (!edp)
- return;
-
- if (edp->ctrl) {
- msm_edp_ctrl_destroy(edp->ctrl);
- edp->ctrl = NULL;
- }
-
- platform_set_drvdata(pdev, NULL);
-}
-
-/* construct eDP at bind/probe time, grab all the resources. */
-static struct msm_edp *edp_init(struct platform_device *pdev)
-{
- struct msm_edp *edp = NULL;
- int ret;
-
- if (!pdev) {
- pr_err("no eDP device\n");
- ret = -ENXIO;
- goto fail;
- }
-
- edp = devm_kzalloc(>dev, sizeof(*edp), GFP_KERNEL);
- if (!edp) {
- ret = -ENOMEM;
- goto fail;
- }
- DBG("eDP probed=%p", edp);
-
- edp->pdev = pdev;
- platform_set_drvdata(pdev, edp);
-
- ret = msm_edp_ctrl_init(edp);
- if (ret)
- goto fail;
-
- return edp;
-
-fail:
- if (edp)
- edp_destroy(pdev);
-
- return ERR_PTR(ret);
-}
-
-static int edp_bind(struct device *dev, struct device *master, void *data)
-{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
- struct msm_edp *edp;
-
- DBG("");
- edp = edp_init(to_platform_device(dev));
- if (IS_ERR(edp))
- return PTR_ERR(edp);
- priv->edp = edp;
-
- return 0;
-}
-
-static void edp_unbind(struct device *dev, struct device *master, void *data)
-{
- struct drm_device *drm = dev_get_drvdata(master);
- struct msm_drm_private *priv = drm->dev_private;
-
- DBG("");
- if (priv->edp) {
- edp_destroy(to_platform_device(dev));
- priv->edp = NULL;
- }
-}
-
-static const struct component_ops edp_ops = {
- .bind = edp_bind,
- .unbind = edp_unbind,
-};
-
-static int edp_dev_probe(struct platform_device *pdev)
-{
- DBG("");
- return component_add(>dev, _ops);
-}
-
-static int edp_dev_remove(struct platform_device *pdev)
-{
- DBG("");
- component_del(>dev, _ops);
- return 0;
-}
-
-static const struct of_device_id dt_match[] = {
- { .compatible = "qcom,mdss-edp" },
- {}
-};
-
-static struct platform_driver edp_driver = {
- .probe = edp_dev_probe,
- .remove =
[Freedreno] [PATCH 3/3] dt-bindings: display/msm: remove edp.txt
eDP driver is being removed, so drop bindings description.
Signed-off-by: Dmitry Baryshkov
---
.../devicetree/bindings/display/msm/edp.txt | 56 ---
1 file changed, 56 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/display/msm/edp.txt
diff --git a/Documentation/devicetree/bindings/display/msm/edp.txt
b/Documentation/devicetree/bindings/display/msm/edp.txt
deleted file mode 100644
index eff9daff418c..
--- a/Documentation/devicetree/bindings/display/msm/edp.txt
+++ /dev/null
@@ -1,56 +0,0 @@
-Qualcomm Technologies Inc. adreno/snapdragon eDP output
-
-Required properties:
-- compatible:
- * "qcom,mdss-edp"
-- reg: Physical base address and length of the registers of controller and PLL
-- reg-names: The names of register regions. The following regions are required:
- * "edp"
- * "pll_base"
-- interrupts: The interrupt signal from the eDP block.
-- power-domains: Should be < MDSS_GDSC>.
-- clocks: device clocks
- See Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
-- clock-names: the following clocks are required:
- * "core"
- * "iface"
- * "mdp_core"
- * "pixel"
- * "link"
-- #clock-cells: The value should be 1.
-- vdda-supply: phandle to vdda regulator device node
-- lvl-vdd-supply: phandle to regulator device node which is used to supply
power
- to HPD receiving chip
-- panel-en-gpios: GPIO pin to supply power to panel.
-- panel-hpd-gpios: GPIO pin used for eDP hpd.
-
-
-Example:
- mdss_edp: qcom,mdss_edp@fd923400 {
- compatible = "qcom,mdss-edp";
- reg-names =
- "edp",
- "pll_base";
- reg = <0xfd923400 0x700>,
- <0xfd923a00 0xd4>;
- interrupt-parent = <_mdp>;
- interrupts = <12 0>;
- power-domains = < MDSS_GDSC>;
- clock-names =
- "core",
- "pixel",
- "iface",
- "link",
- "mdp_core";
- clocks =
- < MDSS_EDPAUX_CLK>,
- < MDSS_EDPPIXEL_CLK>,
- < MDSS_AHB_CLK>,
- < MDSS_EDPLINK_CLK>,
- < MDSS_MDP_CLK>;
- #clock-cells = <1>;
- vdda-supply = <_l12>;
- lvl-vdd-supply = <_vreg>;
- panel-en-gpios = < 137 0>;
- panel-hpd-gpios = < 103 0>;
- };
--
2.33.0
[Freedreno] [PATCH 0/3] drm/msm: drop old eDP support
MSM DRM driver has support for eDP block present on MSM 8x74/8x84 SoC families. However since addition back in 2015 this driver received only generic fixes. No actual devices with these SoCs supported upstream (or by the community) seem to support eDP panels. Judging from downstream kernels the eDP was present only on MSM8974 LIQUID or on APQ8084 CDP. Remove this driver. Dmitry Baryshkov (3): drm/msm/mdp5: drop eDP support drm/msm/edp: drop old eDP support dt-bindings: display/msm: remove edp.txt .../devicetree/bindings/display/msm/edp.txt| 56 - drivers/gpu/drm/msm/Makefile |6 - drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 17 +- drivers/gpu/drm/msm/edp/edp.c | 198 --- drivers/gpu/drm/msm/edp/edp.h | 77 -- drivers/gpu/drm/msm/edp/edp.xml.h | 388 -- drivers/gpu/drm/msm/edp/edp_aux.c | 265 drivers/gpu/drm/msm/edp/edp_bridge.c | 111 -- drivers/gpu/drm/msm/edp/edp_connector.c| 132 -- drivers/gpu/drm/msm/edp/edp_ctrl.c | 1375 drivers/gpu/drm/msm/edp/edp_phy.c | 98 -- drivers/gpu/drm/msm/msm_drv.c |2 - drivers/gpu/drm/msm/msm_drv.h | 12 - 13 files changed, 1 insertion(+), 2736 deletions(-) delete mode 100644 Documentation/devicetree/bindings/display/msm/edp.txt delete mode 100644 drivers/gpu/drm/msm/edp/edp.c delete mode 100644 drivers/gpu/drm/msm/edp/edp.h delete mode 100644 drivers/gpu/drm/msm/edp/edp.xml.h delete mode 100644 drivers/gpu/drm/msm/edp/edp_aux.c delete mode 100644 drivers/gpu/drm/msm/edp/edp_bridge.c delete mode 100644 drivers/gpu/drm/msm/edp/edp_connector.c delete mode 100644 drivers/gpu/drm/msm/edp/edp_ctrl.c delete mode 100644 drivers/gpu/drm/msm/edp/edp_phy.c
[Freedreno] [PATCH 1/3] drm/msm/mdp5: drop eDP support
Prepare for removing old eDP support present in 8x74/8x84 SoC families.
No devices present in mainline support eDP ports.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 17 +
1 file changed, 1 insertion(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index b3b42672b2d4..21707fcd883d 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -376,22 +376,7 @@ static int modeset_init_intf(struct mdp5_kms *mdp5_kms,
switch (intf->type) {
case INTF_eDP:
- if (!priv->edp)
- break;
-
- ctl = mdp5_ctlm_request(ctlm, intf->num);
- if (!ctl) {
- ret = -EINVAL;
- break;
- }
-
- encoder = construct_encoder(mdp5_kms, intf, ctl);
- if (IS_ERR(encoder)) {
- ret = PTR_ERR(encoder);
- break;
- }
-
- ret = msm_edp_modeset_init(priv->edp, dev, encoder);
+ DRM_DEV_INFO(dev->dev, "Skipping eDP interface %d\n",
intf->num);
break;
case INTF_HDMI:
if (!priv->hdmi)
--
2.33.0
Re: [Freedreno] [PATCH] drm/msm/dsi: Fix some reference counted resource leaks
On 06/08/2021 12:15, Christophe JAILLET wrote:
'of_find_device_by_node()' takes a reference that must be released when
not needed anymore.
This is expected to be done in 'dsi_destroy()'.
However, there are 2 issues in 'dsi_get_phy()'.
First, if 'of_find_device_by_node()' succeeds but 'platform_get_drvdata()'
returns NULL, 'msm_dsi->phy_dev' will still be NULL, and the reference
won't be released in 'dsi_destroy()'.
Secondly, as 'of_find_device_by_node()' already takes a reference, there is
no need for an additional 'get_device()'.
Move the assignment to 'msm_dsi->phy_dev' a few lines above and remove the
unneeded 'get_device()' to solve both issues.
Fixes: ec31abf6684e ("drm/msm/dsi: Separate PHY to another platform device")
Signed-off-by: Christophe JAILLET
Reviewed-by: Dmitry Baryshkov
---
Review carefully, management of reference counted resources is sometimes
tricky.
---
drivers/gpu/drm/msm/dsi/dsi.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 75afc12a7b25..29d11f1cb79b 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -26,8 +26,10 @@ static int dsi_get_phy(struct msm_dsi *msm_dsi)
}
phy_pdev = of_find_device_by_node(phy_node);
- if (phy_pdev)
+ if (phy_pdev) {
msm_dsi->phy = platform_get_drvdata(phy_pdev);
+ msm_dsi->phy_dev = _pdev->dev;
+ }
of_node_put(phy_node);
@@ -36,8 +38,6 @@ static int dsi_get_phy(struct msm_dsi *msm_dsi)
return -EPROBE_DEFER;
}
- msm_dsi->phy_dev = get_device(_pdev->dev);
-
return 0;
}
--
With best wishes
Dmitry
[Freedreno] [PATCH v3 14/14] drm/msm: Implement HDCP 1.x using the new drm HDCP helpers
From: Sean Paul
This patch adds HDCP 1.x support to msm DP connectors using the new HDCP
helpers.
Cc: Stephen Boyd
Cc: Abhinav Kumar
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-15-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-14-s...@poorly.run
#v2
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)
Changes in v3:
-Split out the dtsi change from v2 (Stephen)
-Fix set-but-unused warning identified by 0-day
-Fix up a couple of style nits (Stephen)
-Store HDCP key directly in dp_hdcp struct (Stephen)
-Remove wmb in HDCP key initialization, move an_seed (Stephen)
-Use FIELD_PREP for bstatus/bcaps (Stephen)
-#define read_poll_timeout values (Stephen)
-Remove unnecessary parentheses in dp_hdcp_store_ksv_fifo (Stephen)
-Add compatible string for hdcp (Stephen)
-Rename dp_hdcp_write_* functions (Abhinav)
-Add 1us delay between An reads (Abhinav)
-Delete unused dp_hdcp_read_* functions
[1]
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-14-s...@poorly.run
---
drivers/gpu/drm/msm/Makefile| 1 +
drivers/gpu/drm/msm/dp/dp_debug.c | 46 ++-
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| 445
drivers/gpu/drm/msm/dp/dp_hdcp.h| 27 ++
drivers/gpu/drm/msm/dp/dp_parser.c | 23 +-
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 +
13 files changed, 717 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/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..25dc55b5f083 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,35 @@ 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;
+ struct dp_debug_private *debug = file->private_data;
+
+ 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;
+}
+
static const struct file_operations dp_debug_fops = {
.open = simple_open,
.read = dp_debug_read_info,
@@ -363,6 +395,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;
@@ -384,6 +422,10 @@ static int dp_debug_init(struct dp_debug *dp_debug, struct
drm_minor *minor)
minor->debugfs_root,
debug, _test_type_fops);
+ debugfs_create_file("msm_dp_hdcp_key", 0222,
+ minor->debugfs_root,
+ debug, _hdcp_key_fops);
+
debug->root = minor->debugfs_root;
return rc;
@@
[Freedreno] [PATCH v3 13/14] arm64: dts: qcom: sc7180: Add support for HDCP in dp-controller
From: Sean Paul
This patch adds the register ranges required for HDCP key injection and
HDCP TrustZone interaction as described in the dt-bindings for the
sc7180 dp controller. Now that these are supported, change the
compatible string to "dp-hdcp".
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-15-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-14-s...@poorly.run
#v2
Changes in v3:
-Split off into a new patch containing just the dts change (Stephen)
-Add hdcp compatible string (Stephen)
---
arch/arm64/boot/dts/qcom/sc7180.dtsi | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index c8921e2d6480..f2d7f3c95c1f 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -3085,10 +3085,12 @@ dsi_phy: dsi-phy@ae94400 {
};
mdss_dp: displayport-controller@ae9 {
- compatible = "qcom,sc7180-dp";
+ compatible = "qcom,sc7180-dp-hdcp";
status = "disabled";
- reg = <0 0x0ae9 0 0x1400>;
+ reg = <0 0x0ae9 0 0x1400>,
+ <0 0x0aed1000 0 0x174>,
+ <0 0x0aee1000 0 0x2c>;
interrupt-parent = <>;
interrupts = <12>;
--
Sean Paul, Software Engineer, Google / Chromium OS
[Freedreno] [PATCH v3 12/14] dt-bindings: msm/dp: Add bindings for HDCP registers
From: Sean Paul
This patch adds the bindings for the MSM DisplayPort HDCP registers
which are required to write the HDCP key into the display controller as
well as the registers to enable HDCP authentication/key
exchange/encryption.
We'll use a new compatible string for this since the fields are optional.
Cc: Rob Herring
Cc: Stephen Boyd
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-13-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-13-s...@poorly.run
#v2
Changes in v2:
-Drop register range names (Stephen)
-Fix yaml errors (Rob)
Changes in v3:
-Add new compatible string for dp-hdcp
-Add descriptions to reg
-Add minItems/maxItems to reg
-Make reg depend on the new hdcp compatible string
---
Disclaimer: I really don't know if this is the right way to approach
this. I tried using examples from other bindings, but feedback would be
very much welcome on how I could add the optional register ranges.
.../bindings/display/msm/dp-controller.yaml | 34 ---
1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index 64d8d9e5e47a..a176f97b2f4c 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -17,9 +17,10 @@ properties:
compatible:
enum:
- qcom,sc7180-dp
+ - qcom,sc7180-dp-hdcp
- reg:
-maxItems: 1
+ # See compatible-specific constraints below.
+ reg: true
interrupts:
maxItems: 1
@@ -89,6 +90,29 @@ required:
- power-domains
- ports
+allOf:
+ - if:
+ properties:
+compatible:
+ contains:
+const: qcom,sc7180-dp-hdcp
+then:
+ properties:
+reg:
+ minItems: 3
+ maxItems: 3
+ items:
+- description: Registers for base DP functionality
+- description: (Optional) Registers for HDCP device key injection
+- description: (Optional) Registers for HDCP TrustZone interaction
+else:
+ properties:
+reg:
+ minItems: 1
+ maxItems: 1
+ items:
+- description: Registers for base DP functionality
+
additionalProperties: false
examples:
@@ -99,8 +123,10 @@ examples:
#include
displayport-controller@ae9 {
-compatible = "qcom,sc7180-dp";
-reg = <0xae9 0x1400>;
+compatible = "qcom,sc7180-dp-hdcp";
+reg = <0 0x0ae9 0 0x1400>,
+ <0 0x0aed1000 0 0x174>,
+ <0 0x0aee1000 0 0x2c>;
interrupt-parent = <>;
interrupts = <12>;
clocks = < DISP_CC_MDSS_AHB_CLK>,
--
Sean Paul, Software Engineer, Google / Chromium OS
[Freedreno] [PATCH v3 11/14] drm/msm/dp: Re-order dp_audio_put in deinit_sub_modules
From: Sean Paul
Audio is initialized last, it should be de-initialized first to match
the order in dp_init_sub_modules().
Reviewed-by: Abhinav Kumar
Reviewed-by: Stephen Boyd
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-12-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-12-s...@poorly.run
#v2
Changes in v2:
-None
---
drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index fbe4c2cd52a3..19946024e235 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -714,9 +714,9 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp,
u32 data)
static void dp_display_deinit_sub_modules(struct dp_display_private *dp)
{
dp_debug_put(dp->debug);
+ dp_audio_put(dp->audio);
dp_panel_put(dp->panel);
dp_aux_put(dp->aux);
- dp_audio_put(dp->audio);
}
static int dp_init_sub_modules(struct dp_display_private *dp)
--
Sean Paul, Software Engineer, Google / Chromium OS
[Freedreno] [PATCH v3 10/14] drm/msm/dpu: Remove encoder->enable() hack
From: Sean Paul
encoder->commit() was being misused because there were some global
resources which needed to be tweaked in encoder->enable() which were not
accessible in dpu_encoder.c. That is no longer true and the redirect
serves no purpose any longer. So remove the indirection.
Reviewed-by: Stephen Boyd
Tested-by: Stephen Boyd
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-11-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-11-s...@poorly.run
#v2
Changes in v2:
-None
Changes in v3:
-None
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 22 -
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 --
drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 4
4 files changed, 1 insertion(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 984f8a59cb73..ddc542a0d41f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2122,11 +2122,8 @@ static void dpu_encoder_frame_done_timeout(struct
timer_list *t)
static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
.mode_set = dpu_encoder_virt_mode_set,
.disable = dpu_encoder_virt_disable,
- .enable = dpu_kms_encoder_enable,
+ .enable = dpu_encoder_virt_enable,
.atomic_check = dpu_encoder_virt_atomic_check,
-
- /* This is called by dpu_kms_encoder_enable */
- .commit = dpu_encoder_virt_enable,
};
static const struct drm_encoder_funcs dpu_encoder_funcs = {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index fb0d9f781c66..4a0b55d145ad 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -381,28 +381,6 @@ static void dpu_kms_flush_commit(struct msm_kms *kms,
unsigned crtc_mask)
}
}
-/*
- * Override the encoder enable since we need to setup the inline rotator and do
- * some crtc magic before enabling any bridge that might be present.
- */
-void dpu_kms_encoder_enable(struct drm_encoder *encoder)
-{
- const struct drm_encoder_helper_funcs *funcs = encoder->helper_private;
- struct drm_device *dev = encoder->dev;
- struct drm_crtc *crtc;
-
- /* Forward this enable call to the commit hook */
- if (funcs && funcs->commit)
- funcs->commit(encoder);
-
- drm_for_each_crtc(crtc, dev) {
- if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder)))
- continue;
-
- trace_dpu_kms_enc_enable(DRMID(crtc));
- }
-}
-
static void dpu_kms_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
{
struct dpu_kms *dpu_kms = to_dpu_kms(kms);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 323a6bce9e64..f1ebb60dacab 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -248,8 +248,6 @@ void *dpu_debugfs_get_root(struct dpu_kms *dpu_kms);
int dpu_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
void dpu_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
-void dpu_kms_encoder_enable(struct drm_encoder *encoder);
-
/**
* dpu_kms_get_clk_rate() - get the clock rate
* @dpu_kms: pointer to dpu_kms structure
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
index 37bba57675a8..54d74341e690 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
@@ -266,10 +266,6 @@ DEFINE_EVENT(dpu_drm_obj_template,
dpu_crtc_complete_commit,
TP_PROTO(uint32_t drm_id),
TP_ARGS(drm_id)
);
-DEFINE_EVENT(dpu_drm_obj_template, dpu_kms_enc_enable,
- TP_PROTO(uint32_t drm_id),
- TP_ARGS(drm_id)
-);
DEFINE_EVENT(dpu_drm_obj_template, dpu_kms_commit,
TP_PROTO(uint32_t drm_id),
TP_ARGS(drm_id)
--
Sean Paul, Software Engineer, Google / Chromium OS
[Freedreno] [PATCH v3 09/14] drm/msm/dpu: Remove useless checks in dpu_encoder
From: Sean Paul
A couple more useless checks to remove in dpu_encoder.
Reviewed-by: Stephen Boyd
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-10-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-10-s...@poorly.run
#v2
Changes in v2:
-None
Changes in v3:
-None
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12
1 file changed, 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0e9d3fa1544b..984f8a59cb73 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1153,10 +1153,6 @@ static void dpu_encoder_virt_enable(struct drm_encoder
*drm_enc)
struct msm_drm_private *priv;
struct drm_display_mode *cur_mode = NULL;
- if (!drm_enc) {
- DPU_ERROR("invalid encoder\n");
- return;
- }
dpu_enc = to_dpu_encoder_virt(drm_enc);
mutex_lock(_enc->enc_lock);
@@ -1203,14 +1199,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
struct msm_drm_private *priv;
int i = 0;
- if (!drm_enc) {
- DPU_ERROR("invalid encoder\n");
- return;
- } else if (!drm_enc->dev) {
- DPU_ERROR("invalid dev\n");
- return;
- }
-
dpu_enc = to_dpu_encoder_virt(drm_enc);
DPU_DEBUG_ENC(dpu_enc, "\n");
--
Sean Paul, Software Engineer, Google / Chromium OS
[Freedreno] [PATCH v3 08/14] drm/msm/dpu_kms: Re-order dpu includes
From: Sean Paul Make includes alphabetical in dpu_kms.c Reviewed-by: Abhinav Kumar Reviewed-by: Stephen Boyd Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-9-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-9-s...@poorly.run #v2 Changes in v2: -None Changes in v3: -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" -- Sean Paul, Software Engineer, Google / Chromium OS
[Freedreno] [PATCH v3 07/14] drm/i915/hdcp: Use HDCP helpers for i915
From: Sean Paul
Now that all of the HDCP 1.x logic has been migrated to the central HDCP
helpers, use it in the i915 driver.
The majority of the driver code for HDCP 1.x will live in intel_hdcp.c,
however there are a few helper hooks which are connector-specific and
need to be partially or fully implemented in the intel_dp_hdcp.c or
intel_hdmi.c.
We'll leave most of the HDCP 2.x code alone since we don't have another
implementation of HDCP 2.x to use as reference for what should and
should not live in the drm helpers. The helper will call the overly
general enable/disable/is_capable HDCP 2.x callbacks and leave the
interesting stuff for the driver. Once we have another HDCP 2.x
implementation, we should do a similar migration.
Acked-by: Jani Nikula
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-8-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-8-s...@poorly.run
#v2
Changes in v2:
-Fix mst helper function pointer reported by 0-day
Changes in v3:
-Add forward declaration for drm_atomic_state in intel_hdcp.h identified
by 0-day
---
drivers/gpu/drm/i915/display/intel_ddi.c | 29 +-
.../drm/i915/display/intel_display_debugfs.c | 6 +-
.../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 | 935 +++---
drivers/gpu/drm/i915/display/intel_hdcp.h | 31 +-
drivers/gpu/drm/i915/display/intel_hdmi.c | 256 ++---
8 files changed, 418 insertions(+), 1259 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
b/drivers/gpu/drm/i915/display/intel_ddi.c
index 51cd0420e00e..446544c75aa8 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -26,6 +26,7 @@
*/
#include
+#include
#include "i915_drv.h"
#include "intel_audio.h"
@@ -3143,6 +3144,9 @@ static void intel_enable_ddi(struct intel_atomic_state
*state,
const struct intel_crtc_state *crtc_state,
const struct drm_connector_state *conn_state)
{
+ struct intel_connector *connector =
to_intel_connector(conn_state->connector);
+ struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+
drm_WARN_ON(state->base.dev, crtc_state->has_pch_encoder);
if (!crtc_state->bigjoiner_slave)
@@ -3159,12 +3163,10 @@ static void intel_enable_ddi(struct intel_atomic_state
*state,
else
intel_enable_ddi_dp(state, encoder, crtc_state, conn_state);
- /* Enable hdcp if it's desired */
- if (conn_state->content_protection ==
- DRM_MODE_CONTENT_PROTECTION_DESIRED)
- intel_hdcp_enable(to_intel_connector(conn_state->connector),
- crtc_state,
- (u8)conn_state->hdcp_content_type);
+ if (connector->hdcp_helper_data)
+ drm_hdcp_helper_atomic_commit(connector->hdcp_helper_data,
+ >base,
+ _port->hdcp_mutex);
}
static void intel_disable_ddi_dp(struct intel_atomic_state *state,
@@ -3224,7 +3226,13 @@ static void intel_disable_ddi(struct intel_atomic_state
*state,
const struct intel_crtc_state *old_crtc_state,
const struct drm_connector_state *old_conn_state)
{
- intel_hdcp_disable(to_intel_connector(old_conn_state->connector));
+ struct intel_connector *connector =
to_intel_connector(old_conn_state->connector);
+ struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
+
+ if (connector->hdcp_helper_data)
+ drm_hdcp_helper_atomic_commit(connector->hdcp_helper_data,
+ >base,
+ _port->hdcp_mutex);
if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_HDMI))
intel_disable_ddi_hdmi(state, encoder, old_crtc_state,
@@ -3254,13 +3262,18 @@ void intel_ddi_update_pipe(struct intel_atomic_state
*state,
const struct intel_crtc_state *crtc_state,
const struct drm_connector_state *conn_state)
{
+ struct intel_connector *connector =
to_intel_connector(conn_state->connector);
+ struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) &&
!intel_encoder_is_mst(encoder))
intel_ddi_update_pipe_dp(state, encoder, crtc_state,
conn_state);
- intel_hdcp_update_pipe(state, encoder, crtc_state, conn_state);
+ if (connector->hdcp_helper_data)
+ drm_hdcp_helper_atomic_commit(connector->hdcp_helper_data,
+
[Freedreno] [PATCH v3 06/14] drm/i915/hdcp: Retain hdcp_capable return codes
From: Sean Paul
The shim functions return error codes, but they are discarded in
intel_hdcp.c. This patch plumbs the return codes through so they are
properly handled.
Acked-by: Jani Nikula
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-7-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-7-s...@poorly.run
#v2
Changes in v2:
-None
Changes in v3:
-None
---
.../drm/i915/display/intel_display_debugfs.c | 9 +++-
drivers/gpu/drm/i915/display/intel_hdcp.c | 51 ++-
drivers/gpu/drm/i915/display/intel_hdcp.h | 4 +-
3 files changed, 37 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index 309d74fd86ce..88e71aeb88a2 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -644,6 +644,7 @@ static void intel_panel_info(struct seq_file *m, struct
intel_panel *panel)
static void intel_hdcp_info(struct seq_file *m,
struct intel_connector *intel_connector)
{
+ int ret;
bool hdcp_cap, hdcp2_cap;
if (!intel_connector->hdcp.shim) {
@@ -651,8 +652,12 @@ static void intel_hdcp_info(struct seq_file *m,
goto out;
}
- hdcp_cap = intel_hdcp_capable(intel_connector);
- hdcp2_cap = intel_hdcp2_capable(intel_connector);
+ ret = intel_hdcp_capable(intel_connector, _cap);
+ if (ret)
+ hdcp_cap = false;
+ ret = intel_hdcp2_capable(intel_connector, _cap);
+ if (ret)
+ hdcp2_cap = false;
if (hdcp_cap)
seq_puts(m, "HDCP1.4 ");
diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
b/drivers/gpu/drm/i915/display/intel_hdcp.c
index af166baf8c71..59275919e7b9 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
@@ -153,50 +153,49 @@ int intel_hdcp_read_valid_bksv(struct intel_digital_port
*dig_port,
}
/* Is HDCP1.4 capable on Platform and Sink */
-bool intel_hdcp_capable(struct intel_connector *connector)
+int intel_hdcp_capable(struct intel_connector *connector, bool *capable)
{
struct intel_digital_port *dig_port =
intel_attached_dig_port(connector);
const struct intel_hdcp_shim *shim = connector->hdcp.shim;
- bool capable = false;
u8 bksv[5];
+ *capable = false;
+
if (!shim)
- return capable;
+ return 0;
- if (shim->hdcp_capable) {
- shim->hdcp_capable(dig_port, );
- } else {
- if (!intel_hdcp_read_valid_bksv(dig_port, shim, bksv))
- capable = true;
- }
+ if (shim->hdcp_capable)
+ return shim->hdcp_capable(dig_port, capable);
+
+ if (!intel_hdcp_read_valid_bksv(dig_port, shim, bksv))
+ *capable = true;
- return capable;
+ return 0;
}
/* Is HDCP2.2 capable on Platform and Sink */
-bool intel_hdcp2_capable(struct intel_connector *connector)
+int intel_hdcp2_capable(struct intel_connector *connector, bool *capable)
{
struct intel_digital_port *dig_port =
intel_attached_dig_port(connector);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
struct intel_hdcp *hdcp = >hdcp;
- bool capable = false;
+
+ *capable = false;
/* I915 support for HDCP2.2 */
if (!hdcp->hdcp2_supported)
- return false;
+ return 0;
/* MEI interface is solid */
mutex_lock(_priv->hdcp_comp_mutex);
if (!dev_priv->hdcp_comp_added || !dev_priv->hdcp_master) {
mutex_unlock(_priv->hdcp_comp_mutex);
- return false;
+ return 0;
}
mutex_unlock(_priv->hdcp_comp_mutex);
/* Sink's capability for HDCP2.2 */
- hdcp->shim->hdcp_2_2_capable(dig_port, );
-
- return capable;
+ return hdcp->shim->hdcp_2_2_capable(dig_port, capable);
}
static bool intel_hdcp_in_use(struct drm_i915_private *dev_priv,
@@ -2332,6 +2331,7 @@ int intel_hdcp_enable(struct intel_connector *connector,
struct intel_digital_port *dig_port =
intel_attached_dig_port(connector);
struct intel_hdcp *hdcp = >hdcp;
unsigned long check_link_interval = DRM_HDCP_CHECK_PERIOD_MS;
+ bool capable;
int ret = -EINVAL;
if (!hdcp->shim)
@@ -2350,21 +2350,27 @@ int intel_hdcp_enable(struct intel_connector *connector,
* Considering that HDCP2.2 is more secure than HDCP1.4, If the setup
* is capable of HDCP2.2, it is preferred to use HDCP2.2.
*/
- if (intel_hdcp2_capable(connector)) {
+ ret = intel_hdcp2_capable(connector, );
+ if (capable) {
ret = _intel_hdcp2_enable(connector);
-
[Freedreno] [PATCH v3 05/14] drm/i915/hdcp: Consolidate HDCP setup/state cache
From: Sean Paul
Stick all of the setup for HDCP into a dedicated function. No functional
change, but this will facilitate moving HDCP logic into helpers.
Acked-by: Jani Nikula
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-6-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-6-s...@poorly.run
#v2
Changes in v2:
-None
Changes in v3:
-None
---
drivers/gpu/drm/i915/display/intel_hdcp.c | 52 +++
1 file changed, 35 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
b/drivers/gpu/drm/i915/display/intel_hdcp.c
index feebafead046..af166baf8c71 100644
--- a/drivers/gpu/drm/i915/display/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
@@ -2167,6 +2167,37 @@ static enum mei_fw_tc intel_get_mei_fw_tc(enum
transcoder cpu_transcoder)
}
}
+static int
+_intel_hdcp_setup(struct intel_connector *connector,
+ const struct intel_crtc_state *pipe_config, u8 content_type)
+{
+ struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+ struct intel_digital_port *dig_port =
intel_attached_dig_port(connector);
+ struct intel_hdcp *hdcp = >hdcp;
+ int ret = 0;
+
+ if (!connector->encoder) {
+ drm_err(_priv->drm, "[%s:%d] encoder is not initialized\n",
+ connector->base.name, connector->base.base.id);
+ return -ENODEV;
+ }
+
+ hdcp->content_type = content_type;
+
+ if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) {
+ hdcp->cpu_transcoder = pipe_config->mst_master_transcoder;
+ hdcp->stream_transcoder = pipe_config->cpu_transcoder;
+ } else {
+ hdcp->cpu_transcoder = pipe_config->cpu_transcoder;
+ hdcp->stream_transcoder = INVALID_TRANSCODER;
+ }
+
+ if (DISPLAY_VER(dev_priv) >= 12)
+ dig_port->hdcp_port_data.fw_tc =
intel_get_mei_fw_tc(hdcp->cpu_transcoder);
+
+ return ret;
+}
+
static int initialize_hdcp_port_data(struct intel_connector *connector,
struct intel_digital_port *dig_port,
const struct intel_hdcp_shim *shim)
@@ -2306,28 +2337,14 @@ int intel_hdcp_enable(struct intel_connector *connector,
if (!hdcp->shim)
return -ENOENT;
- if (!connector->encoder) {
- drm_err(_priv->drm, "[%s:%d] encoder is not initialized\n",
- connector->base.name, connector->base.base.id);
- return -ENODEV;
- }
-
mutex_lock(>mutex);
mutex_lock(_port->hdcp_mutex);
drm_WARN_ON(_priv->drm,
hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED);
- hdcp->content_type = content_type;
-
- if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) {
- hdcp->cpu_transcoder = pipe_config->mst_master_transcoder;
- hdcp->stream_transcoder = pipe_config->cpu_transcoder;
- } else {
- hdcp->cpu_transcoder = pipe_config->cpu_transcoder;
- hdcp->stream_transcoder = INVALID_TRANSCODER;
- }
- if (DISPLAY_VER(dev_priv) >= 12)
- dig_port->hdcp_port_data.fw_tc =
intel_get_mei_fw_tc(hdcp->cpu_transcoder);
+ ret = _intel_hdcp_setup(connector, pipe_config, content_type);
+ if (ret)
+ goto out;
/*
* Considering that HDCP2.2 is more secure than HDCP1.4, If the setup
@@ -2355,6 +2372,7 @@ int intel_hdcp_enable(struct intel_connector *connector,
true);
}
+out:
mutex_unlock(_port->hdcp_mutex);
mutex_unlock(>mutex);
return ret;
--
Sean Paul, Software Engineer, Google / Chromium OS
[Freedreno] [PATCH v3 04/14] drm/hdcp: Expand HDCP helper library for enable/disable/check
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.
Cc: Abhinav Kumar
Acked-by: Jani Nikula
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-5-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-5-s...@poorly.run
#v2
Changes in v2:
-Fixed set-but-unused variable identified by 0-day
Changes in v3:
-Fixed uninitialized variable warning 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 8c851d40cd45..2bfa07fc3fbc 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
[Freedreno] [PATCH v3 03/14] drm/hdcp: Update property value on content type and user changes
From: Sean Paul
This patch updates the connector's property value in 2 cases which were
previously missed:
1- Content type changes. The value should revert back to DESIRED from
ENABLED in case the driver must re-authenticate the link due to the
new content type.
2- Userspace sets value to DESIRED while ENABLED. In this case, the
value should be reset immediately to ENABLED since the link is
actively being encrypted.
To accommodate these changes, I've split up the conditionals to make
things a bit more clear (as much as one can with this mess of state).
Acked-by: Jani Nikula
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-4-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-4-s...@poorly.run
#v2
Changes in v2:
-None
Changes in v3:
-Fixed indentation issue identified by 0-day
---
drivers/gpu/drm/drm_hdcp.c | 26 +-
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
index dd8fa91c51d6..8c851d40cd45 100644
--- a/drivers/gpu/drm/drm_hdcp.c
+++ b/drivers/gpu/drm/drm_hdcp.c
@@ -487,21 +487,29 @@ bool drm_hdcp_atomic_check(struct drm_connector
*connector,
return true;
/*
-* Nothing to do if content type is unchanged and one of:
-* - state didn't change
+* Content type changes require an HDCP disable/enable cycle.
+*/
+ if (new_conn_state->hdcp_content_type !=
old_conn_state->hdcp_content_type) {
+ new_conn_state->content_protection =
+ DRM_MODE_CONTENT_PROTECTION_DESIRED;
+ return true;
+ }
+
+ /*
+* Ignore meaningless state changes:
* - HDCP was activated since the last commit
-* - attempting to set to desired while already enabled
+* - Attempting to set to desired while already enabled
*/
- if (old_hdcp == new_hdcp ||
- (old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
+ if ((old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
new_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
(old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
- if (old_conn_state->hdcp_content_type ==
- new_conn_state->hdcp_content_type)
- return false;
+ new_conn_state->content_protection =
+ DRM_MODE_CONTENT_PROTECTION_ENABLED;
+ return false;
}
- return true;
+ /* Finally, if state changes, we need action */
+ return old_hdcp != new_hdcp;
}
EXPORT_SYMBOL(drm_hdcp_atomic_check);
--
Sean Paul, Software Engineer, Google / Chromium OS
[Freedreno] [PATCH v3 02/14] drm/hdcp: Avoid changing crtc state in hdcp atomic check
From: Sean Paul
Instead of forcing a modeset in the hdcp atomic check, simply return
true if the content protection value is changing and let the driver
decide whether a modeset is required or not.
Acked-by: Jani Nikula
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-3-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-3-s...@poorly.run
#v2
Changes in v2:
-None
Changes in v3:
-None
---
drivers/gpu/drm/drm_hdcp.c | 33 +++--
drivers/gpu/drm/i915/display/intel_atomic.c | 5 ++--
include/drm/drm_hdcp.h | 2 +-
3 files changed, 27 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
index 522326b03e66..dd8fa91c51d6 100644
--- a/drivers/gpu/drm/drm_hdcp.c
+++ b/drivers/gpu/drm/drm_hdcp.c
@@ -430,11 +430,14 @@ EXPORT_SYMBOL(drm_hdcp_update_content_protection);
* @connector: drm_connector on which content protection state needs an update
*
* This function can be used by display drivers to perform an atomic check on
the
- * hdcp state elements. If hdcp state has changed, this function will set
- * mode_changed on the crtc driving the connector so it can update its hardware
- * to match the hdcp state.
+ * hdcp state elements. If hdcp state has changed in a manner which requires
the
+ * driver to enable or disable content protection, this function will return
+ * true.
+ *
+ * Returns:
+ * true if the driver must enable/disable hdcp, false otherwise
*/
-void drm_hdcp_atomic_check(struct drm_connector *connector,
+bool drm_hdcp_atomic_check(struct drm_connector *connector,
struct drm_atomic_state *state)
{
struct drm_connector_state *new_conn_state, *old_conn_state;
@@ -452,10 +455,12 @@ void drm_hdcp_atomic_check(struct drm_connector
*connector,
* If the connector is being disabled with CP enabled, mark it
* desired so it's re-enabled when the connector is brought back
*/
- if (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED)
+ if (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
new_conn_state->content_protection =
DRM_MODE_CONTENT_PROTECTION_DESIRED;
- return;
+ return true;
+ }
+ return false;
}
new_crtc_state = drm_atomic_get_new_crtc_state(state,
@@ -467,9 +472,19 @@ void drm_hdcp_atomic_check(struct drm_connector *connector,
*/
if (drm_atomic_crtc_needs_modeset(new_crtc_state) &&
(old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
-new_hdcp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED))
+new_hdcp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)) {
new_conn_state->content_protection =
DRM_MODE_CONTENT_PROTECTION_DESIRED;
+ return true;
+ }
+
+ /*
+* Coming back from disable or changing CRTC with DESIRED state requires
+* that the driver try CP enable.
+*/
+ if (new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
+ new_conn_state->crtc != old_conn_state->crtc)
+ return true;
/*
* Nothing to do if content type is unchanged and one of:
@@ -484,9 +499,9 @@ void drm_hdcp_atomic_check(struct drm_connector *connector,
new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
if (old_conn_state->hdcp_content_type ==
new_conn_state->hdcp_content_type)
- return;
+ return false;
}
- new_crtc_state->mode_changed = true;
+ return true;
}
EXPORT_SYMBOL(drm_hdcp_atomic_check);
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
b/drivers/gpu/drm/i915/display/intel_atomic.c
index 1e306e8427ec..c7b5470c40aa 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -122,8 +122,6 @@ int intel_digital_connector_atomic_check(struct
drm_connector *conn,
to_intel_digital_connector_state(old_state);
struct drm_crtc_state *crtc_state;
- drm_hdcp_atomic_check(conn, state);
-
if (!new_state->crtc)
return 0;
@@ -139,7 +137,8 @@ int intel_digital_connector_atomic_check(struct
drm_connector *conn,
new_conn_state->base.picture_aspect_ratio !=
old_conn_state->base.picture_aspect_ratio ||
new_conn_state->base.content_type !=
old_conn_state->base.content_type ||
new_conn_state->base.scaling_mode !=
old_conn_state->base.scaling_mode ||
- !drm_connector_atomic_hdr_metadata_equal(old_state, new_state))
+ !drm_connector_atomic_hdr_metadata_equal(old_state, new_state) ||
+ drm_hdcp_atomic_check(conn,
[Freedreno] [PATCH v3 01/14] drm/hdcp: Add drm_hdcp_atomic_check()
From: Sean Paul
This patch moves the hdcp atomic check from i915 to drm_hdcp so other
drivers can use it. No functional changes, just cleaned up some of the
code when moving it over.
Acked-by: Jani Nikula
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-2-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-2-s...@poorly.run
#v2
Changes in v2:
-None
Changes in v3:
-None
---
drivers/gpu/drm/drm_hdcp.c | 71 -
drivers/gpu/drm/i915/display/intel_atomic.c | 4 +-
drivers/gpu/drm/i915/display/intel_hdcp.c | 47 --
drivers/gpu/drm/i915/display/intel_hdcp.h | 3 -
include/drm/drm_hdcp.h | 3 +
5 files changed, 75 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
index ca9b8f697202..522326b03e66 100644
--- a/drivers/gpu/drm/drm_hdcp.c
+++ b/drivers/gpu/drm/drm_hdcp.c
@@ -13,13 +13,14 @@
#include
#include
+#include
+#include
#include
#include
#include
#include
#include
#include
-#include
#include "drm_internal.h"
@@ -421,3 +422,71 @@ void drm_hdcp_update_content_protection(struct
drm_connector *connector,
dev->mode_config.content_protection_property);
}
EXPORT_SYMBOL(drm_hdcp_update_content_protection);
+
+/**
+ * drm_hdcp_atomic_check - Helper for drivers to call during
connector->atomic_check
+ *
+ * @state: pointer to the atomic state being checked
+ * @connector: drm_connector on which content protection state needs an update
+ *
+ * This function can be used by display drivers to perform an atomic check on
the
+ * hdcp state elements. If hdcp state has changed, this function will set
+ * mode_changed on the crtc driving the connector so it can update its hardware
+ * to match the hdcp state.
+ */
+void drm_hdcp_atomic_check(struct drm_connector *connector,
+ struct drm_atomic_state *state)
+{
+ struct drm_connector_state *new_conn_state, *old_conn_state;
+ struct drm_crtc_state *new_crtc_state;
+ u64 old_hdcp, new_hdcp;
+
+ old_conn_state = drm_atomic_get_old_connector_state(state, connector);
+ old_hdcp = old_conn_state->content_protection;
+
+ new_conn_state = drm_atomic_get_new_connector_state(state, connector);
+ new_hdcp = new_conn_state->content_protection;
+
+ if (!new_conn_state->crtc) {
+ /*
+* If the connector is being disabled with CP enabled, mark it
+* desired so it's re-enabled when the connector is brought back
+*/
+ if (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED)
+ new_conn_state->content_protection =
+ DRM_MODE_CONTENT_PROTECTION_DESIRED;
+ return;
+ }
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(state,
+ new_conn_state->crtc);
+ /*
+ * Fix the HDCP uapi content protection state in case of modeset.
+ * FIXME: As per HDCP content protection property uapi doc, an uevent()
+ * need to be sent if there is transition from ENABLED->DESIRED.
+ */
+ if (drm_atomic_crtc_needs_modeset(new_crtc_state) &&
+ (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
+new_hdcp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED))
+ new_conn_state->content_protection =
+ DRM_MODE_CONTENT_PROTECTION_DESIRED;
+
+ /*
+* Nothing to do if content type is unchanged and one of:
+* - state didn't change
+* - HDCP was activated since the last commit
+* - attempting to set to desired while already enabled
+*/
+ if (old_hdcp == new_hdcp ||
+ (old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
+new_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
+ (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
+new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
+ if (old_conn_state->hdcp_content_type ==
+ new_conn_state->hdcp_content_type)
+ return;
+ }
+
+ new_crtc_state->mode_changed = true;
+}
+EXPORT_SYMBOL(drm_hdcp_atomic_check);
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
b/drivers/gpu/drm/i915/display/intel_atomic.c
index b4e7ac51aa31..1e306e8427ec 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -32,13 +32,13 @@
#include
#include
#include
+#include
#include
#include "intel_atomic.h"
#include "intel_cdclk.h"
#include "intel_display_types.h"
#include "intel_global_state.h"
-#include "intel_hdcp.h"
#include "intel_psr.h"
#include "skl_universal_plane.h"
@@ -122,7 +122,7 @@ int
[Freedreno] [PATCH v3 00/14] drm/hdcp: Pull HDCP auth/exchange/check into helpers
From: Sean Paul Hello again, Here is v3 of the patch series. Notable changes include incorporating review feedback involving: - Changed dt-bindings to introduce new compatible string - Code changes in msm driver as suggested by Stephen & Abhinav - Fixed issues found by 0-day Thank you to the reviewers for their feedback thus far! Please take a look, Sean Link: https://patchwork.freedesktop.org/series/94623/ #v1 Link: https://patchwork.freedesktop.org/series/94713/ #v2 Sean Paul (14): 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 arm64: dts: qcom: sc7180: Add support for HDCP in dp-controller drm/msm: Implement HDCP 1.x using the new drm HDCP helpers .../bindings/display/msm/dp-controller.yaml | 34 +- arch/arm64/boot/dts/qcom/sc7180.dtsi |6 +- 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 | 36 +- 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 | 46 +- drivers/gpu/drm/msm/dp/dp_debug.h |6 +- drivers/gpu/drm/msm/dp/dp_display.c | 49 +- 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 | 445 ++ drivers/gpu/drm/msm/dp/dp_hdcp.h | 27 + drivers/gpu/drm/msm/dp/dp_parser.c| 23 +- 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, 2600 insertions(+), 1392 deletions(-) create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.c create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.h -- Sean Paul, Software Engineer, Google / Chromium OS
Re: [Freedreno] [PATCH v2 3/5] drm/msm/dp: Support up to 3 DP controllers
On Fri 01 Oct 06:58 PDT 2021, Doug Anderson wrote:
> Hi,
>
> On Thu, Aug 26, 2021 at 10:20 PM Stephen Boyd wrote:
> >
> > Quoting Bjorn Andersson (2021-08-25 16:42:31)
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > > b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index 2c7de43f655a..4a6132c18e57 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -78,6 +78,8 @@ struct dp_display_private {
> > > char *name;
> > > int irq;
> > >
> > > + int id;
> > > +
> > > /* state variables */
> > > bool core_initialized;
> > > bool hpd_irq_on;
> > > @@ -115,8 +117,19 @@ struct dp_display_private {
> > > struct dp_audio *audio;
> > > };
> > >
> > > +
> > > +struct msm_dp_config {
> > > + phys_addr_t io_start[3];
> >
> > Can this be made into another struct, like msm_dp_desc, that also
> > indicates what type of DP connector it is, i.e. eDP vs DP? That would
> > help me understand in modetest and /sys/class/drm what sort of connector
> > is probing. dp_drm_connector_init() would need to pass the type of
> > connector appropriately. Right now, eDP connectors still show up as DP
> > instead of eDP in sysfs.
>
> I'm not convinced that's the right way to do it. I think the right way
> forward here is to look at whether drm_of_find_panel_or_bridge() finds
> a panel. If it finds a panel then we're eDP. If it doesn't then we're
> DP. That matches roughly what Laurent was planning to do for
> ti-sn65dsi86:
>
> https://lore.kernel.org/all/20210322030128.2283-11-laurent.pinchart+rene...@ideasonboard.com/
>
When we spoke about this earlier I got the impression that there was
interest in representing the DP display as a panel at some point in the
future. Did I misunderstand you or would we simply update the scheme
when that day comes?
I updated the patch based on Stephen's input and it looks nice, but I
could certainly respin it again to simply rely on us having an explicit
panel or not.
> I know technically an eDP and DP controller can have different sets of
> features but I think what we really are trying to communicate to
> modetest is whether an internal panel or external display is
> connected, right?
>
For me Stephen's suggestion resulted in the monitor names in Wayland
suddenly making more sense, i.e. it makes more sense to say that the lid
on my laptop should control eDP-1, rather than DP-3 on this machine...
Regards,
Bjorn
Re: [Freedreno] [PATCH v2 3/5] drm/msm/dp: Support up to 3 DP controllers
Hi,
On Thu, Aug 26, 2021 at 10:20 PM Stephen Boyd wrote:
>
> Quoting Bjorn Andersson (2021-08-25 16:42:31)
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 2c7de43f655a..4a6132c18e57 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -78,6 +78,8 @@ struct dp_display_private {
> > char *name;
> > int irq;
> >
> > + int id;
> > +
> > /* state variables */
> > bool core_initialized;
> > bool hpd_irq_on;
> > @@ -115,8 +117,19 @@ struct dp_display_private {
> > struct dp_audio *audio;
> > };
> >
> > +
> > +struct msm_dp_config {
> > + phys_addr_t io_start[3];
>
> Can this be made into another struct, like msm_dp_desc, that also
> indicates what type of DP connector it is, i.e. eDP vs DP? That would
> help me understand in modetest and /sys/class/drm what sort of connector
> is probing. dp_drm_connector_init() would need to pass the type of
> connector appropriately. Right now, eDP connectors still show up as DP
> instead of eDP in sysfs.
I'm not convinced that's the right way to do it. I think the right way
forward here is to look at whether drm_of_find_panel_or_bridge() finds
a panel. If it finds a panel then we're eDP. If it doesn't then we're
DP. That matches roughly what Laurent was planning to do for
ti-sn65dsi86:
https://lore.kernel.org/all/20210322030128.2283-11-laurent.pinchart+rene...@ideasonboard.com/
I know technically an eDP and DP controller can have different sets of
features but I think what we really are trying to communicate to
modetest is whether an internal panel or external display is
connected, right?
-Doug
[Freedreno] [PATCH] drm/msm/a3xx: fix error handling in a3xx_gpu_init()
These error paths returned 1 on failure, instead of a negative error
code. This would lead to an Oops in the caller. A second problem is
that the check for "if (ret != -ENODATA)" did not work because "ret" was
set to 1.
Fixes: 5785dd7a8ef0 ("drm/msm: Fix duplicate gpu node in icc summary")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
index 4534633fe7cd..8fb847c174ff 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
@@ -571,13 +571,14 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
}
icc_path = devm_of_icc_get(>dev, "gfx-mem");
- ret = IS_ERR(icc_path);
- if (ret)
+ if (IS_ERR(icc_path)) {
+ ret = PTR_ERR(icc_path);
goto fail;
+ }
ocmem_icc_path = devm_of_icc_get(>dev, "ocmem");
- ret = IS_ERR(ocmem_icc_path);
- if (ret) {
+ if (IS_ERR(ocmem_icc_path)) {
+ ret = PTR_ERR(ocmem_icc_path);
/* allow -ENODATA, ocmem icc is optional */
if (ret != -ENODATA)
goto fail;
--
2.20.1
[Freedreno] [PATCH] drm/msm/a4xx: fix error handling in a4xx_gpu_init()
This code returns 1 on error instead of a negative error. It leads to
an Oops in the caller. A second problem is that the check for
"if (ret != -ENODATA)" cannot be true because "ret" is set to 1.
Fixes: 5785dd7a8ef0 ("drm/msm: Fix duplicate gpu node in icc summary")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
index 82bebb40234d..a96ee79cc5e0 100644
--- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
@@ -699,13 +699,14 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
}
icc_path = devm_of_icc_get(>dev, "gfx-mem");
- ret = IS_ERR(icc_path);
- if (ret)
+ if (IS_ERR(icc_path)) {
+ ret = PTR_ERR(icc_path);
goto fail;
+ }
ocmem_icc_path = devm_of_icc_get(>dev, "ocmem");
- ret = IS_ERR(ocmem_icc_path);
- if (ret) {
+ if (IS_ERR(ocmem_icc_path)) {
+ ret = PTR_ERR(ocmem_icc_path);
/* allow -ENODATA, ocmem icc is optional */
if (ret != -ENODATA)
goto fail;
--
2.20.1
[Freedreno] [PATCH 3/3] drm/msm/dsi: fix signedness bug in msm_dsi_host_cmd_rx()
The "msg->tx_len" variable is type size_t so if dsi_cmds2buf_tx()
returns a negative error code that it type promoted to a high positive
value and treat as a success. The second problem with this code is
that it can return meaningless positive values on error.
Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index c86b5090fae6..42073a562072 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -2133,8 +2133,10 @@ int msm_dsi_host_cmd_rx(struct mipi_dsi_host *host,
}
ret = dsi_cmds2buf_tx(msm_host, msg);
- if (ret < msg->tx_len) {
+ if (ret < 0 || ret < msg->tx_len) {
pr_err("%s: Read cmd Tx failed, %d\n", __func__, ret);
+ if (ret >= 0)
+ ret = -EIO;
return ret;
}
--
2.20.1
[Freedreno] [PATCH 2/3] drm/msm/dsi: fix off by one in dsi_bus_clk_enable error handling
This disables a lock which wasn't enabled and it does not disable
the first lock in the array.
Fixes: 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e269df285136..c86b5090fae6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -451,7 +451,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)
return 0;
err:
- for (; i > 0; i--)
+ while (--i >= 0)
clk_disable_unprepare(msm_host->bus_clks[i]);
return ret;
--
2.20.1
[Freedreno] [PATCH 1/3] drm/msm/dsi: Fix an error code in msm_dsi_modeset_init()
Return an error code if msm_dsi_manager_validate_current_config().
Don't return success.
Fixes: 8b03ad30e314 ("drm/msm/dsi: Use one connector for dual DSI mode")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/dsi/dsi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 614dc7f26f2c..75ae3008b68f 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -215,8 +215,10 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct
drm_device *dev,
goto fail;
}
- if (!msm_dsi_manager_validate_current_config(msm_dsi->id))
+ if (!msm_dsi_manager_validate_current_config(msm_dsi->id)) {
+ ret = -EINVAL;
goto fail;
+ }
msm_dsi->encoder = encoder;
--
2.20.1
Re: [Freedreno] [PATCH v1 2/4] arm64: dts: qcom: sc7280: add display dt nodes
On 2021-09-30 23:28, Stephen Boyd wrote:
Quoting mkri...@codeaurora.org (2021-09-30 04:56:59)
On 2021-08-19 01:27, Stephen Boyd wrote:
> Quoting Krishna Manikandan (2021-08-18 03:27:02)
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 53a21d0..fd7ff1c 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +
>> + status = "disabled";
>> +
>> + mdp: mdp@ae01000 {
>
> display-controller@ae01000
Stephen,
In the current driver code, there is a substring comparison for
"mdp"
in device node name as part of probe sequence. If "mdp" is not present
in the node name, it will
return an error resulting in probe failure. Can we continue using
mdp
as nodename instead of display controller?
Can we fix the driver to not look for node names and look for
compatible
strings instead? It took me a minute to find compare_name_mdp() in
drivers/gpu/drm/msm/msm_drv.c to understand what you're talking about.
Perhaps looking for qcom,mdp5 in there will be sufficient instead of
looking at the node name.
Sure Stephen. I will make the necessary changes in msm_drv.c to look for
compatible string instead of node name.
Can I include these two changes (changing mdp--> display controller and
msm_drv.c changes) in a separate series ?
Thanks,
Krishna
