[Freedreno] [PATCH v4 4/4] drm/vc4: change vc4 driver to use drm_writeback_connector_init_with_encoder()

[Abhinav Kumar]

vc4 driver currently embeds the drm_encoder into struct vc4_txp
and later on uses container_of to retrieve the vc4_txp from
the drm_encoder.

Since drm_encoder has now been made a pointer inside
drm_writeback_connector, make vc4 driver use the new API
so that the embedded encoder model can be retained in the
driver and there is no change in functionality.

changes in v4:
- none

Signed-off-by: Abhinav Kumar 
---
 drivers/gpu/drm/vc4/vc4_txp.c | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 341a9be5..3d24ef5 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -370,6 +370,10 @@ static const struct drm_encoder_helper_funcs 
vc4_txp_encoder_helper_funcs = {
.disable = vc4_txp_encoder_disable,
 };
 
+static const struct drm_encoder_funcs vc4_txp_encoder_funcs = {
+   .destroy = drm_encoder_cleanup,
+};
+
 static int vc4_txp_enable_vblank(struct drm_crtc *crtc)
 {
return 0;
@@ -498,15 +502,24 @@ static int vc4_txp_bind(struct device *dev, struct device 
*master, void *data)
wb_conn = >connector;
wb_conn->encoder = >drm_enc;
 
+   drm_encoder_helper_add(wb_conn->encoder, _txp_encoder_helper_funcs);
+
+   ret = drm_encoder_init(drm, wb_conn->encoder,
+   _txp_encoder_funcs,
+   DRM_MODE_ENCODER_VIRTUAL, NULL);
+
+   if (ret)
+   return ret;
+
drm_connector_helper_add(_conn->base,
 _txp_connector_helper_funcs);
-   ret = drm_writeback_connector_init(drm, wb_conn,
-  _txp_connector_funcs,
-  _txp_encoder_helper_funcs,
-  drm_fmts, ARRAY_SIZE(drm_fmts),
-  0);
-   if (ret)
+
+   ret = drm_writeback_connector_init_with_encoder(drm, wb_conn,
+   _txp_connector_funcs, drm_fmts, 
ARRAY_SIZE(drm_fmts));
+   if (ret) {
+   drm_encoder_cleanup(wb_conn->encoder);
return ret;
+   }
 
ret = vc4_crtc_init(drm, vc4_crtc,
_txp_crtc_funcs, _txp_crtc_helper_funcs);
-- 
2.7.4

[Freedreno] [PATCH v4 3/4] drm: introduce drm_writeback_connector_init_with_encoder() API

[Abhinav Kumar]

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 v4:
- removed the possible_crtcs part

Signed-off-by: Abhinav Kumar 
---
 drivers/gpu/drm/drm_writeback.c | 147 
 include/drm/drm_writeback.h |   5 ++
 2 files changed, 108 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index a4c17d6..d0672f5 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;
+
int ret = create_writeback_properties(dev);
 
if (ret != 0)
@@ -187,20 +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(wb_connector->encoder, enc_helper_funcs);
-
-   wb_connector->encoder = _connector->internal_encoder;
-
-   wb_connector->encoder->possible_crtcs = possible_crtcs;
-
-   ret = drm_encoder_init(dev, wb_connector->encoder,
-  _writeback_encoder_funcs,
-  DRM_MODE_ENCODER_VIRTUAL, NULL);
-   if (ret)
-   goto fail;
+   if (IS_ERR(blob)) {
+   ret = PTR_ERR(blob);
+   return ret;
+   }
 
connector->interlace_allowed = 0;
 
@@ -239,13 +207,104 @@ int drm_writeback_connector_init(struct drm_device *dev,
 attach_fail:
drm_connector_cleanup(connector);
 connector_fail:
-   drm_encoder_cleanup(wb_connector->encoder);
-fail:
drm_property_blob_put(blob);
return ret;
 }
+
+/**
+ * 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 

[Freedreno] [PATCH v4 2/4] drm: allow real encoder to be passed for drm_writeback_connector

[Abhinav Kumar]

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 v4:
- split the possible_crtcs change and the parts which should
  belong to the addition of new API to the next change

Co-developed-by: Kandpal Suraj 
Signed-off-by: Abhinav Kumar 
---
 drivers/gpu/drm/drm_writeback.c | 12 +++-
 drivers/gpu/drm/vc4/vc4_txp.c   | 14 ++
 include/drm/drm_writeback.h | 18 --
 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dc2ef12..a4c17d6 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -190,11 +190,13 @@ 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);
+   drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
 
-   wb_connector->encoder.possible_crtcs = possible_crtcs;
+   wb_connector->encoder = _connector->internal_encoder;
 
-   ret = drm_encoder_init(dev, _connector->encoder,
+   wb_connector->encoder->possible_crtcs = possible_crtcs;
+
+   ret = drm_encoder_init(dev, wb_connector->encoder,
   _writeback_encoder_funcs,
   DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
@@ -208,7 +210,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
goto connector_fail;
 
ret = drm_connector_attach_encoder(connector,
-   _connector->encoder);
+   wb_connector->encoder);
if (ret)
goto attach_fail;
 
@@ -237,7 +239,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
 attach_fail:
drm_connector_cleanup(connector);
 connector_fail:
-   drm_encoder_cleanup(_connector->encoder);
+   drm_encoder_cleanup(wb_connector->encoder);
 fail:
drm_property_blob_put(blob);
return ret;
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 3447eb6..341a9be5 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)
@@ -467,6 +469,7 @@ static int vc4_txp_bind(struct device *dev, struct device 
*master, void *data)
struct vc4_txp *txp;
struct drm_crtc *crtc;
struct drm_encoder *encoder;
+   struct drm_writeback_connector *wb_conn;
int ret, irq;
 
irq = platform_get_irq(pdev, 0);
@@ -492,9 +495,12 @@ static int vc4_txp_bind(struct device *dev, struct device 
*master, void *data)
txp->regset.regs = txp_regs;
txp->regset.nregs = ARRAY_SIZE(txp_regs);
 
-   drm_connector_helper_add(>connector.base,
+   wb_conn = >connector;
+   wb_conn->encoder = >drm_enc;
+
+   drm_connector_helper_add(_conn->base,
 _txp_connector_helper_funcs);
-   ret = drm_writeback_connector_init(drm, >connector,
+   ret = drm_writeback_connector_init(drm, wb_conn,
   _txp_connector_funcs,
   _txp_encoder_helper_funcs,
   drm_fmts, ARRAY_SIZE(drm_fmts),
@@ -507,7 +513,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 db6214f..c525b60 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -25,15 +25,29 @@ struct drm_writeback_connector {
struct drm_connector base;
 
/**
-* @encoder: Internal encoder used by the connector to fulfill
+* @encoder: handle to drm_encoder used by the connector to fulfill
 * the DRM framework requirements. The users of the
 

[Freedreno] [PATCH v4 1/4] drm: allow passing possible_crtcs to drm_writeback_connector_init()

[Abhinav Kumar]

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 v4:
 - keep only changes related to possible_crtcs
 - add line breaks after ARRAY_SIZE
 - stop using temporary variables for possible_crtcs

Signed-off-by: Abhinav Kumar 
---
 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, 

[Freedreno] [PATCH v4 0/4] Allow drm_writeback_connector to accept pointer to drm_encoder

[Abhinav Kumar]

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 v4:
-  split the changes more according to functionality

Abhinav Kumar (4):
  drm: allow passing possible_crtcs to drm_writeback_connector_init()
  drm: allow real encoder to be passed for drm_writeback_connector
  drm: introduce drm_writeback_connector_init_with_encoder() API
  drm/vc4: change vc4 driver to use
drm_writeback_connector_init_with_encoder()

 .../drm/arm/display/komeda/komeda_wb_connector.c   |   3 +-
 drivers/gpu/drm/arm/malidp_mw.c|   4 +-
 drivers/gpu/drm/drm_writeback.c| 143 +++--
 drivers/gpu/drm/rcar-du/rcar_du_writeback.c|   4 +-
 drivers/gpu/drm/vc4/vc4_txp.c  |  36 --
 drivers/gpu/drm/vkms/vkms_writeback.c  |   4 +-
 include/drm/drm_writeback.h|  25 +++-
 7 files changed, 161 insertions(+), 58 deletions(-)

-- 
2.7.4

Re: [Freedreno] [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any sink interaction

[Stephen Boyd]

Quoting Sankeerth Billakanti (2022-03-16 10:35:51)
> The source device should ensure the sink is ready before
> proceeding to read the sink capability or performing any aux transactions.
> The sink will indicate its readiness by asserting the HPD line.
>
> The eDP sink requires 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.
>
> The controller driver needs to wait for the hpd line to be asserted
> by the sink.
>
> Signed-off-by: Sankeerth Billakanti 
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c |  6 ++
>  drivers/gpu/drm/msm/dp/dp_catalog.c | 23 +++
>  drivers/gpu/drm/msm/dp/dp_catalog.h |  1 +
>  drivers/gpu/drm/msm/dp/dp_reg.h |  7 ++-
>  4 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 6d36f63..2ddc303 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -337,6 +337,12 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> goto exit;
> }
>
> +   ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);

Why are we making aux transactions when hpd isn't asserted? Can we only
register the aux device once we know that state is "connected"? I'm
concerned that we're going to be possibly polling the connected bit up
to some amount of time (0x0003 usecs?) for each aux transfer when
that doesn't make any sense to keep checking. We should be able to check
it once, register aux, and then when disconnect happens we can
unregister aux.

> +   if (ret) {
> +   DRM_DEBUG_DP("DP sink not ready for aux transactions\n");
> +   goto exit;
> +   }
> +
> dp_aux_update_offset_and_segment(aux, msg);
> dp_aux_transfer_helper(aux, msg, true);
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c 
> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index fac815f..2c3b0f7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -242,6 +242,29 @@ void dp_catalog_aux_update_cfg(struct dp_catalog 
> *dp_catalog)
> phy_calibrate(phy);
>  }
>
> +int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
> +{
> +   u32 state, hpd_en, timeout;
> +   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) &
> +   DP_DP_HPD_CTRL_HPD_EN;

Use two lines

hpd_en = dp_read_aux();
hpd_en &= DP_DP_HPD_CTRL_HPD_EN;

> +
> +   /* no-hpd case */
> +   if (!hpd_en)
> +   return 0;
> +
> +   /* Poll for HPD connected status */
> +   timeout = dp_read_aux(catalog, REG_DP_DP_HPD_EVENT_TIME_0) &
> +   DP_HPD_CONNECT_TIME_MASK;
> +
> +   return readl_poll_timeout(catalog->io->dp_controller.aux.base +
> +   REG_DP_DP_HPD_INT_STATUS,
> +   state, state & 
> DP_DP_HPD_STATE_STATUS_CONNECTED,
> +   2000, timeout);
> +}
> +
>  static void dump_regs(void __iomem *base, int len)
>  {
> int i;

Re: [Freedreno] [PATCH v5 1/9] arm64: dts: qcom: sc7280: rename edp_out label to mdss_edp_out

[Doug Anderson]

Hi,

On Wed, Mar 16, 2022 at 10:36 AM Sankeerth Billakanti
 wrote:
>
> Rename the edp_out label in the sc7280 platform to mdss_edp_out
> so that the nodes related to mdss are all grouped together in
> the board specific files.
>
> Signed-off-by: Sankeerth Billakanti 
> ---
>
> Changes in v5:
>   - Change the order of patches
>   - Modify commit text
>
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH v6 5/5] arm64: dts: qcom: sm8250: remove assigned-clock-rate property for mdp clk

[Doug Anderson]

Hi,

On Mon, Mar 14, 2022 at 7:47 AM Vinod Polimera
 wrote:
>
> Drop the assigned clock rate property and vote on the mdp clock as per
> calculated value during the usecase.
>
> This patch is dependent on below patch
> https://patchwork.kernel.org/patch/12774067/
>
> Signed-off-by: Vinod Polimera 
> Reviewed-by: Stephen Boyd 
> ---
>  arch/arm64/boot/dts/qcom/sm8250.dtsi | 9 ++---
>  1 file changed, 2 insertions(+), 7 deletions(-)

Similar comments to patch #2 about the commit message, but otherwise:

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH v6 4/5] arm64: dts: qcom: sdm845: remove assigned-clock-rate property for mdp clk

[Doug Anderson]

Hi,

On Mon, Mar 14, 2022 at 7:47 AM Vinod Polimera
 wrote:
>
> Drop the assigned clock rate property and vote on the mdp clock as per
> calculated value during the usecase.
>
> This patch is dependent on below patch
> https://patchwork.kernel.org/patch/12774067/
>
> Signed-off-by: Vinod Polimera 
> Reviewed-by: Stephen Boyd 
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 ++---
>  1 file changed, 2 insertions(+), 7 deletions(-)

Similar comments to patch #2 about the commit message, but otherwise:

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH v6 3/5] arm64: dts: qcom: sm7180: remove assigned-clock-rate property for mdp clk

[Doug Anderson]

Hi,

On Mon, Mar 14, 2022 at 7:47 AM Vinod Polimera
 wrote:
>
> Drop the assigned clock rate property and vote on the mdp clock as per
> calculated value during the usecase.
>
> This patch is dependent on below patch
> https://patchwork.kernel.org/patch/12774067/
>
> Signed-off-by: Vinod Polimera 
> Reviewed-by: Stephen Boyd 
> ---
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 9 ++---
>  1 file changed, 2 insertions(+), 7 deletions(-)

