Re: [PATCH 11/12] drm/msm/dpu: gracefully handle null fb commits for writeback

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

 





On 2/4/2022 2:43 PM, Dmitry Baryshkov wrote:
On 05/02/2022 00:17, Abhinav Kumar wrote:
kms_writeback test cases also verify with a null fb for the
writeback connector job. In addition there are also other
commit paths which can result in kickoffs without a valid
framebuffer like while closing the fb which results in the
callback to drm_atomic_helper_dirtyfb() which internally
triggers a commit.

Add protection in the dpu driver to ensure that commits for
writeback encoders without a valid fb are gracefully skipped.

Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c            |  9 +++++++++
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c         | 21 +++++++++++++++++++++
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h         |  6 ++++++
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h    |  1 +
  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 12 ++++++++++++
  5 files changed, 49 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index e7c9fe1..f7963b0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -869,6 +869,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
      DPU_ATRACE_BEGIN("crtc_commit");
+    drm_for_each_encoder_mask(encoder, crtc->dev,
+            crtc->state->encoder_mask) {
+        if (!dpu_encoder_has_valid_fb(encoder)) {

Two small comments here. First, let's probably rename the function to dpu_encoder_is_valid() or dpu_encoder_is_valid_for_commit() (ugh, too long). There might be other cases (in theory), why encoder is invalid at this moment.

dpu_encoder_is_valid_for_commit() seems fine to me even if long.


Also (a minor nit): I think that we should commit if at least one of encoders is valid. So we might want to create an encoder_valid_mask basing on the calls to dpu_encoder. And then use it later for calling dpu_encoder_prepare_for_kickoff() and dpu_encoder_kickoff().

Its not just these two calls. These can be easily skipped within the encoder itself. I had to bring this to the dpu_crtc level because of the frame_pending.

The issue is atomic_inc_return(&dpu_crtc->frame_pending)

We have to skip this call otherwise it leads to incorrect "frame done timeouts" because CRTC thinks frame was kicked off but it was actually skipped.

Maybe, what we can do is first prepare the mask.

if (hweight(crtc_encoder_mask)) {
	if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
        /* acquire bandwidth and other resources */
        DRM_DEBUG_ATOMIC("crtc%d first commit\n", crtc->base.id);
    	} else
        	DRM_DEBUG_ATOMIC("crtc%d commit\n", crtc->base.id);

  	dpu_crtc->play_count++;

    	dpu_vbif_clear_errors(dpu_kms);
}	
	
do the encoder_kickoff

if (hweight(crtc_encoder_mask))
	reinit_completion(&dpu_crtc->frame_done_comp);
	
calls to dpu_encoder_prepare_for_kickoff() and dpu_encoder_kickoff() can be protected by dpu_encoder_is_valid_for_commit() checks.

This is probably the best we can do here.

Let me know what you think.


+            DRM_DEBUG_ATOMIC("invalid FB not kicking off crtc\n");
+            goto end;
+        }
+    }
      /*
       * Encoder will flush/start now, unless it has a tx pending. If so, it
       * may delay and flush at an irq event (e.g. ppdone)
@@ -891,6 +898,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
          dpu_encoder_kickoff(encoder);
      reinit_completion(&dpu_crtc->frame_done_comp);
+
+end:
      DPU_ATRACE_END("crtc_commit");
  }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 3746432..e990dbc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1832,6 +1832,27 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc)
      }
  }
+bool dpu_encoder_has_valid_fb(struct drm_encoder *drm_enc)
+{
+    struct dpu_encoder_virt *dpu_enc;
+    unsigned int i;
+    struct dpu_encoder_phys *phys;
+
+    dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+    if (drm_enc->encoder_type == DRM_MODE_ENCODER_VIRTUAL) {
+        for (i = 0; i < dpu_enc->num_phys_encs; i++) {
+            phys = dpu_enc->phys_encs[i];
+            if (phys->ops.has_valid_output_fb && !phys->ops.has_valid_output_fb(phys)) {
+                DPU_DEBUG("invalid FB not kicking off\n");
+                return false;
+            }
+        }
+    }
+
+    return true;
+}
+
  void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
  {
      struct dpu_encoder_virt *dpu_enc;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index da5b6d6..63d90b8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -187,4 +187,10 @@ void dpu_encoder_prepare_wb_job(struct drm_encoder *drm_enc,
  void dpu_encoder_cleanup_wb_job(struct drm_encoder *drm_enc,
          struct drm_writeback_job *job);
+/**
+ * dpu_encoder_has_valid_fb - cleanup writeback job for the encoder.
+ * @drm_enc:    Pointer to drm encoder structure
+ */
+bool dpu_encoder_has_valid_fb(struct drm_encoder *drm_enc);
+
  #endif /* __DPU_ENCODER_H__ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 80da0a9..5b45b3c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -151,6 +151,7 @@ struct dpu_encoder_phys_ops {
              struct drm_writeback_job *job);
      void (*cleanup_wb_job)(struct dpu_encoder_phys *phys_enc,
              struct drm_writeback_job *job);
+    bool (*has_valid_output_fb)(struct dpu_encoder_phys *phys_enc);
  };
  /**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 783f83e..7eeed79 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -717,6 +717,16 @@ static void dpu_encoder_phys_wb_cleanup_wb_job(struct dpu_encoder_phys *phys_enc
      wb_enc->wb_conn = NULL;
  }
+static bool dpu_encoder_phys_wb_has_valid_fb(struct dpu_encoder_phys *phys_enc)
+{
+    struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc);
+
+    if (wb_enc->wb_job)
+        return true;
+    else
+        return false;
+}
+
  /**
   * dpu_encoder_phys_wb_init_ops - initialize writeback operations
   * @ops:    Pointer to encoder operation table
@@ -738,6 +748,8 @@ static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
      ops->prepare_wb_job = dpu_encoder_phys_wb_prepare_wb_job;
      ops->cleanup_wb_job = dpu_encoder_phys_wb_cleanup_wb_job;
      ops->irq_control = dpu_encoder_phys_wb_irq_ctrl;
+    ops->has_valid_output_fb = dpu_encoder_phys_wb_has_valid_fb;
+
  }
  /**





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux