Re: [Freedreno] [PATCH 24/25] drm/msm/dpu: remove mutex locking for RM interfaces

[Jeykumar Sankaran <jsanka@codeaurora.org>]

On 2018-10-10 07:36, Sean Paul wrote:
> On Tue, Oct 09, 2018 at 11:03:24PM -0700, Jeykumar Sankaran wrote:
>> On 2018-10-09 12:57, Sean Paul wrote:
>> > On Mon, Oct 08, 2018 at 09:27:41PM -0700, Jeykumar Sankaran wrote:
>> > > Since HW reservations are happening through atomic_check
>> > > and all the display commits are catered by a single commit thread,
>> > > it is not necessary to protect the interfaces by a separate
>> > > mutex.
>> > >
>> > > Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>> > > ---
>> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 24
> ------------------------
>> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h |  2 --
>> > >  2 files changed, 26 deletions(-)
>> > >
>> >
>> > /snip
>> >
>> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> > > index 8676fa5..9acbeba 100644
>> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> > > @@ -24,11 +24,9 @@
>> > >   * struct dpu_rm - DPU dynamic hardware resource manager
>> > >   * @hw_blks: array of lists of hardware resources present in the
>> > system, one
>> > >   *	list per type of hardware block
>> > > - * @rm_lock: resource manager mutex
>> > >   */
>> > >  struct dpu_rm {
>> > >  	struct list_head hw_blks[DPU_HW_BLK_MAX];
>> >
>> > At this point, there's really not much point to even having the rm.
> It's
>> > just
>> > another level of indirection that IMO complicates the code. If you
> look
>> > at the usage of hw_blks, the code is always looking at a specific type
>> > of
>> > hw_blk, so the array is unnecessary.
>> >
>> > dpu_kms could just keep a few arrays/lists of the hw types, and the
> crtc
>> > and encoder
>> > reserve functions can just go in crtc/encoder.
>> >
>> > Sean
>> >
>> RM has been reduced to its current form to manage only LM/PP, CTL and
>> interfaces.
>> Our eventual plan is to support all the advanced HW blocks and its
> features
>> in
>> an upstream friendly way. When RM grows to manage all its subblocks,
>> iteration
>> logic may get heavy since the chipset have HW chain restrictions on
> various
>> hw blocks.
>> To provide room for the growth, I suggest keeping the allocation
>> helpers in a separate file. But I can see why you want to maintain the
> HW
>> block lists
>> in the KMS.
> 
> At least for the blocks that exist, using the RM is unnecessary, does 
> that
> change for the current blocks when you add more? I'm guessing their 
> code
> will
> remain unchanged.
> 
Yes. But to seperate out the allocation logics, I prefered the separate
file. I guess we can hold off the discussion until we need those 
enhancements.

I can get rid of the RM files for now and move the allocation functions 
to
the respective files (CRTC / Encoder).

Thanks,
Jeykumar S.
> If the new blocks you're adding have a lot of commonality, perhaps it
> makes
> sense to re-introduce the RM, but IMO it doesn't make sense for 
> lm/ctl/pp.
> 
> Sean
> 
>> 
>> Thanks,
>> Jeykumar S.
>> > > -	struct mutex rm_lock;
>> > >  };
>> > >
>> > >  /**
>> > > --
>> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> > Forum,
>> > > a Linux Foundation Collaborative Project
>> > >
>> > > _______________________________________________
>> > > Freedreno mailing list
>> > > Freedreno@lists.freedesktop.org
>> > > https://lists.freedesktop.org/mailman/listinfo/freedreno
>> 
>> --
>> Jeykumar S

-- 
Jeykumar S
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel