| Message ID | 1539059262-8326-25-git-send-email-jsanka@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | reserve RM resources in CRTC state | expand |
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 > - 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
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. 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
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. 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
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
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c index 34e09aa..9a63128 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c @@ -74,8 +74,6 @@ int dpu_rm_destroy(struct dpu_rm *rm) } } - mutex_destroy(&rm->rm_lock); - return 0; } @@ -146,8 +144,6 @@ int dpu_rm_init(struct dpu_rm *rm, /* Clear, setup lists */ memset(rm, 0, sizeof(*rm)); - mutex_init(&rm->rm_lock); - for (type = 0; type < DPU_HW_BLK_MAX; type++) INIT_LIST_HEAD(&rm->hw_blks[type]); @@ -473,11 +469,7 @@ void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state) { struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); - mutex_lock(&rm->rm_lock); - _dpu_rm_crtc_release_reservation(rm, dpu_cstate); - - mutex_unlock(&rm->rm_lock); } void dpu_rm_encoder_release(struct dpu_rm *rm, @@ -485,11 +477,7 @@ void dpu_rm_encoder_release(struct dpu_rm *rm, { struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); - mutex_lock(&rm->rm_lock); - _dpu_rm_encoder_release_reservation(rm, dpu_cstate); - - mutex_unlock(&rm->rm_lock); } int dpu_rm_crtc_reserve( @@ -506,8 +494,6 @@ int dpu_rm_crtc_reserve( DRM_DEBUG_KMS("reserving hw for crtc %d\n", crtc_state->crtc->base.id); - mutex_lock(&rm->rm_lock); - ret = _dpu_rm_reserve_lms(rm, dpu_cstate); if (ret) { DPU_ERROR("unable to find appropriate mixers\n"); @@ -520,15 +506,11 @@ int dpu_rm_crtc_reserve( goto cleanup_on_fail; } - mutex_unlock(&rm->rm_lock); - return ret; cleanup_on_fail: _dpu_rm_crtc_release_reservation(rm, dpu_cstate); - mutex_unlock(&rm->rm_lock); - return ret; } @@ -547,8 +529,6 @@ int dpu_rm_encoder_reserve( DRM_DEBUG_KMS("reserving hw for enc %d\n", enc->base.id); - mutex_lock(&rm->rm_lock); - dpu_encoder_get_hw_resources(enc, &hw_res); ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &hw_res); @@ -557,14 +537,10 @@ int dpu_rm_encoder_reserve( goto cleanup_on_fail; } - mutex_unlock(&rm->rm_lock); - return ret; cleanup_on_fail: _dpu_rm_encoder_release_reservation(rm, dpu_cstate); - mutex_unlock(&rm->rm_lock); - return ret; } 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]; - struct mutex rm_lock; }; /**
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(-)