[PATCH 11/12] drm/msm/dpu: gracefully handle null fb commits for writeback
Abhinav Kumar
quic_abhinavk at quicinc.com
Fri Apr 15 01:50:17 UTC 2022
On 4/14/2022 5:36 PM, Dmitry Baryshkov wrote:
> On 15/04/2022 02:17, Abhinav Kumar wrote:
>>
>>
>> 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 at quicinc.com>
>>>> ---
>>>> 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.
>
> Good!
>
>>
>>>
>>> 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.
>
> I see. Let's settle down on a question which should have been aksed
> initially. If we have a set of encoders, some of which are valid and
> others are not (for whatever reason), should we proceed with kickoff of
> the valid ones or should we skip it completely? Which encoder would call
> the frame_done callbacks, in the end decrementing the frame_pending?
Well, to answer this question, lets consider two cases:
1) If we are talking about only the encoder having a valid fb as the
criteria for validity, and lets say there are two encoders one having
valid fb and the other not having one. This is not possible.
We map the same fb which is passed from the prepare_wb_job to both the
physical encoders today so such a case wont happen.
2) In future, if we extend this valid check for other cases like you are
suggesting here, even in that case, lets say we have two physical
encoders, one master and the other slave. Even if one of them is
invalid, the frame is invalid in my opinion as it will end up displaying
corruption on the screen.
I dont think we are missing any other case here.
IF we just consider these two, then perhaps current approach is good
enough with just the renaming ofcourse from dpu_encoder_has_valid_fb
---> dpu_encoder_is_valid_for_commit()
>
>>
>> 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;
>>>> +
>>>> }
>>>> /**
>>>
>>>
>
>
More information about the dri-devel
mailing list