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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <62ca5d60-1b4b-d102-1a5e-50288dae4c37@chromium.org>
Date:   Fri, 18 Aug 2023 14:26:25 +0900
From:   Brandon Ross Pollack <brpol@...omium.org>
To:     marius.vlad@...labora.com
Cc:     corbet@....net, dri-devel@...ts.freedesktop.org,
        hamohammed.sa@...il.com, jshargo@...omium.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        mairacanal@...eup.net, melissa.srw@...il.com, mripard@...nel.org,
        rodrigosiqueiramelo@...il.com, tzimmermann@...e.de
Subject: Re: [PATCH v2 2/6] drm/vkms: Support multiple DRM objects (crtcs,
 etc.) per VKMS device

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 :)