| Message ID | 20210220143850.k22n4r4uel5zhxr2@smtp.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: add overlay plane support | expand |
On Sat, Feb 20, 2021 at 11:38:50AM -0300, Melissa Wen wrote: > Initialize CRTC only with primary plane (without cursor) as a preparation > to init overlay plane before cursor plane and keep cursor on the top. > > Signed-off-by: Melissa Wen <melissa.srw@gmail.com> Why can't we first initialize all the planes (primary, cursor, overlay) and then the crtc? For drivers where there's not really a functional difference between these planes (like vkms, since it's all sw, only difference is z position really) the usual approach is to initialize all planes in a loop. And then call crtc init with the first and last plane for that crtc. Passing NULL for the cursor for crtc_init and then patching it up afterwards is a bit a hack, so would be good to avoid that. -Daniel > --- > drivers/gpu/drm/vkms/vkms_crtc.c | 4 ++-- > drivers/gpu/drm/vkms/vkms_drv.h | 2 +- > drivers/gpu/drm/vkms/vkms_output.c | 14 +++++++++----- > 3 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c > index 0443b7deeaef..2d4cd7736933 100644 > --- a/drivers/gpu/drm/vkms/vkms_crtc.c > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c > @@ -270,12 +270,12 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = { > }; > > int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, > - struct drm_plane *primary, struct drm_plane *cursor) > + struct drm_plane *primary) > { > struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc); > int ret; > > - ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor, > + ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL, > &vkms_crtc_funcs, NULL); > if (ret) { > DRM_ERROR("Failed to init CRTC\n"); > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index 35540c7c4416..9ad5ad8b7737 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -110,7 +110,7 @@ struct vkms_device { > > /* CRTC */ > int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, > - struct drm_plane *primary, struct drm_plane *cursor); > + struct drm_plane *primary); > > int vkms_output_init(struct vkms_device *vkmsdev, int index); > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c > index f5f6f15c362c..05d3bb45e6c1 100644 > --- a/drivers/gpu/drm/vkms/vkms_output.c > +++ b/drivers/gpu/drm/vkms/vkms_output.c > @@ -47,6 +47,10 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > if (IS_ERR(primary)) > return PTR_ERR(primary); > > + ret = vkms_crtc_init(dev, crtc, primary); > + if (ret) > + goto err_crtc; > + > if (vkmsdev->config->cursor) { > cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index); > if (IS_ERR(cursor)) { > @@ -55,9 +59,9 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > } > } > > - ret = vkms_crtc_init(dev, crtc, primary, cursor); > - if (ret) > - goto err_crtc; > + crtc->cursor = cursor; > + if (cursor && !cursor->possible_crtcs) > + cursor->possible_crtcs = drm_crtc_mask(crtc); > > ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > DRM_MODE_CONNECTOR_VIRTUAL); > @@ -100,11 +104,11 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > err_connector: > drm_crtc_cleanup(crtc); > > -err_crtc: > +err_cursor: > if (vkmsdev->config->cursor) > drm_plane_cleanup(cursor); > > -err_cursor: > +err_crtc: > drm_plane_cleanup(primary); > > return ret; > -- > 2.30.0 >
On 02/22, Daniel Vetter wrote: > On Sat, Feb 20, 2021 at 11:38:50AM -0300, Melissa Wen wrote: > > Initialize CRTC only with primary plane (without cursor) as a preparation > > to init overlay plane before cursor plane and keep cursor on the top. > > > > Signed-off-by: Melissa Wen <melissa.srw@gmail.com> > > Why can't we first initialize all the planes (primary, cursor, overlay) > and then the crtc? > > For drivers where there's not really a functional difference between these > planes (like vkms, since it's all sw, only difference is z position > really) the usual approach is to initialize all planes in a loop. And then > call crtc init with the first and last plane for that crtc. > Hi Daniel, It was a misundertanding from my side. I thought that, besides to initialize the planes, setting the possible_crtcs should also be done in order. Thanks for the feeback. I will discard this patch from the series. Melissa > Passing NULL for the cursor for crtc_init and then patching it up > afterwards is a bit a hack, so would be good to avoid that. > -Daniel > > > --- > > drivers/gpu/drm/vkms/vkms_crtc.c | 4 ++-- > > drivers/gpu/drm/vkms/vkms_drv.h | 2 +- > > drivers/gpu/drm/vkms/vkms_output.c | 14 +++++++++----- > > 3 files changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c > > index 0443b7deeaef..2d4cd7736933 100644 > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c > > @@ -270,12 +270,12 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = { > > }; > > > > int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, > > - struct drm_plane *primary, struct drm_plane *cursor) > > + struct drm_plane *primary) > > { > > struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc); > > int ret; > > > > - ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor, > > + ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL, > > &vkms_crtc_funcs, NULL); > > if (ret) { > > DRM_ERROR("Failed to init CRTC\n"); > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > > index 35540c7c4416..9ad5ad8b7737 100644 > > --- a/drivers/gpu/drm/vkms/vkms_drv.h > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > > @@ -110,7 +110,7 @@ struct vkms_device { > > > > /* CRTC */ > > int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, > > - struct drm_plane *primary, struct drm_plane *cursor); > > + struct drm_plane *primary); > > > > int vkms_output_init(struct vkms_device *vkmsdev, int index); > > > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c > > index f5f6f15c362c..05d3bb45e6c1 100644 > > --- a/drivers/gpu/drm/vkms/vkms_output.c > > +++ b/drivers/gpu/drm/vkms/vkms_output.c > > @@ -47,6 +47,10 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > if (IS_ERR(primary)) > > return PTR_ERR(primary); > > > > + ret = vkms_crtc_init(dev, crtc, primary); > > + if (ret) > > + goto err_crtc; > > + > > if (vkmsdev->config->cursor) { > > cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index); > > if (IS_ERR(cursor)) { > > @@ -55,9 +59,9 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > } > > } > > > > - ret = vkms_crtc_init(dev, crtc, primary, cursor); > > - if (ret) > > - goto err_crtc; > > + crtc->cursor = cursor; > > + if (cursor && !cursor->possible_crtcs) > > + cursor->possible_crtcs = drm_crtc_mask(crtc); > > > > ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > > DRM_MODE_CONNECTOR_VIRTUAL); > > @@ -100,11 +104,11 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > err_connector: > > drm_crtc_cleanup(crtc); > > > > -err_crtc: > > +err_cursor: > > if (vkmsdev->config->cursor) > > drm_plane_cleanup(cursor); > > > > -err_cursor: > > +err_crtc: > > drm_plane_cleanup(primary); > > > > return ret; > > -- > > 2.30.0 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Tue, Feb 23, 2021 at 10:42 AM Melissa Wen <melissa.srw@gmail.com> wrote: > > On 02/22, Daniel Vetter wrote: > > On Sat, Feb 20, 2021 at 11:38:50AM -0300, Melissa Wen wrote: > > > Initialize CRTC only with primary plane (without cursor) as a preparation > > > to init overlay plane before cursor plane and keep cursor on the top. > > > > > > Signed-off-by: Melissa Wen <melissa.srw@gmail.com> > > > > Why can't we first initialize all the planes (primary, cursor, overlay) > > and then the crtc? > > > > For drivers where there's not really a functional difference between these > > planes (like vkms, since it's all sw, only difference is z position > > really) the usual approach is to initialize all planes in a loop. And then > > call crtc init with the first and last plane for that crtc. > > > Hi Daniel, > > It was a misundertanding from my side. I thought that, besides to > initialize the planes, setting the possible_crtcs should also be done in > order. Yeah possible_crtcs is a bit fun. Since most drivers set up their crtcs in the order they are numbered in hw registers, and the possible_crtcs mask is index based (not based on the kms object id userspace sees), you can set it before you actually set up the crtc. Or (and this is what i915 does for encoders iirc) you can do a special function which computes the possible_crtcs mask after everything is set up, but before you call drm_dev_register - before that point no one else can see any of this anyone, so no problem if it's inconsistent. But that's rather convoluted way of doing things. So yeah for the future when vkms supports multiple output, we can just pass the index of the output we're creating to that function, and that can be used for possible_crtcs. And as long as we set up all outputs in order, that will then match up with the drm_crtc_index() of the crtc. Fundamentally it's a bit chicken/egg problem, so always a bit confusing. -Daniel > Thanks for the feeback. I will discard this patch from the series. > > Melissa > > > Passing NULL for the cursor for crtc_init and then patching it up > > afterwards is a bit a hack, so would be good to avoid that. > > -Daniel > > > > > --- > > > drivers/gpu/drm/vkms/vkms_crtc.c | 4 ++-- > > > drivers/gpu/drm/vkms/vkms_drv.h | 2 +- > > > drivers/gpu/drm/vkms/vkms_output.c | 14 +++++++++----- > > > 3 files changed, 12 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c > > > index 0443b7deeaef..2d4cd7736933 100644 > > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c > > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c > > > @@ -270,12 +270,12 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = { > > > }; > > > > > > int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, > > > - struct drm_plane *primary, struct drm_plane *cursor) > > > + struct drm_plane *primary) > > > { > > > struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc); > > > int ret; > > > > > > - ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor, > > > + ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL, > > > &vkms_crtc_funcs, NULL); > > > if (ret) { > > > DRM_ERROR("Failed to init CRTC\n"); > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > > > index 35540c7c4416..9ad5ad8b7737 100644 > > > --- a/drivers/gpu/drm/vkms/vkms_drv.h > > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > > > @@ -110,7 +110,7 @@ struct vkms_device { > > > > > > /* CRTC */ > > > int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, > > > - struct drm_plane *primary, struct drm_plane *cursor); > > > + struct drm_plane *primary); > > > > > > int vkms_output_init(struct vkms_device *vkmsdev, int index); > > > > > > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c > > > index f5f6f15c362c..05d3bb45e6c1 100644 > > > --- a/drivers/gpu/drm/vkms/vkms_output.c > > > +++ b/drivers/gpu/drm/vkms/vkms_output.c > > > @@ -47,6 +47,10 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > > if (IS_ERR(primary)) > > > return PTR_ERR(primary); > > > > > > + ret = vkms_crtc_init(dev, crtc, primary); > > > + if (ret) > > > + goto err_crtc; > > > + > > > if (vkmsdev->config->cursor) { > > > cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index); > > > if (IS_ERR(cursor)) { > > > @@ -55,9 +59,9 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > > } > > > } > > > > > > - ret = vkms_crtc_init(dev, crtc, primary, cursor); > > > - if (ret) > > > - goto err_crtc; > > > + crtc->cursor = cursor; > > > + if (cursor && !cursor->possible_crtcs) > > > + cursor->possible_crtcs = drm_crtc_mask(crtc); > > > > > > ret = drm_connector_init(dev, connector, &vkms_connector_funcs, > > > DRM_MODE_CONNECTOR_VIRTUAL); > > > @@ -100,11 +104,11 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) > > > err_connector: > > > drm_crtc_cleanup(crtc); > > > > > > -err_crtc: > > > +err_cursor: > > > if (vkmsdev->config->cursor) > > > drm_plane_cleanup(cursor); > > > > > > -err_cursor: > > > +err_crtc: > > > drm_plane_cleanup(primary); > > > > > > return ret; > > > -- > > > 2.30.0 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index 0443b7deeaef..2d4cd7736933 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -270,12 +270,12 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = { }; int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, - struct drm_plane *primary, struct drm_plane *cursor) + struct drm_plane *primary) { struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc); int ret; - ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor, + ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL, &vkms_crtc_funcs, NULL); if (ret) { DRM_ERROR("Failed to init CRTC\n"); diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 35540c7c4416..9ad5ad8b7737 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -110,7 +110,7 @@ struct vkms_device { /* CRTC */ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, - struct drm_plane *primary, struct drm_plane *cursor); + struct drm_plane *primary); int vkms_output_init(struct vkms_device *vkmsdev, int index); diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c index f5f6f15c362c..05d3bb45e6c1 100644 --- a/drivers/gpu/drm/vkms/vkms_output.c +++ b/drivers/gpu/drm/vkms/vkms_output.c @@ -47,6 +47,10 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) if (IS_ERR(primary)) return PTR_ERR(primary); + ret = vkms_crtc_init(dev, crtc, primary); + if (ret) + goto err_crtc; + if (vkmsdev->config->cursor) { cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index); if (IS_ERR(cursor)) { @@ -55,9 +59,9 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) } } - ret = vkms_crtc_init(dev, crtc, primary, cursor); - if (ret) - goto err_crtc; + crtc->cursor = cursor; + if (cursor && !cursor->possible_crtcs) + cursor->possible_crtcs = drm_crtc_mask(crtc); ret = drm_connector_init(dev, connector, &vkms_connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL); @@ -100,11 +104,11 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index) err_connector: drm_crtc_cleanup(crtc); -err_crtc: +err_cursor: if (vkmsdev->config->cursor) drm_plane_cleanup(cursor); -err_cursor: +err_crtc: drm_plane_cleanup(primary); return ret;
Initialize CRTC only with primary plane (without cursor) as a preparation to init overlay plane before cursor plane and keep cursor on the top. Signed-off-by: Melissa Wen <melissa.srw@gmail.com> --- drivers/gpu/drm/vkms/vkms_crtc.c | 4 ++-- drivers/gpu/drm/vkms/vkms_drv.h | 2 +- drivers/gpu/drm/vkms/vkms_output.c | 14 +++++++++----- 3 files changed, 12 insertions(+), 8 deletions(-)