| Message ID | 20230321011821.635977-13-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2,01/13] drm/atomic-helper: split not-scaling part of drm_atomic_helper_check_plane_state | expand |
On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote: > Only several SSPP blocks support such features as YUV output or scaling, > thus different DRM planes have different features. Properly utilizing > all planes requires the attention of the compositor, who should > prefer simpler planes to YUV-supporting ones. Otherwise it is very easy > to end up in a situation when all featureful planes are already > allocated for simple windows, leaving no spare plane for YUV playback. > > To solve this problem make all planes virtual. Each plane is registered > as if it supports all possible features, but then at the runtime during > the atomic_check phase the driver selects backing SSPP block for each > plane. > > Note, this does not provide support for using two different SSPP blocks > for a single plane or using two rectangles of an SSPP to drive two > planes. Each plane still gets its own SSPP and can utilize either a solo > rectangle or both multirect rectangles depending on the resolution. > > Note #2: By default support for virtual planes is turned off and the > driver still uses old code path with preallocated SSPP block for each > plane. To enable virtual planes, pass 'msm.dpu_use_virtual_planes=1' > kernel parameter. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- There will be some rebase needed to switch back to encoder based allocation so I am not going to comment on those parts and will let you handle that when you post v3. But my questions/comments below are for other things. > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 59 +++++-- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 120 ++++++++++---- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 + > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 187 ++++++++++++++++++---- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 24 ++- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 65 ++++++++ > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 24 +++ > 7 files changed, 413 insertions(+), 70 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 8ef191fd002d..cdece21b81c9 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -1273,6 +1273,29 @@ static int dpu_crtc_assign_resources(struct drm_crtc *crtc, struct drm_crtc_stat > return 0; > } > > +static int dpu_crtc_assign_plane_resources(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state) > +{ > + struct dpu_global_state *global_state; > + struct drm_plane *plane; > + int rc; > + > + global_state = dpu_kms_get_global_state(crtc_state->state); > + if (IS_ERR(global_state)) > + return PTR_ERR(global_state); > + > + dpu_rm_release_all_sspp(global_state, crtc); > + > + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) { > + rc = dpu_plane_virtual_assign_resources(plane, crtc, > + global_state, > + crtc_state->state); > + if (rc) > + return rc; > + } > + > + return 0; > +} > + > static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > struct drm_atomic_state *state) > { > @@ -1281,7 +1304,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); > struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); > > - const struct drm_plane_state *pstate; > struct drm_plane *plane; > > int rc = 0; > @@ -1294,6 +1316,13 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > return rc; > } > > + if (dpu_use_virtual_planes && > + crtc_state->planes_changed) { > + rc = dpu_crtc_assign_plane_resources(crtc, crtc_state); > + if (rc < 0) > + return rc; > + } Can you please explain a bit more about the planes_changed condition? 1) Are we doing this because the plane's atomic check happens before the CRTC atomic check? 2) So the DRM core sets this to true already when plane is switching CRTCs or being connected to a CRTC for the first time, we need to handle the conditions additional to that right? 3) If (2) is correct, arent there other conditions then to be handled for setting planes_changed to true? Some examples include, switching from a scaling to non-scaling scenario, needing rotation vs not needing etc. Basically it seems like all of this is handled within the reserve_sspp() function but if planes_changes is not set then that wont get invoked at all. > + > if (!crtc_state->enable || !crtc_state->active) { > DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip atomic_check\n", > crtc->base.id, crtc_state->enable, > @@ -1311,20 +1340,30 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > if (cstate->num_mixers) > _dpu_crtc_setup_lm_bounds(crtc, crtc_state); > > - /* FIXME: move this to dpu_plane_atomic_check? */ > - drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) { > - struct dpu_plane_state *dpu_pstate = to_dpu_plane_state(pstate); > - > - if (IS_ERR_OR_NULL(pstate)) { > - rc = PTR_ERR(pstate); > - DPU_ERROR("%s: failed to get plane%d state, %d\n", > - dpu_crtc->name, plane->base.id, rc); > - return rc; > + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) { > + const struct drm_plane_state *pstate; > + struct dpu_plane_state *dpu_pstate; > + > + pstate = drm_atomic_get_plane_state(crtc_state->state, plane); > + if (IS_ERR(pstate)) > + return PTR_ERR(pstate); > + > + if (dpu_use_virtual_planes) { > + /* > + * In case of virtual planes, the plane's atomic_check > + * is a shortcut. Perform actual check here, after > + * allocating SSPPs. > + */ > + rc = dpu_plane_atomic_check(plane, crtc_state->state); > + if (rc) > + return rc; > } > > if (!pstate->visible) > continue; > > + /* FIXME: move this to dpu_plane_atomic_check? */ > + dpu_pstate = to_dpu_plane_state(pstate); Anything prevents us from doing it even now instead of a FIXME? > dpu_pstate->needs_dirtyfb = needs_dirtyfb; > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 35194262e628..487bb19ee9d6 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -50,6 +50,9 @@ > #define DPU_DEBUGFS_DIR "msm_dpu" > #define DPU_DEBUGFS_HWMASKNAME "hw_log_mask" > > +bool dpu_use_virtual_planes = false; > +module_param(dpu_use_virtual_planes, bool, 0); > + > static int dpu_kms_hw_init(struct msm_kms *kms); > static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms); > > @@ -735,38 +738,54 @@ static int _dpu_kms_setup_displays(struct drm_device *dev, > return rc; > } > > -#define MAX_PLANES 20 > -static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) > +static int dpu_kms_create_virtual_planes(struct dpu_kms *dpu_kms, > + int max_crtc_count, > + struct drm_plane **primary_planes, > + struct drm_plane **cursor_planes) > { > - struct drm_device *dev; > - struct drm_plane *primary_planes[MAX_PLANES], *plane; > - struct drm_plane *cursor_planes[MAX_PLANES] = { NULL }; > - struct drm_crtc *crtc; > - struct drm_encoder *encoder; > - unsigned int num_encoders; > + const struct dpu_mdss_cfg *catalog = dpu_kms->catalog; > + struct drm_device *dev = dpu_kms->dev; > + int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; > + struct drm_plane *plane; > > - struct msm_drm_private *priv; > - const struct dpu_mdss_cfg *catalog; > + /* Create the planes, keeping track of one primary/cursor per crtc */ > + for (i = 0; i < catalog->sspp_count; i++) { > + enum drm_plane_type type; > > - int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; > - int max_crtc_count; > - dev = dpu_kms->dev; > - priv = dev->dev_private; > - catalog = dpu_kms->catalog; > + if (primary_planes_idx < max_crtc_count) > + type = DRM_PLANE_TYPE_PRIMARY; > + else if (cursor_planes_idx < max_crtc_count) > + type = DRM_PLANE_TYPE_CURSOR; > + else > + type = DRM_PLANE_TYPE_OVERLAY; > > - /* > - * Create encoder and query display drivers to create > - * bridges and connectors > - */ > - ret = _dpu_kms_setup_displays(dev, priv, dpu_kms); > - if (ret) > - return ret; > + DPU_DEBUG("Create plane type %d\n", type); > > - num_encoders = 0; > - drm_for_each_encoder(encoder, dev) > - num_encoders++; > + plane = dpu_plane_init_virtual(dev, type, (1UL << max_crtc_count) - 1); > + if (IS_ERR(plane)) { > + DPU_ERROR("dpu_plane_init failed\n"); > + ret = PTR_ERR(plane); > + return ret; > + } > > - max_crtc_count = min(catalog->mixer_count, num_encoders); > + if (type == DRM_PLANE_TYPE_CURSOR) > + cursor_planes[cursor_planes_idx++] = plane; > + else if (type == DRM_PLANE_TYPE_PRIMARY) > + primary_planes[primary_planes_idx++] = plane; > + } > + > + return primary_planes_idx; > +} > + > +static int dpu_kms_create_planes(struct dpu_kms *dpu_kms, > + int max_crtc_count, > + struct drm_plane **primary_planes, > + struct drm_plane **cursor_planes) > +{ > + const struct dpu_mdss_cfg *catalog = dpu_kms->catalog; > + struct drm_device *dev = dpu_kms->dev; > + int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; > + struct drm_plane *plane; > > /* Create the planes, keeping track of one primary/cursor per crtc */ > for (i = 0; i < catalog->sspp_count; i++) { > @@ -784,8 +803,8 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) > type, catalog->sspp[i].features, > catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR)); > > - plane = dpu_plane_init(dev, catalog->sspp[i].id, type, > - (1UL << max_crtc_count) - 1); > + plane = dpu_plane_init_sspp(dev, catalog->sspp[i].id, type, > + (1UL << max_crtc_count) - 1); > if (IS_ERR(plane)) { > DPU_ERROR("dpu_plane_init failed\n"); > ret = PTR_ERR(plane); > @@ -798,7 +817,50 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) > primary_planes[primary_planes_idx++] = plane; > } > > - max_crtc_count = min(max_crtc_count, primary_planes_idx); > + return primary_planes_idx; > +} > + > +#define MAX_PLANES 20 > +static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) > +{ > + struct drm_device *dev; > + struct drm_plane *primary_planes[MAX_PLANES]; > + struct drm_plane *cursor_planes[MAX_PLANES] = { NULL }; > + struct drm_crtc *crtc; > + struct drm_encoder *encoder; > + unsigned int num_encoders; > + > + struct msm_drm_private *priv; > + const struct dpu_mdss_cfg *catalog; > + int i, ret; > + int max_crtc_count; > + > + dev = dpu_kms->dev; > + priv = dev->dev_private; > + catalog = dpu_kms->catalog; > + > + /* > + * Create encoder and query display drivers to create > + * bridges and connectors > + */ > + ret = _dpu_kms_setup_displays(dev, priv, dpu_kms); > + if (ret) > + return ret; > + > + num_encoders = 0; > + drm_for_each_encoder(encoder, dev) > + num_encoders++; > + > + max_crtc_count = min(catalog->mixer_count, num_encoders); > + > + if (dpu_use_virtual_planes) > + ret = dpu_kms_create_virtual_planes(dpu_kms, max_crtc_count, primary_planes, cursor_planes); > + else > + ret = dpu_kms_create_planes(dpu_kms, max_crtc_count, primary_planes, cursor_planes); > + if (ret < 0) > + return ret; > + > + max_crtc_count = min(max_crtc_count, ret); > > /* Create one CRTC per encoder */ > for (i = 0; i < max_crtc_count; i++) { > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > index 934874eb2248..9f6478f0ced6 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > @@ -65,6 +65,8 @@ > > #define DPU_NAME_SIZE 12 > > +extern bool dpu_use_virtual_planes; > + > struct dpu_kms { > struct msm_kms base; > struct drm_device *dev; > @@ -134,6 +136,8 @@ struct dpu_global_state { > uint32_t ctl_to_crtc_id[CTL_MAX - CTL_0]; > uint32_t dspp_to_crtc_id[DSPP_MAX - DSPP_0]; > uint32_t dsc_to_crtc_id[DSC_MAX - DSC_0]; > + > + uint32_t sspp_to_crtc_id[SSPP_MAX - SSPP_NONE]; > }; > > struct dpu_global_state > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index cf17075676d5..ee906c276aa5 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -837,8 +837,77 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, > return 0; > } > > -static int dpu_plane_atomic_check(struct drm_plane *plane, > - struct drm_atomic_state *state) > +static int dpu_plane_virtual_atomic_check(struct drm_plane *plane, > + struct drm_atomic_state *state) > +{ > + struct drm_plane_state *plane_state = > + drm_atomic_get_plane_state(state, plane); > + struct dpu_plane_state *pstate = to_dpu_plane_state(plane_state); > + const struct dpu_format *format; > + struct drm_crtc_state *crtc_state; > + > + /* > + * Main part of checks, including drm_atomic_helper_check_plane_state() > + * is called from dpu_crtc_atomic_check(). Do minimal processing here. > + */ > + > + if (!plane_state->fb) { > + plane_state->visible = false; > + > + /* resources are freed by dpu_crtc_atomic_check(), but clean them here */ > + pstate->pipe.sspp = NULL; > + pstate->r_pipe.sspp = NULL; > + > + return 0; > + } > + > + format = to_dpu_format(msm_framebuffer_format(plane_state->fb)); > + crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc); > + > + /* force resource reallocation if the format of FB has changed */ > + if (pstate->saved_fmt != format) { > + crtc_state->planes_changed = true; > + pstate->saved_fmt = format; > + } > + > + return 0; > +} > + > +int dpu_plane_virtual_assign_resources(struct drm_plane *plane, > + struct drm_crtc *crtc, > + struct dpu_global_state *global_state, > + struct drm_atomic_state *state) > +{ > + struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); > + struct dpu_plane_state *pstate; > + struct drm_plane_state *plane_state; > + struct dpu_hw_sspp *hw_sspp; > + bool yuv, scale, rot90; > + > + plane_state = drm_atomic_get_plane_state(state, plane); > + if (IS_ERR(plane_state)) > + return PTR_ERR(plane_state); > + > + yuv = plane_state->fb ? > + DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(plane_state->fb))) : > + false; > + scale = (plane_state->src_w >> 16 != plane_state->crtc_w) || > + (plane_state->src_h >> 16 != plane_state->crtc_h); > + > + rot90 = drm_rotation_90_or_270(plane_state->rotation); > + > + hw_sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, yuv, scale, rot90); I think this parameter list is going to explode. Shall we introduce a dpu_plane_sspp_requirements to store these? > + if (!hw_sspp) > + return -ENODEV; > + > + pstate = to_dpu_plane_state(plane_state); > + pstate->pipe.sspp = hw_sspp; > + > + return 0; > +} > + > +int dpu_plane_atomic_check(struct drm_plane *plane, > + struct drm_atomic_state *state) > { > struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, > plane); > @@ -863,8 +932,10 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > crtc_state = drm_atomic_get_new_crtc_state(state, > new_plane_state->crtc); > > - pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); > - r_pipe->sspp = NULL; > + if (pdpu->pipe != SSPP_NONE) { > + pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); > + r_pipe->sspp = NULL; > + } > > pipe_hw_caps = pstate->pipe.sspp->cap; > sblk = pstate->pipe.sspp->cap->sblk; > @@ -1358,12 +1429,14 @@ static void dpu_plane_atomic_print_state(struct drm_printer *p, > > drm_printf(p, "\tstage=%d\n", pstate->stage); > > - drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name); > - drm_printf(p, "\tmultirect_mode[0]=%s\n", dpu_get_multirect_mode(pipe->multirect_mode)); > - drm_printf(p, "\tmultirect_index[0]=%s\n", > - dpu_get_multirect_index(pipe->multirect_index)); > - drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->src_rect)); > - drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->dst_rect)); > + if (pipe->sspp) { > + drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name); > + drm_printf(p, "\tmultirect_mode[0]=%s\n", dpu_get_multirect_mode(pipe->multirect_mode)); > + drm_printf(p, "\tmultirect_index[0]=%s\n", > + dpu_get_multirect_index(pipe->multirect_index)); > + drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->src_rect)); > + drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->dst_rect)); > + } > > if (r_pipe->sspp) { > drm_printf(p, "\tsspp[1]=%s\n", r_pipe->sspp->cap->name); > @@ -1453,18 +1526,30 @@ static const struct drm_plane_helper_funcs dpu_plane_helper_funcs = { > .atomic_update = dpu_plane_atomic_update, > }; > > +/* > + * For virtual planes atomic_check is called from dpu_crtc_atomic_check(), > + * after CRTC code assigning SSPP. > + */ > +static const struct drm_plane_helper_funcs dpu_plane_virtual_helper_funcs = { > + .prepare_fb = dpu_plane_prepare_fb, > + .cleanup_fb = dpu_plane_cleanup_fb, > + .atomic_check = dpu_plane_virtual_atomic_check, > + .atomic_update = dpu_plane_atomic_update, > +}; > + > /* initialize plane */ > -struct drm_plane *dpu_plane_init(struct drm_device *dev, > - uint32_t pipe, enum drm_plane_type type, > - unsigned long possible_crtcs) > +static struct drm_plane *dpu_plane_init(struct drm_device *dev, > + enum drm_plane_type type, > + unsigned long possible_crtcs, > + bool inline_rotation, > + const uint32_t *format_list, > + uint32_t num_formats, > + enum dpu_sspp pipe) > { > struct drm_plane *plane = NULL; > - const uint32_t *format_list; > struct dpu_plane *pdpu; > struct msm_drm_private *priv = dev->dev_private; > struct dpu_kms *kms = to_dpu_kms(priv->kms); > - struct dpu_hw_sspp *pipe_hw; > - uint32_t num_formats; > uint32_t supported_rotations; > int ret = -EINVAL; > > @@ -1480,16 +1565,6 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, > plane = &pdpu->base; > pdpu->pipe = pipe; > > - /* initialize underlying h/w driver */ > - pipe_hw = dpu_rm_get_sspp(&kms->rm, pipe); > - if (!pipe_hw || !pipe_hw->cap || !pipe_hw->cap->sblk) { > - DPU_ERROR("[%u]SSPP is invalid\n", pipe); > - goto clean_plane; > - } > - > - format_list = pipe_hw->cap->sblk->format_list; > - num_formats = pipe_hw->cap->sblk->num_formats; > - > ret = drm_universal_plane_init(dev, plane, 0xff, &dpu_plane_funcs, > format_list, num_formats, > supported_format_modifiers, type, NULL); > @@ -1510,7 +1585,7 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, > > supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_180; > > - if (pipe_hw->cap->features & BIT(DPU_SSPP_INLINE_ROTATION)) > + if (inline_rotation) > supported_rotations |= DRM_MODE_ROTATE_MASK; > > drm_plane_create_rotation_property(plane, > @@ -1519,8 +1594,6 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, > drm_plane_enable_fb_damage_clips(plane); > > /* success! finalize initialization */ > - drm_plane_helper_add(plane, &dpu_plane_helper_funcs); > - > mutex_init(&pdpu->lock); > > DPU_DEBUG("%s created for pipe:%u id:%u\n", plane->name, > @@ -1531,3 +1604,59 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, > kfree(pdpu); > return ERR_PTR(ret); > } > + > +struct drm_plane *dpu_plane_init_sspp(struct drm_device *dev, > + uint32_t pipe, enum drm_plane_type type, > + unsigned long possible_crtcs) > +{ > + struct drm_plane *plane = NULL; > + struct msm_drm_private *priv = dev->dev_private; > + struct dpu_kms *kms = to_dpu_kms(priv->kms); > + struct dpu_hw_sspp *pipe_hw; > + > + /* initialize underlying h/w driver */ > + pipe_hw = dpu_rm_get_sspp(&kms->rm, pipe); > + if (!pipe_hw || !pipe_hw->cap || !pipe_hw->cap->sblk) { > + DPU_ERROR("[%u]SSPP is invalid\n", pipe); > + return ERR_PTR(-EINVAL); > + } > + > + > + plane = dpu_plane_init(dev, type, possible_crtcs, > + pipe_hw->cap->features & BIT(DPU_SSPP_INLINE_ROTATION), > + pipe_hw->cap->sblk->format_list, > + pipe_hw->cap->sblk->num_formats, > + pipe); > + if (IS_ERR(plane)) > + return plane; > + > + drm_plane_helper_add(plane, &dpu_plane_helper_funcs); > + > + DPU_DEBUG("%s created for pipe:%u id:%u\n", plane->name, > + pipe, plane->base.id); > + > + return plane; > +} > + > +struct drm_plane *dpu_plane_init_virtual(struct drm_device *dev, > + enum drm_plane_type type, > + unsigned long possible_crtcs) > +{ > + struct drm_plane *plane = NULL; > + struct msm_drm_private *priv = dev->dev_private; > + struct dpu_kms *kms = to_dpu_kms(priv->kms); > + > + plane = dpu_plane_init(dev, type, possible_crtcs, > + kms->catalog->caps->has_inline_rot, > + kms->catalog->caps->format_list, > + kms->catalog->caps->num_formats, > + SSPP_NONE); > + if (IS_ERR(plane)) > + return plane; > + > + drm_plane_helper_add(plane, &dpu_plane_virtual_helper_funcs); > + > + DPU_DEBUG("%s created virtual id:%u\n", plane->name, plane->base.id); > + > + return plane; > +} Overall I am satisfied with the split of the dpu_plane_init() function into virtual vs non-virtual and happy its protected with a modparam. > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > index abd6b21a049b..cb1e31ef0d3f 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > @@ -31,6 +31,7 @@ > * @plane_clk: calculated clk per plane > * @needs_dirtyfb: whether attached CRTC needs pixel data explicitly flushed > * @rotation: simplified drm rotation hint > + * @saved_fmt: format used by the plane's FB, saved for for virtual plane support > */ > struct dpu_plane_state { > struct drm_plane_state base; > @@ -48,6 +49,8 @@ struct dpu_plane_state { > > bool needs_dirtyfb; > unsigned int rotation; > + > + const struct dpu_format *saved_fmt; > }; > > #define to_dpu_plane_state(x) \ > @@ -66,17 +69,27 @@ void dpu_plane_flush(struct drm_plane *plane); > void dpu_plane_set_error(struct drm_plane *plane, bool error); > > /** > - * dpu_plane_init - create new dpu plane for the given pipe > + * dpu_plane_init_sspp - create new dpu plane for the given pipe > * @dev: Pointer to DRM device > * @pipe: dpu hardware pipe identifier > * @type: Plane type - PRIMARY/OVERLAY/CURSOR > * @possible_crtcs: bitmask of crtc that can be attached to the given pipe > * > */ > -struct drm_plane *dpu_plane_init(struct drm_device *dev, > +struct drm_plane *dpu_plane_init_sspp(struct drm_device *dev, > uint32_t pipe, enum drm_plane_type type, > unsigned long possible_crtcs); > > +/** > + * dpu_plane_init_virtual - create new dpu virtualized plane > + * @dev: Pointer to DRM device > + * @type: Plane type - PRIMARY/OVERLAY/CURSOR > + * @possible_crtcs: bitmask of crtc that can be attached to the given pipe > + */ > +struct drm_plane *dpu_plane_init_virtual(struct drm_device *dev, > + enum drm_plane_type type, > + unsigned long possible_crtcs); > + > /** > * dpu_plane_color_fill - enables color fill on plane > * @plane: Pointer to DRM plane object > @@ -93,4 +106,11 @@ void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable); > static inline void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable) {} > #endif > > +int dpu_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state); > + > +int dpu_plane_virtual_assign_resources(struct drm_plane *plane, > + struct drm_crtc *crtc, > + struct dpu_global_state *global_state, > + struct drm_atomic_state *state); > + > #endif /* _DPU_PLANE_H_ */ > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > index f0a94008d17a..6130ac87d7e3 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > @@ -607,6 +607,71 @@ int dpu_rm_reserve( > return ret; > } > > +struct dpu_hw_sspp *dpu_rm_reserve_sspp(struct dpu_rm *rm, > + struct dpu_global_state *global_state, > + struct drm_crtc *crtc, > + bool yuv, bool scale, bool rot90) > +{ as noted above, lets consider adding a dpu_plane_sspp_requirements struct instead of just expanding the param list. > + uint32_t crtc_id = crtc->base.id; > + struct dpu_hw_sspp *hw_sspp; > + bool retry = false; > + int i; > + > +retry_loop: > + for (i = 0; i < ARRAY_SIZE(rm->hw_sspp); i++) { > + if (!rm->hw_sspp[i]) > + continue; > + > + if (global_state->sspp_to_crtc_id[i]) > + continue; > + > + hw_sspp = rm->hw_sspp[i]; > + > + /* skip incompatible planes */ > + if (scale && !(hw_sspp->cap->features & DPU_SSPP_SCALER)) > + continue; > + > + if (yuv && !(hw_sspp->cap->features & DPU_SSPP_CSC_ANY)) > + continue; > + > + if (rot90 && !(hw_sspp->cap->features & DPU_SSPP_INLINE_ROTATION)) > + continue; > + These are sufficient conditions to start with for the use-cases I can think of. > + /* > + * For non-yuv, non-scaled planes try to find simple (DMA) > + * plane, fallback to VIG on a second try. > + * > + * This way we'd leave VIG sspps to be later used for YUV formats. > + */ > + > + if (!scale && !yuv && !rot90 && !retry && > + (hw_sspp->cap->features & > + (DPU_SSPP_SCALER | DPU_SSPP_CSC_ANY | DPU_SSPP_INLINE_ROTATION))) > + continue; > + > + global_state->sspp_to_crtc_id[hw_sspp->idx - SSPP_NONE] = crtc_id; > + > + return hw_sspp; > + } > + > + /* If we were looking for DMA plane, retry looking for VIG plane */ > + if (!scale && !yuv && !retry) { > + retry = true; > + goto retry_loop; > + } > + > + return NULL; > +} > + > +void dpu_rm_release_all_sspp(struct dpu_global_state *global_state, > + struct drm_crtc *crtc) > +{ > + uint32_t crtc_id = crtc->base.id; > + > + _dpu_rm_clear_mapping(global_state->sspp_to_crtc_id, > + ARRAY_SIZE(global_state->sspp_to_crtc_id), crtc_id); > +} > + > int dpu_rm_get_assigned_resources(struct dpu_rm *rm, > struct dpu_global_state *global_state, struct drm_crtc *crtc, > enum dpu_hw_blk_type type, struct dpu_hw_blk **blks, int blks_size) > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > index f402bec8322b..5bf6740ecb45 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > @@ -82,6 +82,30 @@ int dpu_rm_reserve( > void dpu_rm_release(struct dpu_global_state *global_state, > struct drm_crtc *crtc); > > +/** > + * dpu_rm_reserve_sspp - Reserve the required SSPP for the provided CRTC > + * @rm: DPU Resource Manager handle > + * @global_state: private global state > + * @crtc: DRM CRTC handle > + * @yuv: required SSPP supporting YUV formats > + * @scale: required SSPP supporting scaling > + * @rot90: required SSPP supporting inline 90 degree rotation > + */ > +struct dpu_hw_sspp *dpu_rm_reserve_sspp(struct dpu_rm *rm, > + struct dpu_global_state *global_state, > + struct drm_crtc *crtc, > + bool yuv, bool scale, bool rot90); > + > +/** > + * dpu_rm_release_all_sspp - Given the CRTC, release all SSPP > + * blocks previously reserved for that use case. > + * @rm: DPU Resource Manager handle > + * @crtc: DRM CRTC handle > + * @Return: 0 on Success otherwise -ERROR > + */ > +void dpu_rm_release_all_sspp(struct dpu_global_state *global_state, > + struct drm_crtc *crtc); > + > /** > * Get hw resources of the given type that are assigned to this encoder. > */
On 08/06/2023 00:05, Abhinav Kumar wrote: > > > On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote: >> Only several SSPP blocks support such features as YUV output or scaling, >> thus different DRM planes have different features. Properly utilizing >> all planes requires the attention of the compositor, who should >> prefer simpler planes to YUV-supporting ones. Otherwise it is very easy >> to end up in a situation when all featureful planes are already >> allocated for simple windows, leaving no spare plane for YUV playback. >> >> To solve this problem make all planes virtual. Each plane is registered >> as if it supports all possible features, but then at the runtime during >> the atomic_check phase the driver selects backing SSPP block for each >> plane. >> >> Note, this does not provide support for using two different SSPP blocks >> for a single plane or using two rectangles of an SSPP to drive two >> planes. Each plane still gets its own SSPP and can utilize either a solo >> rectangle or both multirect rectangles depending on the resolution. >> >> Note #2: By default support for virtual planes is turned off and the >> driver still uses old code path with preallocated SSPP block for each >> plane. To enable virtual planes, pass 'msm.dpu_use_virtual_planes=1' >> kernel parameter. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- > > There will be some rebase needed to switch back to encoder based > allocation so I am not going to comment on those parts and will let you > handle that when you post v3. > > But my questions/comments below are for other things. > >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 59 +++++-- >> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 120 ++++++++++---- >> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 + >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 187 ++++++++++++++++++---- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 24 ++- >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 65 ++++++++ >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 24 +++ >> 7 files changed, 413 insertions(+), 70 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> index 8ef191fd002d..cdece21b81c9 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> @@ -1273,6 +1273,29 @@ static int dpu_crtc_assign_resources(struct >> drm_crtc *crtc, struct drm_crtc_stat >> return 0; >> } >> +static int dpu_crtc_assign_plane_resources(struct drm_crtc *crtc, >> struct drm_crtc_state *crtc_state) >> +{ >> + struct dpu_global_state *global_state; >> + struct drm_plane *plane; >> + int rc; >> + >> + global_state = dpu_kms_get_global_state(crtc_state->state); >> + if (IS_ERR(global_state)) >> + return PTR_ERR(global_state); >> + >> + dpu_rm_release_all_sspp(global_state, crtc); >> + >> + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) { >> + rc = dpu_plane_virtual_assign_resources(plane, crtc, >> + global_state, >> + crtc_state->state); >> + if (rc) >> + return rc; >> + } >> + >> + return 0; >> +} >> + >> static int dpu_crtc_atomic_check(struct drm_crtc *crtc, >> struct drm_atomic_state *state) >> { >> @@ -1281,7 +1304,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc >> *crtc, >> struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); >> struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); >> - const struct drm_plane_state *pstate; >> struct drm_plane *plane; >> int rc = 0; >> @@ -1294,6 +1316,13 @@ static int dpu_crtc_atomic_check(struct >> drm_crtc *crtc, >> return rc; >> } >> + if (dpu_use_virtual_planes && >> + crtc_state->planes_changed) { >> + rc = dpu_crtc_assign_plane_resources(crtc, crtc_state); >> + if (rc < 0) >> + return rc; >> + } > > Can you please explain a bit more about the planes_changed condition? > > 1) Are we doing this because the plane's atomic check happens before the > CRTC atomic check? Yes. Another alternative would be to stop using drm_atomic_helper_check() and implement our own code instead, inverting the plane / CRTC check order. > > 2) So the DRM core sets this to true already when plane is switching > CRTCs or being connected to a CRTC for the first time, we need to handle > the conditions additional to that right? Nit: it is not possible to switch CRTCs. Plane first has to be disconnected and then to be connected to another CRTC. > > 3) If (2) is correct, arent there other conditions then to be handled > for setting planes_changed to true? > > Some examples include, switching from a scaling to non-scaling scenario, > needing rotation vs not needing etc. Setting the plane_changed is not required. Both drm_atomic_helper_disable_plane() and drm_atomic_helper_update_plane() will add the plane to the state. Then drm_atomic_helper_check_planes() will call drm_atomic_helper_plane_changed(), setting {old_,new_}crtc_state->planes_changed. I should check if the format check below is required or not. It looks like I can drop that clause too. > > Basically it seems like all of this is handled within the reserve_sspp() > function but if planes_changes is not set then that wont get invoked at > all. > > >> + >> if (!crtc_state->enable || !crtc_state->active) { >> DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip >> atomic_check\n", >> crtc->base.id, crtc_state->enable, >> @@ -1311,20 +1340,30 @@ static int dpu_crtc_atomic_check(struct >> drm_crtc *crtc, >> if (cstate->num_mixers) >> _dpu_crtc_setup_lm_bounds(crtc, crtc_state); >> - /* FIXME: move this to dpu_plane_atomic_check? */ >> - drm_atomic_crtc_state_for_each_plane_state(plane, pstate, >> crtc_state) { >> - struct dpu_plane_state *dpu_pstate = to_dpu_plane_state(pstate); >> - >> - if (IS_ERR_OR_NULL(pstate)) { >> - rc = PTR_ERR(pstate); >> - DPU_ERROR("%s: failed to get plane%d state, %d\n", >> - dpu_crtc->name, plane->base.id, rc); >> - return rc; >> + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) { >> + const struct drm_plane_state *pstate; >> + struct dpu_plane_state *dpu_pstate; >> + >> + pstate = drm_atomic_get_plane_state(crtc_state->state, plane); >> + if (IS_ERR(pstate)) >> + return PTR_ERR(pstate); >> + >> + if (dpu_use_virtual_planes) { >> + /* >> + * In case of virtual planes, the plane's atomic_check >> + * is a shortcut. Perform actual check here, after >> + * allocating SSPPs. >> + */ >> + rc = dpu_plane_atomic_check(plane, crtc_state->state); >> + if (rc) >> + return rc; >> } >> if (!pstate->visible) >> continue; >> + /* FIXME: move this to dpu_plane_atomic_check? */ >> + dpu_pstate = to_dpu_plane_state(pstate); > > Anything prevents us from doing it even now instead of a FIXME? The question mark at the end. I'm not sure that moving it to the plane_atomic_check would actually help. And please note that it is the old code (and old FIXME) being moved around. > >> dpu_pstate->needs_dirtyfb = needs_dirtyfb; >> } >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> index 35194262e628..487bb19ee9d6 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> @@ -50,6 +50,9 @@ >> #define DPU_DEBUGFS_DIR "msm_dpu" >> #define DPU_DEBUGFS_HWMASKNAME "hw_log_mask" >> +bool dpu_use_virtual_planes = false; >> +module_param(dpu_use_virtual_planes, bool, 0); >> + >> static int dpu_kms_hw_init(struct msm_kms *kms); >> static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms); >> @@ -735,38 +738,54 @@ static int _dpu_kms_setup_displays(struct >> drm_device *dev, >> return rc; >> } >> -#define MAX_PLANES 20 >> -static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) >> +static int dpu_kms_create_virtual_planes(struct dpu_kms *dpu_kms, >> + int max_crtc_count, >> + struct drm_plane **primary_planes, >> + struct drm_plane **cursor_planes) >> { >> - struct drm_device *dev; >> - struct drm_plane *primary_planes[MAX_PLANES], *plane; >> - struct drm_plane *cursor_planes[MAX_PLANES] = { NULL }; >> - struct drm_crtc *crtc; >> - struct drm_encoder *encoder; >> - unsigned int num_encoders; >> + const struct dpu_mdss_cfg *catalog = dpu_kms->catalog; >> + struct drm_device *dev = dpu_kms->dev; >> + int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; >> + struct drm_plane *plane; >> - struct msm_drm_private *priv; >> - const struct dpu_mdss_cfg *catalog; >> + /* Create the planes, keeping track of one primary/cursor per >> crtc */ >> + for (i = 0; i < catalog->sspp_count; i++) { >> + enum drm_plane_type type; >> - int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; >> - int max_crtc_count; >> - dev = dpu_kms->dev; >> - priv = dev->dev_private; >> - catalog = dpu_kms->catalog; >> + if (primary_planes_idx < max_crtc_count) >> + type = DRM_PLANE_TYPE_PRIMARY; >> + else if (cursor_planes_idx < max_crtc_count) >> + type = DRM_PLANE_TYPE_CURSOR; >> + else >> + type = DRM_PLANE_TYPE_OVERLAY; >> - /* >> - * Create encoder and query display drivers to create >> - * bridges and connectors >> - */ >> - ret = _dpu_kms_setup_displays(dev, priv, dpu_kms); >> - if (ret) >> - return ret; >> + DPU_DEBUG("Create plane type %d\n", type); >> - num_encoders = 0; >> - drm_for_each_encoder(encoder, dev) >> - num_encoders++; >> + plane = dpu_plane_init_virtual(dev, type, (1UL << >> max_crtc_count) - 1); >> + if (IS_ERR(plane)) { >> + DPU_ERROR("dpu_plane_init failed\n"); >> + ret = PTR_ERR(plane); >> + return ret; >> + } >> - max_crtc_count = min(catalog->mixer_count, num_encoders); >> + if (type == DRM_PLANE_TYPE_CURSOR) >> + cursor_planes[cursor_planes_idx++] = plane; >> + else if (type == DRM_PLANE_TYPE_PRIMARY) >> + primary_planes[primary_planes_idx++] = plane; >> + } >> + >> + return primary_planes_idx; >> +} >> + >> +static int dpu_kms_create_planes(struct dpu_kms *dpu_kms, >> + int max_crtc_count, >> + struct drm_plane **primary_planes, >> + struct drm_plane **cursor_planes) >> +{ >> + const struct dpu_mdss_cfg *catalog = dpu_kms->catalog; >> + struct drm_device *dev = dpu_kms->dev; >> + int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; >> + struct drm_plane *plane; >> /* Create the planes, keeping track of one primary/cursor per >> crtc */ >> for (i = 0; i < catalog->sspp_count; i++) { >> @@ -784,8 +803,8 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms >> *dpu_kms) >> type, catalog->sspp[i].features, >> catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR)); >> - plane = dpu_plane_init(dev, catalog->sspp[i].id, type, >> - (1UL << max_crtc_count) - 1); >> + plane = dpu_plane_init_sspp(dev, catalog->sspp[i].id, type, >> + (1UL << max_crtc_count) - 1); >> if (IS_ERR(plane)) { >> DPU_ERROR("dpu_plane_init failed\n"); >> ret = PTR_ERR(plane); >> @@ -798,7 +817,50 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms >> *dpu_kms) >> primary_planes[primary_planes_idx++] = plane; >> } >> - max_crtc_count = min(max_crtc_count, primary_planes_idx); >> + return primary_planes_idx; >> +} >> + >> +#define MAX_PLANES 20 >> +static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) >> +{ >> + struct drm_device *dev; >> + struct drm_plane *primary_planes[MAX_PLANES]; >> + struct drm_plane *cursor_planes[MAX_PLANES] = { NULL }; >> + struct drm_crtc *crtc; >> + struct drm_encoder *encoder; >> + unsigned int num_encoders; >> + >> + struct msm_drm_private *priv; >> + const struct dpu_mdss_cfg *catalog; >> + int i, ret; >> + int max_crtc_count; >> + >> + dev = dpu_kms->dev; >> + priv = dev->dev_private; >> + catalog = dpu_kms->catalog; >> + >> + /* >> + * Create encoder and query display drivers to create >> + * bridges and connectors >> + */ >> + ret = _dpu_kms_setup_displays(dev, priv, dpu_kms); >> + if (ret) >> + return ret; >> + >> + num_encoders = 0; >> + drm_for_each_encoder(encoder, dev) >> + num_encoders++; >> + >> + max_crtc_count = min(catalog->mixer_count, num_encoders); >> + >> + if (dpu_use_virtual_planes) >> + ret = dpu_kms_create_virtual_planes(dpu_kms, max_crtc_count, >> primary_planes, cursor_planes); >> + else >> + ret = dpu_kms_create_planes(dpu_kms, max_crtc_count, >> primary_planes, cursor_planes); >> + if (ret < 0) >> + return ret; >> + >> + max_crtc_count = min(max_crtc_count, ret); >> /* Create one CRTC per encoder */ >> for (i = 0; i < max_crtc_count; i++) { >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >> index 934874eb2248..9f6478f0ced6 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >> @@ -65,6 +65,8 @@ >> #define DPU_NAME_SIZE 12 >> +extern bool dpu_use_virtual_planes; >> + >> struct dpu_kms { >> struct msm_kms base; >> struct drm_device *dev; >> @@ -134,6 +136,8 @@ struct dpu_global_state { >> uint32_t ctl_to_crtc_id[CTL_MAX - CTL_0]; >> uint32_t dspp_to_crtc_id[DSPP_MAX - DSPP_0]; >> uint32_t dsc_to_crtc_id[DSC_MAX - DSC_0]; >> + >> + uint32_t sspp_to_crtc_id[SSPP_MAX - SSPP_NONE]; >> }; >> struct dpu_global_state >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> index cf17075676d5..ee906c276aa5 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> @@ -837,8 +837,77 @@ static int dpu_plane_atomic_check_pipe(struct >> dpu_plane *pdpu, >> return 0; >> } >> -static int dpu_plane_atomic_check(struct drm_plane *plane, >> - struct drm_atomic_state *state) >> +static int dpu_plane_virtual_atomic_check(struct drm_plane *plane, >> + struct drm_atomic_state *state) >> +{ >> + struct drm_plane_state *plane_state = >> + drm_atomic_get_plane_state(state, plane); >> + struct dpu_plane_state *pstate = to_dpu_plane_state(plane_state); >> + const struct dpu_format *format; >> + struct drm_crtc_state *crtc_state; >> + >> + /* >> + * Main part of checks, including >> drm_atomic_helper_check_plane_state() >> + * is called from dpu_crtc_atomic_check(). Do minimal processing >> here. >> + */ >> + >> + if (!plane_state->fb) { >> + plane_state->visible = false; >> + >> + /* resources are freed by dpu_crtc_atomic_check(), but clean >> them here */ >> + pstate->pipe.sspp = NULL; >> + pstate->r_pipe.sspp = NULL; >> + >> + return 0; >> + } >> + >> + format = to_dpu_format(msm_framebuffer_format(plane_state->fb)); >> + crtc_state = drm_atomic_get_new_crtc_state(state, >> plane_state->crtc); >> + >> + /* force resource reallocation if the format of FB has changed */ >> + if (pstate->saved_fmt != format) { >> + crtc_state->planes_changed = true; >> + pstate->saved_fmt = format; >> + } >> + >> + return 0; >> +} >> + >> +int dpu_plane_virtual_assign_resources(struct drm_plane *plane, >> + struct drm_crtc *crtc, >> + struct dpu_global_state *global_state, >> + struct drm_atomic_state *state) >> +{ >> + struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); >> + struct dpu_plane_state *pstate; >> + struct drm_plane_state *plane_state; >> + struct dpu_hw_sspp *hw_sspp; >> + bool yuv, scale, rot90; >> + >> + plane_state = drm_atomic_get_plane_state(state, plane); >> + if (IS_ERR(plane_state)) >> + return PTR_ERR(plane_state); >> + >> + yuv = plane_state->fb ? >> + >> DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(plane_state->fb))) : >> + false; >> + scale = (plane_state->src_w >> 16 != plane_state->crtc_w) || >> + (plane_state->src_h >> 16 != plane_state->crtc_h); >> + >> + rot90 = drm_rotation_90_or_270(plane_state->rotation); >> + >> + hw_sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, >> yuv, scale, rot90); > > I think this parameter list is going to explode. Shall we introduce a > dpu_plane_sspp_requirements to store these? SG. > >> + if (!hw_sspp) >> + return -ENODEV; >> + >> + pstate = to_dpu_plane_state(plane_state); >> + pstate->pipe.sspp = hw_sspp; >> + >> + return 0; >> +} >> + >> +int dpu_plane_atomic_check(struct drm_plane *plane, >> + struct drm_atomic_state *state) >> { >> struct drm_plane_state *new_plane_state = >> drm_atomic_get_new_plane_state(state, >> plane); >> @@ -863,8 +932,10 @@ static int dpu_plane_atomic_check(struct >> drm_plane *plane, >> crtc_state = drm_atomic_get_new_crtc_state(state, >> new_plane_state->crtc); >> - pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); >> - r_pipe->sspp = NULL; >> + if (pdpu->pipe != SSPP_NONE) { >> + pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); >> + r_pipe->sspp = NULL; >> + } >> pipe_hw_caps = pstate->pipe.sspp->cap; >> sblk = pstate->pipe.sspp->cap->sblk; >> @@ -1358,12 +1429,14 @@ static void >> dpu_plane_atomic_print_state(struct drm_printer *p, >> drm_printf(p, "\tstage=%d\n", pstate->stage); >> - drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name); >> - drm_printf(p, "\tmultirect_mode[0]=%s\n", >> dpu_get_multirect_mode(pipe->multirect_mode)); >> - drm_printf(p, "\tmultirect_index[0]=%s\n", >> - dpu_get_multirect_index(pipe->multirect_index)); >> - drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", >> DRM_RECT_ARG(&pipe_cfg->src_rect)); >> - drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", >> DRM_RECT_ARG(&pipe_cfg->dst_rect)); >> + if (pipe->sspp) { >> + drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name); >> + drm_printf(p, "\tmultirect_mode[0]=%s\n", >> dpu_get_multirect_mode(pipe->multirect_mode)); >> + drm_printf(p, "\tmultirect_index[0]=%s\n", >> + dpu_get_multirect_index(pipe->multirect_index)); >> + drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", >> DRM_RECT_ARG(&pipe_cfg->src_rect)); >> + drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", >> DRM_RECT_ARG(&pipe_cfg->dst_rect)); >> + } >> if (r_pipe->sspp) { >> drm_printf(p, "\tsspp[1]=%s\n", r_pipe->sspp->cap->name); >> @@ -1453,18 +1526,30 @@ static const struct drm_plane_helper_funcs >> dpu_plane_helper_funcs = { >> .atomic_update = dpu_plane_atomic_update, >> }; >> +/* >> + * For virtual planes atomic_check is called from >> dpu_crtc_atomic_check(), >> + * after CRTC code assigning SSPP. >> + */ >> +static const struct drm_plane_helper_funcs >> dpu_plane_virtual_helper_funcs = { >> + .prepare_fb = dpu_plane_prepare_fb, >> + .cleanup_fb = dpu_plane_cleanup_fb, >> + .atomic_check = dpu_plane_virtual_atomic_check, >> + .atomic_update = dpu_plane_atomic_update, >> +}; >> + >> /* initialize plane */ >> -struct drm_plane *dpu_plane_init(struct drm_device *dev, >> - uint32_t pipe, enum drm_plane_type type, >> - unsigned long possible_crtcs) >> +static struct drm_plane *dpu_plane_init(struct drm_device *dev, >> + enum drm_plane_type type, >> + unsigned long possible_crtcs, >> + bool inline_rotation, >> + const uint32_t *format_list, >> + uint32_t num_formats, >> + enum dpu_sspp pipe) >> { >> struct drm_plane *plane = NULL; >> - const uint32_t *format_list; >> struct dpu_plane *pdpu; >> struct msm_drm_private *priv = dev->dev_private; >> struct dpu_kms *kms = to_dpu_kms(priv->kms); >> - struct dpu_hw_sspp *pipe_hw; >> - uint32_t num_formats; >> uint32_t supported_rotations; >> int ret = -EINVAL; >> @@ -1480,16 +1565,6 @@ struct drm_plane *dpu_plane_init(struct >> drm_device *dev, >> plane = &pdpu->base; >> pdpu->pipe = pipe; >> - /* initialize underlying h/w driver */ >> - pipe_hw = dpu_rm_get_sspp(&kms->rm, pipe); >> - if (!pipe_hw || !pipe_hw->cap || !pipe_hw->cap->sblk) { >> - DPU_ERROR("[%u]SSPP is invalid\n", pipe); >> - goto clean_plane; >> - } >> - >> - format_list = pipe_hw->cap->sblk->format_list; >> - num_formats = pipe_hw->cap->sblk->num_formats; >> - >> ret = drm_universal_plane_init(dev, plane, 0xff, &dpu_plane_funcs, >> format_list, num_formats, >> supported_format_modifiers, type, NULL); >> @@ -1510,7 +1585,7 @@ struct drm_plane *dpu_plane_init(struct >> drm_device *dev, >> supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0 >> | DRM_MODE_ROTATE_180; >> - if (pipe_hw->cap->features & BIT(DPU_SSPP_INLINE_ROTATION)) >> + if (inline_rotation) >> supported_rotations |= DRM_MODE_ROTATE_MASK; >> drm_plane_create_rotation_property(plane, >> @@ -1519,8 +1594,6 @@ struct drm_plane *dpu_plane_init(struct >> drm_device *dev, >> drm_plane_enable_fb_damage_clips(plane); >> /* success! finalize initialization */ >> - drm_plane_helper_add(plane, &dpu_plane_helper_funcs); >> - >> mutex_init(&pdpu->lock); >> DPU_DEBUG("%s created for pipe:%u id:%u\n", plane->name, >> @@ -1531,3 +1604,59 @@ struct drm_plane *dpu_plane_init(struct >> drm_device *dev, >> kfree(pdpu); >> return ERR_PTR(ret); >> } >> + >> +struct drm_plane *dpu_plane_init_sspp(struct drm_device *dev, >> + uint32_t pipe, enum drm_plane_type type, >> + unsigned long possible_crtcs) >> +{ >> + struct drm_plane *plane = NULL; >> + struct msm_drm_private *priv = dev->dev_private; >> + struct dpu_kms *kms = to_dpu_kms(priv->kms); >> + struct dpu_hw_sspp *pipe_hw; >> + >> + /* initialize underlying h/w driver */ >> + pipe_hw = dpu_rm_get_sspp(&kms->rm, pipe); >> + if (!pipe_hw || !pipe_hw->cap || !pipe_hw->cap->sblk) { >> + DPU_ERROR("[%u]SSPP is invalid\n", pipe); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + >> + plane = dpu_plane_init(dev, type, possible_crtcs, >> + pipe_hw->cap->features & >> BIT(DPU_SSPP_INLINE_ROTATION), >> + pipe_hw->cap->sblk->format_list, >> + pipe_hw->cap->sblk->num_formats, >> + pipe); >> + if (IS_ERR(plane)) >> + return plane; >> + >> + drm_plane_helper_add(plane, &dpu_plane_helper_funcs); >> + >> + DPU_DEBUG("%s created for pipe:%u id:%u\n", plane->name, >> + pipe, plane->base.id); >> + >> + return plane; >> +} >> + >> +struct drm_plane *dpu_plane_init_virtual(struct drm_device *dev, >> + enum drm_plane_type type, >> + unsigned long possible_crtcs) >> +{ >> + struct drm_plane *plane = NULL; >> + struct msm_drm_private *priv = dev->dev_private; >> + struct dpu_kms *kms = to_dpu_kms(priv->kms); >> + >> + plane = dpu_plane_init(dev, type, possible_crtcs, >> + kms->catalog->caps->has_inline_rot, >> + kms->catalog->caps->format_list, >> + kms->catalog->caps->num_formats, >> + SSPP_NONE); >> + if (IS_ERR(plane)) >> + return plane; >> + >> + drm_plane_helper_add(plane, &dpu_plane_virtual_helper_funcs); >> + >> + DPU_DEBUG("%s created virtual id:%u\n", plane->name, >> plane->base.id); >> + >> + return plane; >> +} > > Overall I am satisfied with the split of the dpu_plane_init() function > into virtual vs non-virtual and happy its protected with a modparam. > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h >> index abd6b21a049b..cb1e31ef0d3f 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h >> @@ -31,6 +31,7 @@ >> * @plane_clk: calculated clk per plane >> * @needs_dirtyfb: whether attached CRTC needs pixel data explicitly >> flushed >> * @rotation: simplified drm rotation hint >> + * @saved_fmt: format used by the plane's FB, saved for for virtual >> plane support >> */ >> struct dpu_plane_state { >> struct drm_plane_state base; >> @@ -48,6 +49,8 @@ struct dpu_plane_state { >> bool needs_dirtyfb; >> unsigned int rotation; >> + >> + const struct dpu_format *saved_fmt; >> }; >> #define to_dpu_plane_state(x) \ >> @@ -66,17 +69,27 @@ void dpu_plane_flush(struct drm_plane *plane); >> void dpu_plane_set_error(struct drm_plane *plane, bool error); >> /** >> - * dpu_plane_init - create new dpu plane for the given pipe >> + * dpu_plane_init_sspp - create new dpu plane for the given pipe >> * @dev: Pointer to DRM device >> * @pipe: dpu hardware pipe identifier >> * @type: Plane type - PRIMARY/OVERLAY/CURSOR >> * @possible_crtcs: bitmask of crtc that can be attached to the >> given pipe >> * >> */ >> -struct drm_plane *dpu_plane_init(struct drm_device *dev, >> +struct drm_plane *dpu_plane_init_sspp(struct drm_device *dev, >> uint32_t pipe, enum drm_plane_type type, >> unsigned long possible_crtcs); >> +/** >> + * dpu_plane_init_virtual - create new dpu virtualized plane >> + * @dev: Pointer to DRM device >> + * @type: Plane type - PRIMARY/OVERLAY/CURSOR >> + * @possible_crtcs: bitmask of crtc that can be attached to the given >> pipe >> + */ >> +struct drm_plane *dpu_plane_init_virtual(struct drm_device *dev, >> + enum drm_plane_type type, >> + unsigned long possible_crtcs); >> + >> /** >> * dpu_plane_color_fill - enables color fill on plane >> * @plane: Pointer to DRM plane object >> @@ -93,4 +106,11 @@ void dpu_plane_danger_signal_ctrl(struct drm_plane >> *plane, bool enable); >> static inline void dpu_plane_danger_signal_ctrl(struct drm_plane >> *plane, bool enable) {} >> #endif >> +int dpu_plane_atomic_check(struct drm_plane *plane, struct >> drm_atomic_state *state); >> + >> +int dpu_plane_virtual_assign_resources(struct drm_plane *plane, >> + struct drm_crtc *crtc, >> + struct dpu_global_state *global_state, >> + struct drm_atomic_state *state); >> + >> #endif /* _DPU_PLANE_H_ */ >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> index f0a94008d17a..6130ac87d7e3 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> @@ -607,6 +607,71 @@ int dpu_rm_reserve( >> return ret; >> } >> +struct dpu_hw_sspp *dpu_rm_reserve_sspp(struct dpu_rm *rm, >> + struct dpu_global_state *global_state, >> + struct drm_crtc *crtc, >> + bool yuv, bool scale, bool rot90) >> +{ > > as noted above, lets consider adding a dpu_plane_sspp_requirements > struct instead of just expanding the param list. > >> + uint32_t crtc_id = crtc->base.id; >> + struct dpu_hw_sspp *hw_sspp; >> + bool retry = false; >> + int i; >> + >> +retry_loop: >> + for (i = 0; i < ARRAY_SIZE(rm->hw_sspp); i++) { >> + if (!rm->hw_sspp[i]) >> + continue; >> + >> + if (global_state->sspp_to_crtc_id[i]) >> + continue; >> + >> + hw_sspp = rm->hw_sspp[i]; >> + >> + /* skip incompatible planes */ >> + if (scale && !(hw_sspp->cap->features & DPU_SSPP_SCALER)) >> + continue; >> + >> + if (yuv && !(hw_sspp->cap->features & DPU_SSPP_CSC_ANY)) >> + continue; >> + >> + if (rot90 && !(hw_sspp->cap->features & >> DPU_SSPP_INLINE_ROTATION)) >> + continue; >> + > > These are sufficient conditions to start with for the use-cases I can > think of. > >> + /* >> + * For non-yuv, non-scaled planes try to find simple (DMA) >> + * plane, fallback to VIG on a second try. >> + * >> + * This way we'd leave VIG sspps to be later used for YUV >> formats. >> + */ >> + >> + if (!scale && !yuv && !rot90 && !retry && >> + (hw_sspp->cap->features & >> + (DPU_SSPP_SCALER | DPU_SSPP_CSC_ANY | >> DPU_SSPP_INLINE_ROTATION))) >> + continue; >> + >> + global_state->sspp_to_crtc_id[hw_sspp->idx - SSPP_NONE] = >> crtc_id; >> + >> + return hw_sspp; >> + } >> + >> + /* If we were looking for DMA plane, retry looking for VIG plane */ >> + if (!scale && !yuv && !retry) { >> + retry = true; >> + goto retry_loop; >> + } >> + >> + return NULL; >> +} >> + >> +void dpu_rm_release_all_sspp(struct dpu_global_state *global_state, >> + struct drm_crtc *crtc) >> +{ >> + uint32_t crtc_id = crtc->base.id; >> + >> + _dpu_rm_clear_mapping(global_state->sspp_to_crtc_id, >> + ARRAY_SIZE(global_state->sspp_to_crtc_id), crtc_id); >> +} >> + >> int dpu_rm_get_assigned_resources(struct dpu_rm *rm, >> struct dpu_global_state *global_state, struct drm_crtc *crtc, >> enum dpu_hw_blk_type type, struct dpu_hw_blk **blks, int blks_size) >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >> index f402bec8322b..5bf6740ecb45 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >> @@ -82,6 +82,30 @@ int dpu_rm_reserve( >> void dpu_rm_release(struct dpu_global_state *global_state, >> struct drm_crtc *crtc); >> +/** >> + * dpu_rm_reserve_sspp - Reserve the required SSPP for the provided CRTC >> + * @rm: DPU Resource Manager handle >> + * @global_state: private global state >> + * @crtc: DRM CRTC handle >> + * @yuv: required SSPP supporting YUV formats >> + * @scale: required SSPP supporting scaling >> + * @rot90: required SSPP supporting inline 90 degree rotation >> + */ >> +struct dpu_hw_sspp *dpu_rm_reserve_sspp(struct dpu_rm *rm, >> + struct dpu_global_state *global_state, >> + struct drm_crtc *crtc, >> + bool yuv, bool scale, bool rot90); >> + >> +/** >> + * dpu_rm_release_all_sspp - Given the CRTC, release all SSPP >> + * blocks previously reserved for that use case. >> + * @rm: DPU Resource Manager handle >> + * @crtc: DRM CRTC handle >> + * @Return: 0 on Success otherwise -ERROR >> + */ >> +void dpu_rm_release_all_sspp(struct dpu_global_state *global_state, >> + struct drm_crtc *crtc); >> + >> /** >> * Get hw resources of the given type that are assigned to this >> encoder. >> */
On 6/7/2023 2:56 PM, Dmitry Baryshkov wrote: > On 08/06/2023 00:05, Abhinav Kumar wrote: >> >> >> On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote: >>> Only several SSPP blocks support such features as YUV output or scaling, >>> thus different DRM planes have different features. Properly utilizing >>> all planes requires the attention of the compositor, who should >>> prefer simpler planes to YUV-supporting ones. Otherwise it is very easy >>> to end up in a situation when all featureful planes are already >>> allocated for simple windows, leaving no spare plane for YUV playback. >>> >>> To solve this problem make all planes virtual. Each plane is registered >>> as if it supports all possible features, but then at the runtime during >>> the atomic_check phase the driver selects backing SSPP block for each >>> plane. >>> >>> Note, this does not provide support for using two different SSPP blocks >>> for a single plane or using two rectangles of an SSPP to drive two >>> planes. Each plane still gets its own SSPP and can utilize either a solo >>> rectangle or both multirect rectangles depending on the resolution. >>> >>> Note #2: By default support for virtual planes is turned off and the >>> driver still uses old code path with preallocated SSPP block for each >>> plane. To enable virtual planes, pass 'msm.dpu_use_virtual_planes=1' >>> kernel parameter. >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >> >> There will be some rebase needed to switch back to encoder based >> allocation so I am not going to comment on those parts and will let >> you handle that when you post v3. >> >> But my questions/comments below are for other things. >> >>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 59 +++++-- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 120 ++++++++++---- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 + >>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 187 ++++++++++++++++++---- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 24 ++- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 65 ++++++++ >>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 24 +++ >>> 7 files changed, 413 insertions(+), 70 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> index 8ef191fd002d..cdece21b81c9 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> @@ -1273,6 +1273,29 @@ static int dpu_crtc_assign_resources(struct >>> drm_crtc *crtc, struct drm_crtc_stat >>> return 0; >>> } >>> +static int dpu_crtc_assign_plane_resources(struct drm_crtc *crtc, >>> struct drm_crtc_state *crtc_state) >>> +{ >>> + struct dpu_global_state *global_state; >>> + struct drm_plane *plane; >>> + int rc; >>> + >>> + global_state = dpu_kms_get_global_state(crtc_state->state); >>> + if (IS_ERR(global_state)) >>> + return PTR_ERR(global_state); >>> + >>> + dpu_rm_release_all_sspp(global_state, crtc); >>> + >>> + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) { >>> + rc = dpu_plane_virtual_assign_resources(plane, crtc, >>> + global_state, >>> + crtc_state->state); >>> + if (rc) >>> + return rc; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static int dpu_crtc_atomic_check(struct drm_crtc *crtc, >>> struct drm_atomic_state *state) >>> { >>> @@ -1281,7 +1304,6 @@ static int dpu_crtc_atomic_check(struct >>> drm_crtc *crtc, >>> struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); >>> struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); >>> - const struct drm_plane_state *pstate; >>> struct drm_plane *plane; >>> int rc = 0; >>> @@ -1294,6 +1316,13 @@ static int dpu_crtc_atomic_check(struct >>> drm_crtc *crtc, >>> return rc; >>> } >>> + if (dpu_use_virtual_planes && >>> + crtc_state->planes_changed) { >>> + rc = dpu_crtc_assign_plane_resources(crtc, crtc_state); >>> + if (rc < 0) >>> + return rc; >>> + } >> >> Can you please explain a bit more about the planes_changed condition? >> >> 1) Are we doing this because the plane's atomic check happens before >> the CRTC atomic check? > > Yes. Another alternative would be to stop using > drm_atomic_helper_check() and implement our own code instead, inverting > the plane / CRTC check order. > I am fine with that too but as you noted in (3), if planes_changed will be set by those functions and you can drop explicit assignment of it in this patch, that will be the best option for me. >> >> 2) So the DRM core sets this to true already when plane is switching >> CRTCs or being connected to a CRTC for the first time, we need to >> handle the conditions additional to that right? > > Nit: it is not possible to switch CRTCs. Plane first has to be > disconnected and then to be connected to another CRTC. > >> >> 3) If (2) is correct, arent there other conditions then to be handled >> for setting planes_changed to true? >> >> Some examples include, switching from a scaling to non-scaling >> scenario, needing rotation vs not needing etc. > > Setting the plane_changed is not required. Both > drm_atomic_helper_disable_plane() and drm_atomic_helper_update_plane() > will add the plane to the state. Then drm_atomic_helper_check_planes() > will call drm_atomic_helper_plane_changed(), setting > {old_,new_}crtc_state->planes_changed. > > I should check if the format check below is required or not. It looks > like I can drop that clause too. > Yes, please do. From the above explanation, it seems like we dont need to explicity set it again ourselves for format change. > >> >> Basically it seems like all of this is handled within the >> reserve_sspp() function but if planes_changes is not set then that >> wont get invoked at all. >> >> >>> + >>> if (!crtc_state->enable || !crtc_state->active) { >>> DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip >>> atomic_check\n", >>> crtc->base.id, crtc_state->enable, >>> @@ -1311,20 +1340,30 @@ static int dpu_crtc_atomic_check(struct >>> drm_crtc *crtc, >>> if (cstate->num_mixers) >>> _dpu_crtc_setup_lm_bounds(crtc, crtc_state); >>> - /* FIXME: move this to dpu_plane_atomic_check? */ >>> - drm_atomic_crtc_state_for_each_plane_state(plane, pstate, >>> crtc_state) { >>> - struct dpu_plane_state *dpu_pstate = >>> to_dpu_plane_state(pstate); >>> - >>> - if (IS_ERR_OR_NULL(pstate)) { >>> - rc = PTR_ERR(pstate); >>> - DPU_ERROR("%s: failed to get plane%d state, %d\n", >>> - dpu_crtc->name, plane->base.id, rc); >>> - return rc; >>> + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) { >>> + const struct drm_plane_state *pstate; >>> + struct dpu_plane_state *dpu_pstate; >>> + >>> + pstate = drm_atomic_get_plane_state(crtc_state->state, plane); >>> + if (IS_ERR(pstate)) >>> + return PTR_ERR(pstate); >>> + >>> + if (dpu_use_virtual_planes) { >>> + /* >>> + * In case of virtual planes, the plane's atomic_check >>> + * is a shortcut. Perform actual check here, after >>> + * allocating SSPPs. >>> + */ >>> + rc = dpu_plane_atomic_check(plane, crtc_state->state); >>> + if (rc) >>> + return rc; >>> } >>> if (!pstate->visible) >>> continue; >>> + /* FIXME: move this to dpu_plane_atomic_check? */ >>> + dpu_pstate = to_dpu_plane_state(pstate); >> >> Anything prevents us from doing it even now instead of a FIXME? > > The question mark at the end. I'm not sure that moving it to the > plane_atomic_check would actually help. And please note that it is the > old code (and old FIXME) being moved around. > Ah sorry, this is old code, so never mind. >> >>> dpu_pstate->needs_dirtyfb = needs_dirtyfb; >>> } >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>> index 35194262e628..487bb19ee9d6 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>> @@ -50,6 +50,9 @@ >>> #define DPU_DEBUGFS_DIR "msm_dpu" >>> #define DPU_DEBUGFS_HWMASKNAME "hw_log_mask" >>> +bool dpu_use_virtual_planes = false; >>> +module_param(dpu_use_virtual_planes, bool, 0); >>> + >>> static int dpu_kms_hw_init(struct msm_kms *kms); >>> static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms); >>> @@ -735,38 +738,54 @@ static int _dpu_kms_setup_displays(struct >>> drm_device *dev, >>> return rc; >>> } >>> -#define MAX_PLANES 20 >>> -static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) >>> +static int dpu_kms_create_virtual_planes(struct dpu_kms *dpu_kms, >>> + int max_crtc_count, >>> + struct drm_plane **primary_planes, >>> + struct drm_plane **cursor_planes) >>> { >>> - struct drm_device *dev; >>> - struct drm_plane *primary_planes[MAX_PLANES], *plane; >>> - struct drm_plane *cursor_planes[MAX_PLANES] = { NULL }; >>> - struct drm_crtc *crtc; >>> - struct drm_encoder *encoder; >>> - unsigned int num_encoders; >>> + const struct dpu_mdss_cfg *catalog = dpu_kms->catalog; >>> + struct drm_device *dev = dpu_kms->dev; >>> + int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; >>> + struct drm_plane *plane; >>> - struct msm_drm_private *priv; >>> - const struct dpu_mdss_cfg *catalog; >>> + /* Create the planes, keeping track of one primary/cursor per >>> crtc */ >>> + for (i = 0; i < catalog->sspp_count; i++) { >>> + enum drm_plane_type type; >>> - int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; >>> - int max_crtc_count; >>> - dev = dpu_kms->dev; >>> - priv = dev->dev_private; >>> - catalog = dpu_kms->catalog; >>> + if (primary_planes_idx < max_crtc_count) >>> + type = DRM_PLANE_TYPE_PRIMARY; >>> + else if (cursor_planes_idx < max_crtc_count) >>> + type = DRM_PLANE_TYPE_CURSOR; >>> + else >>> + type = DRM_PLANE_TYPE_OVERLAY; >>> - /* >>> - * Create encoder and query display drivers to create >>> - * bridges and connectors >>> - */ >>> - ret = _dpu_kms_setup_displays(dev, priv, dpu_kms); >>> - if (ret) >>> - return ret; >>> + DPU_DEBUG("Create plane type %d\n", type); >>> - num_encoders = 0; >>> - drm_for_each_encoder(encoder, dev) >>> - num_encoders++; >>> + plane = dpu_plane_init_virtual(dev, type, (1UL << >>> max_crtc_count) - 1); >>> + if (IS_ERR(plane)) { >>> + DPU_ERROR("dpu_plane_init failed\n"); >>> + ret = PTR_ERR(plane); >>> + return ret; >>> + } >>> - max_crtc_count = min(catalog->mixer_count, num_encoders); >>> + if (type == DRM_PLANE_TYPE_CURSOR) >>> + cursor_planes[cursor_planes_idx++] = plane; >>> + else if (type == DRM_PLANE_TYPE_PRIMARY) >>> + primary_planes[primary_planes_idx++] = plane; >>> + } >>> + >>> + return primary_planes_idx; >>> +} >>> + >>> +static int dpu_kms_create_planes(struct dpu_kms *dpu_kms, >>> + int max_crtc_count, >>> + struct drm_plane **primary_planes, >>> + struct drm_plane **cursor_planes) >>> +{ >>> + const struct dpu_mdss_cfg *catalog = dpu_kms->catalog; >>> + struct drm_device *dev = dpu_kms->dev; >>> + int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; >>> + struct drm_plane *plane; >>> /* Create the planes, keeping track of one primary/cursor per >>> crtc */ >>> for (i = 0; i < catalog->sspp_count; i++) { >>> @@ -784,8 +803,8 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms >>> *dpu_kms) >>> type, catalog->sspp[i].features, >>> catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR)); >>> - plane = dpu_plane_init(dev, catalog->sspp[i].id, type, >>> - (1UL << max_crtc_count) - 1); >>> + plane = dpu_plane_init_sspp(dev, catalog->sspp[i].id, type, >>> + (1UL << max_crtc_count) - 1); >>> if (IS_ERR(plane)) { >>> DPU_ERROR("dpu_plane_init failed\n"); >>> ret = PTR_ERR(plane); >>> @@ -798,7 +817,50 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms >>> *dpu_kms) >>> primary_planes[primary_planes_idx++] = plane; >>> } >>> - max_crtc_count = min(max_crtc_count, primary_planes_idx); >>> + return primary_planes_idx; >>> +} >>> + >>> +#define MAX_PLANES 20 >>> +static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) >>> +{ >>> + struct drm_device *dev; >>> + struct drm_plane *primary_planes[MAX_PLANES]; >>> + struct drm_plane *cursor_planes[MAX_PLANES] = { NULL }; >>> + struct drm_crtc *crtc; >>> + struct drm_encoder *encoder; >>> + unsigned int num_encoders; >>> + >>> + struct msm_drm_private *priv; >>> + const struct dpu_mdss_cfg *catalog; >>> + int i, ret; >>> + int max_crtc_count; >>> + >>> + dev = dpu_kms->dev; >>> + priv = dev->dev_private; >>> + catalog = dpu_kms->catalog; >>> + >>> + /* >>> + * Create encoder and query display drivers to create >>> + * bridges and connectors >>> + */ >>> + ret = _dpu_kms_setup_displays(dev, priv, dpu_kms); >>> + if (ret) >>> + return ret; >>> + >>> + num_encoders = 0; >>> + drm_for_each_encoder(encoder, dev) >>> + num_encoders++; >>> + >>> + max_crtc_count = min(catalog->mixer_count, num_encoders); >>> + >>> + if (dpu_use_virtual_planes) >>> + ret = dpu_kms_create_virtual_planes(dpu_kms, max_crtc_count, >>> primary_planes, cursor_planes); >>> + else >>> + ret = dpu_kms_create_planes(dpu_kms, max_crtc_count, >>> primary_planes, cursor_planes); >>> + if (ret < 0) >>> + return ret; >>> + >>> + max_crtc_count = min(max_crtc_count, ret); >>> /* Create one CRTC per encoder */ >>> for (i = 0; i < max_crtc_count; i++) { >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >>> index 934874eb2248..9f6478f0ced6 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >>> @@ -65,6 +65,8 @@ >>> #define DPU_NAME_SIZE 12 >>> +extern bool dpu_use_virtual_planes; >>> + >>> struct dpu_kms { >>> struct msm_kms base; >>> struct drm_device *dev; >>> @@ -134,6 +136,8 @@ struct dpu_global_state { >>> uint32_t ctl_to_crtc_id[CTL_MAX - CTL_0]; >>> uint32_t dspp_to_crtc_id[DSPP_MAX - DSPP_0]; >>> uint32_t dsc_to_crtc_id[DSC_MAX - DSC_0]; >>> + >>> + uint32_t sspp_to_crtc_id[SSPP_MAX - SSPP_NONE]; >>> }; >>> struct dpu_global_state >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> index cf17075676d5..ee906c276aa5 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> @@ -837,8 +837,77 @@ static int dpu_plane_atomic_check_pipe(struct >>> dpu_plane *pdpu, >>> return 0; >>> } >>> -static int dpu_plane_atomic_check(struct drm_plane *plane, >>> - struct drm_atomic_state *state) >>> +static int dpu_plane_virtual_atomic_check(struct drm_plane *plane, >>> + struct drm_atomic_state *state) >>> +{ >>> + struct drm_plane_state *plane_state = >>> + drm_atomic_get_plane_state(state, plane); >>> + struct dpu_plane_state *pstate = to_dpu_plane_state(plane_state); >>> + const struct dpu_format *format; >>> + struct drm_crtc_state *crtc_state; >>> + >>> + /* >>> + * Main part of checks, including >>> drm_atomic_helper_check_plane_state() >>> + * is called from dpu_crtc_atomic_check(). Do minimal processing >>> here. >>> + */ >>> + >>> + if (!plane_state->fb) { >>> + plane_state->visible = false; >>> + >>> + /* resources are freed by dpu_crtc_atomic_check(), but clean >>> them here */ >>> + pstate->pipe.sspp = NULL; >>> + pstate->r_pipe.sspp = NULL; >>> + >>> + return 0; >>> + } >>> + >>> + format = to_dpu_format(msm_framebuffer_format(plane_state->fb)); >>> + crtc_state = drm_atomic_get_new_crtc_state(state, >>> plane_state->crtc); >>> + >>> + /* force resource reallocation if the format of FB has changed */ >>> + if (pstate->saved_fmt != format) { >>> + crtc_state->planes_changed = true; >>> + pstate->saved_fmt = format; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +int dpu_plane_virtual_assign_resources(struct drm_plane *plane, >>> + struct drm_crtc *crtc, >>> + struct dpu_global_state *global_state, >>> + struct drm_atomic_state *state) >>> +{ >>> + struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); >>> + struct dpu_plane_state *pstate; >>> + struct drm_plane_state *plane_state; >>> + struct dpu_hw_sspp *hw_sspp; >>> + bool yuv, scale, rot90; >>> + >>> + plane_state = drm_atomic_get_plane_state(state, plane); >>> + if (IS_ERR(plane_state)) >>> + return PTR_ERR(plane_state); >>> + >>> + yuv = plane_state->fb ? >>> + >>> DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(plane_state->fb))) : >>> + false; >>> + scale = (plane_state->src_w >> 16 != plane_state->crtc_w) || >>> + (plane_state->src_h >> 16 != plane_state->crtc_h); >>> + >>> + rot90 = drm_rotation_90_or_270(plane_state->rotation); >>> + >>> + hw_sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, >>> yuv, scale, rot90); >> >> I think this parameter list is going to explode. Shall we introduce a >> dpu_plane_sspp_requirements to store these? > > SG. > >> >>> + if (!hw_sspp) >>> + return -ENODEV; >>> + >>> + pstate = to_dpu_plane_state(plane_state); >>> + pstate->pipe.sspp = hw_sspp; >>> + >>> + return 0; >>> +} >>> + >>> +int dpu_plane_atomic_check(struct drm_plane *plane, >>> + struct drm_atomic_state *state) >>> { >>> struct drm_plane_state *new_plane_state = >>> drm_atomic_get_new_plane_state(state, >>> plane); >>> @@ -863,8 +932,10 @@ static int dpu_plane_atomic_check(struct >>> drm_plane *plane, >>> crtc_state = drm_atomic_get_new_crtc_state(state, >>> new_plane_state->crtc); >>> - pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); >>> - r_pipe->sspp = NULL; >>> + if (pdpu->pipe != SSPP_NONE) { >>> + pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); >>> + r_pipe->sspp = NULL; >>> + } >>> pipe_hw_caps = pstate->pipe.sspp->cap; >>> sblk = pstate->pipe.sspp->cap->sblk; >>> @@ -1358,12 +1429,14 @@ static void >>> dpu_plane_atomic_print_state(struct drm_printer *p, >>> drm_printf(p, "\tstage=%d\n", pstate->stage); >>> - drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name); >>> - drm_printf(p, "\tmultirect_mode[0]=%s\n", >>> dpu_get_multirect_mode(pipe->multirect_mode)); >>> - drm_printf(p, "\tmultirect_index[0]=%s\n", >>> - dpu_get_multirect_index(pipe->multirect_index)); >>> - drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", >>> DRM_RECT_ARG(&pipe_cfg->src_rect)); >>> - drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", >>> DRM_RECT_ARG(&pipe_cfg->dst_rect)); >>> + if (pipe->sspp) { >>> + drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name); >>> + drm_printf(p, "\tmultirect_mode[0]=%s\n", >>> dpu_get_multirect_mode(pipe->multirect_mode)); >>> + drm_printf(p, "\tmultirect_index[0]=%s\n", >>> + dpu_get_multirect_index(pipe->multirect_index)); >>> + drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", >>> DRM_RECT_ARG(&pipe_cfg->src_rect)); >>> + drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", >>> DRM_RECT_ARG(&pipe_cfg->dst_rect)); >>> + } >>> if (r_pipe->sspp) { >>> drm_printf(p, "\tsspp[1]=%s\n", r_pipe->sspp->cap->name); >>> @@ -1453,18 +1526,30 @@ static const struct drm_plane_helper_funcs >>> dpu_plane_helper_funcs = { >>> .atomic_update = dpu_plane_atomic_update, >>> }; >>> +/* >>> + * For virtual planes atomic_check is called from >>> dpu_crtc_atomic_check(), >>> + * after CRTC code assigning SSPP. >>> + */ >>> +static const struct drm_plane_helper_funcs >>> dpu_plane_virtual_helper_funcs = { >>> + .prepare_fb = dpu_plane_prepare_fb, >>> + .cleanup_fb = dpu_plane_cleanup_fb, >>> + .atomic_check = dpu_plane_virtual_atomic_check, >>> + .atomic_update = dpu_plane_atomic_update, >>> +}; >>> + >>> /* initialize plane */ >>> -struct drm_plane *dpu_plane_init(struct drm_device *dev, >>> - uint32_t pipe, enum drm_plane_type type, >>> - unsigned long possible_crtcs) >>> +static struct drm_plane *dpu_plane_init(struct drm_device *dev, >>> + enum drm_plane_type type, >>> + unsigned long possible_crtcs, >>> + bool inline_rotation, >>> + const uint32_t *format_list, >>> + uint32_t num_formats, >>> + enum dpu_sspp pipe) >>> { >>> struct drm_plane *plane = NULL; >>> - const uint32_t *format_list; >>> struct dpu_plane *pdpu; >>> struct msm_drm_private *priv = dev->dev_private; >>> struct dpu_kms *kms = to_dpu_kms(priv->kms); >>> - struct dpu_hw_sspp *pipe_hw; >>> - uint32_t num_formats; >>> uint32_t supported_rotations; >>> int ret = -EINVAL; >>> @@ -1480,16 +1565,6 @@ struct drm_plane *dpu_plane_init(struct >>> drm_device *dev, >>> plane = &pdpu->base; >>> pdpu->pipe = pipe; >>> - /* initialize underlying h/w driver */ >>> - pipe_hw = dpu_rm_get_sspp(&kms->rm, pipe); >>> - if (!pipe_hw || !pipe_hw->cap || !pipe_hw->cap->sblk) { >>> - DPU_ERROR("[%u]SSPP is invalid\n", pipe); >>> - goto clean_plane; >>> - } >>> - >>> - format_list = pipe_hw->cap->sblk->format_list; >>> - num_formats = pipe_hw->cap->sblk->num_formats; >>> - >>> ret = drm_universal_plane_init(dev, plane, 0xff, &dpu_plane_funcs, >>> format_list, num_formats, >>> supported_format_modifiers, type, NULL); >>> @@ -1510,7 +1585,7 @@ struct drm_plane *dpu_plane_init(struct >>> drm_device *dev, >>> supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0 >>> | DRM_MODE_ROTATE_180; >>> - if (pipe_hw->cap->features & BIT(DPU_SSPP_INLINE_ROTATION)) >>> + if (inline_rotation) >>> supported_rotations |= DRM_MODE_ROTATE_MASK; >>> drm_plane_create_rotation_property(plane, >>> @@ -1519,8 +1594,6 @@ struct drm_plane *dpu_plane_init(struct >>> drm_device *dev, >>> drm_plane_enable_fb_damage_clips(plane); >>> /* success! finalize initialization */ >>> - drm_plane_helper_add(plane, &dpu_plane_helper_funcs); >>> - >>> mutex_init(&pdpu->lock); >>> DPU_DEBUG("%s created for pipe:%u id:%u\n", plane->name, >>> @@ -1531,3 +1604,59 @@ struct drm_plane *dpu_plane_init(struct >>> drm_device *dev, >>> kfree(pdpu); >>> return ERR_PTR(ret); >>> } >>> + >>> +struct drm_plane *dpu_plane_init_sspp(struct drm_device *dev, >>> + uint32_t pipe, enum drm_plane_type type, >>> + unsigned long possible_crtcs) >>> +{ >>> + struct drm_plane *plane = NULL; >>> + struct msm_drm_private *priv = dev->dev_private; >>> + struct dpu_kms *kms = to_dpu_kms(priv->kms); >>> + struct dpu_hw_sspp *pipe_hw; >>> + >>> + /* initialize underlying h/w driver */ >>> + pipe_hw = dpu_rm_get_sspp(&kms->rm, pipe); >>> + if (!pipe_hw || !pipe_hw->cap || !pipe_hw->cap->sblk) { >>> + DPU_ERROR("[%u]SSPP is invalid\n", pipe); >>> + return ERR_PTR(-EINVAL); >>> + } >>> + >>> + >>> + plane = dpu_plane_init(dev, type, possible_crtcs, >>> + pipe_hw->cap->features & >>> BIT(DPU_SSPP_INLINE_ROTATION), >>> + pipe_hw->cap->sblk->format_list, >>> + pipe_hw->cap->sblk->num_formats, >>> + pipe); >>> + if (IS_ERR(plane)) >>> + return plane; >>> + >>> + drm_plane_helper_add(plane, &dpu_plane_helper_funcs); >>> + >>> + DPU_DEBUG("%s created for pipe:%u id:%u\n", plane->name, >>> + pipe, plane->base.id); >>> + >>> + return plane; >>> +} >>> + >>> +struct drm_plane *dpu_plane_init_virtual(struct drm_device *dev, >>> + enum drm_plane_type type, >>> + unsigned long possible_crtcs) >>> +{ >>> + struct drm_plane *plane = NULL; >>> + struct msm_drm_private *priv = dev->dev_private; >>> + struct dpu_kms *kms = to_dpu_kms(priv->kms); >>> + >>> + plane = dpu_plane_init(dev, type, possible_crtcs, >>> + kms->catalog->caps->has_inline_rot, >>> + kms->catalog->caps->format_list, >>> + kms->catalog->caps->num_formats, >>> + SSPP_NONE); >>> + if (IS_ERR(plane)) >>> + return plane; >>> + >>> + drm_plane_helper_add(plane, &dpu_plane_virtual_helper_funcs); >>> + >>> + DPU_DEBUG("%s created virtual id:%u\n", plane->name, >>> plane->base.id); >>> + >>> + return plane; >>> +} >> >> Overall I am satisfied with the split of the dpu_plane_init() function >> into virtual vs non-virtual and happy its protected with a modparam. >> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h >>> index abd6b21a049b..cb1e31ef0d3f 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h >>> @@ -31,6 +31,7 @@ >>> * @plane_clk: calculated clk per plane >>> * @needs_dirtyfb: whether attached CRTC needs pixel data >>> explicitly flushed >>> * @rotation: simplified drm rotation hint >>> + * @saved_fmt: format used by the plane's FB, saved for for virtual >>> plane support >>> */ >>> struct dpu_plane_state { >>> struct drm_plane_state base; >>> @@ -48,6 +49,8 @@ struct dpu_plane_state { >>> bool needs_dirtyfb; >>> unsigned int rotation; >>> + >>> + const struct dpu_format *saved_fmt; >>> }; >>> #define to_dpu_plane_state(x) \ >>> @@ -66,17 +69,27 @@ void dpu_plane_flush(struct drm_plane *plane); >>> void dpu_plane_set_error(struct drm_plane *plane, bool error); >>> /** >>> - * dpu_plane_init - create new dpu plane for the given pipe >>> + * dpu_plane_init_sspp - create new dpu plane for the given pipe >>> * @dev: Pointer to DRM device >>> * @pipe: dpu hardware pipe identifier >>> * @type: Plane type - PRIMARY/OVERLAY/CURSOR >>> * @possible_crtcs: bitmask of crtc that can be attached to the >>> given pipe >>> * >>> */ >>> -struct drm_plane *dpu_plane_init(struct drm_device *dev, >>> +struct drm_plane *dpu_plane_init_sspp(struct drm_device *dev, >>> uint32_t pipe, enum drm_plane_type type, >>> unsigned long possible_crtcs); >>> +/** >>> + * dpu_plane_init_virtual - create new dpu virtualized plane >>> + * @dev: Pointer to DRM device >>> + * @type: Plane type - PRIMARY/OVERLAY/CURSOR >>> + * @possible_crtcs: bitmask of crtc that can be attached to the >>> given pipe >>> + */ >>> +struct drm_plane *dpu_plane_init_virtual(struct drm_device *dev, >>> + enum drm_plane_type type, >>> + unsigned long possible_crtcs); >>> + >>> /** >>> * dpu_plane_color_fill - enables color fill on plane >>> * @plane: Pointer to DRM plane object >>> @@ -93,4 +106,11 @@ void dpu_plane_danger_signal_ctrl(struct >>> drm_plane *plane, bool enable); >>> static inline void dpu_plane_danger_signal_ctrl(struct drm_plane >>> *plane, bool enable) {} >>> #endif >>> +int dpu_plane_atomic_check(struct drm_plane *plane, struct >>> drm_atomic_state *state); >>> + >>> +int dpu_plane_virtual_assign_resources(struct drm_plane *plane, >>> + struct drm_crtc *crtc, >>> + struct dpu_global_state *global_state, >>> + struct drm_atomic_state *state); >>> + >>> #endif /* _DPU_PLANE_H_ */ >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> index f0a94008d17a..6130ac87d7e3 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >>> @@ -607,6 +607,71 @@ int dpu_rm_reserve( >>> return ret; >>> } >>> +struct dpu_hw_sspp *dpu_rm_reserve_sspp(struct dpu_rm *rm, >>> + struct dpu_global_state *global_state, >>> + struct drm_crtc *crtc, >>> + bool yuv, bool scale, bool rot90) >>> +{ >> >> as noted above, lets consider adding a dpu_plane_sspp_requirements >> struct instead of just expanding the param list. >> >>> + uint32_t crtc_id = crtc->base.id; >>> + struct dpu_hw_sspp *hw_sspp; >>> + bool retry = false; >>> + int i; >>> + >>> +retry_loop: >>> + for (i = 0; i < ARRAY_SIZE(rm->hw_sspp); i++) { >>> + if (!rm->hw_sspp[i]) >>> + continue; >>> + >>> + if (global_state->sspp_to_crtc_id[i]) >>> + continue; >>> + >>> + hw_sspp = rm->hw_sspp[i]; >>> + >>> + /* skip incompatible planes */ >>> + if (scale && !(hw_sspp->cap->features & DPU_SSPP_SCALER)) >>> + continue; >>> + >>> + if (yuv && !(hw_sspp->cap->features & DPU_SSPP_CSC_ANY)) >>> + continue; >>> + >>> + if (rot90 && !(hw_sspp->cap->features & >>> DPU_SSPP_INLINE_ROTATION)) >>> + continue; >>> + >> >> These are sufficient conditions to start with for the use-cases I can >> think of. >> >>> + /* >>> + * For non-yuv, non-scaled planes try to find simple (DMA) >>> + * plane, fallback to VIG on a second try. >>> + * >>> + * This way we'd leave VIG sspps to be later used for YUV >>> formats. >>> + */ >>> + >>> + if (!scale && !yuv && !rot90 && !retry && >>> + (hw_sspp->cap->features & >>> + (DPU_SSPP_SCALER | DPU_SSPP_CSC_ANY | >>> DPU_SSPP_INLINE_ROTATION))) >>> + continue; >>> + >>> + global_state->sspp_to_crtc_id[hw_sspp->idx - SSPP_NONE] = >>> crtc_id; >>> + >>> + return hw_sspp; >>> + } >>> + >>> + /* If we were looking for DMA plane, retry looking for VIG plane */ >>> + if (!scale && !yuv && !retry) { >>> + retry = true; >>> + goto retry_loop; >>> + } >>> + >>> + return NULL; >>> +} >>> + >>> +void dpu_rm_release_all_sspp(struct dpu_global_state *global_state, >>> + struct drm_crtc *crtc) >>> +{ >>> + uint32_t crtc_id = crtc->base.id; >>> + >>> + _dpu_rm_clear_mapping(global_state->sspp_to_crtc_id, >>> + ARRAY_SIZE(global_state->sspp_to_crtc_id), crtc_id); >>> +} >>> + >>> int dpu_rm_get_assigned_resources(struct dpu_rm *rm, >>> struct dpu_global_state *global_state, struct drm_crtc *crtc, >>> enum dpu_hw_blk_type type, struct dpu_hw_blk **blks, int >>> blks_size) >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >>> index f402bec8322b..5bf6740ecb45 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >>> @@ -82,6 +82,30 @@ int dpu_rm_reserve( >>> void dpu_rm_release(struct dpu_global_state *global_state, >>> struct drm_crtc *crtc); >>> +/** >>> + * dpu_rm_reserve_sspp - Reserve the required SSPP for the provided >>> CRTC >>> + * @rm: DPU Resource Manager handle >>> + * @global_state: private global state >>> + * @crtc: DRM CRTC handle >>> + * @yuv: required SSPP supporting YUV formats >>> + * @scale: required SSPP supporting scaling >>> + * @rot90: required SSPP supporting inline 90 degree rotation >>> + */ >>> +struct dpu_hw_sspp *dpu_rm_reserve_sspp(struct dpu_rm *rm, >>> + struct dpu_global_state *global_state, >>> + struct drm_crtc *crtc, >>> + bool yuv, bool scale, bool rot90); >>> + >>> +/** >>> + * dpu_rm_release_all_sspp - Given the CRTC, release all SSPP >>> + * blocks previously reserved for that use case. >>> + * @rm: DPU Resource Manager handle >>> + * @crtc: DRM CRTC handle >>> + * @Return: 0 on Success otherwise -ERROR >>> + */ >>> +void dpu_rm_release_all_sspp(struct dpu_global_state *global_state, >>> + struct drm_crtc *crtc); >>> + >>> /** >>> * Get hw resources of the given type that are assigned to this >>> encoder. >>> */ >
On 6/8/2023 12:51 PM, Abhinav Kumar wrote: > > > On 6/7/2023 2:56 PM, Dmitry Baryshkov wrote: >> On 08/06/2023 00:05, Abhinav Kumar wrote: >>> >>> >>> On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote: >>>> Only several SSPP blocks support such features as YUV output or >>>> scaling, >>>> thus different DRM planes have different features. Properly utilizing >>>> all planes requires the attention of the compositor, who should >>>> prefer simpler planes to YUV-supporting ones. Otherwise it is very easy >>>> to end up in a situation when all featureful planes are already >>>> allocated for simple windows, leaving no spare plane for YUV playback. >>>> >>>> To solve this problem make all planes virtual. Each plane is registered >>>> as if it supports all possible features, but then at the runtime during >>>> the atomic_check phase the driver selects backing SSPP block for each >>>> plane. >>>> >>>> Note, this does not provide support for using two different SSPP blocks >>>> for a single plane or using two rectangles of an SSPP to drive two >>>> planes. Each plane still gets its own SSPP and can utilize either a >>>> solo >>>> rectangle or both multirect rectangles depending on the resolution. >>>> >>>> Note #2: By default support for virtual planes is turned off and the >>>> driver still uses old code path with preallocated SSPP block for each >>>> plane. To enable virtual planes, pass 'msm.dpu_use_virtual_planes=1' >>>> kernel parameter. >>>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>> >>> There will be some rebase needed to switch back to encoder based >>> allocation so I am not going to comment on those parts and will let >>> you handle that when you post v3. >>> >>> But my questions/comments below are for other things. >>> >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 59 +++++-- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 120 ++++++++++---- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 + >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 187 >>>> ++++++++++++++++++---- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 24 ++- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 65 ++++++++ >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 24 +++ >>>> 7 files changed, 413 insertions(+), 70 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> index 8ef191fd002d..cdece21b81c9 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> @@ -1273,6 +1273,29 @@ static int dpu_crtc_assign_resources(struct >>>> drm_crtc *crtc, struct drm_crtc_stat >>>> return 0; >>>> } >>>> +static int dpu_crtc_assign_plane_resources(struct drm_crtc *crtc, >>>> struct drm_crtc_state *crtc_state) >>>> +{ >>>> + struct dpu_global_state *global_state; >>>> + struct drm_plane *plane; >>>> + int rc; >>>> + >>>> + global_state = dpu_kms_get_global_state(crtc_state->state); >>>> + if (IS_ERR(global_state)) >>>> + return PTR_ERR(global_state); >>>> + >>>> + dpu_rm_release_all_sspp(global_state, crtc); >>>> + >>>> + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) { >>>> + rc = dpu_plane_virtual_assign_resources(plane, crtc, >>>> + global_state, >>>> + crtc_state->state); >>>> + if (rc) >>>> + return rc; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int dpu_crtc_atomic_check(struct drm_crtc *crtc, >>>> struct drm_atomic_state *state) >>>> { >>>> @@ -1281,7 +1304,6 @@ static int dpu_crtc_atomic_check(struct >>>> drm_crtc *crtc, >>>> struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); >>>> struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); >>>> - const struct drm_plane_state *pstate; >>>> struct drm_plane *plane; >>>> int rc = 0; >>>> @@ -1294,6 +1316,13 @@ static int dpu_crtc_atomic_check(struct >>>> drm_crtc *crtc, >>>> return rc; >>>> } >>>> + if (dpu_use_virtual_planes && >>>> + crtc_state->planes_changed) { >>>> + rc = dpu_crtc_assign_plane_resources(crtc, crtc_state); >>>> + if (rc < 0) >>>> + return rc; >>>> + } >>> >>> Can you please explain a bit more about the planes_changed condition? >>> >>> 1) Are we doing this because the plane's atomic check happens before >>> the CRTC atomic check? >> >> Yes. Another alternative would be to stop using >> drm_atomic_helper_check() and implement our own code instead, >> inverting the plane / CRTC check order. >> > > I am fine with that too but as you noted in (3), if planes_changed will > be set by those functions and you can drop explicit assignment of it in > this patch, that will be the best option for me. > >>> >>> 2) So the DRM core sets this to true already when plane is switching >>> CRTCs or being connected to a CRTC for the first time, we need to >>> handle the conditions additional to that right? >> >> Nit: it is not possible to switch CRTCs. Plane first has to be >> disconnected and then to be connected to another CRTC. >> >>> >>> 3) If (2) is correct, arent there other conditions then to be handled >>> for setting planes_changed to true? >>> >>> Some examples include, switching from a scaling to non-scaling >>> scenario, needing rotation vs not needing etc. >> >> Setting the plane_changed is not required. Both >> drm_atomic_helper_disable_plane() and drm_atomic_helper_update_plane() >> will add the plane to the state. Then drm_atomic_helper_check_planes() >> will call drm_atomic_helper_plane_changed(), setting >> {old_,new_}crtc_state->planes_changed. >> >> I should check if the format check below is required or not. It looks >> like I can drop that clause too. >> > > Yes, please do. From the above explanation, it seems like we dont need > to explicity set it again ourselves for format change. > >> I have a basic doubt about the split between dpu_plane_atomic_check Vs dpu_crtc_atomic_check() because the next patch is also relying heavily on the fact that plane's atomic check comes first and then crtc. Please help me understand this better. -> dpu_plane_virtual_atomic_check() is called and today that doesnt seem to do much : - marks visible as false if there is no fb (wouldnt the DRM core already do this?) - caches the format (this part is used in the dpu_crtc_atomic_check()) -> dpu_crtc_atomic_check() gets called which calls dpu_crtc_assign_plane_resources() which finally reserves the SSPPs to back the planes -> perform dpu_plane_atomic_check() on each of the planes on the CRTC by checking the planes attached to CRTC state 1) What would be wrong to continue using dpu_plane_atomic_check() instead of the newly created dpu_plane_virtual_atomic_check() to get the backing SSPPs because it seems we need to maintain lot of information in the dpu_plane_state to manage co-ordination between the two atomic checks? A big portion of even the next patch does that. 2) Even dpu_plane_atomic_check() / dpu_plane_virtual_atomic_check() is called only for each plane in the CRTC state, so why all the movement to crtc's atomic_check()? I am missing the link here. Was it done only to move all resource allocation to CRTC ID?
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 8ef191fd002d..cdece21b81c9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1273,6 +1273,29 @@ static int dpu_crtc_assign_resources(struct drm_crtc *crtc, struct drm_crtc_stat return 0; } +static int dpu_crtc_assign_plane_resources(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state) +{ + struct dpu_global_state *global_state; + struct drm_plane *plane; + int rc; + + global_state = dpu_kms_get_global_state(crtc_state->state); + if (IS_ERR(global_state)) + return PTR_ERR(global_state); + + dpu_rm_release_all_sspp(global_state, crtc); + + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) { + rc = dpu_plane_virtual_assign_resources(plane, crtc, + global_state, + crtc_state->state); + if (rc) + return rc; + } + + return 0; +} + static int dpu_crtc_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state) { @@ -1281,7 +1304,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); - const struct drm_plane_state *pstate; struct drm_plane *plane; int rc = 0; @@ -1294,6 +1316,13 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, return rc; } + if (dpu_use_virtual_planes && + crtc_state->planes_changed) { + rc = dpu_crtc_assign_plane_resources(crtc, crtc_state); + if (rc < 0) + return rc; + } + if (!crtc_state->enable || !crtc_state->active) { DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip atomic_check\n", crtc->base.id, crtc_state->enable, @@ -1311,20 +1340,30 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, if (cstate->num_mixers) _dpu_crtc_setup_lm_bounds(crtc, crtc_state); - /* FIXME: move this to dpu_plane_atomic_check? */ - drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) { - struct dpu_plane_state *dpu_pstate = to_dpu_plane_state(pstate); - - if (IS_ERR_OR_NULL(pstate)) { - rc = PTR_ERR(pstate); - DPU_ERROR("%s: failed to get plane%d state, %d\n", - dpu_crtc->name, plane->base.id, rc); - return rc; + drm_atomic_crtc_state_for_each_plane(plane, crtc_state) { + const struct drm_plane_state *pstate; + struct dpu_plane_state *dpu_pstate; + + pstate = drm_atomic_get_plane_state(crtc_state->state, plane); + if (IS_ERR(pstate)) + return PTR_ERR(pstate); + + if (dpu_use_virtual_planes) { + /* + * In case of virtual planes, the plane's atomic_check + * is a shortcut. Perform actual check here, after + * allocating SSPPs. + */ + rc = dpu_plane_atomic_check(plane, crtc_state->state); + if (rc) + return rc; } if (!pstate->visible) continue; + /* FIXME: move this to dpu_plane_atomic_check? */ + dpu_pstate = to_dpu_plane_state(pstate); dpu_pstate->needs_dirtyfb = needs_dirtyfb; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 35194262e628..487bb19ee9d6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -50,6 +50,9 @@ #define DPU_DEBUGFS_DIR "msm_dpu" #define DPU_DEBUGFS_HWMASKNAME "hw_log_mask" +bool dpu_use_virtual_planes = false; +module_param(dpu_use_virtual_planes, bool, 0); + static int dpu_kms_hw_init(struct msm_kms *kms); static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms); @@ -735,38 +738,54 @@ static int _dpu_kms_setup_displays(struct drm_device *dev, return rc; } -#define MAX_PLANES 20 -static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) +static int dpu_kms_create_virtual_planes(struct dpu_kms *dpu_kms, + int max_crtc_count, + struct drm_plane **primary_planes, + struct drm_plane **cursor_planes) { - struct drm_device *dev; - struct drm_plane *primary_planes[MAX_PLANES], *plane; - struct drm_plane *cursor_planes[MAX_PLANES] = { NULL }; - struct drm_crtc *crtc; - struct drm_encoder *encoder; - unsigned int num_encoders; + const struct dpu_mdss_cfg *catalog = dpu_kms->catalog; + struct drm_device *dev = dpu_kms->dev; + int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; + struct drm_plane *plane; - struct msm_drm_private *priv; - const struct dpu_mdss_cfg *catalog; + /* Create the planes, keeping track of one primary/cursor per crtc */ + for (i = 0; i < catalog->sspp_count; i++) { + enum drm_plane_type type; - int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; - int max_crtc_count; - dev = dpu_kms->dev; - priv = dev->dev_private; - catalog = dpu_kms->catalog; + if (primary_planes_idx < max_crtc_count) + type = DRM_PLANE_TYPE_PRIMARY; + else if (cursor_planes_idx < max_crtc_count) + type = DRM_PLANE_TYPE_CURSOR; + else + type = DRM_PLANE_TYPE_OVERLAY; - /* - * Create encoder and query display drivers to create - * bridges and connectors - */ - ret = _dpu_kms_setup_displays(dev, priv, dpu_kms); - if (ret) - return ret; + DPU_DEBUG("Create plane type %d\n", type); - num_encoders = 0; - drm_for_each_encoder(encoder, dev) - num_encoders++; + plane = dpu_plane_init_virtual(dev, type, (1UL << max_crtc_count) - 1); + if (IS_ERR(plane)) { + DPU_ERROR("dpu_plane_init failed\n"); + ret = PTR_ERR(plane); + return ret; + } - max_crtc_count = min(catalog->mixer_count, num_encoders); + if (type == DRM_PLANE_TYPE_CURSOR) + cursor_planes[cursor_planes_idx++] = plane; + else if (type == DRM_PLANE_TYPE_PRIMARY) + primary_planes[primary_planes_idx++] = plane; + } + + return primary_planes_idx; +} + +static int dpu_kms_create_planes(struct dpu_kms *dpu_kms, + int max_crtc_count, + struct drm_plane **primary_planes, + struct drm_plane **cursor_planes) +{ + const struct dpu_mdss_cfg *catalog = dpu_kms->catalog; + struct drm_device *dev = dpu_kms->dev; + int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; + struct drm_plane *plane; /* Create the planes, keeping track of one primary/cursor per crtc */ for (i = 0; i < catalog->sspp_count; i++) { @@ -784,8 +803,8 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) type, catalog->sspp[i].features, catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR)); - plane = dpu_plane_init(dev, catalog->sspp[i].id, type, - (1UL << max_crtc_count) - 1); + plane = dpu_plane_init_sspp(dev, catalog->sspp[i].id, type, + (1UL << max_crtc_count) - 1); if (IS_ERR(plane)) { DPU_ERROR("dpu_plane_init failed\n"); ret = PTR_ERR(plane); @@ -798,7 +817,50 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) primary_planes[primary_planes_idx++] = plane; } - max_crtc_count = min(max_crtc_count, primary_planes_idx); + return primary_planes_idx; +} + +#define MAX_PLANES 20 +static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) +{ + struct drm_device *dev; + struct drm_plane *primary_planes[MAX_PLANES]; + struct drm_plane *cursor_planes[MAX_PLANES] = { NULL }; + struct drm_crtc *crtc; + struct drm_encoder *encoder; + unsigned int num_encoders; + + struct msm_drm_private *priv; + const struct dpu_mdss_cfg *catalog; + int i, ret; + int max_crtc_count; + + dev = dpu_kms->dev; + priv = dev->dev_private; + catalog = dpu_kms->catalog; + + /* + * Create encoder and query display drivers to create + * bridges and connectors + */ + ret = _dpu_kms_setup_displays(dev, priv, dpu_kms); + if (ret) + return ret; + + num_encoders = 0; + drm_for_each_encoder(encoder, dev) + num_encoders++; + + max_crtc_count = min(catalog->mixer_count, num_encoders); + + if (dpu_use_virtual_planes) + ret = dpu_kms_create_virtual_planes(dpu_kms, max_crtc_count, primary_planes, cursor_planes); + else + ret = dpu_kms_create_planes(dpu_kms, max_crtc_count, primary_planes, cursor_planes); + if (ret < 0) + return ret; + + max_crtc_count = min(max_crtc_count, ret); /* Create one CRTC per encoder */ for (i = 0; i < max_crtc_count; i++) { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index 934874eb2248..9f6478f0ced6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -65,6 +65,8 @@ #define DPU_NAME_SIZE 12 +extern bool dpu_use_virtual_planes; + struct dpu_kms { struct msm_kms base; struct drm_device *dev; @@ -134,6 +136,8 @@ struct dpu_global_state { uint32_t ctl_to_crtc_id[CTL_MAX - CTL_0]; uint32_t dspp_to_crtc_id[DSPP_MAX - DSPP_0]; uint32_t dsc_to_crtc_id[DSC_MAX - DSC_0]; + + uint32_t sspp_to_crtc_id[SSPP_MAX - SSPP_NONE]; }; struct dpu_global_state diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index cf17075676d5..ee906c276aa5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -837,8 +837,77 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, return 0; } -static int dpu_plane_atomic_check(struct drm_plane *plane, - struct drm_atomic_state *state) +static int dpu_plane_virtual_atomic_check(struct drm_plane *plane, + struct drm_atomic_state *state) +{ + struct drm_plane_state *plane_state = + drm_atomic_get_plane_state(state, plane); + struct dpu_plane_state *pstate = to_dpu_plane_state(plane_state); + const struct dpu_format *format; + struct drm_crtc_state *crtc_state; + + /* + * Main part of checks, including drm_atomic_helper_check_plane_state() + * is called from dpu_crtc_atomic_check(). Do minimal processing here. + */ + + if (!plane_state->fb) { + plane_state->visible = false; + + /* resources are freed by dpu_crtc_atomic_check(), but clean them here */ + pstate->pipe.sspp = NULL; + pstate->r_pipe.sspp = NULL; + + return 0; + } + + format = to_dpu_format(msm_framebuffer_format(plane_state->fb)); + crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc); + + /* force resource reallocation if the format of FB has changed */ + if (pstate->saved_fmt != format) { + crtc_state->planes_changed = true; + pstate->saved_fmt = format; + } + + return 0; +} + +int dpu_plane_virtual_assign_resources(struct drm_plane *plane, + struct drm_crtc *crtc, + struct dpu_global_state *global_state, + struct drm_atomic_state *state) +{ + struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); + struct dpu_plane_state *pstate; + struct drm_plane_state *plane_state; + struct dpu_hw_sspp *hw_sspp; + bool yuv, scale, rot90; + + plane_state = drm_atomic_get_plane_state(state, plane); + if (IS_ERR(plane_state)) + return PTR_ERR(plane_state); + + yuv = plane_state->fb ? + DPU_FORMAT_IS_YUV(to_dpu_format(msm_framebuffer_format(plane_state->fb))) : + false; + scale = (plane_state->src_w >> 16 != plane_state->crtc_w) || + (plane_state->src_h >> 16 != plane_state->crtc_h); + + rot90 = drm_rotation_90_or_270(plane_state->rotation); + + hw_sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, yuv, scale, rot90); + if (!hw_sspp) + return -ENODEV; + + pstate = to_dpu_plane_state(plane_state); + pstate->pipe.sspp = hw_sspp; + + return 0; +} + +int dpu_plane_atomic_check(struct drm_plane *plane, + struct drm_atomic_state *state) { struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane); @@ -863,8 +932,10 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc); - pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); - r_pipe->sspp = NULL; + if (pdpu->pipe != SSPP_NONE) { + pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); + r_pipe->sspp = NULL; + } pipe_hw_caps = pstate->pipe.sspp->cap; sblk = pstate->pipe.sspp->cap->sblk; @@ -1358,12 +1429,14 @@ static void dpu_plane_atomic_print_state(struct drm_printer *p, drm_printf(p, "\tstage=%d\n", pstate->stage); - drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name); - drm_printf(p, "\tmultirect_mode[0]=%s\n", dpu_get_multirect_mode(pipe->multirect_mode)); - drm_printf(p, "\tmultirect_index[0]=%s\n", - dpu_get_multirect_index(pipe->multirect_index)); - drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->src_rect)); - drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->dst_rect)); + if (pipe->sspp) { + drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name); + drm_printf(p, "\tmultirect_mode[0]=%s\n", dpu_get_multirect_mode(pipe->multirect_mode)); + drm_printf(p, "\tmultirect_index[0]=%s\n", + dpu_get_multirect_index(pipe->multirect_index)); + drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->src_rect)); + drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->dst_rect)); + } if (r_pipe->sspp) { drm_printf(p, "\tsspp[1]=%s\n", r_pipe->sspp->cap->name); @@ -1453,18 +1526,30 @@ static const struct drm_plane_helper_funcs dpu_plane_helper_funcs = { .atomic_update = dpu_plane_atomic_update, }; +/* + * For virtual planes atomic_check is called from dpu_crtc_atomic_check(), + * after CRTC code assigning SSPP. + */ +static const struct drm_plane_helper_funcs dpu_plane_virtual_helper_funcs = { + .prepare_fb = dpu_plane_prepare_fb, + .cleanup_fb = dpu_plane_cleanup_fb, + .atomic_check = dpu_plane_virtual_atomic_check, + .atomic_update = dpu_plane_atomic_update, +}; + /* initialize plane */ -struct drm_plane *dpu_plane_init(struct drm_device *dev, - uint32_t pipe, enum drm_plane_type type, - unsigned long possible_crtcs) +static struct drm_plane *dpu_plane_init(struct drm_device *dev, + enum drm_plane_type type, + unsigned long possible_crtcs, + bool inline_rotation, + const uint32_t *format_list, + uint32_t num_formats, + enum dpu_sspp pipe) { struct drm_plane *plane = NULL; - const uint32_t *format_list; struct dpu_plane *pdpu; struct msm_drm_private *priv = dev->dev_private; struct dpu_kms *kms = to_dpu_kms(priv->kms); - struct dpu_hw_sspp *pipe_hw; - uint32_t num_formats; uint32_t supported_rotations; int ret = -EINVAL; @@ -1480,16 +1565,6 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, plane = &pdpu->base; pdpu->pipe = pipe; - /* initialize underlying h/w driver */ - pipe_hw = dpu_rm_get_sspp(&kms->rm, pipe); - if (!pipe_hw || !pipe_hw->cap || !pipe_hw->cap->sblk) { - DPU_ERROR("[%u]SSPP is invalid\n", pipe); - goto clean_plane; - } - - format_list = pipe_hw->cap->sblk->format_list; - num_formats = pipe_hw->cap->sblk->num_formats; - ret = drm_universal_plane_init(dev, plane, 0xff, &dpu_plane_funcs, format_list, num_formats, supported_format_modifiers, type, NULL); @@ -1510,7 +1585,7 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_180; - if (pipe_hw->cap->features & BIT(DPU_SSPP_INLINE_ROTATION)) + if (inline_rotation) supported_rotations |= DRM_MODE_ROTATE_MASK; drm_plane_create_rotation_property(plane, @@ -1519,8 +1594,6 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, drm_plane_enable_fb_damage_clips(plane); /* success! finalize initialization */ - drm_plane_helper_add(plane, &dpu_plane_helper_funcs); - mutex_init(&pdpu->lock); DPU_DEBUG("%s created for pipe:%u id:%u\n", plane->name, @@ -1531,3 +1604,59 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, kfree(pdpu); return ERR_PTR(ret); } + +struct drm_plane *dpu_plane_init_sspp(struct drm_device *dev, + uint32_t pipe, enum drm_plane_type type, + unsigned long possible_crtcs) +{ + struct drm_plane *plane = NULL; + struct msm_drm_private *priv = dev->dev_private; + struct dpu_kms *kms = to_dpu_kms(priv->kms); + struct dpu_hw_sspp *pipe_hw; + + /* initialize underlying h/w driver */ + pipe_hw = dpu_rm_get_sspp(&kms->rm, pipe); + if (!pipe_hw || !pipe_hw->cap || !pipe_hw->cap->sblk) { + DPU_ERROR("[%u]SSPP is invalid\n", pipe); + return ERR_PTR(-EINVAL); + } + + + plane = dpu_plane_init(dev, type, possible_crtcs, + pipe_hw->cap->features & BIT(DPU_SSPP_INLINE_ROTATION), + pipe_hw->cap->sblk->format_list, + pipe_hw->cap->sblk->num_formats, + pipe); + if (IS_ERR(plane)) + return plane; + + drm_plane_helper_add(plane, &dpu_plane_helper_funcs); + + DPU_DEBUG("%s created for pipe:%u id:%u\n", plane->name, + pipe, plane->base.id); + + return plane; +} + +struct drm_plane *dpu_plane_init_virtual(struct drm_device *dev, + enum drm_plane_type type, + unsigned long possible_crtcs) +{ + struct drm_plane *plane = NULL; + struct msm_drm_private *priv = dev->dev_private; + struct dpu_kms *kms = to_dpu_kms(priv->kms); + + plane = dpu_plane_init(dev, type, possible_crtcs, + kms->catalog->caps->has_inline_rot, + kms->catalog->caps->format_list, + kms->catalog->caps->num_formats, + SSPP_NONE); + if (IS_ERR(plane)) + return plane; + + drm_plane_helper_add(plane, &dpu_plane_virtual_helper_funcs); + + DPU_DEBUG("%s created virtual id:%u\n", plane->name, plane->base.id); + + return plane; +} diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h index abd6b21a049b..cb1e31ef0d3f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h @@ -31,6 +31,7 @@ * @plane_clk: calculated clk per plane * @needs_dirtyfb: whether attached CRTC needs pixel data explicitly flushed * @rotation: simplified drm rotation hint + * @saved_fmt: format used by the plane's FB, saved for for virtual plane support */ struct dpu_plane_state { struct drm_plane_state base; @@ -48,6 +49,8 @@ struct dpu_plane_state { bool needs_dirtyfb; unsigned int rotation; + + const struct dpu_format *saved_fmt; }; #define to_dpu_plane_state(x) \ @@ -66,17 +69,27 @@ void dpu_plane_flush(struct drm_plane *plane); void dpu_plane_set_error(struct drm_plane *plane, bool error); /** - * dpu_plane_init - create new dpu plane for the given pipe + * dpu_plane_init_sspp - create new dpu plane for the given pipe * @dev: Pointer to DRM device * @pipe: dpu hardware pipe identifier * @type: Plane type - PRIMARY/OVERLAY/CURSOR * @possible_crtcs: bitmask of crtc that can be attached to the given pipe * */ -struct drm_plane *dpu_plane_init(struct drm_device *dev, +struct drm_plane *dpu_plane_init_sspp(struct drm_device *dev, uint32_t pipe, enum drm_plane_type type, unsigned long possible_crtcs); +/** + * dpu_plane_init_virtual - create new dpu virtualized plane + * @dev: Pointer to DRM device + * @type: Plane type - PRIMARY/OVERLAY/CURSOR + * @possible_crtcs: bitmask of crtc that can be attached to the given pipe + */ +struct drm_plane *dpu_plane_init_virtual(struct drm_device *dev, + enum drm_plane_type type, + unsigned long possible_crtcs); + /** * dpu_plane_color_fill - enables color fill on plane * @plane: Pointer to DRM plane object @@ -93,4 +106,11 @@ void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable); static inline void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable) {} #endif +int dpu_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state); + +int dpu_plane_virtual_assign_resources(struct drm_plane *plane, + struct drm_crtc *crtc, + struct dpu_global_state *global_state, + struct drm_atomic_state *state); + #endif /* _DPU_PLANE_H_ */ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c index f0a94008d17a..6130ac87d7e3 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c @@ -607,6 +607,71 @@ int dpu_rm_reserve( return ret; } +struct dpu_hw_sspp *dpu_rm_reserve_sspp(struct dpu_rm *rm, + struct dpu_global_state *global_state, + struct drm_crtc *crtc, + bool yuv, bool scale, bool rot90) +{ + uint32_t crtc_id = crtc->base.id; + struct dpu_hw_sspp *hw_sspp; + bool retry = false; + int i; + +retry_loop: + for (i = 0; i < ARRAY_SIZE(rm->hw_sspp); i++) { + if (!rm->hw_sspp[i]) + continue; + + if (global_state->sspp_to_crtc_id[i]) + continue; + + hw_sspp = rm->hw_sspp[i]; + + /* skip incompatible planes */ + if (scale && !(hw_sspp->cap->features & DPU_SSPP_SCALER)) + continue; + + if (yuv && !(hw_sspp->cap->features & DPU_SSPP_CSC_ANY)) + continue; + + if (rot90 && !(hw_sspp->cap->features & DPU_SSPP_INLINE_ROTATION)) + continue; + + /* + * For non-yuv, non-scaled planes try to find simple (DMA) + * plane, fallback to VIG on a second try. + * + * This way we'd leave VIG sspps to be later used for YUV formats. + */ + + if (!scale && !yuv && !rot90 && !retry && + (hw_sspp->cap->features & + (DPU_SSPP_SCALER | DPU_SSPP_CSC_ANY | DPU_SSPP_INLINE_ROTATION))) + continue; + + global_state->sspp_to_crtc_id[hw_sspp->idx - SSPP_NONE] = crtc_id; + + return hw_sspp; + } + + /* If we were looking for DMA plane, retry looking for VIG plane */ + if (!scale && !yuv && !retry) { + retry = true; + goto retry_loop; + } + + return NULL; +} + +void dpu_rm_release_all_sspp(struct dpu_global_state *global_state, + struct drm_crtc *crtc) +{ + uint32_t crtc_id = crtc->base.id; + + _dpu_rm_clear_mapping(global_state->sspp_to_crtc_id, + ARRAY_SIZE(global_state->sspp_to_crtc_id), crtc_id); +} + int dpu_rm_get_assigned_resources(struct dpu_rm *rm, struct dpu_global_state *global_state, struct drm_crtc *crtc, enum dpu_hw_blk_type type, struct dpu_hw_blk **blks, int blks_size) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h index f402bec8322b..5bf6740ecb45 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h @@ -82,6 +82,30 @@ int dpu_rm_reserve( void dpu_rm_release(struct dpu_global_state *global_state, struct drm_crtc *crtc); +/** + * dpu_rm_reserve_sspp - Reserve the required SSPP for the provided CRTC + * @rm: DPU Resource Manager handle + * @global_state: private global state + * @crtc: DRM CRTC handle + * @yuv: required SSPP supporting YUV formats + * @scale: required SSPP supporting scaling + * @rot90: required SSPP supporting inline 90 degree rotation + */ +struct dpu_hw_sspp *dpu_rm_reserve_sspp(struct dpu_rm *rm, + struct dpu_global_state *global_state, + struct drm_crtc *crtc, + bool yuv, bool scale, bool rot90); + +/** + * dpu_rm_release_all_sspp - Given the CRTC, release all SSPP + * blocks previously reserved for that use case. + * @rm: DPU Resource Manager handle + * @crtc: DRM CRTC handle + * @Return: 0 on Success otherwise -ERROR + */ +void dpu_rm_release_all_sspp(struct dpu_global_state *global_state, + struct drm_crtc *crtc); + /** * Get hw resources of the given type that are assigned to this encoder. */
Only several SSPP blocks support such features as YUV output or scaling, thus different DRM planes have different features. Properly utilizing all planes requires the attention of the compositor, who should prefer simpler planes to YUV-supporting ones. Otherwise it is very easy to end up in a situation when all featureful planes are already allocated for simple windows, leaving no spare plane for YUV playback. To solve this problem make all planes virtual. Each plane is registered as if it supports all possible features, but then at the runtime during the atomic_check phase the driver selects backing SSPP block for each plane. Note, this does not provide support for using two different SSPP blocks for a single plane or using two rectangles of an SSPP to drive two planes. Each plane still gets its own SSPP and can utilize either a solo rectangle or both multirect rectangles depending on the resolution. Note #2: By default support for virtual planes is turned off and the driver still uses old code path with preallocated SSPP block for each plane. To enable virtual planes, pass 'msm.dpu_use_virtual_planes=1' kernel parameter. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 59 +++++-- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 120 ++++++++++---- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 + drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 187 ++++++++++++++++++---- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 24 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 65 ++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 24 +++ 7 files changed, 413 insertions(+), 70 deletions(-)