date:20230227

Add GSC engine based method for sending PXP firmware packets
to the GSC firmware for MTL (and future) products.

Use the newly added helpers to populate the GSC-CS memory
header and send the message packet to the FW by dispatching
the GSC_HECI_CMD_PKT instruction on the GSC engine.

We use non-priveleged batches for submission to GSC engine
which require two buffers for the request:
 - a buffer for the HECI packet that contains PXP FW commands
 - a batch-buffer that contains the engine instruction for
   sending the HECI packet to the GSC firmware.

Thus, add the allocation and freeing of these buffers in gsccs
init and fini.

The GSC-fw may reply to commands with a SUCCESS but with an
additional pending-bit set in the reply packet. This bit
means the GSC-FW is currently busy and the caller needs to
try again with the gsc_message_handle the fw gave. The
GSC-fw requires a non-zero host_session_handle provided
by the caller to enable gsc_message_handle tracking.

Thus, allocate the host_session_handle at init and destroy it
at fini (the latter requiring an FYI to the gsc-firmware).
Also ensure the send-message function allows replay of the
gsc_message_handle.

Signed-off-by: Alan Previn 
---
 .../drm/i915/pxp/intel_pxp_cmd_interface_43.h |   4 +
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c| 239 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h|   4 +
 drivers/gpu/drm/i915/pxp/intel_pxp_types.h|   6 +
 4 files changed, 251 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h 
b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
index ad67e3f49c20..b2523d6918c7 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
@@ -12,6 +12,10 @@
 /* PXP-Cmd-Op definitions */
 #define PXP43_CMDID_START_HUC_AUTH 0x003A
 
+/* PXP-Packet sizes for MTL's GSCCS-HECI instruction */
+#define PXP43_MAX_HECI_IN_SIZE (SZ_32K)
+#define PXP43_MAX_HECI_OUT_SIZE (SZ_32K)
+
 /* PXP-Input-Packet: HUC-Authentication */
 struct pxp43_start_huc_auth_in {
struct pxp_cmd_header header;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
index 13693e78b57e..30aa660a975f 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
@@ -6,19 +6,226 @@
 #include "gem/i915_gem_internal.h"
 
 #include "gt/intel_context.h"
+#include "gt/uc/intel_gsc_uc_heci_cmd_submit.h"
 
 #include "i915_drv.h"
 #include "intel_pxp_cmd_interface_43.h"
 #include "intel_pxp_gsccs.h"
 #include "intel_pxp_types.h"
 
+static int
+gsccs_send_message(struct intel_pxp *pxp,
+  void *msg_in, size_t msg_in_size,
+  void *msg_out, size_t msg_out_size_max,
+  size_t *msg_out_len,
+  u64 *gsc_msg_handle_retry)
+{
+   struct intel_gt *gt = pxp->ctrl_gt;
+   struct drm_i915_private *i915 = gt->i915;
+   struct gsccs_session_resources *exec =  &pxp->gsccs_res;
+   struct intel_gsc_mtl_header *header = exec->pkt_vaddr;
+   struct intel_gsc_heci_non_priv_pkt pkt;
+   bool null_pkt = !msg_in && !msg_out;
+   size_t max_msg_size;
+   u32 reply_size;
+   int ret;
+
+   if (!exec->ce)
+   return -ENODEV;
+
+   max_msg_size = PXP43_MAX_HECI_IN_SIZE - sizeof(*header);
+
+   if (msg_in_size > max_msg_size || msg_out_size_max > max_msg_size)
+   return -ENOSPC;
+
+   if (!exec->pkt_vma || !exec->bb_vma)
+   return -ENOENT;
+
+   mutex_lock(&pxp->tee_mutex);
+
+   memset(header, 0, sizeof(*header));
+   intel_gsc_uc_heci_cmd_emit_mtl_header(header, GSC_HECI_MEADDRESS_PXP,
+ msg_in_size + sizeof(*header),
+ exec->host_session_handle);
+
+   /* check if this is a host-session-handle cleanup call */
+   if (null_pkt)
+   header->flags |= GSC_HECI_FLAG_MSG_CLEANUP;
+
+   /* copy caller provided gsc message handle if this is polling for a 
prior msg completion */
+   header->gsc_message_handle = *gsc_msg_handle_retry;
+
+   /* NOTE: zero size packets are used for session-cleanups */
+   if (msg_in && msg_in_size)
+   memcpy(exec->pkt_vaddr + sizeof(*header), msg_in, msg_in_size);
+
+   pkt.addr_in = i915_vma_offset(exec->pkt_vma);
+   pkt.size_in = header->message_size;
+   pkt.addr_out = pkt.addr_in + PXP43_MAX_HECI_IN_SIZE;
+   pkt.size_out = msg_out_size_max + sizeof(*header);
+   pkt.heci_pkt_vma = exec->pkt_vma;
+   pkt.bb_vma = exec->bb_vma;
+
+   ret = intel_gsc_uc_heci_cmd_submit_nonpriv(>->uc.gsc,
+  exec->ce, &pkt, 
exec->bb_vaddr,
+  GSC_REPLY_LATENCY_MS);
+   if (ret) {
+   drm_err(&i915->drm, "fai

[PATCH v6 7/8] drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component

On legacy platforms, KCR HW enabling is done at the time the mei
component interface is bound. It's also disabled during unbind.
However, for MTL onwards, we don't depend on a tee component
to start sending GSC-CS firmware messages.

Thus, immediately enable (or disable) KCR HW on PXP's init,
fini and resume.

Signed-off-by: Alan Previn 
---
 drivers/gpu/drm/i915/pxp/intel_pxp.c| 19 +++
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c |  3 ++-
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 61041277be24..e2f2cc5f6a6e 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -119,6 +119,7 @@ static void destroy_vcs_context(struct intel_pxp *pxp)
 static void pxp_init_full(struct intel_pxp *pxp)
 {
struct intel_gt *gt = pxp->ctrl_gt;
+   intel_wakeref_t wakeref;
int ret;
 
/*
@@ -140,10 +141,15 @@ static void pxp_init_full(struct intel_pxp *pxp)
if (ret)
return;
 
-   if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
+   if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) {
ret = intel_pxp_gsccs_init(pxp);
-   else
+   if (!ret) {
+   with_intel_runtime_pm(&pxp->ctrl_gt->i915->runtime_pm, 
wakeref)
+   intel_pxp_init_hw(pxp);
+   }
+   } else {
ret = intel_pxp_tee_component_init(pxp);
+   }
if (ret)
goto out_context;
 
@@ -239,15 +245,20 @@ int intel_pxp_init(struct drm_i915_private *i915)
 
 void intel_pxp_fini(struct drm_i915_private *i915)
 {
+   intel_wakeref_t wakeref;
+
if (!i915->pxp)
return;
 
i915->pxp->arb_is_valid = false;
 
-   if (HAS_ENGINE(i915->pxp->ctrl_gt, GSC0))
+   if (HAS_ENGINE(i915->pxp->ctrl_gt, GSC0)) {
+   with_intel_runtime_pm(&i915->pxp->ctrl_gt->i915->runtime_pm, 
wakeref)
+   intel_pxp_fini_hw(i915->pxp);
intel_pxp_gsccs_fini(i915->pxp);
-   else
+   } else {
intel_pxp_tee_component_fini(i915->pxp);
+   }
 
destroy_vcs_context(i915->pxp);
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
index 4f836b317424..1a04067f61fc 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
@@ -43,8 +43,9 @@ void intel_pxp_resume_complete(struct intel_pxp *pxp)
 * The PXP component gets automatically unbound when we go into S3 and
 * re-bound after we come out, so in that scenario we can defer the
 * hw init to the bind call.
+* NOTE: GSC-CS backend doesn't rely on components.
 */
-   if (!pxp->pxp_component)
+   if (!HAS_ENGINE(pxp->ctrl_gt, GSC0) && !pxp->pxp_component)
return;
 
intel_pxp_init_hw(pxp);
-- 
2.39.0

[PATCH v6 8/8] drm/i915/pxp: Enable PXP with MTL-GSC-CS

Enable PXP with MTL-GSC-CS: add the has_pxp into device info
and increase the debugfs teardown timeouts to align with
new GSC-CS + firmware specs.

Signed-off-by: Alan Previn 
---
 drivers/gpu/drm/i915/i915_pci.c  | 1 +
 drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 9 -
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 2 +-
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index a8d942b16223..4ecf0f2ab6ec 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -1150,6 +1150,7 @@ static const struct intel_device_info mtl_info = {
.has_guc_deprivilege = 1,
.has_mslice_steering = 0,
.has_snoop = 1,
+   .has_pxp = 1,
.__runtime.memory_regions = REGION_SMEM | REGION_STOLEN_LMEM,
.__runtime.platform_engine_mask = BIT(RCS0) | BIT(BCS0) | BIT(CCS0),
.require_force_probe = 1,
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
index 9f6e300486b4..ddf9f8bb7791 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
@@ -14,6 +14,7 @@
 
 #include "intel_pxp.h"
 #include "intel_pxp_debugfs.h"
+#include "intel_pxp_gsccs.h"
 #include "intel_pxp_irq.h"
 #include "intel_pxp_types.h"
 
@@ -45,6 +46,7 @@ static int pxp_terminate_set(void *data, u64 val)
 {
struct intel_pxp *pxp = data;
struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
+   int timeout_ms;
 
if (!intel_pxp_is_active(pxp))
return -ENODEV;
@@ -54,8 +56,13 @@ static int pxp_terminate_set(void *data, u64 val)
intel_pxp_irq_handler(pxp, 
GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT);
spin_unlock_irq(gt->irq_lock);
 
+   if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
+   timeout_ms = GSC_PENDING_RETRY_LATENCY_MS;
+   else
+   timeout_ms = 250;
+
if (!wait_for_completion_timeout(&pxp->termination,
-msecs_to_jiffies(100)))
+msecs_to_jiffies(timeout_ms)))
return -ETIMEDOUT;
 
return 0;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
index 4ddf2ee60222..03f006f9dc2e 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -44,7 +44,7 @@ static int pxp_wait_for_session_state(struct intel_pxp *pxp, 
u32 id, bool in_pla
  KCR_SIP(pxp->kcr_base),
  mask,
  in_play ? mask : 0,
- 100);
+ 250);
 
intel_runtime_pm_put(uncore->rpm, wakeref);
 
-- 
2.39.0

[PATCH v6 3/8] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC

Add helper functions into a new file for heci-packet-submission.
The helpers will handle generating the MTL GSC-CS Memory-Header
and submission of the Heci-Cmd-Packet instructions to the engine.

NOTE1: These common functions for heci-packet-submission will be used
by different i915 callers:
 1- GSC-SW-Proxy: This is pending upstream publication awaiting
a few remaining opens
 2- MTL-HDCP: An equivalent patch has also been published at:
https://patchwork.freedesktop.org/series/111876/. (Patch 1)
 3- PXP: This series.

NOTE2: A difference in this patch vs what is appearing is in bullet 2
above is that HDCP (and SW-Proxy) will be using priveleged submission
(GGTT and common gsc-uc-context) while PXP will be using non-priveleged
PPGTT, context and batch buffer. Therefore this patch will only slightly
overlap with the MTL-HDCP patches despite have very similar function
names (emit_foo vs emit_nonpriv_foo). This is because HECI_CMD_PKT
instructions require different flows and hw-specific code when done
via PPGTT based submission (not different from other engines). MTL-HDCP
contains the same intel_gsc_mtl_header_t structures as this but the
helpers there are different. Both add the same new file names.

NOTE3: Additional clarity about the heci-cmd-pkt layout and where the
   common helpers come in:
 - On MTL, when an i915 subsystem needs to send a command request
   to the security firmware, it will send that via the GSC-
   engine-command-streamer.
 - However those commands, (lets call them "gsc_specific_fw_api"
   calls), are not understood by the GSC command streamer hw.
 - The GSC CS only looks at the GSC_HECI_CMD_PKT instruction and
   passes it along to the GSC firmware.
 - The GSC FW on the other hand needs additional metadata to know
   which usage service is being called (PXP, HDCP, proxy, etc) along
   with session specific info. Thus an extra header called GSC-CS
   HECI Memory Header, (C) in below diagram is prepended before
   the FW specific API, (D).
 - Thus, the structural layout of the request submitted would
   need to look like the diagram below (for non-priv PXP).
 - In the diagram, the common helper for HDCP, (GSC-Sw-Proxy) and
   PXP (i.e. new function intel_gsc_uc_heci_cmd_emit_mtl_header)
   will populate blob (C) while additional helpers, different for
   PPGGTT (this patch) vs GGTT (HDCP series) will populate
   blobs (A) and (B) below.
  ___
 (A)  |  MI_BATCH_BUFFER_START (ppgtt, batchbuff-addr, ...) |
  | |   |
  |_|   |
  | (B)| GSC_HECI_CMD_PKT (pkt-addr-in, pkt-size-in,|   |
  ||   pkt-addr-out, pkt-size-out)  |
  || MI_BATCH_BUFFER_END|   |   |
  |||   |   |
  | |   |
  |_|   |
|
-
|
   \|/
  __V___
  |   _|
  |(C)|   ||
  |   | struct intel_gsc_mtl_header { ||
  |   |   validity marker ||
  |   |   heci_clent_id   ||
  |   |   ... ||
  |   |  }||
  |   |___||
  |(D)|   ||
  |   | struct gsc_fw_specific_api_foobar {   ||
  |   | ...   ||
  |   | For an example, see   ||
  |   | 'struct pxp43_create_arb_in' at   ||
  |   | intel_pxp_cmd_interface_43.h  ||
  |   |   ||
  |   | } ||
  |   |  Struture depends on command type ||
  |   | struct gsc_fw_specific_api_foobar {   ||
  |   |___||
  ||

That said, this patch provides basic helpers but leaves the
PXP subsystem (i.e. the caller) to handle (D) and everything
else such as input/output size verification or handling the
responses from security firmware (for example, requiring a retry).

Signed-off-by: Alan Previn 
---
 drivers/gpu/drm/i915/Makefile |   1 +
 drivers/gpu/drm/i915/gt/intel_gpu_commands.h  |   2 +
 .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c | 108 +

[PATCH v6 2/8] drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation

Add MTL hw-plumbing enabling for KCR operation under PXP
which includes:

1. Updating 'pick-gt' to get the media tile for
   KCR interrupt handling
2. Adding MTL's KCR registers for PXP operation
   (init, status-checking, etc.).

While doing #2, lets create a separate registers header file for PXP
to be consistent with other i915 global subsystems.

Signed-off-by: Alan Previn 
Reviewed-by: Daniele Ceraolo Spurio 
---
 drivers/gpu/drm/i915/gt/intel_gt_irq.c   |  3 +-
 drivers/gpu/drm/i915/pxp/intel_pxp.c | 32 
 drivers/gpu/drm/i915/pxp/intel_pxp_regs.h| 27 +
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 12 +++-
 drivers/gpu/drm/i915/pxp/intel_pxp_types.h   |  6 
 5 files changed, 58 insertions(+), 22 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_regs.h

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c 
b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
index 1b25a6039152..c360776a98b5 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
@@ -100,7 +100,8 @@ static struct intel_gt *pick_gt(struct intel_gt *gt, u8 
class, u8 instance)
case VIDEO_ENHANCEMENT_CLASS:
return media_gt;
case OTHER_CLASS:
-   if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, 
GSC0))
+   if ((instance == OTHER_GSC_INSTANCE || instance == 
OTHER_KCR_INSTANCE) &&
+   HAS_ENGINE(media_gt, GSC0))
return media_gt;
fallthrough;
default:
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 560d94d23114..aecc65b5da70 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -14,6 +14,7 @@
 #include "intel_pxp.h"
 #include "intel_pxp_gsccs.h"
 #include "intel_pxp_irq.h"
+#include "intel_pxp_regs.h"
 #include "intel_pxp_session.h"
 #include "intel_pxp_tee.h"
 #include "intel_pxp_types.h"
@@ -61,21 +62,22 @@ bool intel_pxp_is_active(const struct intel_pxp *pxp)
return IS_ENABLED(CONFIG_DRM_I915_PXP) && pxp && pxp->arb_is_valid;
 }
 
-/* KCR register definitions */
-#define KCR_INIT _MMIO(0x320f0)
-/* Setting KCR Init bit is required after system boot */
-#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES REG_BIT(14)
+static void kcr_pxp_set_status(const struct intel_pxp *pxp, bool enable)
+{
+   u32 val = enable ? _MASKED_BIT_ENABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES) 
:
+ _MASKED_BIT_DISABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES);
+
+   intel_uncore_write(pxp->ctrl_gt->uncore, KCR_INIT(pxp->kcr_base), val);
+}
 
-static void kcr_pxp_enable(struct intel_gt *gt)
+static void kcr_pxp_enable(const struct intel_pxp *pxp)
 {
-   intel_uncore_write(gt->uncore, KCR_INIT,
-  
_MASKED_BIT_ENABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES));
+   kcr_pxp_set_status(pxp, true);
 }
 
-static void kcr_pxp_disable(struct intel_gt *gt)
+static void kcr_pxp_disable(const struct intel_pxp *pxp)
 {
-   intel_uncore_write(gt->uncore, KCR_INIT,
-  
_MASKED_BIT_DISABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES));
+   kcr_pxp_set_status(pxp, false);
 }
 
 static int create_vcs_context(struct intel_pxp *pxp)
@@ -127,6 +129,11 @@ static void pxp_init_full(struct intel_pxp *pxp)
init_completion(&pxp->termination);
complete_all(&pxp->termination);
 
+   if (pxp->ctrl_gt->type == GT_MEDIA)
+   pxp->kcr_base = MTL_KCR_BASE;
+   else
+   pxp->kcr_base = GEN12_KCR_BASE;
+
intel_pxp_session_management_init(pxp);
 
ret = create_vcs_context(pxp);
@@ -368,14 +375,13 @@ int intel_pxp_start(struct intel_pxp *pxp)
 
 void intel_pxp_init_hw(struct intel_pxp *pxp)
 {
-   kcr_pxp_enable(pxp->ctrl_gt);
+   kcr_pxp_enable(pxp);
intel_pxp_irq_enable(pxp);
 }
 
 void intel_pxp_fini_hw(struct intel_pxp *pxp)
 {
-   kcr_pxp_disable(pxp->ctrl_gt);
-
+   kcr_pxp_disable(pxp);
intel_pxp_irq_disable(pxp);
 }
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_regs.h 
b/drivers/gpu/drm/i915/pxp/intel_pxp_regs.h
new file mode 100644
index ..a9e7e6efa4c7
--- /dev/null
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_regs.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright(c) 2023, Intel Corporation. All rights reserved.
+ */
+
+#ifndef __INTEL_PXP_REGS_H__
+#define __INTEL_PXP_REGS_H__
+
+#include "i915_reg_defs.h"
+
+/* KCR subsystem register base address */
+#define GEN12_KCR_BASE 0x32000
+#define MTL_KCR_BASE 0x386000
+
+/* KCR enable/disable control */
+#define KCR_INIT(base) _MMIO((base) + 0xf0)
+
+/* Setting KCR Init bit is required after system boot */
+#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES REG_BIT(14)
+
+/* KCR hwdrm session in play status 0-31 */
+#define KCR_SIP(base) _MMIO((base) + 0x260)
+
+/* PXP global terminate register for session termination */
+#define KCR_GLOBAL_TERMINATE(bas

[PATCH v6 5/8] drm/i915/pxp: Add ARB session creation and cleanup

Add MTL's function for ARB session creation using PXP firmware
version 4.3 ABI structure format.

Also add MTL's function for ARB session invalidation but this
reuses PXP firmware version 4.2 ABI structure format.

Before checking the return status, look at the GSC-CS-Mem-Header's
pending-bit which means the GSC firmware is busy and we should
resubmit.

Signed-off-by: Alan Previn 
---
 drivers/gpu/drm/i915/pxp/intel_pxp.c  | 34 --
 .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 21 +++
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c| 62 +++
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h|  4 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c  | 11 +++-
 5 files changed, 126 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index aecc65b5da70..61041277be24 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -290,6 +290,8 @@ static bool pxp_component_bound(struct intel_pxp *pxp)
 
 static int __pxp_global_teardown_final(struct intel_pxp *pxp)
 {
+   int timeout;
+
if (!pxp->arb_is_valid)
return 0;
/*
@@ -299,7 +301,12 @@ static int __pxp_global_teardown_final(struct intel_pxp 
*pxp)
intel_pxp_mark_termination_in_progress(pxp);
intel_pxp_terminate(pxp, false);
 
-   if (!wait_for_completion_timeout(&pxp->termination, 
msecs_to_jiffies(250)))
+   if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
+   timeout = GSC_PENDING_RETRY_LATENCY_MS;
+   else
+   timeout = 250;
+
+   if (!wait_for_completion_timeout(&pxp->termination, 
msecs_to_jiffies(timeout)))
return -ETIMEDOUT;
 
return 0;
@@ -307,6 +314,8 @@ static int __pxp_global_teardown_final(struct intel_pxp 
*pxp)
 
 static int __pxp_global_teardown_restart(struct intel_pxp *pxp)
 {
+   int timeout;
+
if (pxp->arb_is_valid)
return 0;
/*
@@ -315,7 +324,12 @@ static int __pxp_global_teardown_restart(struct intel_pxp 
*pxp)
 */
pxp_queue_termination(pxp);
 
-   if (!wait_for_completion_timeout(&pxp->termination, 
msecs_to_jiffies(250)))
+   if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
+   timeout = GSC_PENDING_RETRY_LATENCY_MS;
+   else
+   timeout = 250;
+
+   if (!wait_for_completion_timeout(&pxp->termination, 
msecs_to_jiffies(timeout)))
return -ETIMEDOUT;
 
return 0;
@@ -353,8 +367,20 @@ int intel_pxp_start(struct intel_pxp *pxp)
if (!intel_pxp_is_enabled(pxp))
return -ENODEV;
 
-   if (wait_for(pxp_component_bound(pxp), 250))
-   return -ENXIO;
+   if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) {
+   /*
+* GSC-fw loading, GSC-proxy init (requiring an mei component 
driver) and
+* HuC-fw loading must all occur first before we start 
requesting for PXP
+* sessions. Checking HuC authentication (the last dependency)  
will suffice.
+* Let's use a much larger 8 second timeout considering all the 
types of
+* dependencies prior to that.
+*/
+   if (wait_for(intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc), 
8000))
+   return -ENXIO;
+   } else {
+   if (wait_for(pxp_component_bound(pxp), 250))
+   return -ENXIO;
+   }
 
mutex_lock(&pxp->arb_mutex);
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h 
b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
index b2523d6918c7..9089e02a8c2d 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
@@ -11,6 +11,7 @@
 
 /* PXP-Cmd-Op definitions */
 #define PXP43_CMDID_START_HUC_AUTH 0x003A
+#define PXP43_CMDID_INIT_SESSION 0x0036
 
 /* PXP-Packet sizes for MTL's GSCCS-HECI instruction */
 #define PXP43_MAX_HECI_IN_SIZE (SZ_32K)
@@ -27,4 +28,24 @@ struct pxp43_start_huc_auth_out {
struct pxp_cmd_header header;
 } __packed;
 
+/* PXP-Input-Packet: Init PXP session */
+struct pxp43_create_arb_in {
+   struct pxp_cmd_header header;
+   /* header.stream_id fields for vesion 4.3 of Init PXP session: 
*/
+   #define PXP43_INIT_SESSION_VALID BIT(0)
+   #define PXP43_INIT_SESSION_APPTYPE BIT(1)
+   #define PXP43_INIT_SESSION_APPID GENMASK(17, 2)
+   u32 protection_mode;
+   #define PXP43_INIT_SESSION_PROTECTION_ARB 0x2
+   u32 sub_session_id;
+   u32 init_flags;
+   u32 rsvd[12];
+} __packed;
+
+/* PXP-Input-Packet: Init PXP session */
+struct pxp43_create_arb_out {
+   struct pxp_cmd_header header;
+   u32 rsvd[8];
+} __packed;
+
 #endif /* __INTEL_PXP_FW_INTERFACE_43_H__ */
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp_gscc

[PATCH v6 6/8] drm/i915/pxp: MTL-KCR interrupt ctrl's are in GT-0

Despite KCR subsystem being in the media-tile (close to the
GSC-CS), the IRQ controls for it are on GT-0 with other global
IRQ controls. Thus, add a helper for KCR hw interrupt
enable/disable functions to get the correct gt structure (for
uncore) for MTL.

In the helper, we get GT-0's handle for uncore when touching
IRQ registers despite the pxp->ctrl_gt being the media-tile.
No difference for legacy of course.

Signed-off-by: Alan Previn 
---
 drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  2 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 24 +---
 drivers/gpu/drm/i915/pxp/intel_pxp_irq.h |  8 +++
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
index 4b8e70caa3ad..9f6e300486b4 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
@@ -44,7 +44,7 @@ static int pxp_terminate_get(void *data, u64 *val)
 static int pxp_terminate_set(void *data, u64 val)
 {
struct intel_pxp *pxp = data;
-   struct intel_gt *gt = pxp->ctrl_gt;
+   struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
 
if (!intel_pxp_is_active(pxp))
return -ENODEV;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
index 91e9622c07d0..3a725397349f 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
@@ -4,10 +4,12 @@
  */
 #include 
 
+#include "gt/intel_gt.h"
 #include "gt/intel_gt_irq.h"
 #include "gt/intel_gt_regs.h"
 #include "gt/intel_gt_types.h"
 
+#include "i915_drv.h"
 #include "i915_irq.h"
 #include "i915_reg.h"
 
@@ -17,6 +19,22 @@
 #include "intel_pxp_types.h"
 #include "intel_runtime_pm.h"
 
+/**
+ * intel_pxp_get_irq_gt - Find the correct GT that owns KCR interrupts
+ * @pxp: pointer to pxp struct
+ *
+ * For platforms with a single GT, we return the pxp->ctrl_gt (as expected)
+ * but for MTL+ that has a media-tile, although the KCR engine is in the
+ * media-tile (i.e. pxp->ctrl_gt), the IRQ controls are on the root tile.
+ * In the end, we don't use pxp->ctrl_gt for IRQ, we always return root gt.
+ */
+struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp)
+{
+   WARN_ON_ONCE(!pxp->ctrl_gt->i915->media_gt && 
!gt_is_root(pxp->ctrl_gt));
+
+   return to_gt(pxp->ctrl_gt->i915);
+}
+
 /**
  * intel_pxp_irq_handler - Handles PXP interrupts.
  * @pxp: pointer to pxp struct
@@ -29,7 +47,7 @@ void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
if (GEM_WARN_ON(!intel_pxp_is_enabled(pxp)))
return;
 
-   gt = pxp->ctrl_gt;
+   gt = intel_pxp_get_irq_gt(pxp);
 
lockdep_assert_held(gt->irq_lock);
 
@@ -68,7 +86,7 @@ static inline void pxp_irq_reset(struct intel_gt *gt)
 
 void intel_pxp_irq_enable(struct intel_pxp *pxp)
 {
-   struct intel_gt *gt = pxp->ctrl_gt;
+   struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
 
spin_lock_irq(gt->irq_lock);
 
@@ -83,7 +101,7 @@ void intel_pxp_irq_enable(struct intel_pxp *pxp)
 
 void intel_pxp_irq_disable(struct intel_pxp *pxp)
 {
-   struct intel_gt *gt = pxp->ctrl_gt;
+   struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
 
/*
 * We always need to submit a global termination when we re-enable the
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h 
b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
index 8c292dc86f68..eea87c9eb62b 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
@@ -9,6 +9,7 @@
 #include 
 
 struct intel_pxp;
+struct intel_gt;
 
 #define GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT BIT(1)
 #define GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT BIT(2)
@@ -23,6 +24,8 @@ struct intel_pxp;
 void intel_pxp_irq_enable(struct intel_pxp *pxp);
 void intel_pxp_irq_disable(struct intel_pxp *pxp);
 void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir);
+struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp);
+
 #else
 static inline void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
 {
@@ -35,6 +38,11 @@ static inline void intel_pxp_irq_enable(struct intel_pxp 
*pxp)
 static inline void intel_pxp_irq_disable(struct intel_pxp *pxp)
 {
 }
+
+static inline struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp)
+{
+   return NULL;
+}
 #endif
 
 #endif /* __INTEL_PXP_IRQ_H__ */
-- 
2.39.0

[PATCH v6 0/8] drm/i915/pxp: Add MTL PXP Support

This series enables PXP on MTL. On ADL/TGL platforms, we rely on
the mei driver via the i915-mei PXP component interface to establish
a connection to the security firmware via the HECI device interface.
That interface is used to create and teardown the PXP ARB session.
PXP ARB session is created when protected contexts are created.

