Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
On 03.12.20 22:30, Alex Deucher wrote:
> On Tue, Sep 29, 2020 at 4:04 PM Alex Deucher wrote:
>>
>> On Tue, Sep 29, 2020 at 8:31 AM Jan Kiszka wrote:
>>>
>>> On 10.09.20 20:18, Deucher, Alexander wrote:
>>>> [AMD Public Use]
>>>>
>>>>
>>>>
>>>>> -Original Message-
>>>>> From: amd-gfx On Behalf Of
>>>>> Daniel Vetter
>>>>> Sent: Monday, September 7, 2020 3:15 AM
>>>>> To: Jan Kiszka ; amd-gfx list >>>> g...@lists.freedesktop.org>; Wentland, Harry ;
>>>>> Kazlauskas, Nicholas
>>>>> Cc: dri-devel ; intel-gfx >>>> g...@lists.freedesktop.org>; Thomas Zimmermann
>>>>> ; Ville Syrjala
>>>>> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
>>>>>
>>>>> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka wrote:
>>>>>>
>>>>>> On 11.02.20 18:04, Daniel Vetter wrote:
>>>>>>> On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
>>>>>>>> From: Ville Syrjälä
>>>>>>>>
>>>>>>>> WARN if the encoder possible_crtcs is effectively empty or contains
>>>>>>>> bits for non-existing crtcs.
>>>>>>>>
>>>>>>>> v2: Move to drm_mode_config_validate() (Daniel)
>>>>>>>> Make the docs say we WARN when this is wrong (Daniel)
>>>>>>>> Extract full_crtc_mask()
>>>>>>>>
>>>>>>>> Cc: Thomas Zimmermann
>>>>>>>> Cc: Daniel Vetter
>>>>>>>> Signed-off-by: Ville Syrjälä
>>>>>>>
>>>>>>> When pushing the fixup needs to be applied before the validation
>>>>>>> patch here, because we don't want to anger the bisect gods.
>>>>>>>
>>>>>>> Reviewed-by: Daniel Vetter
>>>>>>>
>>>>>>> I think with the fixup we should be good enough with the existing
>>>>>>> nonsense in drivers. Fingers crossed.
>>>>>>> -Daniel
>>>>>>>
>>>>>>>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/drm_mode_config.c | 27
>>>>> ++-
>>>>>>>> include/drm/drm_encoder.h | 2 +-
>>>>>>>> 2 files changed, 27 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c
>>>>>>>> b/drivers/gpu/drm/drm_mode_config.c
>>>>>>>> index afc91447293a..4c1b350ddb95 100644
>>>>>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>>>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>>>>>> @@ -581,6 +581,29 @@ static void
>>>>> validate_encoder_possible_clones(struct drm_encoder *encoder)
>>>>>>>> encoder->possible_clones, encoder_mask); }
>>>>>>>>
>>>>>>>> +static u32 full_crtc_mask(struct drm_device *dev) {
>>>>>>>> +struct drm_crtc *crtc;
>>>>>>>> +u32 crtc_mask = 0;
>>>>>>>> +
>>>>>>>> +drm_for_each_crtc(crtc, dev)
>>>>>>>> +crtc_mask |= drm_crtc_mask(crtc);
>>>>>>>> +
>>>>>>>> +return crtc_mask;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void validate_encoder_possible_crtcs(struct drm_encoder
>>>>>>>> +*encoder) {
>>>>>>>> +u32 crtc_mask = full_crtc_mask(encoder->dev);
>>>>>>>> +
>>>>>>>> +WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
>>>>>>>> + (encoder->possible_crtcs & ~crtc_mask) != 0,
>>>>>>>> + "Bogus possible_crtcs: "
>>>>>>>> + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc
>>>>>>>> mask=0x%x)\n",
>>>>>>>> + encoder->base.id, encoder->name,
>>>>>>>> + encoder->possible_crtcs, crtc_mask); }
>>>>>>>> +
>>>>>>>> void drm_mode_config_validate(struct drm_
Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
On 10.09.20 20:18, Deucher, Alexander wrote:
> [AMD Public Use]
>
>
>
>> -Original Message-
>> From: amd-gfx On Behalf Of
>> Daniel Vetter
>> Sent: Monday, September 7, 2020 3:15 AM
>> To: Jan Kiszka ; amd-gfx list > g...@lists.freedesktop.org>; Wentland, Harry ;
>> Kazlauskas, Nicholas
>> Cc: dri-devel ; intel-gfx > g...@lists.freedesktop.org>; Thomas Zimmermann
>> ; Ville Syrjala
>> Subject: Re: [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
>>
>> On Sun, Sep 6, 2020 at 1:19 PM Jan Kiszka wrote:
>>>
>>> On 11.02.20 18:04, Daniel Vetter wrote:
>>>> On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
>>>>> From: Ville Syrjälä
>>>>>
>>>>> WARN if the encoder possible_crtcs is effectively empty or contains
>>>>> bits for non-existing crtcs.
>>>>>
>>>>> v2: Move to drm_mode_config_validate() (Daniel)
>>>>> Make the docs say we WARN when this is wrong (Daniel)
>>>>> Extract full_crtc_mask()
>>>>>
>>>>> Cc: Thomas Zimmermann
>>>>> Cc: Daniel Vetter
>>>>> Signed-off-by: Ville Syrjälä
>>>>
>>>> When pushing the fixup needs to be applied before the validation
>>>> patch here, because we don't want to anger the bisect gods.
>>>>
>>>> Reviewed-by: Daniel Vetter
>>>>
>>>> I think with the fixup we should be good enough with the existing
>>>> nonsense in drivers. Fingers crossed.
>>>> -Daniel
>>>>
>>>>
>>>>> ---
>>>>> drivers/gpu/drm/drm_mode_config.c | 27
>> ++-
>>>>> include/drm/drm_encoder.h | 2 +-
>>>>> 2 files changed, 27 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c
>>>>> b/drivers/gpu/drm/drm_mode_config.c
>>>>> index afc91447293a..4c1b350ddb95 100644
>>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>>> @@ -581,6 +581,29 @@ static void
>> validate_encoder_possible_clones(struct drm_encoder *encoder)
>>>>> encoder->possible_clones, encoder_mask); }
>>>>>
>>>>> +static u32 full_crtc_mask(struct drm_device *dev) {
>>>>> +struct drm_crtc *crtc;
>>>>> +u32 crtc_mask = 0;
>>>>> +
>>>>> +drm_for_each_crtc(crtc, dev)
>>>>> +crtc_mask |= drm_crtc_mask(crtc);
>>>>> +
>>>>> +return crtc_mask;
>>>>> +}
>>>>> +
>>>>> +static void validate_encoder_possible_crtcs(struct drm_encoder
>>>>> +*encoder) {
>>>>> +u32 crtc_mask = full_crtc_mask(encoder->dev);
>>>>> +
>>>>> +WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
>>>>> + (encoder->possible_crtcs & ~crtc_mask) != 0,
>>>>> + "Bogus possible_crtcs: "
>>>>> + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
>>>>> + encoder->base.id, encoder->name,
>>>>> + encoder->possible_crtcs, crtc_mask); }
>>>>> +
>>>>> void drm_mode_config_validate(struct drm_device *dev) {
>>>>> struct drm_encoder *encoder;
>>>>> @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct
>> drm_device *dev)
>>>>> drm_for_each_encoder(encoder, dev)
>>>>> fixup_encoder_possible_clones(encoder);
>>>>>
>>>>> -drm_for_each_encoder(encoder, dev)
>>>>> +drm_for_each_encoder(encoder, dev) {
>>>>> validate_encoder_possible_clones(encoder);
>>>>> +validate_encoder_possible_crtcs(encoder);
>>>>> +}
>>>>> }
>>>>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
>>>>> index 3741963b9587..b236269f41ac 100644
>>>>> --- a/include/drm/drm_encoder.h
>>>>> +++ b/include/drm/drm_encoder.h
>>>>> @@ -142,7 +142,7 @@ struct drm_encoder {
>>>>> * the bits for all _crtc objects this encoder can be connected
>>>>> to
>>>>> * before calling drm_dev_register().
>>>>> *
>>>>> - * In reality almost every driver gets this wrong.
>>>>> + * You will get a WARN if you get this wrong in the driver.
>>>>> *
>>>>> * Note that since CRTC objects can't be hotplugged the assigned
>> indices
>>>>> * are stable and hence known before registering all objects.
>>>>> --
>>>>> 2.24.1
>>>>>
>>>>
>>>
>>> Triggers on an Advantech AIMB-228 (R1505G, 3 DP outputs):
>>
>> Adding amdgpu display folks.
>
> I took a quick look at this and it looks like we limit the number of crtcs
> later in the mode init process if the number of physical displays can't
> actually use more crtcs. E.g., the physical board configuration would only
> allow for 3 active displays even if the hardware technically supports 4
> crtcs. I presume that way we can just leave the additional hardware power
> gated all the time.
>
So, will this be fixed any time soon? I don't feel qualified writing
such a patch but I would obviously be happy to test one.
Jan
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 6/7] drm: Validate encoder->possible_crtcs
On 11.02.20 18:04, Daniel Vetter wrote:
> On Tue, Feb 11, 2020 at 06:22:07PM +0200, Ville Syrjala wrote:
>> From: Ville Syrjälä
>>
>> WARN if the encoder possible_crtcs is effectively empty or contains
>> bits for non-existing crtcs.
>>
>> v2: Move to drm_mode_config_validate() (Daniel)
>> Make the docs say we WARN when this is wrong (Daniel)
>> Extract full_crtc_mask()
>>
>> Cc: Thomas Zimmermann
>> Cc: Daniel Vetter
>> Signed-off-by: Ville Syrjälä
>
> When pushing the fixup needs to be applied before the validation patch
> here, because we don't want to anger the bisect gods.
>
> Reviewed-by: Daniel Vetter
>
> I think with the fixup we should be good enough with the existing nonsense
> in drivers. Fingers crossed.
> -Daniel
>
>
>> ---
>> drivers/gpu/drm/drm_mode_config.c | 27 ++-
>> include/drm/drm_encoder.h | 2 +-
>> 2 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mode_config.c
>> b/drivers/gpu/drm/drm_mode_config.c
>> index afc91447293a..4c1b350ddb95 100644
>> --- a/drivers/gpu/drm/drm_mode_config.c
>> +++ b/drivers/gpu/drm/drm_mode_config.c
>> @@ -581,6 +581,29 @@ static void validate_encoder_possible_clones(struct
>> drm_encoder *encoder)
>> encoder->possible_clones, encoder_mask);
>> }
>>
>> +static u32 full_crtc_mask(struct drm_device *dev)
>> +{
>> +struct drm_crtc *crtc;
>> +u32 crtc_mask = 0;
>> +
>> +drm_for_each_crtc(crtc, dev)
>> +crtc_mask |= drm_crtc_mask(crtc);
>> +
>> +return crtc_mask;
>> +}
>> +
>> +static void validate_encoder_possible_crtcs(struct drm_encoder *encoder)
>> +{
>> +u32 crtc_mask = full_crtc_mask(encoder->dev);
>> +
>> +WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
>> + (encoder->possible_crtcs & ~crtc_mask) != 0,
>> + "Bogus possible_crtcs: "
>> + "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
>> + encoder->base.id, encoder->name,
>> + encoder->possible_crtcs, crtc_mask);
>> +}
>> +
>> void drm_mode_config_validate(struct drm_device *dev)
>> {
>> struct drm_encoder *encoder;
>> @@ -588,6 +611,8 @@ void drm_mode_config_validate(struct drm_device *dev)
>> drm_for_each_encoder(encoder, dev)
>> fixup_encoder_possible_clones(encoder);
>>
>> -drm_for_each_encoder(encoder, dev)
>> +drm_for_each_encoder(encoder, dev) {
>> validate_encoder_possible_clones(encoder);
>> +validate_encoder_possible_crtcs(encoder);
>> +}
>> }
>> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
>> index 3741963b9587..b236269f41ac 100644
>> --- a/include/drm/drm_encoder.h
>> +++ b/include/drm/drm_encoder.h
>> @@ -142,7 +142,7 @@ struct drm_encoder {
>> * the bits for all _crtc objects this encoder can be connected to
>> * before calling drm_dev_register().
>> *
>> - * In reality almost every driver gets this wrong.
>> + * You will get a WARN if you get this wrong in the driver.
>> *
>> * Note that since CRTC objects can't be hotplugged the assigned indices
>> * are stable and hence known before registering all objects.
>> --
>> 2.24.1
>>
>
Triggers on an Advantech AIMB-228 (R1505G, 3 DP outputs):
[ 14.033246] [ cut here ]
[ 14.033248] Bogus possible_crtcs: [ENCODER:65:TMDS-65] possible_crtcs=0xf
(full crtc mask=0x7)
[ 14.033279] WARNING: CPU: 0 PID: 282 at
../drivers/gpu/drm/drm_mode_config.c:622 drm_mode_config_validate+0x17d/0x200
[drm]
[ 14.033279] Modules linked in: amdgpu(E+) mfd_core(E)
snd_hda_codec_realtek(E) kvm_amd(E) gpu_sched(E) i2c_algo_bit(E) ttm(E)
snd_hda_codec_generic(E) kvm(E) ledtrig_audio(E) snd_hda_codec_hdmi(E)
drm_kms_helper(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_hda_codec(E) cec(E)
snd_hwdep(E) drm(E) irqbypass(E) snd_hda_core(E) crc32_pclmul(E) snd_pcm(E)
ghash_clmulni_intel(E) snd_timer(E) sg(E) aesni_intel(E) snd(E) ccp(E)
soundcore(E) rng_core(E) glue_helper(E) libaes(E) crypto_simd(E) k10temp(E)
efi_pstore(E) sp5100_tco(E) evdev(E) cryptd(E) pcspkr(E) efivars(E) video(E)
button(E) acpi_cpufreq(E) w83627hf_wdt(E) watchdog(E) nct6775(E) hwmon_vid(E)
efivarfs(E) ip_tables(E) x_tables(E) autofs4(E) ext4(E) crc32c_generic(E)
crc16(E) mbcache(E) jbd2(E) sd_mod(E) t10_pi(E) crc_t10dif(E)
crct10dif_generic(E) uas(E) usb_storage(E) ahci(E) libahci(E) xhci_pci(E)
r8169(E) realtek(E) mdio_devres(E) xhci_hcd(E) libata(E) i2c_amd_mp2_pci(E)
crct10dif_pclmul(E) crct10dif_common(E) scsi_mod(E)
[ 14.033306] crc32c_intel(E) i2c_piix4(E) usbcore(E) libphy(E)
[ 14.033310] CPU: 0 PID: 282 Comm: systemd-udevd Tainted: GE
5.9.0-rc3+ #2
[ 14.033311] Hardware name: Default string Default string/Default string,
BIOS 5.0.1.4 02/14/2020
[ 14.033324] RIP: 0010:drm_mode_config_validate+0x17d/0x200 [drm]
[ 14.033327] Code: 42 f0 75 e6 41 85 f8 74 09 44 89 c0 f7 d0 85 f8 74 1a 49
8b 54 24 38
Re: [Intel-gfx] [ANNOUNCE][RFC] KVMGT - the implementation of Intel GVT-g(full GPU virtualization) for KVM
On 2014-12-04 03:24, Jike Song wrote: Hi all, We are pleased to announce the first release of KVMGT project. KVMGT is the implementation of Intel GVT-g technology, a full GPU virtualization solution. Under Intel GVT-g, a virtual GPU instance is maintained for each VM, with part of performance critical resources directly assigned. The capability of running native graphics driver inside a VM, without hypervisor intervention in performance critical paths, achieves a good balance of performance, feature, and sharing capability. KVMGT is still in the early stage: - Basic functions of full GPU virtualization works, guest can see a full-featured vGPU. We ran several 3D workloads such as lightsmark, nexuiz, urbanterror and warsow. - Only Linux guest supported so far, and PPGTT must be disabled in guest through a kernel parameter(see README.kvmgt in QEMU). - This drop also includes some Xen specific changes, which will be cleaned up later. - Our end goal is to upstream both XenGT and KVMGT, which shares ~90% logic for vGPU device model (will be part of i915 driver), with only difference in hypervisor specific services - insufficient test coverage, so please bear with stability issues :) There are things need to be improved, esp. the KVM interfacing part: 1a domid was added to each KVMGT guest An ID is needed for foreground OS switching, e.g. # echo domid/sys/kernel/vgt/control/foreground_vm domid 0 is reserved for host OS. 2SRCU workarounds. Some KVM functions, such as: kvm_io_bus_register_dev install_new_memslots must be called *without* kvm-srcu read-locked. Otherwise it hangs. In KVMGT, we need to register an iodev only *after* BAR registers are written by guest. That means, we already have kvm-srcu hold - trapping/emulating PIO(BAR registers) makes us in such a condition. That will make kvm_io_bus_register_dev hangs. Currently we have to disable rcu_assign_pointer() in such functions. These were dirty workarounds, your suggestions are high welcome! 3syscalls were called to access /dev/mem from kernel An in-kernel memslot was added for aperture, but using syscalls like open and mmap to open and access the character device /dev/mem, for pass-through. The source codes(kernel, qemu as well as seabios) are available at github: git://github.com/01org/KVMGT-kernel git://github.com/01org/KVMGT-qemu git://github.com/01org/KVMGT-seabios In the KVMGT-qemu repository, there is a README.kvmgt to be referred. More information about Intel GVT-g and KVMGT can be found at: https://www.usenix.org/conference/atc14/technical-sessions/presentation/tian http://events.linuxfoundation.org/sites/events/files/slides/KVMGT-a%20Full%20GPU%20Virtualization%20Solution_1.pdf Appreciate your comments, BUG reports, and contributions! There is an even increasing interest to keep KVM's in-kernel guest interface as small as possible, specifically for security reasons. I'm sure there are some good performance reasons to create a new in-kernel device model, but I suppose those will need good evidences why things are done in the way they finally should be - and not via a user-space device model. This is likely not a binary decision (all userspace vs. no userspace), it is more about the size and robustness of the in-kernel model vs. its performance. One aspect could also be important: Are there hardware improvements in sight that will eventually help to reduce the in-kernel device model and make the overall design even more robust? How will those changes fit best into a proposed user/kernel split? Jan -- Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
