[PATCH v3 2/2] drm/msm/dpu: Add mutex lock in control vblank irq

[Abhinav Kumar]

On 12/1/2023 8:22 AM, Bjorn Andersson wrote:
> On Fri, Dec 01, 2023 at 10:34:50AM +0200, Dmitry Baryshkov wrote:
>> On Fri, 1 Dec 2023 at 05:47, Bjorn Andersson <quic_bjorande at quicinc.com> wrote:
>>> On Thu, Nov 30, 2023 at 05:40:55PM -0800, Paloma Arellano wrote:
> [..]
>>>> @@ -2386,6 +2390,7 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>>>>        dpu_enc->enabled = false;
>>>>        mutex_init(&dpu_enc->enc_lock);
>>>>        mutex_init(&dpu_enc->rc_lock);
>>>> +     mutex_init(&dpu_enc->vblank_ctl_lock);
>>>
>>> Is this somehow propagated to multiple different dpu_encoder_phys
>>> instances, or why do you need to initialize it here and pass the pointer
>>> through 2 different intermediate structures before assigning it to
>>> phys_enc->vblank_ctl_lock below?
>>
>> Yes, there can be two phys_enc instances for a single encoder, so this
>> part is fine.
>>
> 
> Thanks for the clarification, Dmitry. Sounds like it make sense then.
> 
> But, if I read the code correctly the two instances will have separate
> vblank_refcount copies, and the dpu_core_irq_*() interface does mutual
> exclusion within. So why do we need shared mutual exclusion between the
> two? (This is where a proper description of the problem in the commit
> message would have been very helpful)
> 

Are you suggesting we just have one vblank_ctl_lock per encoder and not 
have one vblank_ctl_lock per phys encoder? I cannot think of a display 
specific reason for that other than just the SW layout.

The reason its like this today is that control_vblank_irq is an encoder 
phys op because it does different things based on the type of encoder.

Because its an encoder phys op, it has the vblank_ctl_lock at the phys 
structure and not the encoder one.

Its just more about how the phys op is defined that each phys op 
operates on its phys's structure.

Generally, if we have one encoder with two physical encoders we anyways 
bail out early for the other encoder so this is mostly a no-op for the 
slave phys encoder.

Please take a look at below return point.

715 	/* Slave encoders don't report vblank */
716 	if (!sde_encoder_phys_vid_is_master(phys_enc))
717 		goto end;
718

So technically its still providing protection for the same phys encoder 
but the catch is this control_vblank_irq can get called from different 
threads hence we need exclusion.


> Regards,
> Bjorn