From:   Javier Martinez Canillas <javierm@redhat.com>
To:     Thomas Zimmermann <tzimmermann@suse.de>,
        Gerd Hoffmann <kraxel@redhat.com>,
        Rob Clark <robdclark@gmail.com>
Cc:     Rob Clark <robdclark@chromium.org>,
        Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        open list <linux-kernel@vger.kernel.org>,
        dri-devel@lists.freedesktop.org,
        Gurchetan Singh <gurchetansingh@chromium.org>,
        Ryan Neph <ryanneph@chromium.org>,
        David Airlie <airlied@redhat.com>,
        "open list:VIRTIO GPU DRIVER" 
        <virtualization@lists.linux-foundation.org>
Subject: Re: [PATCH] drm/virtio: Add option to disable KMS support
In-Reply-To: <9631930e-3826-ded7-1a45-1d0a285c5195@suse.de>
References: <20230224180225.2477641-1-robdclark@gmail.com>
 <20230227063821.dg2gbjjwcekbxyzw@sirius.home.kraxel.org>
 <CAF6AEGsv1G7CPSkCPe3iHGB9JEO4iy+bTbkFLoitmx64U78RJw@mail.gmail.com>
 <20230228062809.ccyzgnvizh6jidn4@sirius.home.kraxel.org>
 <87a60yywo0.fsf@minerva.mail-host-address-is-not-set>
 <9631930e-3826-ded7-1a45-1d0a285c5195@suse.de>
Date:   Tue, 28 Feb 2023 12:58:36 +0100
Message-ID: <87pm9uxaqb.fsf@minerva.mail-host-address-is-not-set>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>
> Am 28.02.23 um 10:19 schrieb Javier Martinez Canillas:
>> Gerd Hoffmann <kraxel@redhat.com> writes:
> [...]
>>>
>>> I think it is a bad idea to make that a compile time option, I'd suggest
>>> a runtime switch instead, for example a module parameter to ask the
>>> driver to ignore any scanouts.
>>>
>> 
>> I don't think there's a need for a new module parameter, there's already
>> the virtio-gpu 'modeset' module parameter to enable/disable modsetting
>> and the global 'nomodeset' kernel cmdline parameter to do it for all DRM
>> drivers.
>> 
>> Currently, many drivers just fail to probe when 'nomodeset' is present,
>> but others only disable modsetting but keep the rendering part. In fact,
>> most DRM only drivers just ignore the 'nomodeset' parameter.
>
> Do you have a list of these drivers? Maybe we need to adjust semantics 
> slightly. Please see my comment below.
>

AFAIK i915 and nouveau do this. But also on the rpi4 only the vc4 display
driver is disabled but the v3d driver used for rendering is not disabled.

So the 'nomodeset' semantics are not consistent across all DRM drivers.

[...]

>> -	if (virtio_gpu_modeset == 0)
>> -		return -EINVAL;
>> +	if ((drm_firmware_drivers_only() && virtio_gpu_modeset == -1) ||
>> +	    (virtio_gpu_modeset == 0))
>> +		driver.driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
>
> The kernel-wide option 'nomodeset' affects system behavior. It's a 
> misnomer, as it actually means 'don't replace the firmware-provided 
> framebuffer.' So if you just set these flags here, virtio-gpu would 
> later remove the firmware driver via aperture helpers. Therefore, if 
> drm_formware_drivers_only() returns true, we should fail probing with 
> -ENODEV.
>

Right. Or the DRM aperture helper shouldn't attempt to remove the firmware
provided framebuffer if the DRM driver doesn't have the DRIVER_MODESET set.

> But we could try to repurpose the module's 'modeset' option. It's 
> already obsoleted by nomodeset anyway.  I'd try to make modeset it a 
> boolean that controls modesetting vs render-only. It will then be about 
> the driver's feature set, rather than system behavior.
>

Yes, that could work too. Dmitry mentioned that Rob wanted the compile-time
option to reduce the attack surface area. I don't have a strong opinion on
this, but just wanted to point out that there wasn't a need for a new param
and that the existing module's 'modeset' could be repurposed for this case.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat