Re: [PATCH v2 2/6] drm/vkms: Support multiple DRM objects (crtcs, etc.) per VKMS device

[Brandon Ross Pollack]

Silly addition but I need to apologize for mispelling somone's name in 
my reply.

Maira, please accept my sincere apology.

On 8/18/23 12:36, Brandon Pollack wrote:
> Thanks for taking the time, everyone! Sorry it took so long, we had some
> internal shuffling etc going on and I was building out what we needed these
> chagnes for in the first place, this will be the first of a few replies
> followed by a new version of the series to be sent out.
>
> First up is a respons to Maria, Marius to follow.
>
> ---
>
> Maria,
>
> > -	if (vkms->output.composer_workq)
> > -		destroy_workqueue(vkms->output.composer_workq);
> > +	for (int i = 0; i < vkms->output.num_crtcs; i++)
> > +		destroy_workqueue(vkms->output.crtcs[i].composer_workq);
> I don't believe there is any need for a null check.  If you look in the
> crtc_init, it is zero'd before returning any errors and that is the only place
> it is set.  I don't believe that release can be called by an interrupt/async
> (and if it did it would need a mutex/lock anyway).
>
> >    static const struct drm_plane_funcs vkms_plane_funcs = {
> > -	.update_plane		= drm_atomic_helper_update_plane,
> > -	.disable_plane		= drm_atomic_helper_disable_plane,
> > -	.reset			= vkms_plane_reset,
> Yeah these do seem weirdly formatted on devices that don't treat tabs well.
> The default formatter on my editor has a few suggestions for this file, but
> they are all optional.  I'll send an extra patch that formats stuff and see
> what people think, but ill make it seperate after all this is done.
> For now I reverted this.
>
> > > -	if (IS_ERR(plane))
> > > -		return plane;
> > > +	if (output->num_planes >= VKMS_MAX_PLANES)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	plane = &output->planes[output->num_planes++];
> > > +	ret = drm_universal_plane_init(dev, &plane->base, 0, &vkms_plane_funcs,
> > > +				       vkms_formats, ARRAY_SIZE(vkms_formats),
> > > +				       NULL, type, NULL);
> > Wouldn't be possible to use drmm_universal_plane_alloc?
> Maybe, but the *_init pattern allows these things to be inline in the struct as
> they are now, and consistent with the other drm kernel objects in the
> vkms_output struct.  There are also a few other places we could use drmm,
> surely, but to limit the scope/risk why don't we do that as a followup?
>
> ---
>
> Marius,
>
> Yeah those values could safely be completely removed.  Good catch :)