[RFC PATCH v2 12/13] drm/msm/dpu: add support for virtual planes
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Wed Jun 7 21:56:21 UTC 2023
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 at 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.
>> */
--
With best wishes
Dmitry
More information about the dri-devel
mailing list