Re: [PATCH v2 16/17] drm/msm/dpu: gracefully handle null fb commits for writeback

On 20/04/2022 04:46, 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.

changes in v2:

• rename dpu_encoder_has_valid_fb to dpu_encoder_is_valid_for_commit

Signed-off-by: Abhinav Kumar quic_abhinavk@quicinc.com

Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org

---

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 7763558..d65e124 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_is_valid_for_commit(encoder)) {
  

• 	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 b1475dd..d07e3ee 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1850,6 +1850,27 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) dpu_encoder_prep_dsc(dpu_enc, dpu_enc->dsc); }

+bool dpu_encoder_is_valid_for_commit(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.is_valid_for_commit && !phys->ops.is_valid_for_commit(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 6ceec1d..781d41c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -196,4 +196,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_is_valid_for_commit - check if encode has valid parameters for commit.
• • @drm_enc: Pointer to drm encoder structure
• */

+bool dpu_encoder_is_valid_for_commit(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 5452f98..04d037e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h @@ -142,6 +142,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 (*is_valid_for_commit)(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 128317fe..9acbce0 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 @@ -667,6 +667,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_is_valid_for_commit(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

@@ -687,6 +697,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->is_valid_for_commit = dpu_encoder_phys_wb_is_valid_for_commit;

• }

  /**