Re: [PATCH v3 2/3] drm/msm/dpu: split dpu_encoder_wait_for_event into two functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 2/25/2024 12:52 PM, Dmitry Baryshkov wrote:
On Sun, 25 Feb 2024 at 21:49, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:



On 2/25/2024 6:12 AM, Dmitry Baryshkov wrote:
Stop multiplexing several events via the dpu_encoder_wait_for_event()
function. Split it into two distinct functions two allow separate
handling of those events.


I understand the idea but would handling of those events be really distinct?

We are interested in capturing the state after the first
wait_for_commit_done() timeout. The wait_for_tx_complete doesn't need
such handling. Even if we were to handle it in some way, it would be a
different conditional.


wait_for_tx_complete timeout also needs similar handling.

the timeout mechanisms need to be unified at some point, that time I will re-visit the need to have a common wait_timeout handler.

Lets see how this code evolves.

So with that nit about the kernel doc addressed,

Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>

Last but not least, I don't like multiplexing just for the sake of it.
There is nearly no common behaviour.


the multiplexing allows us to have one common timeout path which I think will eventually happen. So i dont think its true that there is no common behavior.


Like if wait_for_commit_done timedout or wait_for_tx_complete timedout,
the handling will be similar from my PoV.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 74 +++++++++++++++++++++--------
   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 22 ++-------
   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
   drivers/gpu/drm/msm/msm_drv.h               | 10 ----
   4 files changed, 59 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 194dbb08331d..30f349c8a1e5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1282,7 +1282,7 @@ static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc,
       trace_dpu_enc_disable(DRMID(drm_enc));

       /* wait for idle */
-     dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
+     dpu_encoder_wait_for_tx_complete(drm_enc);

       dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_PRE_STOP);

@@ -2402,10 +2402,23 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
       return &dpu_enc->base;
   }

