[RFC PATCH 1/3] drm/vkms: decouple cursor plane setup from crtc_init

[Daniel Vetter]

On Tue, Feb 23, 2021 at 10:42 AM Melissa Wen <melissa.srw at 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 at 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