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

[Bjorn Andersson]

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 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.
> 

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