[Freedreno] [PATCH v6 4/4] drm: allow real encoder to be passed for drm_writeback_connector
For some vendor driver implementations, display hardware can
be shared between the encoder used for writeback and the physical
display.
In addition resources such as clocks and interrupts can
also be shared between writeback and the real encoder.
To accommodate such vendor drivers and hardware, allow
real encoder to be passed for drm_writeback_connector.
changes in v6:
- assign the encoder inside
drm_writeback_connector_init_with_encoder() for
better readability
- improve some documentation for internal encoder
Co-developed-by: Kandpal Suraj
Signed-off-by: Kandpal Suraj
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_writeback.c | 18 --
drivers/gpu/drm/vc4/vc4_txp.c | 14 --
include/drm/drm_writeback.h | 21 +++--
3 files changed, 39 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index 797223c..7f72109 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -179,21 +179,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
{
int ret = 0;
- drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+ drm_encoder_helper_add(_connector->internal_encoder,
enc_helper_funcs);
- wb_connector->encoder.possible_crtcs = possible_crtcs;
+ wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
- ret = drm_encoder_init(dev, _connector->encoder,
+ ret = drm_encoder_init(dev, _connector->internal_encoder,
_writeback_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
return ret;
- ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
_connector->encoder,
- con_funcs, formats, n_formats);
+ ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
+ _connector->internal_encoder, con_funcs, formats,
n_formats);
if (ret)
- drm_encoder_cleanup(_connector->encoder);
+ drm_encoder_cleanup(_connector->internal_encoder);
return ret;
}
@@ -238,6 +238,12 @@ int drm_writeback_connector_init_with_encoder(struct
drm_device *dev,
struct drm_mode_config *config = >mode_config;
int ret = create_writeback_properties(dev);
+ /*
+* Assign the encoder passed to this API to the wb_connector's encoder.
+* For drm_writeback_connector_init(), this shall be the
internal_encoder
+*/
+ wb_connector->encoder = enc;
+
if (ret != 0)
return ret;
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 5e53f02..a9b4f83 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -151,6 +151,8 @@ struct vc4_txp {
struct platform_device *pdev;
+ struct drm_encoder drm_enc;
+
struct drm_writeback_connector connector;
void __iomem *regs;
@@ -159,7 +161,7 @@ struct vc4_txp {
static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
{
- return container_of(encoder, struct vc4_txp, connector.encoder);
+ return container_of(encoder, struct vc4_txp, drm_enc);
}
static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector *conn)
@@ -499,9 +501,9 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
wb_conn = >connector;
- drm_encoder_helper_add(_conn->encoder,
_txp_encoder_helper_funcs);
+ drm_encoder_helper_add(>drm_enc, _txp_encoder_helper_funcs);
- ret = drm_encoder_init(drm, _conn->encoder,
+ ret = drm_encoder_init(drm, >drm_enc,
_txp_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
@@ -511,10 +513,10 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
drm_connector_helper_add(_conn->base,
_txp_connector_helper_funcs);
- ret = drm_writeback_connector_init_with_encoder(drm, wb_conn,
_conn->encoder,
+ ret = drm_writeback_connector_init_with_encoder(drm, wb_conn,
>drm_enc,
_txp_connector_funcs, drm_fmts,
ARRAY_SIZE(drm_fmts));
if (ret) {
- drm_encoder_cleanup(_conn->encoder);
+ drm_encoder_cleanup(>drm_enc);
return ret;
}
@@ -523,7 +525,7 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
if (ret)
return ret;
- encoder = >connector.encoder;
+ encoder = txp->connector.encoder;
encoder->possible_crtcs = drm_crtc_mask(crtc);
ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 4795024..3f5c330 100644
---
[Freedreno] [PATCH v6 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API
For vendors drivers which pass an already allocated and
initialized encoder especially for cases where the encoder
hardware is shared OR the writeback encoder shares the resources
with the rest of the display pipeline introduce a new API,
drm_writeback_connector_init_with_encoder() which expects
an initialized encoder as a parameter and only sets up the
writeback connector.
changes in v6:
- remove drm_writeback_connector_setup() and instead
directly call drm_writeback_connector_init_with_encoder()
- fix a drm_writeback_connector typo and function doc which
incorrectly shows that the function accepts enc_helper_funcs
- pass encoder as a parameter explicitly to the new API
for better readability
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/drm_writeback.c | 72 +
include/drm/drm_writeback.h | 6
2 files changed, 64 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dc2ef12..797223c 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -177,6 +177,62 @@ int drm_writeback_connector_init(struct drm_device *dev,
const struct drm_encoder_helper_funcs
*enc_helper_funcs,
const u32 *formats, int n_formats, uint32_t
possible_crtcs)
{
+ int ret = 0;
+
+ drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+
+ wb_connector->encoder.possible_crtcs = possible_crtcs;
+
+ ret = drm_encoder_init(dev, _connector->encoder,
+ _writeback_encoder_funcs,
+ DRM_MODE_ENCODER_VIRTUAL, NULL);
+ if (ret)
+ return ret;
+
+ ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
_connector->encoder,
+ con_funcs, formats, n_formats);
+
+ if (ret)
+ drm_encoder_cleanup(_connector->encoder);
+
+ return ret;
+}
+EXPORT_SYMBOL(drm_writeback_connector_init);
+
+/**
+ * drm_writeback_connector_init_with_encoder - Initialize a writeback
connector and its properties
+ * using the encoder which already assigned and initialized
+ *
+ * @dev: DRM device
+ * @wb_connector: Writeback connector to initialize
+ * @enc: handle to the already initialized drm encoder
+ * @con_funcs: Connector funcs vtable
+ * @formats: Array of supported pixel formats for the writeback engine
+ * @n_formats: Length of the formats array
+ *
+ * This function creates the writeback-connector-specific properties if they
+ * have not been already created, initializes the connector as
+ * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
+ * values.
+ *
+ * This function assumes that the drm_writeback_connector's encoder has
already been
+ * created and initialized before invoking this function.
+ *
+ * In addition, this function also assumes that callers of this API will manage
+ * assigning the encoder helper functions, possible_crtcs and any other encoder
+ * specific operation which is otherwise handled by
drm_writeback_connector_init().
+ *
+ * Drivers should always use this function instead of drm_connector_init() to
+ * set up writeback connectors if they want to manage themselves the lifetime
of the
+ * associated encoder.
+ *
+ * Returns: 0 on success, or a negative error code
+ */
+int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
+ struct drm_writeback_connector *wb_connector, struct
drm_encoder *enc,
+ const struct drm_connector_funcs *con_funcs, const u32 *formats,
+ int n_formats)
+{
struct drm_property_blob *blob;
struct drm_connector *connector = _connector->base;
struct drm_mode_config *config = >mode_config;
@@ -190,15 +246,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
if (IS_ERR(blob))
return PTR_ERR(blob);
- drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
-
- wb_connector->encoder.possible_crtcs = possible_crtcs;
-
- ret = drm_encoder_init(dev, _connector->encoder,
- _writeback_encoder_funcs,
- DRM_MODE_ENCODER_VIRTUAL, NULL);
- if (ret)
- goto fail;
connector->interlace_allowed = 0;
@@ -207,8 +254,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
if (ret)
goto connector_fail;
- ret = drm_connector_attach_encoder(connector,
- _connector->encoder);
+ ret = drm_connector_attach_encoder(connector, enc);
if (ret)
goto attach_fail;
@@ -237,12 +283,10 @@ int drm_writeback_connector_init(struct drm_device *dev,
attach_fail:
drm_connector_cleanup(connector);
connector_fail:
-
[Freedreno] [PATCH v6 1/4] drm: allow passing possible_crtcs to drm_writeback_connector_init()
Clients of drm_writeback_connector_init() initialize the
possible_crtcs and then invoke the call to this API.
To simplify things, allow passing possible_crtcs as a parameter
to drm_writeback_connector_init() and make changes to the
other drm drivers to make them compatible with this change.
changes in v6:
- None
Signed-off-by: Abhinav Kumar
Acked-by: Liviu Dudau
---
drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 3 +--
drivers/gpu/drm/arm/malidp_mw.c | 4 ++--
drivers/gpu/drm/drm_writeback.c | 6 +-
drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 4 ++--
drivers/gpu/drm/vc4/vc4_txp.c| 3 ++-
drivers/gpu/drm/vkms/vkms_writeback.c| 4 ++--
include/drm/drm_writeback.h | 2 +-
7 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
index e465cc4..40774e6 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
@@ -155,7 +155,6 @@ static int komeda_wb_connector_add(struct komeda_kms_dev
*kms,
kwb_conn->wb_layer = kcrtc->master->wb_layer;
wb_conn = _conn->base;
- wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(>base));
formats = komeda_get_layer_fourcc_list(>fmt_tbl,
kwb_conn->wb_layer->layer_type,
@@ -164,7 +163,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev
*kms,
err = drm_writeback_connector_init(>base, wb_conn,
_wb_connector_funcs,
_wb_encoder_helper_funcs,
- formats, n_formats);
+ formats, n_formats,
BIT(drm_crtc_index(>base)));
komeda_put_fourcc_list(formats);
if (err) {
kfree(kwb_conn);
diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index f5847a7..e54921d 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -212,7 +212,6 @@ int malidp_mw_connector_init(struct drm_device *drm)
if (!malidp->dev->hw->enable_memwrite)
return 0;
- malidp->mw_connector.encoder.possible_crtcs = 1 <<
drm_crtc_index(>crtc);
drm_connector_helper_add(>mw_connector.base,
_mw_connector_helper_funcs);
@@ -223,7 +222,8 @@ int malidp_mw_connector_init(struct drm_device *drm)
ret = drm_writeback_connector_init(drm, >mw_connector,
_mw_connector_funcs,
_mw_encoder_helper_funcs,
- formats, n_formats);
+ formats, n_formats,
+ (1 << drm_crtc_index(>crtc)));
kfree(formats);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dccf4504..dc2ef12 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -157,6 +157,7 @@ static const struct drm_encoder_funcs
drm_writeback_encoder_funcs = {
* @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal
encoder
* @formats: Array of supported pixel formats for the writeback engine
* @n_formats: Length of the formats array
+ * @possible_crtcs: possible crtcs for the internal writeback encoder
*
* This function creates the writeback-connector-specific properties if they
* have not been already created, initializes the connector as
@@ -174,7 +175,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
struct drm_writeback_connector *wb_connector,
const struct drm_connector_funcs *con_funcs,
const struct drm_encoder_helper_funcs
*enc_helper_funcs,
-const u32 *formats, int n_formats)
+const u32 *formats, int n_formats, uint32_t
possible_crtcs)
{
struct drm_property_blob *blob;
struct drm_connector *connector = _connector->base;
@@ -190,6 +191,9 @@ int drm_writeback_connector_init(struct drm_device *dev,
return PTR_ERR(blob);
drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
+
+ wb_connector->encoder.possible_crtcs = possible_crtcs;
+
ret = drm_encoder_init(dev, _connector->encoder,
_writeback_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
index
[Freedreno] [PATCH v6 0/4] Allow drm_writeback_connector to accept pointer to drm_encoder
There are some vendor drivers for which the writeback encoder shares hardware resources such as clocks and interrupts with the rest of the display pipeline. In addition, there can be use-cases where the writeback encoder could be a shared encoder between the physical display path and the writeback path. To accommodate for such cases, change the drm_writeback_connector to accept a pointer to drm_encoder. For existing users of drm_writeback_connector there will not be any change in functionality due to this change. This approach was first posted by Suraj Kandpal here [1] for both encoder and connector. But after discussions [2], the consensus was reached to split this change for the drm_encoder first and the drm_connector part can be reworked in a subsequent change later. Validation of this change was done using igt_writeback tests on MSM based RB5 board using the changes posted here [3]. For all other chipsets, these changes were compile-tested. [1] https://patchwork.kernel.org/project/dri-devel/patch/20220202081702.22119-1-suraj.kand...@intel.com/ [2] https://patchwork.kernel.org/project/dri-devel/patch/20220202085429.22261-6-suraj.kand...@intel.com/ [3] https://patchwork.freedesktop.org/series/99724/ changes in v6: - remove drm_writeback_connector_setup() and instead directly call drm_writeback_connector_init_with_encoder() - pass encoder as a parameter explicitly to the new API for better readability Abhinav Kumar (4): drm: allow passing possible_crtcs to drm_writeback_connector_init() drm: introduce drm_writeback_connector_init_with_encoder() API drm/vc4: change vc4 driver to use drm_writeback_connector_init_with_encoder() drm: allow real encoder to be passed for drm_writeback_connector .../drm/arm/display/komeda/komeda_wb_connector.c | 3 +- drivers/gpu/drm/arm/malidp_mw.c| 4 +- drivers/gpu/drm/drm_writeback.c| 78 ++ drivers/gpu/drm/rcar-du/rcar_du_writeback.c| 4 +- drivers/gpu/drm/vc4/vc4_txp.c | 35 +++--- drivers/gpu/drm/vkms/vkms_writeback.c | 4 +- include/drm/drm_writeback.h| 29 +++- 7 files changed, 126 insertions(+), 31 deletions(-) -- 2.7.4
Re: [Freedreno] [PATCH v6 7/8] drm/msm/dp: Support edp/dp without hpd
Hi,
On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
wrote:
>
> Some eDP sinks or platform boards will not support hpd.
> This patch adds support for those cases.
You could say more, like:
If we're not using HPD then _both_ the panel node and the eDP
controller node will have "no-hpd". This tells the eDP panel code to
hardcode the maximum possible delay for a panel to power up and tells
the eDP driver that it should continue to do transfers even if HPD
isn't asserted.
> Signed-off-by: Sankeerth Billakanti
> ---
> drivers/gpu/drm/msm/dp/dp_catalog.c | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 1809ce2..8f1fc71 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -244,10 +244,17 @@ void dp_catalog_aux_update_cfg(struct dp_catalog
> *dp_catalog)
>
> int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
> {
> - u32 state;
> + u32 state, hpd_en;
> struct dp_catalog_private *catalog = container_of(dp_catalog,
> struct dp_catalog_private, dp_catalog);
>
> + hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL);
> + hpd_en &= DP_DP_HPD_CTRL_HPD_EN;
> +
> + /* no-hpd case */
> + if (!hpd_en)
> + return 0;
> +
> /* poll for hpd connected status every 2ms and timeout after 500ms */
> return readl_poll_timeout(catalog->io->dp_controller.aux.base +
> REG_DP_DP_HPD_INT_STATUS,
> @@ -586,8 +593,10 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
> *dp_catalog)
> reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
>
> - /* Enable HPD */
> - dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN);
> + /* Enable HPD if supported*/
> + if (!of_property_read_bool(catalog->dev->of_node, "no-hpd"))
I don't think this is a particularly lightweight operation. It's
literally iterating through all of our device tree properties and
doing string compares on them. ...but this function is called somewhat
often, isn't it? It feels like the kind of thing that should happen at
probe time and be stored in a boolean.
...and then you can use that same boolean in
dp_catalog_aux_wait_for_hpd_connect_state() rather than reading the
register value, right?
-Doug
Re: [Freedreno] [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp
Hi, On Wed, Mar 30, 2022 at 11:02 PM Sankeerth Billakanti (QUIC) wrote: > > Hi Dmitry, > > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti > > wrote: > > > > > > The panel-edp driver modes needs to be validated differently from DP > > > because the link capabilities are not available for EDP by that time. > > > > > > Signed-off-by: Sankeerth Billakanti > > > > This should not be necessary after > > https://patchwork.freedesktop.org/patch/479261/?series=101682=1. > > Could you please check? > > > > The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore but we need > to return early for eDP because unlike DP, eDP context will not have the > information > about the number of lanes and link clock. > > So, I will modify the patch to return after the DP_MAX_PIXEL_CLK_KHZ check if > is_eDP is set. I haven't walked through all the relevant code but something you said above sounds strange. You say that for eDP we don't have info about the number of lanes? We _should_. It's certainly possible to have a panel that supports _either_ 1 or 2 lanes but then only physically connect 1 lane to it. ...or you could have a panel that supports 2 or 4 lanes and you only connect 1 lane. See, for instance, ti_sn_bridge_parse_lanes. There we assume 4 lanes but if a "data-lanes" property is present then we can use that to know that fewer lanes are physically connected. It's also possible to connect more lanes to a panel than it supports. You could connect 2 lanes to it but then it only supports 1. This case needs to be handled as well... -Doug
Re: [Freedreno] [PATCH v6 5/8] drm/msm/dp: prevent multiple votes for dp resources
Hi, On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti wrote: > > The aux_bus support with the dp_display driver will enable the dp > resources during msm_dp_modeset_init. The host_init has to return early > if the core is already initialized to prevent putting an additional vote > for the dp controller resources. > > Signed-off-by: Sankeerth Billakanti > --- > drivers/gpu/drm/msm/dp/dp_display.c | 10 ++ > 1 file changed, 10 insertions(+) I'm not a huge fan of this but I'll leave it up to Dmitry. In general it feels like there should be _a_ place that enables these resources. Checks like this make it feel like we just scattershot enabling resources in a bunch of random places instead of coming up with the design for enabling them in the right place. In any case, if we do end up landing this patch, it sure feels like it needs to move earlier in the patch series, right? This patch shouldn't hurt even without the other patches in the series but if you apply the earlier patches in the series without this one then you'll have a bug, right? That means this needs to come earlier. -Doug
Re: [Freedreno] [PATCH v6 3/8] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
Hi, On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti wrote: > > @@ -1374,6 +1382,12 @@ static int dp_pm_resume(struct device *dev) > dp_catalog_ctrl_hpd_config(dp->catalog); > > > + if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort) > + dp_catalog_hpd_config_intr(dp->catalog, > + DP_DP_HPD_PLUG_INT_MASK | > + DP_DP_HPD_UNPLUG_INT_MASK, > + true); > + nit: why are there two blank lines above? > @@ -1639,6 +1653,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) > return; > } > > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + dp_hpd_plug_handle(dp_display, 0); > + Should you add a "pre_enable" and do it there? That would make it more symmetric with the fact that you have the countertpart in "post_disable". Overall: I'm probably not familiar enough with this code to give it a full review. I'm hoping that Dmitry knows it well enough... ;-) -Doug
Re: [Freedreno] [PATCH v6 2/8] drm/msm/dp: wait for hpd high before aux transaction
Hi,
On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
wrote:
>
> The source device should ensure the sink is ready before proceeding to
> read the sink capability or performing any aux transactions. The sink
s/performing/perform
> will indicate its readiness by asserting the HPD line. The controller
> driver needs to wait for the hpd line to be asserted by the sink before
> performing any aux transactions.
>
> The eDP sink is assumed to be always connected. It needs power from the
> source and its HPD line will be asserted only after the panel is powered
> on. The panel power will be enabled from the panel-edp driver and only
> after that, the hpd line will be asserted.
>
> Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd
> line gets asserted to indicate the sink is connected and ready. Hence
> there is no need to wait for the hpd line to be asserted for a DP sink.
>
> Signed-off-by: Sankeerth Billakanti
> ---
>
> Changes in v6:
> - Wait for hpd high only for eDP
> - Split into smaller patches
>
> drivers/gpu/drm/msm/dp/dp_aux.c | 13 -
> drivers/gpu/drm/msm/dp/dp_aux.h | 3 ++-
> drivers/gpu/drm/msm/dp/dp_catalog.c | 13 +
> drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
> drivers/gpu/drm/msm/dp/dp_display.c | 3 ++-
> 5 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 6d36f63..a217c80 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -36,6 +36,7 @@ struct dp_aux_private {
> bool initted;
> u32 offset;
> u32 segment;
> + bool is_edp;
>
> struct drm_dp_aux dp_aux;
> };
> @@ -337,6 +338,14 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> goto exit;
> }
>
> + if (aux->is_edp) {
Adding a comment about _why_ you're doing this just for eDP would
probably be a good idea. Like maybe:
/*
* For eDP it's important to give a reasonably long wait here for HPD
* to be asserted. This is because the panel driver may have _just_
* turned on the panel and then tried to do an AUX transfer. The panel
* driver has no way of knowing when the panel is ready, so it's up
* to us to wait. For DP we never get into this situation so let's
* avoid ever doing the extra long wait for DP.
*/
> @@ -491,7 +500,8 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
> drm_dp_aux_unregister(dp_aux);
> }
>
> -struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
> +struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
> + bool is_edp)
nit: I think your indentation of the 2nd line isn't quite right.
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
> index 82afc8d..c99aeec 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.h
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.h
> @@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux);
> void dp_aux_deinit(struct drm_dp_aux *dp_aux);
> void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
>
> -struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog
> *catalog);
> +struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
> + bool is_edp);
nit: I think your indentation of the 2nd line isn't quite right.
Things are pretty much nits, so FWIW:
Reviewed-by: Douglas Anderson
Re: [Freedreno] [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi,
On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
wrote:
>
> @@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
> struct drm_device *dev,
>
> dp_display->encoder = encoder;
>
> + ret = dp_display_get_next_bridge(dp_display);
> + if (ret)
> + return ret;
It feels weird to me that this is in a function called "modeset_init",
though I certainly don't know the structure of the MSM display code
well enough to fully comment. My expectation would have been that
devm_of_dp_aux_populate_ep_devices() would have been called from your
probe routine and then you would have returned -EPROBE_DEFER from your
probe if you were unable to find the panel afterwards.
Huh, but I guess you _are_ getting called (indirectly) from
dpu_kms_hw_init() and I can't imagine AUX transfers working before
that function is called, so maybe I should just accept that it's
complicated and let those who understand this driver better confirm
that it's OK. ;-)
> @@ -140,5 +140,6 @@ struct dp_parser {
> * can be parsed using this module.
> */
> struct dp_parser *dp_parser_get(struct platform_device *pdev);
> +int dp_parser_find_next_bridge(struct dp_parser *parser);
Everything else in this file is described w/ kerneldoc. Shouldn't your
function also have a kerneldoc comment?
Re: [Freedreno] [PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin
On 3/31/22 21:58, Rob Clark wrote:
> On Thu, Mar 31, 2022 at 11:27 AM Dmitry Osipenko
> wrote:
>>
>> On 3/30/22 23:47, Rob Clark wrote:
>>> From: Rob Clark
>>>
>>> Combines duplicate vma lookup in the get_and_pin path.
>>>
>>> Signed-off-by: Rob Clark
>>> ---
>>> drivers/gpu/drm/msm/msm_gem.c | 50 ++-
>>> 1 file changed, 26 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
>>> index deafae6feaa8..218744a490a4 100644
>>> --- a/drivers/gpu/drm/msm/msm_gem.c
>>> +++ b/drivers/gpu/drm/msm/msm_gem.c
>>> @@ -376,39 +376,40 @@ put_iova_vmas(struct drm_gem_object *obj)
>>> }
>>> }
>>>
>>> -static int get_iova_locked(struct drm_gem_object *obj,
>>> - struct msm_gem_address_space *aspace, uint64_t *iova,
>>> +static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj,
>>> + struct msm_gem_address_space *aspace,
>>> u64 range_start, u64 range_end)
>>> {
>>> struct msm_gem_vma *vma;
>>> - int ret = 0;
>>>
>>> GEM_WARN_ON(!msm_gem_is_locked(obj));
>>>
>>> vma = lookup_vma(obj, aspace);
>>>
>>> if (!vma) {
>>> + int ret;
>>> +
>>> vma = add_vma(obj, aspace);
>>> if (IS_ERR(vma))
>>> - return PTR_ERR(vma);
>>> + return vma;
>>>
>>> ret = msm_gem_init_vma(aspace, vma, obj->size,
>>> range_start, range_end);
>>> if (ret) {
>> You're allocation range_start -> range_end
>>
>>
>>> del_vma(vma);
>>> - return ret;
>>> + return ERR_PTR(ret);
>>> }
>>> + } else {
>>> + GEM_WARN_ON(vma->iova < range_start);
>>> + GEM_WARN_ON((vma->iova + obj->size) > range_end);
>>
>> and then comparing range_start -> range_start + obj->size, hence you're
>> assuming that range_end always equals to obj->size during the allocation.
>>
>> I'm not sure what is the idea here.. this looks inconsistent. I think
>> you wanted to write:
>>
>> GEM_WARN_ON(vma->iova < range_start);
>> GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) >
>> range_end);
>>
>> But is it really useful to check whether the new range is inside of the
>> old range? Shouldn't it be always a error to change the IOVA range
>> without reallocating vma?
>
> There are a few cases (for allocations for GMU) where the range is
> larger than the bo.. see a6xx_gmu_memory_alloc()
Ahh, I didn't read the code properly and see now why you're using the
obj->size. It's the range where you want to put the BO. Looks good then.
Reviewed-by: Dmitry Osipenko
Re: [Freedreno] [PATCH v2 10/10] drm/msm: Add a way for userspace to allocate GPU iova
On Thu, Mar 31, 2022 at 12:41 PM Dmitry Osipenko
wrote:
>
> On 3/31/22 22:02, Rob Clark wrote:
> > On Thu, Mar 31, 2022 at 11:52 AM Dmitry Osipenko
> > wrote:
> >>
> >> ...
> >>> +/*
> >>> + * Get the requested iova but don't pin it. Fails if the requested iova
> >>> is
> >>> + * not available. Doesn't need a put because iovas are currently valid
> >>> for
> >>> + * the life of the object.
> >>> + *
> >>> + * Setting an iova of zero will clear the vma.
> >>> + */
> >>> +int msm_gem_set_iova(struct drm_gem_object *obj,
> >>> + struct msm_gem_address_space *aspace, uint64_t iova)
> >>> +{
> >>> + int ret = 0;
> >>
> >> nit: No need to initialize the ret
> >
> > actually, we do
>
> Indeed, sorry :)
>
> ...
> >>> int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj,
> >>> struct msm_gem_address_space *aspace, uint64_t *iova,
> >>> u64 range_start, u64 range_end);
> >> nit: There is an odd mix of uint64_t and u64 (and alike) in the MSM code
> >> :) The uint64_t variant shouldn't be used by kernel code in general and
> >> checkpatch should want about it.
> >
> > one of many things that I disagree with checkpatch about ;-)
> >
> > I prefer standard types to custom ones. I _kinda_ get the argument in
> > case of uapi (but IMHO that doesn't apply to how drm uapi headers are
> > used)
>
> I'd understand if it was all either uint64_t or u64, but the mix.. hm.
yeah, fair, we could be a bit more consistent
BR,
-R
Re: [Freedreno] [PATCH v2 10/10] drm/msm: Add a way for userspace to allocate GPU iova
On 3/31/22 22:02, Rob Clark wrote:
> On Thu, Mar 31, 2022 at 11:52 AM Dmitry Osipenko
> wrote:
>>
>> ...
>>> +/*
>>> + * Get the requested iova but don't pin it. Fails if the requested iova is
>>> + * not available. Doesn't need a put because iovas are currently valid for
>>> + * the life of the object.
>>> + *
>>> + * Setting an iova of zero will clear the vma.
>>> + */
>>> +int msm_gem_set_iova(struct drm_gem_object *obj,
>>> + struct msm_gem_address_space *aspace, uint64_t iova)
>>> +{
>>> + int ret = 0;
>>
>> nit: No need to initialize the ret
>
> actually, we do
Indeed, sorry :)
...
>>> int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj,
>>> struct msm_gem_address_space *aspace, uint64_t *iova,
>>> u64 range_start, u64 range_end);
>> nit: There is an odd mix of uint64_t and u64 (and alike) in the MSM code
>> :) The uint64_t variant shouldn't be used by kernel code in general and
>> checkpatch should want about it.
>
> one of many things that I disagree with checkpatch about ;-)
>
> I prefer standard types to custom ones. I _kinda_ get the argument in
> case of uapi (but IMHO that doesn't apply to how drm uapi headers are
> used)
I'd understand if it was all either uint64_t or u64, but the mix.. hm.
Re: [Freedreno] [PATCH v2 10/10] drm/msm: Add a way for userspace to allocate GPU iova
On Thu, Mar 31, 2022 at 11:52 AM Dmitry Osipenko
wrote:
>
> ...
> > +/*
> > + * Get the requested iova but don't pin it. Fails if the requested iova is
> > + * not available. Doesn't need a put because iovas are currently valid for
> > + * the life of the object.
> > + *
> > + * Setting an iova of zero will clear the vma.
> > + */
> > +int msm_gem_set_iova(struct drm_gem_object *obj,
> > + struct msm_gem_address_space *aspace, uint64_t iova)
> > +{
> > + int ret = 0;
>
> nit: No need to initialize the ret
actually, we do
> > + msm_gem_lock(obj);
> > + if (!iova) {
> > + ret = clear_iova(obj, aspace);
> > + } else {
> > + struct msm_gem_vma *vma;
> > + vma = get_vma_locked(obj, aspace, iova, iova + obj->size);
> > + if (IS_ERR(vma)) {
> > + ret = PTR_ERR(vma);
> > + } else if (GEM_WARN_ON(vma->iova != iova)) {
> > + clear_iova(obj, aspace);
> > + ret = -ENOSPC;
>
> The (vma->iova != iova) means that vma is already set, but to a
> different address. Is -ENOSPC really appropriate here? -EBUSY or -EINVAL
> looks more natural to me.
yeah, -EBUSY is better
> > + }
> > + }
> > + msm_gem_unlock(obj);
> > +
> > + return ret;
> > +}
> > +
> > /*
> > * Unpin a iova by updating the reference counts. The memory isn't actually
> > * purged until something else (shrinker, mm_notifier, destroy, etc)
> > decides
> > diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> > index 38d66e1248b1..efa2e5c19f1e 100644
> > --- a/drivers/gpu/drm/msm/msm_gem.h
> > +++ b/drivers/gpu/drm/msm/msm_gem.h
> > @@ -38,6 +38,12 @@ struct msm_gem_address_space {
> >
> > /* @faults: the number of GPU hangs associated with this address
> > space */
> > int faults;
> > +
> > + /** @va_start: lowest possible address to allocate */
> > + uint64_t va_start;
> > +
> > + /** @va_size: the size of the address space (in bytes) */
> > + uint64_t va_size;
> > };
> >
> > struct msm_gem_address_space *
> > @@ -144,6 +150,8 @@ struct msm_gem_vma *msm_gem_get_vma_locked(struct
> > drm_gem_object *obj,
> > struct msm_gem_address_space
> > *aspace);
> > int msm_gem_get_iova(struct drm_gem_object *obj,
> > struct msm_gem_address_space *aspace, uint64_t *iova);
> > +int msm_gem_set_iova(struct drm_gem_object *obj,
> > + struct msm_gem_address_space *aspace, uint64_t iova);
> > int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj,
> > struct msm_gem_address_space *aspace, uint64_t *iova,
> > u64 range_start, u64 range_end);
> nit: There is an odd mix of uint64_t and u64 (and alike) in the MSM code
> :) The uint64_t variant shouldn't be used by kernel code in general and
> checkpatch should want about it.
one of many things that I disagree with checkpatch about ;-)
I prefer standard types to custom ones. I _kinda_ get the argument in
case of uapi (but IMHO that doesn't apply to how drm uapi headers are
used)
BR,
-R
Re: [Freedreno] [PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin
On Thu, Mar 31, 2022 at 11:27 AM Dmitry Osipenko
wrote:
>
> On 3/30/22 23:47, Rob Clark wrote:
> > From: Rob Clark
> >
> > Combines duplicate vma lookup in the get_and_pin path.
> >
> > Signed-off-by: Rob Clark
> > ---
> > drivers/gpu/drm/msm/msm_gem.c | 50 ++-
> > 1 file changed, 26 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> > index deafae6feaa8..218744a490a4 100644
> > --- a/drivers/gpu/drm/msm/msm_gem.c
> > +++ b/drivers/gpu/drm/msm/msm_gem.c
> > @@ -376,39 +376,40 @@ put_iova_vmas(struct drm_gem_object *obj)
> > }
> > }
> >
> > -static int get_iova_locked(struct drm_gem_object *obj,
> > - struct msm_gem_address_space *aspace, uint64_t *iova,
> > +static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj,
> > + struct msm_gem_address_space *aspace,
> > u64 range_start, u64 range_end)
> > {
> > struct msm_gem_vma *vma;
> > - int ret = 0;
> >
> > GEM_WARN_ON(!msm_gem_is_locked(obj));
> >
> > vma = lookup_vma(obj, aspace);
> >
> > if (!vma) {
> > + int ret;
> > +
> > vma = add_vma(obj, aspace);
> > if (IS_ERR(vma))
> > - return PTR_ERR(vma);
> > + return vma;
> >
> > ret = msm_gem_init_vma(aspace, vma, obj->size,
> > range_start, range_end);
> > if (ret) {
> You're allocation range_start -> range_end
>
>
> > del_vma(vma);
> > - return ret;
> > + return ERR_PTR(ret);
> > }
> > + } else {
> > + GEM_WARN_ON(vma->iova < range_start);
> > + GEM_WARN_ON((vma->iova + obj->size) > range_end);
>
> and then comparing range_start -> range_start + obj->size, hence you're
> assuming that range_end always equals to obj->size during the allocation.
>
> I'm not sure what is the idea here.. this looks inconsistent. I think
> you wanted to write:
>
> GEM_WARN_ON(vma->iova < range_start);
> GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) >
> range_end);
>
> But is it really useful to check whether the new range is inside of the
> old range? Shouldn't it be always a error to change the IOVA range
> without reallocating vma?
There are a few cases (for allocations for GMU) where the range is
larger than the bo.. see a6xx_gmu_memory_alloc()
BR,
-R
>
> I'd expect to see:
>
> GEM_WARN_ON(vma->iova != range_start);
> GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) !=
> range_end);
>
> and then error out if range mismatches.
Re: [Freedreno] [PATCH v2 10/10] drm/msm: Add a way for userspace to allocate GPU iova
On 3/31/22 21:52, Dmitry Osipenko wrote:
> ...
>> +/*
>> + * Get the requested iova but don't pin it. Fails if the requested iova is
>> + * not available. Doesn't need a put because iovas are currently valid for
>> + * the life of the object.
>> + *
>> + * Setting an iova of zero will clear the vma.
>> + */
>> +int msm_gem_set_iova(struct drm_gem_object *obj,
>> + struct msm_gem_address_space *aspace, uint64_t iova)
>> +{
>> +int ret = 0;
>
> nit: No need to initialize the ret
>
>> +msm_gem_lock(obj);
>> +if (!iova) {
>> +ret = clear_iova(obj, aspace);
>> +} else {
>> +struct msm_gem_vma *vma;
>> +vma = get_vma_locked(obj, aspace, iova, iova + obj->size);
>> +if (IS_ERR(vma)) {
>> +ret = PTR_ERR(vma);
>> +} else if (GEM_WARN_ON(vma->iova != iova)) {
>> +clear_iova(obj, aspace);
>> +ret = -ENOSPC;
>
> The (vma->iova != iova) means that vma is already set, but to a
> different address. Is -ENOSPC really appropriate here? -EBUSY or -EINVAL
> looks more natural to me.
>
>> +}
>> +}
>> +msm_gem_unlock(obj);
>> +
>> +return ret;
>> +}
>> +
>> /*
>> * Unpin a iova by updating the reference counts. The memory isn't actually
>> * purged until something else (shrinker, mm_notifier, destroy, etc) decides
>> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
>> index 38d66e1248b1..efa2e5c19f1e 100644
>> --- a/drivers/gpu/drm/msm/msm_gem.h
>> +++ b/drivers/gpu/drm/msm/msm_gem.h
>> @@ -38,6 +38,12 @@ struct msm_gem_address_space {
>>
>> /* @faults: the number of GPU hangs associated with this address space
>> */
>> int faults;
>> +
>> +/** @va_start: lowest possible address to allocate */
>> +uint64_t va_start;
>> +
>> +/** @va_size: the size of the address space (in bytes) */
>> +uint64_t va_size;
>> };
>>
>> struct msm_gem_address_space *
>> @@ -144,6 +150,8 @@ struct msm_gem_vma *msm_gem_get_vma_locked(struct
>> drm_gem_object *obj,
>> struct msm_gem_address_space
>> *aspace);
>> int msm_gem_get_iova(struct drm_gem_object *obj,
>> struct msm_gem_address_space *aspace, uint64_t *iova);
>> +int msm_gem_set_iova(struct drm_gem_object *obj,
>> +struct msm_gem_address_space *aspace, uint64_t iova);
>> int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj,
>> struct msm_gem_address_space *aspace, uint64_t *iova,
>> u64 range_start, u64 range_end);
> nit: There is an odd mix of uint64_t and u64 (and alike) in the MSM code
> :) The uint64_t variant shouldn't be used by kernel code in general and
> checkpatch should want about it.
s/want/warn/
Re: [Freedreno] [PATCH v2 10/10] drm/msm: Add a way for userspace to allocate GPU iova
...
> +/*
> + * Get the requested iova but don't pin it. Fails if the requested iova is
> + * not available. Doesn't need a put because iovas are currently valid for
> + * the life of the object.
> + *
> + * Setting an iova of zero will clear the vma.
> + */
> +int msm_gem_set_iova(struct drm_gem_object *obj,
> + struct msm_gem_address_space *aspace, uint64_t iova)
> +{
> + int ret = 0;
nit: No need to initialize the ret
> + msm_gem_lock(obj);
> + if (!iova) {
> + ret = clear_iova(obj, aspace);
> + } else {
> + struct msm_gem_vma *vma;
> + vma = get_vma_locked(obj, aspace, iova, iova + obj->size);
> + if (IS_ERR(vma)) {
> + ret = PTR_ERR(vma);
> + } else if (GEM_WARN_ON(vma->iova != iova)) {
> + clear_iova(obj, aspace);
> + ret = -ENOSPC;
The (vma->iova != iova) means that vma is already set, but to a
different address. Is -ENOSPC really appropriate here? -EBUSY or -EINVAL
looks more natural to me.
> + }
> + }
> + msm_gem_unlock(obj);
> +
> + return ret;
> +}
> +
> /*
> * Unpin a iova by updating the reference counts. The memory isn't actually
> * purged until something else (shrinker, mm_notifier, destroy, etc) decides
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 38d66e1248b1..efa2e5c19f1e 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -38,6 +38,12 @@ struct msm_gem_address_space {
>
> /* @faults: the number of GPU hangs associated with this address space
> */
> int faults;
> +
> + /** @va_start: lowest possible address to allocate */
> + uint64_t va_start;
> +
> + /** @va_size: the size of the address space (in bytes) */
> + uint64_t va_size;
> };
>
> struct msm_gem_address_space *
> @@ -144,6 +150,8 @@ struct msm_gem_vma *msm_gem_get_vma_locked(struct
> drm_gem_object *obj,
> struct msm_gem_address_space
> *aspace);
> int msm_gem_get_iova(struct drm_gem_object *obj,
> struct msm_gem_address_space *aspace, uint64_t *iova);
> +int msm_gem_set_iova(struct drm_gem_object *obj,
> + struct msm_gem_address_space *aspace, uint64_t iova);
> int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj,
> struct msm_gem_address_space *aspace, uint64_t *iova,
> u64 range_start, u64 range_end);
nit: There is an odd mix of uint64_t and u64 (and alike) in the MSM code
:) The uint64_t variant shouldn't be used by kernel code in general and
checkpatch should want about it.
Re: [Freedreno] [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API
Hi Liviu
Thanks for the response.
Let me fix up and spin a new version.
Abhinav
On 3/31/2022 3:01 AM, Liviu Dudau wrote:
On Fri, Mar 25, 2022 at 09:31:35AM -0700, Abhinav Kumar wrote:
Hi Liviu
Hi Abhinav,
Sorry for the delay, got busy with other things at the beginning of the week.
On 3/25/2022 3:19 AM, Liviu Dudau wrote:
On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:
Hi Liviu
Hello,
Thanks for the response.
On 3/24/2022 3:12 AM, Liviu Dudau wrote:
On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
Hi Liviu
Hello,
Thanks for the review.
On 3/23/2022 9:46 AM, Liviu Dudau wrote:
On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
For vendors drivers which pass an already allocated and
initialized encoder especially for cases where the encoder
hardware is shared OR the writeback encoder shares the resources
with the rest of the display pipeline introduce a new API,
drm_writeback_connector_init_with_encoder() which expects
an initialized encoder as a parameter and only sets up the
writeback connector.
changes in v5:
- reorder this change to come before in the series
to avoid incorrect functionality in subsequent changes
- continue using struct drm_encoder instead of
struct drm_encoder * and switch it in next change
Signed-off-by: Abhinav Kumar
Hi Abhinav,
---
drivers/gpu/drm/drm_writeback.c | 143
include/drm/drm_writeback.h | 5 ++
2 files changed, 106 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dc2ef12..abe78b9 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -149,37 +149,15 @@ static const struct drm_encoder_funcs
drm_writeback_encoder_funcs = {
.destroy = drm_encoder_cleanup,
};
-/**
- * drm_writeback_connector_init - Initialize a writeback connector and its
properties
- * @dev: DRM device
- * @wb_connector: Writeback connector to initialize
- * @con_funcs: Connector funcs vtable
- * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal
encoder
- * @formats: Array of supported pixel formats for the writeback engine
- * @n_formats: Length of the formats array
- * @possible_crtcs: possible crtcs for the internal writeback encoder
- *
- * This function creates the writeback-connector-specific properties if they
- * have not been already created, initializes the connector as
- * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
- * values. It will also create an internal encoder associated with the
- * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
- * the encoder helper.
- *
- * Drivers should always use this function instead of drm_connector_init() to
- * set up writeback connectors.
- *
- * Returns: 0 on success, or a negative error code
- */
-int drm_writeback_connector_init(struct drm_device *dev,
-struct drm_writeback_connector *wb_connector,
-const struct drm_connector_funcs *con_funcs,
-const struct drm_encoder_helper_funcs
*enc_helper_funcs,
-const u32 *formats, int n_formats, uint32_t
possible_crtcs)
+static int drm_writeback_connector_setup(struct drm_device *dev,
+ struct drm_writeback_connector *wb_connector,
+ const struct drm_connector_funcs *con_funcs, const u32 *formats,
+ int n_formats)
{
struct drm_property_blob *blob;
- struct drm_connector *connector = _connector->base;
struct drm_mode_config *config = >mode_config;
+ struct drm_connector *connector = _connector->base;
+
Point of this reordering the statements is...?
This diff can be avoided. There was no reason to reorder this. I will remove
this re-order.
int ret = create_writeback_properties(dev);
if (ret != 0)
@@ -187,18 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev,
blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
formats);
- if (IS_ERR(blob))
- return PTR_ERR(blob);
-
- drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
-
- wb_connector->encoder.possible_crtcs = possible_crtcs;
-
- ret = drm_encoder_init(dev, _connector->encoder,
- _writeback_encoder_funcs,
- DRM_MODE_ENCODER_VIRTUAL, NULL);
- if (ret)
- goto fail;
+ if (IS_ERR(blob)) {
+ ret = PTR_ERR(blob);
+ return ret;
+ }
I don't see why you have changed the earlier code to store the result of
PTR_ERR into
ret other than to help your debugging. I suggest that you keep the existing
code that
returns PTR_ERR(blob) directly and you will
Re: [Freedreno] [PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin
On 3/31/22 21:27, Dmitry Osipenko wrote:
> On 3/30/22 23:47, Rob Clark wrote:
>> From: Rob Clark
>>
>> Combines duplicate vma lookup in the get_and_pin path.
>>
>> Signed-off-by: Rob Clark
>> ---
>> drivers/gpu/drm/msm/msm_gem.c | 50 ++-
>> 1 file changed, 26 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
>> index deafae6feaa8..218744a490a4 100644
>> --- a/drivers/gpu/drm/msm/msm_gem.c
>> +++ b/drivers/gpu/drm/msm/msm_gem.c
>> @@ -376,39 +376,40 @@ put_iova_vmas(struct drm_gem_object *obj)
>> }
>> }
>>
>> -static int get_iova_locked(struct drm_gem_object *obj,
>> -struct msm_gem_address_space *aspace, uint64_t *iova,
>> +static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj,
>> +struct msm_gem_address_space *aspace,
>> u64 range_start, u64 range_end)
>> {
>> struct msm_gem_vma *vma;
>> -int ret = 0;
>>
>> GEM_WARN_ON(!msm_gem_is_locked(obj));
>>
>> vma = lookup_vma(obj, aspace);
>>
>> if (!vma) {
>> +int ret;
>> +
>> vma = add_vma(obj, aspace);
>> if (IS_ERR(vma))
>> -return PTR_ERR(vma);
>> +return vma;
>>
>> ret = msm_gem_init_vma(aspace, vma, obj->size,
>> range_start, range_end);
>> if (ret) {
> You're allocation range_start -> range_end
*allocating
>
>> del_vma(vma);
>> -return ret;
>> +return ERR_PTR(ret);
>> }
>> +} else {
>> +GEM_WARN_ON(vma->iova < range_start);
>> +GEM_WARN_ON((vma->iova + obj->size) > range_end);
>
> and then comparing range_start -> range_start + obj->size, hence you're
> assuming that range_end always equals to obj->size during the allocation.
>
> I'm not sure what is the idea here.. this looks inconsistent. I think
> you wanted to write:
>
> GEM_WARN_ON(vma->iova < range_start);
> GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) >
> range_end);
>
> But is it really useful to check whether the new range is inside of the
> old range? Shouldn't it be always a error to change the IOVA range
> without reallocating vma?
>
> I'd expect to see:
>
> GEM_WARN_ON(vma->iova != range_start);
> GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) !=
> range_end);
>
> and then error out if range mismatches.
Re: [Freedreno] [PATCH v2 07/10] drm/msm/gem: Rework vma lookup and pin
On 3/30/22 23:47, Rob Clark wrote:
> From: Rob Clark
>
> Combines duplicate vma lookup in the get_and_pin path.
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/msm_gem.c | 50 ++-
> 1 file changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index deafae6feaa8..218744a490a4 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -376,39 +376,40 @@ put_iova_vmas(struct drm_gem_object *obj)
> }
> }
>
> -static int get_iova_locked(struct drm_gem_object *obj,
> - struct msm_gem_address_space *aspace, uint64_t *iova,
> +static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj,
> + struct msm_gem_address_space *aspace,
> u64 range_start, u64 range_end)
> {
> struct msm_gem_vma *vma;
> - int ret = 0;
>
> GEM_WARN_ON(!msm_gem_is_locked(obj));
>
> vma = lookup_vma(obj, aspace);
>
> if (!vma) {
> + int ret;
> +
> vma = add_vma(obj, aspace);
> if (IS_ERR(vma))
> - return PTR_ERR(vma);
> + return vma;
>
> ret = msm_gem_init_vma(aspace, vma, obj->size,
> range_start, range_end);
> if (ret) {
You're allocation range_start -> range_end
> del_vma(vma);
> - return ret;
> + return ERR_PTR(ret);
> }
> + } else {
> + GEM_WARN_ON(vma->iova < range_start);
> + GEM_WARN_ON((vma->iova + obj->size) > range_end);
and then comparing range_start -> range_start + obj->size, hence you're
assuming that range_end always equals to obj->size during the allocation.
I'm not sure what is the idea here.. this looks inconsistent. I think
you wanted to write:
GEM_WARN_ON(vma->iova < range_start);
GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) >
range_end);
But is it really useful to check whether the new range is inside of the
old range? Shouldn't it be always a error to change the IOVA range
without reallocating vma?
I'd expect to see:
GEM_WARN_ON(vma->iova != range_start);
GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) !=
range_end);
and then error out if range mismatches.
Re: [Freedreno] [PATCH] dt-bindings: display: msm: dsi: remove address/size cells
On Thu, Mar 31, 2022 at 4:35 AM Dmitry Baryshkov
wrote:
>
> On Thu, 31 Mar 2022 at 09:05, Vinod Koul wrote:
> >
> > On 29-03-22, 10:52, Rob Herring wrote:
> > > On Tue, Mar 29, 2022 at 12:01:52PM +0530, Vinod Koul wrote:
> > > > On 28-03-22, 13:21, Rob Herring wrote:
> > > > > On Mon, Mar 28, 2022 at 12:18 PM Krzysztof Kozlowski
> > > > > wrote:
> > > > > >
> > > > > > On 28/03/2022 19:16, Vinod Koul wrote:
> > > > > > > On 28-03-22, 19:43, Dmitry Baryshkov wrote:
> > > > > > >> On Mon, 28 Mar 2022 at 18:30, Krzysztof Kozlowski
> > > > > > >> wrote:
> > > > > > >>>
> > > > > > >>> The DSI node is not a bus and the children do not have unit
> > > > > > >>> addresses.
> > > > > > >>>
> > > > > > >>> Reported-by: Vinod Koul
> > > > > > >>> Signed-off-by: Krzysztof Kozlowski
> > > > > > >>>
> > > > > > >>
> > > > > > >> NAK.
> > > > > > >> DSI panels are children of the DSI device tree node with the reg
> > > > > > >> = <0>; address.
> > > > > > >> This is the convention used by other platforms too (see e.g.
> > > > > > >> arch/arm64/boot/dts/freescale/imx8mq-evk.dts).
> > > > > > >
> > > > > > > So we should add reg = 0, i will update my dtsi fix
> > > > > > >
> > > > > >
> > > > > > To "ports" node? No. The reg=0 is for children of the bus, so the
> > > > > > panels. How to combine both without warnings - ports and panel@0 - I
> > > > > > don't know yet...
> > > > >
> > > > > I don't think that should case a warning. Or at least it's one we
> > > > > turn off.
> > > >
> > > > Well in this case I think we might need a fix:
> > > > Here is the example quoted in the binding. We have ports{} and then the
> > > > two port@0 and port@1 underneath.
> > >
> > > It's the #address-cells/#size-cells under 'ports' that applies to 'port'
> > > nodes. As 'ports' has no address (reg) itself, it doesn't need
> > > #address-cells/#size-cells in its parent node.
> > >
> > > >
> > > > So it should be okay to drop #address-cells/#size-cells from dsi node
> > > > but keep in ports node...
> > >
> > > Yes.
> > >
> > > > Thoughts...?
> > >
> > > But I thought a panel@0 node was being added? If so then you need to add
> > > them back.
> >
> > I guess we should make this optional, keep it when adding panel@0 node
> > and skip for rest where not applicable..? Dmitry is that fine with you?
>
> This sounds like a workaround. When a panel node is added together
> with the '#address-cells' / '#size-cells' properties, we will get
> warnings for the 'ports' node.
What warning exactly? Is that with W=1?
Some warnings are more "don't do this on new designs" rather than
never allowed and need to fix current bindings/dts. As such, these
warnings will probably never be enabled by default.
Rob
Re: [Freedreno] [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts
On Thu, 31 Mar 2022 at 14:05, Sankeerth Billakanti
wrote:
>
> Hi Dmitry,
>
> > On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote:
> > > Hi Dmitry,
> > >
> > >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
> > >> wrote:
> > >>>
> > >>> The interrupt register will still reflect the connect and disconnect
> > >>> interrupt status without generating an actual HW interrupt.
> > >>> The controller driver should not handle those masked interrupts.
> > >>>
> > >>> Signed-off-by: Sankeerth Billakanti
> > >>> ---
> > >>> drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
> > >>> 1 file changed, 3 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>> index 3c16f95..1809ce2 100644
> > >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > >>> @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct
> > >>> dp_catalog *dp_catalog) {
> > >>> struct dp_catalog_private *catalog = container_of(dp_catalog,
> > >>> struct dp_catalog_private, dp_catalog);
> > >>> - int isr = 0;
> > >>> + int isr, mask;
> > >>>
> > >>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> > >>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> > >>> (isr & DP_DP_HPD_INT_MASK));
> > >>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
> > >>>
> > >>> - return isr;
> > >>> + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask);
> > >>
> > >> I suspect that the logic is inverted here. Shouldn't it be:
> > >>
> > >> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
> > >>
> > >> ?
> > >>
> > >
> > > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE000 and the
> > value
> > > of the read interrupt mask variable could be is 0xF.
> > >
> > > The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK,
> > bits 3:0.
> > > The HPD status is indicated via a different read-only register
> > REG_DP_DP_HPD_INT_STATUS, bits 31:29.
> >
> > I see. Maybe the following expression would be better?
> >
> > return isr & (mask & ~DP_DP_HPD_INT_MASK);
Ugh, excuse me please. I meant:
return isr & (mask | ~DP_DP_HPD_INT_MASK);
> >
>
> I believe the confusion occurred because the DP_DP_HPD_STATE_STATUS_CONNECTED
> and others were defined under the same register definition as
> REG_DP_DP_HPD_INT_MASK
> I will rearrange the definitions below.
>
> #define REG_DP_DP_HPD_INT_MASK (0x000C)
> #define DP_DP_HPD_PLUG_INT_MASK (0x0001)
> #define DP_DP_IRQ_HPD_INT_MASK (0x0002)
> #define DP_DP_HPD_REPLUG_INT_MASK (0x0004)
> #define DP_DP_HPD_UNPLUG_INT_MASK (0x0008)
> #define DP_DP_HPD_INT_MASK (DP_DP_HPD_PLUG_INT_MASK | \
> DP_DP_IRQ_HPD_INT_MASK | \
> DP_DP_HPD_REPLUG_INT_MASK | \
> DP_DP_HPD_UNPLUG_INT_MASK)
>
> Below are status bits from register REG_DP_DP_HPD_INT_STATUS
>
> #define DP_DP_HPD_STATE_STATUS_CONNECTED(0x4000)
> #define DP_DP_HPD_STATE_STATUS_PENDING (0x2000)
> #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x)
> #define DP_DP_HPD_STATE_STATUS_MASK (0xE000)
>
> DP_DP_HPD_INT_MASK is 0xF and scope of mask variable is also 0xF (bits 3:0),
> mask & ~DP_DP_HPD_INT_MASK is 0 always.
>
> For DP, we want to enable all interrupts.
> So the programmed mask value is 0xF. We want to return 0x4001 for connect
> and 8 for disconnect
>
> For eDP, we want to disable the connect and disconnect interrupts. So, the
> mask will be 0x6 (i.e. DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK)
> We want to return 0x4000 (or 0x2000 based on hpd line status) and 0
> for eDP connect and disconnect respectively.
>
> > >
> > > isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.
> > >
> > >>> }
> > >>>
> > >>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
> > >>> --
> > >>> 2.7.4
> > >>>
> > >>
> > >>
> > >> --
> > >> With best wishes
> > >> Dmitry
> > >
> > > Thank you,
> > > Sankeerth
> >
> >
> > --
> > With best wishes
> > Dmitry
>
> Thank you,
> Sankeerth
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts
Hi Dmitry,
> On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote:
> > Hi Dmitry,
> >
> >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
> >> wrote:
> >>>
> >>> The interrupt register will still reflect the connect and disconnect
> >>> interrupt status without generating an actual HW interrupt.
> >>> The controller driver should not handle those masked interrupts.
> >>>
> >>> Signed-off-by: Sankeerth Billakanti
> >>> ---
> >>> drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
> >>> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>> index 3c16f95..1809ce2 100644
> >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>> @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct
> >>> dp_catalog *dp_catalog) {
> >>> struct dp_catalog_private *catalog = container_of(dp_catalog,
> >>> struct dp_catalog_private, dp_catalog);
> >>> - int isr = 0;
> >>> + int isr, mask;
> >>>
> >>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> >>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> >>> (isr & DP_DP_HPD_INT_MASK));
> >>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
> >>>
> >>> - return isr;
> >>> + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask);
> >>
> >> I suspect that the logic is inverted here. Shouldn't it be:
> >>
> >> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
> >>
> >> ?
> >>
> >
> > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE000 and the
> value
> > of the read interrupt mask variable could be is 0xF.
> >
> > The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK,
> bits 3:0.
> > The HPD status is indicated via a different read-only register
> REG_DP_DP_HPD_INT_STATUS, bits 31:29.
>
> I see. Maybe the following expression would be better?
>
> return isr & (mask & ~DP_DP_HPD_INT_MASK);
>
I believe the confusion occurred because the DP_DP_HPD_STATE_STATUS_CONNECTED
and others were defined under the same register definition as
REG_DP_DP_HPD_INT_MASK
I will rearrange the definitions below.
#define REG_DP_DP_HPD_INT_MASK (0x000C)
#define DP_DP_HPD_PLUG_INT_MASK (0x0001)
#define DP_DP_IRQ_HPD_INT_MASK (0x0002)
#define DP_DP_HPD_REPLUG_INT_MASK (0x0004)
#define DP_DP_HPD_UNPLUG_INT_MASK (0x0008)
#define DP_DP_HPD_INT_MASK (DP_DP_HPD_PLUG_INT_MASK | \
DP_DP_IRQ_HPD_INT_MASK | \
DP_DP_HPD_REPLUG_INT_MASK | \
DP_DP_HPD_UNPLUG_INT_MASK)
Below are status bits from register REG_DP_DP_HPD_INT_STATUS
#define DP_DP_HPD_STATE_STATUS_CONNECTED(0x4000)
#define DP_DP_HPD_STATE_STATUS_PENDING (0x2000)
#define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x)
#define DP_DP_HPD_STATE_STATUS_MASK (0xE000)
DP_DP_HPD_INT_MASK is 0xF and scope of mask variable is also 0xF (bits 3:0),
mask & ~DP_DP_HPD_INT_MASK is 0 always.
For DP, we want to enable all interrupts.
So the programmed mask value is 0xF. We want to return 0x4001 for connect
and 8 for disconnect
For eDP, we want to disable the connect and disconnect interrupts. So, the mask
will be 0x6 (i.e. DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK)
We want to return 0x4000 (or 0x2000 based on hpd line status) and 0 for
eDP connect and disconnect respectively.
> >
> > isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.
> >
> >>> }
> >>>
> >>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
> >>> --
> >>> 2.7.4
> >>>
> >>
> >>
> >> --
> >> With best wishes
> >> Dmitry
> >
> > Thank you,
> > Sankeerth
>
>
> --
> With best wishes
> Dmitry
Thank you,
Sankeerth
Re: [Freedreno] [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts
On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote:
Hi Dmitry,
On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
wrote:
The interrupt register will still reflect the connect and disconnect
interrupt status without generating an actual HW interrupt.
The controller driver should not handle those masked interrupts.
Signed-off-by: Sankeerth Billakanti
---
drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 3c16f95..1809ce2 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct
dp_catalog *dp_catalog) {
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
- int isr = 0;
+ int isr, mask;
isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
(isr & DP_DP_HPD_INT_MASK));
+ mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
- return isr;
+ return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask);
I suspect that the logic is inverted here. Shouldn't it be:
return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
?
The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE000 and the value of the read
interrupt mask variable could be is 0xF.
The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK, bits 3:0.
The HPD status is indicated via a different read-only register
REG_DP_DP_HPD_INT_STATUS, bits 31:29.
I see. Maybe the following expression would be better?
return isr & (mask & ~DP_DP_HPD_INT_MASK);
isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.
}
int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
--
2.7.4
--
With best wishes
Dmitry
Thank you,
Sankeerth
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API
On Fri, Mar 25, 2022 at 09:31:35AM -0700, Abhinav Kumar wrote:
> Hi Liviu
Hi Abhinav,
Sorry for the delay, got busy with other things at the beginning of the week.
>
> On 3/25/2022 3:19 AM, Liviu Dudau wrote:
> > On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:
> > > Hi Liviu
> >
> > Hello,
> >
> > >
> > > Thanks for the response.
> > >
> > > On 3/24/2022 3:12 AM, Liviu Dudau wrote:
> > > > On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
> > > > > Hi Liviu
> > > >
> > > > Hello,
> > > >
> > > > >
> > > > > Thanks for the review.
> > > > >
> > > > > On 3/23/2022 9:46 AM, Liviu Dudau wrote:
> > > > > > On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
> > > > > > > For vendors drivers which pass an already allocated and
> > > > > > > initialized encoder especially for cases where the encoder
> > > > > > > hardware is shared OR the writeback encoder shares the resources
> > > > > > > with the rest of the display pipeline introduce a new API,
> > > > > > > drm_writeback_connector_init_with_encoder() which expects
> > > > > > > an initialized encoder as a parameter and only sets up the
> > > > > > > writeback connector.
> > > > > > >
> > > > > > > changes in v5:
> > > > > > > - reorder this change to come before in the series
> > > > > > > to avoid incorrect functionality in subsequent changes
> > > > > > > - continue using struct drm_encoder instead of
> > > > > > > struct drm_encoder * and switch it in next change
> > > > > > >
> > > > > > > Signed-off-by: Abhinav Kumar
> > > > > >
> > > > > > Hi Abhinav,
> > > > > >
> > > > > > > ---
> > > > > > > drivers/gpu/drm/drm_writeback.c | 143
> > > > > > >
> > > > > > > include/drm/drm_writeback.h | 5 ++
> > > > > > > 2 files changed, 106 insertions(+), 42 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c
> > > > > > > b/drivers/gpu/drm/drm_writeback.c
> > > > > > > index dc2ef12..abe78b9 100644
> > > > > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > > > > @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs
> > > > > > > drm_writeback_encoder_funcs = {
> > > > > > > .destroy = drm_encoder_cleanup,
> > > > > > > };
> > > > > > > -/**
> > > > > > > - * drm_writeback_connector_init - Initialize a writeback
> > > > > > > connector and its properties
> > > > > > > - * @dev: DRM device
> > > > > > > - * @wb_connector: Writeback connector to initialize
> > > > > > > - * @con_funcs: Connector funcs vtable
> > > > > > > - * @enc_helper_funcs: Encoder helper funcs vtable to be used by
> > > > > > > the internal encoder
> > > > > > > - * @formats: Array of supported pixel formats for the writeback
> > > > > > > engine
> > > > > > > - * @n_formats: Length of the formats array
> > > > > > > - * @possible_crtcs: possible crtcs for the internal writeback
> > > > > > > encoder
> > > > > > > - *
> > > > > > > - * This function creates the writeback-connector-specific
> > > > > > > properties if they
> > > > > > > - * have not been already created, initializes the connector as
> > > > > > > - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes
> > > > > > > the property
> > > > > > > - * values. It will also create an internal encoder associated
> > > > > > > with the
> > > > > > > - * drm_writeback_connector and set it to use the
> > > > > > > @enc_helper_funcs vtable for
> > > > > > > - * the encoder helper.
> > > > > > > - *
> > > > > > > - * Drivers should always use this function instead of
> > > > > > > drm_connector_init() to
> > > > > > > - * set up writeback connectors.
> > > > > > > - *
> > > > > > > - * Returns: 0 on success, or a negative error code
> > > > > > > - */
> > > > > > > -int drm_writeback_connector_init(struct drm_device *dev,
> > > > > > > - struct drm_writeback_connector
> > > > > > > *wb_connector,
> > > > > > > - const struct drm_connector_funcs
> > > > > > > *con_funcs,
> > > > > > > - const struct drm_encoder_helper_funcs
> > > > > > > *enc_helper_funcs,
> > > > > > > - const u32 *formats, int n_formats,
> > > > > > > uint32_t possible_crtcs)
> > > > > > > +static int drm_writeback_connector_setup(struct drm_device *dev,
> > > > > > > + struct drm_writeback_connector *wb_connector,
> > > > > > > + const struct drm_connector_funcs *con_funcs, const u32
> > > > > > > *formats,
> > > > > > > + int n_formats)
> > > > > > > {
> > > > > > > struct drm_property_blob *blob;
> > > > > > > - struct drm_connector *connector = _connector->base;
> > > > > > > struct drm_mode_config *config = >mode_config;
> > > > > > > + struct drm_connector *connector = _connector->base;
> > > > > > > +
> > > > > >
> > > > > > Point of this reordering the statements
Re: [Freedreno] [PATCH] dt-bindings: display: msm: dsi: remove address/size cells
On Thu, 31 Mar 2022 at 09:05, Vinod Koul wrote:
>
> On 29-03-22, 10:52, Rob Herring wrote:
> > On Tue, Mar 29, 2022 at 12:01:52PM +0530, Vinod Koul wrote:
> > > On 28-03-22, 13:21, Rob Herring wrote:
> > > > On Mon, Mar 28, 2022 at 12:18 PM Krzysztof Kozlowski
> > > > wrote:
> > > > >
> > > > > On 28/03/2022 19:16, Vinod Koul wrote:
> > > > > > On 28-03-22, 19:43, Dmitry Baryshkov wrote:
> > > > > >> On Mon, 28 Mar 2022 at 18:30, Krzysztof Kozlowski
> > > > > >> wrote:
> > > > > >>>
> > > > > >>> The DSI node is not a bus and the children do not have unit
> > > > > >>> addresses.
> > > > > >>>
> > > > > >>> Reported-by: Vinod Koul
> > > > > >>> Signed-off-by: Krzysztof Kozlowski
> > > > > >>>
> > > > > >>
> > > > > >> NAK.
> > > > > >> DSI panels are children of the DSI device tree node with the reg =
> > > > > >> <0>; address.
> > > > > >> This is the convention used by other platforms too (see e.g.
> > > > > >> arch/arm64/boot/dts/freescale/imx8mq-evk.dts).
> > > > > >
> > > > > > So we should add reg = 0, i will update my dtsi fix
> > > > > >
> > > > >
> > > > > To "ports" node? No. The reg=0 is for children of the bus, so the
> > > > > panels. How to combine both without warnings - ports and panel@0 - I
> > > > > don't know yet...
> > > >
> > > > I don't think that should case a warning. Or at least it's one we turn
> > > > off.
> > >
> > > Well in this case I think we might need a fix:
> > > Here is the example quoted in the binding. We have ports{} and then the
> > > two port@0 and port@1 underneath.
> >
> > It's the #address-cells/#size-cells under 'ports' that applies to 'port'
> > nodes. As 'ports' has no address (reg) itself, it doesn't need
> > #address-cells/#size-cells in its parent node.
> >
> > >
> > > So it should be okay to drop #address-cells/#size-cells from dsi node
> > > but keep in ports node...
> >
> > Yes.
> >
> > > Thoughts...?
> >
> > But I thought a panel@0 node was being added? If so then you need to add
> > them back.
>
> I guess we should make this optional, keep it when adding panel@0 node
> and skip for rest where not applicable..? Dmitry is that fine with you?
This sounds like a workaround. When a panel node is added together
with the '#address-cells' / '#size-cells' properties, we will get
warnings for the 'ports' node.
I'd prefer to leave things to pinpoint that the problem is generic
rather than being specific to several device trees with the DSI panel
nodes.
How do other platforms solve the issue?
In fact we can try shifting to the following dts schema:
dsi@ae94 {
compatible = "qcom,mdss-dsi-ctrl";
ports {
#adress-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
dsi0_in: endpoint {};
};
port@1 {
reg = <1>;
dsi0_out: endpoint {
remote-endpoint = <_in>;
};
};
/* dsi-bus is a generic part */
dsi-bus {
#adress-cells = <1>;
#size-cells = <0>;
/* panel@0 goes to the board file */
panel@0 {
compatible = "vendor,some-panel";
ports {
#adress-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
panel_in: endpoint {
remote-endpoint = <_out>;
};
};
};
};
};
WDYT?
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] dt-bindings: display: msm: dsi: remove address/size cells
On 29-03-22, 10:52, Rob Herring wrote:
> On Tue, Mar 29, 2022 at 12:01:52PM +0530, Vinod Koul wrote:
> > On 28-03-22, 13:21, Rob Herring wrote:
> > > On Mon, Mar 28, 2022 at 12:18 PM Krzysztof Kozlowski
> > > wrote:
> > > >
> > > > On 28/03/2022 19:16, Vinod Koul wrote:
> > > > > On 28-03-22, 19:43, Dmitry Baryshkov wrote:
> > > > >> On Mon, 28 Mar 2022 at 18:30, Krzysztof Kozlowski
> > > > >> wrote:
> > > > >>>
> > > > >>> The DSI node is not a bus and the children do not have unit
> > > > >>> addresses.
> > > > >>>
> > > > >>> Reported-by: Vinod Koul
> > > > >>> Signed-off-by: Krzysztof Kozlowski
> > > > >>
> > > > >> NAK.
> > > > >> DSI panels are children of the DSI device tree node with the reg =
> > > > >> <0>; address.
> > > > >> This is the convention used by other platforms too (see e.g.
> > > > >> arch/arm64/boot/dts/freescale/imx8mq-evk.dts).
> > > > >
> > > > > So we should add reg = 0, i will update my dtsi fix
> > > > >
> > > >
> > > > To "ports" node? No. The reg=0 is for children of the bus, so the
> > > > panels. How to combine both without warnings - ports and panel@0 - I
> > > > don't know yet...
> > >
> > > I don't think that should case a warning. Or at least it's one we turn
> > > off.
> >
> > Well in this case I think we might need a fix:
> > Here is the example quoted in the binding. We have ports{} and then the
> > two port@0 and port@1 underneath.
>
> It's the #address-cells/#size-cells under 'ports' that applies to 'port'
> nodes. As 'ports' has no address (reg) itself, it doesn't need
> #address-cells/#size-cells in its parent node.
>
> >
> > So it should be okay to drop #address-cells/#size-cells from dsi node
> > but keep in ports node...
>
> Yes.
>
> > Thoughts...?
>
> But I thought a panel@0 node was being added? If so then you need to add
> them back.
I guess we should make this optional, keep it when adding panel@0 node
and skip for rest where not applicable..? Dmitry is that fine with you?
--
~Vinod
Re: [Freedreno] [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp
Hi Dmitry,
> On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
> wrote:
> >
> > The panel-edp driver modes needs to be validated differently from DP
> > because the link capabilities are not available for EDP by that time.
> >
> > Signed-off-by: Sankeerth Billakanti
>
> This should not be necessary after
> https://patchwork.freedesktop.org/patch/479261/?series=101682=1.
> Could you please check?
>
The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore but we need
to return early for eDP because unlike DP, eDP context will not have the
information
about the number of lanes and link clock.
So, I will modify the patch to return after the DP_MAX_PIXEL_CLK_KHZ check if
is_eDP is set.
> > ---
> > drivers/gpu/drm/msm/dp/dp_display.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 8bafdd0..f9c7d9a 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -1003,6 +1003,12 @@ enum drm_mode_status
> dp_bridge_mode_valid(struct drm_bridge *bridge,
> > return -EINVAL;
> > }
> >
> > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) {
> > + if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ)
> > + return MODE_CLOCK_HIGH;
> > + return MODE_OK;
> > + }
> > +
> > if ((dp->max_pclk_khz <= 0) ||
> > (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
> > (mode->clock > dp->max_pclk_khz))
> > --
> > 2.7.4
> >
>
>
> --
> With best wishes
> Dmitry
