Re: [Freedreno] [DPU PATCH 4/4] drm/msm/dpu: use private obj to track hw resources
On Tue, Jun 12, 2018 at 06:17:47PM -0700, Jeykumar Sankaran wrote:
> Switch to state based resource management. This patch
> overhauls the resource manager and HW allocation methods by
> maintaining the global resource pool and allocated hw
> blocks in respective drm component states.
>
> Global resource manager(RM) is tracked in private object.
> Allocation strategy is switched from single point allocation
> of HW resources for the display pipeline to per component
> based allocation, where each drm component allocates HW
> blocks mapped to it's domain and tracks them in their respective
> state objects.
>
> Fixes resource contention due to race conditions between
> user space and display thread by reserving resources
> only in atomic check.
>
> Signed-off-by: Jeykumar Sankaran
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 210 +++---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 59 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 223 ++
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h| 4 -
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 9 +-
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 +-
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 86 +--
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 19 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h| 8 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 805
> ++---
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 149 ++--
> 11 files changed, 534 insertions(+), 1070 deletions(-)
Ok, there's a lot going on here. It's pretty easy to review megapatches where
the diffstat is mostly negative. However, this patch has a lot of code deleted
and moving around, along with the new private obj. It's really hard to review
changes like this.
Could you please split this up into a bunch of simple patches which do one
thing? ie: Moving topology is a patch, using cstate instead of crtc is a patch,
using private obj is a patch, etc, etc.
Basically, cut things down into small enough pieces such that each patch can
be easily explained without using "and" in the commit message :-)
/snip
> +
> dpu_crtc = to_dpu_crtc(crtc);
> cstate = to_dpu_crtc_state(crtc->state);
> mode = >base.adjusted_mode;
> priv = crtc->dev->dev_private;
> + dpu_kms = to_dpu_kms(priv->kms);
> +
> + /* accessing after swap state. piv_obj.state is the current state */
s/piv_obj/priv_obj/
> + dpu_priv_state = to_dpu_private_state(dpu_kms->priv_obj.state);
>
> DPU_DEBUG("crtc%d\n", crtc->base.id);
>
/snip
--
Sean Paul, Software Engineer, Google / Chromium OS
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [DPU PATCH 4/4] drm/msm/dpu: use private obj to track hw resources
On Wed, Jun 13, 2018 at 12:01:21PM -0700, Jeykumar Sankaran wrote: > On 2018-06-13 09:44, Jordan Crouse wrote: > > On Tue, Jun 12, 2018 at 06:17:47PM -0700, Jeykumar Sankaran wrote: > > > Switch to state based resource management. This patch > > > overhauls the resource manager and HW allocation methods by > > > maintaining the global resource pool and allocated hw > > > blocks in respective drm component states. > > > > > > Global resource manager(RM) is tracked in private object. > > > Allocation strategy is switched from single point allocation > > > of HW resources for the display pipeline to per component > > > based allocation, where each drm component allocates HW > > > blocks mapped to it's domain and tracks them in their respective > > > state objects. > > > > > > Fixes resource contention due to race conditions between > > > user space and display thread by reserving resources > > > only in atomic check. > > > > > > Signed-off-by: Jeykumar Sankaran > > > --- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 210 +++--- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 59 +- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 223 ++ > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h| 4 - > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 9 +- > > > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 +- > > > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 86 +-- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 19 +- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h| 8 +- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 805 > > ++--- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 149 ++-- > > > 11 files changed, 534 insertions(+), 1070 deletions(-) /snip > > cstate->num_mixers); > > > > Nit - this could be worded a bit better - "too many mixers" would be > > better, but > > I have to ask - under what circumstances would the number of mixers be > > larger > > than CRTC_DUAL_MIXERS and/or why don't we support a dynamic number of > > mixers? > > > Comes from downstream driver implementation where CRTC iterates through > RM free pool to identify mixers tagged with the current crtc id. If the > previous clean up was screwed up this may have more than 2 mixers. With > the new state based RM, its very unlikely we hit this condition. > In this case, add a comment with "/* This should never happen */" I'm just kidding, that would virtually guarantee that it does happen and we certainly don't need that bad juju around! Sean > We do support dynamic mixer counts. Based on the connector (panel) > resolution, > CRTC allocates 1 or 2 (panel_width > hw mixer width) mixer block(s) on the > first > atomic check. DPU limits max no. of hw mixers that can be ganged up for a > display to 2. > > > > return; > > > } > > /snip -- Sean Paul, Software Engineer, Google / Chromium OS ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [DPU PATCH 4/4] drm/msm/dpu: use private obj to track hw resources
On 2018-06-13 09:44, Jordan Crouse wrote:
On Tue, Jun 12, 2018 at 06:17:47PM -0700, Jeykumar Sankaran wrote:
Switch to state based resource management. This patch
overhauls the resource manager and HW allocation methods by
maintaining the global resource pool and allocated hw
blocks in respective drm component states.
Global resource manager(RM) is tracked in private object.
Allocation strategy is switched from single point allocation
of HW resources for the display pipeline to per component
based allocation, where each drm component allocates HW
blocks mapped to it's domain and tracks them in their respective
state objects.
Fixes resource contention due to race conditions between
user space and display thread by reserving resources
only in atomic check.
Signed-off-by: Jeykumar Sankaran
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 210 +++---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 59 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 223 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h| 4 -
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 9 +-
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 +-
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 86 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 19 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h| 8 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 805
++---
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 149 ++--
11 files changed, 534 insertions(+), 1070 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 426e2ad..a484c06 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -47,6 +47,8 @@
#define RIGHT_MIXER 1
#define MISR_BUFF_SIZE 256
+#define MAX_VDISPLAY_SPLIT 1080
+
static inline struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc
*crtc)
{
@@ -142,9 +144,9 @@ static void _dpu_crtc_program_lm_output_roi(struct
drm_crtc *crtc)
crtc_state = to_dpu_crtc_state(crtc->state);
lm_horiz_position = 0;
- for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++) {
+ for (lm_idx = 0; lm_idx < crtc_state->num_mixers; lm_idx++) {
const struct dpu_rect *lm_roi =
_state->lm_bounds[lm_idx];
- struct dpu_hw_mixer *hw_lm =
dpu_crtc->mixers[lm_idx].hw_lm;
+ struct dpu_hw_mixer *hw_lm =
crtc_state->mixers[lm_idx].hw_lm;
struct dpu_hw_mixer_cfg cfg;
if (dpu_kms_rect_is_null(lm_roi))
@@ -182,7 +184,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
drm_crtc *crtc,
return;
}
- ctl = mixer->hw_ctl;
+ ctl = mixer->lm_ctl;
lm = mixer->hw_lm;
stage_cfg = _crtc->stage_cfg;
cstate = to_dpu_crtc_state(crtc->state);
@@ -237,7 +239,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
drm_crtc *crtc,
format->base.pixel_format, fb ? fb->modifier : 0);
/* blend config update */
- for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++)
{
+ for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
_dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate);
mixer[lm_idx].flush_mask |= flush_mask;
@@ -260,7 +262,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
drm_crtc *crtc,
static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
{
struct dpu_crtc *dpu_crtc;
- struct dpu_crtc_state *dpu_crtc_state;
+ struct dpu_crtc_state *cstate;
struct dpu_crtc_mixer *mixer;
struct dpu_hw_ctl *ctl;
struct dpu_hw_mixer *lm;
@@ -271,26 +273,26 @@ static void _dpu_crtc_blend_setup(struct
drm_crtc
*crtc)
return;
dpu_crtc = to_dpu_crtc(crtc);
- dpu_crtc_state = to_dpu_crtc_state(crtc->state);
- mixer = dpu_crtc->mixers;
+ cstate = to_dpu_crtc_state(crtc->state);
+ mixer = cstate->mixers;
DPU_DEBUG("%s\n", dpu_crtc->name);
- if (dpu_crtc->num_mixers > CRTC_DUAL_MIXERS) {
- DPU_ERROR("invalid number mixers: %d\n",
dpu_crtc->num_mixers);
+ if (cstate->num_mixers > CRTC_DUAL_MIXERS) {
+ DPU_ERROR("invalid number mixers: %d\n",
cstate->num_mixers);
Nit - this could be worded a bit better - "too many mixers" would be
better, but
I have to ask - under what circumstances would the number of mixers be
larger
than CRTC_DUAL_MIXERS and/or why don't we support a dynamic number of
mixers?
Comes from downstream driver implementation where CRTC iterates through
RM free pool to identify mixers tagged with the current crtc id. If the
previous clean up was screwed up this may have more than 2 mixers. With
the new state based RM, its very unlikely we hit this condition.
We do support dynamic mixer counts. Based
Re: [Freedreno] [DPU PATCH 4/4] drm/msm/dpu: use private obj to track hw resources
On Tue, Jun 12, 2018 at 06:17:47PM -0700, Jeykumar Sankaran wrote:
> Switch to state based resource management. This patch
> overhauls the resource manager and HW allocation methods by
> maintaining the global resource pool and allocated hw
> blocks in respective drm component states.
>
> Global resource manager(RM) is tracked in private object.
> Allocation strategy is switched from single point allocation
> of HW resources for the display pipeline to per component
> based allocation, where each drm component allocates HW
> blocks mapped to it's domain and tracks them in their respective
> state objects.
>
> Fixes resource contention due to race conditions between
> user space and display thread by reserving resources
> only in atomic check.
>
> Signed-off-by: Jeykumar Sankaran
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 210 +++---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 59 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 223 ++
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h| 4 -
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 9 +-
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 +-
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 86 +--
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 19 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h| 8 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 805
> ++---
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 149 ++--
> 11 files changed, 534 insertions(+), 1070 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 426e2ad..a484c06 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -47,6 +47,8 @@
> #define RIGHT_MIXER 1
>
> #define MISR_BUFF_SIZE 256
> +#define MAX_VDISPLAY_SPLIT 1080
> +
>
> static inline struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
> {
> @@ -142,9 +144,9 @@ static void _dpu_crtc_program_lm_output_roi(struct
> drm_crtc *crtc)
> crtc_state = to_dpu_crtc_state(crtc->state);
>
> lm_horiz_position = 0;
> - for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++) {
> + for (lm_idx = 0; lm_idx < crtc_state->num_mixers; lm_idx++) {
> const struct dpu_rect *lm_roi = _state->lm_bounds[lm_idx];
> - struct dpu_hw_mixer *hw_lm = dpu_crtc->mixers[lm_idx].hw_lm;
> + struct dpu_hw_mixer *hw_lm = crtc_state->mixers[lm_idx].hw_lm;
> struct dpu_hw_mixer_cfg cfg;
>
> if (dpu_kms_rect_is_null(lm_roi))
> @@ -182,7 +184,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
> *crtc,
> return;
> }
>
> - ctl = mixer->hw_ctl;
> + ctl = mixer->lm_ctl;
> lm = mixer->hw_lm;
> stage_cfg = _crtc->stage_cfg;
> cstate = to_dpu_crtc_state(crtc->state);
> @@ -237,7 +239,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
> *crtc,
> format->base.pixel_format, fb ? fb->modifier : 0);
>
> /* blend config update */
> - for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++) {
> + for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
> _dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate);
>
> mixer[lm_idx].flush_mask |= flush_mask;
> @@ -260,7 +262,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
> *crtc,
> static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
> {
> struct dpu_crtc *dpu_crtc;
> - struct dpu_crtc_state *dpu_crtc_state;
> + struct dpu_crtc_state *cstate;
> struct dpu_crtc_mixer *mixer;
> struct dpu_hw_ctl *ctl;
> struct dpu_hw_mixer *lm;
> @@ -271,26 +273,26 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
> return;
>
> dpu_crtc = to_dpu_crtc(crtc);
> - dpu_crtc_state = to_dpu_crtc_state(crtc->state);
> - mixer = dpu_crtc->mixers;
> + cstate = to_dpu_crtc_state(crtc->state);
> + mixer = cstate->mixers;
>
> DPU_DEBUG("%s\n", dpu_crtc->name);
>
> - if (dpu_crtc->num_mixers > CRTC_DUAL_MIXERS) {
> - DPU_ERROR("invalid number mixers: %d\n", dpu_crtc->num_mixers);
> + if (cstate->num_mixers > CRTC_DUAL_MIXERS) {
> + DPU_ERROR("invalid number mixers: %d\n", cstate->num_mixers);
Nit - this could be worded a bit better - "too many mixers" would be better, but
I have to ask - under what circumstances would the number of mixers be larger
than CRTC_DUAL_MIXERS and/or why don't we support a dynamic number of mixers?
> return;
> }
> +static void _dpu_crtc_setup_mixers(struct drm_crtc_state *crtc_state)
> {
> - struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> - struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
> - struct
