On Fri, Dec 01, 2023 at 11:43:36AM -0800, Abhinav Kumar wrote: > > > 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@xxxxxxxxxxx> 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. > The way I understand the code is that the atomic is used to refcount when to enable/disable the interrupt, and the new lock protects this refcount during concurrent updates. I have no concerns with this part. What I'm seeing is that the refcount it per phys_enc, and as such there would be no reason to have a common mutex to protect the two independent refcounts. But I'm probably misunderstanding something here... Regards, Bjorn
