Re: [Freedreno] [PATCH v5 4/5] drm/msm/a6xx: Remove cx gdsc polling using 'reset'
On Mon, Jan 02, 2023 at 04:18:30PM +0530, Akhil P Oommen wrote:
> Remove the unused 'reset' interface which was supposed to help to ensure
> that cx gdsc has collapsed during gpu recovery. This is was not enabled
> so far due to missing gpucc driver support. Similar functionality using
> genpd framework will be implemented in the upcoming patch.
>
> This effectively reverts commit 1f6cca404918
> ("drm/msm/a6xx: Ensure CX collapse during gpu recovery").
>
> Signed-off-by: Akhil P Oommen
> Reviewed-by: Ulf Hansson
Reviewed-by: Philipp Zabel
regards
Philipp
Re: [Freedreno] [PATCH v2 4/5] drm/msm/a6xx: Remove cx gdsc polling using 'reset'
On Fr, 2022-12-16 at 15:51 +0530, Akhil P Oommen wrote:
> Remove the unused 'reset' interface which was supposed to help to ensure
> that cx gdsc has collapsed during gpu recovery. This is was not enabled
> so far due to missing gpucc driver support. Similar functionality using
> genpd framework will be implemented in the upcoming patch.
Maybe mention that this effectively reverts commit 1f6cca404918
("drm/msm/a6xx: Ensure CX collapse during gpu recovery").
regards
Philipp
Re: [Freedreno] [PATCH v6 4/6] clk: qcom: gpucc-sc7280: Add cx collapse reset support
On Wed, Aug 31, 2022 at 10:48:25AM +0530, Akhil P Oommen wrote:
> Allow a consumer driver to poll for cx gdsc collapse through Reset
> framework.
>
> Signed-off-by: Akhil P Oommen
> Reviewed-by: Dmitry Baryshkov
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Convert 'struct qcom_reset_ops cx_gdsc_reset' to 'static const' (Krzysztof)
>
> Changes in v2:
> - Minor update to use the updated custom reset ops implementation
>
> drivers/clk/qcom/gpucc-sc7280.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/clk/qcom/gpucc-sc7280.c b/drivers/clk/qcom/gpucc-sc7280.c
> index 9a832f2..fece3f4 100644
> --- a/drivers/clk/qcom/gpucc-sc7280.c
> +++ b/drivers/clk/qcom/gpucc-sc7280.c
> @@ -433,12 +433,22 @@ static const struct regmap_config
> gpu_cc_sc7280_regmap_config = {
> .fast_io = true,
> };
>
> +static const struct qcom_reset_ops cx_gdsc_reset = {
> + .reset = gdsc_wait_for_collapse,
This should be accompanied by a comment explaining the not-quite-reset
nature of this workaround, i.e. what is the prerequisite for this to
actually work as expected?
> +};
> +
> +static const struct qcom_reset_map gpucc_sc7280_resets[] = {
> + [GPU_CX_COLLAPSE] = { .ops = _gdsc_reset, .priv = _gdsc },
> +};
> +
> static const struct qcom_cc_desc gpu_cc_sc7280_desc = {
> .config = _cc_sc7280_regmap_config,
> .clks = gpu_cc_sc7280_clocks,
> .num_clks = ARRAY_SIZE(gpu_cc_sc7280_clocks),
> .gdscs = gpu_cc_sc7180_gdscs,
> .num_gdscs = ARRAY_SIZE(gpu_cc_sc7180_gdscs),
> + .resets = gpucc_sc7280_resets,
> + .num_resets = ARRAY_SIZE(gpucc_sc7280_resets),
See my comment on patch 2. I think instead of adding a const struct
qcom_reset_ops * to gpucc_sc7280_resets, this should just add a const
struct reset_control * to gpu_cc_sc7280_desc.
regards
Philipp
Re: [Freedreno] [PATCH v6 3/6] clk: qcom: gdsc: Add a reset op to poll gdsc collapse
On Wed, Aug 31, 2022 at 10:48:24AM +0530, Akhil P Oommen wrote:
> Add a reset op compatible function to poll for gdsc collapse.
>
> Signed-off-by: Akhil P Oommen
> Reviewed-by: Dmitry Baryshkov
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Minor update to function prototype
>
> drivers/clk/qcom/gdsc.c | 23 +++
> drivers/clk/qcom/gdsc.h | 7 +++
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 44520ef..2d0f1d1 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -17,6 +17,7 @@
> #include
> #include
> #include "gdsc.h"
> +#include "reset.h"
>
> #define PWR_ON_MASK BIT(31)
> #define EN_REST_WAIT_MASKGENMASK_ULL(23, 20)
> @@ -116,7 +117,8 @@ static int gdsc_hwctrl(struct gdsc *sc, bool en)
> return regmap_update_bits(sc->regmap, sc->gdscr, HW_CONTROL_MASK, val);
> }
>
> -static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
> +static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status,
> + s64 timeout_us, unsigned int interval_ms)
> {
> ktime_t start;
>
> @@ -124,7 +126,9 @@ static int gdsc_poll_status(struct gdsc *sc, enum
> gdsc_status status)
> do {
> if (gdsc_check_status(sc, status))
> return 0;
> - } while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
> + if (interval_ms)
> + msleep(interval_ms);
> + } while (ktime_us_delta(ktime_get(), start) < timeout_us);
Could this loop be implemented with read_poll_timeout()?
> if (gdsc_check_status(sc, status))
> return 0;
> @@ -172,7 +176,7 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum
> gdsc_status status)
> udelay(1);
> }
>
> - ret = gdsc_poll_status(sc, status);
> + ret = gdsc_poll_status(sc, status, TIMEOUT_US, 0);
> WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
>
> if (!ret && status == GDSC_OFF && sc->rsupply) {
> @@ -343,7 +347,7 @@ static int _gdsc_disable(struct gdsc *sc)
>*/
> udelay(1);
>
> - ret = gdsc_poll_status(sc, GDSC_ON);
> + ret = gdsc_poll_status(sc, GDSC_ON, TIMEOUT_US, 0);
> if (ret)
> return ret;
> }
> @@ -565,3 +569,14 @@ int gdsc_gx_do_nothing_enable(struct generic_pm_domain
> *domain)
> return 0;
> }
> EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable);
> +
> +int gdsc_wait_for_collapse(void *priv)
> +{
> + struct gdsc *sc = priv;
> + int ret;
> +
> + ret = gdsc_poll_status(sc, GDSC_OFF, 50, 5);
> + WARN(ret, "%s status stuck at 'on'", sc->pd.name);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(gdsc_wait_for_collapse);
Superficially, using this as a reset op seems like abuse of the reset
controller API. Calling reset_control_reset() on this in the GPU driver
will not trigger a reset signal on the GPU's "cx_collapse" reset input.
So at the very least, this patchset should contain an explanation why
this is a good idea regardless, and how this is almost a reset control.
I have read the linked discussion, and I'm not sure I understand all
of it, so please correct me if I'm wrong: There is some other way to
force the GDSC into a state that will eventually cause a GPU reset, and
this is just the remaining part to make sure that the workaround dance
is finished?
If so, it should be explained that this depends on something else to
actually indirectly trigger the reset, and where this happens.
regards
Philipp
Re: [Freedreno] [PATCH v6 2/6] clk: qcom: Allow custom reset ops
Hi Akhil,
On Wed, Aug 31, 2022 at 10:48:23AM +0530, Akhil P Oommen wrote:
> Allow soc specific clk drivers to specify a custom reset operation. We
> will use this in an upcoming patch to allow gpucc driver to specify a
> differet reset operation for cx_gdsc.
>
> Signed-off-by: Akhil P Oommen
> Reviewed-by: Dmitry Baryshkov
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Use pointer to const for "struct qcom_reset_ops" in qcom_reset_map
> (Krzysztof)
>
> Changes in v2:
> - Return error when a particular custom reset op is not implemented. (Dmitry)
>
> drivers/clk/qcom/reset.c | 27 +++
> drivers/clk/qcom/reset.h | 8
> 2 files changed, 35 insertions(+)
>
> diff --git a/drivers/clk/qcom/reset.c b/drivers/clk/qcom/reset.c
> index 819d194..b7ae4a3 100644
> --- a/drivers/clk/qcom/reset.c
> +++ b/drivers/clk/qcom/reset.c
> @@ -13,6 +13,21 @@
>
> static int qcom_reset(struct reset_controller_dev *rcdev, unsigned long id)
> {
> + struct qcom_reset_controller *rst;
> + const struct qcom_reset_map *map;
> +
> + rst = to_qcom_reset_controller(rcdev);
> + map = >reset_map[id];
> +
> + if (map->ops && map->ops->reset)
> + return map->ops->reset(map->priv);
> + /*
> + * If custom ops is implemented but just not this callback, return
> + * error
> + */
> + else if (map->ops)
> + return -EOPNOTSUPP;
> +
It doesn't seem necessary to stack reset_ops -> qcom_reset_ops for what
you need here.
Just add an optional const struct reset_ops * to qcom_cc_desc and feed
that into qcom_cc_really_probe to replace _reset_ops.
[...]
> +struct qcom_reset_ops {
> + int (*reset)(void *priv);
> + int (*assert)(void *priv);
> + int (*deassert)(void *priv);
Why add assert and deassert ops? There doesn't seem to be any user.
> +};
> +
> struct qcom_reset_map {
> unsigned int reg;
> u8 bit;
> + const struct qcom_reset_ops *ops;
> + void *priv;
Adding per-reset ops + priv counters seems excessive to me.
Are you expecting different reset controls in the same reset controller
to have completely different ops at some point? If so, I would wonder
whether it wouldn't be better to split the reset controller in that
case. Either way, for the GDSC collapse workaround this does not seem
to be required at all.
regards
Philipp
Re: [Freedreno] [PATCH v4 5/7] drm/msm/a6xx: Ensure CX collapse during gpu recovery
Hi Akhil, On Wed, Aug 17, 2022 at 08:44:18PM +0530, Akhil P Oommen wrote: > Because there could be transient votes from other drivers/tz/hyp which > may keep the cx gdsc enabled, we should poll until cx gdsc collapses. > We can use the reset framework to poll for cx gdsc collapse from gpucc > clk driver. > > This feature requires support from the platform's gpucc driver. > > Signed-off-by: Akhil P Oommen > Reviewed-by: Dmitry Baryshkov > --- > > (no changes since v3) > > Changes in v3: > - Use reset interface from gpucc driver to poll for cx gdsc collapse > https://patchwork.freedesktop.org/series/106860/ > > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 > drivers/gpu/drm/msm/msm_gpu.c | 4 > drivers/gpu/drm/msm/msm_gpu.h | 4 > 3 files changed, 12 insertions(+) > [...] > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > index 07e55a6..4a57627 100644 > --- a/drivers/gpu/drm/msm/msm_gpu.c > +++ b/drivers/gpu/drm/msm/msm_gpu.c [...] > @@ -903,6 +904,9 @@ int msm_gpu_init(struct drm_device *drm, struct > platform_device *pdev, > if (IS_ERR(gpu->gpu_cx)) > gpu->gpu_cx = NULL; > > + gpu->cx_collapse = devm_reset_control_get_optional(>dev, > + "cx_collapse"); Please use devm_reset_control_get_optional_exclusive() instead. With that, Reviewed-by: Philipp Zabel regards Philipp
Re: [Freedreno] [PATCH 07/13] drm/mediatek: Provide ddc symlink in hdmi connector sysfs directory
On Wed, 2019-07-31 at 18:58 +0200, Andrzej Pietrasiewicz wrote:
> Use the ddc pointer provided by the generic connector.
>
> Signed-off-by: Andrzej Pietrasiewicz
> Acked-by: Sam Ravnborg
> Reviewed-by: Emil Velikov
Reviewed-by: Philipp Zabel
regards
Philipp
> ---
> drivers/gpu/drm/mediatek/mtk_hdmi.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> index ce91b61364eb..f419765b7cc0 100644
> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> @@ -1299,9 +1299,10 @@ static int mtk_hdmi_bridge_attach(struct drm_bridge
> *bridge)
> struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
> int ret;
>
> - ret = drm_connector_init(bridge->encoder->dev, >conn,
> - _hdmi_connector_funcs,
> - DRM_MODE_CONNECTOR_HDMIA);
> + ret = drm_connector_init_with_ddc(bridge->encoder->dev, >conn,
> + _hdmi_connector_funcs,
> + DRM_MODE_CONNECTOR_HDMIA,
> + hdmi->ddc_adpt);
> if (ret) {
> dev_err(hdmi->dev, "Failed to initialize connector: %d\n", ret);
> return ret;
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v6 10/24] drm/imx: imx-ldb: Provide ddc symlink in connector's sysfs
On Fri, 2019-07-26 at 19:23 +0200, Andrzej Pietrasiewicz wrote: > Use the ddc pointer provided by the generic connector. > > Signed-off-by: Andrzej Pietrasiewicz Acked-by: Philipp Zabel Thanks! regards Philipp > --- > drivers/gpu/drm/imx/imx-ldb.c | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c > index de62a4cd4827..db461b6a257f 100644 > --- a/drivers/gpu/drm/imx/imx-ldb.c > +++ b/drivers/gpu/drm/imx/imx-ldb.c > @@ -462,9 +462,10 @@ static int imx_ldb_register(struct drm_device *drm, >*/ > drm_connector_helper_add(_ldb_ch->connector, > _ldb_connector_helper_funcs); > - drm_connector_init(drm, _ldb_ch->connector, > - _ldb_connector_funcs, > - DRM_MODE_CONNECTOR_LVDS); > + drm_connector_init_with_ddc(drm, _ldb_ch->connector, > + _ldb_connector_funcs, > + DRM_MODE_CONNECTOR_LVDS, > + imx_ldb_ch->ddc); > drm_connector_attach_encoder(_ldb_ch->connector, encoder); > } > ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v6 11/24] drm/imx: imx-tve: Provide ddc symlink in connector's sysfs
On Fri, 2019-07-26 at 19:23 +0200, Andrzej Pietrasiewicz wrote: > Use the ddc pointer provided by the generic connector. > > Signed-off-by: Andrzej Pietrasiewicz Acked-by: Philipp Zabel regards Philipp > --- > drivers/gpu/drm/imx/imx-tve.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c > index 649515868f86..5bbfaa2cd0f4 100644 > --- a/drivers/gpu/drm/imx/imx-tve.c > +++ b/drivers/gpu/drm/imx/imx-tve.c > @@ -484,8 +484,10 @@ static int imx_tve_register(struct drm_device *drm, > struct imx_tve *tve) > > drm_connector_helper_add(>connector, > _tve_connector_helper_funcs); > - drm_connector_init(drm, >connector, _tve_connector_funcs, > -DRM_MODE_CONNECTOR_VGA); > + drm_connector_init_with_ddc(drm, >connector, > + _tve_connector_funcs, > + DRM_MODE_CONNECTOR_VGA, > + tve->ddc); > > drm_connector_attach_encoder(>connector, >encoder); > ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] drm/msm: for array in-fences, check if all backing fences are from our own context before waiting
Use the dma_fence_match_context helper to check if all backing fences
are from our own context, in which case we don't have to wait.
Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
Cc: Rob Clark <robdcl...@gmail.com>
Cc: Gustavo Padovan <gustavo.pado...@collabora.com>
---
Not sure if this can be handled exactly the same as for etnaviv. This depends on
d5b72a2123df ("dma-fence: add dma_fence_match_context helper").
---
drivers/gpu/drm/msm/msm_gem_submit.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 1172fe7a9252c..507a6f5b911f0 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -439,17 +439,15 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void
*data,
goto out;
}
- /* TODO if we get an array-fence due to userspace merging
multiple
-* fences, we need a way to determine if all the backing fences
-* are from our own context..
+ /*
+* Wait if the fence is from a foreign context, or if the fence
+* array contains any fence from a foreign context.
*/
-
- if (in_fence->context != gpu->fctx->context) {
+ if (!dma_fence_match_context(in_fence, gpu->fctx->context)) {
ret = dma_fence_wait(in_fence, true);
if (ret)
goto out;
}
-
}
if (!(args->fence & MSM_SUBMIT_NO_IMPLICIT)) {
--
2.11.0
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