-int dpu_encoder_wait_for_event(struct drm_encoder *drm_enc,
-     enum msm_event_wait event)
+/**
+ * dpu_encoder_wait_for_commit_done() - Wait for encoder to flush pending state
+ * @drm_enc: encoder pointer
+ *
+ * Wait for hardware to have flushed the current pending frames to hardware at
+ * a vblank or ctl_start Encoders will map this differently depending on the
+ * panel type.
+ *

Missing a '.' between ctl_start and Encoder?

Ack. Also I should drop the leftovers afterwards.


+ * MSM_ENC_TX_COMPLETE -  Wait for the hardware to transfer all the pixels to
+ *                        the panel. Encoders will map this differently
+ *                        depending on the panel type.
+ *                        vid mode -> vsync_irq
+ *                        cmd mode -> pp_done
+ * Return: 0 on success, -EWOULDBLOCK if already signaled, error otherwise
+ */
+int dpu_encoder_wait_for_commit_done(struct drm_encoder *drm_enc)
   {
-     int (*fn_wait)(struct dpu_encoder_phys *phys_enc) = NULL;
       struct dpu_encoder_virt *dpu_enc = NULL;
       int i, ret = 0;

@@ -2419,23 +2432,46 @@ int dpu_encoder_wait_for_event(struct drm_encoder *drm_enc,
       for (i = 0; i < dpu_enc->num_phys_encs; i++) {
               struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];

-             switch (event) {
-             case MSM_ENC_COMMIT_DONE:
-                     fn_wait = phys->ops.wait_for_commit_done;
-                     break;
-             case MSM_ENC_TX_COMPLETE:
-                     fn_wait = phys->ops.wait_for_tx_complete;
-                     break;
-             default:
-                     DPU_ERROR_ENC(dpu_enc, "unknown wait event %d\n",
-                                     event);
-                     return -EINVAL;
+             if (phys->ops.wait_for_commit_done) {
+                     DPU_ATRACE_BEGIN("wait_for_commit_done");
+                     ret = phys->ops.wait_for_commit_done(phys);
+                     DPU_ATRACE_END("wait_for_commit_done");
+                     if (ret)
+                             return ret;
               }
+     }
+
+     return ret;
+}
+
+/**
+ * dpu_encoder_wait_for_tx_complete() - Wait for encoder to transfer pixels to panel
+ * @drm_enc: encoder pointer
+ *
+ * Wait for the hardware to transfer all the pixels to the panel. Encoders will
+ * map this differently depending on the panel type.
+ *
+ * Return: 0 on success, -EWOULDBLOCK if already signaled, error otherwise
+ */
+int dpu_encoder_wait_for_tx_complete(struct drm_encoder *drm_enc)
+{
+     struct dpu_encoder_virt *dpu_enc = NULL;
+     int i, ret = 0;
+
+     if (!drm_enc) {
+             DPU_ERROR("invalid encoder\n");
+             return -EINVAL;
+     }
+     dpu_enc = to_dpu_encoder_virt(drm_enc);
+     DPU_DEBUG_ENC(dpu_enc, "\n");
+
+     for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+             struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];

-             if (fn_wait) {
-                     DPU_ATRACE_BEGIN("wait_for_completion_event");
-                     ret = fn_wait(phys);
-                     DPU_ATRACE_END("wait_for_completion_event");
+             if (phys->ops.wait_for_tx_complete) {
+                     DPU_ATRACE_BEGIN("wait_for_tx_complete");
+                     ret = phys->ops.wait_for_tx_complete(phys);
+                     DPU_ATRACE_END("wait_for_tx_complete");
                       if (ret)
                               return ret;
               }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index fe6b1d312a74..0c928d1876e4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -93,25 +93,9 @@ void dpu_encoder_kickoff(struct drm_encoder *encoder);
    */
   int dpu_encoder_vsync_time(struct drm_encoder *drm_enc, ktime_t *wakeup_time);

-/**
- * dpu_encoder_wait_for_event - Waits for encoder events
- * @encoder: encoder pointer
- * @event:      event to wait for
- * MSM_ENC_COMMIT_DONE -  Wait for hardware to have flushed the current pending
- *                        frames to hardware at a vblank or ctl_start
- *                        Encoders will map this differently depending on the
- *                        panel type.
- *                     vid mode -> vsync_irq
- *                        cmd mode -> ctl_start
- * MSM_ENC_TX_COMPLETE -  Wait for the hardware to transfer all the pixels to
- *                        the panel. Encoders will map this differently
- *                        depending on the panel type.
- *                        vid mode -> vsync_irq
- *                        cmd mode -> pp_done
- * Returns: 0 on success, -EWOULDBLOCK if already signaled, error otherwise
- */
-int dpu_encoder_wait_for_event(struct drm_encoder *drm_encoder,
-                                             enum msm_event_wait event);
+int dpu_encoder_wait_for_commit_done(struct drm_encoder *drm_encoder);
+
+int dpu_encoder_wait_for_tx_complete(struct drm_encoder *drm_encoder);

   /*
    * dpu_encoder_get_intf_mode - get interface mode of the given encoder
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index d6412395bacc..26b5e54031d9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -476,7 +476,7 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms,
                * mode panels. This may be a no-op for command mode panels.
                */
               trace_dpu_kms_wait_for_commit_done(DRMID(crtc));
-             ret = dpu_encoder_wait_for_event(encoder, MSM_ENC_COMMIT_DONE);
+             ret = dpu_encoder_wait_for_commit_done(encoder);
               if (ret && ret != -EWOULDBLOCK) {
                       DPU_ERROR("wait for commit done returned %d\n", ret);
                       break;
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 762e13e2df74..91cf57f72321 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -74,16 +74,6 @@ enum msm_dsi_controller {
   #define MSM_GPU_MAX_RINGS 4
   #define MAX_H_TILES_PER_DISPLAY 2

-/**
- * enum msm_event_wait - type of HW events to wait for
- * @MSM_ENC_COMMIT_DONE - wait for the driver to flush the registers to HW
- * @MSM_ENC_TX_COMPLETE - wait for the HW to transfer the frame to panel
- */
-enum msm_event_wait {
-     MSM_ENC_COMMIT_DONE = 0,
-     MSM_ENC_TX_COMPLETE,
-};
-
   /**
    * struct msm_display_topology - defines a display topology pipeline
    * @num_lm:       number of layer mixers used







[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux