Re: Design session notes: GPU acceleration in Xen
On 14.06.2024 18:35, Demi Marie Obenour wrote: > On Fri, Jun 14, 2024 at 08:38:51AM +0200, Jan Beulich wrote: >> On 13.06.2024 20:43, Demi Marie Obenour wrote: >>> 2. Add support for `XEN_DOMCTL_memory_mapping` to use system RAM, not >>>just IOMEM. Mappings made with `XEN_DOMCTL_memory_mapping` are >>>guaranteed to be able to be successfully revoked with >>>`XEN_DOMCTL_memory_mapping`, so all operations that would create >>>extra references to the mapped memory must be forbidden. These >>>include, but may not be limited to: >>> >>>1. Granting the pages to the same or other domains. >>>2. Mapping into another domain using `XEN_DOMCTL_memory_mapping`. >>>3. Another domain accessing the pages using the foreign memory APIs, >>> unless it is privileged over the domain that owns the pages. >> >> All of which may call for actually converting the memory to kind-of-MMIO, >> with a means to later convert it back. > > Would this support the case where the mapping domain is not fully > priviliged, and where it might be a PV guest? I suppose that should be a goal. Jan
Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh
On 16.03.2023 14:53, Alex Deucher wrote: > On Thu, Mar 16, 2023 at 9:48 AM Juergen Gross wrote: >> >> On 16.03.23 14:45, Alex Deucher wrote: >>> On Thu, Mar 16, 2023 at 3:50 AM Jan Beulich wrote: >>>> >>>> On 16.03.2023 00:25, Stefano Stabellini wrote: >>>>> On Wed, 15 Mar 2023, Jan Beulich wrote: >>>>>> On 15.03.2023 01:52, Stefano Stabellini wrote: >>>>>>> On Mon, 13 Mar 2023, Jan Beulich wrote: >>>>>>>> On 12.03.2023 13:01, Huang Rui wrote: >>>>>>>>> Xen PVH is the paravirtualized mode and takes advantage of hardware >>>>>>>>> virtualization support when possible. It will using the hardware IOMMU >>>>>>>>> support instead of xen-swiotlb, so disable swiotlb if current domain >>>>>>>>> is >>>>>>>>> Xen PVH. >>>>>>>> >>>>>>>> But the kernel has no way (yet) to drive the IOMMU, so how can it get >>>>>>>> away without resorting to swiotlb in certain cases (like I/O to an >>>>>>>> address-restricted device)? >>>>>>> >>>>>>> I think Ray meant that, thanks to the IOMMU setup by Xen, there is no >>>>>>> need for swiotlb-xen in Dom0. Address translations are done by the IOMMU >>>>>>> so we can use guest physical addresses instead of machine addresses for >>>>>>> DMA. This is a similar case to Dom0 on ARM when the IOMMU is available >>>>>>> (see include/xen/arm/swiotlb-xen.h:xen_swiotlb_detect, the corresponding >>>>>>> case is XENFEAT_not_direct_mapped). >>>>>> >>>>>> But how does Xen using an IOMMU help with, as said, address-restricted >>>>>> devices? They may still need e.g. a 32-bit address to be programmed in, >>>>>> and if the kernel has memory beyond the 4G boundary not all I/O buffers >>>>>> may fulfill this requirement. >>>>> >>>>> In short, it is going to work as long as Linux has guest physical >>>>> addresses (not machine addresses, those could be anything) lower than >>>>> 4GB. >>>>> >>>>> If the address-restricted device does DMA via an IOMMU, then the device >>>>> gets programmed by Linux using its guest physical addresses (not machine >>>>> addresses). >>>>> >>>>> The 32-bit restriction would be applied by Linux to its choice of guest >>>>> physical address to use to program the device, the same way it does on >>>>> native. The device would be fine as it always uses Linux-provided <4GB >>>>> addresses. After the IOMMU translation (pagetable setup by Xen), we >>>>> could get any address, including >4GB addresses, and that is expected to >>>>> work. >>>> >>>> I understand that's the "normal" way of working. But whatever the swiotlb >>>> is used for in baremetal Linux, that would similarly require its use in >>>> PVH (or HVM) aiui. So unconditionally disabling it in PVH would look to >>>> me like an incomplete attempt to disable its use altogether on x86. What >>>> difference of PVH vs baremetal am I missing here? >>> >>> swiotlb is not usable for GPUs even on bare metal. They often have >>> hundreds or megs or even gigs of memory mapped on the device at any >>> given time. Also, AMD GPUs support 44-48 bit DMA masks (depending on >>> the chip family). >> >> But the swiotlb isn't per device, but system global. > > Sure, but if the swiotlb is in use, then you can't really use the GPU. > So you get to pick one. Yet that "pick one" then can't be an unconditional disable in the source code. If there's no way to avoid swiotlb on a per-device basis, then users will need to be told to arrange for this via command line option when they want to use the GPU is certain ways. Jan
Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh
On 16.03.2023 00:25, Stefano Stabellini wrote: > On Wed, 15 Mar 2023, Jan Beulich wrote: >> On 15.03.2023 01:52, Stefano Stabellini wrote: >>> On Mon, 13 Mar 2023, Jan Beulich wrote: >>>> On 12.03.2023 13:01, Huang Rui wrote: >>>>> Xen PVH is the paravirtualized mode and takes advantage of hardware >>>>> virtualization support when possible. It will using the hardware IOMMU >>>>> support instead of xen-swiotlb, so disable swiotlb if current domain is >>>>> Xen PVH. >>>> >>>> But the kernel has no way (yet) to drive the IOMMU, so how can it get >>>> away without resorting to swiotlb in certain cases (like I/O to an >>>> address-restricted device)? >>> >>> I think Ray meant that, thanks to the IOMMU setup by Xen, there is no >>> need for swiotlb-xen in Dom0. Address translations are done by the IOMMU >>> so we can use guest physical addresses instead of machine addresses for >>> DMA. This is a similar case to Dom0 on ARM when the IOMMU is available >>> (see include/xen/arm/swiotlb-xen.h:xen_swiotlb_detect, the corresponding >>> case is XENFEAT_not_direct_mapped). >> >> But how does Xen using an IOMMU help with, as said, address-restricted >> devices? They may still need e.g. a 32-bit address to be programmed in, >> and if the kernel has memory beyond the 4G boundary not all I/O buffers >> may fulfill this requirement. > > In short, it is going to work as long as Linux has guest physical > addresses (not machine addresses, those could be anything) lower than > 4GB. > > If the address-restricted device does DMA via an IOMMU, then the device > gets programmed by Linux using its guest physical addresses (not machine > addresses). > > The 32-bit restriction would be applied by Linux to its choice of guest > physical address to use to program the device, the same way it does on > native. The device would be fine as it always uses Linux-provided <4GB > addresses. After the IOMMU translation (pagetable setup by Xen), we > could get any address, including >4GB addresses, and that is expected to > work. I understand that's the "normal" way of working. But whatever the swiotlb is used for in baremetal Linux, that would similarly require its use in PVH (or HVM) aiui. So unconditionally disabling it in PVH would look to me like an incomplete attempt to disable its use altogether on x86. What difference of PVH vs baremetal am I missing here? Jan
Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh
On 15.03.2023 05:14, Huang Rui wrote: > On Wed, Mar 15, 2023 at 08:52:30AM +0800, Stefano Stabellini wrote: >> On Mon, 13 Mar 2023, Jan Beulich wrote: >>> On 12.03.2023 13:01, Huang Rui wrote: >>>> Xen PVH is the paravirtualized mode and takes advantage of hardware >>>> virtualization support when possible. It will using the hardware IOMMU >>>> support instead of xen-swiotlb, so disable swiotlb if current domain is >>>> Xen PVH. >>> >>> But the kernel has no way (yet) to drive the IOMMU, so how can it get >>> away without resorting to swiotlb in certain cases (like I/O to an >>> address-restricted device)? >> >> I think Ray meant that, thanks to the IOMMU setup by Xen, there is no >> need for swiotlb-xen in Dom0. Address translations are done by the IOMMU >> so we can use guest physical addresses instead of machine addresses for >> DMA. This is a similar case to Dom0 on ARM when the IOMMU is available >> (see include/xen/arm/swiotlb-xen.h:xen_swiotlb_detect, the corresponding >> case is XENFEAT_not_direct_mapped). > > Hi Jan, sorry to late reply. We are using the native kernel amdgpu and ttm > driver on Dom0, amdgpu/ttm would like to use IOMMU to allocate coherent > buffers for userptr that map the user space memory to gpu access, however, > swiotlb doesn't support this. In other words, with swiotlb, we only can > handle the buffer page by page. But how does outright disabling swiotlb help with this? There still wouldn't be an IOMMU that your kernel has control over. Looks like you want something like pvIOMMU, but that work was never completed. And even then the swiotlb may continue to be needed for other purposes. Jan
Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh
On 15.03.2023 01:52, Stefano Stabellini wrote: > On Mon, 13 Mar 2023, Jan Beulich wrote: >> On 12.03.2023 13:01, Huang Rui wrote: >>> Xen PVH is the paravirtualized mode and takes advantage of hardware >>> virtualization support when possible. It will using the hardware IOMMU >>> support instead of xen-swiotlb, so disable swiotlb if current domain is >>> Xen PVH. >> >> But the kernel has no way (yet) to drive the IOMMU, so how can it get >> away without resorting to swiotlb in certain cases (like I/O to an >> address-restricted device)? > > I think Ray meant that, thanks to the IOMMU setup by Xen, there is no > need for swiotlb-xen in Dom0. Address translations are done by the IOMMU > so we can use guest physical addresses instead of machine addresses for > DMA. This is a similar case to Dom0 on ARM when the IOMMU is available > (see include/xen/arm/swiotlb-xen.h:xen_swiotlb_detect, the corresponding > case is XENFEAT_not_direct_mapped). But how does Xen using an IOMMU help with, as said, address-restricted devices? They may still need e.g. a 32-bit address to be programmed in, and if the kernel has memory beyond the 4G boundary not all I/O buffers may fulfill this requirement. Jan
Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh
On 12.03.2023 13:01, Huang Rui wrote: > Xen PVH is the paravirtualized mode and takes advantage of hardware > virtualization support when possible. It will using the hardware IOMMU > support instead of xen-swiotlb, so disable swiotlb if current domain is > Xen PVH. But the kernel has no way (yet) to drive the IOMMU, so how can it get away without resorting to swiotlb in certain cases (like I/O to an address-restricted device)? Jan
Re: i915 "GPU HANG", bisected to a2daa27c0c61 "swiotlb: simplify swiotlb_max_segment"
On 18.10.2022 13:02, Christoph Hellwig wrote: > On Tue, Oct 18, 2022 at 10:57:37AM +0200, Jan Beulich wrote: >> Shouldn't this then be xen_pv_domain() that you use here, and - if you >> really want IS_ENABLED() in addition - CONFIG_XEN_PV? > > I'll need help from people that understand Xen better than me what > the exact conditions (and maybe also comments are). Leaving the "i915 abuses" part aside (because I can't tell what exactly the abuse is), but assuming that "can't cope with bounce buffering" means they don't actually use the allocated buffers, I'd suggest this: /* * For Xen PV guests pages aren't contiguous in DMA (machine) address * space. The DMA API takes care of that both in dma_alloc_* (by * calling into the hypervisor to make the pages contiguous) and in * dma_map_* (by bounce buffering). But i915 abuses ignores the * coherency aspects of the DMA API and thus can't cope with bounce * buffering actually happening, so add a hack here to force small * allocations and mappings when running in PV mode on Xen. */ if (IS_ENABLED(CONFIG_XEN_PV) && xen_pv_domain()) max = PAGE_SIZE; I've dropped the TDX related remark because I don't think it's meaningful for PV guests. Otoh I've left the "abuses ignores" word sequence as is, no matter that it reads odd to me. Plus, as hinted at before, I'm not convinced the IS_ENABLED() use is actually necessary or warranted here. Jan
Re: i915 "GPU HANG", bisected to a2daa27c0c61 "swiotlb: simplify swiotlb_max_segment"
On 18.10.2022 10:24, Christoph Hellwig wrote: > @@ -127,19 +128,22 @@ static inline unsigned int i915_sg_dma_sizes(struct > scatterlist *sg) > return page_sizes; > } > > -static inline unsigned int i915_sg_segment_size(void) > +static inline unsigned int i915_sg_segment_size(struct device *dev) > { > - unsigned int size = swiotlb_max_segment(); > - > - if (size == 0) > - size = UINT_MAX; > - > - size = rounddown(size, PAGE_SIZE); > - /* swiotlb_max_segment_size can return 1 byte when it means one page. */ > - if (size < PAGE_SIZE) > - size = PAGE_SIZE; > - > - return size; > + size_t max = min_t(size_t, UINT_MAX, dma_max_mapping_size(dev)); > + > + /* > + * Xen on x86 can reshuffle pages under us. The DMA API takes > + * care of that both in dma_alloc_* (by calling into the hypervisor > + * to make the pages contigous) and in dma_map_* (by bounce buffering). > + * But i915 abuses ignores the coherency aspects of the DMA API and > + * thus can't cope with bounce buffering actually happening, so add > + * a hack here to force small allocations and mapping when running on > + * Xen. (good luck with TDX, btw --hch) > + */ > + if (IS_ENABLED(CONFIG_X86) && xen_domain()) > + max = PAGE_SIZE; > + return round_down(max, PAGE_SIZE); > } Shouldn't this then be xen_pv_domain() that you use here, and - if you really want IS_ENABLED() in addition - CONFIG_XEN_PV? Jan
Re: [PATCH 0/5] xen: cleanup detection of non-essential pv devices
On 22.10.2021 08:47, Juergen Gross wrote: > Today the non-essential pv devices are hard coded in the xenbus driver > and this list is lacking multiple entries. > > This series reworks the detection logic of non-essential devices by > adding a flag for that purpose to struct xenbus_driver. I'm wondering whether it wouldn't better be the other way around: The (hopefully few) essential ones get flagged, thus also making it more prominent during patch review that a flag gets added (and justification provided), instead of having to spot the lack of a flag getting set. Jan
[PATCH] drm/xen: adjust Kconfig
By having selected DRM_XEN, I was assuming I would build the frontend driver. As it turns out this is a dummy option, and I have really not been building this (because I had DRM disabled). Make it a promptless one, moving the "depends on" to the other, real option, and "select"ing the dummy one. Signed-off-by: Jan Beulich --- a/drivers/gpu/drm/xen/Kconfig +++ b/drivers/gpu/drm/xen/Kconfig @@ -1,15 +1,11 @@ # SPDX-License-Identifier: GPL-2.0-only config DRM_XEN - bool "DRM Support for Xen guest OS" - depends on XEN - help - Choose this option if you want to enable DRM support - for Xen. + bool config DRM_XEN_FRONTEND tristate "Para-virtualized frontend driver for Xen guest OS" - depends on DRM_XEN - depends on DRM + depends on XEN && DRM + select DRM_XEN select DRM_KMS_HELPER select VIDEOMODE_HELPERS select XEN_XENBUS_FRONTEND ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Xen-devel] [PATCH v9 4/5] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v5
>>> On 28.11.17 at 11:17, wrote: > Am 28.11.2017 um 10:46 schrieb Jan Beulich: >>>>> On 28.11.17 at 10:12, wrote: >>> In theory the BIOS would search for address space and won't find >>> anything, so the hotplug operation should fail even before it reaches >>> the kernel in the first place. >> How would the BIOS know what the OS does or plans to do? > > As far as I know the ACPI BIOS should work directly with the register > content. > > So when we update the register content to enable the MMIO decoding the > BIOS should know that as well. I'm afraid I don't follow: During memory hotplug, surely you don't expect the BIOS to do a PCI bus scan? Plus even if it did, it would be racy - some device could, at this very moment, have memory decoding disabled, just for the OS to re-enable it a millisecond later. Yet looking at BAR values is meaningless when memory decode of a device is disabled. >> I think >> it's the other way around - the OS needs to avoid using any regions >> for MMIO which are marked as hotpluggable in SRAT. > > I was under the impression that this is exactly what > acpi_numa_memory_affinity_init() does. Perhaps, except that (when I last looked) insufficient state is (was) being recorded to have that information readily available at the time MMIO space above 4Gb needs to be allocated for some device. >> Since there is >> no vNUMA yet for Xen Dom0, that would need special handling. > > I think that the problem is rather that SRAT is NUMA specific and if I'm > not totally mistaken the content is ignored when NUMA support isn't > compiled into the kernel. > > When Xen steals some memory from Dom0 by hocking up itself into the e820 > call then I would say the cleanest way is to report this memory in e820 > as reserved as well. But take that with a grain of salt, I'm seriously > not a Xen expert. The E820 handling in PV Linux is all fake anyway - there's a single chunk of memory given to a PV guest (including Dom0), contiguous in what PV guests know as "physical address space" (not to be mixed up with "machine address space", which is where MMIO needs to be allocated from). Xen code in the kernel then mimics an E820 matching the host one, moving around pieces of memory in physical address space if necessary. Since Dom0 knows the machine E820, MMIO allocation shouldn't need to be much different there from the non-Xen case. Jan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Xen-devel] [PATCH v9 4/5] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v5
>>> On 28.11.17 at 10:12, wrote: > In theory the BIOS would search for address space and won't find > anything, so the hotplug operation should fail even before it reaches > the kernel in the first place. How would the BIOS know what the OS does or plans to do? I think it's the other way around - the OS needs to avoid using any regions for MMIO which are marked as hotpluggable in SRAT. Since there is no vNUMA yet for Xen Dom0, that would need special handling. Jan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ping: [PATCH] radeon: avoid boot hang in Xen Dom0
>>> On 04.11.16 at 15:32, wrote: > On Fri, Nov 4, 2016 at 6:44 AM, Jan Beulich wrote: >>>>> On 13.09.16 at 17:54, wrote: >>> While a hard hang in atom_asic_init() likely points at a deeper problem >>> in the driver, restore the capability to boot a Xen Dom0 by simply >>> avoiding the call there: Other than for Xen DomU, Dom0 owning a device >>> does not really mean is has got passed through to it. >>> >>> In case it is of interest for further investigation, lspci for the >>> offending device says: >>> >>> ATI Technologies Inc RS480 [Radeon Xpress 200G Series] [1002:5954] >>> >>> Fixes: 05082b8bbd "drm/radeon: fix asic initialization for virtualized >>> environments" >> >> I may have overlooked a different fix dealing with the problem; if >> so, I'd appreciate that fix being pointed out. > > Already fixed: > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=88 > 4031f0aacf57dad1575f96714efc80de9b19cc Hmm, that indeed should make the immediate problem go away. Nevertheless I do think that Xen Dom0 should be treated just like native here. Only DomU should be forced through that extra path. I did also notice that the equivalent function under amdgpu/ has gone away again during the 4.9 merge window... Jan
Ping: [PATCH] radeon: avoid boot hang in Xen Dom0
>>> On 13.09.16 at 17:54, wrote: > While a hard hang in atom_asic_init() likely points at a deeper problem > in the driver, restore the capability to boot a Xen Dom0 by simply > avoiding the call there: Other than for Xen DomU, Dom0 owning a device > does not really mean is has got passed through to it. > > In case it is of interest for further investigation, lspci for the > offending device says: > > ATI Technologies Inc RS480 [Radeon Xpress 200G Series] [1002:5954] > > Fixes: 05082b8bbd "drm/radeon: fix asic initialization for virtualized > environments" I may have overlooked a different fix dealing with the problem; if so, I'd appreciate that fix being pointed out. Thanks, Jan > Signed-off-by: Jan Beulich > --- > drivers/gpu/drm/radeon/radeon_device.c |3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > --- 4.8-rc6/drivers/gpu/drm/radeon/radeon_device.c > +++ 4.8-rc6-radeon-Xen-boot/drivers/gpu/drm/radeon/radeon_device.c > @@ -34,6 +34,7 @@ > #include > #include > #include > +#include > #include "radeon_reg.h" > #include "radeon.h" > #include "atom.h" > @@ -642,7 +643,7 @@ void radeon_gtt_location(struct radeon_d > static bool radeon_device_is_virtual(void) > { > #ifdef CONFIG_X86 > - return boot_cpu_has(X86_FEATURE_HYPERVISOR); > + return boot_cpu_has(X86_FEATURE_HYPERVISOR) && !xen_initial_domain(); > #else > return false; > #endif > > >
[PATCH] radeon: avoid boot hang in Xen Dom0
While a hard hang in atom_asic_init() likely points at a deeper problem in the driver, restore the capability to boot a Xen Dom0 by simply avoiding the call there: Other than for Xen DomU, Dom0 owning a device does not really mean is has got passed through to it. In case it is of interest for further investigation, lspci for the offending device says: ATI Technologies Inc RS480 [Radeon Xpress 200G Series] [1002:5954] Fixes: 05082b8bbd "drm/radeon: fix asic initialization for virtualized environments" Signed-off-by: Jan Beulich --- drivers/gpu/drm/radeon/radeon_device.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) --- 4.8-rc6/drivers/gpu/drm/radeon/radeon_device.c +++ 4.8-rc6-radeon-Xen-boot/drivers/gpu/drm/radeon/radeon_device.c @@ -34,6 +34,7 @@ #include #include #include +#include #include "radeon_reg.h" #include "radeon.h" #include "atom.h" @@ -642,7 +643,7 @@ void radeon_gtt_location(struct radeon_d static bool radeon_device_is_virtual(void) { #ifdef CONFIG_X86 - return boot_cpu_has(X86_FEATURE_HYPERVISOR); + return boot_cpu_has(X86_FEATURE_HYPERVISOR) && !xen_initial_domain(); #else return false; #endif
[PATCH v2] drm/mgag200: don't use uninitialized variables in mga_g200se_set_plls()
I can only guess that instead of testm/testn (which are either uninitialized or have pre-determined values at the end of the preceding loops) n and m were meant to be used by commit e829d7ef9f ("drm/mgag200: Add support for a new rev of G200e"). In any event the compiler is right in warning that testm/testn are possibly uninitalized at this point, i.e. some change is needed no matter what. Signed-off-by: Jan Beulich Cc: Mathieu Larouche --- v2: As pointed out by Mathieu, 1 needs to be added to both m and n when consuming them. --- drivers/gpu/drm/mgag200/mgag200_mode.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- 4.3-rc6/drivers/gpu/drm/mgag200/mgag200_mode.c +++ 4.3-rc6-mgag200-uninit/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -194,7 +194,7 @@ static int mga_g200se_set_plls(struct mg } } - fvv = pllreffreq * testn / testm; + fvv = pllreffreq * (n + 1) / (m + 1); fvv = (fvv - 80) / 5; if (fvv > 15)
drm/mgag200: don't use uninitialized variables in mga_g200se_set_plls()
>>> On 20.10.15 at 16:47, wrote: > On 19/10/2015 6:29 AM, Jan Beulich wrote: >> --- 4.3-rc6/drivers/gpu/drm/mgag200/mgag200_mode.c >> +++ 4.3-rc6-mgag200-uninit/drivers/gpu/drm/mgag200/mgag200_mode.c >> @@ -194,7 +194,7 @@ static int mga_g200se_set_plls(struct mg >> } >> } >> >> -fvv = pllreffreq * testn / testm; >> +fvv = pllreffreq * n / m; >> fvv = (fvv - 80) / 5; >> >> if (fvv > 15) >> > If you are using n/m instead of testn/testm, you need to > add 1 to the variables to keep consistency. However, you > have the same issue where m/n could be used without being > initialized. So, I propose to keep testm, testn & testp > and initialized them with the default value. That makes them initialized, but since testn and testm are used as loop variables I don't see how initializing them to default values helps overcome the other half of the problem described (them having a constant, pre-determined value at the end of the loops: testn=257, testm=33). As to testp - it is always initialized anyway (so long as P_ARRAY_SIZE is not zero). And in the if() branch initialization isn't needed either, since the variables aren't being used after the loops. Jan
drm/mgag200: don't use uninitialized variables in mga_g200se_set_plls()
I can only guess that instead of testm/testn (which are either uninitialized or have pre-determined values at the end of the preceding loops) n and m were meant to be used by commit e829d7ef9f ("drm/mgag200: Add support for a new rev of G200e"). In any event the compiler is right in warning that testm/testn are possibly uninitalized at this point, i.e. some change is needed no matter what. Signed-off-by: Jan Beulich Cc: Mathieu Larouche --- drivers/gpu/drm/mgag200/mgag200_mode.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- 4.3-rc6/drivers/gpu/drm/mgag200/mgag200_mode.c +++ 4.3-rc6-mgag200-uninit/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -194,7 +194,7 @@ static int mga_g200se_set_plls(struct mg } } - fvv = pllreffreq * testn / testm; + fvv = pllreffreq * n / m; fvv = (fvv - 80) / 5; if (fvv > 15)
[PATCH] drm/mgag200: don't pass NULL file_priv to drm_gem_object_lookup()
Commit bf89209a6d ("drm/mga200g: Hold a proper reference for cursor_set") clearly didn't take the call site in drm_fb_helper.c:restore_fbdev_mode() into account, which passes NULL for file_priv and hence causes drm_gem_object_lookup() to fault. Move the lookup back to before "obj" is actually needed, adjusting error paths suitably once again. Signed-off-by: Jan Beulich Cc: Daniel Vetter Cc: Thierry Reding --- drivers/gpu/drm/mgag200/mgag200_cursor.c | 18 ++ 1 file changed, 10 insertions(+), 8 deletions(-) --- 4.3-rc6/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ 4.3-rc6-mgag200-cursor-set/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -40,7 +40,7 @@ int mga_crtc_cursor_set(struct drm_crtc struct mgag200_bo *pixels_2 = mdev->cursor.pixels_2; struct mgag200_bo *pixels_current = mdev->cursor.pixels_current; struct mgag200_bo *pixels_prev = mdev->cursor.pixels_prev; - struct drm_gem_object *obj; + struct drm_gem_object *obj = NULL; struct mgag200_bo *bo = NULL; int ret = 0; unsigned int i, row, col; @@ -70,15 +70,11 @@ int mga_crtc_cursor_set(struct drm_crtc BUG_ON(pixels_2 != pixels_current && pixels_2 != pixels_prev); BUG_ON(pixels_current == pixels_prev); - obj = drm_gem_object_lookup(dev, file_priv, handle); - if (!obj) - return -ENOENT; - ret = mgag200_bo_reserve(pixels_1, true); if (ret) { WREG8(MGA_CURPOSXL, 0); WREG8(MGA_CURPOSXH, 0); - goto out_unref; + return ret; } ret = mgag200_bo_reserve(pixels_2, true); if (ret) { @@ -110,6 +106,12 @@ int mga_crtc_cursor_set(struct drm_crtc } } + obj = drm_gem_object_lookup(dev, file_priv, handle); + if (!obj) { + ret = -ENOENT; + goto out1; + } + bo = gem_to_mga_bo(obj); ret = mgag200_bo_reserve(bo, true); if (ret) { @@ -243,13 +245,13 @@ int mga_crtc_cursor_set(struct drm_crtc out2: mgag200_bo_unreserve(bo); out1: + if (obj) + drm_gem_object_unreference_unlocked(obj); if (ret) mga_hide_cursor(mdev); mgag200_bo_unreserve(pixels_1); out_unreserve1: mgag200_bo_unreserve(pixels_2); -out_unref: - drm_gem_object_unreference_unlocked(obj); return ret; }