Similar comments to patch #2 about the commit message, but otherwise:

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH v6 2/5] arm64: dts: qcom: sm7280: remove assigned-clock-rate property for mdp clk

[Doug Anderson]

Hi,

On Mon, Mar 14, 2022 at 7:47 AM Vinod Polimera
 wrote:
>
> Drop the assigned clock rate property and vote on the mdp clock as per
> calculated value during the usecase.
>
> This patch is dependent on below patch
> https://patchwork.kernel.org/patch/12774067/

Some nits on how you're referring to the dependent patch:

1. In the commit message it's really nice to also include the subject
line of the patch so someone looking at the commit after it lands can
more easily identify the patch you depend on.

2. It's better to use links that have the message ID in them. In the
past patchwork's magic IDs have been list.

So I'd write:

This patch is dependent on the patch ("drm/msm/disp/dpu1: set mdp clk
to the maximum frequency in opp table during probe") [1].

[1] 
https://lore.kernel.org/r/1647269217-14064-2-git-send-email-quic_vpoli...@quicinc.com/


> Signed-off-by: Vinod Polimera 
> Reviewed-by: Stephen Boyd 
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 9 ++---
>  1 file changed, 2 insertions(+), 7 deletions(-)

Reviewed-by: Douglas Anderson 

Re: [Freedreno] [PATCH v6 1/5] drm/msm/disp/dpu1: set mdp clk to the maximum frequency in opp table during probe

[Doug Anderson]

Hi,

On Mon, Mar 14, 2022 at 7:47 AM Vinod Polimera
 wrote:
>
> use max clock during probe/bind sequence from the opp table.
> The clock will be scaled down when framework sends an update.
>
> Fixes: 25fdd5933("drm/msm: Add SDM845 DPU support")

The "Fixes:" format is a little wrong. Should have more digits and a
space before the parenthesis. AKA:

Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")

> Signed-off-by: Vinod Polimera 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 
>  1 file changed, 8 insertions(+)

This looks good to me now other than the bad Fixes tag. I presume
you'll want to spin with the extra verbosity in the CL description
that Stephen asked for, though.

Reviewed-by: Douglas Anderson 

[Freedreno] [PATCH] drm/msm/dsi: Use connector directly in msm_dsi_manager_connector_init()

[Stephen Boyd]

The member 'msm_dsi->connector' isn't assigned until
msm_dsi_manager_connector_init() returns (see msm_dsi_modeset_init() and
how it assigns the return value). Therefore this pointer is going to be
NULL here. Let's use 'connector' which is what was intended.

Cc: Dmitry Baryshkov 
Cc: Sean Paul 
Fixes: 6d5e78406991 ("drm/msm/dsi: Move dsi panel init into modeset init path")
Signed-off-by: Stephen Boyd 
---

I don't know if this is superseeded by something else but I found this
while trying to use the connector from msm_dsi in this function.

 drivers/gpu/drm/msm/dsi/dsi_manager.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 0c1b7dde377c..9f6af0f0fe00 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -638,7 +638,7 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id)
return connector;
 
 fail:
-   connector->funcs->destroy(msm_dsi->connector);
+   connector->funcs->destroy(connector);
return ERR_PTR(ret);
 }
 

base-commit: 05afd57f4d34602a652fdaf58e0a2756b3c20fd4
-- 
https://chromeos.dev

Re: [Freedreno] [PATCH v5 4/9] drm/panel-edp: add LQ140M1JW46 edp panel entry

[Doug Anderson]

Hi,

On Wed, Mar 16, 2022 at 10:37 AM Sankeerth Billakanti
 wrote:
>
> Add panel identification entry for the sharp LQ140M1JW46 eDP panel
> with power sequencing delay information.
>
> Signed-off-by: Sankeerth Billakanti 
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Douglas Anderson 

This is trivial and going through a different tree than everything
else, so I'm just pushing it to drm-misc-next (which is setup to land
things without regard to the merge window) without sitting on it.

You can leave it out of future spins of this series.

9f493fd71d4b drm/panel-edp: add LQ140M1JW46 edp panel entry

[Freedreno] [RFC] drm/msm: Add a way for userspace to allocate GPU iova

[Rob Clark]

From: Rob Clark 

The motivation at this point is mainly native userspace mesa driver in a
VM guest.  The one remaining synchronous "hotpath" is buffer allocation,
because guest needs to wait to know the bo's iova before it can start
emitting cmdstream/state that references the new bo.  By allocating the
iova in the guest userspace, we no longer need to wait for a response
from the host, but can just rely on the allocation request being
processed before the cmdstream submission.  Allocation faulures (OoM,
etc) would just be treated as context-lost (ie. GL_GUILTY_CONTEXT_RESET)
or subsequent allocations (or readpix, etc) can raise GL_OUT_OF_MEMORY.

TODO bump uapi version, or combine w/ other changes that bump uapi
version

Signed-off-by: Rob Clark 
---
So, I was initially planning on adding some extra guard-rails, ie.
some userspace opt-in and preventing mixing of kernel and userspace
allocated iova.  Because in general mixing and matching userspace and
kernel allocated iova is not going to go over too well.

But the address-space is per drm_file, and I couldn't come up with
any scenario where, on a given drm device fd, we would be trying to
mix/match userspace doing kernel iova allocation vs userspace iova
allocation.

Ofc, now is a good time to prove me wrong ;-)

 drivers/gpu/drm/msm/msm_drv.c | 21 +
 drivers/gpu/drm/msm/msm_gem.c | 20 
 drivers/gpu/drm/msm/msm_gem.h |  2 ++
 include/uapi/drm/msm_drm.h|  1 +
 4 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index a5eed5738ac8..7394312cf075 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -719,6 +719,23 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev,
return msm_gem_get_iova(obj, ctx->aspace, iova);
 }
 
+static int msm_ioctl_gem_info_set_iova(struct drm_device *dev,
+   struct drm_file *file, struct drm_gem_object *obj,
+   uint64_t iova)
+{
+   struct msm_drm_private *priv = dev->dev_private;
+   struct msm_file_private *ctx = file->driver_priv;
+
+   if (!priv->gpu)
+   return -EINVAL;
+
+   /* Only supported if per-process address space is supported: */
+   if (priv->gpu->aspace == ctx->aspace)
+   return -EINVAL;
+
+   return msm_gem_set_iova(obj, ctx->aspace, iova);
+}
+
 static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
