* [RFC PATCH 0/3] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
@ 2022-07-10 18:45 ` Dmitry Baryshkov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-07-10 18:45 UTC (permalink / raw)
To: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd
Cc: Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter,
Bjorn Andersson, linux-arm-msm, dri-devel, freedreno
An RFC (or rather RFT, Request-for-Testing) series adding support for
DRM_BRIDGE_ATTACH_NO_CONNECTOR. Note, it was compile-tested only. This
bridge is the last one used on the Qualcomm platforms (in
upstream-supported devices) and thus it is the only bridge that prevents
us from removing support for bridge-created connectors from MSM DSI
code.
Dmitry Baryshkov (3):
drm/bridge: ti-sn65dsi86: switch to atomic ops
drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 55 +++++++++++++++------------
1 file changed, 31 insertions(+), 24 deletions(-)
--
2.35.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC PATCH 0/3] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
@ 2022-07-10 18:45 ` Dmitry Baryshkov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-07-10 18:45 UTC (permalink / raw)
To: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd
Cc: freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Bjorn Andersson, Sean Paul
An RFC (or rather RFT, Request-for-Testing) series adding support for
DRM_BRIDGE_ATTACH_NO_CONNECTOR. Note, it was compile-tested only. This
bridge is the last one used on the Qualcomm platforms (in
upstream-supported devices) and thus it is the only bridge that prevents
us from removing support for bridge-created connectors from MSM DSI
code.
Dmitry Baryshkov (3):
drm/bridge: ti-sn65dsi86: switch to atomic ops
drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 55 +++++++++++++++------------
1 file changed, 31 insertions(+), 24 deletions(-)
--
2.35.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC PATCH 1/3] drm/bridge: ti-sn65dsi86: switch to atomic ops
2022-07-10 18:45 ` Dmitry Baryshkov
@ 2022-07-10 18:45 ` Dmitry Baryshkov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-07-10 18:45 UTC (permalink / raw)
To: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd
Cc: Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter,
Bjorn Andersson, linux-arm-msm, dri-devel, freedreno
Make ti-sn65dsi86 use atomic_enable / atomic_disable / atomic_pre_enable
/ atomic_post_disable rather than their non-atomic versions.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 8cad662de9bb..01171547f638 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -752,7 +752,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
return MODE_OK;
}
-static void ti_sn_bridge_disable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
@@ -1011,7 +1012,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
return ret;
}
-static void ti_sn_bridge_enable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
const char *last_err_str = "No supported DP rate";
@@ -1080,7 +1082,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
VSTREAM_ENABLE);
}
-static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
@@ -1093,7 +1096,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
usleep_range(100, 110);
}
-static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
@@ -1114,10 +1118,10 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
.attach = ti_sn_bridge_attach,
.detach = ti_sn_bridge_detach,
.mode_valid = ti_sn_bridge_mode_valid,
- .pre_enable = ti_sn_bridge_pre_enable,
- .enable = ti_sn_bridge_enable,
- .disable = ti_sn_bridge_disable,
- .post_disable = ti_sn_bridge_post_disable,
+ .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
+ .atomic_enable = ti_sn_bridge_atomic_enable,
+ .atomic_disable = ti_sn_bridge_atomic_disable,
+ .atomic_post_disable = ti_sn_bridge_atomic_post_disable,
};
static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
--
2.35.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC PATCH 1/3] drm/bridge: ti-sn65dsi86: switch to atomic ops
@ 2022-07-10 18:45 ` Dmitry Baryshkov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-07-10 18:45 UTC (permalink / raw)
To: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd
Cc: freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Bjorn Andersson, Sean Paul
Make ti-sn65dsi86 use atomic_enable / atomic_disable / atomic_pre_enable
/ atomic_post_disable rather than their non-atomic versions.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 8cad662de9bb..01171547f638 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -752,7 +752,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
return MODE_OK;
}
-static void ti_sn_bridge_disable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
@@ -1011,7 +1012,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
return ret;
}
-static void ti_sn_bridge_enable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
const char *last_err_str = "No supported DP rate";
@@ -1080,7 +1082,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
VSTREAM_ENABLE);
}
-static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
@@ -1093,7 +1096,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
usleep_range(100, 110);
}
-static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
+static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
@@ -1114,10 +1118,10 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
.attach = ti_sn_bridge_attach,
.detach = ti_sn_bridge_detach,
.mode_valid = ti_sn_bridge_mode_valid,
- .pre_enable = ti_sn_bridge_pre_enable,
- .enable = ti_sn_bridge_enable,
- .disable = ti_sn_bridge_disable,
- .post_disable = ti_sn_bridge_post_disable,
+ .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
+ .atomic_enable = ti_sn_bridge_atomic_enable,
+ .atomic_disable = ti_sn_bridge_atomic_disable,
+ .atomic_post_disable = ti_sn_bridge_atomic_post_disable,
};
static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
--
2.35.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC PATCH 2/3] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
2022-07-10 18:45 ` Dmitry Baryshkov
@ 2022-07-10 18:45 ` Dmitry Baryshkov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-07-10 18:45 UTC (permalink / raw)
To: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd
Cc: Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter,
Bjorn Andersson, linux-arm-msm, dri-devel, freedreno
Rather than reading the pdata->connector directly, fetch the connector
using drm_atomic_state. This allows us to make pdata->connector optional
(and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR).
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 01171547f638..df08207d6223 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
}
-static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
+static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
{
- if (pdata->connector->display_info.bpc <= 6)
+ if (connector->display_info.bpc <= 6)
return 18;
else
return 24;
@@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
};
-static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
+static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp)
{
unsigned int bit_rate_khz, dp_rate_mhz;
unsigned int i;
@@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
&pdata->bridge.encoder->crtc->state->adjusted_mode;
/* Calculate minimum bit rate based on our pixel clock. */
- bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata);
+ bit_rate_khz = mode->clock * bpp;
/* Calculate minimum DP data rate, taking 80% as per DP spec */
dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM,
@@ -1016,12 +1016,19 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
struct drm_bridge_state *old_bridge_state)
{
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+ struct drm_connector *connector;
const char *last_err_str = "No supported DP rate";
unsigned int valid_rates;
int dp_rate_idx;
unsigned int val;
int ret = -EINVAL;
int max_dp_lanes;
+ unsigned int bpp;
+
+ connector = drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state,
+ bridge->encoder);
+ if (!connector)
+ return;
max_dp_lanes = ti_sn_get_max_lanes(pdata);
pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
@@ -1047,8 +1054,9 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
+ bpp = ti_sn_bridge_get_bpp(connector);
/* Set the DP output format (18 bpp or 24 bpp) */
- val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
+ val = (bpp == 18) ? BPP_18_RGB : 0;
regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
/* DP lane config */
@@ -1059,7 +1067,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
valid_rates = ti_sn_bridge_read_valid_rates(pdata);
/* Train until we run out of rates */
- for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
+ for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp);
dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
dp_rate_idx++) {
if (!(valid_rates & BIT(dp_rate_idx)))
--
2.35.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC PATCH 2/3] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
@ 2022-07-10 18:45 ` Dmitry Baryshkov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-07-10 18:45 UTC (permalink / raw)
To: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd
Cc: freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Bjorn Andersson, Sean Paul
Rather than reading the pdata->connector directly, fetch the connector
using drm_atomic_state. This allows us to make pdata->connector optional
(and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR).
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 01171547f638..df08207d6223 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
}
-static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
+static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
{
- if (pdata->connector->display_info.bpc <= 6)
+ if (connector->display_info.bpc <= 6)
return 18;
else
return 24;
@@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
};
-static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
+static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp)
{
unsigned int bit_rate_khz, dp_rate_mhz;
unsigned int i;
@@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
&pdata->bridge.encoder->crtc->state->adjusted_mode;
/* Calculate minimum bit rate based on our pixel clock. */
- bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata);
+ bit_rate_khz = mode->clock * bpp;
/* Calculate minimum DP data rate, taking 80% as per DP spec */
dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM,
@@ -1016,12 +1016,19 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
struct drm_bridge_state *old_bridge_state)
{
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
+ struct drm_connector *connector;
const char *last_err_str = "No supported DP rate";
unsigned int valid_rates;
int dp_rate_idx;
unsigned int val;
int ret = -EINVAL;
int max_dp_lanes;
+ unsigned int bpp;
+
+ connector = drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state,
+ bridge->encoder);
+ if (!connector)
+ return;
max_dp_lanes = ti_sn_get_max_lanes(pdata);
pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
@@ -1047,8 +1054,9 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
+ bpp = ti_sn_bridge_get_bpp(connector);
/* Set the DP output format (18 bpp or 24 bpp) */
- val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
+ val = (bpp == 18) ? BPP_18_RGB : 0;
regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
/* DP lane config */
@@ -1059,7 +1067,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
valid_rates = ti_sn_bridge_read_valid_rates(pdata);
/* Train until we run out of rates */
- for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
+ for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp);
dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
dp_rate_idx++) {
if (!(valid_rates & BIT(dp_rate_idx)))
--
2.35.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC PATCH 3/3] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
2022-07-10 18:45 ` Dmitry Baryshkov
@ 2022-07-10 18:45 ` Dmitry Baryshkov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-07-10 18:45 UTC (permalink / raw)
To: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd
Cc: Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter,
Bjorn Andersson, linux-arm-msm, dri-devel, freedreno
Now as the driver does not depend on pdata->connector, add support for
attaching the bridge with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index df08207d6223..9bca4615f71b 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -698,11 +698,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
int ret;
- if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
- DRM_ERROR("Fix bridge driver to make connector optional!");
- return -EINVAL;
- }
-
pdata->aux.drm_dev = bridge->dev;
ret = drm_dp_aux_register(&pdata->aux);
if (ret < 0) {
@@ -710,15 +705,15 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
return ret;
}
- /* We never want the next bridge to *also* create a connector: */
- flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
-
- /* Attach the next bridge */
+ /* Attach the next bridge, We never want the next bridge to *also* create a connector. */
ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
- &pdata->bridge, flags);
+ &pdata->bridge, flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
if (ret < 0)
goto err_initted_aux;
+ if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
+ return 0;
+
pdata->connector = drm_bridge_connector_init(pdata->bridge.dev,
pdata->bridge.encoder);
if (IS_ERR(pdata->connector)) {
--
2.35.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC PATCH 3/3] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
@ 2022-07-10 18:45 ` Dmitry Baryshkov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-07-10 18:45 UTC (permalink / raw)
To: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd
Cc: freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Bjorn Andersson, Sean Paul
Now as the driver does not depend on pdata->connector, add support for
attaching the bridge with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index df08207d6223..9bca4615f71b 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -698,11 +698,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
int ret;
- if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
- DRM_ERROR("Fix bridge driver to make connector optional!");
- return -EINVAL;
- }
-
pdata->aux.drm_dev = bridge->dev;
ret = drm_dp_aux_register(&pdata->aux);
if (ret < 0) {
@@ -710,15 +705,15 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
return ret;
}
- /* We never want the next bridge to *also* create a connector: */
- flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
-
- /* Attach the next bridge */
+ /* Attach the next bridge, We never want the next bridge to *also* create a connector. */
ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
- &pdata->bridge, flags);
+ &pdata->bridge, flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
if (ret < 0)
goto err_initted_aux;
+ if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
+ return 0;
+
pdata->connector = drm_bridge_connector_init(pdata->bridge.dev,
pdata->bridge.encoder);
if (IS_ERR(pdata->connector)) {
--
2.35.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 1/3] drm/bridge: ti-sn65dsi86: switch to atomic ops
2022-07-10 18:45 ` Dmitry Baryshkov
@ 2022-07-10 19:04 ` Sam Ravnborg
-1 siblings, 0 replies; 22+ messages in thread
From: Sam Ravnborg @ 2022-07-10 19:04 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd,
freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Bjorn Andersson, Sean Paul
Hi Dmitry,
On Sun, Jul 10, 2022 at 09:45:34PM +0300, Dmitry Baryshkov wrote:
> Make ti-sn65dsi86 use atomic_enable / atomic_disable / atomic_pre_enable
> / atomic_post_disable rather than their non-atomic versions.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
a more or less identical patch was applied to drm-misc-next
the other day.
See d8b599bf625d1d818fdbb322a272fd2a5ea32e38.
Sam
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 8cad662de9bb..01171547f638 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -752,7 +752,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
> return MODE_OK;
> }
>
> -static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1011,7 +1012,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> return ret;
> }
>
> -static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> const char *last_err_str = "No supported DP rate";
> @@ -1080,7 +1082,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> VSTREAM_ENABLE);
> }
>
> -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1093,7 +1096,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> usleep_range(100, 110);
> }
>
> -static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1114,10 +1118,10 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .attach = ti_sn_bridge_attach,
> .detach = ti_sn_bridge_detach,
> .mode_valid = ti_sn_bridge_mode_valid,
> - .pre_enable = ti_sn_bridge_pre_enable,
> - .enable = ti_sn_bridge_enable,
> - .disable = ti_sn_bridge_disable,
> - .post_disable = ti_sn_bridge_post_disable,
> + .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> + .atomic_enable = ti_sn_bridge_atomic_enable,
> + .atomic_disable = ti_sn_bridge_atomic_disable,
> + .atomic_post_disable = ti_sn_bridge_atomic_post_disable,
> };
>
> static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> --
> 2.35.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 1/3] drm/bridge: ti-sn65dsi86: switch to atomic ops
@ 2022-07-10 19:04 ` Sam Ravnborg
0 siblings, 0 replies; 22+ messages in thread
From: Sam Ravnborg @ 2022-07-10 19:04 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Sean Paul, Jernej Skrabec, Abhinav Kumar, Jonas Karlman,
David Airlie, linux-arm-msm, dri-devel, Neil Armstrong,
Douglas Anderson, Robert Foss, Stephen Boyd, Laurent Pinchart,
Andrzej Hajda, Bjorn Andersson, freedreno
Hi Dmitry,
On Sun, Jul 10, 2022 at 09:45:34PM +0300, Dmitry Baryshkov wrote:
> Make ti-sn65dsi86 use atomic_enable / atomic_disable / atomic_pre_enable
> / atomic_post_disable rather than their non-atomic versions.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
a more or less identical patch was applied to drm-misc-next
the other day.
See d8b599bf625d1d818fdbb322a272fd2a5ea32e38.
Sam
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 8cad662de9bb..01171547f638 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -752,7 +752,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
> return MODE_OK;
> }
>
> -static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1011,7 +1012,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> return ret;
> }
>
> -static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> const char *last_err_str = "No supported DP rate";
> @@ -1080,7 +1082,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> VSTREAM_ENABLE);
> }
>
> -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1093,7 +1096,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> usleep_range(100, 110);
> }
>
> -static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> +static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>
> @@ -1114,10 +1118,10 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .attach = ti_sn_bridge_attach,
> .detach = ti_sn_bridge_detach,
> .mode_valid = ti_sn_bridge_mode_valid,
> - .pre_enable = ti_sn_bridge_pre_enable,
> - .enable = ti_sn_bridge_enable,
> - .disable = ti_sn_bridge_disable,
> - .post_disable = ti_sn_bridge_post_disable,
> + .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> + .atomic_enable = ti_sn_bridge_atomic_enable,
> + .atomic_disable = ti_sn_bridge_atomic_disable,
> + .atomic_post_disable = ti_sn_bridge_atomic_post_disable,
> };
>
> static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> --
> 2.35.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 2/3] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
2022-07-10 18:45 ` Dmitry Baryshkov
@ 2022-07-10 19:11 ` Sam Ravnborg
-1 siblings, 0 replies; 22+ messages in thread
From: Sam Ravnborg @ 2022-07-10 19:11 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd,
freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Bjorn Andersson, Sean Paul
Hi Dmitry,
On Sun, Jul 10, 2022 at 09:45:35PM +0300, Dmitry Baryshkov wrote:
> Rather than reading the pdata->connector directly, fetch the connector
> using drm_atomic_state. This allows us to make pdata->connector optional
> (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR).
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 01171547f638..df08207d6223 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> }
>
> -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> +static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
> {
> - if (pdata->connector->display_info.bpc <= 6)
> + if (connector->display_info.bpc <= 6)
> return 18;
> else
> return 24;
> @@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
> 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
> };
>
> -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp)
> {
> unsigned int bit_rate_khz, dp_rate_mhz;
> unsigned int i;
> @@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> &pdata->bridge.encoder->crtc->state->adjusted_mode;
>
> /* Calculate minimum bit rate based on our pixel clock. */
> - bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata);
> + bit_rate_khz = mode->clock * bpp;
>
> /* Calculate minimum DP data rate, taking 80% as per DP spec */
> dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM,
> @@ -1016,12 +1016,19 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> struct drm_bridge_state *old_bridge_state)
> {
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + struct drm_connector *connector;
> const char *last_err_str = "No supported DP rate";
> unsigned int valid_rates;
> int dp_rate_idx;
> unsigned int val;
> int ret = -EINVAL;
> int max_dp_lanes;
> + unsigned int bpp;
> +
> + connector = drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state,
> + bridge->encoder);
> + if (!connector)
> + return;
It would be prudent with a dev_err() logging here as we do not expect to
fail.
I looked into something similar, but with a less elegant solution, and
could not convince myself that the display driver would create the
connector before ti_sn_bridge_atomic_enable() was called.
This is another reason why a dev_err would be nice - so tester could see
if this fails or not.
With the dev_err added:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>
> max_dp_lanes = ti_sn_get_max_lanes(pdata);
> pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
> @@ -1047,8 +1054,9 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
> DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
>
> + bpp = ti_sn_bridge_get_bpp(connector);
> /* Set the DP output format (18 bpp or 24 bpp) */
> - val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
> + val = (bpp == 18) ? BPP_18_RGB : 0;
> regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
>
> /* DP lane config */
> @@ -1059,7 +1067,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> valid_rates = ti_sn_bridge_read_valid_rates(pdata);
>
> /* Train until we run out of rates */
> - for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
> + for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp);
> dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
> dp_rate_idx++) {
> if (!(valid_rates & BIT(dp_rate_idx)))
> --
> 2.35.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 2/3] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
@ 2022-07-10 19:11 ` Sam Ravnborg
0 siblings, 0 replies; 22+ messages in thread
From: Sam Ravnborg @ 2022-07-10 19:11 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Sean Paul, Jernej Skrabec, Abhinav Kumar, Jonas Karlman,
David Airlie, linux-arm-msm, dri-devel, Neil Armstrong,
Douglas Anderson, Robert Foss, Stephen Boyd, Laurent Pinchart,
Andrzej Hajda, Bjorn Andersson, freedreno
Hi Dmitry,
On Sun, Jul 10, 2022 at 09:45:35PM +0300, Dmitry Baryshkov wrote:
> Rather than reading the pdata->connector directly, fetch the connector
> using drm_atomic_state. This allows us to make pdata->connector optional
> (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR).
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 01171547f638..df08207d6223 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> }
>
> -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> +static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
> {
> - if (pdata->connector->display_info.bpc <= 6)
> + if (connector->display_info.bpc <= 6)
> return 18;
> else
> return 24;
> @@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
> 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
> };
>
> -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp)
> {
> unsigned int bit_rate_khz, dp_rate_mhz;
> unsigned int i;
> @@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> &pdata->bridge.encoder->crtc->state->adjusted_mode;
>
> /* Calculate minimum bit rate based on our pixel clock. */
> - bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata);
> + bit_rate_khz = mode->clock * bpp;
>
> /* Calculate minimum DP data rate, taking 80% as per DP spec */
> dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM,
> @@ -1016,12 +1016,19 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> struct drm_bridge_state *old_bridge_state)
> {
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + struct drm_connector *connector;
> const char *last_err_str = "No supported DP rate";
> unsigned int valid_rates;
> int dp_rate_idx;
> unsigned int val;
> int ret = -EINVAL;
> int max_dp_lanes;
> + unsigned int bpp;
> +
> + connector = drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state,
> + bridge->encoder);
> + if (!connector)
> + return;
It would be prudent with a dev_err() logging here as we do not expect to
fail.
I looked into something similar, but with a less elegant solution, and
could not convince myself that the display driver would create the
connector before ti_sn_bridge_atomic_enable() was called.
This is another reason why a dev_err would be nice - so tester could see
if this fails or not.
With the dev_err added:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>
> max_dp_lanes = ti_sn_get_max_lanes(pdata);
> pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
> @@ -1047,8 +1054,9 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
> DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
>
> + bpp = ti_sn_bridge_get_bpp(connector);
> /* Set the DP output format (18 bpp or 24 bpp) */
> - val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
> + val = (bpp == 18) ? BPP_18_RGB : 0;
> regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
>
> /* DP lane config */
> @@ -1059,7 +1067,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> valid_rates = ti_sn_bridge_read_valid_rates(pdata);
>
> /* Train until we run out of rates */
> - for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
> + for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp);
> dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
> dp_rate_idx++) {
> if (!(valid_rates & BIT(dp_rate_idx)))
> --
> 2.35.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 3/3] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
2022-07-10 18:45 ` Dmitry Baryshkov
@ 2022-07-10 19:12 ` Sam Ravnborg
-1 siblings, 0 replies; 22+ messages in thread
From: Sam Ravnborg @ 2022-07-10 19:12 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd,
freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Bjorn Andersson, Sean Paul
Hi Dmitry,
On Sun, Jul 10, 2022 at 09:45:36PM +0300, Dmitry Baryshkov wrote:
> Now as the driver does not depend on pdata->connector, add support for
> attaching the bridge with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Looks good,
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 3/3] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
@ 2022-07-10 19:12 ` Sam Ravnborg
0 siblings, 0 replies; 22+ messages in thread
From: Sam Ravnborg @ 2022-07-10 19:12 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Sean Paul, Jernej Skrabec, Abhinav Kumar, Jonas Karlman,
David Airlie, linux-arm-msm, dri-devel, Neil Armstrong,
Douglas Anderson, Robert Foss, Stephen Boyd, Laurent Pinchart,
Andrzej Hajda, Bjorn Andersson, freedreno
Hi Dmitry,
On Sun, Jul 10, 2022 at 09:45:36PM +0300, Dmitry Baryshkov wrote:
> Now as the driver does not depend on pdata->connector, add support for
> attaching the bridge with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Looks good,
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 3/3] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
2022-07-10 18:45 ` Dmitry Baryshkov
@ 2022-07-10 21:23 ` Laurent Pinchart
-1 siblings, 0 replies; 22+ messages in thread
From: Laurent Pinchart @ 2022-07-10 21:23 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Jonas Karlman, Jernej Skrabec, Stephen Boyd, Rob Clark,
Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter,
Bjorn Andersson, linux-arm-msm, dri-devel, freedreno
Hi Dmitry,
Thank you for the patch.
On Sun, Jul 10, 2022 at 09:45:36PM +0300, Dmitry Baryshkov wrote:
> Now as the driver does not depend on pdata->connector, add support for
> attaching the bridge with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index df08207d6223..9bca4615f71b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -698,11 +698,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> int ret;
>
> - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> - DRM_ERROR("Fix bridge driver to make connector optional!");
> - return -EINVAL;
> - }
> -
> pdata->aux.drm_dev = bridge->dev;
> ret = drm_dp_aux_register(&pdata->aux);
> if (ret < 0) {
> @@ -710,15 +705,15 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> return ret;
> }
>
> - /* We never want the next bridge to *also* create a connector: */
> - flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
> -
> - /* Attach the next bridge */
> + /* Attach the next bridge, We never want the next bridge to *also* create a connector. */
s/bridge,/bridge./
I also would have wrapped this line.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
> - &pdata->bridge, flags);
> + &pdata->bridge, flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> if (ret < 0)
> goto err_initted_aux;
>
> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> + return 0;
> +
> pdata->connector = drm_bridge_connector_init(pdata->bridge.dev,
> pdata->bridge.encoder);
> if (IS_ERR(pdata->connector)) {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 3/3] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
@ 2022-07-10 21:23 ` Laurent Pinchart
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Pinchart @ 2022-07-10 21:23 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, Abhinav Kumar, Jonas Karlman, David Airlie,
linux-arm-msm, dri-devel, Neil Armstrong, Douglas Anderson,
Robert Foss, Stephen Boyd, Jernej Skrabec, Andrzej Hajda,
Bjorn Andersson, Sean Paul
Hi Dmitry,
Thank you for the patch.
On Sun, Jul 10, 2022 at 09:45:36PM +0300, Dmitry Baryshkov wrote:
> Now as the driver does not depend on pdata->connector, add support for
> attaching the bridge with DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index df08207d6223..9bca4615f71b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -698,11 +698,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> int ret;
>
> - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> - DRM_ERROR("Fix bridge driver to make connector optional!");
> - return -EINVAL;
> - }
> -
> pdata->aux.drm_dev = bridge->dev;
> ret = drm_dp_aux_register(&pdata->aux);
> if (ret < 0) {
> @@ -710,15 +705,15 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> return ret;
> }
>
> - /* We never want the next bridge to *also* create a connector: */
> - flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
> -
> - /* Attach the next bridge */
> + /* Attach the next bridge, We never want the next bridge to *also* create a connector. */
s/bridge,/bridge./
I also would have wrapped this line.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
> - &pdata->bridge, flags);
> + &pdata->bridge, flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> if (ret < 0)
> goto err_initted_aux;
>
> + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> + return 0;
> +
> pdata->connector = drm_bridge_connector_init(pdata->bridge.dev,
> pdata->bridge.encoder);
> if (IS_ERR(pdata->connector)) {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 2/3] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
2022-07-10 18:45 ` Dmitry Baryshkov
@ 2022-07-10 21:28 ` Laurent Pinchart
-1 siblings, 0 replies; 22+ messages in thread
From: Laurent Pinchart @ 2022-07-10 21:28 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, Abhinav Kumar, Jonas Karlman, David Airlie,
linux-arm-msm, dri-devel, Neil Armstrong, Douglas Anderson,
Robert Foss, Stephen Boyd, Jernej Skrabec, Andrzej Hajda,
Bjorn Andersson, Sean Paul
Hi Dmitry,
Thank you for the patch.
On Sun, Jul 10, 2022 at 09:45:35PM +0300, Dmitry Baryshkov wrote:
> Rather than reading the pdata->connector directly, fetch the connector
> using drm_atomic_state. This allows us to make pdata->connector optional
> (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR).
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 01171547f638..df08207d6223 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> }
>
> -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> +static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
> {
> - if (pdata->connector->display_info.bpc <= 6)
> + if (connector->display_info.bpc <= 6)
> return 18;
> else
> return 24;
> @@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
> 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
> };
>
> -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp)
> {
> unsigned int bit_rate_khz, dp_rate_mhz;
> unsigned int i;
> @@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> &pdata->bridge.encoder->crtc->state->adjusted_mode;
>
> /* Calculate minimum bit rate based on our pixel clock. */
> - bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata);
> + bit_rate_khz = mode->clock * bpp;
>
> /* Calculate minimum DP data rate, taking 80% as per DP spec */
> dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM,
> @@ -1016,12 +1016,19 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> struct drm_bridge_state *old_bridge_state)
> {
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + struct drm_connector *connector;
> const char *last_err_str = "No supported DP rate";
> unsigned int valid_rates;
> int dp_rate_idx;
> unsigned int val;
> int ret = -EINVAL;
> int max_dp_lanes;
> + unsigned int bpp;
> +
> + connector = drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state,
> + bridge->encoder);
> + if (!connector)
As Sam mentioned, a dev_err() would be good here if you think this can
happen. If there's no risk that connector will be null, you could drop
the check.
> + return;
>
> max_dp_lanes = ti_sn_get_max_lanes(pdata);
> pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
> @@ -1047,8 +1054,9 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
> DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
>
> + bpp = ti_sn_bridge_get_bpp(connector);
> /* Set the DP output format (18 bpp or 24 bpp) */
> - val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
> + val = (bpp == 18) ? BPP_18_RGB : 0;
You can drop the parentheses.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
>
> /* DP lane config */
> @@ -1059,7 +1067,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> valid_rates = ti_sn_bridge_read_valid_rates(pdata);
>
> /* Train until we run out of rates */
> - for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
> + for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp);
> dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
> dp_rate_idx++) {
> if (!(valid_rates & BIT(dp_rate_idx)))
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 2/3] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
@ 2022-07-10 21:28 ` Laurent Pinchart
0 siblings, 0 replies; 22+ messages in thread
From: Laurent Pinchart @ 2022-07-10 21:28 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Jonas Karlman, Jernej Skrabec, Stephen Boyd, Rob Clark,
Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter,
Bjorn Andersson, linux-arm-msm, dri-devel, freedreno
Hi Dmitry,
Thank you for the patch.
On Sun, Jul 10, 2022 at 09:45:35PM +0300, Dmitry Baryshkov wrote:
> Rather than reading the pdata->connector directly, fetch the connector
> using drm_atomic_state. This allows us to make pdata->connector optional
> (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR).
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 01171547f638..df08207d6223 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> }
>
> -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> +static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
> {
> - if (pdata->connector->display_info.bpc <= 6)
> + if (connector->display_info.bpc <= 6)
> return 18;
> else
> return 24;
> @@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
> 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
> };
>
> -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp)
> {
> unsigned int bit_rate_khz, dp_rate_mhz;
> unsigned int i;
> @@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> &pdata->bridge.encoder->crtc->state->adjusted_mode;
>
> /* Calculate minimum bit rate based on our pixel clock. */
> - bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata);
> + bit_rate_khz = mode->clock * bpp;
>
> /* Calculate minimum DP data rate, taking 80% as per DP spec */
> dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM,
> @@ -1016,12 +1016,19 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> struct drm_bridge_state *old_bridge_state)
> {
> struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> + struct drm_connector *connector;
> const char *last_err_str = "No supported DP rate";
> unsigned int valid_rates;
> int dp_rate_idx;
> unsigned int val;
> int ret = -EINVAL;
> int max_dp_lanes;
> + unsigned int bpp;
> +
> + connector = drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state,
> + bridge->encoder);
> + if (!connector)
As Sam mentioned, a dev_err() would be good here if you think this can
happen. If there's no risk that connector will be null, you could drop
the check.
> + return;
>
> max_dp_lanes = ti_sn_get_max_lanes(pdata);
> pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
> @@ -1047,8 +1054,9 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
> DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
>
> + bpp = ti_sn_bridge_get_bpp(connector);
> /* Set the DP output format (18 bpp or 24 bpp) */
> - val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
> + val = (bpp == 18) ? BPP_18_RGB : 0;
You can drop the parentheses.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
>
> /* DP lane config */
> @@ -1059,7 +1067,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> valid_rates = ti_sn_bridge_read_valid_rates(pdata);
>
> /* Train until we run out of rates */
> - for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
> + for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp);
> dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
> dp_rate_idx++) {
> if (!(valid_rates & BIT(dp_rate_idx)))
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 1/3] drm/bridge: ti-sn65dsi86: switch to atomic ops
2022-07-10 19:04 ` Sam Ravnborg
@ 2022-07-11 7:29 ` Dmitry Baryshkov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-07-11 7:29 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd,
freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Bjorn Andersson, Sean Paul
On Sun, 10 Jul 2022 at 22:04, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Dmitry,
>
> On Sun, Jul 10, 2022 at 09:45:34PM +0300, Dmitry Baryshkov wrote:
> > Make ti-sn65dsi86 use atomic_enable / atomic_disable / atomic_pre_enable
> > / atomic_post_disable rather than their non-atomic versions.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> a more or less identical patch was applied to drm-misc-next
> the other day.
> See d8b599bf625d1d818fdbb322a272fd2a5ea32e38.
Ugh, thanks for pointing this out.
>
> Sam
>
> > ---
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++--------
> > 1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 8cad662de9bb..01171547f638 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -752,7 +752,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
> > return MODE_OK;
> > }
> >
> > -static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> > +static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
> > + struct drm_bridge_state *old_bridge_state)
> > {
> > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >
> > @@ -1011,7 +1012,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> > return ret;
> > }
> >
> > -static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> > +static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> > + struct drm_bridge_state *old_bridge_state)
> > {
> > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > const char *last_err_str = "No supported DP rate";
> > @@ -1080,7 +1082,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> > VSTREAM_ENABLE);
> > }
> >
> > -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> > +static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> > + struct drm_bridge_state *old_bridge_state)
> > {
> > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >
> > @@ -1093,7 +1096,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> > usleep_range(100, 110);
> > }
> >
> > -static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> > +static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> > + struct drm_bridge_state *old_bridge_state)
> > {
> > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >
> > @@ -1114,10 +1118,10 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> > .attach = ti_sn_bridge_attach,
> > .detach = ti_sn_bridge_detach,
> > .mode_valid = ti_sn_bridge_mode_valid,
> > - .pre_enable = ti_sn_bridge_pre_enable,
> > - .enable = ti_sn_bridge_enable,
> > - .disable = ti_sn_bridge_disable,
> > - .post_disable = ti_sn_bridge_post_disable,
> > + .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> > + .atomic_enable = ti_sn_bridge_atomic_enable,
> > + .atomic_disable = ti_sn_bridge_atomic_disable,
> > + .atomic_post_disable = ti_sn_bridge_atomic_post_disable,
> > };
> >
> > static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> > --
> > 2.35.1
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 1/3] drm/bridge: ti-sn65dsi86: switch to atomic ops
@ 2022-07-11 7:29 ` Dmitry Baryshkov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-07-11 7:29 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Sean Paul, Jernej Skrabec, Abhinav Kumar, Jonas Karlman,
David Airlie, linux-arm-msm, dri-devel, Neil Armstrong,
Douglas Anderson, Robert Foss, Stephen Boyd, Laurent Pinchart,
Andrzej Hajda, Bjorn Andersson, freedreno
On Sun, 10 Jul 2022 at 22:04, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Dmitry,
>
> On Sun, Jul 10, 2022 at 09:45:34PM +0300, Dmitry Baryshkov wrote:
> > Make ti-sn65dsi86 use atomic_enable / atomic_disable / atomic_pre_enable
> > / atomic_post_disable rather than their non-atomic versions.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> a more or less identical patch was applied to drm-misc-next
> the other day.
> See d8b599bf625d1d818fdbb322a272fd2a5ea32e38.
Ugh, thanks for pointing this out.
>
> Sam
>
> > ---
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++--------
> > 1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 8cad662de9bb..01171547f638 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -752,7 +752,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,
> > return MODE_OK;
> > }
> >
> > -static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> > +static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
> > + struct drm_bridge_state *old_bridge_state)
> > {
> > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >
> > @@ -1011,7 +1012,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
> > return ret;
> > }
> >
> > -static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> > +static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> > + struct drm_bridge_state *old_bridge_state)
> > {
> > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > const char *last_err_str = "No supported DP rate";
> > @@ -1080,7 +1082,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> > VSTREAM_ENABLE);
> > }
> >
> > -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> > +static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> > + struct drm_bridge_state *old_bridge_state)
> > {
> > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >
> > @@ -1093,7 +1096,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> > usleep_range(100, 110);
> > }
> >
> > -static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> > +static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> > + struct drm_bridge_state *old_bridge_state)
> > {
> > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> >
> > @@ -1114,10 +1118,10 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> > .attach = ti_sn_bridge_attach,
> > .detach = ti_sn_bridge_detach,
> > .mode_valid = ti_sn_bridge_mode_valid,
> > - .pre_enable = ti_sn_bridge_pre_enable,
> > - .enable = ti_sn_bridge_enable,
> > - .disable = ti_sn_bridge_disable,
> > - .post_disable = ti_sn_bridge_post_disable,
> > + .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> > + .atomic_enable = ti_sn_bridge_atomic_enable,
> > + .atomic_disable = ti_sn_bridge_atomic_disable,
> > + .atomic_post_disable = ti_sn_bridge_atomic_post_disable,
> > };
> >
> > static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> > --
> > 2.35.1
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 2/3] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
2022-07-10 19:11 ` Sam Ravnborg
@ 2022-07-11 7:31 ` Dmitry Baryshkov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-07-11 7:31 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Douglas Anderson, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Stephen Boyd,
freedreno, David Airlie, linux-arm-msm, Abhinav Kumar, dri-devel,
Bjorn Andersson, Sean Paul
On Sun, 10 Jul 2022 at 22:11, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Dmitry,
>
> On Sun, Jul 10, 2022 at 09:45:35PM +0300, Dmitry Baryshkov wrote:
> > Rather than reading the pdata->connector directly, fetch the connector
> > using drm_atomic_state. This allows us to make pdata->connector optional
> > (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR).
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++++------
> > 1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 01171547f638..df08207d6223 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> > regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> > }
> >
> > -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> > +static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
> > {
> > - if (pdata->connector->display_info.bpc <= 6)
> > + if (connector->display_info.bpc <= 6)
> > return 18;
> > else
> > return 24;
> > @@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
> > 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
> > };
> >
> > -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> > +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp)
> > {
> > unsigned int bit_rate_khz, dp_rate_mhz;
> > unsigned int i;
> > @@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> > &pdata->bridge.encoder->crtc->state->adjusted_mode;
> >
> > /* Calculate minimum bit rate based on our pixel clock. */
> > - bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata);
> > + bit_rate_khz = mode->clock * bpp;
> >
> > /* Calculate minimum DP data rate, taking 80% as per DP spec */
> > dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM,
> > @@ -1016,12 +1016,19 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> > struct drm_bridge_state *old_bridge_state)
> > {
> > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > + struct drm_connector *connector;
> > const char *last_err_str = "No supported DP rate";
> > unsigned int valid_rates;
> > int dp_rate_idx;
> > unsigned int val;
> > int ret = -EINVAL;
> > int max_dp_lanes;
> > + unsigned int bpp;
> > +
> > + connector = drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state,
> > + bridge->encoder);
> > + if (!connector)
> > + return;
> It would be prudent with a dev_err() logging here as we do not expect to
> fail.
> I looked into something similar, but with a less elegant solution, and
> could not convince myself that the display driver would create the
> connector before ti_sn_bridge_atomic_enable() was called.
If I understand your concern, the connectors (as does the rest of
CRTC/encoder/etc objects) are not dynamic, so they must be created
before being able to use the DRM device or any part of thereof is
being actually used (enable/disable/modeset).
>
> This is another reason why a dev_err would be nice - so tester could see
> if this fails or not.
Will fix this in v2.
>
> With the dev_err added:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>
> >
> > max_dp_lanes = ti_sn_get_max_lanes(pdata);
> > pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
> > @@ -1047,8 +1054,9 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> > drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
> > DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
> >
> > + bpp = ti_sn_bridge_get_bpp(connector);
> > /* Set the DP output format (18 bpp or 24 bpp) */
> > - val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
> > + val = (bpp == 18) ? BPP_18_RGB : 0;
> > regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
> >
> > /* DP lane config */
> > @@ -1059,7 +1067,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> > valid_rates = ti_sn_bridge_read_valid_rates(pdata);
> >
> > /* Train until we run out of rates */
> > - for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
> > + for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp);
> > dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
> > dp_rate_idx++) {
> > if (!(valid_rates & BIT(dp_rate_idx)))
> > --
> > 2.35.1
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 2/3] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
@ 2022-07-11 7:31 ` Dmitry Baryshkov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Baryshkov @ 2022-07-11 7:31 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Sean Paul, Jernej Skrabec, Abhinav Kumar, Jonas Karlman,
David Airlie, linux-arm-msm, dri-devel, Neil Armstrong,
Douglas Anderson, Robert Foss, Stephen Boyd, Laurent Pinchart,
Andrzej Hajda, Bjorn Andersson, freedreno
On Sun, 10 Jul 2022 at 22:11, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Dmitry,
>
> On Sun, Jul 10, 2022 at 09:45:35PM +0300, Dmitry Baryshkov wrote:
> > Rather than reading the pdata->connector directly, fetch the connector
> > using drm_atomic_state. This allows us to make pdata->connector optional
> > (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR).
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++++------
> > 1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 01171547f638..df08207d6223 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> > regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> > }
> >
> > -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata)
> > +static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
> > {
> > - if (pdata->connector->display_info.bpc <= 6)
> > + if (connector->display_info.bpc <= 6)
> > return 18;
> > else
> > return 24;
> > @@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
> > 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
> > };
> >
> > -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> > +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp)
> > {
> > unsigned int bit_rate_khz, dp_rate_mhz;
> > unsigned int i;
> > @@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
> > &pdata->bridge.encoder->crtc->state->adjusted_mode;
> >
> > /* Calculate minimum bit rate based on our pixel clock. */
> > - bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata);
> > + bit_rate_khz = mode->clock * bpp;
> >
> > /* Calculate minimum DP data rate, taking 80% as per DP spec */
> > dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM,
> > @@ -1016,12 +1016,19 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> > struct drm_bridge_state *old_bridge_state)
> > {
> > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > + struct drm_connector *connector;
> > const char *last_err_str = "No supported DP rate";
> > unsigned int valid_rates;
> > int dp_rate_idx;
> > unsigned int val;
> > int ret = -EINVAL;
> > int max_dp_lanes;
> > + unsigned int bpp;
> > +
> > + connector = drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state,
> > + bridge->encoder);
> > + if (!connector)
> > + return;
> It would be prudent with a dev_err() logging here as we do not expect to
> fail.
> I looked into something similar, but with a less elegant solution, and
> could not convince myself that the display driver would create the
> connector before ti_sn_bridge_atomic_enable() was called.
If I understand your concern, the connectors (as does the rest of
CRTC/encoder/etc objects) are not dynamic, so they must be created
before being able to use the DRM device or any part of thereof is
being actually used (enable/disable/modeset).
>
> This is another reason why a dev_err would be nice - so tester could see
> if this fails or not.
Will fix this in v2.
>
> With the dev_err added:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>
> >
> > max_dp_lanes = ti_sn_get_max_lanes(pdata);
> > pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes);
> > @@ -1047,8 +1054,9 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> > drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET,
> > DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);
> >
> > + bpp = ti_sn_bridge_get_bpp(connector);
> > /* Set the DP output format (18 bpp or 24 bpp) */
> > - val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0;
> > + val = (bpp == 18) ? BPP_18_RGB : 0;
> > regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val);
> >
> > /* DP lane config */
> > @@ -1059,7 +1067,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
> > valid_rates = ti_sn_bridge_read_valid_rates(pdata);
> >
> > /* Train until we run out of rates */
> > - for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
> > + for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp);
> > dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
> > dp_rate_idx++) {
> > if (!(valid_rates & BIT(dp_rate_idx)))
> > --
> > 2.35.1
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2022-07-11 7:31 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-10 18:45 [RFC PATCH 0/3] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR Dmitry Baryshkov
2022-07-10 18:45 ` Dmitry Baryshkov
2022-07-10 18:45 ` [RFC PATCH 1/3] drm/bridge: ti-sn65dsi86: switch to atomic ops Dmitry Baryshkov
2022-07-10 18:45 ` Dmitry Baryshkov
2022-07-10 19:04 ` Sam Ravnborg
2022-07-10 19:04 ` Sam Ravnborg
2022-07-11 7:29 ` Dmitry Baryshkov
2022-07-11 7:29 ` Dmitry Baryshkov
2022-07-10 18:45 ` [RFC PATCH 2/3] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state Dmitry Baryshkov
2022-07-10 18:45 ` Dmitry Baryshkov
2022-07-10 19:11 ` Sam Ravnborg
2022-07-10 19:11 ` Sam Ravnborg
2022-07-11 7:31 ` Dmitry Baryshkov
2022-07-11 7:31 ` Dmitry Baryshkov
2022-07-10 21:28 ` Laurent Pinchart
2022-07-10 21:28 ` Laurent Pinchart
2022-07-10 18:45 ` [RFC PATCH 3/3] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR Dmitry Baryshkov
2022-07-10 18:45 ` Dmitry Baryshkov
2022-07-10 19:12 ` Sam Ravnborg
2022-07-10 19:12 ` Sam Ravnborg
2022-07-10 21:23 ` Laurent Pinchart
2022-07-10 21:23 ` Laurent Pinchart
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.