In this series, the front end behaviors and interfaces (uapi) remain
the same. We add backend support for MTL but with MTL we directly use
the GSC-CS engine on the MTL GPU device to send messages to the PXP
(a.k.a. GSC a.k.a graphics-security) firmware. With MTL, the format
of the message is slightly different with a 2-layer packetization
that is explained in detail in Patch #3. Also, the second layer
which is the actual PXP firmware packet is now rev'd to version 4.3
for MTL that is defined in Patch #5.

Take note that Patch #4 adds the buffer allocation and gsccs-send-
message without actually being called by the arb-creation code
which gets added in Patch #5. Additionally, a seperate series being
reviewed is introducing a change for session teardown (in pxp front-
end layer that will need backend support by both legacy and gsccs).
If we squash all of these together (buffer-alloc, send-message,
arb-creation and, in future, session-termination), the single patch
will be rather large. That said, we are keeping Patch #4 and #5
separate for now, but at merge time, we can squash them together
if maintainer requires it.

Changes from prior revs:
   v1 : - fixed when building with CONFIG_PXP disabled.
- more alignment with gsc_mtl_header structure from the HDCP
   v2 : - (all following changes as per reviews from Daniele)
- squashed Patch #1 from v1 into the next one.
- replaced unnecessary "uses_gsccs" boolean in the pxp
  with "HAS_ENGINE(pxp->ctrl_gt, GSC0)".
- moved the stashing of gsccs resources from a dynamically
  allocated opaque handle to an explicit sub-struct in
  'struct intel_pxp'.
- moved the buffer object allocations from Patch #1 of this
  series to Patch #5 (but keep the context allocation in
  Patch #1).
- used the kernel default ppgtt for the gsccs context.
- optimized the buffer allocation and deallocation code
  and drop the need to stash the drm_i915_gem_object.
- use a macro with the right mmio reg base (depending
  on root-tile vs media-tile) along with common relative
  offset to access all KCR registers thus minimizing
  changes to the KCR register access codes.
- fixed bugs in the heci packet request submission code
  in Patch #3 (of this series)
- add comments in the mtl-gsc-heci-header regarding the
  host-session-handle.
- re-use tee-mutex instead of introducing a gsccs specific
  cmd mutex.
- minor cosmetic improvements in Patch #5.
- before creating arb session, ensure intel_pxp_start
  first ensures the GSC FW is up and running.
- use 2 second timeout for the pending-bit scenario when
  sending command to GSC-FW as per specs.
- simplify intel_pxp_get_irq_gt with addition comments
- redo Patch #7 to minimize the changes without introducing
  a common  abstraction helper for suspend/resume/init/fini
  codes that have to change the kcr power state.
   v3 : - rebase onto latest drm-tip with the updated firmware
  session invalidation flow
- on Patch#1: move 'mutex_init(&pxp->tee_mutex)' to a common
  init place in intel_pxp_init since its needed everywhere
  (Daniele)
- on Patch#1: remove unneccasary "ce->vm = i915_vm_get(vm);"
  (Daniele)
- on Patch#2: move the introduction of host_session_handle to
  Patch#4 where it starts getting used.
