* [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
@ 2022-06-27 16:54 ` Dmitry Baryshkov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Baryshkov @ 2022-06-27 16:54 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno, Kuogee Hsieh
If for some reason the msm_dp_config::descs array starts from non-zero
index or contains the hole, setting the msm_dp_config::num_descs might
be not that obvious and error-prone. Use ARRAY_SIZE to set this field
rather than encoding the value manually.
Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/dp/dp_display.c | 46 +++++++++++++++++------------
1 file changed, 27 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index f87fa3ba1e25..6fed738a9467 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -131,35 +131,43 @@ struct msm_dp_config {
size_t num_descs;
};
+static const struct msm_dp_desc sc7180_dp_descs[] = {
+ [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+};
+
static const struct msm_dp_config sc7180_dp_cfg = {
- .descs = (const struct msm_dp_desc[]) {
- [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
- },
- .num_descs = 1,
+ .descs = sc7180_dp_descs,
+ .num_descs = ARRAY_SIZE(sc7180_dp_descs),
+};
+
+static const struct msm_dp_desc sc7280_dp_descs[] = {
+ [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
+ [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
};
static const struct msm_dp_config sc7280_dp_cfg = {
- .descs = (const struct msm_dp_desc[]) {
- [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
- [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
- },
- .num_descs = 2,
+ .descs = sc7280_dp_descs,
+ .num_descs = ARRAY_SIZE(sc7280_dp_descs),
+};
+
+static const struct msm_dp_desc sc8180x_dp_descs[] = {
+ [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+ [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+ [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
};
static const struct msm_dp_config sc8180x_dp_cfg = {
- .descs = (const struct msm_dp_desc[]) {
- [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
- [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
- [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
- },
- .num_descs = 3,
+ .descs = sc8180x_dp_descs,
+ .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
+};
+
+static const struct msm_dp_desc sm8350_dp_descs[] = {
+ [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
};
static const struct msm_dp_config sm8350_dp_cfg = {
- .descs = (const struct msm_dp_desc[]) {
- [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
- },
- .num_descs = 1,
+ .descs = sm8350_dp_descs,
+ .num_descs = ARRAY_SIZE(sm8350_dp_descs),
};
static const struct of_device_id dp_dt_match[] = {
--
2.35.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
@ 2022-06-27 16:54 ` Dmitry Baryshkov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Baryshkov @ 2022-06-27 16:54 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar
Cc: David Airlie, linux-arm-msm, Kuogee Hsieh, dri-devel,
Bjorn Andersson, Stephen Boyd, freedreno
If for some reason the msm_dp_config::descs array starts from non-zero
index or contains the hole, setting the msm_dp_config::num_descs might
be not that obvious and error-prone. Use ARRAY_SIZE to set this field
rather than encoding the value manually.
Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/msm/dp/dp_display.c | 46 +++++++++++++++++------------
1 file changed, 27 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index f87fa3ba1e25..6fed738a9467 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -131,35 +131,43 @@ struct msm_dp_config {
size_t num_descs;
};
+static const struct msm_dp_desc sc7180_dp_descs[] = {
+ [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+};
+
static const struct msm_dp_config sc7180_dp_cfg = {
- .descs = (const struct msm_dp_desc[]) {
- [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
- },
- .num_descs = 1,
+ .descs = sc7180_dp_descs,
+ .num_descs = ARRAY_SIZE(sc7180_dp_descs),
+};
+
+static const struct msm_dp_desc sc7280_dp_descs[] = {
+ [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
+ [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
};
static const struct msm_dp_config sc7280_dp_cfg = {
- .descs = (const struct msm_dp_desc[]) {
- [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
- [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
- },
- .num_descs = 2,
+ .descs = sc7280_dp_descs,
+ .num_descs = ARRAY_SIZE(sc7280_dp_descs),
+};
+
+static const struct msm_dp_desc sc8180x_dp_descs[] = {
+ [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+ [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+ [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
};
static const struct msm_dp_config sc8180x_dp_cfg = {
- .descs = (const struct msm_dp_desc[]) {
- [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
- [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
- [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
- },
- .num_descs = 3,
+ .descs = sc8180x_dp_descs,
+ .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
+};
+
+static const struct msm_dp_desc sm8350_dp_descs[] = {
+ [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
};
static const struct msm_dp_config sm8350_dp_cfg = {
- .descs = (const struct msm_dp_desc[]) {
- [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
- },
- .num_descs = 1,
+ .descs = sm8350_dp_descs,
+ .num_descs = ARRAY_SIZE(sm8350_dp_descs),
};
static const struct of_device_id dp_dt_match[] = {
--
2.35.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
2022-06-27 16:54 ` Dmitry Baryshkov
@ 2022-06-27 19:19 ` Stephen Boyd
-1 siblings, 0 replies; 14+ messages in thread
From: Stephen Boyd @ 2022-06-27 19:19 UTC (permalink / raw)
To: Abhinav Kumar, Dmitry Baryshkov, Rob Clark, Sean Paul
Cc: David Airlie, Daniel Vetter, Bjorn Andersson, linux-arm-msm,
dri-devel, freedreno, Kuogee Hsieh
Quoting Dmitry Baryshkov (2022-06-27 09:54:13)
> If for some reason the msm_dp_config::descs array starts from non-zero
> index or contains the hole, setting the msm_dp_config::num_descs might
> be not that obvious and error-prone. Use ARRAY_SIZE to set this field
> rather than encoding the value manually.
>
> Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
@ 2022-06-27 19:19 ` Stephen Boyd
0 siblings, 0 replies; 14+ messages in thread
From: Stephen Boyd @ 2022-06-27 19:19 UTC (permalink / raw)
To: Abhinav Kumar, Dmitry Baryshkov, Rob Clark, Sean Paul
Cc: David Airlie, linux-arm-msm, dri-devel, Bjorn Andersson,
Kuogee Hsieh, freedreno
Quoting Dmitry Baryshkov (2022-06-27 09:54:13)
> If for some reason the msm_dp_config::descs array starts from non-zero
> index or contains the hole, setting the msm_dp_config::num_descs might
> be not that obvious and error-prone. Use ARRAY_SIZE to set this field
> rather than encoding the value manually.
>
> Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
2022-06-27 16:54 ` Dmitry Baryshkov
@ 2022-06-27 19:26 ` Kuogee Hsieh
-1 siblings, 0 replies; 14+ messages in thread
From: Kuogee Hsieh @ 2022-06-27 19:26 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Abhinav Kumar
Cc: Stephen Boyd, David Airlie, Daniel Vetter, Bjorn Andersson,
linux-arm-msm, dri-devel, freedreno
On 6/27/2022 9:54 AM, Dmitry Baryshkov wrote:
> If for some reason the msm_dp_config::descs array starts from non-zero
> index or contains the hole, setting the msm_dp_config::num_descs might
> be not that obvious and error-prone. Use ARRAY_SIZE to set this field
> rather than encoding the value manually.
>
> Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 46 +++++++++++++++++------------
> 1 file changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index f87fa3ba1e25..6fed738a9467 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -131,35 +131,43 @@ struct msm_dp_config {
> size_t num_descs;
> };
>
> +static const struct msm_dp_desc sc7180_dp_descs[] = {
> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> +};
> +
> static const struct msm_dp_config sc7180_dp_cfg = {
> - .descs = (const struct msm_dp_desc[]) {
> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> - },
> - .num_descs = 1,
> + .descs = sc7180_dp_descs,
> + .num_descs = ARRAY_SIZE(sc7180_dp_descs),
> +};
> +
why you want to do that?
It is very clear only one entry, why you want to make it 2 entry here?
can you just embedded MSM_DP_COTROLLER_x into struct msm_dp_config?
> +static const struct msm_dp_desc sc7280_dp_descs[] = {
> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> };
>
> static const struct msm_dp_config sc7280_dp_cfg = {
> - .descs = (const struct msm_dp_desc[]) {
> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> - },
> - .num_descs = 2,
> + .descs = sc7280_dp_descs,
> + .num_descs = ARRAY_SIZE(sc7280_dp_descs),
> +};
> +
> +static const struct msm_dp_desc sc8180x_dp_descs[] = {
> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> + [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
> };
>
> static const struct msm_dp_config sc8180x_dp_cfg = {
> - .descs = (const struct msm_dp_desc[]) {
> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> - [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
> - },
> - .num_descs = 3,
> + .descs = sc8180x_dp_descs,
> + .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
> +};
> +
> +static const struct msm_dp_desc sm8350_dp_descs[] = {
> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> };
>
> static const struct msm_dp_config sm8350_dp_cfg = {
> - .descs = (const struct msm_dp_desc[]) {
> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> - },
> - .num_descs = 1,
> + .descs = sm8350_dp_descs,
> + .num_descs = ARRAY_SIZE(sm8350_dp_descs),
> };
>
> static const struct of_device_id dp_dt_match[] = {
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
@ 2022-06-27 19:26 ` Kuogee Hsieh
0 siblings, 0 replies; 14+ messages in thread
From: Kuogee Hsieh @ 2022-06-27 19:26 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul, Abhinav Kumar
Cc: David Airlie, linux-arm-msm, dri-devel, Bjorn Andersson,
Stephen Boyd, freedreno
On 6/27/2022 9:54 AM, Dmitry Baryshkov wrote:
> If for some reason the msm_dp_config::descs array starts from non-zero
> index or contains the hole, setting the msm_dp_config::num_descs might
> be not that obvious and error-prone. Use ARRAY_SIZE to set this field
> rather than encoding the value manually.
>
> Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 46 +++++++++++++++++------------
> 1 file changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index f87fa3ba1e25..6fed738a9467 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -131,35 +131,43 @@ struct msm_dp_config {
> size_t num_descs;
> };
>
> +static const struct msm_dp_desc sc7180_dp_descs[] = {
> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> +};
> +
> static const struct msm_dp_config sc7180_dp_cfg = {
> - .descs = (const struct msm_dp_desc[]) {
> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> - },
> - .num_descs = 1,
> + .descs = sc7180_dp_descs,
> + .num_descs = ARRAY_SIZE(sc7180_dp_descs),
> +};
> +
why you want to do that?
It is very clear only one entry, why you want to make it 2 entry here?
can you just embedded MSM_DP_COTROLLER_x into struct msm_dp_config?
> +static const struct msm_dp_desc sc7280_dp_descs[] = {
> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> };
>
> static const struct msm_dp_config sc7280_dp_cfg = {
> - .descs = (const struct msm_dp_desc[]) {
> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> - },
> - .num_descs = 2,
> + .descs = sc7280_dp_descs,
> + .num_descs = ARRAY_SIZE(sc7280_dp_descs),
> +};
> +
> +static const struct msm_dp_desc sc8180x_dp_descs[] = {
> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> + [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
> };
>
> static const struct msm_dp_config sc8180x_dp_cfg = {
> - .descs = (const struct msm_dp_desc[]) {
> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> - [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
> - },
> - .num_descs = 3,
> + .descs = sc8180x_dp_descs,
> + .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
> +};
> +
> +static const struct msm_dp_desc sm8350_dp_descs[] = {
> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> };
>
> static const struct msm_dp_config sm8350_dp_cfg = {
> - .descs = (const struct msm_dp_desc[]) {
> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> - },
> - .num_descs = 1,
> + .descs = sm8350_dp_descs,
> + .num_descs = ARRAY_SIZE(sm8350_dp_descs),
> };
>
> static const struct of_device_id dp_dt_match[] = {
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
2022-06-27 19:26 ` Kuogee Hsieh
@ 2022-06-27 20:05 ` Dmitry Baryshkov
-1 siblings, 0 replies; 14+ messages in thread
From: Dmitry Baryshkov @ 2022-06-27 20:05 UTC (permalink / raw)
To: Kuogee Hsieh
Cc: Rob Clark, Sean Paul, Abhinav Kumar, Stephen Boyd, David Airlie,
Daniel Vetter, Bjorn Andersson, linux-arm-msm, dri-devel,
freedreno
On Mon, 27 Jun 2022 at 22:26, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 6/27/2022 9:54 AM, Dmitry Baryshkov wrote:
> > If for some reason the msm_dp_config::descs array starts from non-zero
> > index or contains the hole, setting the msm_dp_config::num_descs might
> > be not that obvious and error-prone. Use ARRAY_SIZE to set this field
> > rather than encoding the value manually.
> >
> > Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > drivers/gpu/drm/msm/dp/dp_display.c | 46 +++++++++++++++++------------
> > 1 file changed, 27 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index f87fa3ba1e25..6fed738a9467 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -131,35 +131,43 @@ struct msm_dp_config {
> > size_t num_descs;
> > };
> >
> > +static const struct msm_dp_desc sc7180_dp_descs[] = {
> > + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > +};
> > +
> > static const struct msm_dp_config sc7180_dp_cfg = {
> > - .descs = (const struct msm_dp_desc[]) {
> > - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > - },
> > - .num_descs = 1,
> > + .descs = sc7180_dp_descs,
> > + .num_descs = ARRAY_SIZE(sc7180_dp_descs),
> > +};
> > +
>
> why you want to do that?
>
> It is very clear only one entry, why you want to make it 2 entry here?
>
> can you just embedded MSM_DP_COTROLLER_x into struct msm_dp_config?
Because we had enough stories of using a manually set 'number of
something' field. So I'd prefer to have it done automatically.
Also using the indexed array spares us from 'look for the DP
controller number N' functions. You can just get it.
>
> > +static const struct msm_dp_desc sc7280_dp_descs[] = {
> > + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> > + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> > };
> >
> > static const struct msm_dp_config sc7280_dp_cfg = {
> > - .descs = (const struct msm_dp_desc[]) {
> > - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> > - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> > - },
> > - .num_descs = 2,
> > + .descs = sc7280_dp_descs,
> > + .num_descs = ARRAY_SIZE(sc7280_dp_descs),
> > +};
> > +
> > +static const struct msm_dp_desc sc8180x_dp_descs[] = {
> > + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > + [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
> > };
> >
> > static const struct msm_dp_config sc8180x_dp_cfg = {
> > - .descs = (const struct msm_dp_desc[]) {
> > - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > - [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
> > - },
> > - .num_descs = 3,
> > + .descs = sc8180x_dp_descs,
> > + .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
> > +};
> > +
> > +static const struct msm_dp_desc sm8350_dp_descs[] = {
> > + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > };
> >
> > static const struct msm_dp_config sm8350_dp_cfg = {
> > - .descs = (const struct msm_dp_desc[]) {
> > - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > - },
> > - .num_descs = 1,
> > + .descs = sm8350_dp_descs,
> > + .num_descs = ARRAY_SIZE(sm8350_dp_descs),
> > };
> >
> > static const struct of_device_id dp_dt_match[] = {
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
@ 2022-06-27 20:05 ` Dmitry Baryshkov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Baryshkov @ 2022-06-27 20:05 UTC (permalink / raw)
To: Kuogee Hsieh
Cc: freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Stephen Boyd, Bjorn Andersson, Sean Paul
On Mon, 27 Jun 2022 at 22:26, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
>
> On 6/27/2022 9:54 AM, Dmitry Baryshkov wrote:
> > If for some reason the msm_dp_config::descs array starts from non-zero
> > index or contains the hole, setting the msm_dp_config::num_descs might
> > be not that obvious and error-prone. Use ARRAY_SIZE to set this field
> > rather than encoding the value manually.
> >
> > Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > drivers/gpu/drm/msm/dp/dp_display.c | 46 +++++++++++++++++------------
> > 1 file changed, 27 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index f87fa3ba1e25..6fed738a9467 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -131,35 +131,43 @@ struct msm_dp_config {
> > size_t num_descs;
> > };
> >
> > +static const struct msm_dp_desc sc7180_dp_descs[] = {
> > + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > +};
> > +
> > static const struct msm_dp_config sc7180_dp_cfg = {
> > - .descs = (const struct msm_dp_desc[]) {
> > - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > - },
> > - .num_descs = 1,
> > + .descs = sc7180_dp_descs,
> > + .num_descs = ARRAY_SIZE(sc7180_dp_descs),
> > +};
> > +
>
> why you want to do that?
>
> It is very clear only one entry, why you want to make it 2 entry here?
>
> can you just embedded MSM_DP_COTROLLER_x into struct msm_dp_config?
Because we had enough stories of using a manually set 'number of
something' field. So I'd prefer to have it done automatically.
Also using the indexed array spares us from 'look for the DP
controller number N' functions. You can just get it.
>
> > +static const struct msm_dp_desc sc7280_dp_descs[] = {
> > + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> > + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> > };
> >
> > static const struct msm_dp_config sc7280_dp_cfg = {
> > - .descs = (const struct msm_dp_desc[]) {
> > - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
> > - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> > - },
> > - .num_descs = 2,
> > + .descs = sc7280_dp_descs,
> > + .num_descs = ARRAY_SIZE(sc7280_dp_descs),
> > +};
> > +
> > +static const struct msm_dp_desc sc8180x_dp_descs[] = {
> > + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > + [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
> > };
> >
> > static const struct msm_dp_config sc8180x_dp_cfg = {
> > - .descs = (const struct msm_dp_desc[]) {
> > - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > - [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
> > - },
> > - .num_descs = 3,
> > + .descs = sc8180x_dp_descs,
> > + .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
> > +};
> > +
> > +static const struct msm_dp_desc sm8350_dp_descs[] = {
> > + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > };
> >
> > static const struct msm_dp_config sm8350_dp_cfg = {
> > - .descs = (const struct msm_dp_desc[]) {
> > - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> > - },
> > - .num_descs = 1,
> > + .descs = sm8350_dp_descs,
> > + .num_descs = ARRAY_SIZE(sm8350_dp_descs),
> > };
> >
> > static const struct of_device_id dp_dt_match[] = {
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
2022-06-27 20:05 ` Dmitry Baryshkov
@ 2022-06-27 21:01 ` Kuogee Hsieh
-1 siblings, 0 replies; 14+ messages in thread
From: Kuogee Hsieh @ 2022-06-27 21:01 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Rob Clark, Sean Paul, Abhinav Kumar, Stephen Boyd, David Airlie,
Daniel Vetter, Bjorn Andersson, linux-arm-msm, dri-devel,
freedreno
On 6/27/2022 1:05 PM, Dmitry Baryshkov wrote:
> On Mon, 27 Jun 2022 at 22:26, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>
>> On 6/27/2022 9:54 AM, Dmitry Baryshkov wrote:
>>> If for some reason the msm_dp_config::descs array starts from non-zero
>>> index or contains the hole, setting the msm_dp_config::num_descs might
>>> be not that obvious and error-prone. Use ARRAY_SIZE to set this field
>>> rather than encoding the value manually.
>>>
>>> Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>> drivers/gpu/drm/msm/dp/dp_display.c | 46 +++++++++++++++++------------
>>> 1 file changed, 27 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index f87fa3ba1e25..6fed738a9467 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -131,35 +131,43 @@ struct msm_dp_config {
>>> size_t num_descs;
>>> };
>>>
>>> +static const struct msm_dp_desc sc7180_dp_descs[] = {
>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> +};
>>> +
>>> static const struct msm_dp_config sc7180_dp_cfg = {
>>> - .descs = (const struct msm_dp_desc[]) {
>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> - },
>>> - .num_descs = 1,
>>> + .descs = sc7180_dp_descs,
>>> + .num_descs = ARRAY_SIZE(sc7180_dp_descs),
>>> +};
>>> +
>> why you want to do that?
>>
>> It is very clear only one entry, why you want to make it 2 entry here?
>>
>> can you just embedded MSM_DP_COTROLLER_x into struct msm_dp_config?
> Because we had enough stories of using a manually set 'number of
> something' field. So I'd prefer to have it done automatically.
> Also using the indexed array spares us from 'look for the DP
> controller number N' functions. You can just get it.
static const struct msm_dp_config sc7280_dp_cfg = {
.descs = (const struct msm_dp_desc[]) {
[MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
},
.num_descs = ARRAY_SIZE(sc7280_dp_descs),
};
At above example table, it just waste one entry. is it ok?
can you elaborate more on 'look for the DP controller number N'
functions, where is it?
>>> +static const struct msm_dp_desc sc7280_dp_descs[] = {
>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>> };
>>>
>>> static const struct msm_dp_config sc7280_dp_cfg = {
>>> - .descs = (const struct msm_dp_desc[]) {
>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>> - },
>>> - .num_descs = 2,
>>> + .descs = sc7280_dp_descs,
>>> + .num_descs = ARRAY_SIZE(sc7280_dp_descs),
>>> +};
>>> +
>>> +static const struct msm_dp_desc sc8180x_dp_descs[] = {
>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> + [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
>>> };
>>>
>>> static const struct msm_dp_config sc8180x_dp_cfg = {
>>> - .descs = (const struct msm_dp_desc[]) {
>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> - [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
>>> - },
>>> - .num_descs = 3,
>>> + .descs = sc8180x_dp_descs,
>>> + .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
>>> +};
>>> +
>>> +static const struct msm_dp_desc sm8350_dp_descs[] = {
>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> };
>>>
>>> static const struct msm_dp_config sm8350_dp_cfg = {
>>> - .descs = (const struct msm_dp_desc[]) {
>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> - },
>>> - .num_descs = 1,
>>> + .descs = sm8350_dp_descs,
>>> + .num_descs = ARRAY_SIZE(sm8350_dp_descs),
>>> };
>>>
>>> static const struct of_device_id dp_dt_match[] = {
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
@ 2022-06-27 21:01 ` Kuogee Hsieh
0 siblings, 0 replies; 14+ messages in thread
From: Kuogee Hsieh @ 2022-06-27 21:01 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Stephen Boyd, Bjorn Andersson, Sean Paul
On 6/27/2022 1:05 PM, Dmitry Baryshkov wrote:
> On Mon, 27 Jun 2022 at 22:26, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>>
>> On 6/27/2022 9:54 AM, Dmitry Baryshkov wrote:
>>> If for some reason the msm_dp_config::descs array starts from non-zero
>>> index or contains the hole, setting the msm_dp_config::num_descs might
>>> be not that obvious and error-prone. Use ARRAY_SIZE to set this field
>>> rather than encoding the value manually.
>>>
>>> Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>> drivers/gpu/drm/msm/dp/dp_display.c | 46 +++++++++++++++++------------
>>> 1 file changed, 27 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index f87fa3ba1e25..6fed738a9467 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -131,35 +131,43 @@ struct msm_dp_config {
>>> size_t num_descs;
>>> };
>>>
>>> +static const struct msm_dp_desc sc7180_dp_descs[] = {
>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> +};
>>> +
>>> static const struct msm_dp_config sc7180_dp_cfg = {
>>> - .descs = (const struct msm_dp_desc[]) {
>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> - },
>>> - .num_descs = 1,
>>> + .descs = sc7180_dp_descs,
>>> + .num_descs = ARRAY_SIZE(sc7180_dp_descs),
>>> +};
>>> +
>> why you want to do that?
>>
>> It is very clear only one entry, why you want to make it 2 entry here?
>>
>> can you just embedded MSM_DP_COTROLLER_x into struct msm_dp_config?
> Because we had enough stories of using a manually set 'number of
> something' field. So I'd prefer to have it done automatically.
> Also using the indexed array spares us from 'look for the DP
> controller number N' functions. You can just get it.
static const struct msm_dp_config sc7280_dp_cfg = {
.descs = (const struct msm_dp_desc[]) {
[MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
},
.num_descs = ARRAY_SIZE(sc7280_dp_descs),
};
At above example table, it just waste one entry. is it ok?
can you elaborate more on 'look for the DP controller number N'
functions, where is it?
>>> +static const struct msm_dp_desc sc7280_dp_descs[] = {
>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>> };
>>>
>>> static const struct msm_dp_config sc7280_dp_cfg = {
>>> - .descs = (const struct msm_dp_desc[]) {
>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en = true },
>>> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>> - },
>>> - .num_descs = 2,
>>> + .descs = sc7280_dp_descs,
>>> + .num_descs = ARRAY_SIZE(sc7280_dp_descs),
>>> +};
>>> +
>>> +static const struct msm_dp_desc sc8180x_dp_descs[] = {
>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> + [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
>>> };
>>>
>>> static const struct msm_dp_config sc8180x_dp_cfg = {
>>> - .descs = (const struct msm_dp_desc[]) {
>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> - [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
>>> - },
>>> - .num_descs = 3,
>>> + .descs = sc8180x_dp_descs,
>>> + .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
>>> +};
>>> +
>>> +static const struct msm_dp_desc sm8350_dp_descs[] = {
>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> };
>>>
>>> static const struct msm_dp_config sm8350_dp_cfg = {
>>> - .descs = (const struct msm_dp_desc[]) {
>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>> - },
>>> - .num_descs = 1,
>>> + .descs = sm8350_dp_descs,
>>> + .num_descs = ARRAY_SIZE(sm8350_dp_descs),
>>> };
>>>
>>> static const struct of_device_id dp_dt_match[] = {
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
2022-06-27 21:01 ` Kuogee Hsieh
@ 2022-06-27 21:23 ` Dmitry Baryshkov
-1 siblings, 0 replies; 14+ messages in thread
From: Dmitry Baryshkov @ 2022-06-27 21:23 UTC (permalink / raw)
To: Kuogee Hsieh
Cc: Rob Clark, Sean Paul, Abhinav Kumar, Stephen Boyd, David Airlie,
Daniel Vetter, Bjorn Andersson, linux-arm-msm, dri-devel,
freedreno
On 28/06/2022 00:01, Kuogee Hsieh wrote:
>
> On 6/27/2022 1:05 PM, Dmitry Baryshkov wrote:
>> On Mon, 27 Jun 2022 at 22:26, Kuogee Hsieh <quic_khsieh@quicinc.com>
>> wrote:
>>>
>>> On 6/27/2022 9:54 AM, Dmitry Baryshkov wrote:
>>>> If for some reason the msm_dp_config::descs array starts from non-zero
>>>> index or contains the hole, setting the msm_dp_config::num_descs might
>>>> be not that obvious and error-prone. Use ARRAY_SIZE to set this field
>>>> rather than encoding the value manually.
>>>>
>>>> Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>> drivers/gpu/drm/msm/dp/dp_display.c | 46
>>>> +++++++++++++++++------------
>>>> 1 file changed, 27 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index f87fa3ba1e25..6fed738a9467 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -131,35 +131,43 @@ struct msm_dp_config {
>>>> size_t num_descs;
>>>> };
>>>>
>>>> +static const struct msm_dp_desc sc7180_dp_descs[] = {
>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> +};
>>>> +
>>>> static const struct msm_dp_config sc7180_dp_cfg = {
>>>> - .descs = (const struct msm_dp_desc[]) {
>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> - },
>>>> - .num_descs = 1,
>>>> + .descs = sc7180_dp_descs,
>>>> + .num_descs = ARRAY_SIZE(sc7180_dp_descs),
>>>> +};
>>>> +
>>> why you want to do that?
>>>
>>> It is very clear only one entry, why you want to make it 2 entry here?
>>>
>>> can you just embedded MSM_DP_COTROLLER_x into struct msm_dp_config?
>> Because we had enough stories of using a manually set 'number of
>> something' field. So I'd prefer to have it done automatically.
>> Also using the indexed array spares us from 'look for the DP
>> controller number N' functions. You can just get it.
>
> static const struct msm_dp_config sc7280_dp_cfg = {
> .descs = (const struct msm_dp_desc[]) {
> [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> },
> .num_descs = ARRAY_SIZE(sc7280_dp_descs),
This will not work.
> };
>
> At above example table, it just waste one entry. is it ok?
I'd assume this is an interim development state. Then it's fine. In the
final version one would fill all existing DP controllers starting from 0.
>
> can you elaborate more on 'look for the DP controller number N'
> functions, where is it?
Ignore this.
>
>
>>>> +static const struct msm_dp_desc sc7280_dp_descs[] = {
>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en =
>>>> true },
>>>> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>> };
>>>>
>>>> static const struct msm_dp_config sc7280_dp_cfg = {
>>>> - .descs = (const struct msm_dp_desc[]) {
>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en =
>>>> true },
>>>> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>> - },
>>>> - .num_descs = 2,
>>>> + .descs = sc7280_dp_descs,
>>>> + .num_descs = ARRAY_SIZE(sc7280_dp_descs),
>>>> +};
>>>> +
>>>> +static const struct msm_dp_desc sc8180x_dp_descs[] = {
>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> + [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000,
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP },
>>>> };
>>>>
>>>> static const struct msm_dp_config sc8180x_dp_cfg = {
>>>> - .descs = (const struct msm_dp_desc[]) {
>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> - [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000,
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP },
>>>> - },
>>>> - .num_descs = 3,
>>>> + .descs = sc8180x_dp_descs,
>>>> + .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
>>>> +};
>>>> +
>>>> +static const struct msm_dp_desc sm8350_dp_descs[] = {
>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> };
>>>>
>>>> static const struct msm_dp_config sm8350_dp_cfg = {
>>>> - .descs = (const struct msm_dp_desc[]) {
>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> - },
>>>> - .num_descs = 1,
>>>> + .descs = sm8350_dp_descs,
>>>> + .num_descs = ARRAY_SIZE(sm8350_dp_descs),
>>>> };
>>>>
>>>> static const struct of_device_id dp_dt_match[] = {
>>
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
@ 2022-06-27 21:23 ` Dmitry Baryshkov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Baryshkov @ 2022-06-27 21:23 UTC (permalink / raw)
To: Kuogee Hsieh
Cc: freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Stephen Boyd, Bjorn Andersson, Sean Paul
On 28/06/2022 00:01, Kuogee Hsieh wrote:
>
> On 6/27/2022 1:05 PM, Dmitry Baryshkov wrote:
>> On Mon, 27 Jun 2022 at 22:26, Kuogee Hsieh <quic_khsieh@quicinc.com>
>> wrote:
>>>
>>> On 6/27/2022 9:54 AM, Dmitry Baryshkov wrote:
>>>> If for some reason the msm_dp_config::descs array starts from non-zero
>>>> index or contains the hole, setting the msm_dp_config::num_descs might
>>>> be not that obvious and error-prone. Use ARRAY_SIZE to set this field
>>>> rather than encoding the value manually.
>>>>
>>>> Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>> drivers/gpu/drm/msm/dp/dp_display.c | 46
>>>> +++++++++++++++++------------
>>>> 1 file changed, 27 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index f87fa3ba1e25..6fed738a9467 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -131,35 +131,43 @@ struct msm_dp_config {
>>>> size_t num_descs;
>>>> };
>>>>
>>>> +static const struct msm_dp_desc sc7180_dp_descs[] = {
>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> +};
>>>> +
>>>> static const struct msm_dp_config sc7180_dp_cfg = {
>>>> - .descs = (const struct msm_dp_desc[]) {
>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> - },
>>>> - .num_descs = 1,
>>>> + .descs = sc7180_dp_descs,
>>>> + .num_descs = ARRAY_SIZE(sc7180_dp_descs),
>>>> +};
>>>> +
>>> why you want to do that?
>>>
>>> It is very clear only one entry, why you want to make it 2 entry here?
>>>
>>> can you just embedded MSM_DP_COTROLLER_x into struct msm_dp_config?
>> Because we had enough stories of using a manually set 'number of
>> something' field. So I'd prefer to have it done automatically.
>> Also using the indexed array spares us from 'look for the DP
>> controller number N' functions. You can just get it.
>
> static const struct msm_dp_config sc7280_dp_cfg = {
> .descs = (const struct msm_dp_desc[]) {
> [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
> },
> .num_descs = ARRAY_SIZE(sc7280_dp_descs),
This will not work.
> };
>
> At above example table, it just waste one entry. is it ok?
I'd assume this is an interim development state. Then it's fine. In the
final version one would fill all existing DP controllers starting from 0.
>
> can you elaborate more on 'look for the DP controller number N'
> functions, where is it?
Ignore this.
>
>
>>>> +static const struct msm_dp_desc sc7280_dp_descs[] = {
>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en =
>>>> true },
>>>> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>> };
>>>>
>>>> static const struct msm_dp_config sc7280_dp_cfg = {
>>>> - .descs = (const struct msm_dp_desc[]) {
>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en =
>>>> true },
>>>> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>> - },
>>>> - .num_descs = 2,
>>>> + .descs = sc7280_dp_descs,
>>>> + .num_descs = ARRAY_SIZE(sc7280_dp_descs),
>>>> +};
>>>> +
>>>> +static const struct msm_dp_desc sc8180x_dp_descs[] = {
>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> + [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000,
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP },
>>>> };
>>>>
>>>> static const struct msm_dp_config sc8180x_dp_cfg = {
>>>> - .descs = (const struct msm_dp_desc[]) {
>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> - [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000,
>>>> .connector_type = DRM_MODE_CONNECTOR_eDP },
>>>> - },
>>>> - .num_descs = 3,
>>>> + .descs = sc8180x_dp_descs,
>>>> + .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
>>>> +};
>>>> +
>>>> +static const struct msm_dp_desc sm8350_dp_descs[] = {
>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> };
>>>>
>>>> static const struct msm_dp_config sm8350_dp_cfg = {
>>>> - .descs = (const struct msm_dp_desc[]) {
>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>> - },
>>>> - .num_descs = 1,
>>>> + .descs = sm8350_dp_descs,
>>>> + .num_descs = ARRAY_SIZE(sm8350_dp_descs),
>>>> };
>>>>
>>>> static const struct of_device_id dp_dt_match[] = {
>>
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
2022-06-27 21:23 ` Dmitry Baryshkov
@ 2022-06-28 17:16 ` Kuogee Hsieh
-1 siblings, 0 replies; 14+ messages in thread
From: Kuogee Hsieh @ 2022-06-28 17:16 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Rob Clark, Sean Paul, Abhinav Kumar, Stephen Boyd, David Airlie,
Daniel Vetter, Bjorn Andersson, linux-arm-msm, dri-devel,
freedreno
On 6/27/2022 2:23 PM, Dmitry Baryshkov wrote:
> On 28/06/2022 00:01, Kuogee Hsieh wrote:
>>
>> On 6/27/2022 1:05 PM, Dmitry Baryshkov wrote:
>>> On Mon, 27 Jun 2022 at 22:26, Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> wrote:
>>>>
>>>> On 6/27/2022 9:54 AM, Dmitry Baryshkov wrote:
>>>>> If for some reason the msm_dp_config::descs array starts from
>>>>> non-zero
>>>>> index or contains the hole, setting the msm_dp_config::num_descs
>>>>> might
>>>>> be not that obvious and error-prone. Use ARRAY_SIZE to set this field
>>>>> rather than encoding the value manually.
>>>>>
>>>>> Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>> ---
>>>>> drivers/gpu/drm/msm/dp/dp_display.c | 46
>>>>> +++++++++++++++++------------
>>>>> 1 file changed, 27 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> index f87fa3ba1e25..6fed738a9467 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> @@ -131,35 +131,43 @@ struct msm_dp_config {
>>>>> size_t num_descs;
>>>>> };
>>>>>
>>>>> +static const struct msm_dp_desc sc7180_dp_descs[] = {
>>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> +};
>>>>> +
>>>>> static const struct msm_dp_config sc7180_dp_cfg = {
>>>>> - .descs = (const struct msm_dp_desc[]) {
>>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> - },
>>>>> - .num_descs = 1,
>>>>> + .descs = sc7180_dp_descs,
>>>>> + .num_descs = ARRAY_SIZE(sc7180_dp_descs),
>>>>> +};
>>>>> +
>>>> why you want to do that?
>>>>
>>>> It is very clear only one entry, why you want to make it 2 entry here?
>>>>
>>>> can you just embedded MSM_DP_COTROLLER_x into struct msm_dp_config?
>>> Because we had enough stories of using a manually set 'number of
>>> something' field. So I'd prefer to have it done automatically.
>>> Also using the indexed array spares us from 'look for the DP
>>> controller number N' functions. You can just get it.
>>
>> static const struct msm_dp_config sc7280_dp_cfg = {
>> .descs = (const struct msm_dp_desc[]) {
>> [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>> },
>> .num_descs = ARRAY_SIZE(sc7280_dp_descs),
>
> This will not work.
>
>> };
>>
>> At above example table, it just waste one entry. is it ok?
>
> I'd assume this is an interim development state. Then it's fine. In
> the final version one would fill all existing DP controllers starting
> from 0.
>
>>
>> can you elaborate more on 'look for the DP controller number N'
>> functions, where is it?
>
> Ignore this.
>
>>
>>
>>>>> +static const struct msm_dp_desc sc7280_dp_descs[] = {
>>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en =
>>>>> true },
>>>>> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>>> };
>>>>>
>>>>> static const struct msm_dp_config sc7280_dp_cfg = {
>>>>> - .descs = (const struct msm_dp_desc[]) {
>>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en =
>>>>> true },
>>>>> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>>> - },
>>>>> - .num_descs = 2,
>>>>> + .descs = sc7280_dp_descs,
>>>>> + .num_descs = ARRAY_SIZE(sc7280_dp_descs),
>>>>> +};
>>>>> +
>>>>> +static const struct msm_dp_desc sc8180x_dp_descs[] = {
>>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> + [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_eDP },
>>>>> };
>>>>>
>>>>> static const struct msm_dp_config sc8180x_dp_cfg = {
>>>>> - .descs = (const struct msm_dp_desc[]) {
>>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> - [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_eDP },
>>>>> - },
>>>>> - .num_descs = 3,
>>>>> + .descs = sc8180x_dp_descs,
>>>>> + .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
>>>>> +};
>>>>> +
>>>>> +static const struct msm_dp_desc sm8350_dp_descs[] = {
>>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> };
>>>>>
>>>>> static const struct msm_dp_config sm8350_dp_cfg = {
>>>>> - .descs = (const struct msm_dp_desc[]) {
>>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> - },
>>>>> - .num_descs = 1,
>>>>> + .descs = sm8350_dp_descs,
>>>>> + .num_descs = ARRAY_SIZE(sm8350_dp_descs),
>>>>> };
>>>>>
>>>>> static const struct of_device_id dp_dt_match[] = {
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs
@ 2022-06-28 17:16 ` Kuogee Hsieh
0 siblings, 0 replies; 14+ messages in thread
From: Kuogee Hsieh @ 2022-06-28 17:16 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Stephen Boyd, Bjorn Andersson, Sean Paul
On 6/27/2022 2:23 PM, Dmitry Baryshkov wrote:
> On 28/06/2022 00:01, Kuogee Hsieh wrote:
>>
>> On 6/27/2022 1:05 PM, Dmitry Baryshkov wrote:
>>> On Mon, 27 Jun 2022 at 22:26, Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> wrote:
>>>>
>>>> On 6/27/2022 9:54 AM, Dmitry Baryshkov wrote:
>>>>> If for some reason the msm_dp_config::descs array starts from
>>>>> non-zero
>>>>> index or contains the hole, setting the msm_dp_config::num_descs
>>>>> might
>>>>> be not that obvious and error-prone. Use ARRAY_SIZE to set this field
>>>>> rather than encoding the value manually.
>>>>>
>>>>> Reported-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>>> ---
>>>>> drivers/gpu/drm/msm/dp/dp_display.c | 46
>>>>> +++++++++++++++++------------
>>>>> 1 file changed, 27 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> index f87fa3ba1e25..6fed738a9467 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> @@ -131,35 +131,43 @@ struct msm_dp_config {
>>>>> size_t num_descs;
>>>>> };
>>>>>
>>>>> +static const struct msm_dp_desc sc7180_dp_descs[] = {
>>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> +};
>>>>> +
>>>>> static const struct msm_dp_config sc7180_dp_cfg = {
>>>>> - .descs = (const struct msm_dp_desc[]) {
>>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> - },
>>>>> - .num_descs = 1,
>>>>> + .descs = sc7180_dp_descs,
>>>>> + .num_descs = ARRAY_SIZE(sc7180_dp_descs),
>>>>> +};
>>>>> +
>>>> why you want to do that?
>>>>
>>>> It is very clear only one entry, why you want to make it 2 entry here?
>>>>
>>>> can you just embedded MSM_DP_COTROLLER_x into struct msm_dp_config?
>>> Because we had enough stories of using a manually set 'number of
>>> something' field. So I'd prefer to have it done automatically.
>>> Also using the indexed array spares us from 'look for the DP
>>> controller number N' functions. You can just get it.
>>
>> static const struct msm_dp_config sc7280_dp_cfg = {
>> .descs = (const struct msm_dp_desc[]) {
>> [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>> },
>> .num_descs = ARRAY_SIZE(sc7280_dp_descs),
>
> This will not work.
>
>> };
>>
>> At above example table, it just waste one entry. is it ok?
>
> I'd assume this is an interim development state. Then it's fine. In
> the final version one would fill all existing DP controllers starting
> from 0.
>
>>
>> can you elaborate more on 'look for the DP controller number N'
>> functions, where is it?
>
> Ignore this.
>
>>
>>
>>>>> +static const struct msm_dp_desc sc7280_dp_descs[] = {
>>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en =
>>>>> true },
>>>>> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>>> };
>>>>>
>>>>> static const struct msm_dp_config sc7280_dp_cfg = {
>>>>> - .descs = (const struct msm_dp_desc[]) {
>>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort, .wide_bus_en =
>>>>> true },
>>>>> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_eDP, .wide_bus_en = true },
>>>>> - },
>>>>> - .num_descs = 2,
>>>>> + .descs = sc7280_dp_descs,
>>>>> + .num_descs = ARRAY_SIZE(sc7280_dp_descs),
>>>>> +};
>>>>> +
>>>>> +static const struct msm_dp_desc sc8180x_dp_descs[] = {
>>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> + [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_eDP },
>>>>> };
>>>>>
>>>>> static const struct msm_dp_config sc8180x_dp_cfg = {
>>>>> - .descs = (const struct msm_dp_desc[]) {
>>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0ae98000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> - [MSM_DP_CONTROLLER_2] = { .io_start = 0x0ae9a000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_eDP },
>>>>> - },
>>>>> - .num_descs = 3,
>>>>> + .descs = sc8180x_dp_descs,
>>>>> + .num_descs = ARRAY_SIZE(sc8180x_dp_descs),
>>>>> +};
>>>>> +
>>>>> +static const struct msm_dp_desc sm8350_dp_descs[] = {
>>>>> + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> };
>>>>>
>>>>> static const struct msm_dp_config sm8350_dp_cfg = {
>>>>> - .descs = (const struct msm_dp_desc[]) {
>>>>> - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000,
>>>>> .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
>>>>> - },
>>>>> - .num_descs = 1,
>>>>> + .descs = sm8350_dp_descs,
>>>>> + .num_descs = ARRAY_SIZE(sm8350_dp_descs),
>>>>> };
>>>>>
>>>>> static const struct of_device_id dp_dt_match[] = {
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-06-28 17:16 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-27 16:54 [PATCH] drm/msm/dp: use ARRAY_SIZE for calculating num_descs Dmitry Baryshkov
2022-06-27 16:54 ` Dmitry Baryshkov
2022-06-27 19:19 ` Stephen Boyd
2022-06-27 19:19 ` Stephen Boyd
2022-06-27 19:26 ` Kuogee Hsieh
2022-06-27 19:26 ` Kuogee Hsieh
2022-06-27 20:05 ` Dmitry Baryshkov
2022-06-27 20:05 ` Dmitry Baryshkov
2022-06-27 21:01 ` Kuogee Hsieh
2022-06-27 21:01 ` Kuogee Hsieh
2022-06-27 21:23 ` Dmitry Baryshkov
2022-06-27 21:23 ` Dmitry Baryshkov
2022-06-28 17:16 ` Kuogee Hsieh
2022-06-28 17:16 ` Kuogee Hsieh
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.