On 12/5/2023 7:51 PM, Bjorn Andersson wrote:
On Mon, Dec 04, 2023 at 11:22:24AM -0800, Abhinav Kumar wrote:
On 12/3/2023 7:31 PM, Bjorn Andersson wrote:
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.
Correct.
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...
There is no reason to have a common mutex to protect the two independent
refcounts. In fact, there is no need to even have two independent refcounts
because whenever we have one encoder with two physical encoders, we use only
the master physical encoder for vblanks like I pointed above.
The only reason we have it like this is because today the vblank_refcount is
part of phys_enc so the mutex handle is also now a part of it.
Do you think if we move both the mutex and the vblank_refcount to the
dpu_encoder from the dpu_encoder_phys and maintain the mutex at that level
it will be less confusing for you?
The two functions operate on dpu_encoder_phys objects, and as you say
above the two instances doesn't need to be handled under shared mutual
exclusion.
Moving the serialization mechanism to dpu_encoder seems like it would
create an entanglement, for the sake of making the lock common. If
nothing else this would act as documentation to me that the two
functions are intertwined somehow.
I was rather hoping that we'd move the mutex_init() to
dpu_encoder_phys_init() and avoid passing a reference around in
unrelated parts of the code just to set up the sharing, if that's not
necessary.
Regards,
Bjorn
In principle we need only one mutex per encoder and not per encoder phys.
Hence the two phys encoders are having the handle to the same mutex for
that reason.
If having it that way is confusing you for some reason, I offered the
alternative.
Otherwise I think what we have is enough and correct. We can update the
commit text and doc around the mutex to explain what it does.
I dont think your suggestion will work.
dpu_encoder_phys_init() will be called once per each phys encoder so it
can get called twice. Are you suggesting we have one mutex per phys
encoder? Thats not necessary and its not correct as well.
If not, perhaps you can post something for us to review your idea.
I am fine with that.
Regards,
Bjorn
