* [RFC PATCH 1/3] drm/vkms: decouple cursor plane setup from crtc_init
@ 2021-02-20 14:38 Melissa Wen
2021-02-22 16:38 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Melissa Wen @ 2021-02-20 14:38 UTC (permalink / raw)
To: Rodrigo Siqueira, Haneen Mohammed, Daniel Vetter, Simon Ser,
Sumera Priyadarsini, David Airlie
Cc: dri-devel
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(-)
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH 1/3] drm/vkms: decouple cursor plane setup from crtc_init
2021-02-20 14:38 [RFC PATCH 1/3] drm/vkms: decouple cursor plane setup from crtc_init Melissa Wen
@ 2021-02-22 16:38 ` Daniel Vetter
2021-02-23 9:42 ` Melissa Wen
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2021-02-22 16:38 UTC (permalink / raw)
To: Melissa Wen
Cc: Haneen Mohammed, Sumera Priyadarsini, Rodrigo Siqueira,
David Airlie, dri-devel
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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH 1/3] drm/vkms: decouple cursor plane setup from crtc_init
2021-02-22 16:38 ` Daniel Vetter
@ 2021-02-23 9:42 ` Melissa Wen
2021-02-23 10:12 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Melissa Wen @ 2021-02-23 9:42 UTC (permalink / raw)
To: Daniel Vetter
Cc: Haneen Mohammed, Sumera Priyadarsini, Rodrigo Siqueira,
David Airlie, dri-devel
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH 1/3] drm/vkms: decouple cursor plane setup from crtc_init
2021-02-23 9:42 ` Melissa Wen
@ 2021-02-23 10:12 ` Daniel Vetter
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2021-02-23 10:12 UTC (permalink / raw)
To: Melissa Wen
Cc: Haneen Mohammed, Sumera Priyadarsini, Rodrigo Siqueira,
David Airlie, dri-devel
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-23 10:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-20 14:38 [RFC PATCH 1/3] drm/vkms: decouple cursor plane setup from crtc_init Melissa Wen
2021-02-22 16:38 ` Daniel Vetter
2021-02-23 9:42 ` Melissa Wen
2021-02-23 10:12 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).