- on Patch#4: move host-session-handle initialization to the
  allocate-resources during gsccs-init (away from Patch#5)
  and add the required call to PXP-firmware to cleanup the
  host-session-handle in destroy-resources during gsccs-fini
- on Patch#5: add dispatching of arb session termination
  firmware cmd during session teardown (as per latest
  upstream flows)
   v4 : - Added proper initialization and cleanup of host-session-handle
  that the gsc firmware expects.
- Fix the stream-session-key invalidation instruction for MTL.
   v5 : - In Patch #4, move the tee_mutex locking to after we check for
  valid vma allocations.
- In Patch #5, wait for intel_huc_is_authenticated instead of
  intel_uc_fw_is_running(gsc-fw) before starting ARB session.
- In Patch #5, increase the necessary timeouts at the top-level
  (PXP arb session creation / termination) to accomodate SLA of
  GSC firmware when busy with pending-bit responses.
- In Patch #5, remove redundant host_session_handle i

[PATCH v6 1/8] drm/i915/pxp: Add GSC-CS back-end resource init and cleanup

For MTL, the PXP back-end transport uses the GSC engine to submit
HECI packets through the HW to the GSC firmware for PXP arb
session management. This submission uses a non-priveleged
batch buffer, a buffer for the command packet and of course
a context targeting the GSC-CS.

Thus for MTL, we need to allocate and free a set of execution
submission resources for the management of the arbitration session.
Lets start with the context creation first since that object and
its usage is very straight-forward. We'll add the buffer allocation
and freeing later when we introduce the gsccs' send-message function.

Do this one time allocation of gsccs specific resources in
a new gsccs source file with intel_pxp_gsccs_init / fini functions
and hook them up from the PXP front-end.

Signed-off-by: Alan Previn 
Reviewed-by: Daniele Ceraolo Spurio 
---
 drivers/gpu/drm/i915/Makefile  |  1 +
 drivers/gpu/drm/i915/pxp/intel_pxp.c   | 19 +--
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 61 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h | 29 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c   |  2 -
 drivers/gpu/drm/i915/pxp/intel_pxp_types.h |  8 +++
 6 files changed, 114 insertions(+), 6 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
 create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index b2f91a1f8268..ad5f2660808d 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -334,6 +334,7 @@ i915-y += \
 i915-$(CONFIG_DRM_I915_PXP) += \
pxp/intel_pxp_cmd.o \
pxp/intel_pxp_debugfs.o \
+   pxp/intel_pxp_gsccs.o \
pxp/intel_pxp_irq.o \
pxp/intel_pxp_pm.o \
pxp/intel_pxp_session.o
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 9d4c7724e98e..560d94d23114 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -12,6 +12,7 @@
 #include "i915_drv.h"
 
 #include "intel_pxp.h"
+#include "intel_pxp_gsccs.h"
 #include "intel_pxp_irq.h"
 #include "intel_pxp_session.h"
 #include "intel_pxp_tee.h"
@@ -132,7 +133,10 @@ static void pxp_init_full(struct intel_pxp *pxp)
if (ret)
return;
 
-   ret = intel_pxp_tee_component_init(pxp);
+   if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
+   ret = intel_pxp_gsccs_init(pxp);
+   else
+   ret = intel_pxp_tee_component_init(pxp);
if (ret)
goto out_context;
 
@@ -165,9 +169,11 @@ static struct intel_gt 
*find_gt_for_required_protected_content(struct drm_i915_p
/*
 * For MTL onwards, PXP-controller-GT needs to have a valid GSC engine
 * on the media GT. NOTE: if we have a media-tile with a GSC-engine,
-* the VDBOX is already present so skip that check
+* the VDBOX is already present so skip that check. We also have to
+* ensure the GSC firmware is coming online
 */
-   if (i915->media_gt && HAS_ENGINE(i915->media_gt, GSC0))
+   if (i915->media_gt && HAS_ENGINE(i915->media_gt, GSC0) &&
+   intel_uc_fw_is_loadable(&i915->media_gt->uc.gsc.fw))
return i915->media_gt;
 
/*
@@ -207,7 +213,9 @@ int intel_pxp_init(struct drm_i915_private *i915)
if (!i915->pxp)
return -ENOMEM;
 
+   /* init common info used by all feature-mode usages*/
i915->pxp->ctrl_gt = gt;
+   mutex_init(&i915->pxp->tee_mutex);
 
/*
 * If full PXP feature is not available but HuC is loaded by GSC on 
pre-MTL
@@ -229,7 +237,10 @@ void intel_pxp_fini(struct drm_i915_private *i915)
 
i915->pxp->arb_is_valid = false;
 
-   intel_pxp_tee_component_fini(i915->pxp);
+   if (HAS_ENGINE(i915->pxp->ctrl_gt, GSC0))
+   intel_pxp_gsccs_fini(i915->pxp);
+   else
+   intel_pxp_tee_component_fini(i915->pxp);
 
destroy_vcs_context(i915->pxp);
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c 
b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
new file mode 100644
index ..13693e78b57e
--- /dev/null
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright(c) 2023 Intel Corporation.
+ */
+
+#include "gem/i915_gem_internal.h"
+
+#include "gt/intel_context.h"
+
+#include "i915_drv.h"
+#include "intel_pxp_cmd_interface_43.h"
+#include "intel_pxp_gsccs.h"
+#include "intel_pxp_types.h"
+
+static void
+gsccs_destroy_execution_resource(struct intel_pxp *pxp)
+{
+   struct gsccs_session_resources *strm_res = &pxp->gsccs_res;
+
+   if (strm_res->ce)
+   intel_context_put(strm_res->ce);
+
+   memset(strm_res, 0, sizeof(*strm_res));
+}
+
+static int
+gsccs_allocate_execution_resource(struct intel_pxp *pxp)
+{
+   struct intel_gt *gt = pxp->ctrl_gt;
+   struct gsccs_session_resources *st

Re: [PATCH drm-next v2 05/16] drm: manager to keep track of GPUs VA mappings

2023-02-27 Thread Danilo Krummrich

On Tue, Feb 21, 2023 at 01:20:50PM -0500, Liam R. Howlett wrote:
> * Danilo Krummrich  [230217 08:45]:
> > Add infrastructure to keep track of GPU virtual address (VA) mappings
> > with a decicated VA space manager implementation.
> > 
> > New UAPIs, motivated by Vulkan sparse memory bindings graphics drivers
> > start implementing, allow userspace applications to request multiple and
> > arbitrary GPU VA mappings of buffer objects. The DRM GPU VA manager is
> > intended to serve the following purposes in this context.
> > 
> > 1) Provide infrastructure to track GPU VA allocations and mappings,
> >making use of the maple_tree.
> > 
> > 2) Generically connect GPU VA mappings to their backing buffers, in
> >particular DRM GEM objects.
> > 
> > 3) Provide a common implementation to perform more complex mapping
> >operations on the GPU VA space. In particular splitting and merging
> >of GPU VA mappings, e.g. for intersecting mapping requests or partial
> >unmap requests.
> > 
> > Suggested-by: Dave Airlie 
> > Signed-off-by: Danilo Krummrich 
> > ---
> >  Documentation/gpu/drm-mm.rst|   31 +
> >  drivers/gpu/drm/Makefile|1 +
> >  drivers/gpu/drm/drm_gem.c   |3 +
> >  drivers/gpu/drm/drm_gpuva_mgr.c | 1704 +++
> >  include/drm/drm_drv.h   |6 +
> >  include/drm/drm_gem.h   |   75 ++
> >  include/drm/drm_gpuva_mgr.h |  714 +
> >  7 files changed, 2534 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_gpuva_mgr.c
> >  create mode 100644 include/drm/drm_gpuva_mgr.h
> > 
> > diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
> > index a52e6f4117d6..c9f120cfe730 100644
> > --- a/Documentation/gpu/drm-mm.rst
> > +++ b/Documentation/gpu/drm-mm.rst
> > @@ -466,6 +466,37 @@ DRM MM Range Allocator Function References
> >  .. kernel-doc:: drivers/gpu/drm/drm_mm.c
> > :export:
> >  
> ...
> 
> > +
> > +/**
> > + * drm_gpuva_remove_iter - removes the iterators current element
> > + * @it: the &drm_gpuva_iterator
> > + *
> > + * This removes the element the iterator currently points to.
> > + */
> > +void
> > +drm_gpuva_iter_remove(struct drm_gpuva_iterator *it)
> > +{
> > +   mas_erase(&it->mas);
> > +}
> > +EXPORT_SYMBOL(drm_gpuva_iter_remove);
> > +
> > +/**
> > + * drm_gpuva_insert - insert a &drm_gpuva
> > + * @mgr: the &drm_gpuva_manager to insert the &drm_gpuva in
> > + * @va: the &drm_gpuva to insert
> > + * @addr: the start address of the GPU VA
> > + * @range: the range of the GPU VA
> > + *
> > + * Insert a &drm_gpuva with a given address and range into a
> > + * &drm_gpuva_manager.
> > + *
> > + * Returns: 0 on success, negative error code on failure.
> > + */
> > +int
> > +drm_gpuva_insert(struct drm_gpuva_manager *mgr,
> > +struct drm_gpuva *va)
> > +{
> > +   u64 addr = va->va.addr;
> > +   u64 range = va->va.range;
> > +   MA_STATE(mas, &mgr->va_mt, addr, addr + range - 1);
> > +   struct drm_gpuva_region *reg = NULL;
> > +   int ret;
> > +
> > +   if (unlikely(!drm_gpuva_in_mm_range(mgr, addr, range)))
> > +   return -EINVAL;
> > +
> > +   if (unlikely(drm_gpuva_in_kernel_region(mgr, addr, range)))
> > +   return -EINVAL;
> > +
> > +   if (mgr->flags & DRM_GPUVA_MANAGER_REGIONS) {
> > +   reg = drm_gpuva_in_region(mgr, addr, range);
> > +   if (unlikely(!reg))
> > +   return -EINVAL;
> > +   }
> > +
> 
> -
> 
> > +   if (unlikely(drm_gpuva_find_first(mgr, addr, range)))
> > +   return -EEXIST;
> > +
> > +   ret = mas_store_gfp(&mas, va, GFP_KERNEL);
> 
> mas_walk() will set the internal maple state to the limits to what it
> finds.  So, instead of an iterator, you can use the walk function and
> ensure there is a large enough area in the existing NULL:
> 
> /*
>  * Nothing at addr, mas now points to the location where the store would
>  * happen
>  */
> if (mas_walk(&mas))
>   return -EEXIST;
> 

For some reason mas_walk() finds an entry and hence this function returns
-EEXIST for the following sequence of insertions.

A = [0xc - 0xf]
B = [0x0 - 0xb]

Interestingly, inserting B before A works fine.

I attached a test module that reproduces the issue. I hope its just a stupid
mistake I just can't spot though.

> /* The NULL entry ends at mas.last, make sure there is room */
> if (mas.last < (addr + range - 1))
>   return -EEXIST;
> 
> /* Limit the store size to the correct end address, and store */
>  mas.last = addr + range - 1;
>  ret = mas_store_gfp(&mas, va, GFP_KERNEL);
> 
/* SPDX-License-Identifier: GPL-2.0 */
#if 1
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#endif

struct maple_tree mt;

struct va {
	u64 addr;
	u64 range;
};

static int va_store(struct va *va)
{
	void *entry = NULL;
	u64 addr = va->addr;
	u64 range = va->range;
	u64 last = addr + range - 1;
	MA_STATE(mas, &mt, addr, addr)

[PATCH v3 2/2] drm/i915/gt: Make sure that errors are propagated through request chains

Currently, when we perform operations such as clearing or copying
large blocks of memory, we generate multiple requests that are
executed in a chain.

However, if one of these requests fails, we may not realize it
unless it happens to be the last request in the chain. This is
because errors are not properly propagated.

For this we need to keep propagating the chain of fence
notification in order to always reach the final fence associated
to the final request.

To address this issue, we need to ensure that the chain of fence
notifications is always propagated so that we can reach the final
fence associated with the last request. By doing so, we will be
able to detect any memory operation  failures and determine
whether the memory is still invalid.

On copy and clear migration signal fences upon completion.

On copy and clear migration, signal fences upon request
completion to ensure that we have a reliable perpetuation of the
operation outcome.

Fixes: cf586021642d80 ("drm/i915/gt: Pipelined page migration")
Reported-by: Matthew Auld 
Suggested-by: Chris Wilson 
Signed-off-by: Andi Shyti 
Cc: sta...@vger.kernel.org
Reviewed-by: Matthew Auld 
---
 drivers/gpu/drm/i915/gt/intel_migrate.c | 39 ++---
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c 
b/drivers/gpu/drm/i915/gt/intel_migrate.c
index 3f638f1987968..6b497640d3a0a 100644
--- a/drivers/gpu/drm/i915/gt/intel_migrate.c
+++ b/drivers/gpu/drm/i915/gt/intel_migrate.c
@@ -742,13 +742,19 @@ intel_context_migrate_copy(struct intel_context *ce,
dst_offset = 2 * CHUNK_SZ;
}
 
+   /*
+* While building the chain of requests, we need to ensure
+* that no one can sneak into the timeline unnoticed.
+*/
+   mutex_lock(&ce->timeline->mutex);
+
do {
int len;
 
rq = i915_request_create(ce);
if (IS_ERR(rq)) {
err = PTR_ERR(rq);
-   goto out_ce;
+   break;
}
 
if (deps) {
@@ -878,10 +884,14 @@ intel_context_migrate_copy(struct intel_context *ce,
 
/* Arbitration is re-enabled between requests. */
 out_rq:
-   if (*out)
-   i915_request_put(*out);
-   *out = i915_request_get(rq);
+   i915_sw_fence_await(&rq->submit);
+   i915_request_get(rq);
i915_request_add(rq);
+   if (*out) {
+   i915_sw_fence_complete(&(*out)->submit);
+   i915_request_put(*out);
+   }
+   *out = rq;
 
if (err)
break;
@@ -905,7 +915,10 @@ intel_context_migrate_copy(struct intel_context *ce,
cond_resched();
} while (1);
 
-out_ce:
+   mutex_unlock(&ce->timeline->mutex);
+
+   if (*out)
+   i915_sw_fence_complete(&(*out)->submit);
return err;
 }
 
@@ -1005,7 +1018,7 @@ intel_context_migrate_clear(struct intel_context *ce,
rq = i915_request_create(ce);
if (IS_ERR(rq)) {
err = PTR_ERR(rq);
-   goto out_ce;
+   break;
}
 
if (deps) {
@@ -1056,17 +1069,23 @@ intel_context_migrate_clear(struct intel_context *ce,
 
/* Arbitration is re-enabled between requests. */
 out_rq:
-   if (*out)
-   i915_request_put(*out);
-   *out = i915_request_get(rq);
+   i915_sw_fence_await(&rq->submit);
+   i915_request_get(rq);
i915_request_add(rq);
+   if (*out) {
+   i915_sw_fence_complete(&(*out)->submit);
+   i915_request_put(*out);
+   }
+   *out = rq;
+
if (err || !it.sg || !sg_dma_len(it.sg))
break;
 
cond_resched();
} while (1);
 
-out_ce:
+   if (*out)
+   i915_sw_fence_complete(&(*out)->submit);
return err;
 }
 
-- 
2.39.1

[PATCH v3 0/2] Fix error propagation amongst request

Hi,

This series of two patches fixes the issue introduced in
cf586021642d80 ("drm/i915/gt: Pipelined page migration") where,
as reported by Matt, in a chain of requests an error is reported
only if happens in the last request.

However Chris noticed that without ensuring exclusivity in the
locking we might end up in some deadlock. That's why patch 1
throttles for the ringspace in order to make sure that no one is
holding it.

Version 1 of this patch has been reviewed by matt and this
version is adding Chris exclusive locking.

Thanks Chris for this work.

Andi

Changelog
=
v1 -> v2
 - Add patch 1 for ensuring exclusive locking of the timeline
 - Reword git commit of patch 2.

Andi Shyti (1):
  drm/i915/gt: Make sure that errors are propagated through request
chains

Chris Wilson (1):
  drm/i915: Throttle for ringspace prior to taking the timeline mutex

 drivers/gpu/drm/i915/gt/intel_context.c | 41 +
 drivers/gpu/drm/i915/gt/intel_context.h |  2 ++
 drivers/gpu/drm/i915/gt/intel_migrate.c | 39 +--
 drivers/gpu/drm/i915/i915_request.c |  3 ++
 4 files changed, 75 insertions(+), 10 deletions(-)

-- 
2.39.1

[PATCH v3 1/2] drm/i915: Throttle for ringspace prior to taking the timeline mutex

From: Chris Wilson 

Before taking exclusive ownership of the ring for emitting the request,
wait for space in the ring to become available. This allows others to
take the timeline->mutex to make forward progresses while userspace is
blocked.

In particular, this allows regular clients to issue requests on the
kernel context, potentially filling the ring, but allow the higher
priority heartbeats and pulses to still be submitted without being
blocked by the less critical work.

Fixes: cf586021642d80 ("drm/i915/gt: Pipelined page migration")
Signed-off-by: Chris Wilson 
Cc: Maciej Patelczyk 
Cc: sta...@vger.kernel.org
Signed-off-by: Andi Shyti 
---
Hi,

I'm not sure I need to add the Fixes tag here as this is more
preparatory for the next patch. Together, though, patch 1 and 2
make the fix with proper locking mechanism.

Andi

 drivers/gpu/drm/i915/gt/intel_context.c | 41 +
 drivers/gpu/drm/i915/gt/intel_context.h |  2 ++
 drivers/gpu/drm/i915/i915_request.c |  3 ++
 3 files changed, 46 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c 
b/drivers/gpu/drm/i915/gt/intel_context.c
index 2aa63ec521b89..59cd612a23561 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -626,6 +626,47 @@ bool intel_context_revoke(struct intel_context *ce)
return ret;
 }
 
+int intel_context_throttle(const struct intel_context *ce)
+{
+   const struct intel_ring *ring = ce->ring;
+   const struct intel_timeline *tl = ce->timeline;
+   struct i915_request *rq;
+   int err = 0;
+
+   if (READ_ONCE(ring->space) >= SZ_1K)
+   return 0;
+
+   rcu_read_lock();
+   list_for_each_entry_reverse(rq, &tl->requests, link) {
+   if (__i915_request_is_complete(rq))
+   break;
+
+   if (rq->ring != ring)
+   continue;
+
+   /* Wait until there will be enough space following that rq */
+   if (__intel_ring_space(rq->postfix,
+  ring->emit,
+  ring->size) < ring->size / 2) {
+   if (i915_request_get_rcu(rq)) {
+   rcu_read_unlock();
+
+   if (i915_request_wait(rq,
+ I915_WAIT_INTERRUPTIBLE,
+ MAX_SCHEDULE_TIMEOUT) < 0)
+   err = -EINTR;
+
+   rcu_read_lock();
+   i915_request_put(rq);
+   }
+   break;
+   }
+   }
+   rcu_read_unlock();
+
+   return err;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftest_context.c"
 #endif
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h 
b/drivers/gpu/drm/i915/gt/intel_context.h
index 0a8d553da3f43..f919a66cebf5b 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -226,6 +226,8 @@ static inline void intel_context_exit(struct intel_context 
*ce)
ce->ops->exit(ce);
 }
 
+int intel_context_throttle(const struct intel_context *ce);
+
 static inline struct intel_context *intel_context_get(struct intel_context *ce)
 {
kref_get(&ce->ref);
diff --git a/drivers/gpu/drm/i915/i915_request.c 
b/drivers/gpu/drm/i915/i915_request.c
index 7503dcb9043bb..a1741c4a8cffd 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1035,6 +1035,9 @@ i915_request_create(struct intel_context *ce)
struct i915_request *rq;
struct intel_timeline *tl;
 
+   if (intel_context_throttle(ce))
+   return ERR_PTR(-EINTR);
+
tl = intel_context_timeline_lock(ce);
if (IS_ERR(tl))
return ERR_CAST(tl);
-- 
2.39.1

Re: [RFT PATCH v2 3/3] drm/msm/dsi: More properly handle errors in regards to dsi_mgr_bridge_power_on()


On 01/02/2023 00:18, Douglas Anderson wrote:

In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time") the error handling with regards to dsi_mgr_bridge_power_on()
got a bit worse. Specifically if we failed to power the bridge on then
nothing would really notice. The modeset function couldn't return an
error and thus we'd blindly go forward and try to do the pre-enable.

In commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time
for parade-ps8640") we added a special case to move the powerup back
to pre-enable time for ps8640. When we did that, we didn't try to
recover the old/better error handling just for ps8640.

In the patch ("drm/msm/dsi: Stop unconditionally powering up DSI hosts
at modeset") we've now moved the powering up back to exclusively being
during pre-enable. That means we can add the better error handling
back in, so let's do it. To do so we'll add a new function
dsi_mgr_bridge_power_off() that's matches how errors were handled
prior to commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to
modeset time").

NOTE: Now that we have dsi_mgr_bridge_power_off(), it feels as if we
should be calling it in dsi_mgr_bridge_post_disable(). That would make
some sense, but doing so would change the current behavior and thus
should be a separate patch. Specifically:
* dsi_mgr_bridge_post_disable() always calls dsi_mgr_phy_disable()
   even in the slave-DSI case of bonded DSI. We'd need to add special
   handling for this if it's truly needed.
* dsi_mgr_bridge_post_disable() calls msm_dsi_phy_pll_save_state()
   midway through the poweroff.
* dsi_mgr_bridge_post_disable() has a different order of some of the
   poweroffs / IRQ disables.
For now we'll leave dsi_mgr_bridge_post_disable() alone.

Signed-off-by: Douglas Anderson 
---


Reviewed-by: Dmitry Baryshkov 

--
With best wishes
Dmitry

Re: [RFT PATCH v2 2/3] drm/msm/dsi: Stop unconditionally powering up DSI hosts at modeset


On 01/02/2023 00:18, Douglas Anderson wrote:

In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time"), we moved powering up DSI hosts to modeset time. This wasn't
because it was an elegant design, but there were no better options.

That commit actually ended up breaking ps8640, and thus was born
commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
the old way of doing things. It turns out that ps8640 _really_ doesn't
like its pre_enable() function to be called after
dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
because I have any inside knowledge), it looks like the assertion of
"RST#" in the ps8640 runtime resume handler seems like it's not
allowed to happen after dsi_mgr_bridge_power_on()

Recently, Dave Stevenson's series landed allowing bridges some control
over pre_enable ordering. The meaty commit for our purposes is commit
4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
bridge init order"). As documented by that series, if a bridge doesn't
set "pre_enable_prev_first" then we should use the old ordering.

Now that we have the commit ("drm/bridge: tc358762: Set
pre_enable_prev_first") we can go back to the old ordering, which also
allows us to remove the ps8640 special case.

One last note is that even without reverting commit 7d8e9a90509f
("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
revert the ps8640 special case and try it out then it doesn't seem to
fail anymore. I spent time bisecting / debugging this and it turns out
to be mostly luck, so we still want this patch to make sure it's
solid. Specifically the reason it sorta works these days is because
we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
"pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
implemented means that we actually power the bridge chip up just a wee
bit earlier and then the bridge happens to stay on because of
autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().

Cc: Dave Stevenson 
Cc: Dmitry Baryshkov 
Cc: Abhinav Kumar 
Signed-off-by: Douglas Anderson 
---


Reviewed-by: Dmitry Baryshkov 

--
With best wishes
Dmitry

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first


On 01/02/2023 00:18, Douglas Anderson wrote:

Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
order"). This should allow us to revert commit ec7981e6c614
("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time").

Cc: Dave Stevenson 
Cc: Dmitry Baryshkov 
Cc: Abhinav Kumar 
Signed-off-by: Douglas Anderson 


Reviewed-by: Dmitry Baryshkov 

--
With best wishes
Dmitry

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first


On 28/02/2023 02:26, Doug Anderson wrote:

Hi,

On Wed, Feb 1, 2023 at 1:51 AM Dave Stevenson
 wrote:


On Tue, 31 Jan 2023 at 22:22, Douglas Anderson  wrote:


Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
order"). This should allow us to revert commit ec7981e6c614
("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
time").


I see no reference in the TC358762 datasheet to requiring the DSI
interface to be in any particular state.
However, setting this flag does mean that the DSI host doesn't need to
power up and down for each host_transfer request from
tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.

Reviewed-by: Dave Stevenson 


Cc: Dave Stevenson 
Cc: Dmitry Baryshkov 
Cc: Abhinav Kumar 
Signed-off-by: Douglas Anderson 
---

(no changes since v1)

  drivers/gpu/drm/bridge/tc358762.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/tc358762.c 
b/drivers/gpu/drm/bridge/tc358762.c
index 0b6a28436885..77f7f7f54757 100644
--- a/drivers/gpu/drm/bridge/tc358762.c
+++ b/drivers/gpu/drm/bridge/tc358762.c
@@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
 ctx->bridge.funcs = &tc358762_bridge_funcs;
 ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
 ctx->bridge.of_node = dev->of_node;
+   ctx->bridge.pre_enable_prev_first = true;

 drm_bridge_add(&ctx->bridge);


Abhinav asked what the plan was for landing this [1]. Since this isn't
urgent, I guess the plan is to land patch #1 in drm-misc-next. Then we
sit and wait until it percolates into mainline and, once it does, then
patch #2 and #3 can land.

Since I have Dave's review I can commit this to drm-misc-next myself.
My plan will be to wait until Thursday or Friday of this week (to give
people a bit of time to object) and then land patch #1. Then I'll
snooze things for a while and poke Abhinav and Dmitry to land patch #2
/ #3 when I notice it in mainline. If, at any point, someone comes out
of the woodwork and yells that this is breaking them then, worst case,
we can revert.


This plan sounds good to me.



[1] https://lore.kernel.org/r/1f204585-88e2-abae-1216-92f739ac9...@quicinc.com/


--
With best wishes
Dmitry

[PATCH v2] drm/vram-helper: fix function names in vram helper doc

2023-02-27 Thread Luc Ma

Refer to drmm_vram_helper_init() instead of the non-existent
drmm_vram_helper_alloc_mm().

Fixes: a5f23a72355d ("drm/vram-helper: Managed vram helpers")
Signed-off-by: Luc Ma 
---
v1 -> v2: improved commit message and add changelog

 drivers/gpu/drm/drm_gem_vram_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c 
b/drivers/gpu/drm/drm_gem_vram_helper.c
index d40b3edb52d0..f1539d4448c6 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -45,7 +45,7 @@ static const struct drm_gem_object_funcs 
drm_gem_vram_object_funcs;
  * the frame's scanout buffer or the cursor image. If there's no more space
  * left in VRAM, inactive GEM objects can be moved to system memory.
  *
- * To initialize the VRAM helper library call drmm_vram_helper_alloc_mm().
+ * To initialize the VRAM helper library call drmm_vram_helper_init().
  * The function allocates and initializes an instance of &struct drm_vram_mm
  * in &struct drm_device.vram_mm . Use &DRM_GEM_VRAM_DRIVER to initialize
  * &struct drm_driver and  &DRM_VRAM_MM_FILE_OPERATIONS to initialize
@@ -73,7 +73,7 @@ static const struct drm_gem_object_funcs 
drm_gem_vram_object_funcs;
  * // setup device, vram base and size
  * // ...
  *
- * ret = drmm_vram_helper_alloc_mm(dev, vram_base, vram_size);
+ * ret = drmm_vram_helper_init(dev, vram_base, vram_size);
  * if (ret)
  * return ret;
  * return 0;
@@ -86,7 +86,7 @@ static const struct drm_gem_object_funcs 
drm_gem_vram_object_funcs;
  * to userspace.
  *
  * You don't have to clean up the instance of VRAM MM.
- * drmm_vram_helper_alloc_mm() is a managed interface that installs a
+ * drmm_vram_helper_init() is a managed interface that installs a
  * clean-up handler to run during the DRM device's release.
  *
  * For drawing or scanout operations, rsp. buffer objects have to be pinned
-- 
2.25.1

Re: [RFT PATCH v2 1/3] drm/bridge: tc358762: Set pre_enable_prev_first

2023-02-27 Thread Doug Anderson

Hi,

On Wed, Feb 1, 2023 at 1:51 AM Dave Stevenson
 wrote:
>
> On Tue, 31 Jan 2023 at 22:22, Douglas Anderson  wrote:
> >
> > Set the "pre_enable_prev_first" as provided by commit 4fb912e5e190
> > ("drm/bridge: Introduce pre_enable_prev_first to alter bridge init
> > order"). This should allow us to revert commit ec7981e6c614
> > ("drm/msm/dsi: don't powerup at modeset time for parade-ps8640") and
> > commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
> > time").
>
> I see no reference in the TC358762 datasheet to requiring the DSI
> interface to be in any particular state.
> However, setting this flag does mean that the DSI host doesn't need to
> power up and down for each host_transfer request from
> tc358762_pre_enable/tc358762_init, so on that basis I'm good with it.
>
> Reviewed-by: Dave Stevenson 
>
> > Cc: Dave Stevenson 
> > Cc: Dmitry Baryshkov 
> > Cc: Abhinav Kumar 
> > Signed-off-by: Douglas Anderson 
> > ---
> >
> > (no changes since v1)
> >
> >  drivers/gpu/drm/bridge/tc358762.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358762.c 
> > b/drivers/gpu/drm/bridge/tc358762.c
> > index 0b6a28436885..77f7f7f54757 100644
> > --- a/drivers/gpu/drm/bridge/tc358762.c
> > +++ b/drivers/gpu/drm/bridge/tc358762.c
> > @@ -229,6 +229,7 @@ static int tc358762_probe(struct mipi_dsi_device *dsi)
> > ctx->bridge.funcs = &tc358762_bridge_funcs;
> > ctx->bridge.type = DRM_MODE_CONNECTOR_DPI;
> > ctx->bridge.of_node = dev->of_node;
> > +   ctx->bridge.pre_enable_prev_first = true;
> >
> > drm_bridge_add(&ctx->bridge);

Abhinav asked what the plan was for landing this [1]. Since this isn't
urgent, I guess the plan is to land patch #1 in drm-misc-next. Then we
sit and wait until it percolates into mainline and, once it does, then
patch #2 and #3 can land.

Since I have Dave's review I can commit this to drm-misc-next myself.
My plan will be to wait until Thursday or Friday of this week (to give
people a bit of time to object) and then land patch #1. Then I'll
snooze things for a while and poke Abhinav and Dmitry to land patch #2
/ #3 when I notice it in mainline. If, at any point, someone comes out
of the woodwork and yells that this is breaking them then, worst case,
we can revert.

[1] https://lore.kernel.org/r/1f204585-88e2-abae-1216-92f739ac9...@quicinc.com/

Re: [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI

On Mon, Feb 27, 2023 at 2:44 PM Sebastian Wick
 wrote:
>
> On Mon, Feb 27, 2023 at 11:20 PM Rob Clark  wrote:
> >
> > On Mon, Feb 27, 2023 at 1:36 PM Rodrigo Vivi  wrote:
> > >
> > > On Fri, Feb 24, 2023 at 09:59:57AM -0800, Rob Clark wrote:
> > > > On Fri, Feb 24, 2023 at 7:27 AM Luben Tuikov  
> > > > wrote:
> > > > >
> > > > > On 2023-02-24 06:37, Tvrtko Ursulin wrote:
> > > > > >
> > > > > > On 24/02/2023 11:00, Pekka Paalanen wrote:
> > > > > >> On Fri, 24 Feb 2023 10:50:51 +
> > > > > >> Tvrtko Ursulin  wrote:
> > > > > >>
> > > > > >>> On 24/02/2023 10:24, Pekka Paalanen wrote:
> > > > >  On Fri, 24 Feb 2023 09:41:46 +
> > > > >  Tvrtko Ursulin  wrote:
> > > > > 
> > > > > > On 24/02/2023 09:26, Pekka Paalanen wrote:
> > > > > >> On Thu, 23 Feb 2023 10:51:48 -0800
> > > > > >> Rob Clark  wrote:
> > > > > >>
> > > > > >>> On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen 
> > > > > >>>  wrote:
> > > > > 
> > > > >  On Wed, 22 Feb 2023 07:37:26 -0800
> > > > >  Rob Clark  wrote:
> > > > > 
> > > > > > On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen 
> > > > > >  wrote:
> > > > > >>
> > > > > >> ...
> > > > > >>
> > > > > >> On another matter, if the application uses SET_DEADLINE 
> > > > > >> with one
> > > > > >> timestamp, and the compositor uses SET_DEADLINE on the 
> > > > > >> same thing with
> > > > > >> another timestamp, what should happen?
> > > > > >
> > > > > > The expectation is that many deadline hints can be set on a 
> > > > > > fence.
> > > > > > The fence signaller should track the soonest deadline.
> > > > > 
> > > > >  You need to document that as UAPI, since it is observable to 
> > > > >  userspace.
> > > > >  It would be bad if drivers or subsystems would differ in 
> > > > >  behaviour.
> > > > > 
> > > > > >>>
> > > > > >>> It is in the end a hint.  It is about giving the driver more
> > > > > >>> information so that it can make better choices.  But the 
> > > > > >>> driver is
> > > > > >>> even free to ignore it.  So maybe "expectation" is too strong 
> > > > > >>> of a
> > > > > >>> word.  Rather, any other behavior doesn't really make sense.  
> > > > > >>> But it
> > > > > >>> could end up being dictated by how the hw and/or fw works.
> > > > > >>
> > > > > >> It will stop being a hint once it has been implemented and 
> > > > > >> used in the
> > > > > >> wild long enough. The kernel userspace regression rules make 
> > > > > >> sure of
> > > > > >> that.
> > > > > >
> > > > > > Yeah, tricky and maybe a gray area in this case. I think we 
> > > > > > eluded
> > > > > > elsewhere in the thread that renaming the thing might be an 
> > > > > > option.
> > > > > >
> > > > > > So maybe instead of deadline, which is a very strong word, use 
> > > > > > something
> > > > > > along the lines of "present time hint", or "signalled time 
> > > > > > hint"? Maybe
> > > > > > reads clumsy. Just throwing some ideas for a start.
> > > > > 
> > > > >  You can try, but I fear that if it ever changes behaviour and
> > > > >  someone notices that, it's labelled as a kernel regression. I 
> > > > >  don't
> > > > >  think documentation has ever been the authoritative definition 
> > > > >  of UABI
> > > > >  in Linux, it just guides drivers and userspace towards a common
> > > > >  understanding and common usage patterns.
> > > > > 
> > > > >  So even if the UABI contract is not documented (ugh), you need 
> > > > >  to be
> > > > >  prepared to set the UABI contract through kernel implementation.
> > > > > >>>
> > > > > >>> To be the devil's advocate it probably wouldn't be an ABI 
> > > > > >>> regression but
> > > > > >>> just an regression. Same way as what nice(2) priorities mean 
> > > > > >>> hasn't
> > > > > >>> always been the same over the years, I don't think there is a 
> > > > > >>> strict
> > > > > >>> contract.
> > > > > >>>
> > > > > >>> Having said that, it may be different with latency sensitive 
> > > > > >>> stuff such
> > > > > >>> as UIs though since it is very observable and can be very painful 
> > > > > >>> to users.
> > > > > >>>
> > > > >  If you do not document the UABI contract, then different drivers 
> > > > >  are
> > > > >  likely to implement it differently, leading to differing 
> > > > >  behaviour.
> > > > >  Also userspace will invent wild ways to abuse the UABI if there 
> > > > >  is no
> > > > >  documentation guiding it on proper use. If userspace or end users
> > > > >  observe different behaviour, that's bad even if it's not a 
> > > > >  regression.
> > > > > 
> > > > >  I don't like the situation either, but it is what it is. UABI 
> > >

Re: [RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver

2023-02-27 Thread Bjorn Andersson

On Thu, Feb 23, 2023 at 07:26:35PM +0530, Sankeerth Billakanti wrote:
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
[..]
> +static int dp_runtime_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct msm_dp *dp_display = platform_get_drvdata(pdev);
> + struct dp_display_private *dp;
> +
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
> + dp_display_host_init(dp);
> +
> + if (dp->dp_display.is_edp) {
> + dp_display_host_phy_init(dp);
> + } else {
> + dp_catalog_hpd_config_intr(dp->catalog,
> + DP_DP_HPD_PLUG_INT_MASK |
> + DP_DP_HPD_UNPLUG_INT_MASK,
> + true);

I believe this is backwards.

Only in the event that there's no "downstream" HPD handler should we use
the internal HPD. This is signalled by the DRM framework by a call to
dp_bridge_hpd_enable(). So we should use that to enable/disable the
internal HPD handler.

When this happens, we have a reason for keeping power on; i.e. call
pm_runtime_get(). Once we have power/clocking, we'd call
dp_catalog_hpd_config_intr(), from dp_bridge_hpd_enable().

In the case that the internal HPD handling is not use,
dp_bridge_hpd_enable() will not be called, instead once the downstream
hpd handler switches state dp_bridge_hpd_notify() will be invoked.

In this case, we need the DP controller to be powered/clocked between
connector_status_connected and connector_status_disconnected.

I believe this should allow the DP controller(s) to stay powered down in
the case where we have external HPD handling (e.g. USB Type-C or
gpio-based dp-connector).

Regards,
Bjorn

Re: [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI

2023-02-27 Thread Sebastian Wick

On Mon, Feb 27, 2023 at 11:20 PM Rob Clark  wrote:
>
> On Mon, Feb 27, 2023 at 1:36 PM Rodrigo Vivi  wrote:
> >
> > On Fri, Feb 24, 2023 at 09:59:57AM -0800, Rob Clark wrote:
> > > On Fri, Feb 24, 2023 at 7:27 AM Luben Tuikov  wrote:
> > > >
> > > > On 2023-02-24 06:37, Tvrtko Ursulin wrote:
> > > > >
> > > > > On 24/02/2023 11:00, Pekka Paalanen wrote:
> > > > >> On Fri, 24 Feb 2023 10:50:51 +
> > > > >> Tvrtko Ursulin  wrote:
> > > > >>
> > > > >>> On 24/02/2023 10:24, Pekka Paalanen wrote:
> > > >  On Fri, 24 Feb 2023 09:41:46 +
> > > >  Tvrtko Ursulin  wrote:
> > > > 
> > > > > On 24/02/2023 09:26, Pekka Paalanen wrote:
> > > > >> On Thu, 23 Feb 2023 10:51:48 -0800
> > > > >> Rob Clark  wrote:
> > > > >>
> > > > >>> On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen 
> > > > >>>  wrote:
> > > > 
> > > >  On Wed, 22 Feb 2023 07:37:26 -0800
> > > >  Rob Clark  wrote:
> > > > 
> > > > > On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen 
> > > > >  wrote:
> > > > >>
> > > > >> ...
> > > > >>
> > > > >> On another matter, if the application uses SET_DEADLINE with 
> > > > >> one
> > > > >> timestamp, and the compositor uses SET_DEADLINE on the same 
> > > > >> thing with
> > > > >> another timestamp, what should happen?
> > > > >
> > > > > The expectation is that many deadline hints can be set on a 
> > > > > fence.
> > > > > The fence signaller should track the soonest deadline.
> > > > 
> > > >  You need to document that as UAPI, since it is observable to 
> > > >  userspace.
> > > >  It would be bad if drivers or subsystems would differ in 
> > > >  behaviour.
> > > > 
> > > > >>>
> > > > >>> It is in the end a hint.  It is about giving the driver more
> > > > >>> information so that it can make better choices.  But the driver 
> > > > >>> is
> > > > >>> even free to ignore it.  So maybe "expectation" is too strong 
> > > > >>> of a
> > > > >>> word.  Rather, any other behavior doesn't really make sense.  
> > > > >>> But it
> > > > >>> could end up being dictated by how the hw and/or fw works.
> > > > >>
> > > > >> It will stop being a hint once it has been implemented and used 
> > > > >> in the
> > > > >> wild long enough. The kernel userspace regression rules make 
> > > > >> sure of
> > > > >> that.
> > > > >
> > > > > Yeah, tricky and maybe a gray area in this case. I think we eluded
> > > > > elsewhere in the thread that renaming the thing might be an 
> > > > > option.
> > > > >
> > > > > So maybe instead of deadline, which is a very strong word, use 
> > > > > something
> > > > > along the lines of "present time hint", or "signalled time hint"? 
> > > > > Maybe
> > > > > reads clumsy. Just throwing some ideas for a start.
> > > > 
> > > >  You can try, but I fear that if it ever changes behaviour and
> > > >  someone notices that, it's labelled as a kernel regression. I don't
> > > >  think documentation has ever been the authoritative definition of 
> > > >  UABI
> > > >  in Linux, it just guides drivers and userspace towards a common
> > > >  understanding and common usage patterns.
> > > > 
> > > >  So even if the UABI contract is not documented (ugh), you need to 
> > > >  be
> > > >  prepared to set the UABI contract through kernel implementation.
> > > > >>>
> > > > >>> To be the devil's advocate it probably wouldn't be an ABI 
> > > > >>> regression but
> > > > >>> just an regression. Same way as what nice(2) priorities mean hasn't
> > > > >>> always been the same over the years, I don't think there is a strict
> > > > >>> contract.
> > > > >>>
> > > > >>> Having said that, it may be different with latency sensitive stuff 
> > > > >>> such
> > > > >>> as UIs though since it is very observable and can be very painful 
> > > > >>> to users.
> > > > >>>
> > > >  If you do not document the UABI contract, then different drivers 
> > > >  are
> > > >  likely to implement it differently, leading to differing behaviour.
> > > >  Also userspace will invent wild ways to abuse the UABI if there is 
> > > >  no
> > > >  documentation guiding it on proper use. If userspace or end users
> > > >  observe different behaviour, that's bad even if it's not a 
> > > >  regression.
> > > > 
> > > >  I don't like the situation either, but it is what it is. UABI 
> > > >  stability
> > > >  trumps everything regardless of whether it was documented or not.
> > > > 
> > > >  I bet userspace is going to use this as a "make it faster, make it
> > > >  hotter" button. I would not be surprised if someone wrote a 
> > > >  LD_PRELOAD
> > > >  library that stamps any and all fences with an expired

Re: [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI

On Mon, Feb 27, 2023 at 1:36 PM Rodrigo Vivi  wrote:
>
> On Fri, Feb 24, 2023 at 09:59:57AM -0800, Rob Clark wrote:
> > On Fri, Feb 24, 2023 at 7:27 AM Luben Tuikov  wrote:
> > >
> > > On 2023-02-24 06:37, Tvrtko Ursulin wrote:
> > > >
> > > > On 24/02/2023 11:00, Pekka Paalanen wrote:
> > > >> On Fri, 24 Feb 2023 10:50:51 +
> > > >> Tvrtko Ursulin  wrote:
> > > >>
> > > >>> On 24/02/2023 10:24, Pekka Paalanen wrote:
> > >  On Fri, 24 Feb 2023 09:41:46 +
> > >  Tvrtko Ursulin  wrote:
> > > 
> > > > On 24/02/2023 09:26, Pekka Paalanen wrote:
> > > >> On Thu, 23 Feb 2023 10:51:48 -0800
> > > >> Rob Clark  wrote:
> > > >>
> > > >>> On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen 
> > > >>>  wrote:
> > > 
> > >  On Wed, 22 Feb 2023 07:37:26 -0800
> > >  Rob Clark  wrote:
> > > 
> > > > On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen 
> > > >  wrote:
> > > >>
> > > >> ...
> > > >>
> > > >> On another matter, if the application uses SET_DEADLINE with 
> > > >> one
> > > >> timestamp, and the compositor uses SET_DEADLINE on the same 
> > > >> thing with
> > > >> another timestamp, what should happen?
> > > >
> > > > The expectation is that many deadline hints can be set on a 
> > > > fence.
> > > > The fence signaller should track the soonest deadline.
> > > 
> > >  You need to document that as UAPI, since it is observable to 
> > >  userspace.
> > >  It would be bad if drivers or subsystems would differ in 
> > >  behaviour.
> > > 
> > > >>>
> > > >>> It is in the end a hint.  It is about giving the driver more
> > > >>> information so that it can make better choices.  But the driver is
> > > >>> even free to ignore it.  So maybe "expectation" is too strong of a
> > > >>> word.  Rather, any other behavior doesn't really make sense.  But 
> > > >>> it
> > > >>> could end up being dictated by how the hw and/or fw works.
> > > >>
> > > >> It will stop being a hint once it has been implemented and used in 
> > > >> the
> > > >> wild long enough. The kernel userspace regression rules make sure 
> > > >> of
> > > >> that.
> > > >
> > > > Yeah, tricky and maybe a gray area in this case. I think we eluded
> > > > elsewhere in the thread that renaming the thing might be an option.
> > > >
> > > > So maybe instead of deadline, which is a very strong word, use 
> > > > something
> > > > along the lines of "present time hint", or "signalled time hint"? 
> > > > Maybe
> > > > reads clumsy. Just throwing some ideas for a start.
> > > 
> > >  You can try, but I fear that if it ever changes behaviour and
> > >  someone notices that, it's labelled as a kernel regression. I don't
> > >  think documentation has ever been the authoritative definition of 
> > >  UABI
> > >  in Linux, it just guides drivers and userspace towards a common
> > >  understanding and common usage patterns.
> > > 
> > >  So even if the UABI contract is not documented (ugh), you need to be
> > >  prepared to set the UABI contract through kernel implementation.
> > > >>>
> > > >>> To be the devil's advocate it probably wouldn't be an ABI regression 
> > > >>> but
> > > >>> just an regression. Same way as what nice(2) priorities mean hasn't
> > > >>> always been the same over the years, I don't think there is a strict
> > > >>> contract.
> > > >>>
> > > >>> Having said that, it may be different with latency sensitive stuff 
> > > >>> such
> > > >>> as UIs though since it is very observable and can be very painful to 
> > > >>> users.
> > > >>>
> > >  If you do not document the UABI contract, then different drivers are
> > >  likely to implement it differently, leading to differing behaviour.
> > >  Also userspace will invent wild ways to abuse the UABI if there is no
> > >  documentation guiding it on proper use. If userspace or end users
> > >  observe different behaviour, that's bad even if it's not a 
> > >  regression.
> > > 
> > >  I don't like the situation either, but it is what it is. UABI 
> > >  stability
> > >  trumps everything regardless of whether it was documented or not.
> > > 
> > >  I bet userspace is going to use this as a "make it faster, make it
> > >  hotter" button. I would not be surprised if someone wrote a 
> > >  LD_PRELOAD
> > >  library that stamps any and all fences with an expired deadline to
> > >  just squeeze out a little more through some weird side-effect.
> > > 
> > >  Well, that's hopefully overboard in scaring, but in the end, I would
> > >  like to see UABI documented so I can have a feeling of what it is for
> > >  and how it was intended to be used. That's all.
> > > >>>
> > > >>> We share the sa

[PATCH v2] drm/msm/disp/dpu: fix sc7280_pp base offset

2023-02-27 Thread Kuogee Hsieh

At sc7280, pingpong block is used to management the dither effects
to reduce distortion at panel. Currently pingpong-0 base offset is
wrongly set at 0x59000. This mistake will not cause system to crash.
However it will make dither not work. This patch correct sc7280 ping
pong-0 block base offset.

Changes in v2:
-- add more details info n regrading of pingpong block at commit text

Fixes: 591e34a091d1 ("drm/msm/disp/dpu1: add support for display for SC7280 
target")
Signed-off-by: Kuogee Hsieh 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 7deffc9f9..286437e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -1707,7 +1707,7 @@ static const struct dpu_pingpong_cfg sm8350_pp[] = {
 };
 
 static const struct dpu_pingpong_cfg sc7280_pp[] = {
-   PP_BLK("pingpong_0", PINGPONG_0, 0x59000, 0, sc7280_pp_sblk, -1, -1),
+   PP_BLK("pingpong_0", PINGPONG_0, 0x69000, 0, sc7280_pp_sblk, -1, -1),
PP_BLK("pingpong_1", PINGPONG_1, 0x6a000, 0, sc7280_pp_sblk, -1, -1),
PP_BLK("pingpong_2", PINGPONG_2, 0x6b000, 0, sc7280_pp_sblk, -1, -1),
PP_BLK("pingpong_3", PINGPONG_3, 0x6c000, 0, sc7280_pp_sblk, -1, -1),
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI

2023-02-27 Thread Rodrigo Vivi

On Fri, Feb 24, 2023 at 09:59:57AM -0800, Rob Clark wrote:
> On Fri, Feb 24, 2023 at 7:27 AM Luben Tuikov  wrote:
> >
> > On 2023-02-24 06:37, Tvrtko Ursulin wrote:
> > >
> > > On 24/02/2023 11:00, Pekka Paalanen wrote:
> > >> On Fri, 24 Feb 2023 10:50:51 +
> > >> Tvrtko Ursulin  wrote:
> > >>
> > >>> On 24/02/2023 10:24, Pekka Paalanen wrote:
> >  On Fri, 24 Feb 2023 09:41:46 +
> >  Tvrtko Ursulin  wrote:
> > 
> > > On 24/02/2023 09:26, Pekka Paalanen wrote:
> > >> On Thu, 23 Feb 2023 10:51:48 -0800
> > >> Rob Clark  wrote:
> > >>
> > >>> On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen 
> > >>>  wrote:
> > 
> >  On Wed, 22 Feb 2023 07:37:26 -0800
> >  Rob Clark  wrote:
> > 
> > > On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen 
> > >  wrote:
> > >>
> > >> ...
> > >>
> > >> On another matter, if the application uses SET_DEADLINE with one
> > >> timestamp, and the compositor uses SET_DEADLINE on the same 
> > >> thing with
> > >> another timestamp, what should happen?
> > >
> > > The expectation is that many deadline hints can be set on a fence.
> > > The fence signaller should track the soonest deadline.
> > 
> >  You need to document that as UAPI, since it is observable to 
> >  userspace.
> >  It would be bad if drivers or subsystems would differ in behaviour.
> > 
> > >>>
> > >>> It is in the end a hint.  It is about giving the driver more
> > >>> information so that it can make better choices.  But the driver is
> > >>> even free to ignore it.  So maybe "expectation" is too strong of a
> > >>> word.  Rather, any other behavior doesn't really make sense.  But it
> > >>> could end up being dictated by how the hw and/or fw works.
> > >>
> > >> It will stop being a hint once it has been implemented and used in 
> > >> the
> > >> wild long enough. The kernel userspace regression rules make sure of
> > >> that.
> > >
> > > Yeah, tricky and maybe a gray area in this case. I think we eluded
> > > elsewhere in the thread that renaming the thing might be an option.
> > >
> > > So maybe instead of deadline, which is a very strong word, use 
> > > something
> > > along the lines of "present time hint", or "signalled time hint"? 
> > > Maybe
> > > reads clumsy. Just throwing some ideas for a start.
> > 
> >  You can try, but I fear that if it ever changes behaviour and
> >  someone notices that, it's labelled as a kernel regression. I don't
> >  think documentation has ever been the authoritative definition of UABI
> >  in Linux, it just guides drivers and userspace towards a common
> >  understanding and common usage patterns.
> > 
> >  So even if the UABI contract is not documented (ugh), you need to be
> >  prepared to set the UABI contract through kernel implementation.
> > >>>
> > >>> To be the devil's advocate it probably wouldn't be an ABI regression but
> > >>> just an regression. Same way as what nice(2) priorities mean hasn't
> > >>> always been the same over the years, I don't think there is a strict
> > >>> contract.
> > >>>
> > >>> Having said that, it may be different with latency sensitive stuff such
> > >>> as UIs though since it is very observable and can be very painful to 
> > >>> users.
> > >>>
> >  If you do not document the UABI contract, then different drivers are
> >  likely to implement it differently, leading to differing behaviour.
> >  Also userspace will invent wild ways to abuse the UABI if there is no
> >  documentation guiding it on proper use. If userspace or end users
> >  observe different behaviour, that's bad even if it's not a regression.
> > 
> >  I don't like the situation either, but it is what it is. UABI stability
> >  trumps everything regardless of whether it was documented or not.
> > 
> >  I bet userspace is going to use this as a "make it faster, make it
> >  hotter" button. I would not be surprised if someone wrote a LD_PRELOAD
> >  library that stamps any and all fences with an expired deadline to
> >  just squeeze out a little more through some weird side-effect.
> > 
> >  Well, that's hopefully overboard in scaring, but in the end, I would
> >  like to see UABI documented so I can have a feeling of what it is for
> >  and how it was intended to be used. That's all.
> > >>>
> > >>> We share the same concern. If you read elsewhere in these threads you
> > >>> will notice I have been calling this an "arms race". If the ability to
> > >>> make yourself go faster does not required additional privilege I also
> > >>> worry everyone will do it at which point it becomes pointless. So yes, I
> > >>> do share this concern about exposing any of this as an unprivileged 
> > >>> uapi.
> > >>>
> > >>> I

Re: [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions

2023-02-27 Thread Abhinav Kumar

On 2/27/2023 11:25 AM, Dmitry Baryshkov wrote:

27 февраля 2023 г. 19:59:35 GMT+02:00, Abhinav Kumar
пишет:

On 2/27/2023 4:45 AM, Dmitry Baryshkov wrote:

On Mon, 27 Feb 2023 at 01:49, Abhinav Kumar wrote:

On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:

On 26/02/2023 02:47, Abhinav Kumar wrote:

Hi Dmitry

On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:

On 25/02/2023 02:36, Abhinav Kumar wrote:

On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:

On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar
wrote:

On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:

24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar
пишет:

On 2/24/2023 1:13 PM, Dmitry Baryshkov wrote:

On 24/02/2023 21:40, Kuogee Hsieh wrote:

Add DSC helper functions based on DSC configuration profiles
to produce
DSC related runtime parameters through both table look up and
runtime
calculation to support DSC on DPU.

There are 6 different DSC configuration profiles are supported
currently.
DSC configuration profiles are differiented by 5 keys, DSC
version (V1.1),
chroma (444/422/420), colorspace (RGB/YUV), bpc(8/10),
bpp (6/7/7.5/8/9/10/12/15) and SCR (0/1).

Only DSC version V1.1 added and V1.2 will be added later.

These helpers should go to
drivers/gpu/drm/display/drm_dsc_helper.c
Also please check that they can be used for i915 or for amdgpu
(ideally for both of them).

No, it cannot. So each DSC encoder parameter is calculated based
on the HW core which is being used.

They all get packed to the same DSC structure which is the
struct drm_dsc_config but the way the parameters are computed is
specific to the HW.

This DPU file helper still uses the drm_dsc_helper's
drm_dsc_compute_rc_parameters() like all other vendors do but
the parameters themselves are very HW specific and belong to
each vendor's dir.

This is not unique to MSM.

Lets take a few other examples:

AMD:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165

i915:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379

I checked several values here. Intel driver defines more bpc/bpp
combinations, but the ones which are defined in intel_vdsc and in
this patch seem to match. If there are major differences there,
please point me to the exact case.

I remember that AMD driver might have different values.

Some values in the rc_params table do match. But the
rc_buf_thresh[] doesnt.

Because later they do:

vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;

https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40

+static u16 dpu_dsc_rc_buf_thresh[DSC_NUM_BUF_RANGES - 1] = {
+ 0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54,
+ 0x62, 0x69, 0x70, 0x77, 0x79, 0x7b, 0x7d, 0x7e
+};

I'd prefer to have 896, 1792, etc. here, as those values come from the
standard. As it's done in the Intel driver.

Got it, thanks

I dont know the AMD calculation very well to say that moving this
to the
helper is going to help.

Those calculations correspond (more or less) at the first glance to
what intel does for their newer generations. I think that's not our
problem for now.

Well, we have to figure out if each value matches and if each of
them come from the spec for us and i915 and from which section. So
it is unfortunately our problem.

Otherwise it will have to be handled by Marijn, me or anybody else
wanting to hack up the DSC code. Or by anybody adding DSC support to
the next platform and having to figure out the difference between
i915, msm and their platform.

Yes, I wonder why the same doubt didn't arise when the other vendors
added their support both from other maintainers and others.

Which makes me think that like I wrote in my previous response, these
are "recommended" values in the spec but its not mandatory.

I think, it is because there were no other drivers to compare. In other
words, for a first driver it is pretty logical to have everything
handled on its own. As soon as we start getting other implementations of
a feature, it becomes logical to think if the code can be generalized.
This is what we see we with the HDCP series or with the code being moved
to DP helpers.

We were not the second, MSM was/is the third to add support for DSC afer
i915 and AMD. Thats what made me think why whoever was the second didnt
end up generalizing. Was it just missed out or was it intentionally left
in the vendor driver.

I didn't count AMD here, since it calculates some of the params rather
than using the fixed ones from the model.

Moving this to the drm_dsc_helper is generalizing the tables and not
giving room for the vendors to customize even if they want to (which
the spec does allow).

That depends on the API you select. For example, in
intel_dsc_compute_params() I see customization being applied to
rc_buf_thresh in 6bpp case. I'd leave that to the i915 driver.

Thanks for going through the i915 to

[PATCH v2 1/2] drm/i915: Throttle for ringspace prior to taking the timeline mutex

From: Chris Wilson 

Before taking exclusive ownership of the ring for emitting the request,
wait for space in the ring to become available. This allows others to
take the timeline->mutex to make forward progresses while userspace is
blocked.

In particular, this allows regular clients to issue requests on the
kernel context, potentially filling the ring, but allow the higher
priority heartbeats and pulses to still be submitted without being
blocked by the less critical work.

Fixes: cf586021642d80 ("drm/i915/gt: Pipelined page migration")
Signed-off-by: Chris Wilson 
Cc: Maciej Patelczyk 
Cc: sta...@vger.kernel.org
Signed-off-by: Andi Shyti 
---
Hi,

I'm not sure I need to add the Fixes tag here as this is more
preparatory for the next patch. Together, though, patch 1 and 2
make the fix with proper locking mechanism.

Andi

 drivers/gpu/drm/i915/gt/intel_context.c | 41 +
 drivers/gpu/drm/i915/gt/intel_context.h |  2 ++
 drivers/gpu/drm/i915/i915_request.c |  3 ++
 3 files changed, 46 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c 
b/drivers/gpu/drm/i915/gt/intel_context.c
index 2aa63ec521b89..59cd612a23561 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -626,6 +626,47 @@ bool intel_context_revoke(struct intel_context *ce)
return ret;
 }
 
+int intel_context_throttle(const struct intel_context *ce)
+{
+   const struct intel_ring *ring = ce->ring;
+   const struct intel_timeline *tl = ce->timeline;
+   struct i915_request *rq;
+   int err = 0;
+
+   if (READ_ONCE(ring->space) >= SZ_1K)
+   return 0;
+
+   rcu_read_lock();
+   list_for_each_entry_reverse(rq, &tl->requests, link) {
+   if (__i915_request_is_complete(rq))
+   break;
+
+   if (rq->ring != ring)
+   continue;
+
+   /* Wait until there will be enough space following that rq */
+   if (__intel_ring_space(rq->postfix,
+  ring->emit,
+  ring->size) < ring->size / 2) {
+   if (i915_request_get_rcu(rq)) {
+   rcu_read_unlock();
+
+   if (i915_request_wait(rq,
+ I915_WAIT_INTERRUPTIBLE,
+ MAX_SCHEDULE_TIMEOUT) < 0)
+   err = -EINTR;
+
+   rcu_read_lock();
+   i915_request_put(rq);
+   }
+   break;
+   }
+   }
+   rcu_read_unlock();
+
+   return err;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftest_context.c"
 #endif
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h 
b/drivers/gpu/drm/i915/gt/intel_context.h
index 0a8d553da3f43..f919a66cebf5b 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -226,6 +226,8 @@ static inline void intel_context_exit(struct intel_context 
*ce)
ce->ops->exit(ce);
 }
 
+int intel_context_throttle(const struct intel_context *ce);
+
 static inline struct intel_context *intel_context_get(struct intel_context *ce)
 {
kref_get(&ce->ref);
diff --git a/drivers/gpu/drm/i915/i915_request.c 
b/drivers/gpu/drm/i915/i915_request.c
index 7503dcb9043bb..a1741c4a8cffd 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1035,6 +1035,9 @@ i915_request_create(struct intel_context *ce)
struct i915_request *rq;
struct intel_timeline *tl;
 
+   if (intel_context_throttle(ce))
+   return ERR_PTR(-EINTR);
+
tl = intel_context_timeline_lock(ce);
if (IS_ERR(tl))
return ERR_CAST(tl);
-- 
2.39.1

[PATCH v2 2/2] drm/i915/gt: Make sure that errors are propagated through request chains

Currently, when we perform operations such as clearing or copying
large blocks of memory, we generate multiple requests that are
executed in a chain.

However, if one of these requests fails, we may not realize it
unless it happens to be the last request in the chain. This is
because errors are not properly propagated.

For this we need to keep propagating the chain of fence
notification in order to always reach the final fence associated
to the final request.

To address this issue, we need to ensure that the chain of fence
notifications is always propagated so that we can reach the final
fence associated with the last request. By doing so, we will be
able to detect any memory operation  failures and determine
whether the memory is still invalid.

On copy and clear migration signal fences upon completion.

On copy and clear migration, signal fences upon request
completion to ensure that we have a reliable perpetuation of the
operation outcome.

Fixes: cf586021642d80 ("drm/i915/gt: Pipelined page migration")
Reported-by: Matthew Auld 
Suggested-by: Chris Wilson 
Signed-off-by: Andi Shyti 
Cc: sta...@vger.kernel.org
Reviewed-by: Matthew Auld 
---
 drivers/gpu/drm/i915/gt/intel_migrate.c | 31 +
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_migrate.c 
b/drivers/gpu/drm/i915/gt/intel_migrate.c
index 3f638f1987968..8a293045a7b96 100644
--- a/drivers/gpu/drm/i915/gt/intel_migrate.c
+++ b/drivers/gpu/drm/i915/gt/intel_migrate.c
@@ -748,7 +748,7 @@ intel_context_migrate_copy(struct intel_context *ce,
rq = i915_request_create(ce);
if (IS_ERR(rq)) {
err = PTR_ERR(rq);
-   goto out_ce;
+   break;
}
 
if (deps) {
@@ -878,10 +878,14 @@ intel_context_migrate_copy(struct intel_context *ce,
 
/* Arbitration is re-enabled between requests. */
 out_rq:
-   if (*out)
-   i915_request_put(*out);
-   *out = i915_request_get(rq);
+   i915_sw_fence_await(&rq->submit);
+   i915_request_get(rq);
i915_request_add(rq);
+   if (*out) {
+   i915_sw_fence_complete(&(*out)->submit);
+   i915_request_put(*out);
+   }
+   *out = rq;
 
if (err)
break;
@@ -905,7 +909,8 @@ intel_context_migrate_copy(struct intel_context *ce,
cond_resched();
} while (1);
 
-out_ce:
+   if (*out)
+   i915_sw_fence_complete(&(*out)->submit);
return err;
 }
 
@@ -1005,7 +1010,7 @@ intel_context_migrate_clear(struct intel_context *ce,
rq = i915_request_create(ce);
if (IS_ERR(rq)) {
err = PTR_ERR(rq);
-   goto out_ce;
+   break;
}
 
if (deps) {
@@ -1056,17 +1061,23 @@ intel_context_migrate_clear(struct intel_context *ce,
 
/* Arbitration is re-enabled between requests. */
 out_rq:
-   if (*out)
-   i915_request_put(*out);
-   *out = i915_request_get(rq);
+   i915_sw_fence_await(&rq->submit);
+   i915_request_get(rq);
i915_request_add(rq);
+   if (*out) {
+   i915_sw_fence_complete(&(*out)->submit);
+   i915_request_put(*out);
+   }
+   *out = rq;
+
if (err || !it.sg || !sg_dma_len(it.sg))
break;
 
cond_resched();
} while (1);
 
-out_ce:
+   if (*out)
+   i915_sw_fence_complete(&(*out)->submit);
return err;
 }
 
-- 
2.39.1

[PATCH v2 0/2] Fix error propagation amongst requests

Hi,

This series of two patches fixes the issue introduced in
cf586021642d80 ("drm/i915/gt: Pipelined page migration") where,
as reported by Matt, in a chain of requests an error is reported
only if happens in the last request.

However Chris noticed that without ensuring exclusivity in the
locking we might end up in some deadlock. That's why patch 1
throttles for the ringspace in order to make sure that no one is
holding it.

Version 1 of this patch has been reviewed by matt and this
version is adding Chris exclusive locking.

Thanks Chris for this work.

Andi

Changelog
=
v1 -> v2
 - Add patch 1 for ensuring exclusive locking of the timeline
 - Reword git commit of patch 2.

Andi Shyti (1):
  drm/i915/gt: Make sure that errors are propagated through request
chains

Chris Wilson (1):
  drm/i915: Throttle for ringspace prior to taking the timeline mutex

 drivers/gpu/drm/i915/gt/intel_context.c | 41 +
 drivers/gpu/drm/i915/gt/intel_context.h |  2 ++
 drivers/gpu/drm/i915/gt/intel_migrate.c | 31 +--
 drivers/gpu/drm/i915/i915_request.c |  3 ++
 4 files changed, 67 insertions(+), 10 deletions(-)

-- 
2.39.1

[PATCH v2] drm/msm/dp: check core_initialized flag at both host_init() and host_deinit()

2023-02-27 Thread Kuogee Hsieh

There is a reboot/suspend test case where system suspend is forced during
system booting up. Since dp_display_host_init() of external DP is executed
at hpd thread context, this test case may created a scenario that
dp_display_host_deinit() from pm_suspend() run before dp_display_host_init()
if hpd thread has no chance to run during booting up while suspend request
command was issued. At this scenario system will crash at aux register
access at dp_display_host_deinit() since aux clock had not yet been enabled
by dp_display_host_init().  Therefore we have to ensure aux clock enabled
by checking core_initialized flag before access aux registers at pm_suspend.

Changes in v2:
-- at commit text, dp_display_host_init() instead of host_init()
-- at commit text, dp_display_host_deinit() instead of host_deinit()

Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin interrupt 
received")
Signed-off-by: Kuogee Hsieh 
Reviewed-by: Stephen Boyd 
---
 drivers/gpu/drm/msm/dp/dp_display.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index bde1a7c..1850738 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -460,10 +460,12 @@ static void dp_display_host_init(struct 
dp_display_private *dp)
dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized);
 
-   dp_power_init(dp->power, false);
-   dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
-   dp_aux_init(dp->aux);
-   dp->core_initialized = true;
+   if (!dp->core_initialized) {
+   dp_power_init(dp->power, false);
+   dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
+   dp_aux_init(dp->aux);
+   dp->core_initialized = true;
+   }
 }
 
 static void dp_display_host_deinit(struct dp_display_private *dp)
@@ -472,10 +474,12 @@ static void dp_display_host_deinit(struct 
dp_display_private *dp)
dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized);
 
-   dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
-   dp_aux_deinit(dp->aux);
-   dp_power_deinit(dp->power);
-   dp->core_initialized = false;
+   if (dp->core_initialized) {
+   dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
+   dp_aux_deinit(dp->aux);
+   dp_power_deinit(dp->power);
+   dp->core_initialized = false;
+   }
 }
 
 static int dp_display_usbpd_configure_cb(struct device *dev)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND v2 PATCH] init/do_mounts.c: add virtiofs root fs support

2023-02-27 Thread David Heidelberg


Thank you all!

We'll drop this patch in next MesaCI kernel uprev without this patch!

David

On 27/02/2023 19:06, Vivek Goyal wrote:

On Mon, Feb 27, 2023 at 10:53:45AM -0500, Stefan Hajnoczi wrote:

On Fri, Feb 24, 2023 at 03:37:51PM +0100, David Heidelberg wrote:

From: Stefan Hajnoczi 

Make it possible to boot directly from a virtiofs file system with tag
'myfs' using the following kernel parameters:

   rootfstype=virtiofs root=myfs rw

Booting directly from virtiofs makes it possible to use a directory on
the host as the root file system.  This is convenient for testing and
situations where manipulating disk image files is cumbersome.

Reviewed-by: Helen Koike 
Signed-off-by: Stefan Hajnoczi 
Signed-off-by: David Heidelberg 
---
v2: added Reviewed-by and CCed everyone interested.

We have used this option in Mesa3D CI for testing crosvm for
more than one years and it's proven to work reliably.

We are working on effort to removing custom patches to be able to do
automated apply and test of patches from any tree.

https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/.gitlab-ci/crosvm-runner.sh#L85
  init/do_mounts.c | 10 ++
  1 file changed, 10 insertions(+)

Vivek, do you remember where we ended up with boot from virtiofs? I
thought a different solution was merged some time ago.

We merged a patch from Christoph Hellwig to support this.

commit f9259be6a9e7c22d92e5a5000913147ae17e8321
Author: Christoph Hellwig 
Date:   Wed Jul 14 16:23:20 2021 -0400

 init: allow mounting arbitrary non-blockdevice filesystems as root

Now one should be able to mount virtiofs using following syntax.

"root=myfs rootfstype=virtiofs rw"

IIUC, this patch should not be required anymore.

Thanks
Vivek


There is documentation from the virtiofs community here:
https://virtio-fs.gitlab.io/howto-boot.html

Stefan


diff --git a/init/do_mounts.c b/init/do_mounts.c
index 811e94daf0a8..11c11abe23d7 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -578,6 +578,16 @@ void __init mount_root(void)
printk(KERN_ERR "VFS: Unable to mount root fs via 
SMB.\n");
return;
}
+#endif
+#ifdef CONFIG_VIRTIO_FS
+   if (root_fs_names && !strcmp(root_fs_names, "virtiofs")) {
+   if (!do_mount_root(root_device_name, "virtiofs",
+  root_mountflags, root_mount_data))
+   return;
+
+   panic("VFS: Unable to mount root fs \"%s\" from virtiofs",
+ root_device_name);
+   }
  #endif
if (ROOT_DEV == 0 && root_device_name && root_fs_names) {
if (mount_nodev_root() == 0)
--
2.39.1




--
David Heidelberg
Consultant Software Engineer

[PATCH v2 1/1] drm/doc: Document DRM device reset expectations

2023-02-27 Thread André Almeida

Create a section that specifies how to deal with DRM device resets for
kernel and userspace drivers.

Signed-off-by: André Almeida 
---
 Documentation/gpu/drm-uapi.rst | 51 ++
 1 file changed, 51 insertions(+)

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index 65fb3036a580..3d6c3ed392ea 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -285,6 +285,57 @@ for GPU1 and GPU2 from different vendors, and a third 
handler for
 mmapped regular files. Threads cause additional pain with signal
 handling as well.
 
+Device reset
+
+
+The GPU stack is really complex and is prone to errors, from hardware bugs,
+faulty applications and everything in the many layers in between. To recover
+from this kind of state, sometimes is needed to reset the GPU. Unproper 
handling
+of GPU resets can lead to an unstable userspace. This section describes what's
+the expected behaviour from DRM drivers to do in those situations, from 
usermode
+drivers and compositors as well. The end goal is to have a seamless experience
+as possible, either the stack being able to recover itself or resetting to a 
new
+stable state.
+
+Robustness
+--
+
+First of all, application robust APIs, when available, should be used. This
+allows the application to correctly recover and continue to run after a reset.
+Apps that doesn't use this should be promptly killed when the kernel driver
+detects that it's in broken state. Specifically guidelines for some APIs:
+
+- OpenGL: KMD signals the abortion of submitted commands and the UMD should 
then
+  react accordingly and abort the application.
+
+- Vulkan: Assumes that every app is able to deal with ``VK_ERROR_DEVICE_LOST``.
+  If it doesn't do it right, it's considered a broken application and UMD will
+  deal with it, aborting it.
+
+Kernel mode driver
+--
+
+The KMD must be able to detect that something is wrong with the application
+and that a reset is needed to take place to recover the device (e.g. an endless
+wait). It needs to properly track the context that is broken and mark it as
+dead, so any other syscalls to that context should be further rejected. The
+other contexts should be preserved when possible, avoid crashing the rest of
+userspace. KMD can ban a file descriptor that keeps causing resets, as it's
+likely in a broken loop.
+
+User mode driver
+
+
+During a reset, UMD should be aware that rejected syscalls indicates that the
+context is broken and for robust apps the recovery should happen for the
+context. Non-robust apps must be terminated.
+
+Compositors
+---
+
+Compositors should be robust as well to properly deal with its errors.
+
+
 .. _drm_driver_ioctl:
 
 IOCTL Support on Device Nodes
-- 
2.39.2

[PATCH v2 0/1] drm: Add doc about GPU reset

2023-02-27 Thread André Almeida

Hi,

Thanks everyone that gave feedback. v2 Changes:
- This new version is a section of drm-uapi instead of a new file
- Drop requirement for KMD to kill applications
- Drop role of init systems on compositors recover
- Drop assumption that robust apps creates new contexts

Original cover letter bellow:

Due to the complexity of its stack and the apps that we run on it, GPU resets
are for granted. What's left for driver developers is how to make resets a
smooth experience as possible. While some OS's can recover or show an error
message in such cases, Linux is more a hit-and-miss due to its lack of
standardization and guidelines of what to do in such cases.

This is the goal of this document, to proper define what should happen after a
GPU reset so developers can start acting on top of this. An IGT test should be
created to validate this for each driver.

Initially my approach was to expose an uevent for GPU resets, as it can be seen
here[1]. However, even if an uevent can be useful for some use cases (e.g.
telemetry and error reporting), for the "OS integration" case of GPU resets
it would be more productive to have something defined through the stack.

Thanks,
André

[1] 
https://lore.kernel.org/amd-gfx/20221125175203.52481-1-andrealm...@igalia.com/

André Almeida (1):
  drm/doc: Document DRM device reset expectations

 Documentation/gpu/drm-uapi.rst | 51 ++
 1 file changed, 51 insertions(+)

-- 
2.39.2

Re: [PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper

On Tue, Feb 28, 2023 at 1:40 AM Marek Vasut  wrote:
>
> On 2/27/23 20:49, Jagan Teki wrote:
> > On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut  wrote:
> >>
> >> On 2/27/23 20:34, Jagan Teki wrote:
> >>> On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut  wrote:
> 
>  On 2/27/23 20:15, Jagan Teki wrote:
> > On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut  wrote:
> >>
> >> On 2/27/23 20:01, Jagan Teki wrote:
> >>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut  wrote:
> 
>  On 2/27/23 12:39, Jagan Teki wrote:
> > drm_of_dsi_find_panel_or_bridge is capable of looking up the
> > downstream DSI bridge and panel and trying to add a panel bridge
> > if the panel is found.
> >
> > Replace explicit finding calls with drm_of_dsi_find_panel_or_bridge
> > followed with drmm_panel_bridge_add.
> >
> > Signed-off-by: Jagan Teki 
> > ---
> > Changes for v13, v12, v11:
> > - none
> > Changes for v10:
> > - new patch
> >
> >   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 
> > +
> >   1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
> > b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > index df15501b1075..12a6dd987e8f 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > @@ -25,6 +25,7 @@
> >   #include 
> >   #include 
> >   #include 
> > +#include 
> >   #include 
> >   #include 
> >   #include 
> > @@ -1470,24 +1471,26 @@ static int exynos_dsi_host_attach(struct 
> > mipi_dsi_host *host,
> >   struct device *dev = dsi->dev;
> >   struct drm_encoder *encoder = &dsi->encoder;
> >   struct drm_device *drm = encoder->dev;
> > + struct drm_bridge *bridge;
> >   struct drm_panel *panel;
> >   int ret;
> >
> > - panel = of_drm_find_panel(device->dev.of_node);
> > - if (!IS_ERR(panel)) {
> > - dsi->out_bridge = devm_drm_panel_bridge_add(dev, 
> > panel);
> > - } else {
> > - dsi->out_bridge = 
> > of_drm_find_bridge(device->dev.of_node);
> > - if (!dsi->out_bridge)
> > - dsi->out_bridge = ERR_PTR(-EINVAL);
> > - }
> 
>  As far as I understand this from my conversation with Maxime (please 
>  put
>  him on CC of V15), the change here should instead perform the panel 
>  look
>  up NOT in exynos_dsi_host_attach() , but in exynos_dsi_bind() , i.e. 
>  in
>  the component_ops .bind callback . Here is an example of correct
>  implementation:
> 
>  https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/vc4/vc4_dsi.c#n1805
> >>>
> >>> But, we don't have component_ops.bind for imx8m case, so where do we
> >>> want to place the panel hook?
> >>>
> >>> Exynos drm drivers are component-based but, imx8mm is not.
> >>
> >> In 14/18 patch, the same should be added to 
> >> generic_dsim_register_host()
> >> , which is called from the driver .probe() callback, but that is OK.
> >>
> >> That's ^ is the iMX part.
> >
> > Ohh. You mean, we need to find the panel hook separately in two places 
> > like
> > - bind for exynos
> > - probe for imx8mm
> 
>  Yes
> 
> > If so? how can I find the drm_device pointer in the probe?
> 
>  What for ? The panel lookup uses OF graph . Where do you need the
>  drm_device in it ?
> >>>
> >>> May I can summarize the whole setback here so that everybody is on the
> >>> same page and find the proper solution?
> >>>
> >>> The key blocker is accessing the DRM pointer in order to use the
> >>> DRM-managed action helper.
> >>>
> >>> 1. If we find the panel hook in Exynos component_ops.bind then we can
> >>> use the DRM pointer directly as VC4 does.
> >>> 2. if we find the panel hook in Samsung drm_bridge_funcs.attach (for
> >>> imx8mm) then we can use the DRM pointer directly via bridge->dev.
> >>>
> >>> If we make 2) has common across like finding the panel hook in
> >>> drm_bridge_funcs.attach the Exynos drm pipeline cannot find the
> >>> panels.
> >>>
> >>> The common solution for both exynos and imx8m is host.attach but if we
> >>> do so we cannot get find the DRM pointer.
> >>
> >> There isn't going to be common solution, you would really have to do the
> >> look up in component_ops .bind callback for exynos , and
> >> generic_dsim_register_host() for i.MX .
> >>
> >>> If we go ahead with no need for DRM-managed helper at the moment, the

Re: [PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper


On 2/27/23 20:49, Jagan Teki wrote:

On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut  wrote:


On 2/27/23 20:34, Jagan Teki wrote:

On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut  wrote:


On 2/27/23 20:15, Jagan Teki wrote:

On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut  wrote:


On 2/27/23 20:01, Jagan Teki wrote:

On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut  wrote:


On 2/27/23 12:39, Jagan Teki wrote:

drm_of_dsi_find_panel_or_bridge is capable of looking up the
downstream DSI bridge and panel and trying to add a panel bridge
if the panel is found.

Replace explicit finding calls with drm_of_dsi_find_panel_or_bridge
followed with drmm_panel_bridge_add.

Signed-off-by: Jagan Teki 
---
Changes for v13, v12, v11:
- none
Changes for v10:
- new patch

  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 +
  1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index df15501b1075..12a6dd987e8f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -25,6 +25,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -1470,24 +1471,26 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host 
*host,
  struct device *dev = dsi->dev;
  struct drm_encoder *encoder = &dsi->encoder;
  struct drm_device *drm = encoder->dev;
+ struct drm_bridge *bridge;
  struct drm_panel *panel;
  int ret;

- panel = of_drm_find_panel(device->dev.of_node);
- if (!IS_ERR(panel)) {
- dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
- } else {
- dsi->out_bridge = of_drm_find_bridge(device->dev.of_node);
- if (!dsi->out_bridge)
- dsi->out_bridge = ERR_PTR(-EINVAL);
- }


As far as I understand this from my conversation with Maxime (please put
him on CC of V15), the change here should instead perform the panel look
up NOT in exynos_dsi_host_attach() , but in exynos_dsi_bind() , i.e. in
the component_ops .bind callback . Here is an example of correct
implementation:

https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/vc4/vc4_dsi.c#n1805


But, we don't have component_ops.bind for imx8m case, so where do we
want to place the panel hook?

Exynos drm drivers are component-based but, imx8mm is not.


In 14/18 patch, the same should be added to generic_dsim_register_host()
, which is called from the driver .probe() callback, but that is OK.

That's ^ is the iMX part.


Ohh. You mean, we need to find the panel hook separately in two places like
- bind for exynos
- probe for imx8mm


Yes


If so? how can I find the drm_device pointer in the probe?


What for ? The panel lookup uses OF graph . Where do you need the
drm_device in it ?


May I can summarize the whole setback here so that everybody is on the
same page and find the proper solution?

The key blocker is accessing the DRM pointer in order to use the
DRM-managed action helper.

1. If we find the panel hook in Exynos component_ops.bind then we can
use the DRM pointer directly as VC4 does.
2. if we find the panel hook in Samsung drm_bridge_funcs.attach (for
imx8mm) then we can use the DRM pointer directly via bridge->dev.

If we make 2) has common across like finding the panel hook in
drm_bridge_funcs.attach the Exynos drm pipeline cannot find the
panels.

The common solution for both exynos and imx8m is host.attach but if we
do so we cannot get find the DRM pointer.


There isn't going to be common solution, you would really have to do the
look up in component_ops .bind callback for exynos , and
generic_dsim_register_host() for i.MX .


If we go ahead with no need for DRM-managed helper at the moment, then
find the panel hook in host.attach and then drop 2/18.


The panel lookup must happen in .bind/.probe for exynos/imx respectively
, that's really all that is required here. Then you can drop 1,2,3/18
and get this series applied (I hope) .


I'm not quite sure, if the panel hook in .bind work for exynos or not?


Marek should be able to test exynos for you one more time I hope.


the host. attach has KMS hotplug code after the panel hook and
bridge_attach as before. I believe that is a working scenario for
Exynos to be the panel hook in the host. attach.


As far as I understand it, the look up has to be in .bind callback (and 
generic_dsim_register_host() for imx). Can you try if/how that works ?



Can you implement just this one change ?

There is no need to use drmm_* helper for now, that can be improved
later if possible.


If this is the case then 1/18 will need otherwise


Ah yes, 1/18 will be needed indeed. It should just be called from .bind 
callback or generic_dsim_register_host() (i.e. probe callback).



 we can drop 1,2,3/18
by doing the panel hook we did in v7
https://patchwork.kernel.org/project/dri-devel/patch/20221005151309.727

Re: [PATCH] drm/msm/dp: check core_initialized flag at both host_init() and host_deinit()

2023-02-27 Thread Abhinav Kumar


Hi Stephen

On 2/27/2023 11:53 AM, Stephen Boyd wrote:

Quoting Kuogee Hsieh (2023-02-24 10:29:58)

There is a reboot/suspend test case where system suspend is forced during
system booting up. Since host_init() of external DP is executed at hpd


dp_display_host_init()?


thread context, this test case may created a scenario that host_deinit()


dp_display_host_deinit()?


ack for both.

from pm_suspend() run before host_init() if hpd thread has no chance to
run during booting up while suspend request command was issued.
At this scenario system will crash at aux register access at host_deinit()
since aux clock had not yet been enabled by host_init().  Therefore we


The aux clk is enabled in dp_power_clk_enable() right? Can you clarify?



Yes, thats right. Its mapped to core_*** in the dts hence the name goes 
to the DP_CORE_PM case in the dp_power_clk_enable()


3092clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
3093 <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
3094 <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
3095 <&dispcc 
DISP_CC_MDSS_DP_LINK_INTF_CLK>,
3096 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>;
3097clock-names = "core_iface", "core_aux"


have to ensure aux clock enabled by checking core_initialized flag before
access aux registers at pm_suspend.


I'd much more like to get rid of 'core_initialized'. What is preventing
us from enabling the power (i.e. dp_power_init()), or at least enough
clks and pm runtime state, during probe? That would fix this problem and
also clean things up. As I understand, the device is half initialized in
probe and half initialized in the kthread. If we put all power
management into the runtime PM ops and synced that state during probe so
that runtime PM state matched device probe state we could make runtime
PM be the only suspend function and then push the power state tracking
into the device core.



You are correct. https://patchwork.freedesktop.org/patch/523879/ will be 
doing that. Its still in review.




Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin interrupt 
received")
Signed-off-by: Kuogee Hsieh 


The code looks OK to me, so

Reviewed-by: Stephen Boyd 

once the commit text is cleaned up to indicate the proper function
names.


Thanks , will fix all those.

Re: [PATCH 0/2] char/agp: consolidate asm/agp.h

2023-02-27 Thread Arnd Bergmann

On Sun, Feb 12, 2023, at 09:46, Mike Rapoport wrote:
> From: "Mike Rapoport (IBM)" 
>
> asm/agp.h is duplicated in several architectures, with x86 being the
> only instance that differs from the rest.
>
> Introduce asm-generic/agp.h and use it instead of per-architecture
> headers for the most cases.
>
> I believe that asm-generic is the best tree to pick up this patches.
>
> Mike Rapoport (IBM) (2):  char/agp: consolidate
> {alloc,free}_gatt_pages()  char/agp: introduce asm-generic/agp.h

Hi Mike,

It looks like I wrote an email saying I merged these two patches,
but never actually sent it out. Not sure if you found out another
way, but this was part of the asm-generic tree for 6.3 and is now
merged upstream.

Thanks for the cleanup,

 Arnd

Re: [PATCH] drm/msm/dp: check core_initialized flag at both host_init() and host_deinit()

2023-02-27 Thread Stephen Boyd

Quoting Kuogee Hsieh (2023-02-24 10:29:58)
> There is a reboot/suspend test case where system suspend is forced during
> system booting up. Since host_init() of external DP is executed at hpd

dp_display_host_init()?

> thread context, this test case may created a scenario that host_deinit()

dp_display_host_deinit()?

> from pm_suspend() run before host_init() if hpd thread has no chance to
> run during booting up while suspend request command was issued.
> At this scenario system will crash at aux register access at host_deinit()
> since aux clock had not yet been enabled by host_init().  Therefore we

The aux clk is enabled in dp_power_clk_enable() right? Can you clarify?

> have to ensure aux clock enabled by checking core_initialized flag before
> access aux registers at pm_suspend.

I'd much more like to get rid of 'core_initialized'. What is preventing
us from enabling the power (i.e. dp_power_init()), or at least enough
clks and pm runtime state, during probe? That would fix this problem and
also clean things up. As I understand, the device is half initialized in
probe and half initialized in the kthread. If we put all power
management into the runtime PM ops and synced that state during probe so
that runtime PM state matched device probe state we could make runtime
PM be the only suspend function and then push the power state tracking
into the device core.

>
> Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin 
> interrupt received")
> Signed-off-by: Kuogee Hsieh 

The code looks OK to me, so

Reviewed-by: Stephen Boyd 

once the commit text is cleaned up to indicate the proper function
names.

Re: [PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper

On Tue, Feb 28, 2023 at 1:19 AM Jagan Teki  wrote:
>
> On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut  wrote:
> >
> > On 2/27/23 20:34, Jagan Teki wrote:
> > > On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut  wrote:
> > >>
> > >> On 2/27/23 20:15, Jagan Teki wrote:
> > >>> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut  wrote:
> > 
> >  On 2/27/23 20:01, Jagan Teki wrote:
> > > On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut  wrote:
> > >>
> > >> On 2/27/23 12:39, Jagan Teki wrote:
> > >>> drm_of_dsi_find_panel_or_bridge is capable of looking up the
> > >>> downstream DSI bridge and panel and trying to add a panel bridge
> > >>> if the panel is found.
> > >>>
> > >>> Replace explicit finding calls with drm_of_dsi_find_panel_or_bridge
> > >>> followed with drmm_panel_bridge_add.
> > >>>
> > >>> Signed-off-by: Jagan Teki 
> > >>> ---
> > >>> Changes for v13, v12, v11:
> > >>> - none
> > >>> Changes for v10:
> > >>> - new patch
> > >>>
> > >>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 
> > >>> +
> > >>>  1 file changed, 13 insertions(+), 12 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
> > >>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > >>> index df15501b1075..12a6dd987e8f 100644
> > >>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > >>> @@ -25,6 +25,7 @@
> > >>>  #include 
> > >>>  #include 
> > >>>  #include 
> > >>> +#include 
> > >>>  #include 
> > >>>  #include 
> > >>>  #include 
> > >>> @@ -1470,24 +1471,26 @@ static int exynos_dsi_host_attach(struct 
> > >>> mipi_dsi_host *host,
> > >>>  struct device *dev = dsi->dev;
> > >>>  struct drm_encoder *encoder = &dsi->encoder;
> > >>>  struct drm_device *drm = encoder->dev;
> > >>> + struct drm_bridge *bridge;
> > >>>  struct drm_panel *panel;
> > >>>  int ret;
> > >>>
> > >>> - panel = of_drm_find_panel(device->dev.of_node);
> > >>> - if (!IS_ERR(panel)) {
> > >>> - dsi->out_bridge = devm_drm_panel_bridge_add(dev, 
> > >>> panel);
> > >>> - } else {
> > >>> - dsi->out_bridge = 
> > >>> of_drm_find_bridge(device->dev.of_node);
> > >>> - if (!dsi->out_bridge)
> > >>> - dsi->out_bridge = ERR_PTR(-EINVAL);
> > >>> - }
> > >>
> > >> As far as I understand this from my conversation with Maxime (please 
> > >> put
> > >> him on CC of V15), the change here should instead perform the panel 
> > >> look
> > >> up NOT in exynos_dsi_host_attach() , but in exynos_dsi_bind() , i.e. 
> > >> in
> > >> the component_ops .bind callback . Here is an example of correct
> > >> implementation:
> > >>
> > >> https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/vc4/vc4_dsi.c#n1805
> > >
> > > But, we don't have component_ops.bind for imx8m case, so where do we
> > > want to place the panel hook?
> > >
> > > Exynos drm drivers are component-based but, imx8mm is not.
> > 
> >  In 14/18 patch, the same should be added to 
> >  generic_dsim_register_host()
> >  , which is called from the driver .probe() callback, but that is OK.
> > 
> >  That's ^ is the iMX part.
> > >>>
> > >>> Ohh. You mean, we need to find the panel hook separately in two places 
> > >>> like
> > >>> - bind for exynos
> > >>> - probe for imx8mm
> > >>
> > >> Yes
> > >>
> > >>> If so? how can I find the drm_device pointer in the probe?
> > >>
> > >> What for ? The panel lookup uses OF graph . Where do you need the
> > >> drm_device in it ?
> > >
> > > May I can summarize the whole setback here so that everybody is on the
> > > same page and find the proper solution?
> > >
> > > The key blocker is accessing the DRM pointer in order to use the
> > > DRM-managed action helper.
> > >
> > > 1. If we find the panel hook in Exynos component_ops.bind then we can
> > > use the DRM pointer directly as VC4 does.
> > > 2. if we find the panel hook in Samsung drm_bridge_funcs.attach (for
> > > imx8mm) then we can use the DRM pointer directly via bridge->dev.
> > >
> > > If we make 2) has common across like finding the panel hook in
> > > drm_bridge_funcs.attach the Exynos drm pipeline cannot find the
> > > panels.
> > >
> > > The common solution for both exynos and imx8m is host.attach but if we
> > > do so we cannot get find the DRM pointer.
> >
> > There isn't going to be common solution, you would really have to do the
> > look up in component_ops .bind callback for exynos , and
> > generic_dsim_register_host() for i.MX .
> >
> > > If we go ahead with no need for DRM-managed helper at the moment, then
> > > find the panel hook in host.attach and then drop 2/18

Re: [PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper

On Tue, Feb 28, 2023 at 1:11 AM Marek Vasut  wrote:
>
> On 2/27/23 20:34, Jagan Teki wrote:
> > On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut  wrote:
> >>
> >> On 2/27/23 20:15, Jagan Teki wrote:
> >>> On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut  wrote:
> 
>  On 2/27/23 20:01, Jagan Teki wrote:
> > On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut  wrote:
> >>
> >> On 2/27/23 12:39, Jagan Teki wrote:
> >>> drm_of_dsi_find_panel_or_bridge is capable of looking up the
> >>> downstream DSI bridge and panel and trying to add a panel bridge
> >>> if the panel is found.
> >>>
> >>> Replace explicit finding calls with drm_of_dsi_find_panel_or_bridge
> >>> followed with drmm_panel_bridge_add.
> >>>
> >>> Signed-off-by: Jagan Teki 
> >>> ---
> >>> Changes for v13, v12, v11:
> >>> - none
> >>> Changes for v10:
> >>> - new patch
> >>>
> >>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 
> >>> +
> >>>  1 file changed, 13 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
> >>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> index df15501b1075..12a6dd987e8f 100644
> >>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> @@ -25,6 +25,7 @@
> >>>  #include 
> >>>  #include 
> >>>  #include 
> >>> +#include 
> >>>  #include 
> >>>  #include 
> >>>  #include 
> >>> @@ -1470,24 +1471,26 @@ static int exynos_dsi_host_attach(struct 
> >>> mipi_dsi_host *host,
> >>>  struct device *dev = dsi->dev;
> >>>  struct drm_encoder *encoder = &dsi->encoder;
> >>>  struct drm_device *drm = encoder->dev;
> >>> + struct drm_bridge *bridge;
> >>>  struct drm_panel *panel;
> >>>  int ret;
> >>>
> >>> - panel = of_drm_find_panel(device->dev.of_node);
> >>> - if (!IS_ERR(panel)) {
> >>> - dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> >>> - } else {
> >>> - dsi->out_bridge = 
> >>> of_drm_find_bridge(device->dev.of_node);
> >>> - if (!dsi->out_bridge)
> >>> - dsi->out_bridge = ERR_PTR(-EINVAL);
> >>> - }
> >>
> >> As far as I understand this from my conversation with Maxime (please 
> >> put
> >> him on CC of V15), the change here should instead perform the panel 
> >> look
> >> up NOT in exynos_dsi_host_attach() , but in exynos_dsi_bind() , i.e. in
> >> the component_ops .bind callback . Here is an example of correct
> >> implementation:
> >>
> >> https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/vc4/vc4_dsi.c#n1805
> >
> > But, we don't have component_ops.bind for imx8m case, so where do we
> > want to place the panel hook?
> >
> > Exynos drm drivers are component-based but, imx8mm is not.
> 
>  In 14/18 patch, the same should be added to generic_dsim_register_host()
>  , which is called from the driver .probe() callback, but that is OK.
> 
>  That's ^ is the iMX part.
> >>>
> >>> Ohh. You mean, we need to find the panel hook separately in two places 
> >>> like
> >>> - bind for exynos
> >>> - probe for imx8mm
> >>
> >> Yes
> >>
> >>> If so? how can I find the drm_device pointer in the probe?
> >>
> >> What for ? The panel lookup uses OF graph . Where do you need the
> >> drm_device in it ?
> >
> > May I can summarize the whole setback here so that everybody is on the
> > same page and find the proper solution?
> >
> > The key blocker is accessing the DRM pointer in order to use the
> > DRM-managed action helper.
> >
> > 1. If we find the panel hook in Exynos component_ops.bind then we can
> > use the DRM pointer directly as VC4 does.
> > 2. if we find the panel hook in Samsung drm_bridge_funcs.attach (for
> > imx8mm) then we can use the DRM pointer directly via bridge->dev.
> >
> > If we make 2) has common across like finding the panel hook in
> > drm_bridge_funcs.attach the Exynos drm pipeline cannot find the
> > panels.
> >
> > The common solution for both exynos and imx8m is host.attach but if we
> > do so we cannot get find the DRM pointer.
>
> There isn't going to be common solution, you would really have to do the
> look up in component_ops .bind callback for exynos , and
> generic_dsim_register_host() for i.MX .
>
> > If we go ahead with no need for DRM-managed helper at the moment, then
> > find the panel hook in host.attach and then drop 2/18.
>
> The panel lookup must happen in .bind/.probe for exynos/imx respectively
> , that's really all that is required here. Then you can drop 1,2,3/18
> and get this series applied (I hope) .

I'm not quite sure, if the panel hook in .bind work for exynos or not?
the host. attach has KMS hotplug code after the panel hook a

Re: [PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper


On 2/27/23 20:34, Jagan Teki wrote:

On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut  wrote:


On 2/27/23 20:15, Jagan Teki wrote:

On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut  wrote:


On 2/27/23 20:01, Jagan Teki wrote:

On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut  wrote:


On 2/27/23 12:39, Jagan Teki wrote:

drm_of_dsi_find_panel_or_bridge is capable of looking up the
downstream DSI bridge and panel and trying to add a panel bridge
if the panel is found.

Replace explicit finding calls with drm_of_dsi_find_panel_or_bridge
followed with drmm_panel_bridge_add.

Signed-off-by: Jagan Teki 
---
Changes for v13, v12, v11:
- none
Changes for v10:
- new patch

 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index df15501b1075..12a6dd987e8f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1470,24 +1471,26 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host 
*host,
 struct device *dev = dsi->dev;
 struct drm_encoder *encoder = &dsi->encoder;
 struct drm_device *drm = encoder->dev;
+ struct drm_bridge *bridge;
 struct drm_panel *panel;
 int ret;

- panel = of_drm_find_panel(device->dev.of_node);
- if (!IS_ERR(panel)) {
- dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
- } else {
- dsi->out_bridge = of_drm_find_bridge(device->dev.of_node);
- if (!dsi->out_bridge)
- dsi->out_bridge = ERR_PTR(-EINVAL);
- }


As far as I understand this from my conversation with Maxime (please put
him on CC of V15), the change here should instead perform the panel look
up NOT in exynos_dsi_host_attach() , but in exynos_dsi_bind() , i.e. in
the component_ops .bind callback . Here is an example of correct
implementation:

https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/vc4/vc4_dsi.c#n1805


But, we don't have component_ops.bind for imx8m case, so where do we
want to place the panel hook?

Exynos drm drivers are component-based but, imx8mm is not.


In 14/18 patch, the same should be added to generic_dsim_register_host()
, which is called from the driver .probe() callback, but that is OK.

That's ^ is the iMX part.


Ohh. You mean, we need to find the panel hook separately in two places like
- bind for exynos
- probe for imx8mm


Yes


If so? how can I find the drm_device pointer in the probe?


What for ? The panel lookup uses OF graph . Where do you need the
drm_device in it ?


May I can summarize the whole setback here so that everybody is on the
same page and find the proper solution?

The key blocker is accessing the DRM pointer in order to use the
DRM-managed action helper.

1. If we find the panel hook in Exynos component_ops.bind then we can
use the DRM pointer directly as VC4 does.
2. if we find the panel hook in Samsung drm_bridge_funcs.attach (for
imx8mm) then we can use the DRM pointer directly via bridge->dev.

If we make 2) has common across like finding the panel hook in
drm_bridge_funcs.attach the Exynos drm pipeline cannot find the
panels.

The common solution for both exynos and imx8m is host.attach but if we
do so we cannot get find the DRM pointer.


There isn't going to be common solution, you would really have to do the 
look up in component_ops .bind callback for exynos , and 
generic_dsim_register_host() for i.MX .



If we go ahead with no need for DRM-managed helper at the moment, then
find the panel hook in host.attach and then drop 2/18.


The panel lookup must happen in .bind/.probe for exynos/imx respectively 
, that's really all that is required here. Then you can drop 1,2,3/18 
and get this series applied (I hope) .


Can you implement just this one change ?

There is no need to use drmm_* helper for now, that can be improved 
later if possible.

[PATCH v7 14/15] drm/msm/atomic: Switch to vblank_start helper

From: Rob Clark 

Drop our custom thing and switch to drm_crtc_next_vblank_start() for
calculating the time of the start of the next vblank period.

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 15 ---
 drivers/gpu/drm/msm/msm_atomic.c|  8 +---
 drivers/gpu/drm/msm/msm_kms.h   |  8 
 3 files changed, 5 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index a683bd9b5a04..43996aecaf8c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -411,20 +411,6 @@ static void dpu_kms_disable_commit(struct msm_kms *kms)
pm_runtime_put_sync(&dpu_kms->pdev->dev);
 }
 
-static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
-{
-   struct drm_encoder *encoder;
-
-   drm_for_each_encoder_mask(encoder, crtc->dev, 
crtc->state->encoder_mask) {
-   ktime_t vsync_time;
-
-   if (dpu_encoder_vsync_time(encoder, &vsync_time) == 0)
-   return vsync_time;
-   }
-
-   return ktime_get();
-}
-
 static void dpu_kms_prepare_commit(struct msm_kms *kms,
struct drm_atomic_state *state)
 {
@@ -953,7 +939,6 @@ static const struct msm_kms_funcs kms_funcs = {
.irq = dpu_core_irq,
.enable_commit   = dpu_kms_enable_commit,
.disable_commit  = dpu_kms_disable_commit,
-   .vsync_time  = dpu_kms_vsync_time,
.prepare_commit  = dpu_kms_prepare_commit,
.flush_commit= dpu_kms_flush_commit,
.wait_flush  = dpu_kms_wait_flush,
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 1686fbb611fd..de7ce17489cc 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -186,8 +186,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
struct msm_kms *kms = priv->kms;
struct drm_crtc *async_crtc = NULL;
unsigned crtc_mask = get_crtc_mask(state);
-   bool async = kms->funcs->vsync_time &&
-   can_do_async(state, &async_crtc);
+   bool async = can_do_async(state, &async_crtc);
 
trace_msm_atomic_commit_tail_start(async, crtc_mask);
 
@@ -231,7 +230,9 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 
kms->pending_crtc_mask |= crtc_mask;
 
-   vsync_time = kms->funcs->vsync_time(kms, async_crtc);
+   if (!drm_crtc_next_vblank_start(async_crtc, 
&vsync_time))
+   goto fallback;
+
wakeup_time = ktime_sub(vsync_time, ms_to_ktime(1));
 
msm_hrtimer_queue_work(&timer->work, wakeup_time,
@@ -253,6 +254,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
return;
}
 
+fallback:
/*
 * If there is any async flush pending on updated crtcs, fold
 * them into the current flush.
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index f8ed7588928c..086a3f1ff956 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -59,14 +59,6 @@ struct msm_kms_funcs {
void (*enable_commit)(struct msm_kms *kms);
void (*disable_commit)(struct msm_kms *kms);
 
-   /**
-* If the kms backend supports async commit, it should implement
-* this method to return the time of the next vsync.  This is
-* used to determine a time slightly before vsync, for the async
-* commit timer to run and complete an async commit.
-*/
-   ktime_t (*vsync_time)(struct msm_kms *kms, struct drm_crtc *crtc);
-
/**
 * Prepare for atomic commit.  This is called after any previous
 * (async or otherwise) commit has completed.
-- 
2.39.1

[PATCH v7 15/15] drm/i915: Add deadline based boost support

From: Rob Clark 

v2: rebase

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/i915/i915_request.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_request.c 
b/drivers/gpu/drm/i915/i915_request.c
index 7503dcb9043b..44491e7e214c 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -97,6 +97,25 @@ static bool i915_fence_enable_signaling(struct dma_fence 
*fence)
return i915_request_enable_breadcrumb(to_request(fence));
 }
 
+static void i915_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+   struct i915_request *rq = to_request(fence);
+
+   if (i915_request_completed(rq))
+   return;
+
+   if (i915_request_started(rq))
+   return;
+
+   /*
+* TODO something more clever for deadlines that are in the
+* future.  I think probably track the nearest deadline in
+* rq->timeline and set timer to trigger boost accordingly?
+*/
+
+   intel_rps_boost(rq);
+}
+
 static signed long i915_fence_wait(struct dma_fence *fence,
   bool interruptible,
   signed long timeout)
@@ -182,6 +201,7 @@ const struct dma_fence_ops i915_fence_ops = {
.signaled = i915_fence_signaled,
.wait = i915_fence_wait,
.release = i915_fence_release,
+   .set_deadline = i915_fence_set_deadline,
 };
 
 static void irq_execute_cb(struct irq_work *wrk)
-- 
2.39.1

[PATCH v7 13/15] drm/msm: Add wait-boost support

From: Rob Clark 

Add a way for various userspace waits to signal urgency.

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/msm_drv.c | 12 
 drivers/gpu/drm/msm/msm_gem.c |  5 +
 include/uapi/drm/msm_drm.h| 14 --
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index aca48c868c14..f6764a86b2da 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -46,6 +46,7 @@
  * - 1.8.0 - Add MSM_BO_CACHED_COHERENT for supported GPUs (a6xx)
  * - 1.9.0 - Add MSM_SUBMIT_FENCE_SN_IN
  * - 1.10.0 - Add MSM_SUBMIT_BO_NO_IMPLICIT
+ * - 1.11.0 - Add wait boost (MSM_WAIT_FENCE_BOOST, MSM_PREP_BOOST)
  */
 #define MSM_VERSION_MAJOR  1
 #define MSM_VERSION_MINOR  10
@@ -899,7 +900,7 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void 
*data,
 }
 
 static int wait_fence(struct msm_gpu_submitqueue *queue, uint32_t fence_id,
- ktime_t timeout)
+ ktime_t timeout, uint32_t flags)
 {
struct dma_fence *fence;
int ret;
@@ -929,6 +930,9 @@ static int wait_fence(struct msm_gpu_submitqueue *queue, 
uint32_t fence_id,
if (!fence)
return 0;
 
+   if (flags & MSM_WAIT_FENCE_BOOST)
+   dma_fence_set_deadline(fence, ktime_get());
+
ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout));
if (ret == 0) {
ret = -ETIMEDOUT;
@@ -949,8 +953,8 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, 
void *data,
struct msm_gpu_submitqueue *queue;
int ret;
 
-   if (args->pad) {
-   DRM_ERROR("invalid pad: %08x\n", args->pad);
+   if (args->flags & ~MSM_WAIT_FENCE_FLAGS) {
+   DRM_ERROR("invalid flags: %08x\n", args->flags);
return -EINVAL;
}
 
@@ -961,7 +965,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, 
void *data,
if (!queue)
return -ENOENT;
 
-   ret = wait_fence(queue, args->fence, to_ktime(args->timeout));
+   ret = wait_fence(queue, args->fence, to_ktime(args->timeout), 
args->flags);
 
msm_submitqueue_put(queue);
 
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 1dee0d18abbb..dd4a0d773f6e 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -846,6 +846,11 @@ int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t 
op, ktime_t *timeout)
op & MSM_PREP_NOSYNC ? 0 : timeout_to_jiffies(timeout);
long ret;
 
+   if (op & MSM_PREP_BOOST) {
+   dma_resv_set_deadline(obj->resv, dma_resv_usage_rw(write),
+ ktime_get());
+   }
+
ret = dma_resv_wait_timeout(obj->resv, dma_resv_usage_rw(write),
true,  remain);
if (ret == 0)
diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
index 329100016e7c..dbf0d6f43fa9 100644
--- a/include/uapi/drm/msm_drm.h
+++ b/include/uapi/drm/msm_drm.h
@@ -151,8 +151,13 @@ struct drm_msm_gem_info {
 #define MSM_PREP_READ0x01
 #define MSM_PREP_WRITE   0x02
 #define MSM_PREP_NOSYNC  0x04
+#define MSM_PREP_BOOST   0x08
 
-#define MSM_PREP_FLAGS   (MSM_PREP_READ | MSM_PREP_WRITE | MSM_PREP_NOSYNC)
+#define MSM_PREP_FLAGS   (MSM_PREP_READ | \
+ MSM_PREP_WRITE | \
+ MSM_PREP_NOSYNC | \
+ MSM_PREP_BOOST | \
+ 0)
 
 struct drm_msm_gem_cpu_prep {
__u32 handle; /* in */
@@ -286,6 +291,11 @@ struct drm_msm_gem_submit {
 
 };
 
+#define MSM_WAIT_FENCE_BOOST   0x0001
+#define MSM_WAIT_FENCE_FLAGS   ( \
+   MSM_WAIT_FENCE_BOOST | \
+   0)
+
 /* The normal way to synchronize with the GPU is just to CPU_PREP on
  * a buffer if you need to access it from the CPU (other cmdstream
  * submission from same or other contexts, PAGE_FLIP ioctl, etc, all
@@ -295,7 +305,7 @@ struct drm_msm_gem_submit {
  */
 struct drm_msm_wait_fence {
__u32 fence;  /* in */
-   __u32 pad;
+   __u32 flags;  /* in, bitmask of MSM_WAIT_FENCE_x */
struct drm_msm_timespec timeout;   /* in */
__u32 queueid; /* in, submitqueue id */
 };
-- 
2.39.1

[PATCH v7 12/15] drm/msm: Add deadline based boost support

From: Rob Clark 

Track the nearest deadline on a fence timeline and set a timer to expire
shortly before to trigger boost if the fence has not yet been signaled.

v2: rebase

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/msm_fence.c | 74 +
 drivers/gpu/drm/msm/msm_fence.h | 20 +
 2 files changed, 94 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
index 56641408ea74..51b461f32103 100644
--- a/drivers/gpu/drm/msm/msm_fence.c
+++ b/drivers/gpu/drm/msm/msm_fence.c
@@ -8,6 +8,35 @@
 
 #include "msm_drv.h"
 #include "msm_fence.h"
+#include "msm_gpu.h"
+
+static struct msm_gpu *fctx2gpu(struct msm_fence_context *fctx)
+{
+   struct msm_drm_private *priv = fctx->dev->dev_private;
+   return priv->gpu;
+}
+
+static enum hrtimer_restart deadline_timer(struct hrtimer *t)
+{
+   struct msm_fence_context *fctx = container_of(t,
+   struct msm_fence_context, deadline_timer);
+
+   kthread_queue_work(fctx2gpu(fctx)->worker, &fctx->deadline_work);
+
+   return HRTIMER_NORESTART;
+}
+
+static void deadline_work(struct kthread_work *work)
+{
+   struct msm_fence_context *fctx = container_of(work,
+   struct msm_fence_context, deadline_work);
+
+   /* If deadline fence has already passed, nothing to do: */
+   if (msm_fence_completed(fctx, fctx->next_deadline_fence))
+   return;
+
+   msm_devfreq_boost(fctx2gpu(fctx), 2);
+}
 
 
 struct msm_fence_context *
@@ -36,6 +65,13 @@ msm_fence_context_alloc(struct drm_device *dev, volatile 
uint32_t *fenceptr,
fctx->completed_fence = fctx->last_fence;
*fctx->fenceptr = fctx->last_fence;
 
+   hrtimer_init(&fctx->deadline_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
+   fctx->deadline_timer.function = deadline_timer;
+
+   kthread_init_work(&fctx->deadline_work, deadline_work);
+
+   fctx->next_deadline = ktime_get();
+
return fctx;
 }
 
@@ -62,6 +98,8 @@ void msm_update_fence(struct msm_fence_context *fctx, 
uint32_t fence)
spin_lock_irqsave(&fctx->spinlock, flags);
if (fence_after(fence, fctx->completed_fence))
fctx->completed_fence = fence;
+   if (msm_fence_completed(fctx, fctx->next_deadline_fence))
+   hrtimer_cancel(&fctx->deadline_timer);
spin_unlock_irqrestore(&fctx->spinlock, flags);
 }
 
@@ -92,10 +130,46 @@ static bool msm_fence_signaled(struct dma_fence *fence)
return msm_fence_completed(f->fctx, f->base.seqno);
 }
 
+static void msm_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+   struct msm_fence *f = to_msm_fence(fence);
+   struct msm_fence_context *fctx = f->fctx;
+   unsigned long flags;
+   ktime_t now;
+
+   spin_lock_irqsave(&fctx->spinlock, flags);
+   now = ktime_get();
+
+   if (ktime_after(now, fctx->next_deadline) ||
+   ktime_before(deadline, fctx->next_deadline)) {
+   fctx->next_deadline = deadline;
+   fctx->next_deadline_fence =
+   max(fctx->next_deadline_fence, (uint32_t)fence->seqno);
+
+   /*
+* Set timer to trigger boost 3ms before deadline, or
+* if we are already less than 3ms before the deadline
+* schedule boost work immediately.
+*/
+   deadline = ktime_sub(deadline, ms_to_ktime(3));
+
+   if (ktime_after(now, deadline)) {
+   kthread_queue_work(fctx2gpu(fctx)->worker,
+   &fctx->deadline_work);
+   } else {
+   hrtimer_start(&fctx->deadline_timer, deadline,
+   HRTIMER_MODE_ABS);
+   }
+   }
+
+   spin_unlock_irqrestore(&fctx->spinlock, flags);
+}
+
 static const struct dma_fence_ops msm_fence_ops = {
.get_driver_name = msm_fence_get_driver_name,
.get_timeline_name = msm_fence_get_timeline_name,
.signaled = msm_fence_signaled,
+   .set_deadline = msm_fence_set_deadline,
 };
 
 struct dma_fence *
diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h
index 7f1798c54cd1..cdaebfb94f5c 100644
--- a/drivers/gpu/drm/msm/msm_fence.h
+++ b/drivers/gpu/drm/msm/msm_fence.h
@@ -52,6 +52,26 @@ struct msm_fence_context {
volatile uint32_t *fenceptr;
 
spinlock_t spinlock;
+
+   /*
+* TODO this doesn't really deal with multiple deadlines, like
+* if userspace got multiple frames ahead.. OTOH atomic updates
+* don't queue, so maybe that is ok
+*/
+
+   /** next_deadline: Time of next deadline */
+   ktime_t next_deadline;
+
+   /**
+* next_deadline_fence:
+*
+* Fence value for next pending deadline.  The deadline timer is
+* canceled when this fence is signaled.
+*/
+   uint

[PATCH v7 10/15] drm/vblank: Add helper to get next vblank time

From: Rob Clark 

Will be used in the next commit to set a deadline on fences that an
atomic update is waiting on.

v2: Calculate time at *start* of vblank period, not end
v3: Fix kbuild complaints

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/drm_vblank.c | 53 ++--
 include/drm/drm_vblank.h |  1 +
 2 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 2ff31717a3de..299fa2a19a90 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -844,10 +844,9 @@ bool drm_crtc_vblank_helper_get_vblank_timestamp(struct 
drm_crtc *crtc,
 EXPORT_SYMBOL(drm_crtc_vblank_helper_get_vblank_timestamp);
 
 /**
- * drm_get_last_vbltimestamp - retrieve raw timestamp for the most recent
- * vblank interval
- * @dev: DRM device
- * @pipe: index of CRTC whose vblank timestamp to retrieve
+ * drm_crtc_get_last_vbltimestamp - retrieve raw timestamp for the most
+ *  recent vblank interval
+ * @crtc: CRTC whose vblank timestamp to retrieve
  * @tvblank: Pointer to target time which should receive the timestamp
  * @in_vblank_irq:
  * True when called from drm_crtc_handle_vblank().  Some drivers
@@ -865,10 +864,9 @@ EXPORT_SYMBOL(drm_crtc_vblank_helper_get_vblank_timestamp);
  * True if timestamp is considered to be very precise, false otherwise.
  */
 static bool
-drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
- ktime_t *tvblank, bool in_vblank_irq)
+drm_crtc_get_last_vbltimestamp(struct drm_crtc *crtc, ktime_t *tvblank,
+  bool in_vblank_irq)
 {
-   struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
bool ret = false;
 
/* Define requested maximum error on timestamps (nanoseconds). */
@@ -876,8 +874,6 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned 
int pipe,
 
/* Query driver if possible and precision timestamping enabled. */
if (crtc && crtc->funcs->get_vblank_timestamp && max_error > 0) {
-   struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
-
ret = crtc->funcs->get_vblank_timestamp(crtc, &max_error,
tvblank, in_vblank_irq);
}
@@ -891,6 +887,15 @@ drm_get_last_vbltimestamp(struct drm_device *dev, unsigned 
int pipe,
return ret;
 }
 
+static bool
+drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
+ ktime_t *tvblank, bool in_vblank_irq)
+{
+   struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
+
+   return drm_crtc_get_last_vbltimestamp(crtc, tvblank, in_vblank_irq);
+}
+
 /**
  * drm_crtc_vblank_count - retrieve "cooked" vblank counter value
  * @crtc: which counter to retrieve
@@ -980,6 +985,36 @@ u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
 }
 EXPORT_SYMBOL(drm_crtc_vblank_count_and_time);
 
+/**
+ * drm_crtc_next_vblank_start - calculate the time of the next vblank
+ * @crtc: the crtc for which to calculate next vblank time
+ * @vblanktime: pointer to time to receive the next vblank timestamp.
+ *
+ * Calculate the expected time of the start of the next vblank period,
+ * based on time of previous vblank and frame duration
+ */
+int drm_crtc_next_vblank_start(struct drm_crtc *crtc, ktime_t *vblanktime)
+{
+   unsigned int pipe = drm_crtc_index(crtc);
+   struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe];
+   struct drm_display_mode *mode = &vblank->hwmode;
+   u64 vblank_start;
+
+   if (!vblank->framedur_ns || !vblank->linedur_ns)
+   return -EINVAL;
+
+   if (!drm_crtc_get_last_vbltimestamp(crtc, vblanktime, false))
+   return -EINVAL;
+
+   vblank_start = DIV_ROUND_DOWN_ULL(
+   (u64)vblank->framedur_ns * mode->crtc_vblank_start,
+   mode->crtc_vtotal);
+   *vblanktime  = ktime_add(*vblanktime, ns_to_ktime(vblank_start));
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_crtc_next_vblank_start);
+
 static void send_vblank_event(struct drm_device *dev,
struct drm_pending_vblank_event *e,
u64 seq, ktime_t now)
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 733a3e2d1d10..7f3957943dd1 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -230,6 +230,7 @@ bool drm_dev_has_vblank(const struct drm_device *dev);
 u64 drm_crtc_vblank_count(struct drm_crtc *crtc);
 u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
   ktime_t *vblanktime);
+int drm_crtc_next_vblank_start(struct drm_crtc *crtc, ktime_t *vblanktime);
 void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
   struct drm_pending_vblank_event *e);
 void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
-- 
2.39.1

[PATCH v7 11/15] drm/atomic-helper: Set fence deadline for vblank

From: Rob Clark 

For an atomic commit updating a single CRTC (ie. a pageflip) calculate
the next vblank time, and inform the fence(s) of that deadline.

v2: Comment typo fix (danvet)

Signed-off-by: Rob Clark 
Reviewed-by: Daniel Vetter 
Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/drm_atomic_helper.c | 36 +
 1 file changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index d579fd8f7cb8..d8ee98ce2fc5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1511,6 +1511,40 @@ void drm_atomic_helper_commit_modeset_enables(struct 
drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
 
+/*
+ * For atomic updates which touch just a single CRTC, calculate the time of the
+ * next vblank, and inform all the fences of the deadline.
+ */
+static void set_fence_deadline(struct drm_device *dev,
+  struct drm_atomic_state *state)
+{
+   struct drm_crtc *crtc, *wait_crtc = NULL;
+   struct drm_crtc_state *new_crtc_state;
+   struct drm_plane *plane;
+   struct drm_plane_state *new_plane_state;
+   ktime_t vbltime;
+   int i;
+
+   for_each_new_crtc_in_state (state, crtc, new_crtc_state, i) {
+   if (wait_crtc)
+   return;
+   wait_crtc = crtc;
+   }
+
+   /* If no CRTCs updated, then nothing to do: */
+   if (!wait_crtc)
+   return;
+
+   if (drm_crtc_next_vblank_start(wait_crtc, &vbltime))
+   return;
+
+   for_each_new_plane_in_state (state, plane, new_plane_state, i) {
+   if (!new_plane_state->fence)
+   continue;
+   dma_fence_set_deadline(new_plane_state->fence, vbltime);
+   }
+}
+
 /**
  * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
  * @dev: DRM device
@@ -1540,6 +1574,8 @@ int drm_atomic_helper_wait_for_fences(struct drm_device 
*dev,
struct drm_plane_state *new_plane_state;
int i, ret;
 
+   set_fence_deadline(dev, state);
+
for_each_new_plane_in_state(state, plane, new_plane_state, i) {
if (!new_plane_state->fence)
continue;
-- 
2.39.1

[PATCH v7 09/15] drm/syncobj: Add deadline support for syncobj waits

From: Rob Clark 

Add a new flag to let userspace provide a deadline as a hint for syncobj
and timeline waits.  This gives a hint to the driver signaling the
backing fences about how soon userspace needs it to compete work, so it
can addjust GPU frequency accordingly.  An immediate deadline can be
given to provide something equivalent to i915 "wait boost".

v2: Use absolute u64 ns value for deadline hint, drop cap and driver
feature flag in favor of allowing count_handles==0 as a way for
userspace to probe kernel for support of new flag
v3: More verbose comments about UAPI

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/drm_syncobj.c | 64 ---
 include/uapi/drm/drm.h| 17 ++
 2 files changed, 68 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 0c2be8360525..a85e9464f07b 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -126,6 +126,11 @@
  * synchronize between the two.
  * This requirement is inherited from the Vulkan fence API.
  *
+ * If &DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE is set, the ioctl will also set
+ * a fence deadline hint on the backing fences before waiting, to provide the
+ * fence signaler with an appropriate sense of urgency.  The deadline is
+ * specified as an absolute &CLOCK_MONOTONIC value in units of ns.
+ *
  * Similarly, &DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT takes an array of syncobj
  * handles as well as an array of u64 points and does a host-side wait on all
  * of syncobj fences at the given points simultaneously.
@@ -973,7 +978,8 @@ static signed long drm_syncobj_array_wait_timeout(struct 
drm_syncobj **syncobjs,
  uint32_t count,
  uint32_t flags,
  signed long timeout,
- uint32_t *idx)
+ uint32_t *idx,
+ ktime_t *deadline)
 {
struct syncobj_wait_entry *entries;
struct dma_fence *fence;
@@ -1053,6 +1059,15 @@ static signed long drm_syncobj_array_wait_timeout(struct 
drm_syncobj **syncobjs,
drm_syncobj_fence_add_wait(syncobjs[i], &entries[i]);
}
 
+   if (deadline) {
+   for (i = 0; i < count; ++i) {
+   fence = entries[i].fence;
+   if (!fence)
+   continue;
+   dma_fence_set_deadline(fence, *deadline);
+   }
+   }
+
do {
set_current_state(TASK_INTERRUPTIBLE);
 
@@ -1151,7 +1166,8 @@ static int drm_syncobj_array_wait(struct drm_device *dev,
  struct drm_file *file_private,
  struct drm_syncobj_wait *wait,
  struct drm_syncobj_timeline_wait 
*timeline_wait,
- struct drm_syncobj **syncobjs, bool timeline)
+ struct drm_syncobj **syncobjs, bool timeline,
+ ktime_t *deadline)
 {
signed long timeout = 0;
uint32_t first = ~0;
@@ -1162,7 +1178,8 @@ static int drm_syncobj_array_wait(struct drm_device *dev,
 NULL,
 wait->count_handles,
 wait->flags,
-timeout, &first);
+timeout, &first,
+deadline);
if (timeout < 0)
return timeout;
wait->first_signaled = first;
@@ -1172,7 +1189,8 @@ static int drm_syncobj_array_wait(struct drm_device *dev,
 
u64_to_user_ptr(timeline_wait->points),
 
timeline_wait->count_handles,
 timeline_wait->flags,
-timeout, &first);
+timeout, &first,
+deadline);
if (timeout < 0)
return timeout;
timeline_wait->first_signaled = first;
@@ -1243,17 +1261,22 @@ drm_syncobj_wait_ioctl(struct drm_device *dev, void 
*data,
 {
struct drm_syncobj_wait *args = data;
struct drm_syncobj **syncobjs;
+   unsigned possible_flags;
+   ktime_t t, *tp = NULL;
int ret = 0;
 
if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
return -EOPNOTSUPP;
 
-

[PATCH v7 05/15] dma-buf/sync_file: Add SET_DEADLINE ioctl

From: Rob Clark 

The initial purpose is for igt tests, but this would also be useful for
compositors that wait until close to vblank deadline to make decisions
about which frame to show.

The igt tests can be found at:

https://gitlab.freedesktop.org/robclark/igt-gpu-tools/-/commits/fence-deadline

v2: Clarify the timebase, add link to igt tests
v3: Use u64 value in ns to express deadline.
v4: More doc

Signed-off-by: Rob Clark 
---
 drivers/dma-buf/dma-fence.c|  3 ++-
 drivers/dma-buf/sync_file.c| 19 +++
 include/uapi/linux/sync_file.h | 22 ++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index e103e821d993..7761ceeae620 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -933,7 +933,8 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
  *   the GPU's devfreq to reduce frequency, when in fact the opposite is what 
is
  *   needed.
  *
- * To this end, deadline hint(s) can be set on a &dma_fence via 
&dma_fence_set_deadline.
+ * To this end, deadline hint(s) can be set on a &dma_fence via 
&dma_fence_set_deadline
+ * (or indirectly via userspace facing ioctls like &SYNC_IOC_SET_DEADLINE).
  * The deadline hint provides a way for the waiting driver, or userspace, to
  * convey an appropriate sense of urgency to the signaling driver.
  *
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index af57799c86ce..418021cfb87c 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -350,6 +350,22 @@ static long sync_file_ioctl_fence_info(struct sync_file 
*sync_file,
return ret;
 }
 
+static int sync_file_ioctl_set_deadline(struct sync_file *sync_file,
+   unsigned long arg)
+{
+   struct sync_set_deadline ts;
+
+   if (copy_from_user(&ts, (void __user *)arg, sizeof(ts)))
+   return -EFAULT;
+
+   if (ts.pad)
+   return -EINVAL;
+
+   dma_fence_set_deadline(sync_file->fence, ns_to_ktime(ts.deadline_ns));
+
+   return 0;
+}
+
 static long sync_file_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
 {
@@ -362,6 +378,9 @@ static long sync_file_ioctl(struct file *file, unsigned int 
cmd,
case SYNC_IOC_FILE_INFO:
return sync_file_ioctl_fence_info(sync_file, arg);
 
+   case SYNC_IOC_SET_DEADLINE:
+   return sync_file_ioctl_set_deadline(sync_file, arg);
+
default:
return -ENOTTY;
}
diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
index ee2dcfb3d660..49325cf6749b 100644
--- a/include/uapi/linux/sync_file.h
+++ b/include/uapi/linux/sync_file.h
@@ -67,6 +67,21 @@ struct sync_file_info {
__u64   sync_fence_info;
 };
 
+/**
+ * struct sync_set_deadline - set a deadline hint on a fence
+ * @deadline_ns: absolute time of the deadline
+ * @pad:   must be zero
+ *
+ * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank)
+ */
+struct sync_set_deadline {
+   __u64   deadline_ns;
+   /* Not strictly needed for alignment but gives some possibility
+* for future extension:
+*/
+   __u64   pad;
+};
+
 #define SYNC_IOC_MAGIC '>'
 
 /**
@@ -95,4 +110,11 @@ struct sync_file_info {
  */
 #define SYNC_IOC_FILE_INFO _IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
 
+/**
+ * DOC: SYNC_IOC_SET_DEADLINE - set a deadline on a fence
+ *
+ * Allows userspace to set a deadline on a fence, see dma_fence_set_deadline()
+ */
+#define SYNC_IOC_SET_DEADLINE  _IOW(SYNC_IOC_MAGIC, 5, struct 
sync_set_deadline)
+
 #endif /* _UAPI_LINUX_SYNC_H */
-- 
2.39.1

[PATCH v7 08/15] drm/scheduler: Add fence deadline support

As the finished fence is the one that is exposed to userspace, and
therefore the one that other operations, like atomic update, would
block on, we need to propagate the deadline from from the finished
fence to the actual hw fence.

v2: Split into drm_sched_fence_set_parent() (ckoenig)
v3: Ensure a thread calling drm_sched_fence_set_deadline_finished() sees
fence->parent set before drm_sched_fence_set_parent() does this
test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT).

Signed-off-by: Rob Clark 
Acked-by: Luben Tuikov 
---
 drivers/gpu/drm/scheduler/sched_fence.c | 46 +
 drivers/gpu/drm/scheduler/sched_main.c  |  2 +-
 include/drm/gpu_scheduler.h | 17 +
 3 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/sched_fence.c 
b/drivers/gpu/drm/scheduler/sched_fence.c
index 7fd869520ef2..fe9c6468e440 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -123,6 +123,37 @@ static void drm_sched_fence_release_finished(struct 
dma_fence *f)
dma_fence_put(&fence->scheduled);
 }
 
+static void drm_sched_fence_set_deadline_finished(struct dma_fence *f,
+ ktime_t deadline)
+{
+   struct drm_sched_fence *fence = to_drm_sched_fence(f);
+   struct dma_fence *parent;
+   unsigned long flags;
+
+   spin_lock_irqsave(&fence->lock, flags);
+
+   /* If we already have an earlier deadline, keep it: */
+   if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) &&
+   ktime_before(fence->deadline, deadline)) {
+   spin_unlock_irqrestore(&fence->lock, flags);
+   return;
+   }
+
+   fence->deadline = deadline;
+   set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags);
+
+   spin_unlock_irqrestore(&fence->lock, flags);
+
+   /*
+* smp_load_aquire() to ensure that if we are racing another
+* thread calling drm_sched_fence_set_parent(), that we see
+* the parent set before it calls test_bit(HAS_DEADLINE_BIT)
+*/
+   parent = smp_load_acquire(&fence->parent);
+   if (parent)
+   dma_fence_set_deadline(parent, deadline);
+}
+
 static const struct dma_fence_ops drm_sched_fence_ops_scheduled = {
.get_driver_name = drm_sched_fence_get_driver_name,
.get_timeline_name = drm_sched_fence_get_timeline_name,
@@ -133,6 +164,7 @@ static const struct dma_fence_ops 
drm_sched_fence_ops_finished = {
.get_driver_name = drm_sched_fence_get_driver_name,
.get_timeline_name = drm_sched_fence_get_timeline_name,
.release = drm_sched_fence_release_finished,
+   .set_deadline = drm_sched_fence_set_deadline_finished,
 };
 
 struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f)
@@ -147,6 +179,20 @@ struct drm_sched_fence *to_drm_sched_fence(struct 
dma_fence *f)
 }
 EXPORT_SYMBOL(to_drm_sched_fence);
 
+void drm_sched_fence_set_parent(struct drm_sched_fence *s_fence,
+   struct dma_fence *fence)
+{
+   /*
+* smp_store_release() to ensure another thread racing us
+* in drm_sched_fence_set_deadline_finished() sees the
+* fence's parent set before test_bit()
+*/
+   smp_store_release(&s_fence->parent, dma_fence_get(fence));
+   if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT,
+&s_fence->finished.flags))
+   dma_fence_set_deadline(fence, s_fence->deadline);
+}
+
 struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity,
  void *owner)
 {
diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index 4e6ad6e122bc..007f98c48f8d 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1019,7 +1019,7 @@ static int drm_sched_main(void *param)
drm_sched_fence_scheduled(s_fence);
 
if (!IS_ERR_OR_NULL(fence)) {
-   s_fence->parent = dma_fence_get(fence);
+   drm_sched_fence_set_parent(s_fence, fence);
/* Drop for original kref_init of the fence */
dma_fence_put(fence);
 
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 9db9e5e504ee..99584e457153 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -41,6 +41,15 @@
  */
 #define DRM_SCHED_FENCE_DONT_PIPELINE  DMA_FENCE_FLAG_USER_BITS
 
+/**
+ * DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT - A fence deadline hint has been set
+ *
+ * Because we could have a deadline hint can be set before the backing hw
+ * fence is created, we need to keep track of whether a deadline has already
+ * been set.
+ */
+#define DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT  (DMA_FENCE_FLAG_USER_BITS + 1)
+
 enum dma_resv_usage;
 struct dma_resv;
 struct drm_gem_object;
@@

[PATCH v7 06/15] dma-buf/sync_file: Support (E)POLLPRI

From: Rob Clark 

Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent
wait (as opposed to a "housekeeping" wait to know when to cleanup after
some work has completed).  Usermode components of GPU driver stacks
often poll() on fence fd's to know when it is safe to do things like
free or reuse a buffer, but they can also poll() on a fence fd when
waiting to read back results from the GPU.  The EPOLLPRI/POLLPRI flag
lets the kernel differentiate these two cases.

Signed-off-by: Rob Clark 
---
 drivers/dma-buf/sync_file.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 418021cfb87c..cbe96295373b 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file, 
poll_table *wait)
 {
struct sync_file *sync_file = file->private_data;
 
+   /*
+* The POLLPRI/EPOLLPRI flag can be used to signal that
+* userspace wants the fence to signal ASAP, express this
+* as an immediate deadline.
+*/
+   if (poll_requested_events(wait) & EPOLLPRI)
+   dma_fence_set_deadline(sync_file->fence, ktime_get());
+
poll_wait(file, &sync_file->wq, wait);
 
if (list_empty(&sync_file->cb.node) &&
-- 
2.39.1

[PATCH v7 07/15] dma-buf/sw_sync: Add fence deadline support

From: Rob Clark 

This consists of simply storing the most recent deadline, and adding an
ioctl to retrieve the deadline.  This can be used in conjunction with
the SET_DEADLINE ioctl on a fence fd for testing.  Ie. create various
sw_sync fences, merge them into a fence-array, set deadline on the
fence-array and confirm that it is propagated properly to each fence.

v2: Switch UABI to express deadline as u64
v3: More verbose UAPI docs, show how to convert from timespec

Signed-off-by: Rob Clark 
Reviewed-by: Christian König 
---
 drivers/dma-buf/sw_sync.c  | 58 ++
 drivers/dma-buf/sync_debug.h   |  2 ++
 include/uapi/linux/sync_file.h |  6 +++-
 3 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 348b3a9170fa..3e2315ee955b 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -52,12 +52,28 @@ struct sw_sync_create_fence_data {
__s32   fence; /* fd of new fence */
 };
 
+/**
+ * struct sw_sync_get_deadline - get the deadline hint of a sw_sync fence
+ * @deadline_ns: absolute time of the deadline
+ * @pad:   must be zero
+ * @fence_fd:  the sw_sync fence fd (in)
+ *
+ * The timebase for the deadline is CLOCK_MONOTONIC (same as vblank)
+ */
+struct sw_sync_get_deadline {
+   __u64   deadline_ns;
+   __u32   pad;
+   __s32   fence_fd;
+};
+
 #define SW_SYNC_IOC_MAGIC  'W'
 
 #define SW_SYNC_IOC_CREATE_FENCE   _IOWR(SW_SYNC_IOC_MAGIC, 0,\
struct sw_sync_create_fence_data)
 
 #define SW_SYNC_IOC_INC_IOW(SW_SYNC_IOC_MAGIC, 1, 
__u32)
+#define SW_SYNC_GET_DEADLINE   _IOWR(SW_SYNC_IOC_MAGIC, 2, \
+   struct sw_sync_get_deadline)
 
 static const struct dma_fence_ops timeline_fence_ops;
 
@@ -171,6 +187,13 @@ static void timeline_fence_timeline_value_str(struct 
dma_fence *fence,
snprintf(str, size, "%d", parent->value);
 }
 
+static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t 
deadline)
+{
+   struct sync_pt *pt = dma_fence_to_sync_pt(fence);
+
+   pt->deadline = deadline;
+}
+
 static const struct dma_fence_ops timeline_fence_ops = {
.get_driver_name = timeline_fence_get_driver_name,
.get_timeline_name = timeline_fence_get_timeline_name,
@@ -179,6 +202,7 @@ static const struct dma_fence_ops timeline_fence_ops = {
.release = timeline_fence_release,
.fence_value_str = timeline_fence_value_str,
.timeline_value_str = timeline_fence_timeline_value_str,
+   .set_deadline = timeline_fence_set_deadline,
 };
 
 /**
@@ -387,6 +411,37 @@ static long sw_sync_ioctl_inc(struct sync_timeline *obj, 
unsigned long arg)
return 0;
 }
 
+static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long 
arg)
+{
+   struct sw_sync_get_deadline data;
+   struct dma_fence *fence;
+   struct sync_pt *pt;
+
+   if (copy_from_user(&data, (void __user *)arg, sizeof(data)))
+   return -EFAULT;
+
+   if (data.deadline_ns || data.pad)
+   return -EINVAL;
+
+   fence = sync_file_get_fence(data.fence_fd);
+   if (!fence)
+   return -EINVAL;
+
+   pt = dma_fence_to_sync_pt(fence);
+   if (!pt)
+   return -EINVAL;
+
+
+   data.deadline_ns = ktime_to_ns(pt->deadline);
+
+   dma_fence_put(fence);
+
+   if (copy_to_user((void __user *)arg, &data, sizeof(data)))
+   return -EFAULT;
+
+   return 0;
+}
+
 static long sw_sync_ioctl(struct file *file, unsigned int cmd,
  unsigned long arg)
 {
@@ -399,6 +454,9 @@ static long sw_sync_ioctl(struct file *file, unsigned int 
cmd,
case SW_SYNC_IOC_INC:
return sw_sync_ioctl_inc(obj, arg);
 
+   case SW_SYNC_GET_DEADLINE:
+   return sw_sync_ioctl_get_deadline(obj, arg);
+
default:
return -ENOTTY;
}
diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
index 6176e52ba2d7..2e0146d0bdbb 100644
--- a/drivers/dma-buf/sync_debug.h
+++ b/drivers/dma-buf/sync_debug.h
@@ -55,11 +55,13 @@ static inline struct sync_timeline *dma_fence_parent(struct 
dma_fence *fence)
  * @base: base fence object
  * @link: link on the sync timeline's list
  * @node: node in the sync timeline's tree
+ * @deadline: the most recently set fence deadline
  */
 struct sync_pt {
struct dma_fence base;
struct list_head link;
struct rb_node node;
+   ktime_t deadline;
 };
 
 extern const struct file_operations sw_sync_debugfs_fops;
diff --git a/include/uapi/linux/sync_file.h b/include/uapi/linux/sync_file.h
index 49325cf6749b..dc6645b2598b 100644
--- a/include/uapi/linux/sync_file.h
+++ b/include/uapi/linux/sync_file.h
@@ -72,7 +72,11 @@ struct sync_file_info {
  * @deadline_ns: absolute time of the deadline
  * @pad:   must be zero
  *
- * The timebase for the deadline is CLOCK_MO

[PATCH v7 04/15] dma-buf/dma-resv: Add a way to set fence deadline

From: Rob Clark 

Add a way to set a deadline on remaining resv fences according to the
requested usage.

Signed-off-by: Rob Clark 
Reviewed-by: Christian König 
---
 drivers/dma-buf/dma-resv.c | 22 ++
 include/linux/dma-resv.h   |  2 ++
 2 files changed, 24 insertions(+)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 1c76aed8e262..2a594b754af1 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
@@ -684,6 +684,28 @@ long dma_resv_wait_timeout(struct dma_resv *obj, enum 
dma_resv_usage usage,
 }
 EXPORT_SYMBOL_GPL(dma_resv_wait_timeout);
 
+/**
+ * dma_resv_set_deadline - Set a deadline on reservation's objects fences
+ * @obj: the reservation object
+ * @usage: controls which fences to include, see enum dma_resv_usage.
+ * @deadline: the requested deadline (MONOTONIC)
+ *
+ * May be called without holding the dma_resv lock.  Sets @deadline on
+ * all fences filtered by @usage.
+ */
+void dma_resv_set_deadline(struct dma_resv *obj, enum dma_resv_usage usage,
+  ktime_t deadline)
+{
+   struct dma_resv_iter cursor;
+   struct dma_fence *fence;
+
+   dma_resv_iter_begin(&cursor, obj, usage);
+   dma_resv_for_each_fence_unlocked(&cursor, fence) {
+   dma_fence_set_deadline(fence, deadline);
+   }
+   dma_resv_iter_end(&cursor);
+}
+EXPORT_SYMBOL_GPL(dma_resv_set_deadline);
 
 /**
  * dma_resv_test_signaled - Test if a reservation object's fences have been
diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
index 0637659a702c..8d0e34dad446 100644
--- a/include/linux/dma-resv.h
+++ b/include/linux/dma-resv.h
@@ -479,6 +479,8 @@ int dma_resv_get_singleton(struct dma_resv *obj, enum 
dma_resv_usage usage,
 int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src);
 long dma_resv_wait_timeout(struct dma_resv *obj, enum dma_resv_usage usage,
   bool intr, unsigned long timeout);
+void dma_resv_set_deadline(struct dma_resv *obj, enum dma_resv_usage usage,
+  ktime_t deadline);
 bool dma_resv_test_signaled(struct dma_resv *obj, enum dma_resv_usage usage);
 void dma_resv_describe(struct dma_resv *obj, struct seq_file *seq);
 
-- 
2.39.1

[PATCH v7 03/15] dma-buf/fence-chain: Add fence deadline support

From: Rob Clark 

Propagate the deadline to all the fences in the chain.

v2: Use dma_fence_chain_contained [Tvrtko]

Signed-off-by: Rob Clark 
Reviewed-by: Christian König  for this one.
---
 drivers/dma-buf/dma-fence-chain.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/dma-buf/dma-fence-chain.c 
b/drivers/dma-buf/dma-fence-chain.c
index a0d920576ba6..9663ba1bb6ac 100644
--- a/drivers/dma-buf/dma-fence-chain.c
+++ b/drivers/dma-buf/dma-fence-chain.c
@@ -206,6 +206,17 @@ static void dma_fence_chain_release(struct dma_fence 
*fence)
dma_fence_free(fence);
 }
 
+
+static void dma_fence_chain_set_deadline(struct dma_fence *fence,
+ktime_t deadline)
+{
+   dma_fence_chain_for_each(fence, fence) {
+   struct dma_fence *f = dma_fence_chain_contained(fence);
+
+   dma_fence_set_deadline(f, deadline);
+   }
+}
+
 const struct dma_fence_ops dma_fence_chain_ops = {
.use_64bit_seqno = true,
.get_driver_name = dma_fence_chain_get_driver_name,
@@ -213,6 +224,7 @@ const struct dma_fence_ops dma_fence_chain_ops = {
.enable_signaling = dma_fence_chain_enable_signaling,
.signaled = dma_fence_chain_signaled,
.release = dma_fence_chain_release,
+   .set_deadline = dma_fence_chain_set_deadline,
 };
 EXPORT_SYMBOL(dma_fence_chain_ops);
 
-- 
2.39.1

[PATCH v7 01/15] dma-buf/dma-fence: Add deadline awareness

From: Rob Clark 

Add a way to hint to the fence signaler of an upcoming deadline, such as
vblank, which the fence waiter would prefer not to miss.  This is to aid
the fence signaler in making power management decisions, like boosting
frequency as the deadline approaches and awareness of missing deadlines
so that can be factored in to the frequency scaling.

v2: Drop dma_fence::deadline and related logic to filter duplicate
deadlines, to avoid increasing dma_fence size.  The fence-context
implementation will need similar logic to track deadlines of all
the fences on the same timeline.  [ckoenig]
v3: Clarify locking wrt. set_deadline callback
v4: Clarify in docs comment that this is a hint
v5: Drop DMA_FENCE_FLAG_HAS_DEADLINE_BIT.
v6: More docs

Signed-off-by: Rob Clark 
Reviewed-by: Christian König 
---
 Documentation/driver-api/dma-buf.rst |  6 +++
 drivers/dma-buf/dma-fence.c  | 59 
 include/linux/dma-fence.h| 20 ++
 3 files changed, 85 insertions(+)

diff --git a/Documentation/driver-api/dma-buf.rst 
b/Documentation/driver-api/dma-buf.rst
index 622b8156d212..183e480d8cea 100644
--- a/Documentation/driver-api/dma-buf.rst
+++ b/Documentation/driver-api/dma-buf.rst
@@ -164,6 +164,12 @@ DMA Fence Signalling Annotations
 .. kernel-doc:: drivers/dma-buf/dma-fence.c
:doc: fence signalling annotation
 
+DMA Fence Deadline Hints
+
+
+.. kernel-doc:: drivers/dma-buf/dma-fence.c
+   :doc: deadline hints
+
 DMA Fences Functions Reference
 ~~
 
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 0de0482cd36e..e103e821d993 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -912,6 +912,65 @@ dma_fence_wait_any_timeout(struct dma_fence **fences, 
uint32_t count,
 }
 EXPORT_SYMBOL(dma_fence_wait_any_timeout);
 
+/**
+ * DOC: deadline hints
+ *
+ * In an ideal world, it would be possible to pipeline a workload sufficiently
+ * that a utilization based device frequency governor could arrive at a minimum
+ * frequency that meets the requirements of the use-case, in order to minimize
+ * power consumption.  But in the real world there are many workloads which
+ * defy this ideal.  For example, but not limited to:
+ *
+ * * Workloads that ping-pong between device and CPU, with alternating periods
+ *   of CPU waiting for device, and device waiting on CPU.  This can result in
+ *   devfreq and cpufreq seeing idle time in their respective domains and in
+ *   result reduce frequency.
+ *
+ * * Workloads that interact with a periodic time based deadline, such as 
double
+ *   buffered GPU rendering vs vblank sync'd page flipping.  In this scenario,
+ *   missing a vblank deadline results in an *increase* in idle time on the GPU
+ *   (since it has to wait an additional vblank period), sending a single to
+ *   the GPU's devfreq to reduce frequency, when in fact the opposite is what 
is
+ *   needed.
+ *
+ * To this end, deadline hint(s) can be set on a &dma_fence via 
&dma_fence_set_deadline.
+ * The deadline hint provides a way for the waiting driver, or userspace, to
+ * convey an appropriate sense of urgency to the signaling driver.
+ *
+ * A deadline hint is given in absolute ktime (CLOCK_MONOTONIC for userspace
+ * facing APIs).  The time could either be some point in the future (such as
+ * the vblank based deadline for page-flipping, or the start of a compositor's
+ * composition cycle), or the current time to indicate an immediate deadline
+ * hint (Ie. forward progress cannot be made until this fence is signaled).
+ *
+ * Multiple deadlines may be set on a given fence, even in parallel.  See the
+ * documentation for &dma_fence_ops.set_deadline.
+ *
+ * The deadline hint is just that, a hint.  The driver that created the fence
+ * may react by increasing frequency, making different scheduling choices, etc.
+ * Or doing nothing at all.
+ */
+
+/**
+ * dma_fence_set_deadline - set desired fence-wait deadline hint
+ * @fence:the fence that is to be waited on
+ * @deadline: the time by which the waiter hopes for the fence to be
+ *signaled
+ *
+ * Give the fence signaler a hint about an upcoming deadline, such as
+ * vblank, by which point the waiter would prefer the fence to be
+ * signaled by.  This is intended to give feedback to the fence signaler
+ * to aid in power management decisions, such as boosting GPU frequency
+ * if a periodic vblank deadline is approaching but the fence is not
+ * yet signaled..
+ */
+void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
+{
+   if (fence->ops->set_deadline && !dma_fence_is_signaled(fence))
+   fence->ops->set_deadline(fence, deadline);
+}
+EXPORT_SYMBOL(dma_fence_set_deadline);
+
 /**
  * dma_fence_describe - Dump fence describtion into seq_file
  * @fence: the 6fence to describe
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 77

[PATCH v7 02/15] dma-buf/fence-array: Add fence deadline support

From: Rob Clark 

Propagate the deadline to all the fences in the array.

Signed-off-by: Rob Clark 
Reviewed-by: Christian König 
---
 drivers/dma-buf/dma-fence-array.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/dma-buf/dma-fence-array.c 
b/drivers/dma-buf/dma-fence-array.c
index 5c8a7084577b..9b3ce8948351 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -123,12 +123,23 @@ static void dma_fence_array_release(struct dma_fence 
*fence)
dma_fence_free(fence);
 }
 
+static void dma_fence_array_set_deadline(struct dma_fence *fence,
+ktime_t deadline)
+{
+   struct dma_fence_array *array = to_dma_fence_array(fence);
+   unsigned i;
+
+   for (i = 0; i < array->num_fences; ++i)
+   dma_fence_set_deadline(array->fences[i], deadline);
+}
+
 const struct dma_fence_ops dma_fence_array_ops = {
.get_driver_name = dma_fence_array_get_driver_name,
.get_timeline_name = dma_fence_array_get_timeline_name,
.enable_signaling = dma_fence_array_enable_signaling,
.signaled = dma_fence_array_signaled,
.release = dma_fence_array_release,
+   .set_deadline = dma_fence_array_set_deadline,
 };
 EXPORT_SYMBOL(dma_fence_array_ops);
 
-- 
2.39.1

[PATCH v7 00/15] dma-fence: Deadline awareness

From: Rob Clark 

This series adds a deadline hint to fences, so realtime deadlines
such as vblank can be communicated to the fence signaller for power/
frequency management decisions.

This is partially inspired by a trick i915 does, but implemented
via dma-fence for a couple of reasons:

1) To continue to be able to use the atomic helpers
2) To support cases where display and gpu are different drivers

This iteration adds a dma-fence ioctl to set a deadline (both to
support igt-tests, and compositors which delay decisions about which
client buffer to display), and a sw_sync ioctl to read back the
deadline.  IGT tests utilizing these can be found at:

  https://gitlab.freedesktop.org/robclark/igt-gpu-tools/-/commits/fence-deadline


v1: https://patchwork.freedesktop.org/series/93035/
v2: Move filtering out of later deadlines to fence implementation
to avoid increasing the size of dma_fence
v3: Add support in fence-array and fence-chain; Add some uabi to
support igt tests and userspace compositors.
v4: Rebase, address various comments, and add syncobj deadline
support, and sync_file EPOLLPRI based on experience with perf/
freq issues with clvk compute workloads on i915 (anv)
v5: Clarify that this is a hint as opposed to a more hard deadline
guarantee, switch to using u64 ns values in UABI (still absolute
CLOCK_MONOTONIC values), drop syncobj related cap and driver
feature flag in favor of allowing count_handles==0 for probing
kernel support.
v6: Re-work vblank helper to calculate time of _start_ of vblank,
and work correctly if the last vblank event was more than a
frame ago.  Add (mostly unrelated) drm/msm patch which also
uses the vblank helper.  Use dma_fence_chain_contained().  More
verbose syncobj UABI comments.  Drop DMA_FENCE_FLAG_HAS_DEADLINE_BIT.
v7: Fix kbuild complaints about vblank helper.  Add more docs.

Rob Clark (15):
  dma-buf/dma-fence: Add deadline awareness
  dma-buf/fence-array: Add fence deadline support
  dma-buf/fence-chain: Add fence deadline support
  dma-buf/dma-resv: Add a way to set fence deadline
  dma-buf/sync_file: Add SET_DEADLINE ioctl
  dma-buf/sync_file: Support (E)POLLPRI
  dma-buf/sw_sync: Add fence deadline support
  drm/scheduler: Add fence deadline support
  drm/syncobj: Add deadline support for syncobj waits
  drm/vblank: Add helper to get next vblank time
  drm/atomic-helper: Set fence deadline for vblank
  drm/msm: Add deadline based boost support
  drm/msm: Add wait-boost support
  drm/msm/atomic: Switch to vblank_start helper
  drm/i915: Add deadline based boost support

 Documentation/driver-api/dma-buf.rst|  6 ++
 drivers/dma-buf/dma-fence-array.c   | 11 
 drivers/dma-buf/dma-fence-chain.c   | 12 
 drivers/dma-buf/dma-fence.c | 60 
 drivers/dma-buf/dma-resv.c  | 22 
 drivers/dma-buf/sw_sync.c   | 58 +++
 drivers/dma-buf/sync_debug.h|  2 +
 drivers/dma-buf/sync_file.c | 27 +
 drivers/gpu/drm/drm_atomic_helper.c | 36 
 drivers/gpu/drm/drm_syncobj.c   | 64 -
 drivers/gpu/drm/drm_vblank.c| 53 +++---
 drivers/gpu/drm/i915/i915_request.c | 20 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 15 -
 drivers/gpu/drm/msm/msm_atomic.c|  8 ++-
 drivers/gpu/drm/msm/msm_drv.c   | 12 ++--
 drivers/gpu/drm/msm/msm_fence.c | 74 +
 drivers/gpu/drm/msm/msm_fence.h | 20 +++
 drivers/gpu/drm/msm/msm_gem.c   |  5 ++
 drivers/gpu/drm/msm/msm_kms.h   |  8 ---
 drivers/gpu/drm/scheduler/sched_fence.c | 46 +++
 drivers/gpu/drm/scheduler/sched_main.c  |  2 +-
 include/drm/drm_vblank.h|  1 +
 include/drm/gpu_scheduler.h | 17 ++
 include/linux/dma-fence.h   | 20 +++
 include/linux/dma-resv.h|  2 +
 include/uapi/drm/drm.h  | 17 ++
 include/uapi/drm/msm_drm.h  | 14 -
 include/uapi/linux/sync_file.h  | 26 +
 28 files changed, 603 insertions(+), 55 deletions(-)

-- 
2.39.1

Re: [PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper

On Tue, Feb 28, 2023 at 12:54 AM Marek Vasut  wrote:
>
> On 2/27/23 20:15, Jagan Teki wrote:
> > On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut  wrote:
> >>
> >> On 2/27/23 20:01, Jagan Teki wrote:
> >>> On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut  wrote:
> 
>  On 2/27/23 12:39, Jagan Teki wrote:
> > drm_of_dsi_find_panel_or_bridge is capable of looking up the
> > downstream DSI bridge and panel and trying to add a panel bridge
> > if the panel is found.
> >
> > Replace explicit finding calls with drm_of_dsi_find_panel_or_bridge
> > followed with drmm_panel_bridge_add.
> >
> > Signed-off-by: Jagan Teki 
> > ---
> > Changes for v13, v12, v11:
> > - none
> > Changes for v10:
> > - new patch
> >
> > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 
> > +
> > 1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
> > b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > index df15501b1075..12a6dd987e8f 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > @@ -25,6 +25,7 @@
> > #include 
> > #include 
> > #include 
> > +#include 
> > #include 
> > #include 
> > #include 
> > @@ -1470,24 +1471,26 @@ static int exynos_dsi_host_attach(struct 
> > mipi_dsi_host *host,
> > struct device *dev = dsi->dev;
> > struct drm_encoder *encoder = &dsi->encoder;
> > struct drm_device *drm = encoder->dev;
> > + struct drm_bridge *bridge;
> > struct drm_panel *panel;
> > int ret;
> >
> > - panel = of_drm_find_panel(device->dev.of_node);
> > - if (!IS_ERR(panel)) {
> > - dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > - } else {
> > - dsi->out_bridge = of_drm_find_bridge(device->dev.of_node);
> > - if (!dsi->out_bridge)
> > - dsi->out_bridge = ERR_PTR(-EINVAL);
> > - }
> 
>  As far as I understand this from my conversation with Maxime (please put
>  him on CC of V15), the change here should instead perform the panel look
>  up NOT in exynos_dsi_host_attach() , but in exynos_dsi_bind() , i.e. in
>  the component_ops .bind callback . Here is an example of correct
>  implementation:
> 
>  https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/vc4/vc4_dsi.c#n1805
> >>>
> >>> But, we don't have component_ops.bind for imx8m case, so where do we
> >>> want to place the panel hook?
> >>>
> >>> Exynos drm drivers are component-based but, imx8mm is not.
> >>
> >> In 14/18 patch, the same should be added to generic_dsim_register_host()
> >> , which is called from the driver .probe() callback, but that is OK.
> >>
> >> That's ^ is the iMX part.
> >
> > Ohh. You mean, we need to find the panel hook separately in two places like
> > - bind for exynos
> > - probe for imx8mm
>
> Yes
>
> > If so? how can I find the drm_device pointer in the probe?
>
> What for ? The panel lookup uses OF graph . Where do you need the
> drm_device in it ?

May I can summarize the whole setback here so that everybody is on the
same page and find the proper solution?

The key blocker is accessing the DRM pointer in order to use the
DRM-managed action helper.

1. If we find the panel hook in Exynos component_ops.bind then we can
use the DRM pointer directly as VC4 does.
2. if we find the panel hook in Samsung drm_bridge_funcs.attach (for
imx8mm) then we can use the DRM pointer directly via bridge->dev.

If we make 2) has common across like finding the panel hook in
drm_bridge_funcs.attach the Exynos drm pipeline cannot find the
panels.

The common solution for both exynos and imx8m is host.attach but if we
do so we cannot get find the DRM pointer.

If we go ahead with no need for DRM-managed helper at the moment, then
find the panel hook in host.attach and then drop 2/18.

Thanks,
Jagan.

Re: [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions

27 февраля 2023 г. 19:59:35 GMT+02:00, Abhinav Kumar 
 пишет:
>
>
>On 2/27/2023 4:45 AM, Dmitry Baryshkov wrote:
>> On Mon, 27 Feb 2023 at 01:49, Abhinav Kumar  
>> wrote:
>>> 
>>> 
>>> 
>>> On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:
 On 26/02/2023 02:47, Abhinav Kumar wrote:
> Hi Dmitry
> 
> On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
>> On 25/02/2023 02:36, Abhinav Kumar wrote:
>>> 
>>> 
>>> On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
 On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar
  wrote:
> On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
>> 24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar
>>  пишет:
>>> On 2/24/2023 1:13 PM, Dmitry Baryshkov wrote:
 On 24/02/2023 21:40, Kuogee Hsieh wrote:
> Add DSC helper functions based on DSC configuration profiles
> to produce
> DSC related runtime parameters through both table look up and
> runtime
> calculation to support DSC on DPU.
> 
> There are 6 different DSC configuration profiles are supported
> currently.
> DSC configuration profiles are differiented by 5 keys, DSC
> version (V1.1),
> chroma (444/422/420), colorspace (RGB/YUV), bpc(8/10),
> bpp (6/7/7.5/8/9/10/12/15) and SCR (0/1).
> 
> Only DSC version V1.1 added and V1.2 will be added later.
 
 These helpers should go to
 drivers/gpu/drm/display/drm_dsc_helper.c
 Also please check that they can be used for i915 or for amdgpu
 (ideally for both of them).
 
>>> 
>>> No, it cannot. So each DSC encoder parameter is calculated based
>>> on the HW core which is being used.
>>> 
>>> They all get packed to the same DSC structure which is the
>>> struct drm_dsc_config but the way the parameters are computed is
>>> specific to the HW.
>>> 
>>> This DPU file helper still uses the drm_dsc_helper's
>>> drm_dsc_compute_rc_parameters() like all other vendors do but
>>> the parameters themselves are very HW specific and belong to
>>> each vendor's dir.
>>> 
>>> This is not unique to MSM.
>>> 
>>> Lets take a few other examples:
>>> 
>>> AMD:
>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
>>> 
>>> 
>>> i915:
>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
>>> 
>> 
>> I checked several values here. Intel driver defines more bpc/bpp
>> combinations, but the ones which are defined in intel_vdsc and in
>> this patch seem to match. If there are major differences there,
>> please point me to the exact case.
>> 
>> I remember that AMD driver might have different values.
>> 
> 
> Some values in the rc_params table do match. But the
> rc_buf_thresh[] doesnt.
 
 Because later they do:
 
 vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
 
> 
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
> 
> 
> Vs
> 
> +static u16 dpu_dsc_rc_buf_thresh[DSC_NUM_BUF_RANGES - 1] = {
> +   0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54,
> +   0x62, 0x69, 0x70, 0x77, 0x79, 0x7b, 0x7d, 0x7e
> +};
 
 I'd prefer to have 896, 1792, etc. here, as those values come from the
 standard. As it's done in the Intel driver.
 
>>> 
>>> Got it, thanks
>>> 
> I dont know the AMD calculation very well to say that moving this
> to the
> helper is going to help.
 
 Those calculations correspond (more or less) at the first glance to
 what intel does for their newer generations. I think that's not our
 problem for now.
 
>>> 
>>> Well, we have to figure out if each value matches and if each of
>>> them come from the spec for us and i915 and from which section. So
>>> it is unfortunately our problem.
>> 
>> Otherwise it will have to be handled by Marijn, me or anybody else
>> wanting to hack up the DSC code. Or by anybody adding DSC support to
>> the next platform and having to figure out the difference between
>> i915, msm and their platform.
>> 
> 
> Yes, I wonder why the same doubt didn't arise when the other vendors
> added their support both from other maintainers and others.
> 
> Which makes me think that like I wrote in my previous response, these
> are "recommended" values in

Re: [PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper


On 2/27/23 20:15, Jagan Teki wrote:

On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut  wrote:


On 2/27/23 20:01, Jagan Teki wrote:

On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut  wrote:


On 2/27/23 12:39, Jagan Teki wrote:

drm_of_dsi_find_panel_or_bridge is capable of looking up the
downstream DSI bridge and panel and trying to add a panel bridge
if the panel is found.

Replace explicit finding calls with drm_of_dsi_find_panel_or_bridge
followed with drmm_panel_bridge_add.

Signed-off-by: Jagan Teki 
---
Changes for v13, v12, v11:
- none
Changes for v10:
- new patch

drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index df15501b1075..12a6dd987e8f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -25,6 +25,7 @@
#include 
#include 
#include 
+#include 
#include 
#include 
#include 
@@ -1470,24 +1471,26 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host 
*host,
struct device *dev = dsi->dev;
struct drm_encoder *encoder = &dsi->encoder;
struct drm_device *drm = encoder->dev;
+ struct drm_bridge *bridge;
struct drm_panel *panel;
int ret;

- panel = of_drm_find_panel(device->dev.of_node);
- if (!IS_ERR(panel)) {
- dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
- } else {
- dsi->out_bridge = of_drm_find_bridge(device->dev.of_node);
- if (!dsi->out_bridge)
- dsi->out_bridge = ERR_PTR(-EINVAL);
- }


As far as I understand this from my conversation with Maxime (please put
him on CC of V15), the change here should instead perform the panel look
up NOT in exynos_dsi_host_attach() , but in exynos_dsi_bind() , i.e. in
the component_ops .bind callback . Here is an example of correct
implementation:

https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/vc4/vc4_dsi.c#n1805


But, we don't have component_ops.bind for imx8m case, so where do we
want to place the panel hook?

Exynos drm drivers are component-based but, imx8mm is not.


In 14/18 patch, the same should be added to generic_dsim_register_host()
, which is called from the driver .probe() callback, but that is OK.

That's ^ is the iMX part.


Ohh. You mean, we need to find the panel hook separately in two places like
- bind for exynos
- probe for imx8mm


Yes


If so? how can I find the drm_device pointer in the probe?


What for ? The panel lookup uses OF graph . Where do you need the 
drm_device in it ?

Re: [PATCH v3 1/2] drm/edid: parse DRM VESA dsc bpp target

2023-02-27 Thread Jani Nikula

On Mon, 27 Feb 2023, Harry Wentland  wrote:
> On 2/27/23 12:12, Jani Nikula wrote:
>> On Mon, 27 Feb 2023, Harry Wentland  wrote:
>>> On 2/26/23 09:10, Yaroslav Bolyukin wrote:
 As per DisplayID v2.0 Errata E9 spec "DSC pass-through timing support"
 VESA vendor-specific data block may contain target DSC bits per pixel
 fields

>>>
>>> According to the errata this should only apply to VII timings. The way
>>> it is currently implemented will make it apply to everything which is
>>> not what we want.
>>>
>>> Can we add this field to drm_mode_info instead of drm_display_info and
>>> set it inside drm_mode_displayid_detailed when parsing a type_7 timing?
>> 
>> That's actually difficult to do nicely. I think the patch at hand is
>> fine, and it's fine to add the information to drm_display_info. It's a
>> dependency to parsing the modes.
>> 
>> How the info will actually be used is a different matter, and obviously
>> needs to follow the spec. As it is, *this* patch doesn't say anything
>> about that. But whether it's handled in VII timings parsing or
>> elsewhere, I still think this part is fine.
>> 
>
> This patch itself is okay but without further work can't be used by
> drivers since they don't currently have an indication whether a mode
> is based on a VII timing.

Right, agreed.

All I'm saying is info->dp_dsc_bpp is the way to pass that info from
VESA vendor block to mode parsing.

Come to think of it, this patch should probably also rename
drm_update_mso() and drm_parse_vesa_mso_data() to reflect the more
generic VESA vendor block parsing instead of just MSO.

BR,
Jani.

>
> Harry
>
>> 
>> BR,
>> Jani.
>> 
>>>
>>> Harry
>>>
>>>
 Signed-off-by: Yaroslav Bolyukin 
 ---
  drivers/gpu/drm/drm_edid.c  | 38 +
  include/drm/drm_connector.h |  6 ++
  include/drm/drm_displayid.h |  4 
  3 files changed, 36 insertions(+), 12 deletions(-)

 diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
 index 3d0a4da661bc..aa88ac82cbe0 100644
 --- a/drivers/gpu/drm/drm_edid.c
 +++ b/drivers/gpu/drm/drm_edid.c
 @@ -6338,7 +6338,7 @@ static void drm_parse_vesa_mso_data(struct 
 drm_connector *connector,
if (oui(vesa->oui[0], vesa->oui[1], vesa->oui[2]) != VESA_IEEE_OUI)
return;
  
 -  if (sizeof(*vesa) != sizeof(*block) + block->num_bytes) {
 +  if (block->num_bytes < 5) {
drm_dbg_kms(connector->dev,
"[CONNECTOR:%d:%s] Unexpected VESA vendor block 
 size\n",
connector->base.id, connector->name);
 @@ -6361,24 +6361,37 @@ static void drm_parse_vesa_mso_data(struct 
 drm_connector *connector,
break;
}
  
 -  if (!info->mso_stream_count) {
 -  info->mso_pixel_overlap = 0;
 -  return;
 -  }
 +  info->mso_pixel_overlap = 0;
 +
 +  if (info->mso_stream_count) {
 +  info->mso_pixel_overlap = FIELD_GET(DISPLAYID_VESA_MSO_OVERLAP, 
 vesa->mso);
 +
 +  if (info->mso_pixel_overlap > 8) {
 +  drm_dbg_kms(connector->dev,
 +  "[CONNECTOR:%d:%s] Reserved MSO pixel 
 overlap value %u\n",
 +  connector->base.id, connector->name,
 +  info->mso_pixel_overlap);
 +  info->mso_pixel_overlap = 8;
 +  }
  
 -  info->mso_pixel_overlap = FIELD_GET(DISPLAYID_VESA_MSO_OVERLAP, 
 vesa->mso);
 -  if (info->mso_pixel_overlap > 8) {
drm_dbg_kms(connector->dev,
 -  "[CONNECTOR:%d:%s] Reserved MSO pixel overlap value 
 %u\n",
 +  "[CONNECTOR:%d:%s] MSO stream count %u, pixel 
 overlap %u\n",
connector->base.id, connector->name,
 -  info->mso_pixel_overlap);
 -  info->mso_pixel_overlap = 8;
 +  info->mso_stream_count, info->mso_pixel_overlap);
 +  }
 +
 +  if (block->num_bytes < 7) {
 +  /* DSC bpp is optional */
 +  return;
}
  
 +  info->dp_dsc_bpp = FIELD_GET(DISPLAYID_VESA_DSC_BPP_INT, 
 vesa->dsc_bpp_int) * 16
 +  + FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
 +
drm_dbg_kms(connector->dev,
 -  "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
 +  "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
connector->base.id, connector->name,
 -  info->mso_stream_count, info->mso_pixel_overlap);
 +  info->dp_dsc_bpp);
  }
  
  static void drm_update_mso(struct drm_connector *connector,
 @@ -6425,6 +6438,7 @@ static void drm_reset_display_info(struct 
 drm_connector *connector)
info->mso_s

Re: [PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper

On Tue, Feb 28, 2023 at 12:38 AM Marek Vasut  wrote:
>
> On 2/27/23 20:01, Jagan Teki wrote:
> > On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut  wrote:
> >>
> >> On 2/27/23 12:39, Jagan Teki wrote:
> >>> drm_of_dsi_find_panel_or_bridge is capable of looking up the
> >>> downstream DSI bridge and panel and trying to add a panel bridge
> >>> if the panel is found.
> >>>
> >>> Replace explicit finding calls with drm_of_dsi_find_panel_or_bridge
> >>> followed with drmm_panel_bridge_add.
> >>>
> >>> Signed-off-by: Jagan Teki 
> >>> ---
> >>> Changes for v13, v12, v11:
> >>> - none
> >>> Changes for v10:
> >>> - new patch
> >>>
> >>>drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 +
> >>>1 file changed, 13 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
> >>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> index df15501b1075..12a6dd987e8f 100644
> >>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> >>> @@ -25,6 +25,7 @@
> >>>#include 
> >>>#include 
> >>>#include 
> >>> +#include 
> >>>#include 
> >>>#include 
> >>>#include 
> >>> @@ -1470,24 +1471,26 @@ static int exynos_dsi_host_attach(struct 
> >>> mipi_dsi_host *host,
> >>>struct device *dev = dsi->dev;
> >>>struct drm_encoder *encoder = &dsi->encoder;
> >>>struct drm_device *drm = encoder->dev;
> >>> + struct drm_bridge *bridge;
> >>>struct drm_panel *panel;
> >>>int ret;
> >>>
> >>> - panel = of_drm_find_panel(device->dev.of_node);
> >>> - if (!IS_ERR(panel)) {
> >>> - dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> >>> - } else {
> >>> - dsi->out_bridge = of_drm_find_bridge(device->dev.of_node);
> >>> - if (!dsi->out_bridge)
> >>> - dsi->out_bridge = ERR_PTR(-EINVAL);
> >>> - }
> >>
> >> As far as I understand this from my conversation with Maxime (please put
> >> him on CC of V15), the change here should instead perform the panel look
> >> up NOT in exynos_dsi_host_attach() , but in exynos_dsi_bind() , i.e. in
> >> the component_ops .bind callback . Here is an example of correct
> >> implementation:
> >>
> >> https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/vc4/vc4_dsi.c#n1805
> >
> > But, we don't have component_ops.bind for imx8m case, so where do we
> > want to place the panel hook?
> >
> > Exynos drm drivers are component-based but, imx8mm is not.
>
> In 14/18 patch, the same should be added to generic_dsim_register_host()
> , which is called from the driver .probe() callback, but that is OK.
>
> That's ^ is the iMX part.

Ohh. You mean, we need to find the panel hook separately in two places like
- bind for exynos
- probe for imx8mm

If so? how can I find the drm_device pointer in the probe?

Jagan.

Re: [PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper


On 2/27/23 20:01, Jagan Teki wrote:

On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut  wrote:


On 2/27/23 12:39, Jagan Teki wrote:

drm_of_dsi_find_panel_or_bridge is capable of looking up the
downstream DSI bridge and panel and trying to add a panel bridge
if the panel is found.

Replace explicit finding calls with drm_of_dsi_find_panel_or_bridge
followed with drmm_panel_bridge_add.

Signed-off-by: Jagan Teki 
---
Changes for v13, v12, v11:
- none
Changes for v10:
- new patch

   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 +
   1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index df15501b1075..12a6dd987e8f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -25,6 +25,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include 
   #include 
@@ -1470,24 +1471,26 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host 
*host,
   struct device *dev = dsi->dev;
   struct drm_encoder *encoder = &dsi->encoder;
   struct drm_device *drm = encoder->dev;
+ struct drm_bridge *bridge;
   struct drm_panel *panel;
   int ret;

- panel = of_drm_find_panel(device->dev.of_node);
- if (!IS_ERR(panel)) {
- dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
- } else {
- dsi->out_bridge = of_drm_find_bridge(device->dev.of_node);
- if (!dsi->out_bridge)
- dsi->out_bridge = ERR_PTR(-EINVAL);
- }


As far as I understand this from my conversation with Maxime (please put
him on CC of V15), the change here should instead perform the panel look
up NOT in exynos_dsi_host_attach() , but in exynos_dsi_bind() , i.e. in
the component_ops .bind callback . Here is an example of correct
implementation:

https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/vc4/vc4_dsi.c#n1805


But, we don't have component_ops.bind for imx8m case, so where do we
want to place the panel hook?

Exynos drm drivers are component-based but, imx8mm is not.


In 14/18 patch, the same should be added to generic_dsim_register_host() 
, which is called from the driver .probe() callback, but that is OK.


That's ^ is the iMX part.

Re: [PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper

On Tue, Feb 28, 2023 at 12:25 AM Marek Vasut  wrote:
>
> On 2/27/23 12:39, Jagan Teki wrote:
> > drm_of_dsi_find_panel_or_bridge is capable of looking up the
> > downstream DSI bridge and panel and trying to add a panel bridge
> > if the panel is found.
> >
> > Replace explicit finding calls with drm_of_dsi_find_panel_or_bridge
> > followed with drmm_panel_bridge_add.
> >
> > Signed-off-by: Jagan Teki 
> > ---
> > Changes for v13, v12, v11:
> > - none
> > Changes for v10:
> > - new patch
> >
> >   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 +
> >   1 file changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
> > b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > index df15501b1075..12a6dd987e8f 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > @@ -25,6 +25,7 @@
> >   #include 
> >   #include 
> >   #include 
> > +#include 
> >   #include 
> >   #include 
> >   #include 
> > @@ -1470,24 +1471,26 @@ static int exynos_dsi_host_attach(struct 
> > mipi_dsi_host *host,
> >   struct device *dev = dsi->dev;
> >   struct drm_encoder *encoder = &dsi->encoder;
> >   struct drm_device *drm = encoder->dev;
> > + struct drm_bridge *bridge;
> >   struct drm_panel *panel;
> >   int ret;
> >
> > - panel = of_drm_find_panel(device->dev.of_node);
> > - if (!IS_ERR(panel)) {
> > - dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > - } else {
> > - dsi->out_bridge = of_drm_find_bridge(device->dev.of_node);
> > - if (!dsi->out_bridge)
> > - dsi->out_bridge = ERR_PTR(-EINVAL);
> > - }
>
> As far as I understand this from my conversation with Maxime (please put
> him on CC of V15), the change here should instead perform the panel look
> up NOT in exynos_dsi_host_attach() , but in exynos_dsi_bind() , i.e. in
> the component_ops .bind callback . Here is an example of correct
> implementation:
>
> https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/vc4/vc4_dsi.c#n1805

But, we don't have component_ops.bind for imx8m case, so where do we
want to place the panel hook?

Exynos drm drivers are component-based but, imx8mm is not.

Jagan.

Re: [PATCH drm-next v2 04/16] maple_tree: add flag MT_FLAGS_LOCK_NONE

2023-02-27 Thread Danilo Krummrich


On 2/27/23 19:36, Matthew Wilcox wrote:

On Mon, Feb 27, 2023 at 06:39:33PM +0100, Danilo Krummrich wrote:

On 2/21/23 19:31, Matthew Wilcox wrote:

Lockdep will shout at you if you get it wrong ;-)  But you can safely
take the spinlock before calling mas_store_gfp(GFP_KERNEL) because
mas_nomem() knows to drop the lock before doing a sleeping allocation.
Essentially you're open-coding mtree_store_range() but doing your own
thing in addition to the store.


As already mentioned, I went with your advice to just take the maple tree's
internal spinlock within the GPUVA manager and leave all the other locking
to the drivers as intended.

However, I run into the case that lockdep shouts at me for not taking the
spinlock before calling mas_find() in the iterator macros.

Now, I definitely don't want to let the drivers take the maple tree's
spinlock before they use the iterator macro. Of course, drivers shouldn't
even know about the underlying maple tree of the GPUVA manager.

One way to make lockdep happy in this case seems to be taking the spinlock
right before mas_find() and drop it right after for each iteration.


While we don't have any lockdep checking of this, you really shouldn't be
using an iterator if you're going to drop the lock between invocations.
The iterator points into the tree, so you need to invalidate the iterator
any time you drop the lock.


The tree can't change either way in my case. Changes to the DRM GPUVA 
manager (and hence the tree) are protected by drivers, either by 
serializing tree accesses or by having another external lock ensuring 
mutual exclusion. Just as a reminder, in the latter case drivers usually 
lock multiple transactions to the manager (and hence the tree) to ensure 
they appear atomic.


So, really the only purpose for me taking the internal lock is to ensure 
I satisfy lockdep and the maple tree's internal requirements on locking 
for future use cases you mentioned (e.g. slab cache defragmentation).


It's the rcu_dereference_check() in mas_root() that triggers in my case:

[   28.745706] lib/maple_tree.c:851 suspicious rcu_dereference_check() 
usage!


   stack backtrace:
[   28.746057] CPU: 8 PID: 1518 Comm: nouveau_dma_cop Not tainted 
6.2.0-rc6-vmbind-0.2+ #104
[   28.746061] Hardware name: ASUS System Product Name/PRIME Z690-A, 
BIOS 2103 09/30/2022

[   28.746064] Call Trace:
[   28.746067]  
[   28.746070]  dump_stack_lvl+0x5b/0x77
[   28.746077]  mas_walk+0x16d/0x1b0
[   28.746082]  mas_find+0xf7/0x300
[   28.746088]  drm_gpuva_in_region+0x63/0xa0
[   28.746099]  __drm_gpuva_sm_map.isra.0+0x465/0x9f0
[   28.746103]  ? lock_acquire+0xbf/0x2b0
[   28.746111]  ? __pfx_drm_gpuva_sm_step+0x10/0x10
[   28.746114]  ? lock_is_held_type+0xe3/0x140
[   28.746121]  ? mark_held_locks+0x49/0x80
[   28.746125]  ? _raw_spin_unlock_irqrestore+0x30/0x60
[   28.746138]  drm_gpuva_sm_map_ops_create+0x80/0xc0
[   28.746145]  uvmm_bind_job_submit+0x3c2/0x470 [nouveau]
[   28.746272]  nouveau_job_submit+0x60/0x450 [nouveau]
[   28.746393]  nouveau_uvmm_ioctl_vm_bind+0x179/0x1e0 [nouveau]
[   28.746510]  ? __pfx_nouveau_uvmm_ioctl_vm_bind+0x10/0x10 [nouveau]
[   28.746622]  drm_ioctl_kernel+0xa9/0x160
[   28.746629]  drm_ioctl+0x1f7/0x4b0



You don't have to use a spinlock to do a read iteration.  You can just
take the rcu_read_lock() around your iteration, as long as you can
tolerate the mild inconsistencies that RCU permits.



Doing that would mean that the driver needs to do it. However, the 
driver either needs to serialize accesses or use it's own mutex for 
protection for the above reasons. Hence, that should not be needed.

Re: [PATCH v1] drm/msm/disp/dpu: fix sc7280_pp base address

2023-02-27 Thread Stephen Boyd

Quoting Kuogee Hsieh (2023-02-27 09:04:31)
> Correct sc7280 pp block base address.

What goes wrong if this is left unchanged? How important is it to fix
this? Does the display fail to work? Does it fix something for a new
feature that isn't yet enabled upstream?

This information is useful to put in the commit text so we know the
severity of the problem that is being fixed and so that maintainers can
understand the importance of the patch.

>
> Fixes: 591e34a091d1 ("drm/msm/disp/dpu1: add support for display for SC7280 
> target")
> Signed-off-by: Kuogee Hsieh

Re: [PATCH v13 04/18] drm: exynos: dsi: Switch to DSI panel or bridge find helper


On 2/27/23 12:39, Jagan Teki wrote:

drm_of_dsi_find_panel_or_bridge is capable of looking up the
downstream DSI bridge and panel and trying to add a panel bridge
if the panel is found.

Replace explicit finding calls with drm_of_dsi_find_panel_or_bridge
followed with drmm_panel_bridge_add.

Signed-off-by: Jagan Teki 
---
Changes for v13, v12, v11:
- none
Changes for v10:
- new patch

  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 25 +
  1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index df15501b1075..12a6dd987e8f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -25,6 +25,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -1470,24 +1471,26 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host 
*host,
struct device *dev = dsi->dev;
struct drm_encoder *encoder = &dsi->encoder;
struct drm_device *drm = encoder->dev;
+   struct drm_bridge *bridge;
struct drm_panel *panel;
int ret;
  
-	panel = of_drm_find_panel(device->dev.of_node);

-   if (!IS_ERR(panel)) {
-   dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
-   } else {
-   dsi->out_bridge = of_drm_find_bridge(device->dev.of_node);
-   if (!dsi->out_bridge)
-   dsi->out_bridge = ERR_PTR(-EINVAL);
-   }


As far as I understand this from my conversation with Maxime (please put 
him on CC of V15), the change here should instead perform the panel look 
up NOT in exynos_dsi_host_attach() , but in exynos_dsi_bind() , i.e. in 
the component_ops .bind callback . Here is an example of correct 
implementation:


https://cgit.freedesktop.org/drm-misc/tree/drivers/gpu/drm/vc4/vc4_dsi.c#n1805

In 14/18 patch, the same should be added to generic_dsim_register_host() 
, which is called from the driver .probe() callback, but that is OK.


With that implemented, you shouldn't need patches 1,2,3/18 anymore , so 
they can be dropped . And with this fixed, I think the driver should be 
good to be picked.


Please correct me if I'm wrong here.

Re: [PATCH v3] drm/virtio: Add option to disable KMS support

2023-02-27 Thread Dmitry Osipenko

On 2/27/23 20:38, Rob Clark wrote:
>  static const struct drm_driver driver = {
> - .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | 
> DRIVER_ATOMIC,
> + .driver_features =
> +#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)

I'd also replace the `#if defined()` with `#if IS_ENABLED()`, for
consistency.

Maybe won't hurt to expand the commit message a tad, emphasizing the
security aspect and telling about the new num_scanouts=0 behaviour.

I can change it all while applying, if Gerd is okay with this patch.

Othwerise, good to me:

Reviewed-by: Dmitry Osipenko 

-- 
Best regards,
Dmitry

Re: [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI

On Mon, Feb 27, 2023 at 1:34 AM Pekka Paalanen  wrote:
>
> On Fri, 24 Feb 2023 11:44:53 -0800
> Rob Clark  wrote:
>
> > On Fri, Feb 24, 2023 at 2:24 AM Pekka Paalanen  wrote:
> > >
> > > On Fri, 24 Feb 2023 09:41:46 +
> > > Tvrtko Ursulin  wrote:
> > >
> > > > On 24/02/2023 09:26, Pekka Paalanen wrote:
> > > > > On Thu, 23 Feb 2023 10:51:48 -0800
> > > > > Rob Clark  wrote:
> > > > >
> > > > >> On Thu, Feb 23, 2023 at 1:38 AM Pekka Paalanen  
> > > > >> wrote:
> > > > >>>
> > > > >>> On Wed, 22 Feb 2023 07:37:26 -0800
> > > > >>> Rob Clark  wrote:
> > > > >>>
> > > >  On Wed, Feb 22, 2023 at 1:49 AM Pekka Paalanen 
> > > >   wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > On another matter, if the application uses SET_DEADLINE with one
> > > > > timestamp, and the compositor uses SET_DEADLINE on the same thing 
> > > > > with
> > > > > another timestamp, what should happen?
> > > > 
> > > >  The expectation is that many deadline hints can be set on a fence.
> > > >  The fence signaller should track the soonest deadline.
> > > > >>>
> > > > >>> You need to document that as UAPI, since it is observable to 
> > > > >>> userspace.
> > > > >>> It would be bad if drivers or subsystems would differ in behaviour.
> > > > >>>
> > > > >>
> > > > >> It is in the end a hint.  It is about giving the driver more
> > > > >> information so that it can make better choices.  But the driver is
> > > > >> even free to ignore it.  So maybe "expectation" is too strong of a
> > > > >> word.  Rather, any other behavior doesn't really make sense.  But it
> > > > >> could end up being dictated by how the hw and/or fw works.
> > > > >
> > > > > It will stop being a hint once it has been implemented and used in the
> > > > > wild long enough. The kernel userspace regression rules make sure of
> > > > > that.
> > > >
> > > > Yeah, tricky and maybe a gray area in this case. I think we eluded
> > > > elsewhere in the thread that renaming the thing might be an option.
> > > >
> > > > So maybe instead of deadline, which is a very strong word, use something
> > > > along the lines of "present time hint", or "signalled time hint"? Maybe
> > > > reads clumsy. Just throwing some ideas for a start.
> > >
> > > You can try, but I fear that if it ever changes behaviour and
> > > someone notices that, it's labelled as a kernel regression. I don't
> > > think documentation has ever been the authoritative definition of UABI
> > > in Linux, it just guides drivers and userspace towards a common
> > > understanding and common usage patterns.
> > >
> > > So even if the UABI contract is not documented (ugh), you need to be
> > > prepared to set the UABI contract through kernel implementation.
> > >
> > > If you do not document the UABI contract, then different drivers are
> > > likely to implement it differently, leading to differing behaviour.
> > > Also userspace will invent wild ways to abuse the UABI if there is no
> > > documentation guiding it on proper use. If userspace or end users
> > > observe different behaviour, that's bad even if it's not a regression.
> > >
> > > I don't like the situation either, but it is what it is. UABI stability
> > > trumps everything regardless of whether it was documented or not.
> > >
> > > I bet userspace is going to use this as a "make it faster, make it
> > > hotter" button. I would not be surprised if someone wrote a LD_PRELOAD
> > > library that stamps any and all fences with an expired deadline to
> > > just squeeze out a little more through some weird side-effect.
> >
> > Userspace already has various (driver specific) debugfs/sysfs that it
> > can use if it wants to make it hotter and drain batteries faster, so
> > I'm not seeing a strong need to cater to the "turn it up to eleven"
> > crowd here.  And really your point feels like a good reason to _not_
> > document this as anything more than a hint.
>
> My point is that no matter what you say in documentation or leave
> unsaid, people can and will abuse this by the behaviour it provides
> anyway, like every other UABI.
>
> So why not just document what it is supposed to do? It cannot get any
> worse. Maybe you get lucky instead and people don't abuse it that much
> if they read the docs.
>
> E.g. can it affect GPU job scheduling, can it affect GPU clocks, can it
> affect power consumption, and so on.

I expect, potentially, all or any, or none of the above ;-)

I guess I could document it as such, just to preempt potential
complaints about broken spacebar-heater.  The question is, where?  I
could add something about fence deadline hints in dma-buf.rst, which
would I think be useful in general for driver writers.  But there
isn't really any existing documents other than headerdoc comments for
sync_file ioctls, or drm_syncobj related ioctls (the latter are really
just for mesa to use, so maybe that is ok).

>
> > Back in the real world, mobile games are already well aware of the fps
> > vs battery-life (and therefo

Re: [PATCH drm-next v2 04/16] maple_tree: add flag MT_FLAGS_LOCK_NONE

2023-02-27 Thread Matthew Wilcox

On Mon, Feb 27, 2023 at 06:39:33PM +0100, Danilo Krummrich wrote:
> On 2/21/23 19:31, Matthew Wilcox wrote:
> > Lockdep will shout at you if you get it wrong ;-)  But you can safely
> > take the spinlock before calling mas_store_gfp(GFP_KERNEL) because
> > mas_nomem() knows to drop the lock before doing a sleeping allocation.
> > Essentially you're open-coding mtree_store_range() but doing your own
> > thing in addition to the store.
> 
> As already mentioned, I went with your advice to just take the maple tree's
> internal spinlock within the GPUVA manager and leave all the other locking
> to the drivers as intended.
> 
> However, I run into the case that lockdep shouts at me for not taking the
> spinlock before calling mas_find() in the iterator macros.
> 
> Now, I definitely don't want to let the drivers take the maple tree's
> spinlock before they use the iterator macro. Of course, drivers shouldn't
> even know about the underlying maple tree of the GPUVA manager.
> 
> One way to make lockdep happy in this case seems to be taking the spinlock
> right before mas_find() and drop it right after for each iteration.

While we don't have any lockdep checking of this, you really shouldn't be
using an iterator if you're going to drop the lock between invocations.
The iterator points into the tree, so you need to invalidate the iterator
any time you drop the lock.

You don't have to use a spinlock to do a read iteration.  You can just
take the rcu_read_lock() around your iteration, as long as you can
tolerate the mild inconsistencies that RCU permits.

Re: [PATCH v3 1/2] drm/edid: parse DRM VESA dsc bpp target

2023-02-27 Thread Harry Wentland




On 2/27/23 12:12, Jani Nikula wrote:
> On Mon, 27 Feb 2023, Harry Wentland  wrote:
>> On 2/26/23 09:10, Yaroslav Bolyukin wrote:
>>> As per DisplayID v2.0 Errata E9 spec "DSC pass-through timing support"
>>> VESA vendor-specific data block may contain target DSC bits per pixel
>>> fields
>>>
>>
>> According to the errata this should only apply to VII timings. The way
>> it is currently implemented will make it apply to everything which is
>> not what we want.
>>
>> Can we add this field to drm_mode_info instead of drm_display_info and
>> set it inside drm_mode_displayid_detailed when parsing a type_7 timing?
> 
> That's actually difficult to do nicely. I think the patch at hand is
> fine, and it's fine to add the information to drm_display_info. It's a
> dependency to parsing the modes.
> 
> How the info will actually be used is a different matter, and obviously
> needs to follow the spec. As it is, *this* patch doesn't say anything
> about that. But whether it's handled in VII timings parsing or
> elsewhere, I still think this part is fine.
> 

This patch itself is okay but without further work can't be used by
drivers since they don't currently have an indication whether a mode
is based on a VII timing.

Harry

> 
> BR,
> Jani.
> 
>>
>> Harry
>>
>>
>>> Signed-off-by: Yaroslav Bolyukin 
>>> ---
>>>  drivers/gpu/drm/drm_edid.c  | 38 +
>>>  include/drm/drm_connector.h |  6 ++
>>>  include/drm/drm_displayid.h |  4 
>>>  3 files changed, 36 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 3d0a4da661bc..aa88ac82cbe0 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -6338,7 +6338,7 @@ static void drm_parse_vesa_mso_data(struct 
>>> drm_connector *connector,
>>> if (oui(vesa->oui[0], vesa->oui[1], vesa->oui[2]) != VESA_IEEE_OUI)
>>> return;
>>>  
>>> -   if (sizeof(*vesa) != sizeof(*block) + block->num_bytes) {
>>> +   if (block->num_bytes < 5) {
>>> drm_dbg_kms(connector->dev,
>>> "[CONNECTOR:%d:%s] Unexpected VESA vendor block 
>>> size\n",
>>> connector->base.id, connector->name);
>>> @@ -6361,24 +6361,37 @@ static void drm_parse_vesa_mso_data(struct 
>>> drm_connector *connector,
>>> break;
>>> }
>>>  
>>> -   if (!info->mso_stream_count) {
>>> -   info->mso_pixel_overlap = 0;
>>> -   return;
>>> -   }
>>> +   info->mso_pixel_overlap = 0;
>>> +
>>> +   if (info->mso_stream_count) {
>>> +   info->mso_pixel_overlap = FIELD_GET(DISPLAYID_VESA_MSO_OVERLAP, 
>>> vesa->mso);
>>> +
>>> +   if (info->mso_pixel_overlap > 8) {
>>> +   drm_dbg_kms(connector->dev,
>>> +   "[CONNECTOR:%d:%s] Reserved MSO pixel 
>>> overlap value %u\n",
>>> +   connector->base.id, connector->name,
>>> +   info->mso_pixel_overlap);
>>> +   info->mso_pixel_overlap = 8;
>>> +   }
>>>  
>>> -   info->mso_pixel_overlap = FIELD_GET(DISPLAYID_VESA_MSO_OVERLAP, 
>>> vesa->mso);
>>> -   if (info->mso_pixel_overlap > 8) {
>>> drm_dbg_kms(connector->dev,
>>> -   "[CONNECTOR:%d:%s] Reserved MSO pixel overlap value 
>>> %u\n",
>>> +   "[CONNECTOR:%d:%s] MSO stream count %u, pixel 
>>> overlap %u\n",
>>> connector->base.id, connector->name,
>>> -   info->mso_pixel_overlap);
>>> -   info->mso_pixel_overlap = 8;
>>> +   info->mso_stream_count, info->mso_pixel_overlap);
>>> +   }
>>> +
>>> +   if (block->num_bytes < 7) {
>>> +   /* DSC bpp is optional */
>>> +   return;
>>> }
>>>  
>>> +   info->dp_dsc_bpp = FIELD_GET(DISPLAYID_VESA_DSC_BPP_INT, 
>>> vesa->dsc_bpp_int) * 16
>>> +   + FIELD_GET(DISPLAYID_VESA_DSC_BPP_FRACT, vesa->dsc_bpp_fract);
>>> +
>>> drm_dbg_kms(connector->dev,
>>> -   "[CONNECTOR:%d:%s] MSO stream count %u, pixel overlap %u\n",
>>> +   "[CONNECTOR:%d:%s] DSC bits per pixel %u\n",
>>> connector->base.id, connector->name,
>>> -   info->mso_stream_count, info->mso_pixel_overlap);
>>> +   info->dp_dsc_bpp);
>>>  }
>>>  
>>>  static void drm_update_mso(struct drm_connector *connector,
>>> @@ -6425,6 +6438,7 @@ static void drm_reset_display_info(struct 
>>> drm_connector *connector)
>>> info->mso_stream_count = 0;
>>> info->mso_pixel_overlap = 0;
>>> info->max_dsc_bpp = 0;
>>> +   info->dp_dsc_bpp = 0;
>>>  
>>> kfree(info->vics);
>>> info->vics = NULL;
>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>> index 7b5048516185..1d01e0146a7f 100644
>>> --- a/include/drm/drm_connector.h
>>> +++ b/include/drm/drm_connector.h
>>> @@ -719,6 +719,12 @@ struct drm_display_info {
>>>  */
>>>

Re: [PATCH v3 2/2] drm/amd: use fixed dsc bits-per-pixel from edid

2023-02-27 Thread Harry Wentland

On 2/26/23 09:10, Yaroslav Bolyukin wrote:
> VESA vendor header from DisplayID spec may contain fixed bit per pixel
> rate, it should be respected by drm driver
> 

This will apply the fixed bpp for all modes. I don't think that's right.
It should apply only to VII timings.

Harry

> Signed-off-by: Yaroslav Bolyukin 
> Reviewed-by: Wayne Lin 
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 ++
>  drivers/gpu/drm/amd/display/dc/core/dc_stream.c   | 2 ++
>  drivers/gpu/drm/amd/display/dc/dc_types.h | 3 +++
>  3 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 6fdc2027c2b4..dba720d5df4c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -89,6 +89,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>  
>   edid_caps->edid_hdmi = connector->display_info.is_hdmi;
>  
> + edid_caps->dsc_fixed_bits_per_pixel_x16 = 
> connector->display_info.dp_dsc_bpp;
> +
>   sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads);
>   if (sad_count <= 0)
>   return result;
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c 
> b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
> index 72b261ad9587..a82362417379 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_stream.c
> @@ -103,6 +103,8 @@ static bool dc_stream_construct(struct dc_stream_state 
> *stream,
>  
>   /* EDID CAP translation for HDMI 2.0 */
>   stream->timing.flags.LTE_340MCSC_SCRAMBLE = 
> dc_sink_data->edid_caps.lte_340mcsc_scramble;
> + stream->timing.dsc_fixed_bits_per_pixel_x16 =
> + dc_sink_data->edid_caps.dsc_fixed_bits_per_pixel_x16;
>  
>   memset(&stream->timing.dsc_cfg, 0, sizeof(stream->timing.dsc_cfg));
>   stream->timing.dsc_cfg.num_slices_h = 0;
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_types.h 
> b/drivers/gpu/drm/amd/display/dc/dc_types.h
> index 27d0242d6cbd..22fedf4c7547 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_types.h
> @@ -228,6 +228,9 @@ struct dc_edid_caps {
>   bool edid_hdmi;
>   bool hdr_supported;
>  
> + /* DisplayPort caps */
> + uint32_t dsc_fixed_bits_per_pixel_x16;
> +
>   struct dc_panel_patch panel_patch;
>  };
>

Re: [PATCH v3] drm/virtio: Add option to disable KMS support

On Mon, Feb 27, 2023 at 9:57 AM Dmitry Osipenko
 wrote:
>
> On 2/27/23 20:38, Rob Clark wrote:
> ...
> > + if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS)) {
> > + /* get display info */
> > + virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
> > + num_scanouts, &num_scanouts);
> > + vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
> > + VIRTIO_GPU_MAX_SCANOUTS);
> > + if (!vgdev->num_scanouts) {
> > + /*
> > +  * Having an EDID but no scanouts is non-sensical,
> > +  * but it is permitted to have no scanouts and no
> > +  * EDID (in which case DRIVER_MODESET and
> > +  * DRIVER_ATOMIC are not advertised)
> > +  */
> > + if (vgdev->has_edid) {
> > + DRM_ERROR("num_scanouts is zero\n");
> > + ret = -EINVAL;
> > + goto err_scanouts;
> > + }
> > + dev->driver_features &= ~(DRIVER_MODESET | 
> > DRIVER_ATOMIC);
>
> If it's now configurable by host, why do we need the
> CONFIG_DRM_VIRTIO_GPU_KMS?

Because a kernel config option makes it more obvious that
modeset/atomic ioctls are blocked.  Which makes it more obvious about
where any potential security issues apply and where fixes need to get
backported to.  The config option is the only thing _I_ want,
everything else is just a bonus to help other people's use-cases.

BR,
-R

Re: [PATCH v14 2/3] drm: bridge: panel: Add drmm_panel_bridge_add_nodrm