On Thu, Aug 02, 2018 at 04:24:44PM +0530, Sravanthi Kollukuduru wrote: > Reserve DMA pipe for cursor plane and attach it to the > crtc during the initialization. > > Signed-off-by: Sravanthi Kollukuduru <skolluku@xxxxxxxxxxxxxx> Hi Sravanthi, Thanks for sending this. I have a few comments, mostly based off my own cursor patch [1] I posted last week. It's mostly the same as what you have here, but takes a couple different things into consideration. [1]- https://gitlab.freedesktop.org/seanpaul/dpu-staging/commit/fde9f11f8f3be3ceaacfd0751e250a3c03476a8c > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 5 ++- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 53 +++++++++++--------------- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 35 +++++++++++------ > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 9 +---- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 4 +- > 6 files changed, 55 insertions(+), 55 deletions(-) > /snip > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 8d4678d..dc647ee 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -531,12 +531,13 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) > { > 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 msm_drm_private *priv; > struct dpu_mdss_cfg *catalog; > > - int primary_planes_idx = 0, i, ret; > + int primary_planes_idx = 0, cursor_planes_idx = 0, i, ret; > int max_crtc_count; > > if (!dpu_kms || !dpu_kms->dev || !dpu_kms->dev->dev) { > @@ -556,16 +557,24 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) > > max_crtc_count = min(catalog->mixer_count, priv->num_encoders); > > - /* Create the planes */ > + /* Create the planes, keeping track of one primary/cursor per crtc */ > for (i = 0; i < catalog->sspp_count; i++) { > - bool primary = true; > - > - if (catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR) > - || primary_planes_idx >= max_crtc_count) > - primary = false; > - > - plane = dpu_plane_init(dev, catalog->sspp[i].id, primary, > - (1UL << max_crtc_count) - 1, 0); > + enum drm_plane_type type; > + > + if ((catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR)) > + && cursor_planes_idx < max_crtc_count) You don't need to check that the index is less than max_crtc_count, it'll fit in the array regardless. > + type = DRM_PLANE_TYPE_CURSOR; > + else if (primary_planes_idx < max_crtc_count) > + type = DRM_PLANE_TYPE_PRIMARY; > + else > + type = DRM_PLANE_TYPE_OVERLAY; > + > + DPU_DEBUG("Create plane type %d with features %lx (cur %lx)\n", > + 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, 0); > if (IS_ERR(plane)) { > DPU_ERROR("dpu_plane_init failed\n"); > ret = PTR_ERR(plane); > @@ -573,7 +582,9 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) > } > priv->planes[priv->num_planes++] = plane; > > - if (primary) > + 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; > } > > @@ -581,7 +592,7 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms) > > /* Create one CRTC per encoder */ > for (i = 0; i < max_crtc_count; i++) { > - crtc = dpu_crtc_init(dev, primary_planes[i]); > + crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]); Here you assume that there is at least one cursor per crtc. That might not be the case. Check out how I handle this in my patch with max_cursor_planes, it's a bit more safe than this. > if (IS_ERR(crtc)) { > ret = PTR_ERR(crtc); > goto fail; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index b640e39..efdf9b2 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -1827,7 +1827,7 @@ bool is_dpu_plane_virtual(struct drm_plane *plane) > > /* initialize plane */ > struct drm_plane *dpu_plane_init(struct drm_device *dev, > - uint32_t pipe, bool primary_plane, > + uint32_t pipe, enum drm_plane_type type, > unsigned long possible_crtcs, u32 master_plane_id) > { > struct drm_plane *plane = NULL, *master_plane = NULL; > @@ -1835,7 +1835,6 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, > struct dpu_plane *pdpu; > struct msm_drm_private *priv; > struct dpu_kms *kms; > - enum drm_plane_type type; > int zpos_max = DPU_ZPOS_MAX; > int ret = -EINVAL; > > @@ -1916,12 +1915,6 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, > goto clean_sspp; > } > > - if (pdpu->features & BIT(DPU_SSPP_CURSOR)) > - type = DRM_PLANE_TYPE_CURSOR; > - else if (primary_plane) > - type = DRM_PLANE_TYPE_PRIMARY; > - else > - type = DRM_PLANE_TYPE_OVERLAY; > ret = drm_universal_plane_init(dev, plane, 0xff, &dpu_plane_funcs, > pdpu->formats, pdpu->nformats, > NULL, type, NULL); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > index f6fe6dd..7fed0b6 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > @@ -122,7 +122,7 @@ void dpu_plane_get_ctl_flush(struct drm_plane *plane, struct dpu_hw_ctl *ctl, > * dpu_plane_init - create new dpu plane for the given pipe > * @dev: Pointer to DRM device > * @pipe: dpu hardware pipe identifier > - * @primary_plane: true if this pipe is primary plane for crtc > + * @type: Plane type - PRIMARY/OVERLAY/CURSOR > * @possible_crtcs: bitmask of crtc that can be attached to the given pipe > * @master_plane_id: primary plane id of a multirect pipe. 0 value passed for > * a regular plane initialization. A non-zero primary plane > @@ -130,7 +130,7 @@ void dpu_plane_get_ctl_flush(struct drm_plane *plane, struct dpu_hw_ctl *ctl, > * > */ > struct drm_plane *dpu_plane_init(struct drm_device *dev, > - uint32_t pipe, bool primary_plane, > + uint32_t pipe, enum drm_plane_type type, > unsigned long possible_crtcs, u32 master_plane_id); > > /** > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Sean Paul, Software Engineer, Google / Chromium OS -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