struct drm_file *file)
 {
@@ -733,6 +750,7 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void 
*data,
switch (args->info) {
case MSM_INFO_GET_OFFSET:
case MSM_INFO_GET_IOVA:
+   case MSM_INFO_SET_IOVA:
/* value returned as immediate, not pointer, so len==0: */
if (args->len)
return -EINVAL;
@@ -757,6 +775,9 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void 
*data,
case MSM_INFO_GET_IOVA:
ret = msm_ioctl_gem_info_iova(dev, file, obj, >value);
break;
+   case MSM_INFO_SET_IOVA:
+   ret = msm_ioctl_gem_info_set_iova(dev, file, obj, args->value);
+   break;
case MSM_INFO_SET_NAME:
/* length check should leave room for terminating null: */
if (args->len >= sizeof(msm_obj->name)) {
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index a4f61972667b..41ae8d9c8b3c 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -510,6 +510,26 @@ int msm_gem_get_iova(struct drm_gem_object *obj,
return ret;
 }
 
+/*
+ * 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
+ */
+int msm_gem_set_iova(struct drm_gem_object *obj,
+   struct msm_gem_address_space *aspace, uint64_t iova)
+{
+   int ret;
+   uint64_t assigned_iova;
+
+   msm_gem_lock(obj);
+   ret = get_iova_locked(obj, aspace, _iova,
+ iova >> PAGE_SHIFT,
+ (iova + obj->size) >> PAGE_SHIFT);
+   msm_gem_unlock(obj);
+
+   return ret;
+}
+
 /* get iova without taking a reference, used in places where you have
  * already done a 'msm_gem_get_and_pin_iova' or 'msm_gem_get_iova'
  */
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 58e11c282928..40d839f61d15 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -112,6 +112,8 @@ struct msm_gem_object {
 uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj);
 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 

Re: [Freedreno] [PATCH v5 5/9] drm/msm/dp: Add eDP support via aux_bus

[Stephen Boyd]

Quoting Sankeerth Billakanti (2022-03-16 10:35:50)
> This patch adds support for generic eDP sink through aux_bus.

Please unindent commit text paragraphs. This isn't a book.

> The eDP/DP controller driver should support aux transactions originating
> from the panel-edp driver and hence should be initialized and ready.
>
> The panel bridge supporting the panel should be ready before
> the bridge connector is initialized. The generic panel probe needs the
> controller resources to be enabled to support aux tractions originating

s/tractions/transactions/

> from it. So, the host_init and phy_init are moved to execute before the
> panel probe.
>
> The host_init has to return early if the core is already
> initialized so that the regulator and clock votes for the controller
> resources are balanced.
>
> EV_HPD_INIT_SETUP needs to execute immediately to enable the
> interrupts for the aux transactions from panel-edp to get the modes
> supported.

There are a lot of things going on in this patch. Can it be split up?

>
> Signed-off-by: Sankeerth Billakanti 
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 65 
> +++--
>  drivers/gpu/drm/msm/dp/dp_drm.c | 10 +++---
>  drivers/gpu/drm/msm/dp/dp_parser.c  | 21 +---
>  drivers/gpu/drm/msm/dp/dp_parser.h  |  1 +
>  4 files changed, 70 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 382b3aa..688bbed 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -10,6 +10,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "msm_drv.h"
>  #include "msm_kms.h"
> @@ -265,8 +266,6 @@ static int dp_display_bind(struct device *dev, struct 
> device *master,
> goto end;
> }
>
> -   dp->dp_display.next_bridge = dp->parser->next_bridge;
> -
> dp->aux->drm_dev = drm;
> rc = dp_aux_register(dp->aux);
> if (rc) {
> @@ -421,6 +420,11 @@ static void dp_display_host_init(struct 
> dp_display_private *dp)
> dp->dp_display.connector_type, dp->core_initialized,
> dp->phy_initialized);
>
> +   if (dp->core_initialized) {
> +   DRM_DEBUG_DP("DP core already initialized\n");
> +   return;
> +   }
> +
> dp_power_init(dp->power, false);
> dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
> dp_aux_init(dp->aux);
> @@ -433,6 +437,11 @@ static void dp_display_host_deinit(struct 
> dp_display_private *dp)
> dp->dp_display.connector_type, dp->core_initialized,
> dp->phy_initialized);
>
> +   if (!dp->core_initialized) {
> +   DRM_DEBUG_DP("DP core not initialized\n");
> +   return;
> +   }
> +
> dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
> dp_aux_deinit(dp->aux);
> dp_power_deinit(dp->power);
> @@ -1502,7 +1511,7 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
>
> dp_hpd_event_setup(dp);
>
> -   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> +   dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0);
>  }
>
>  void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
> @@ -1524,6 +1533,52 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, 
> struct drm_minor *minor)
> }
>  }
>
> +static int dp_display_get_next_bridge(struct msm_dp *dp)
> +{
> +   int rc = 0;

Drop initialization.

> +   struct dp_display_private *dp_priv;
> +   struct device_node *aux_bus;
> +   struct device *dev;
> +
> +   dp_priv = container_of(dp, struct dp_display_private, dp_display);
> +   dev = _priv->pdev->dev;
> +   aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> +
> +   if (aux_bus) {
> +   dp_display_host_init(dp_priv);
> +   dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> +   enable_irq(dp_priv->irq);
> +   dp_display_host_phy_init(dp_priv);
> +
> +   devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
> +
> +   disable_irq(dp_priv->irq);

Why do we disable irq?

> +   }

The aux_bus node leaked.

> +
> +   /*
> +* External bridges are mandatory for eDP interfaces: one has to
> +* provide at least an eDP panel (which gets wrapped into 
> panel-bridge).
> +*
> +* For DisplayPort interfaces external bridges are optional, so
> +* silently ignore an error if one is not present (-ENODEV).
> +*/
> +   rc = dp_parser_find_next_bridge(dp_priv->parser);
> +   if (rc == -ENODEV) {
> +   if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) {
> +   DRM_ERROR("eDP: next bridge is not present\n");
> +   return rc;
> +   }
> +   } else if (rc) {
> +   if (rc != -EPROBE_DEFER)
> +   

Re: [Freedreno] [PATCH v5 4/9] drm/panel-edp: add LQ140M1JW46 edp panel entry

[Stephen Boyd]

Quoting Sankeerth Billakanti (2022-03-16 10:35:49)
> Add panel identification entry for the sharp LQ140M1JW46 eDP panel
> with power sequencing delay information.
>
> Signed-off-by: Sankeerth Billakanti 
> ---

Reviewed-by: Stephen Boyd 

Re: [Freedreno] [PATCH v5 3/9] arm64: dts: qcom: sc7280: Enable backlight for eDP panel

[Stephen Boyd]

Quoting Sankeerth Billakanti (2022-03-16 10:35:48)
> Enable backlight support for eDP panel on CRD platform for sc7280.
>
> Signed-off-by: Sankeerth Billakanti 
> ---
>
> Changes in v5:
>   - Separate out backlight nodes
>
>  arch/arm64/boot/dts/qcom/sc7280-crd.dts | 18 ++
>  1 file changed, 18 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts 
> b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> index 2df654e..16d1a5b 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> @@ -37,6 +37,15 @@
> pinctrl-0 = <_panel_power>;
> };
>
> +   edp_backlight: edp-backlight {

Does this also move to qcard.dtsi? Why can't this be combined with the
previous patch?

> +   compatible = "pwm-backlight";
> +
> +   power-supply = <_edp_bp>;
> +   pwms = <_pwm 3 65535>;
> +
> +   enable-gpios = <_gpios 7 GPIO_ACTIVE_HIGH>;
> +   };
> +
> vreg_edp_bp: vreg-edp-bp-regulator {
> compatible = "regulator-fixed";
> regulator-name = "vreg_edp_bp";
> @@ -123,7 +132,9 @@ ap_ts_pen_1v8:  {
> edp_panel: edp-panel {
> compatible = "edp-panel";
>
> +   backlight = <_backlight>;
> power-supply = <_3v3_regulator>;
> +

Nitpick: Remove this newline from this hunk and put it in when
power-supply is introduced.

> ports {
> #address-cells = <1>;
> #size-cells = <0>;
> @@ -172,6 +183,13 @@ ap_ts_pen_1v8:  {
> };
>  };
>
> +_pwm {
> +   status = "okay";
> +
> +   pinctrl-names = "default";
> +   pinctrl-0 = <_bl_pwm>;

I see the pinctrl is used now but it would be easier to review this
patch if the pinctrl was in this patch.

Re: [Freedreno] [PATCH v5 2/9] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD

[Stephen Boyd]

Quoting Sankeerth Billakanti (2022-03-16 10:35:47)
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts 
> b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> index e2efbdd..2df654e 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> @@ -7,6 +7,7 @@
>
>  /dts-v1/;
>
> +#include 
>  #include "sc7280-idp.dtsi"
>  #include "sc7280-idp-ec-h1.dtsi"
>
> @@ -21,6 +22,27 @@
> chosen {
> stdout-path = "serial0:115200n8";
> };
> +
> +   edp_3v3_regulator: edp-3v3-regulator {
> +   compatible = "regulator-fixed";
> +   regulator-name = "edp_3v3_regulator";
> +
> +   regulator-min-microvolt = <330>;
> +   regulator-max-microvolt = <330>;
> +
> +   gpio = < 80 GPIO_ACTIVE_HIGH>;
> +   enable-active-high;
> +
> +   pinctrl-names = "default";
> +   pinctrl-0 = <_panel_power>;
> +   };
> +
> +   vreg_edp_bp: vreg-edp-bp-regulator {
> +   compatible = "regulator-fixed";
> +   regulator-name = "vreg_edp_bp";
> +   regulator-always-on;
> +   regulator-boot-on;
> +   };
>  };
>
>  _rsc {
> @@ -76,6 +98,58 @@ ap_ts_pen_1v8:  {
> };
>  };
>
> + {
> +   status = "okay";
> +};
> +
> +_dp {
> +   status = "okay";
> +
> +   pinctrl-names = "default";
> +   pinctrl-0 = <_hot_plug_det>;
> +   data-lanes = <0 1>;
> +   vdda-1p2-supply = <_l6b_1p2>;
> +   vdda-0p9-supply = <_l1b_0p8>;
> +};
> +
> +_edp {
> +   status = "okay";
> +
> +   data-lanes = <0 1 2 3>;

Is this property necessary? It looks like the default.

> +   vdda-1p2-supply = <_l6b_1p2>;
> +   vdda-0p9-supply = <_l10c_0p8>;
> +
> +   aux-bus {

Can this move to sc7280.dtsi and get a phandle?

> +   edp_panel: edp-panel {

I'd prefer

edp_panel: panel {

because there's only one panel node at this level.

> +   compatible = "edp-panel";
> +
> +   power-supply = <_3v3_regulator>;

This is board specific, but I thought it was on the qcard so we should
move this to sc7280-qcard.dtsi?

> +   ports {
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> +   port@0 {
> +   reg = <0>;
> +   edp_panel_in: endpoint {

This can be shortened to

port {
edp_panel_in: endpoint {

according to panel-edp.yaml

> +   remote-endpoint = 
> <_edp_out>;
> +   };
> +   };
> +   };
> +   };
> +   };
> +};
> +
> +_edp_out {
> +   remote-endpoint = <_panel_in>;
> +};
> +
> +_edp_phy {
> +   status = "okay";
> +};
> +
> +_mdp {
> +   status = "okay";
> +};
> +
>  _3v3_regulator {
> gpio = < 51 GPIO_ACTIVE_HIGH>;
>  };
> @@ -84,7 +158,26 @@ ap_ts_pen_1v8:  {
> pins = "gpio51";
>  };
>
> +_gpios {
> +   edp_bl_power: edp-bl-power {

Is this used in a later patch?

> +   pins = "gpio7";
> +   function = "normal";
> +   qcom,drive-strength = ;
> +   };
> +
> +   edp_bl_pwm: edp-bl-pwm {

Is this used in a later patch? Can it be moved to the patch that uses
it?

> +   pins = "gpio8";
> +   function = "func1";
> +   qcom,drive-strength = ;
> +   };
> +};
> +
>   {
> +   edp_panel_power: edp-panel-power {
> +   pins = "gpio80";
> +   function = "gpio";

function of gpio is unnecessary. Where is the bias and drive-strength
settings?

> +   };
> +
> tp_int_odl: tp-int-odl {
> pins = "gpio7";
> function = "gpio";
> --
> 2.7.4
>

Re: [Freedreno] [PATCH v5 1/9] arm64: dts: qcom: sc7280: rename edp_out label to mdss_edp_out

[Stephen Boyd]

Quoting Sankeerth Billakanti (2022-03-16 10:35:46)
> Rename the edp_out label in the sc7280 platform to mdss_edp_out
> so that the nodes related to mdss are all grouped together in
> the board specific files.
>
> Signed-off-by: Sankeerth Billakanti 
> ---

Reviewed-by: Stephen Boyd 

Re: [Freedreno] [PATCH v6 1/5] drm/msm/disp/dpu1: set mdp clk to the maximum frequency in opp table during probe

[Stephen Boyd]

Quoting Vinod Polimera (2022-03-14 07:46:53)
> use max clock during probe/bind sequence from the opp table.
> The clock will be scaled down when framework sends an update.

Capitalize 'use'.

Why is it important to use max frequency during probe/bind? Does not
setting the clk rate during probe mean that we'll never use the max
rate? Does it speed things up during probe?

Re: [Freedreno] [PATCH 3/3] drm/msm/gpu: Remove mutex from wait_event condition

[Rob Clark]

On Thu, Mar 17, 2022 at 1:45 PM Akhil P Oommen  wrote:
>
> On 3/11/2022 5:16 AM, Rob Clark wrote:
> > From: Rob Clark 
> >
> > The mutex wasn't really protecting anything before.  Before the previous
> > patch we could still be racing with the scheduler's kthread, as that is
> > not necessarily frozen yet.  Now that we've parked the sched threads,
> > the only race is with jobs retiring, and that is harmless, ie.
> >
> > Signed-off-by: Rob Clark 
> > ---
> >   drivers/gpu/drm/msm/adreno/adreno_device.c | 11 +--
> >   1 file changed, 1 insertion(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> > b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index 0440a98988fc..661dfa7681fb 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -607,15 +607,6 @@ static int adreno_runtime_resume(struct device *dev)
> >   return gpu->funcs->pm_resume(gpu);
> >   }
> >
> > -static int active_submits(struct msm_gpu *gpu)
> > -{
> > - int active_submits;
> > - mutex_lock(>active_lock);
> > - active_submits = gpu->active_submits;
> > - mutex_unlock(>active_lock);
> I assumed that this lock here was to ensure proper barriers while
> reading active_submits. Is that not required?

There is a spinlock in prepare_to_wait_event() ahead of checking the
condition, which AFAIU is a sufficient barrier

BR,
-R

>
> -Akhil.
> > - return active_submits;
> > -}
> > -
> >   static int adreno_runtime_suspend(struct device *dev)
> >   {
> >   struct msm_gpu *gpu = dev_to_gpu(dev);
> > @@ -669,7 +660,7 @@ static int adreno_system_suspend(struct device *dev)
> >   suspend_scheduler(gpu);
> >
> >   remaining = wait_event_timeout(gpu->retire_event,
> > -active_submits(gpu) == 0,
> > +gpu->active_submits == 0,
> >  msecs_to_jiffies(1000));
> >   if (remaining == 0) {
> >   dev_err(dev, "Timeout waiting for GPU to suspend\n");
>

Re: [Freedreno] [PATCH 3/3] drm/msm/gpu: Remove mutex from wait_event condition

[Akhil P Oommen]

On 3/11/2022 5:16 AM, Rob Clark wrote:

From: Rob Clark 

The mutex wasn't really protecting anything before.  Before the previous
patch we could still be racing with the scheduler's kthread, as that is
not necessarily frozen yet.  Now that we've parked the sched threads,
the only race is with jobs retiring, and that is harmless, ie.

Signed-off-by: Rob Clark 
---
  drivers/gpu/drm/msm/adreno/adreno_device.c | 11 +--
  1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 0440a98988fc..661dfa7681fb 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -607,15 +607,6 @@ static int adreno_runtime_resume(struct device *dev)
return gpu->funcs->pm_resume(gpu);
  }
  
-static int active_submits(struct msm_gpu *gpu)

-{
-   int active_submits;
-   mutex_lock(>active_lock);
-   active_submits = gpu->active_submits;
-   mutex_unlock(>active_lock);
I assumed that this lock here was to ensure proper barriers while 
reading active_submits. Is that not required?


-Akhil.

-   return active_submits;
-}
-
  static int adreno_runtime_suspend(struct device *dev)
  {
struct msm_gpu *gpu = dev_to_gpu(dev);
@@ -669,7 +660,7 @@ static int adreno_system_suspend(struct device *dev)
suspend_scheduler(gpu);
  
  	remaining = wait_event_timeout(gpu->retire_event,

-  active_submits(gpu) == 0,
+  gpu->active_submits == 0,
   msecs_to_jiffies(1000));
if (remaining == 0) {
dev_err(dev, "Timeout waiting for GPU to suspend\n");

Re: [Freedreno] [PATCH 2/3] drm/msm/gpu: Park scheduler threads for system suspend

[Rob Clark]

On Thu, Mar 17, 2022 at 12:50 PM Andrey Grodzovsky
 wrote:
>
>
> On 2022-03-17 14:25, Rob Clark wrote:
> > On Thu, Mar 17, 2022 at 11:10 AM Andrey Grodzovsky
> >  wrote:
> >>
> >> On 2022-03-17 13:35, Rob Clark wrote:
> >>> On Thu, Mar 17, 2022 at 9:45 AM Christian König
> >>>  wrote:
>  Am 17.03.22 um 17:18 schrieb Rob Clark:
> > On Thu, Mar 17, 2022 at 9:04 AM Christian König
> >  wrote:
> >> Am 17.03.22 um 16:10 schrieb Rob Clark:
> >>> [SNIP]
> >>> userspace frozen != kthread frozen .. that is what this patch is
> >>> trying to address, so we aren't racing between shutting down the hw
> >>> and the scheduler shoveling more jobs at us.
> >> Well exactly that's the problem. The scheduler is supposed to shoveling
> >> more jobs at us until it is empty.
> >>
> >> Thinking more about it we will then keep some dma_fence instance
> >> unsignaled and that is and extremely bad idea since it can lead to
> >> deadlocks during suspend.
> > Hmm, perhaps that is true if you need to migrate things out of vram?
> > It is at least not a problem when vram is not involved.
>  No, it's much wider than that.
> 
>  See what can happen is that the memory management shrinkers want to wait
>  for a dma_fence during suspend.
> >>> we don't wait on fences in shrinker, only purging or evicting things
> >>> that are already ready.  Actually, waiting on fences in shrinker path
> >>> sounds like a pretty bad idea.
> >>>
>  And if you stop the scheduler they will just wait forever.
> 
>  What you need to do instead is to drain the scheduler, e.g. call
>  drm_sched_entity_flush() with a proper timeout for each entity you have
>  created.
> >>> yeah, it would work to drain the scheduler.. I guess that might be the
> >>> more portable approach as far as generic solution for suspend.
> >>>
> >>> BR,
> >>> -R
> >>
> >> I am not sure how this drains the scheduler ? Suppose we done the
> >> waiting in drm_sched_entity_flush,
> >> what prevents someone to push right away another job into the same
> >> entity's queue  right after that ?
> >> Shouldn't we first disable further pushing of jobs into entity before we
> >> wait for  sched->job_scheduled ?
> >>
> > In the system suspend path, userspace processes will have already been
> > frozen, so there should be no way to push more jobs to the scheduler,
> > unless they are pushed from the kernel itself.
>
>
> It was my suspicion but I wasn't sure about it.
>
>
> > We don't do that in
> > drm/msm, but maybe you need to to move things btwn vram and system
> > memory?
>
>
> Exactly, that was my main concern - if we use this method we have to use
> it in a point in
> suspend sequence when all the in kernel job submissions activity already
> suspended
>
> > But even in that case, if the # of jobs you push is bounded I
> > guess that is ok?
>
> Submissions to scheduler entities are using unbounded queue, the bounded
> part is when
> you extract next job from entity to submit to HW ring and it rejects if
> submission limit reached (drm_sched_ready)
>
> In general - It looks to me at least that what we what we want her is
> more of a drain operation then flush (i.e.
> we first want to disable any further job submission to entity's queue
> and then flush all in flight ones). As example
> for this i was looking at  flush_workqueue vs. drain_workqueue

Would it be possible for amdgpu to, in the system suspend task,

1) first queue up all the jobs needed to migrate bos out of vram, and
whatever other housekeeping jobs are needed
2) then drain gpu scheduler's queues
3) and then finally wait for jobs executing on GPU to complete

BR,
-R

> Andrey
>
>
> >
> > BR,
> > -R

[Freedreno] [PATCH] drm/msm: Add missing put_task_struct() in debugfs path

[Rob Clark]

From: Rob Clark 

Fixes: 25faf2f2e065 ("drm/msm: Show process names in gem_describe")
Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/msm_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 02b9ae65a96a..a4f61972667b 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -926,6 +926,7 @@ void msm_gem_describe(struct drm_gem_object *obj, struct 
seq_file *m,
get_pid_task(aspace->pid, PIDTYPE_PID);
if (task) {
comm = kstrdup(task->comm, GFP_KERNEL);
+   put_task_struct(task);
} else {
comm = NULL;
}
-- 
2.35.1

Re: [Freedreno] [PATCH 2/3] drm/msm/gpu: Park scheduler threads for system suspend

[Rob Clark]

On Thu, Mar 17, 2022 at 11:10 AM Andrey Grodzovsky
 wrote:
>
>
> On 2022-03-17 13:35, Rob Clark wrote:
> > On Thu, Mar 17, 2022 at 9:45 AM Christian König
> >  wrote:
> >> Am 17.03.22 um 17:18 schrieb Rob Clark:
> >>> On Thu, Mar 17, 2022 at 9:04 AM Christian König
> >>>  wrote:
>  Am 17.03.22 um 16:10 schrieb Rob Clark:
> > [SNIP]
> > userspace frozen != kthread frozen .. that is what this patch is
> > trying to address, so we aren't racing between shutting down the hw
> > and the scheduler shoveling more jobs at us.
>  Well exactly that's the problem. The scheduler is supposed to shoveling
>  more jobs at us until it is empty.
> 
>  Thinking more about it we will then keep some dma_fence instance
>  unsignaled and that is and extremely bad idea since it can lead to
>  deadlocks during suspend.
> >>> Hmm, perhaps that is true if you need to migrate things out of vram?
> >>> It is at least not a problem when vram is not involved.
> >> No, it's much wider than that.
> >>
> >> See what can happen is that the memory management shrinkers want to wait
> >> for a dma_fence during suspend.
> > we don't wait on fences in shrinker, only purging or evicting things
> > that are already ready.  Actually, waiting on fences in shrinker path
> > sounds like a pretty bad idea.
> >
> >> And if you stop the scheduler they will just wait forever.
> >>
> >> What you need to do instead is to drain the scheduler, e.g. call
> >> drm_sched_entity_flush() with a proper timeout for each entity you have
> >> created.
> > yeah, it would work to drain the scheduler.. I guess that might be the
> > more portable approach as far as generic solution for suspend.
> >
> > BR,
> > -R
>
>
> I am not sure how this drains the scheduler ? Suppose we done the
> waiting in drm_sched_entity_flush,
> what prevents someone to push right away another job into the same
> entity's queue  right after that ?
> Shouldn't we first disable further pushing of jobs into entity before we
> wait for  sched->job_scheduled ?
>

In the system suspend path, userspace processes will have already been
frozen, so there should be no way to push more jobs to the scheduler,
unless they are pushed from the kernel itself.  We don't do that in
drm/msm, but maybe you need to to move things btwn vram and system
memory?  But even in that case, if the # of jobs you push is bounded I
guess that is ok?

BR,
-R

Re: [Freedreno] [PATCH 1/6] drm: allow real encoder to be passed for drm_writeback_connector

[Abhinav Kumar]

Hi Daniel

On 3/17/2022 3:01 AM, Daniel Vetter wrote:

On Fri, Mar 11, 2022 at 10:05:53AM +0200, Laurent Pinchart wrote:

On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:

On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar  wrote:


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.

Co-developed-by: Kandpal Suraj 
Signed-off-by: Abhinav Kumar 
---
  drivers/gpu/drm/drm_writeback.c |  8 
  include/drm/drm_writeback.h | 13 +++--
  2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dccf4504..4dad687 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -189,8 +189,8 @@ 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);
-   ret = drm_encoder_init(dev, _connector->encoder,
+   drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
+   ret = drm_encoder_init(dev, wb_connector->encoder,
_writeback_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);


If the encoder is provided by a separate driver, it might use a
different set of encoder funcs.


More than that, if the encoder is provided externally but doesn't have
custom operations, I don't really see the point of having an external
encoder in the first place.

Has this series been tested with a driver that needs to provide an
encoder, to make sure it fits the purpose ?


Also, can we not force all drivers to do this setup that don't need it? We
have a ton of kms drivers, forcing unnecessary busiwork on drivers is
really not good.
-Daniel

No, there will not be any requirement forced for existing drivers.
A new API will be exposed for new clients which setup the encoder.





I'd suggest checking whether the wb_connector->encoder is NULL here.
If it is, allocate one using drmm_kzalloc and init it.
If it is not NULL, assume that it has been initialized already, so
skip the drm_encoder_init() and just call the drm_encoder_helper_add()


 if (ret)
@@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
 goto connector_fail;

 ret = drm_connector_attach_encoder(connector,
-   _connector->encoder);
+   wb_connector->encoder);
 if (ret)
 goto attach_fail;

@@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
  attach_fail:
 drm_connector_cleanup(connector);
  connector_fail:
-   drm_encoder_cleanup(_connector->encoder);
+   drm_encoder_cleanup(wb_connector->encoder);
  fail:
 drm_property_blob_put(blob);
 return ret;
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 9697d27..0ba266e 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -25,13 +25,22 @@ struct drm_writeback_connector {
 struct drm_connector base;

 /**
-* @encoder: Internal encoder used by the connector to fulfill
+* @encoder: handle to drm_encoder used by the connector to fulfill
  * the DRM framework requirements. The users of the
  * @drm_writeback_connector control the behaviour of the @encoder
  * by passing the @enc_funcs parameter to 
drm_writeback_connector_init()
  * function.
+*
+* For some vendor drivers, the hardware resources are shared between
+* writeback encoder and rest of the display pipeline.
+* To accommodate such cases, encoder is a handle to the real encoder
+* hardware.
+*
+* For current existing writeback users, this shall continue to be the
+* embedded encoder for the writeback connector.
+*
  */
-   struct drm_encoder encoder;
+   struct drm_encoder *encoder;

 /**
  * @pixel_formats_blob_ptr:


--
Regards,

Laurent Pinchart

Re: [Freedreno] [PATCH 2/3] drm/msm/gpu: Park scheduler threads for system suspend

[Rob Clark]

On Thu, Mar 17, 2022 at 9:45 AM Christian König
 wrote:
>
> Am 17.03.22 um 17:18 schrieb Rob Clark:
> > On Thu, Mar 17, 2022 at 9:04 AM Christian König
> >  wrote:
> >> Am 17.03.22 um 16:10 schrieb Rob Clark:
> >>> [SNIP]
> >>> userspace frozen != kthread frozen .. that is what this patch is
> >>> trying to address, so we aren't racing between shutting down the hw
> >>> and the scheduler shoveling more jobs at us.
> >> Well exactly that's the problem. The scheduler is supposed to shoveling
> >> more jobs at us until it is empty.
> >>
> >> Thinking more about it we will then keep some dma_fence instance
> >> unsignaled and that is and extremely bad idea since it can lead to
> >> deadlocks during suspend.
> > Hmm, perhaps that is true if you need to migrate things out of vram?
> > It is at least not a problem when vram is not involved.
>
> No, it's much wider than that.
>
> See what can happen is that the memory management shrinkers want to wait
> for a dma_fence during suspend.

we don't wait on fences in shrinker, only purging or evicting things
that are already ready.  Actually, waiting on fences in shrinker path
sounds like a pretty bad idea.

> And if you stop the scheduler they will just wait forever.
>
> What you need to do instead is to drain the scheduler, e.g. call
> drm_sched_entity_flush() with a proper timeout for each entity you have
> created.

yeah, it would work to drain the scheduler.. I guess that might be the
more portable approach as far as generic solution for suspend.

BR,
-R

> Regards,
> Christian.
>
> >
> >> So this patch here is an absolute clear NAK from my side. If amdgpu is
> >> doing something similar that is a severe bug and needs to be addressed
> >> somehow.
> > I think amdgpu's use of kthread_park is not related to suspend, but
> > didn't look too closely.
> >
> > And perhaps the solution for this problem is more complex in the case
> > of amdgpu, I'm not super familiar with the constraints there.  But I
> > think it is a fine solution for integrated GPUs.
> >
> > BR,
> > -R
> >
> >> Regards,
> >> Christian.
> >>
> >>> BR,
> >>> -R
> >>>
>

Re: [Freedreno] [PATCH 2/3] drm/msm/gpu: Park scheduler threads for system suspend

[Daniel Vetter]

On Thu, Mar 17, 2022 at 05:44:57PM +0100, Christian König wrote:
> Am 17.03.22 um 17:18 schrieb Rob Clark:
> > On Thu, Mar 17, 2022 at 9:04 AM Christian König
> >  wrote:
> > > Am 17.03.22 um 16:10 schrieb Rob Clark:
> > > > [SNIP]
> > > > userspace frozen != kthread frozen .. that is what this patch is
> > > > trying to address, so we aren't racing between shutting down the hw
> > > > and the scheduler shoveling more jobs at us.
> > > Well exactly that's the problem. The scheduler is supposed to shoveling
> > > more jobs at us until it is empty.
> > > 
> > > Thinking more about it we will then keep some dma_fence instance
> > > unsignaled and that is and extremely bad idea since it can lead to
> > > deadlocks during suspend.
> > Hmm, perhaps that is true if you need to migrate things out of vram?
> > It is at least not a problem when vram is not involved.
> 
> No, it's much wider than that.
> 
> See what can happen is that the memory management shrinkers want to wait for
> a dma_fence during suspend.
> 
> And if you stop the scheduler they will just wait forever.
> 
> What you need to do instead is to drain the scheduler, e.g. call
> drm_sched_entity_flush() with a proper timeout for each entity you have
> created.

Yeah I think properly flushing the scheduler and stopping it and cutting
all drivers over to that sounds like the right approach. Generally suspend
shouldn't be such a critical path that this will hurt us, all the other io
queues get flushed too afaik.

Resume is the thing that needs to go real fast.

So a patch set to move all drivers that open code the kthread_park to the
right scheduler function sounds like the right idea here to me.
-Daniel

> 
> Regards,
> Christian.
> 
> > 
> > > So this patch here is an absolute clear NAK from my side. If amdgpu is
> > > doing something similar that is a severe bug and needs to be addressed
> > > somehow.
> > I think amdgpu's use of kthread_park is not related to suspend, but
> > didn't look too closely.
> > 
> > And perhaps the solution for this problem is more complex in the case
> > of amdgpu, I'm not super familiar with the constraints there.  But I
> > think it is a fine solution for integrated GPUs.
> > 
> > BR,
> > -R
> > 
> > > Regards,
> > > Christian.
> > > 
> > > > BR,
> > > > -R
> > > > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Freedreno] [PATCH v3 1/3] drm: allow real encoder to be passed for drm_writeback_connector

[Abhinav Kumar]

Hi Laurent

Thanks for the review.

On 3/17/2022 1:51 AM, Laurent Pinchart wrote:

Hi Abhinav,

Thank you for the patch.

On Wed, Mar 16, 2022 at 11:48:16AM -0700, Abhinav Kumar wrote:

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 using a new
drm_writeback_connector_init_with_encoder() API.


The commit message doesn't match the commit.
Sorry, while splitting the change , I missed this part of the commit 
text. Will fix it up.



In addition, to preserve the same call flows for the existing
users of drm_writeback_connector_init(), also allow passing
possible_crtcs as a parameter so that encoder can be initialized
with it.

changes in v3:
- allow passing possible_crtcs for existing users of
  drm_writeback_connector_init()
- squash the vendor changes into the same commit so
  that each patch in the series can compile individually

Co-developed-by: Kandpal Suraj 
Signed-off-by: Abhinav Kumar 
---
  .../drm/arm/display/komeda/komeda_wb_connector.c   |   3 +-
  drivers/gpu/drm/arm/malidp_mw.c|   5 +-
  drivers/gpu/drm/drm_writeback.c| 103 +
  drivers/gpu/drm/rcar-du/rcar_du_writeback.c|   5 +-
  drivers/gpu/drm/vc4/vc4_txp.c  |  19 ++--
  drivers/gpu/drm/vkms/vkms_writeback.c  |   3 +-
  include/drm/drm_writeback.h|  22 -
  7 files changed, 103 insertions(+), 57 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..b882066 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -208,11 +208,12 @@ int malidp_mw_connector_init(struct drm_device *drm)
struct malidp_drm *malidp = drm->dev_private;
u32 *formats;
int ret, n_formats;
+   uint32_t possible_crtcs;
  
  	if (!malidp->dev->hw->enable_memwrite)

return 0;
  
-	malidp->mw_connector.encoder.possible_crtcs = 1 << drm_crtc_index(>crtc);

+   possible_crtcs = 1 << drm_crtc_index(>crtc);
drm_connector_helper_add(>mw_connector.base,
 _mw_connector_helper_funcs);
  
@@ -223,7 +224,7 @@ 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, possible_crtcs);


Do you need the local variable ?


Yes, we can dtop this. I just used this instead of "1 << 
drm_crtc_index(>crtc)" to simplify it.

No strong preference.




kfree(formats);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dccf4504..17c1471 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -149,36 +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 

[Freedreno] [PATCH v2 3/3] drm/msm: Add a way to override processes comm/cmdline

[Rob Clark]

From: Rob Clark 

In the cause of using the GPU via virtgpu, the host side process is
really a sort of proxy, and not terribly interesting from the PoV of
crash/fault logging.  Add a way to override these per process so that
we can see the guest process's name.

v2: Handle kmalloc failure, add comment to explain kstrdup returns
NULL if passed NULL [Dan Carpenter]

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 43 +++--
 drivers/gpu/drm/msm/msm_gpu.c   | 12 +--
 drivers/gpu/drm/msm/msm_gpu.h   |  6 
 drivers/gpu/drm/msm/msm_submitqueue.c   |  2 ++
 include/uapi/drm/msm_drm.h  |  2 ++
 5 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 3d307b34854d..45f2c6084aa7 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -290,11 +290,48 @@ int adreno_get_param(struct msm_gpu *gpu, struct 
msm_file_private *ctx,
 int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
 uint32_t param, uint64_t value, uint32_t len)
 {
-   /* No pointer params yet */
-   if (len != 0)
-   return -EINVAL;
+   switch (param) {
+   case MSM_PARAM_COMM:
+   case MSM_PARAM_CMDLINE:
+   /* kstrdup_quotable_cmdline() limits to PAGE_SIZE, so
+* that should be a reasonable upper bound
+*/
+   if (len > PAGE_SIZE)
+   return -EINVAL;
+   break;
+   default:
+   if (len != 0)
+   return -EINVAL;
+   }
 
switch (param) {
+   case MSM_PARAM_COMM:
+   case MSM_PARAM_CMDLINE: {
+   char *str, **paramp;
+
+   str = kmalloc(len + 1, GFP_KERNEL);
+   if (!str)
+   return -ENOMEM;
+
+   if (copy_from_user(str, u64_to_user_ptr(value), len)) {
+   kfree(str);
+   return -EFAULT;
+   }
+
+   /* Ensure string is null terminated: */
+   str[len] = '\0';
+
+   if (param == MSM_PARAM_COMM) {
+   paramp = >comm;
+   } else {
+   paramp = >cmdline;
+   }
+
+   kfree(*paramp);
+   *paramp = str;
+
+   return 0;
+   }
case MSM_PARAM_SYSPROF:
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 4ec62b601adc..747b89aa9d13 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -364,14 +364,22 @@ static void retire_submits(struct msm_gpu *gpu);
 
 static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char 
**cmd)
 {
+   struct msm_file_private *ctx = submit->queue->ctx;
struct task_struct *task;
 
+   /* Note that kstrdup will return NULL if argument is NULL: */
+   *comm = kstrdup(ctx->comm, GFP_KERNEL);
+   *cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
+
task = get_pid_task(submit->pid, PIDTYPE_PID);
if (!task)
return;
 
-   *comm = kstrdup(task->comm, GFP_KERNEL);
-   *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
+   if (!*comm)
+   *comm = kstrdup(task->comm, GFP_KERNEL);
+
+   if (!*cmd)
+   *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
 
put_task_struct(task);
 }
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index c28c2ad9f52e..2c0203fd6ce3 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -355,6 +355,12 @@ struct msm_file_private {
 */
int sysprof;
 
+   /** comm: Overridden task comm, see MSM_PARAM_COMM */
+   char *comm;
+
+   /** cmdline: Overridden task cmdline, see MSM_PARAM_CMDLINE */
+   char *cmdline;
+
/**
 * elapsed:
 *
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c 
b/drivers/gpu/drm/msm/msm_submitqueue.c
index 79b6ccd6ce64..f486a3cd4e55 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -61,6 +61,8 @@ void __msm_file_private_destroy(struct kref *kref)
}
 
msm_gem_address_space_put(ctx->aspace);
+   kfree(ctx->comm);
+   kfree(ctx->cmdline);
kfree(ctx);
 }
 
diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
index 0aa1a8cb4e0d..794ad1948497 100644
--- a/include/uapi/drm/msm_drm.h
+++ b/include/uapi/drm/msm_drm.h
@@ -82,6 +82,8 @@ struct drm_msm_timespec {
 #define MSM_PARAM_FAULTS 0x09  /* RO */
 #define MSM_PARAM_SUSPENDS   0x0a  /* RO */
 #define MSM_PARAM_SYSPROF0x0b  /* WO: 1 preserves perfcntrs, 2 also 
disables suspend */
+#define MSM_PARAM_COMM   

[Freedreno] [PATCH v2 2/3] drm/msm: Split out helper to get comm/cmdline

[Rob Clark]

From: Rob Clark 

Deduplicate this from fault_worker and recover_worker.

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/msm_gpu.c | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 8fe4aee96aa9..4ec62b601adc 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -362,6 +362,20 @@ find_submit(struct msm_ringbuffer *ring, uint32_t fence)
 
 static void retire_submits(struct msm_gpu *gpu);
 
+static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, char 
**cmd)
+{
+   struct task_struct *task;
+
+   task = get_pid_task(submit->pid, PIDTYPE_PID);
+   if (!task)
+   return;
+
+   *comm = kstrdup(task->comm, GFP_KERNEL);
+   *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
+
+   put_task_struct(task);
+}
+
 static void recover_worker(struct kthread_work *work)
 {
struct msm_gpu *gpu = container_of(work, struct msm_gpu, recover_work);
@@ -378,18 +392,11 @@ static void recover_worker(struct kthread_work *work)
 
submit = find_submit(cur_ring, cur_ring->memptrs->fence + 1);
if (submit) {
-   struct task_struct *task;
-
/* Increment the fault counts */
submit->queue->faults++;
submit->aspace->faults++;
 
-   task = get_pid_task(submit->pid, PIDTYPE_PID);
-   if (task) {
-   comm = kstrdup(task->comm, GFP_KERNEL);
-   cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
-   put_task_struct(task);
-   }
+   get_comm_cmdline(submit, , );
 
if (comm && cmd) {
DRM_DEV_ERROR(dev->dev, "%s: offending task: %s (%s)\n",
@@ -478,14 +485,7 @@ static void fault_worker(struct kthread_work *work)
goto resume_smmu;
 
if (submit) {
-   struct task_struct *task;
-
-   task = get_pid_task(submit->pid, PIDTYPE_PID);
-   if (task) {
-   comm = kstrdup(task->comm, GFP_KERNEL);
-   cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
-   put_task_struct(task);
-   }
+   get_comm_cmdline(submit, , );
 
/*
 * When we get GPU iova faults, we can get 1000s of them,
-- 
2.35.1

[Freedreno] [PATCH v2 1/3] drm/msm: Add support for pointer params

[Rob Clark]

From: Rob Clark 

The 64b value field is already suffient to hold a pointer instead of
immediate, but we also need a length field.

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 12 ++--
 drivers/gpu/drm/msm/adreno/adreno_gpu.h |  4 ++--
 drivers/gpu/drm/msm/msm_drv.c   |  8 
 drivers/gpu/drm/msm/msm_gpu.h   |  4 ++--
 drivers/gpu/drm/msm/msm_rd.c|  5 +++--
 include/uapi/drm/msm_drm.h  |  2 ++
 6 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 9efc84929be0..3d307b34854d 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -229,10 +229,14 @@ adreno_iommu_create_address_space(struct msm_gpu *gpu,
 }
 
 int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
-uint32_t param, uint64_t *value)
+uint32_t param, uint64_t *value, uint32_t *len)
 {
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 
+   /* No pointer params yet */
+   if (*len != 0)
+   return -EINVAL;
+
switch (param) {
case MSM_PARAM_GPU_ID:
*value = adreno_gpu->info->revn;
@@ -284,8 +288,12 @@ int adreno_get_param(struct msm_gpu *gpu, struct 
msm_file_private *ctx,
 }
 
 int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
-uint32_t param, uint64_t value)
+uint32_t param, uint64_t value, uint32_t len)
 {
+   /* No pointer params yet */
+   if (len != 0)
+   return -EINVAL;
+
switch (param) {
case MSM_PARAM_SYSPROF:
if (!capable(CAP_SYS_ADMIN))
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 0490c5fbb780..ab3b5ef80332 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -281,9 +281,9 @@ static inline int adreno_is_a650_family(struct adreno_gpu 
*gpu)
 }
 
 int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
-uint32_t param, uint64_t *value);
+uint32_t param, uint64_t *value, uint32_t *len);
 int adreno_set_param(struct msm_gpu *gpu, struct msm_file_private *ctx,
-uint32_t param, uint64_t value);
+uint32_t param, uint64_t value, uint32_t len);
 const struct firmware *adreno_request_fw(struct adreno_gpu *adreno_gpu,
const char *fwname);
 struct drm_gem_object *adreno_fw_create_bo(struct msm_gpu *gpu,
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 780f9748aaaf..a5eed5738ac8 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -610,7 +610,7 @@ static int msm_ioctl_get_param(struct drm_device *dev, void 
*data,
/* for now, we just have 3d pipe.. eventually this would need to
 * be more clever to dispatch to appropriate gpu module:
 */
-   if (args->pipe != MSM_PIPE_3D0)
+   if ((args->pipe != MSM_PIPE_3D0) || (args->pad != 0))
return -EINVAL;
 
gpu = priv->gpu;
@@ -619,7 +619,7 @@ static int msm_ioctl_get_param(struct drm_device *dev, void 
*data,
return -ENXIO;
 
return gpu->funcs->get_param(gpu, file->driver_priv,
-args->param, >value);
+args->param, >value, >len);
 }
 
 static int msm_ioctl_set_param(struct drm_device *dev, void *data,
@@ -629,7 +629,7 @@ static int msm_ioctl_set_param(struct drm_device *dev, void 
*data,
struct drm_msm_param *args = data;
struct msm_gpu *gpu;
 
-   if (args->pipe != MSM_PIPE_3D0)
+   if ((args->pipe != MSM_PIPE_3D0) || (args->pad != 0))
return -EINVAL;
 
gpu = priv->gpu;
@@ -638,7 +638,7 @@ static int msm_ioctl_set_param(struct drm_device *dev, void 
*data,
return -ENXIO;
 
return gpu->funcs->set_param(gpu, file->driver_priv,
-args->param, args->value);
+args->param, args->value, args->len);
 }
 
 static int msm_ioctl_gem_new(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index a84140055920..c28c2ad9f52e 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -44,9 +44,9 @@ struct msm_gpu_config {
  */
 struct msm_gpu_funcs {
int (*get_param)(struct msm_gpu *gpu, struct msm_file_private *ctx,
-uint32_t param, uint64_t *value);
+uint32_t param, uint64_t *value, uint32_t *len);
int (*set_param)(struct msm_gpu *gpu, struct msm_file_private *ctx,
-uint32_t param, uint64_t value);
+uint32_t param, 

[Freedreno] [PATCH v2 0/3] drm/msm: Add comm/cmdline override

[Rob Clark]

From: Rob Clark 

Add a way to override comm/cmdline per-drm_file.  This is useful for
VM scenarios where the host process is just a proxy for the actual
guest process.

Rob Clark (3):
  drm/msm: Add support for pointer params
  drm/msm: Split out helper to get comm/cmdline
  drm/msm: Add a way to override processes comm/cmdline

 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 49 -
 drivers/gpu/drm/msm/adreno/adreno_gpu.h |  4 +-
 drivers/gpu/drm/msm/msm_drv.c   |  8 ++--
 drivers/gpu/drm/msm/msm_gpu.c   | 40 
 drivers/gpu/drm/msm/msm_gpu.h   | 10 -
 drivers/gpu/drm/msm/msm_rd.c|  5 ++-
 drivers/gpu/drm/msm/msm_submitqueue.c   |  2 +
 include/uapi/drm/msm_drm.h  |  4 ++
 8 files changed, 94 insertions(+), 28 deletions(-)

-- 
2.35.1

Re: [Freedreno] [PATCH 2/3] drm/msm/gpu: Park scheduler threads for system suspend

[Rob Clark]

On Thu, Mar 17, 2022 at 9:04 AM Christian König
 wrote:
>
> Am 17.03.22 um 16:10 schrieb Rob Clark:
> > [SNIP]
> > userspace frozen != kthread frozen .. that is what this patch is
> > trying to address, so we aren't racing between shutting down the hw
> > and the scheduler shoveling more jobs at us.
>
> Well exactly that's the problem. The scheduler is supposed to shoveling
> more jobs at us until it is empty.
>
> Thinking more about it we will then keep some dma_fence instance
> unsignaled and that is and extremely bad idea since it can lead to
> deadlocks during suspend.

Hmm, perhaps that is true if you need to migrate things out of vram?
It is at least not a problem when vram is not involved.

> So this patch here is an absolute clear NAK from my side. If amdgpu is
> doing something similar that is a severe bug and needs to be addressed
> somehow.

I think amdgpu's use of kthread_park is not related to suspend, but
didn't look too closely.

And perhaps the solution for this problem is more complex in the case
of amdgpu, I'm not super familiar with the constraints there.  But I
think it is a fine solution for integrated GPUs.

BR,
-R

> Regards,
> Christian.
>
> >
> > BR,
> > -R
> >
>

Re: [Freedreno] [PATCH v3 5/5] drm/msm: allow compile time selection of driver components

[Dmitry Baryshkov]

On 17/03/2022 15:44, Dmitry Baryshkov wrote:

On 16/03/2022 20:26, Abhinav Kumar wrote:



On 3/16/2022 12:31 AM, Dmitry Baryshkov wrote:

On 16/03/2022 03:28, Abhinav Kumar wrote:



On 3/3/2022 7:21 PM, Dmitry Baryshkov wrote:
MSM DRM driver already allows one to compile out the DP or DSI 
support.

Add support for disabling other features like MDP4/MDP5/DPU drivers or
direct HDMI output support.

Suggested-by: Stephen Boyd 
Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/Kconfig    | 50 
--

  drivers/gpu/drm/msm/Makefile   | 18 ++--
  drivers/gpu/drm/msm/msm_drv.h  | 33 ++
  drivers/gpu/drm/msm/msm_mdss.c | 13 +++--
  4 files changed, 106 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index 9b019598e042..3735fd41eb3b 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -46,12 +46,39 @@ config DRM_MSM_GPU_SUDO
    Only use this if you are a driver developer.  This should 
*not*

    be enabled for production kernels.  If unsure, say N.
-config DRM_MSM_HDMI_HDCP
-    bool "Enable HDMI HDCP support in MSM DRM driver"
+config DRM_MSM_MDSS
+    bool
+    depends on DRM_MSM
+    default n

shouldnt DRM_MSM_MDSS be defaulted to y?


No, it will be selected either by MDP5 or by DPU1. It is not used if 
DRM_MSM is compiled with just MDP4 or headless support in mind.

Ok got it.




Another question is the compilation validation of the combinations 
of these.


So we need to try:

1) DRM_MSM_MDSS + DRM_MSM_MDP4
2) DRM_MSM_MDSS + DRM_MSM_MDP5
3) DRM_MSM_MDSS + DRM_MSM_DPU

Earlier since all of them were compiled together any 
inter-dependencies will not show up. Now since we are separating it 
out, just wanted to make sure each of the combos compile?


I think you meant:
- headless
- MDP4
- MDP5
- DPU1
- MDP4 + MDP5
- MDP4 + DPU1
- MDP5 + DPU1
- all three drivers


Yes, each of these combinations.


Each of them was tested.


Hmm. It looks like I had DSI disabled during the tests. Will fix it up.






+
+config DRM_MSM_MDP4
+    bool "Enable MDP4 support in MSM DRM driver"
  depends on DRM_MSM
  default y
  help
-  Choose this option to enable HDCP state machine
+  Compile in support for the Mobile Display Processor v4 
(MDP4) in

+  the MSM DRM driver. It is the older display controller found in
+  devices using APQ8064/MSM8960/MSM8x60 platforms.
+
+config DRM_MSM_MDP5
+    bool "Enable MDP5 support in MSM DRM driver"
+    depends on DRM_MSM
+    select DRM_MSM_MDSS
+    default y
+    help
+  Compile in support for the Mobile Display Processor v5 
(MDP4) in
+  the MSM DRM driver. It is the display controller found in 
devices
+  using e.g. APQ8016/MSM8916/APQ8096/MSM8996/MSM8974/SDM6x0 
platforms.

+
+config DRM_MSM_DPU
+    bool "Enable DPU support in MSM DRM driver"
+    depends on DRM_MSM
+    select DRM_MSM_MDSS
+    default y
+    help
+  Compile in support for the Display Processing Unit in
+  the MSM DRM driver. It is the display controller found in 
devices

+  using e.g. SDM845 and newer platforms.
  config DRM_MSM_DP
  bool "Enable DisplayPort support in MSM DRM driver"
@@ -116,3 +143,20 @@ config DRM_MSM_DSI_7NM_PHY
  help
    Choose this option if DSI PHY on SM8150/SM8250/SC7280 is 
used on

    the platform.
+
+config DRM_MSM_HDMI
+    bool "Enable HDMI support in MSM DRM driver"
+    depends on DRM_MSM
+    default y
+    help
+  Compile in support for the HDMI output MSM DRM driver. It can
+  be a primary or a secondary display on device. Note that 
this is used
+  only for the direct HDMI output. If the device outputs HDMI 
data
+  throught some kind of DSI-to-HDMI bridge, this option can be 
disabled.

+
+config DRM_MSM_HDMI_HDCP
+    bool "Enable HDMI HDCP support in MSM DRM driver"
+    depends on DRM_MSM && DRM_MSM_HDMI
+    default y
+    help
+  Choose this option to enable HDCP state machine
diff --git a/drivers/gpu/drm/msm/Makefile 
b/drivers/gpu/drm/msm/Makefile

index e76927b42033..5fe9c20ab9ee 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -16,6 +16,8 @@ msm-y := \
  adreno/a6xx_gpu.o \
  adreno/a6xx_gmu.o \
  adreno/a6xx_hfi.o \
+
+msm-$(CONFIG_DRM_MSM_HDMI) += \
  hdmi/hdmi.o \
  hdmi/hdmi_audio.o \
  hdmi/hdmi_bridge.o \
@@ -27,8 +29,8 @@ msm-y := \
  hdmi/hdmi_phy_8x60.o \
  hdmi/hdmi_phy_8x74.o \
  hdmi/hdmi_pll_8960.o \
-    disp/mdp_format.o \
-    disp/mdp_kms.o \
+
+msm-$(CONFIG_DRM_MSM_MDP4) += \
  disp/mdp4/mdp4_crtc.o \
  disp/mdp4/mdp4_dtv_encoder.o \
  disp/mdp4/mdp4_lcdc_encoder.o \
@@ -37,6 +39,8 @@ msm-y := \
  disp/mdp4/mdp4_irq.o \
  disp/mdp4/mdp4_kms.o \
  disp/mdp4/mdp4_plane.o \
+
+msm-$(CONFIG_DRM_MSM_MDP5) += \
  disp/mdp5/mdp5_cfg.o \
  disp/mdp5/mdp5_ctl.o \
  disp/mdp5/mdp5_crtc.o \
@@ -47,6 +51,8 @@ 

Re: [Freedreno] [PATCH 2/3] drm/msm/gpu: Park scheduler threads for system suspend

[Matthew Brost]

On Thu, Mar 17, 2022 at 03:06:18AM -0700, Christian König wrote:
> Am 17.03.22 um 10:59 schrieb Daniel Vetter:
> > On Thu, Mar 10, 2022 at 03:46:05PM -0800, Rob Clark wrote:
> >> From: Rob Clark 
> >>
> >> In the system suspend path, we don't want to be racing with the
> >> scheduler kthreads pushing additional queued up jobs to the hw
> >> queue (ringbuffer).  So park them first.  While we are at it,
> >> move the wait for active jobs to complete into the new system-
> >> suspend path.
> >>
> >> Signed-off-by: Rob Clark 
> >> ---
> >>   drivers/gpu/drm/msm/adreno/adreno_device.c | 68 --
> >>   1 file changed, 64 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> >> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> >> index 8859834b51b8..0440a98988fc 100644
> >> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> >> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> >> @@ -619,22 +619,82 @@ static int active_submits(struct msm_gpu *gpu)
> >>   static int adreno_runtime_suspend(struct device *dev)
> >>   {
> >>struct msm_gpu *gpu = dev_to_gpu(dev);
> >> -  int remaining;
> >> +
> >> +  /*
> >> +   * We should be holding a runpm ref, which will prevent
> >> +   * runtime suspend.  In the system suspend path, we've
> >> +   * already waited for active jobs to complete.
> >> +   */
> >> +  WARN_ON_ONCE(gpu->active_submits);
> >> +
> >> +  return gpu->funcs->pm_suspend(gpu);
> >> +}
> >> +
> >> +static void suspend_scheduler(struct msm_gpu *gpu)
> >> +{
> >> +  int i;
> >> +
> >> +  /*
> >> +   * Shut down the scheduler before we force suspend, so that
> >> +   * suspend isn't racing with scheduler kthread feeding us
> >> +   * more work.
> >> +   *
> >> +   * Note, we just want to park the thread, and let any jobs
> >> +   * that are already on the hw queue complete normally, as
> >> +   * opposed to the drm_sched_stop() path used for handling
> >> +   * faulting/timed-out jobs.  We can't really cancel any jobs
> >> +   * already on the hw queue without racing with the GPU.
> >> +   */
> >> +  for (i = 0; i < gpu->nr_rings; i++) {
> >> +  struct drm_gpu_scheduler *sched = >rb[i]->sched;
> >> +  kthread_park(sched->thread);
> > Shouldn't we have some proper interfaces for this?
> 
> If I'm not completely mistaken we already should have one, yes.
> 
> > Also I'm kinda wondering how other drivers do this, feels like we should 
> > have a standard
> > way.
> >
> > Finally not flushing out all in-flight requests sounds a bit like a bad
> > idea for system suspend/resume since that's also the hibernation path, and
> > that would mean your shrinker/page reclaim stops working. At least in full
> > generality. Which ain't good for hibernation.
> 
> Completely agree, that looks like an incorrect workaround to me.
> 
> During suspend all userspace applications should be frozen and all f 
> their hardware activity flushed out and waited for completion.
>

Isn't that what Rob is doing?

He kills the scheduler preventing any new job from being submitted then
waits for an outstanding jobs to complete naturally complete (see the
wait_event_timeout below). If the jobs don't naturally complete the
suspend seems to be aborted? That flow makes sense to me and seems like
a novel way to avoid races.

Matt 
 
> I do remember that our internal guys came up with pretty much the same 
> idea and it sounded broken to me back then as well.
> 
> Regards,
> Christian.
> 
> >
> > Adding Christian and Andrey.
> > -Daniel
> >
> >> +  }
> >> +}
> >> +
> >> +static void resume_scheduler(struct msm_gpu *gpu)
> >> +{
> >> +  int i;
> >> +
> >> +  for (i = 0; i < gpu->nr_rings; i++) {
> >> +  struct drm_gpu_scheduler *sched = >rb[i]->sched;
> >> +  kthread_unpark(sched->thread);
> >> +  }
> >> +}
> >> +
> >> +static int adreno_system_suspend(struct device *dev)
> >> +{
> >> +  struct msm_gpu *gpu = dev_to_gpu(dev);
> >> +  int remaining, ret;
> >> +
> >> +  suspend_scheduler(gpu);
> >>   
> >>remaining = wait_event_timeout(gpu->retire_event,
> >>   active_submits(gpu) == 0,
> >>   msecs_to_jiffies(1000));
> >>if (remaining == 0) {
> >>dev_err(dev, "Timeout waiting for GPU to suspend\n");
> >> -  return -EBUSY;
> >> +  ret = -EBUSY;
> >> +  goto out;
> >>}
> >>   
> >> -  return gpu->funcs->pm_suspend(gpu);
> >> +  ret = pm_runtime_force_suspend(dev);
> >> +out:
> >> +  if (ret)
> >> +  resume_scheduler(gpu);
> >> +
> >> +  return ret;
> >>   }
> >> +
> >> +static int adreno_system_resume(struct device *dev)
> >> +{
> >> +  resume_scheduler(dev_to_gpu(dev));
> >> +  return pm_runtime_force_resume(dev);
> >> +}
> >> +
> >>   #endif
> >>   
> >>   static const struct dev_pm_ops adreno_pm_ops = {
> >> -  SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, 
> >> pm_runtime_force_resume)
> >> +  

Re: [Freedreno] [PATCH 2/3] drm/msm/gpu: Park scheduler threads for system suspend

[Rob Clark]

On Thu, Mar 17, 2022 at 3:06 AM Christian König
 wrote:
>
> Am 17.03.22 um 10:59 schrieb Daniel Vetter:
> > On Thu, Mar 10, 2022 at 03:46:05PM -0800, Rob Clark wrote:
> >> From: Rob Clark 
> >>
> >> In the system suspend path, we don't want to be racing with the
> >> scheduler kthreads pushing additional queued up jobs to the hw
> >> queue (ringbuffer).  So park them first.  While we are at it,
> >> move the wait for active jobs to complete into the new system-
> >> suspend path.
> >>
> >> Signed-off-by: Rob Clark 
> >> ---
> >>   drivers/gpu/drm/msm/adreno/adreno_device.c | 68 --
> >>   1 file changed, 64 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> >> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> >> index 8859834b51b8..0440a98988fc 100644
> >> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> >> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> >> @@ -619,22 +619,82 @@ static int active_submits(struct msm_gpu *gpu)
> >>   static int adreno_runtime_suspend(struct device *dev)
> >>   {
> >>  struct msm_gpu *gpu = dev_to_gpu(dev);
> >> -int remaining;
> >> +
> >> +/*
> >> + * We should be holding a runpm ref, which will prevent
> >> + * runtime suspend.  In the system suspend path, we've
> >> + * already waited for active jobs to complete.
> >> + */
> >> +WARN_ON_ONCE(gpu->active_submits);
> >> +
> >> +return gpu->funcs->pm_suspend(gpu);
> >> +}
> >> +
> >> +static void suspend_scheduler(struct msm_gpu *gpu)
> >> +{
> >> +int i;
> >> +
> >> +/*
> >> + * Shut down the scheduler before we force suspend, so that
> >> + * suspend isn't racing with scheduler kthread feeding us
> >> + * more work.
> >> + *
> >> + * Note, we just want to park the thread, and let any jobs
> >> + * that are already on the hw queue complete normally, as
> >> + * opposed to the drm_sched_stop() path used for handling
> >> + * faulting/timed-out jobs.  We can't really cancel any jobs
> >> + * already on the hw queue without racing with the GPU.
> >> + */
> >> +for (i = 0; i < gpu->nr_rings; i++) {
> >> +struct drm_gpu_scheduler *sched = >rb[i]->sched;
> >> +kthread_park(sched->thread);
> > Shouldn't we have some proper interfaces for this?
>
> If I'm not completely mistaken we already should have one, yes.

drm_sched_stop() was my first thought, but it carries extra baggage.
Really I *just* want to park the kthread.

Note that amdgpu does (for afaict different reasons) park the kthread
directly as well.

> > Also I'm kinda wondering how other drivers do this, feels like we should 
> > have a standard
> > way.

As far as other drivers, it seems like they largely ignore it.  I
suspect other drivers also have problems in this area.

Fwiw, I have a piglit test to try to exercise this path if you want to
try it on other drivers.. might need some futzing around to make sure
enough work is queued up that there is some on hw ring and some queued
up in the scheduler when you try to suspend.

https://gitlab.freedesktop.org/mesa/piglit/-/merge_requests/643

> >
> > Finally not flushing out all in-flight requests sounds a bit like a bad
> > idea for system suspend/resume since that's also the hibernation path, and
> > that would mean your shrinker/page reclaim stops working. At least in full
> > generality. Which ain't good for hibernation.
>
> Completely agree, that looks like an incorrect workaround to me.
>
> During suspend all userspace applications should be frozen and all f
> their hardware activity flushed out and waited for completion.
>
> I do remember that our internal guys came up with pretty much the same
> idea and it sounded broken to me back then as well.

userspace frozen != kthread frozen .. that is what this patch is
trying to address, so we aren't racing between shutting down the hw
and the scheduler shoveling more jobs at us.

BR,
-R

> Regards,
> Christian.
>
> >
> > Adding Christian and Andrey.
> > -Daniel
> >
> >> +}
> >> +}
> >> +
> >> +static void resume_scheduler(struct msm_gpu *gpu)
> >> +{
> >> +int i;
> >> +
> >> +for (i = 0; i < gpu->nr_rings; i++) {
> >> +struct drm_gpu_scheduler *sched = >rb[i]->sched;
> >> +kthread_unpark(sched->thread);
> >> +}
> >> +}
> >> +
> >> +static int adreno_system_suspend(struct device *dev)
> >> +{
> >> +struct msm_gpu *gpu = dev_to_gpu(dev);
> >> +int remaining, ret;
> >> +
> >> +suspend_scheduler(gpu);
> >>
> >>  remaining = wait_event_timeout(gpu->retire_event,
> >> active_submits(gpu) == 0,
> >> msecs_to_jiffies(1000));
> >>  if (remaining == 0) {
> >>  dev_err(dev, "Timeout waiting for GPU to suspend\n");
> >> -return -EBUSY;
> >> +ret = -EBUSY;
> >> +goto out;
> >>  }
> >>
> >> -return 

Re: [Freedreno] [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline

[Rob Clark]

On Thu, Mar 17, 2022 at 1:21 AM Dan Carpenter  wrote:
>
> On Wed, Mar 16, 2022 at 05:29:45PM -0700, Rob Clark wrote:
> >   switch (param) {
> > + case MSM_PARAM_COMM:
> > + case MSM_PARAM_CMDLINE: {
> > + char *str, **paramp;
> > +
> > + str = kmalloc(len + 1, GFP_KERNEL);
>
> if (!str)
> return -ENOMEM;
>
> > + if (copy_from_user(str, u64_to_user_ptr(value), len)) {
> > + kfree(str);
> > + return -EFAULT;
> > + }
> > +
> > + /* Ensure string is null terminated: */
> > + str[len] = '\0';
> > +
> > + if (param == MSM_PARAM_COMM) {
> > + paramp = >comm;
> > + } else {
> > + paramp = >cmdline;
> > + }
> > +
> > + kfree(*paramp);
> > + *paramp = str;
> > +
> > + return 0;
> > + }
> >   case MSM_PARAM_SYSPROF:
> >   if (!capable(CAP_SYS_ADMIN))
> >   return -EPERM;
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > index 4ec62b601adc..68f3f8ade76d 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu);
> >
> >  static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, 
> > char **cmd)
> >  {
> > + struct msm_file_private *ctx = submit->queue->ctx;
> >   struct task_struct *task;
> >
> > + *comm = kstrdup(ctx->comm, GFP_KERNEL);
> > + *cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
> > +
> >   task = get_pid_task(submit->pid, PIDTYPE_PID);
> >   if (!task)
> >   return;
> >
> > - *comm = kstrdup(task->comm, GFP_KERNEL);
> > - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
> > + if (!*comm)
> > + *comm = kstrdup(task->comm, GFP_KERNEL);
>
> What?
>
> If the first allocation failed, then this one is going to fail as well.
> Just return -ENOMEM.  Or maybe this is meant to be checking for an empty
> string?

fwiw, if ctx->comm is NULL, the kstrdup() will return NULL, so this
isn't intended to deal with OoM, but the case that comm and/or cmdline
is not overridden.

BR,
-R

>
> > +
> > + if (!*cmd)
> > + *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
>
> Same.
>
> >
> >   put_task_struct(task);
> >  }
>
> regards,
> dan carpenter
>

Re: [Freedreno] [PATCH v3 5/5] drm/msm: allow compile time selection of driver components

[Dmitry Baryshkov]

On 16/03/2022 20:26, Abhinav Kumar wrote:



On 3/16/2022 12:31 AM, Dmitry Baryshkov wrote:

On 16/03/2022 03:28, Abhinav Kumar wrote:



On 3/3/2022 7:21 PM, Dmitry Baryshkov wrote:

MSM DRM driver already allows one to compile out the DP or DSI support.
Add support for disabling other features like MDP4/MDP5/DPU drivers or
direct HDMI output support.

Suggested-by: Stephen Boyd 
Signed-off-by: Dmitry Baryshkov 
---
  drivers/gpu/drm/msm/Kconfig    | 50 
--

  drivers/gpu/drm/msm/Makefile   | 18 ++--
  drivers/gpu/drm/msm/msm_drv.h  | 33 ++
  drivers/gpu/drm/msm/msm_mdss.c | 13 +++--
  4 files changed, 106 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index 9b019598e042..3735fd41eb3b 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -46,12 +46,39 @@ config DRM_MSM_GPU_SUDO
    Only use this if you are a driver developer.  This should *not*
    be enabled for production kernels.  If unsure, say N.
-config DRM_MSM_HDMI_HDCP
-    bool "Enable HDMI HDCP support in MSM DRM driver"
+config DRM_MSM_MDSS
+    bool
+    depends on DRM_MSM
+    default n

shouldnt DRM_MSM_MDSS be defaulted to y?


No, it will be selected either by MDP5 or by DPU1. It is not used if 
DRM_MSM is compiled with just MDP4 or headless support in mind.

Ok got it.




Another question is the compilation validation of the combinations of 
these.


So we need to try:

1) DRM_MSM_MDSS + DRM_MSM_MDP4
2) DRM_MSM_MDSS + DRM_MSM_MDP5
3) DRM_MSM_MDSS + DRM_MSM_DPU

Earlier since all of them were compiled together any 
inter-dependencies will not show up. Now since we are separating it 
out, just wanted to make sure each of the combos compile?


I think you meant:
- headless
- MDP4
- MDP5
- DPU1
- MDP4 + MDP5
- MDP4 + DPU1
- MDP5 + DPU1
- all three drivers


Yes, each of these combinations.


Each of them was tested.




+
+config DRM_MSM_MDP4
+    bool "Enable MDP4 support in MSM DRM driver"
  depends on DRM_MSM
  default y
  help
-  Choose this option to enable HDCP state machine
+  Compile in support for the Mobile Display Processor v4 (MDP4) in
+  the MSM DRM driver. It is the older display controller found in
+  devices using APQ8064/MSM8960/MSM8x60 platforms.
+
+config DRM_MSM_MDP5
+    bool "Enable MDP5 support in MSM DRM driver"
+    depends on DRM_MSM
+    select DRM_MSM_MDSS
+    default y
+    help
+  Compile in support for the Mobile Display Processor v5 (MDP4) in
+  the MSM DRM driver. It is the display controller found in 
devices
+  using e.g. APQ8016/MSM8916/APQ8096/MSM8996/MSM8974/SDM6x0 
platforms.

+
+config DRM_MSM_DPU
+    bool "Enable DPU support in MSM DRM driver"
+    depends on DRM_MSM
+    select DRM_MSM_MDSS
+    default y
+    help
+  Compile in support for the Display Processing Unit in
+  the MSM DRM driver. It is the display controller found in 
devices

+  using e.g. SDM845 and newer platforms.
  config DRM_MSM_DP
  bool "Enable DisplayPort support in MSM DRM driver"
@@ -116,3 +143,20 @@ config DRM_MSM_DSI_7NM_PHY
  help
    Choose this option if DSI PHY on SM8150/SM8250/SC7280 is 
used on

    the platform.
+
+config DRM_MSM_HDMI
+    bool "Enable HDMI support in MSM DRM driver"
+    depends on DRM_MSM
+    default y
+    help
+  Compile in support for the HDMI output MSM DRM driver. It can
+  be a primary or a secondary display on device. Note that this 
is used

+  only for the direct HDMI output. If the device outputs HDMI data
+  throught some kind of DSI-to-HDMI bridge, this option can be 
disabled.

+
+config DRM_MSM_HDMI_HDCP
+    bool "Enable HDMI HDCP support in MSM DRM driver"
+    depends on DRM_MSM && DRM_MSM_HDMI
+    default y
+    help
+  Choose this option to enable HDCP state machine
diff --git a/drivers/gpu/drm/msm/Makefile 
b/drivers/gpu/drm/msm/Makefile

index e76927b42033..5fe9c20ab9ee 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -16,6 +16,8 @@ msm-y := \
  adreno/a6xx_gpu.o \
  adreno/a6xx_gmu.o \
  adreno/a6xx_hfi.o \
+
+msm-$(CONFIG_DRM_MSM_HDMI) += \
  hdmi/hdmi.o \
  hdmi/hdmi_audio.o \
  hdmi/hdmi_bridge.o \
@@ -27,8 +29,8 @@ msm-y := \
  hdmi/hdmi_phy_8x60.o \
  hdmi/hdmi_phy_8x74.o \
  hdmi/hdmi_pll_8960.o \
-    disp/mdp_format.o \
-    disp/mdp_kms.o \
+
+msm-$(CONFIG_DRM_MSM_MDP4) += \
  disp/mdp4/mdp4_crtc.o \
  disp/mdp4/mdp4_dtv_encoder.o \
  disp/mdp4/mdp4_lcdc_encoder.o \
@@ -37,6 +39,8 @@ msm-y := \
  disp/mdp4/mdp4_irq.o \
  disp/mdp4/mdp4_kms.o \
  disp/mdp4/mdp4_plane.o \
+
+msm-$(CONFIG_DRM_MSM_MDP5) += \
  disp/mdp5/mdp5_cfg.o \
  disp/mdp5/mdp5_ctl.o \
  disp/mdp5/mdp5_crtc.o \
@@ -47,6 +51,8 @@ msm-y := \
  disp/mdp5/mdp5_mixer.o \
  disp/mdp5/mdp5_plane.o \
  disp/mdp5/mdp5_smp.o \
+
+msm-$(CONFIG_DRM_MSM_DPU) 

Re: [Freedreno] [PATCH 1/6] drm: allow real encoder to be passed for drm_writeback_connector

[Daniel Vetter]

On Fri, Mar 11, 2022 at 10:05:53AM +0200, Laurent Pinchart wrote:
> On Fri, Mar 11, 2022 at 10:46:13AM +0300, Dmitry Baryshkov wrote:
> > On Fri, 11 Mar 2022 at 04:50, Abhinav Kumar  
> > wrote:
> > >
> > > 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.
> > >
> > > Co-developed-by: Kandpal Suraj 
> > > Signed-off-by: Abhinav Kumar 
> > > ---
> > >  drivers/gpu/drm/drm_writeback.c |  8 
> > >  include/drm/drm_writeback.h | 13 +++--
> > >  2 files changed, 15 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_writeback.c 
> > > b/drivers/gpu/drm/drm_writeback.c
> > > index dccf4504..4dad687 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -189,8 +189,8 @@ 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);
> > > -   ret = drm_encoder_init(dev, _connector->encoder,
> > > +   drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
> > > +   ret = drm_encoder_init(dev, wb_connector->encoder,
> > >_writeback_encoder_funcs,
> > >DRM_MODE_ENCODER_VIRTUAL, NULL);
> > 
> > If the encoder is provided by a separate driver, it might use a
> > different set of encoder funcs.
> 
> More than that, if the encoder is provided externally but doesn't have
> custom operations, I don't really see the point of having an external
> encoder in the first place.
> 
> Has this series been tested with a driver that needs to provide an
> encoder, to make sure it fits the purpose ?

Also, can we not force all drivers to do this setup that don't need it? We
have a ton of kms drivers, forcing unnecessary busiwork on drivers is
really not good.
-Daniel

> 
> > I'd suggest checking whether the wb_connector->encoder is NULL here.
> > If it is, allocate one using drmm_kzalloc and init it.
> > If it is not NULL, assume that it has been initialized already, so
> > skip the drm_encoder_init() and just call the drm_encoder_helper_add()
> > 
> > > if (ret)
> > > @@ -204,7 +204,7 @@ int drm_writeback_connector_init(struct drm_device 
> > > *dev,
> > > goto connector_fail;
> > >
> > > ret = drm_connector_attach_encoder(connector,
> > > -   _connector->encoder);
> > > +   wb_connector->encoder);
> > > if (ret)
> > > goto attach_fail;
> > >
> > > @@ -233,7 +233,7 @@ int drm_writeback_connector_init(struct drm_device 
> > > *dev,
> > >  attach_fail:
> > > drm_connector_cleanup(connector);
> > >  connector_fail:
> > > -   drm_encoder_cleanup(_connector->encoder);
> > > +   drm_encoder_cleanup(wb_connector->encoder);
> > >  fail:
> > > drm_property_blob_put(blob);
> > > return ret;
> > > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > > index 9697d27..0ba266e 100644
> > > --- a/include/drm/drm_writeback.h
> > > +++ b/include/drm/drm_writeback.h
> > > @@ -25,13 +25,22 @@ struct drm_writeback_connector {
> > > struct drm_connector base;
> > >
> > > /**
> > > -* @encoder: Internal encoder used by the connector to fulfill
> > > +* @encoder: handle to drm_encoder used by the connector to 
> > > fulfill
> > >  * the DRM framework requirements. The users of the
> > >  * @drm_writeback_connector control the behaviour of the @encoder
> > >  * by passing the @enc_funcs parameter to 
> > > drm_writeback_connector_init()
> > >  * function.
> > > +*
> > > +* For some vendor drivers, the hardware resources are shared 
> > > between
> > > +* writeback encoder and rest of the display pipeline.
> > > +* To accommodate such cases, encoder is a handle to the real 
> > > encoder
> > > +* hardware.
> > > +*
> > > +* For current existing writeback users, this shall continue to 
> > > be the
> > > +* embedded encoder for the writeback connector.
> > > +*
> > >  */
> > > -   struct drm_encoder encoder;
> > > +   struct drm_encoder *encoder;
> > >
> > > /**
> > >  * @pixel_formats_blob_ptr:
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Freedreno] [PATCH 2/3] drm/msm/gpu: Park scheduler threads for system suspend

[Daniel Vetter]

On Thu, Mar 10, 2022 at 03:46:05PM -0800, Rob Clark wrote:
> From: Rob Clark 
> 
> In the system suspend path, we don't want to be racing with the
> scheduler kthreads pushing additional queued up jobs to the hw
> queue (ringbuffer).  So park them first.  While we are at it,
> move the wait for active jobs to complete into the new system-
> suspend path.
> 
> Signed-off-by: Rob Clark 
> ---
>  drivers/gpu/drm/msm/adreno/adreno_device.c | 68 --
>  1 file changed, 64 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c 
> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 8859834b51b8..0440a98988fc 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -619,22 +619,82 @@ static int active_submits(struct msm_gpu *gpu)
>  static int adreno_runtime_suspend(struct device *dev)
>  {
>   struct msm_gpu *gpu = dev_to_gpu(dev);
> - int remaining;
> +
> + /*
> +  * We should be holding a runpm ref, which will prevent
> +  * runtime suspend.  In the system suspend path, we've
> +  * already waited for active jobs to complete.
> +  */
> + WARN_ON_ONCE(gpu->active_submits);
> +
> + return gpu->funcs->pm_suspend(gpu);
> +}
> +
> +static void suspend_scheduler(struct msm_gpu *gpu)
> +{
> + int i;
> +
> + /*
> +  * Shut down the scheduler before we force suspend, so that
> +  * suspend isn't racing with scheduler kthread feeding us
> +  * more work.
> +  *
> +  * Note, we just want to park the thread, and let any jobs
> +  * that are already on the hw queue complete normally, as
> +  * opposed to the drm_sched_stop() path used for handling
> +  * faulting/timed-out jobs.  We can't really cancel any jobs
> +  * already on the hw queue without racing with the GPU.
> +  */
> + for (i = 0; i < gpu->nr_rings; i++) {
> + struct drm_gpu_scheduler *sched = >rb[i]->sched;
> + kthread_park(sched->thread);

Shouldn't we have some proper interfaces for this? Also I'm kinda
wondering how other drivers do this, feels like we should have a standard
way.

Finally not flushing out all in-flight requests sounds a bit like a bad
idea for system suspend/resume since that's also the hibernation path, and
that would mean your shrinker/page reclaim stops working. At least in full
generality. Which ain't good for hibernation.

Adding Christian and Andrey.
-Daniel

> + }
> +}
> +
> +static void resume_scheduler(struct msm_gpu *gpu)
> +{
> + int i;
> +
> + for (i = 0; i < gpu->nr_rings; i++) {
> + struct drm_gpu_scheduler *sched = >rb[i]->sched;
> + kthread_unpark(sched->thread);
> + }
> +}
> +
> +static int adreno_system_suspend(struct device *dev)
> +{
> + struct msm_gpu *gpu = dev_to_gpu(dev);
> + int remaining, ret;
> +
> + suspend_scheduler(gpu);
>  
>   remaining = wait_event_timeout(gpu->retire_event,
>  active_submits(gpu) == 0,
>  msecs_to_jiffies(1000));
>   if (remaining == 0) {
>   dev_err(dev, "Timeout waiting for GPU to suspend\n");
> - return -EBUSY;
> + ret = -EBUSY;
> + goto out;
>   }
>  
> - return gpu->funcs->pm_suspend(gpu);
> + ret = pm_runtime_force_suspend(dev);
> +out:
> + if (ret)
> + resume_scheduler(gpu);
> +
> + return ret;
>  }
> +
> +static int adreno_system_resume(struct device *dev)
> +{
> + resume_scheduler(dev_to_gpu(dev));
> + return pm_runtime_force_resume(dev);
> +}
> +
>  #endif
>  
>  static const struct dev_pm_ops adreno_pm_ops = {
> - SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, 
> pm_runtime_force_resume)
> + SET_SYSTEM_SLEEP_PM_OPS(adreno_system_suspend, adreno_system_resume)
>   SET_RUNTIME_PM_OPS(adreno_runtime_suspend, adreno_runtime_resume, NULL)
>  };
>  
> -- 
> 2.35.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Freedreno] [PATCH v3 1/3] drm: allow real encoder to be passed for drm_writeback_connector

[Laurent Pinchart]

Hi Abhinav,

Thank you for the patch.

On Wed, Mar 16, 2022 at 11:48:16AM -0700, Abhinav Kumar wrote:
> 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 using a new
> drm_writeback_connector_init_with_encoder() API.

The commit message doesn't match the commit.

> In addition, to preserve the same call flows for the existing
> users of drm_writeback_connector_init(), also allow passing
> possible_crtcs as a parameter so that encoder can be initialized
> with it.
> 
> changes in v3:
>   - allow passing possible_crtcs for existing users of
> drm_writeback_connector_init()
>   - squash the vendor changes into the same commit so
> that each patch in the series can compile individually
> 
> Co-developed-by: Kandpal Suraj 
> Signed-off-by: Abhinav Kumar 
> ---
>  .../drm/arm/display/komeda/komeda_wb_connector.c   |   3 +-
>  drivers/gpu/drm/arm/malidp_mw.c|   5 +-
>  drivers/gpu/drm/drm_writeback.c| 103 
> +
>  drivers/gpu/drm/rcar-du/rcar_du_writeback.c|   5 +-
>  drivers/gpu/drm/vc4/vc4_txp.c  |  19 ++--
>  drivers/gpu/drm/vkms/vkms_writeback.c  |   3 +-
>  include/drm/drm_writeback.h|  22 -
>  7 files changed, 103 insertions(+), 57 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..b882066 100644
> --- a/drivers/gpu/drm/arm/malidp_mw.c
> +++ b/drivers/gpu/drm/arm/malidp_mw.c
> @@ -208,11 +208,12 @@ int malidp_mw_connector_init(struct drm_device *drm)
>   struct malidp_drm *malidp = drm->dev_private;
>   u32 *formats;
>   int ret, n_formats;
> + uint32_t possible_crtcs;
>  
>   if (!malidp->dev->hw->enable_memwrite)
>   return 0;
>  
> - malidp->mw_connector.encoder.possible_crtcs = 1 << 
> drm_crtc_index(>crtc);
> + possible_crtcs = 1 << drm_crtc_index(>crtc);
>   drm_connector_helper_add(>mw_connector.base,
>_mw_connector_helper_funcs);
>  
> @@ -223,7 +224,7 @@ 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, possible_crtcs);

Do you need the local variable ?

>   kfree(formats);
>   if (ret)
>   return ret;
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dccf4504..17c1471 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -149,36 +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
> - *
> - * This function creates the writeback-connector-specific properties if they
> - * have not been 

Re: [Freedreno] [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline

[Dan Carpenter]

On Wed, Mar 16, 2022 at 05:29:45PM -0700, Rob Clark wrote:
>   switch (param) {
> + case MSM_PARAM_COMM:
> + case MSM_PARAM_CMDLINE: {
> + char *str, **paramp;
> +
> + str = kmalloc(len + 1, GFP_KERNEL);

if (!str)
return -ENOMEM;

> + if (copy_from_user(str, u64_to_user_ptr(value), len)) {
> + kfree(str);
> + return -EFAULT;
> + }
> +
> + /* Ensure string is null terminated: */
> + str[len] = '\0';
> +
> + if (param == MSM_PARAM_COMM) {
> + paramp = >comm;
> + } else {
> + paramp = >cmdline;
> + }
> +
> + kfree(*paramp);
> + *paramp = str;
> +
> + return 0;
> + }
>   case MSM_PARAM_SYSPROF:
>   if (!capable(CAP_SYS_ADMIN))
>   return -EPERM;
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 4ec62b601adc..68f3f8ade76d 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu);
>  
>  static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm, 
> char **cmd)
>  {
> + struct msm_file_private *ctx = submit->queue->ctx;
>   struct task_struct *task;
>  
> + *comm = kstrdup(ctx->comm, GFP_KERNEL);
> + *cmd  = kstrdup(ctx->cmdline, GFP_KERNEL);
> +
>   task = get_pid_task(submit->pid, PIDTYPE_PID);
>   if (!task)
>   return;
>  
> - *comm = kstrdup(task->comm, GFP_KERNEL);
> - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
> + if (!*comm)
> + *comm = kstrdup(task->comm, GFP_KERNEL);

What?

If the first allocation failed, then this one is going to fail as well.
Just return -ENOMEM.  Or maybe this is meant to be checking for an empty
string?

> +
> + if (!*cmd)
> + *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);

Same.

>  
>   put_task_struct(task);
>  }

regards,
dan carpenter

Re: [Freedreno] [PATCH 00/25] drm/msm/dpu: wide planes support

[Dmitry Baryshkov]

On 17/03/2022 04:10, Abhinav Kumar wrote:

Hi Dmitry

I have reviewed the series , some patches completely , some of them 
especially the plane to sspp mapping is something i still need to check.


But I had one question on the design.

I thought we were going to have a boot param to control whether driver 
will internally use both rectangles for the layer so that in the future 
if compositors can do this splitting, we can use that instead of driver 
doing it ( keep boot param disabled ? ).


No need to for this patch series. If your composer allocates smaller 
planes, then the driver won't do a thing. For the proper multirect there 
will be a boot param (at least initially) and then you can work on the 
custom properties, etc.




Thanks

Abhinav

On 2/9/2022 9:24 AM, Dmitry Baryshkov wrote:

It took me a way longer to finish than I expected. And more patches that
I initially hoped. This patchset brings in multirect usage to support
using two SSPP rectangles for a single plane. Virtual planes support is
omitted from this pull request, it will come later.

Dmitry Baryshkov (25):
   drm/msm/dpu: rip out master planes support
   drm/msm/dpu: do not limit the zpos property
   drm/msm/dpu: add support for SSPP allocation to RM
   drm/msm/dpu: move SSPP debugfs creation to dpu_kms.c
   drm/msm/dpu: move pipe_hw to dpu_plane_state
   drm/msm/dpu: inline dpu_plane_get_ctl_flush
   drm/msm/dpu: drop dpu_plane_pipe function
   drm/msm/dpu: get rid of cached flush_mask
   drm/msm/dpu: dpu_crtc_blend_setup: split mixer and ctl logic
   drm/msm/dpu: introduce struct dpu_sw_pipe
   drm/msm/dpu: use dpu_sw_pipe for dpu_hw_sspp callbacks
   drm/msm/dpu: inline _dpu_plane_set_scanout
   drm/msm/dpu: pass dpu_format to _dpu_hw_sspp_setup_scaler3()
   drm/msm/dpu: move stride programming to
 dpu_hw_sspp_setup_sourceaddress
   drm/msm/dpu: remove dpu_hw_fmt_layout from struct dpu_hw_pipe_cfg
   drm/msm/dpu: drop EAGAIN check from dpu_format_populate_layout
   drm/msm/dpu: drop src_split and multirect check from
 dpu_crtc_atomic_check
   drm/msm/dpu: move the rest of plane checks to dpu_plane_atomic_check()
   drm/msm/dpu: don't use unsupported blend stages
   drm/msm/dpu: add dpu_hw_pipe_cfg to dpu_plane_state
   drm/msm/dpu: simplify dpu_plane_validate_src()
   drm/msm/dpu: rewrite plane's QoS-related functions to take dpu_sw_pipe
 and dpu_format
   drm/msm/dpu: rework dpu_plane_atomic_check() and
 dpu_plane_sspp_atomic_update()
   drm/msm/dpu: populate SmartDMA features in hw catalog
   drm/msm/dpu: add support for wide planes

  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 355 +++-
  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h  |   1 -
  drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c   |   4 -
  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c    |  10 +-
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c    |  78 +-
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h    |  35 +-
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c   | 136 +--
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h   |  88 +-
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  21 +-
  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h   |   1 +
  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 813 +-
  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |  42 +-
  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c    |  81 ++
  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h    |   6 +
  drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h |  19 +-
  15 files changed, 827 insertions(+), 863 deletions(-)




--
With best wishes
Dmitry