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

[Melissa Wen]

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.

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