Re: [PATCH libdrm] Add basic CONTRIBUTING file

[Dave Airlie]

On Mon, 3 Sep 2018 at 18:47, Daniel Vetter  wrote:
>
> I picked up a bunch of the pieces from wayland's version:
>
> https://gitlab.freedesktop.org/wayland/wayland/blob/master/CONTRIBUTING.md
>
> The weston one is fairly similar. Then I rather massively trimmed it
> down since in reality libdrm is a bit a dumping ground with very few
> real rules. The commit rights and CoC sections I've copied verbatim
> from igt respectively drm-misc. Weston/Wayland only differ in their
> pick of how many patches you need (10 instead of 5). I think for
> libdrm this is supremely relevant, since most everyone will get their
> commit rights by contributing already to the kernel or mesa and having
> commit rights there already.
>
> Anyway, I figured this is good to get the rules documented, even if
> there's mostly not many rules.
>
> Note: This references maintainers in a MAINTAINERS file, which needs
> to be created first.
>
> Note: With the gitlab migration the entire commit rights process is
> still a bit up in the air. But gitlab commit rights and roles are
> hierarchical, so we can do libdrm-only maintainer/commiter roles
> ("Owner" and "Developer" in gitlab-speak). This should avoid
> conflating libdrm roles with mesa roles, useful for those pushing to
> libdrm as primarily kernel contributors.

Fine with me,

Acked-by: Dave Airlie 

Dave.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] [RFC]drm: add syncobj timeline support v3

[zhoucm1]

On 2018年09月03日 19:19, Christian König wrote:

Am 03.09.2018 um 12:07 schrieb Chunming Zhou:



在 2018/9/3 16:50, Christian König 写道:

Am 03.09.2018 um 06:13 schrieb Chunming Zhou:



在 2018/8/30 19:32, Christian König 写道:

[SNIP]



+
+struct drm_syncobj_wait_pt {
+    struct drm_syncobj_stub_fence base;
+    u64    value;
+    struct rb_node   node;
+};
+struct drm_syncobj_signal_pt {
+    struct drm_syncobj_stub_fence base;
+    struct dma_fence *signal_fence;
+    struct dma_fence *pre_pt_base;
+    struct dma_fence_cb signal_cb;
+    struct dma_fence_cb pre_pt_cb;
+    struct drm_syncobj *syncobj;
+    u64    value;
+    struct list_head list;
+};


What are those two structures good for

They are used to record wait ops points and signal ops points.
For timeline, they are connected by timeline value, works like:
    a. signal pt increase timeline value to signal_pt value 
when signal_pt is signaled. signal_pt is depending on previous 
pt fence and itself signal fence from signal ops.

    b. wait pt tree checks if timeline value over itself value.

For normal, works like:
    a. signal pt increase 1 for syncobj_timeline->signal_point 
every time when signal ops is performed.
    b. when wait ops is comming, wait pt will fetch above last 
signal pt value as its wait point. wait pt will be only 
signaled by equal point value signal_pt.




and why is the stub fence their base?
Good question, I tried to kzalloc for them as well when I debug 
them, I encountered a problem:
I lookup/find wait_pt or signal_pt successfully, but when I 
tried to use them, they are freed sometimes, and results in 
NULL point.
and generally, when lookup them, we often need their stub fence 
as well, and in the meantime,  their lifecycle are same.

Above reason, I make stub fence as their base.


That sounds like you only did this because you messed up the 
lifecycle.


Additional to that I don't think you correctly considered the 
lifecycle of the waits and the sync object itself. E.g. blocking 
in drm_syncobj_timeline_fini() until all waits are done is not a 
good idea.


What you should do instead is to create a fence_array object 
with all the fence we need to wait for when a wait point is 
requested.
Yeah, this is our initial discussion result, but when I tried to 
do that, I found that cannot meet the advance signal requirement:
    a. We need to consider the wait and signal pt value are not 
one-to-one match, it's difficult to find dependent point, at 
least, there is some overhead.


As far as I can see that is independent of using a fence array 
here. See you can either use a ring buffer or an rb-tree, but when 
you want to wait for a specific point we need to condense the not 
yet signaled fences into an array.
again, need to find the range of where the specific point in, we 
should close to timeline semantics, I also refered the sw_sync.c 
timeline, usally wait_pt is signalled by timeline point. And I 
agree we can implement it with several methods, but I don't think 
there are basical differences.


The problem is that with your current approach you need the sync_obj 
alive for the synchronization to work. That is most likely not a 
good idea.
Indeed, I will fix that. How abount only creating fence array for 
every wait pt when syncobj release? when syncobj release, wait pt 
must have waited the signal opertation, then we can easily condense 
fences for every wait pt. And meantime, we can take timeline based 
wait pt advantage.


That could work, but I don't see how you want to replace the already 
issued fence with a fence_array when the sync object is destroyed.


Additional to that I would rather prefer a consistent handling, e.g. 
without extra rarely used code paths.
Ah, I find a easy way, we just need to make syncobj_timeline structure 
as a reference. This way syncobj itself can be released first, 
wait_pt/signal_pt don't need syncobj at all.

every wait_pt/signal_pt keep a reference of syncobj_timeline.







Additional to that you enable signaling without a need from the 
waiting side. That is rather bad for implementations which need that 
optimization.
Do you mean increasing timeline based on signal fence is not better? 
only update timeline value when requested by a wait pt?


Yes, exactly.

This way, we will not update timeline value immidiately and cannot 
free signal pt immidiately, and we also need to consider it to CPU 
query and wait.


That is actually the better coding style. We usually try to avoid 
doing things in interrupt handlers as much as possible.
OK, I see your concern, how about to delay handling to a workqueue? this 
way, we only increase timeline value and wake up workqueue in fence cb, 
is that acceptable?





How about this idea:
1. Each signaling point is a fence implementation with an rb node.
2. Each node keeps a reference to the last previously inserted node.
3. Each node is referenced by the sync object itself.
4. Before each signal/wait operation we do a garbage collection

Re: [PATCH] drm/amdgpu: enable AGP aperture for GMC9 v2

[Zhang, Jerry (Junwei)]

On 09/03/2018 08:22 PM, Christian König wrote:

Enable the old AGP aperture to avoid GART mappings.

v2: don't enable it for SRIOV

Signed-off-by: Christian König 

Reviewed-by: Junwei Zhang 


---
  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 10 +-
  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c|  2 ++
  drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 10 +-
  3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
index 3403ded39d13..ffd0ec9586d1 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
@@ -65,16 +65,16 @@ static void gfxhub_v1_0_init_system_aperture_regs(struct 
amdgpu_device *adev)
  {
uint64_t value;

-   /* Disable AGP. */
+   /* Program the AGP BAR */
WREG32_SOC15(GC, 0, mmMC_VM_AGP_BASE, 0);
-   WREG32_SOC15(GC, 0, mmMC_VM_AGP_TOP, 0);
-   WREG32_SOC15(GC, 0, mmMC_VM_AGP_BOT, 0x);
+   WREG32_SOC15(GC, 0, mmMC_VM_AGP_BOT, adev->gmc.agp_start >> 24);
+   WREG32_SOC15(GC, 0, mmMC_VM_AGP_TOP, adev->gmc.agp_end >> 24);

/* Program the system aperture low logical page number. */
WREG32_SOC15(GC, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR,
-adev->gmc.vram_start >> 18);
+min(adev->gmc.vram_start, adev->gmc.agp_start) >> 18);
WREG32_SOC15(GC, 0, mmMC_VM_SYSTEM_APERTURE_HIGH_ADDR,
-adev->gmc.vram_end >> 18);
+max(adev->gmc.vram_end, adev->gmc.agp_end) >> 18);

/* Set default page address. */
value = adev->vram_scratch.gpu_addr - adev->gmc.vram_start
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index f467638eb49d..3529c55ab52d 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -772,6 +772,8 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device 
*adev,
base = mmhub_v1_0_get_fb_location(adev);
amdgpu_gmc_vram_location(adev, &adev->gmc, base);
amdgpu_gmc_gart_location(adev, mc);
+   if (!amdgpu_sriov_vf(adev))
+   amdgpu_gmc_agp_location(adev, mc);
/* base offset of vram pages */
adev->vm_manager.vram_base_offset = gfxhub_v1_0_get_mc_fb_offset(adev);
  }
diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c 
b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
index 5f6a9c85488f..73d7c075dd33 100644
--- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
@@ -76,16 +76,16 @@ static void mmhub_v1_0_init_system_aperture_regs(struct 
amdgpu_device *adev)
uint64_t value;
uint32_t tmp;

-   /* Disable AGP. */
+   /* Program the AGP BAR */
WREG32_SOC15(MMHUB, 0, mmMC_VM_AGP_BASE, 0);
-   WREG32_SOC15(MMHUB, 0, mmMC_VM_AGP_TOP, 0);
-   WREG32_SOC15(MMHUB, 0, mmMC_VM_AGP_BOT, 0x00FF);
+   WREG32_SOC15(MMHUB, 0, mmMC_VM_AGP_BOT, adev->gmc.agp_start >> 24);
+   WREG32_SOC15(MMHUB, 0, mmMC_VM_AGP_TOP, adev->gmc.agp_end >> 24);

/* Program the system aperture low logical page number. */
WREG32_SOC15(MMHUB, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR,
-adev->gmc.vram_start >> 18);
+min(adev->gmc.vram_start, adev->gmc.agp_start) >> 18);
WREG32_SOC15(MMHUB, 0, mmMC_VM_SYSTEM_APERTURE_HIGH_ADDR,
-adev->gmc.vram_end >> 18);
+max(adev->gmc.vram_end, adev->gmc.agp_end) >> 18);

/* Set default page address. */
value = adev->vram_scratch.gpu_addr - adev->gmc.vram_start +


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: fix typo in function comment

[Zhang, Jerry (Junwei)]

On 09/03/2018 06:59 PM, Qiang Yu wrote:

Signed-off-by: Qiang Yu 

Reviewed-by: Junwei Zhang 


---
  amdgpu/amdgpu.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index dc51659..e6ec7a8 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -731,7 +731,7 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle);
  void amdgpu_bo_inc_ref(amdgpu_bo_handle bo);

  /**
- * Request CPU access to GPU accessable memory
+ * Request CPU access to GPU accessible memory
   *
   * \param   buf_handle - \c [in] Buffer handle
   * \param   cpu- \c [out] CPU address to be used for access


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[RFC] drm/amdgpu: Add macros and documentation for format modifiers.

[Bas Nieuwenhuizen]

This is an initial proposal for format modifiers for AMD hardware.

It uses 48 bits including a chip generation, leaving 8 bits for
a format version number.

On gfx6-gfx8 we have all the fields influencing sample locations
in memory.

Tile split bytes are optional for single sample buffers as no
hardware reaches the split size with 1 sample and hence the actual
size does not matter.

The macrotile fields are duplicated for images with multiple planes.
If the planes have different bitdepth they need different macro
tile fields and different tile split bytes if multisample.

I could not fit multiple copies in for tile split bytes, but
multisample & multiplane images are very rare. Overall, I think
we should punt on multisample for a later format version since
they are generally not shared on any modifier aware windowing
system, and we have more issues like fmask & cmask.

We need these copies because the drm modifier of all planes in an
image needs to be equal, so we need to fit these together.

This adds fields for compression support, using metadata that is
compatible with AMDVLK and for which radv and radeonsi can
reasonably be extended.

The big open question for compression is between which generations
the format changed to see if we can share more.

This explicitly does not try to solve the linear stride alignment
issue, thoguh we could internally just use the tiling modes for
the linear modes to indicate linear images with the stride for the
given chip.

Signed-off-by: Bas Nieuwenhuizen 
CC: Chad Versace 
CC: Dave Airlie 
CC: Marek Olšák 
CC: Nicolai Hähnle 
CC: Alex Deucher 
CC: Daniel Vetter 
---
 include/uapi/drm/amdgpu_drm.h | 130 ++
 1 file changed, 130 insertions(+)

diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 9eeba55b..4e1452161dbf 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -990,6 +990,136 @@ struct drm_amdgpu_info_vce_clock_table {
 #define AMDGPU_FAMILY_AI   141 /* Vega10 */
 #define AMDGPU_FAMILY_RV   142 /* Raven */
 
+#define AMDGPU_CHIP_TAHITI 0
+#define AMDGPU_CHIP_PITCAIRN   1
+#define AMDGPU_CHIP_VERDE  2
+#define AMDGPU_CHIP_OLAND  3
+#define AMDGPU_CHIP_HAINAN 4
+#define AMDGPU_CHIP_BONAIRE5
+#define AMDGPU_CHIP_KAVERI 6
+#define AMDGPU_CHIP_KABINI 7
+#define AMDGPU_CHIP_HAWAII 8
+#define AMDGPU_CHIP_MULLINS9
+#define AMDGPU_CHIP_TOPAZ  10
+#define AMDGPU_CHIP_TONGA  11
+#define AMDGPU_CHIP_FIJI   12
+#define AMDGPU_CHIP_CARRIZO13
+#define AMDGPU_CHIP_STONEY 14
+#define AMDGPU_CHIP_POLARIS10  15
+#define AMDGPU_CHIP_POLARIS11  16
+#define AMDGPU_CHIP_POLARIS12  17
+#define AMDGPU_CHIP_VEGAM  18
+#define AMDGPU_CHIP_VEGA10 19
+#define AMDGPU_CHIP_VEGA12 20
+#define AMDGPU_CHIP_VEGA20 21
+#define AMDGPU_CHIP_RAVEN  22
+
+/* Format for GFX6-GFX8 DRM format modifiers. These are intentionally the same
+ * as AMDGPU_TILING_*. However, the the rules as to when to set them are
+ * different.
+ *
+ * Do not use linear ARRAY_MODEs or SWIZZLE_MODEs. Use DRM_FORMAT_MOD_LINEAR
+ * instead.
+ *
+ * If ARRAY_MODE is 1D, only the micro tile mode and the pipe config should be
+ * set.
+ *
+ * For other ARRAY_MODEs:
+ *  - Only set TILE_SPLIT if the image is multisample.
+ *
+ * We have 1 extra bit for the micro tile mode, as GFX6 and GFX7+ have 1
+ * different value there. The values are
+ *   - depth   : 0
+ *   - displayable : 1
+ *   - thin: 2
+ *   - thick (GFX6): 3
+ *   - rotated (GFX7+) : 4
+ *
+ * TODO: What to do with multisample multi plane images? More tile split
+ * fields don't fit if we want to keep a few bits for a format version.
+ */
+#define AMDGPU_MODIFIER_GFX8_ARRAY_MODE_SHIFT  0
+#define AMDGPU_MODIFIER_GFX8_ARRAY_MODE_MASK   0xf
+#define AMDGPU_MODIFIER_GFX8_PIPE_CONFIG_SHIFT 4
+#define AMDGPU_MODIFIER_GFX8_PIPE_CONFIG_MASK  0x1f
+#define AMDGPU_MODIFIER_GFX8_TILE_SPLIT_SHIFT  9
+#define AMDGPU_MODIFIER_GFX8_TILE_SPLIT_MASK   0x7
+#define AMDGPU_MODIFIER_GFX8_MICRO_TILE_MODE_SHIFT 12
+#define AMDGPU_MODIFIER_GFX8_MICRO_TILE_MODE_MASK  0x7
+#define AMDGPU_MODIFIER_GFX8_BANK_WIDTH_SHIFT  15
+#define AMDGPU_MODIFIER_GFX8_BANK_WIDTH_MASK   0x3
+#define AMDGPU_MODIFIER_GFX8_BANK_HEIGHT_SHIFT 17
+#define AMDGPU_MODIFIER_GFX8_BANK_HEIGHT_MASK  0x3
+#define AMDGPU_MODIFIER_GFX8_MACRO_TILE_ASPECT_SHIFT   19
+#define AMDGPU_MODIFIER_GFX8_MACRO_TILE_ASPECT_MASK0x3
+#define AMDGPU_MODIFIER_GFX8_NUM_BANKS_SHIFT   21
+#define AMDGPU_MODIFIER_GFX8_NUM_BANKS_MASK0x3
+
+/* Macrotile parameters for a second plane if existing */
+#define AMDGPU_MODIFIER_GFX8_BANK_WIDTH_1_SHIFT23
+#define AMDGPU_MODIFIER_GFX8_BANK_WIDTH_1_MASK 0x3
+#define AMDGPU_MODIFIER_GFX8_BANK_HEIGHT_1_SHIFT   25
+#define AMDGPU_MODIFIER_

Re: [PATCH libdrm] libdrm: Allow dynamic drm majors on linux

[Thomas Hellstrom]

On 09/03/2018 06:33 PM, Daniel Vetter wrote:

On Mon, Sep 03, 2018 at 11:16:29AM +0200, Thomas Hellstrom wrote:

On 08/31/2018 05:30 PM, Thomas Hellstrom wrote:

On 08/31/2018 05:27 PM, Emil Velikov wrote:

On 31 August 2018 at 15:38, Michel Dänzer  wrote:

[ Adding the amd-gfx list ]

On 2018-08-31 3:05 p.m., Thomas Hellstrom wrote:

On 08/31/2018 02:30 PM, Emil Velikov wrote:

On 31 August 2018 at 12:54, Thomas Hellstrom 
wrote:

To determine whether a device node is a drm device
node or not, the code
currently compares the node's major number to the static drm major
device
number.

This breaks the standalone vmwgfx driver on XWayland dri clients,


Any particular reason why the code doesn't use a fixed node there?
It will make the diff vs the in-kernel driver a bit smaller.

Because then it won't be able to interoperate with other in-tree
drivers, like virtual drm drivers or passthrough usb drm drivers.
There is no clean way to share the minor number allocation
with in-tree
drm, so standalone vmwgfx is using dynamic major allocation.

I wonder why I haven't heard of any of these issues with the standalone
version of amdgpu shipped in packaged AMD releases. Does that
also use a
different major number? If yes, maybe it's just that nobody has tried
Xwayland clients with that driver. If no, how does it avoid the other
issues described above?


AFAICT, the difference is that the standalone vmwgfx uses an internal
copy of drm core.
It doesn't reuse the in-kernel drm, hence it cannot know which minor
it can use.

-Emil

Actually, standalone vmwgfx could perhaps also try to allocate minors
from 63 and downwards. That might work, but needs some verification.


So unfortuntately this doesn't work since the in-tree drm's file operations
are registered with the DRM_MAJOR.
So I still think the patch is the way to go. If people are concerned that
also fbdev file descriptors are allowed, perhaps there are other sysfs
traits we can look at?

Somewhat out of curiosity, but why do you have to overwrite all of drm?
amdgpu seems to be able to pull their stunt off without ...
-Daniel


At the time we launched the standalone vmwgfx, the DRM <-> driver 
interface was moving considerably more rapidly than the DRM <-> kernel 
interface. I think that's still the case. Hence less work for us. Also 
meant we can install the full driver stack with latest features on 
fairly old VMs without backported DRM functionality.


/Thomas



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm: Allow dynamic drm majors on linux

[Daniel Vetter]

On Mon, Sep 03, 2018 at 11:16:29AM +0200, Thomas Hellstrom wrote:
> On 08/31/2018 05:30 PM, Thomas Hellstrom wrote:
> > On 08/31/2018 05:27 PM, Emil Velikov wrote:
> > > On 31 August 2018 at 15:38, Michel Dänzer  wrote:
> > > > [ Adding the amd-gfx list ]
> > > > 
> > > > On 2018-08-31 3:05 p.m., Thomas Hellstrom wrote:
> > > > > On 08/31/2018 02:30 PM, Emil Velikov wrote:
> > > > > > On 31 August 2018 at 12:54, Thomas Hellstrom 
> > > > > > wrote:
> > > > > > > To determine whether a device node is a drm device
> > > > > > > node or not, the code
> > > > > > > currently compares the node's major number to the static drm major
> > > > > > > device
> > > > > > > number.
> > > > > > > 
> > > > > > > This breaks the standalone vmwgfx driver on XWayland dri clients,
> > > > > > > 
> > > > > > Any particular reason why the code doesn't use a fixed node there?
> > > > > > It will make the diff vs the in-kernel driver a bit smaller.
> > > > > Because then it won't be able to interoperate with other in-tree
> > > > > drivers, like virtual drm drivers or passthrough usb drm drivers.
> > > > > There is no clean way to share the minor number allocation
> > > > > with in-tree
> > > > > drm, so standalone vmwgfx is using dynamic major allocation.
> > > > I wonder why I haven't heard of any of these issues with the standalone
> > > > version of amdgpu shipped in packaged AMD releases. Does that
> > > > also use a
> > > > different major number? If yes, maybe it's just that nobody has tried
> > > > Xwayland clients with that driver. If no, how does it avoid the other
> > > > issues described above?
> > > > 
> > > AFAICT, the difference is that the standalone vmwgfx uses an internal
> > > copy of drm core.
> > > It doesn't reuse the in-kernel drm, hence it cannot know which minor
> > > it can use.
> > > 
> > > -Emil
> > 
> > Actually, standalone vmwgfx could perhaps also try to allocate minors
> > from 63 and downwards. That might work, but needs some verification.
> > 
> 
> So unfortuntately this doesn't work since the in-tree drm's file operations
> are registered with the DRM_MAJOR.
> So I still think the patch is the way to go. If people are concerned that
> also fbdev file descriptors are allowed, perhaps there are other sysfs
> traits we can look at?

Somewhat out of curiosity, but why do you have to overwrite all of drm?
amdgpu seems to be able to pull their stunt off without ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 00/13] remove_conflicting_framebuffers() cleanup

[Daniel Vetter]

On Mon, Sep 03, 2018 at 01:31:34PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Monday, September 03, 2018 09:43:15 AM Daniel Vetter wrote:
> > On Sat, Sep 01, 2018 at 04:08:41PM +0200, Michał Mirosław wrote:
> > > This series cleans up duplicated code for replacing firmware FB
> > > driver with proper DRI driver and adds handover support to
> > > Tegra driver.
> > > 
> > > This is a sligtly updated version of a series sent on 24 Nov 2017.
> > > 
> > > ---
> > > v2:
> > >  - rebased on current drm-next
> > >  - dropped staging/sm750fb changes
> > >  - added kernel docs for DRM helpers
> > > v3:
> > >  - move kerneldoc to fbdev, where functions are implemented
> > >  - split kerneldoc for remove_conflicting_framebuffers()
> > 
> > Ah, that's not quite what I had in mind. I think having the docs (also) in
> > the drm helpers would be good, since that's where drm people will look,
> > and that's the function they'll call. I just wanted you to split the fbdev
> > and drm parts into 2 patches (since those are two different maintainers).
> > 
> > Anyway, this is ok too, so imo ready for merging. If you can resurrect the
> > drm docs (with a patch title of "drm/fb-helper: document fbdev remove
> > functions" or similar) that would be great.
> > 
> > Only thing we need for merging now is the ack from Bartlomiej.
> 
> For the whole patchset:
> 
> Acked-by: Bartlomiej Zolnierkiewicz 

Thanks, entire patch set applied to drm-misc-next for 4.20.
-Daniel

> 
> > -Daniel
> > 
> > >  - propagate return value in remove_conflicting_pci_framebuffers()
> > > 
> > > ---
> > > Michał Mirosław (13):
> > >   fbdev: show fbdev number for debugging
> > >   fbdev: allow apertures == NULL in remove_conflicting_framebuffers()
> > >   fbdev: add kerneldoc do remove_conflicting_framebuffers()
> > >   fbdev: add remove_conflicting_pci_framebuffers()
> > >   drm/amdgpu: use simpler remove_conflicting_pci_framebuffers()
> > >   drm/bochs: use simpler remove_conflicting_pci_framebuffers()
> > >   drm/cirrus: use simpler remove_conflicting_pci_framebuffers()
> > >   drm/mgag200: use simpler remove_conflicting_pci_framebuffers()
> > >   drm/radeon: use simpler remove_conflicting_pci_framebuffers()
> > >   drm/virtio: use simpler remove_conflicting_pci_framebuffers()
> > >   drm/vc4: use simpler remove_conflicting_framebuffers(NULL)
> > >   drm/sun4i: use simpler remove_conflicting_framebuffers(NULL)
> > >   drm/tegra: kick out simplefb
> > > 
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 24 +
> > >  drivers/gpu/drm/bochs/bochs_drv.c| 18 +--
> > >  drivers/gpu/drm/cirrus/cirrus_drv.c  | 23 +
> > >  drivers/gpu/drm/mgag200/mgag200_drv.c| 21 +---
> > >  drivers/gpu/drm/mgag200/mgag200_main.c   |  9 
> > >  drivers/gpu/drm/radeon/radeon_drv.c  | 23 +
> > >  drivers/gpu/drm/sun4i/sun4i_drv.c| 18 +--
> > >  drivers/gpu/drm/tegra/drm.c  |  4 ++
> > >  drivers/gpu/drm/vc4/vc4_drv.c| 20 +---
> > >  drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 24 ++---
> > >  drivers/video/fbdev/core/fbmem.c | 63 +++-
> > >  include/drm/drm_fb_helper.h  | 12 +
> > >  include/linux/fb.h   |  2 +
> > >  13 files changed, 89 insertions(+), 172 deletions(-)
> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 04/13] fbdev: add remove_conflicting_pci_framebuffers()

[Daniel Vetter]

On Sat, Sep 01, 2018 at 04:08:45PM +0200, Michał Mirosław wrote:
> Almost all PCI drivers using remove_conflicting_framebuffers() wrap it
> with the same code.
> 
> ---

This cuts away the sob. Just fyi.
-Daniel

> v2: add kerneldoc for DRM helper
> v3: propagate remove_conflicting_framebuffers() return value
>   + move kerneldoc to where function is implemented
> 
> Signed-off-by: Michał Mirosław 
> ---
>  drivers/video/fbdev/core/fbmem.c | 35 
>  include/drm/drm_fb_helper.h  | 12 +++
>  include/linux/fb.h   |  2 ++
>  3 files changed, 49 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c 
> b/drivers/video/fbdev/core/fbmem.c
> index 2de93b5014e3..cd96b1c62bbe 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -34,6 +34,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -1812,6 +1813,40 @@ int remove_conflicting_framebuffers(struct 
> apertures_struct *a,
>  }
>  EXPORT_SYMBOL(remove_conflicting_framebuffers);
>  
> +/**
> + * remove_conflicting_pci_framebuffers - remove firmware-configured 
> framebuffers for PCI devices
> + * @pdev: PCI device
> + * @resource_id: index of PCI BAR configuring framebuffer memory
> + * @name: requesting driver name
> + *
> + * This function removes framebuffer devices (eg. initialized by firmware)
> + * using memory range configured for @pdev's BAR @resource_id.
> + *
> + * The function assumes that PCI device with shadowed ROM drives a primary
> + * display and so kicks out vga16fb.
> + */
> +int remove_conflicting_pci_framebuffers(struct pci_dev *pdev, int res_id, 
> const char *name)
> +{
> + struct apertures_struct *ap;
> + bool primary = false;
> + int err;
> +
> + ap = alloc_apertures(1);
> + if (!ap)
> + return -ENOMEM;
> +
> + ap->ranges[0].base = pci_resource_start(pdev, res_id);
> + ap->ranges[0].size = pci_resource_len(pdev, res_id);
> +#ifdef CONFIG_X86
> + primary = pdev->resource[PCI_ROM_RESOURCE].flags &
> + IORESOURCE_ROM_SHADOW;
> +#endif
> + err = remove_conflicting_framebuffers(ap, name, primary);
> + kfree(ap);
> + return err;
> +}
> +EXPORT_SYMBOL(remove_conflicting_pci_framebuffers);
> +
>  /**
>   *   register_framebuffer - registers a frame buffer device
>   *   @fb_info: frame buffer info structure
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index b069433e7fc1..20ea856db900 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -577,4 +577,16 @@ drm_fb_helper_remove_conflicting_framebuffers(struct 
> apertures_struct *a,
>  #endif
>  }
>  
> +static inline int
> +drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
> +   int resource_id,
> +   const char *name)
> +{
> +#if IS_REACHABLE(CONFIG_FB)
> + return remove_conflicting_pci_framebuffers(pdev, resource_id, name);
> +#else
> + return 0;
> +#endif
> +}
> +
>  #endif
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index aa74a228bb92..abeffd55b66a 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -632,6 +632,8 @@ extern ssize_t fb_sys_write(struct fb_info *info, const 
> char __user *buf,
>  extern int register_framebuffer(struct fb_info *fb_info);
>  extern int unregister_framebuffer(struct fb_info *fb_info);
>  extern int unlink_framebuffer(struct fb_info *fb_info);
> +extern int remove_conflicting_pci_framebuffers(struct pci_dev *pdev, int 
> res_id,
> +const char *name);
>  extern int remove_conflicting_framebuffers(struct apertures_struct *a,
>  const char *name, bool primary);
>  extern int fb_prepare_logo(struct fb_info *fb_info, int rotate);
> -- 
> 2.18.0
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: enable AGP aperture for GMC9 v2

[Christian König]

Enable the old AGP aperture to avoid GART mappings.

v2: don't enable it for SRIOV

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c | 10 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c|  2 ++
 drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c  | 10 +-
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
index 3403ded39d13..ffd0ec9586d1 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_0.c
@@ -65,16 +65,16 @@ static void gfxhub_v1_0_init_system_aperture_regs(struct 
amdgpu_device *adev)
 {
uint64_t value;
 
-   /* Disable AGP. */
+   /* Program the AGP BAR */
WREG32_SOC15(GC, 0, mmMC_VM_AGP_BASE, 0);
-   WREG32_SOC15(GC, 0, mmMC_VM_AGP_TOP, 0);
-   WREG32_SOC15(GC, 0, mmMC_VM_AGP_BOT, 0x);
+   WREG32_SOC15(GC, 0, mmMC_VM_AGP_BOT, adev->gmc.agp_start >> 24);
+   WREG32_SOC15(GC, 0, mmMC_VM_AGP_TOP, adev->gmc.agp_end >> 24);
 
/* Program the system aperture low logical page number. */
WREG32_SOC15(GC, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR,
-adev->gmc.vram_start >> 18);
+min(adev->gmc.vram_start, adev->gmc.agp_start) >> 18);
WREG32_SOC15(GC, 0, mmMC_VM_SYSTEM_APERTURE_HIGH_ADDR,
-adev->gmc.vram_end >> 18);
+max(adev->gmc.vram_end, adev->gmc.agp_end) >> 18);
 
/* Set default page address. */
value = adev->vram_scratch.gpu_addr - adev->gmc.vram_start
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index f467638eb49d..3529c55ab52d 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -772,6 +772,8 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device 
*adev,
base = mmhub_v1_0_get_fb_location(adev);
amdgpu_gmc_vram_location(adev, &adev->gmc, base);
amdgpu_gmc_gart_location(adev, mc);
+   if (!amdgpu_sriov_vf(adev))
+   amdgpu_gmc_agp_location(adev, mc);
/* base offset of vram pages */
adev->vm_manager.vram_base_offset = gfxhub_v1_0_get_mc_fb_offset(adev);
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c 
b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
index 5f6a9c85488f..73d7c075dd33 100644
--- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_0.c
@@ -76,16 +76,16 @@ static void mmhub_v1_0_init_system_aperture_regs(struct 
amdgpu_device *adev)
uint64_t value;
uint32_t tmp;
 
-   /* Disable AGP. */
+   /* Program the AGP BAR */
WREG32_SOC15(MMHUB, 0, mmMC_VM_AGP_BASE, 0);
-   WREG32_SOC15(MMHUB, 0, mmMC_VM_AGP_TOP, 0);
-   WREG32_SOC15(MMHUB, 0, mmMC_VM_AGP_BOT, 0x00FF);
+   WREG32_SOC15(MMHUB, 0, mmMC_VM_AGP_BOT, adev->gmc.agp_start >> 24);
+   WREG32_SOC15(MMHUB, 0, mmMC_VM_AGP_TOP, adev->gmc.agp_end >> 24);
 
/* Program the system aperture low logical page number. */
WREG32_SOC15(MMHUB, 0, mmMC_VM_SYSTEM_APERTURE_LOW_ADDR,
-adev->gmc.vram_start >> 18);
+min(adev->gmc.vram_start, adev->gmc.agp_start) >> 18);
WREG32_SOC15(MMHUB, 0, mmMC_VM_SYSTEM_APERTURE_HIGH_ADDR,
-adev->gmc.vram_end >> 18);
+max(adev->gmc.vram_end, adev->gmc.agp_end) >> 18);
 
/* Set default page address. */
value = adev->vram_scratch.gpu_addr - adev->gmc.vram_start +
-- 
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] gpu: drm: drm_mm: Fix a sleep-in-atomic-context bug in show_leaks()

[Christian König]

Am 03.09.2018 um 09:52 schrieb Daniel Vetter:

On Sat, Sep 01, 2018 at 01:32:54PM +0100, Chris Wilson wrote:

Quoting Jia-Ju Bai (2018-09-01 13:20:41)

The driver may sleep with holding a spinlock.

The function call paths (from bottom to top) in Linux-4.16 are:

[FUNC] kmalloc(GFP_KERNEL)
drivers/gpu/drm/drm_mm.c, 130:
 kmalloc in show_leaks
drivers/gpu/drm/drm_mm.c, 913:
 show_leaks in drm_mm_takedown
drivers/gpu/drm/drm_vma_manager.c, 107:
 drm_mm_takedown in drm_vma_offset_manager_destroy
drivers/gpu/drm/drm_vma_manager.c, 106:
 _raw_write_lock in drm_vma_offset_manager_destroy

[FUNC] kmalloc(GFP_KERNEL)
drivers/gpu/drm/drm_mm.c, 130:
 kmalloc in show_leaks
drivers/gpu/drm/drm_mm.c, 913:
 show_leaks in drm_mm_takedown
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c, 71:
 drm_mm_takedown in amdgpu_vram_mgr_fini
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c, 70:
 spin_lock in amdgpu_vram_mgr_fini

[FUNC] kmalloc(GFP_KERNEL)
drivers/gpu/drm/drm_mm.c, 130:
 kmalloc in show_leaks
drivers/gpu/drm/drm_mm.c, 913:
 show_leaks in drm_mm_takedown
drivers/gpu/drm/ttm/ttm_bo_manager.c, 128:
 drm_mm_takedown in ttm_bo_man_takedown
drivers/gpu/drm/ttm/ttm_bo_manager.c, 126:
 spin_lock in ttm_bo_man_takedown

To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC.

The bug are above, since those spinlocks do not protect the data and
imply use-after-free.

Adding amdgpu, since that's where the bug seems to be.


When we have use after free we might have concurrent uses as well.

I think taking the lock here is probably a good idea if you don't want 
to accidentally access freed memory in show_leaks.


So Chris change sounds valid to me.

Christian.


-Daniel


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 00/13] remove_conflicting_framebuffers() cleanup

[Bartlomiej Zolnierkiewicz]

On Monday, September 03, 2018 09:43:15 AM Daniel Vetter wrote:
> On Sat, Sep 01, 2018 at 04:08:41PM +0200, Michał Mirosław wrote:
> > This series cleans up duplicated code for replacing firmware FB
> > driver with proper DRI driver and adds handover support to
> > Tegra driver.
> > 
> > This is a sligtly updated version of a series sent on 24 Nov 2017.
> > 
> > ---
> > v2:
> >  - rebased on current drm-next
> >  - dropped staging/sm750fb changes
> >  - added kernel docs for DRM helpers
> > v3:
> >  - move kerneldoc to fbdev, where functions are implemented
> >  - split kerneldoc for remove_conflicting_framebuffers()
> 
> Ah, that's not quite what I had in mind. I think having the docs (also) in
> the drm helpers would be good, since that's where drm people will look,
> and that's the function they'll call. I just wanted you to split the fbdev
> and drm parts into 2 patches (since those are two different maintainers).
> 
> Anyway, this is ok too, so imo ready for merging. If you can resurrect the
> drm docs (with a patch title of "drm/fb-helper: document fbdev remove
> functions" or similar) that would be great.
> 
> Only thing we need for merging now is the ack from Bartlomiej.

For the whole patchset:

Acked-by: Bartlomiej Zolnierkiewicz 

> -Daniel
> 
> >  - propagate return value in remove_conflicting_pci_framebuffers()
> > 
> > ---
> > Michał Mirosław (13):
> >   fbdev: show fbdev number for debugging
> >   fbdev: allow apertures == NULL in remove_conflicting_framebuffers()
> >   fbdev: add kerneldoc do remove_conflicting_framebuffers()
> >   fbdev: add remove_conflicting_pci_framebuffers()
> >   drm/amdgpu: use simpler remove_conflicting_pci_framebuffers()
> >   drm/bochs: use simpler remove_conflicting_pci_framebuffers()
> >   drm/cirrus: use simpler remove_conflicting_pci_framebuffers()
> >   drm/mgag200: use simpler remove_conflicting_pci_framebuffers()
> >   drm/radeon: use simpler remove_conflicting_pci_framebuffers()
> >   drm/virtio: use simpler remove_conflicting_pci_framebuffers()
> >   drm/vc4: use simpler remove_conflicting_framebuffers(NULL)
> >   drm/sun4i: use simpler remove_conflicting_framebuffers(NULL)
> >   drm/tegra: kick out simplefb
> > 
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 24 +
> >  drivers/gpu/drm/bochs/bochs_drv.c| 18 +--
> >  drivers/gpu/drm/cirrus/cirrus_drv.c  | 23 +
> >  drivers/gpu/drm/mgag200/mgag200_drv.c| 21 +---
> >  drivers/gpu/drm/mgag200/mgag200_main.c   |  9 
> >  drivers/gpu/drm/radeon/radeon_drv.c  | 23 +
> >  drivers/gpu/drm/sun4i/sun4i_drv.c| 18 +--
> >  drivers/gpu/drm/tegra/drm.c  |  4 ++
> >  drivers/gpu/drm/vc4/vc4_drv.c| 20 +---
> >  drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 24 ++---
> >  drivers/video/fbdev/core/fbmem.c | 63 +++-
> >  include/drm/drm_fb_helper.h  | 12 +
> >  include/linux/fb.h   |  2 +
> >  13 files changed, 89 insertions(+), 172 deletions(-)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] [RFC]drm: add syncobj timeline support v3

[Christian König]

Am 03.09.2018 um 12:07 schrieb Chunming Zhou:



在 2018/9/3 16:50, Christian König 写道:

Am 03.09.2018 um 06:13 schrieb Chunming Zhou:



在 2018/8/30 19:32, Christian König 写道:

[SNIP]



+
+struct drm_syncobj_wait_pt {
+    struct drm_syncobj_stub_fence base;
+    u64    value;
+    struct rb_node   node;
+};
+struct drm_syncobj_signal_pt {
+    struct drm_syncobj_stub_fence base;
+    struct dma_fence *signal_fence;
+    struct dma_fence *pre_pt_base;
+    struct dma_fence_cb signal_cb;
+    struct dma_fence_cb pre_pt_cb;
+    struct drm_syncobj *syncobj;
+    u64    value;
+    struct list_head list;
+};


What are those two structures good for

They are used to record wait ops points and signal ops points.
For timeline, they are connected by timeline value, works like:
    a. signal pt increase timeline value to signal_pt value when 
signal_pt is signaled. signal_pt is depending on previous pt 
fence and itself signal fence from signal ops.

    b. wait pt tree checks if timeline value over itself value.

For normal, works like:
    a. signal pt increase 1 for syncobj_timeline->signal_point 
every time when signal ops is performed.
    b. when wait ops is comming, wait pt will fetch above last 
signal pt value as its wait point. wait pt will be only signaled 
by equal point value signal_pt.




and why is the stub fence their base?
Good question, I tried to kzalloc for them as well when I debug 
them, I encountered a problem:
I lookup/find wait_pt or signal_pt successfully, but when I 
tried to use them, they are freed sometimes, and results in NULL 
point.
and generally, when lookup them, we often need their stub fence 
as well, and in the meantime,  their lifecycle are same.

Above reason, I make stub fence as their base.


That sounds like you only did this because you messed up the 
lifecycle.


Additional to that I don't think you correctly considered the 
lifecycle of the waits and the sync object itself. E.g. blocking 
in drm_syncobj_timeline_fini() until all waits are done is not a 
good idea.


What you should do instead is to create a fence_array object with 
all the fence we need to wait for when a wait point is requested.
Yeah, this is our initial discussion result, but when I tried to 
do that, I found that cannot meet the advance signal requirement:
    a. We need to consider the wait and signal pt value are not 
one-to-one match, it's difficult to find dependent point, at 
least, there is some overhead.


As far as I can see that is independent of using a fence array 
here. See you can either use a ring buffer or an rb-tree, but when 
you want to wait for a specific point we need to condense the not 
yet signaled fences into an array.
again, need to find the range of where the specific point in, we 
should close to timeline semantics, I also refered the sw_sync.c 
timeline, usally wait_pt is signalled by timeline point. And I agree 
we can implement it with several methods, but I don't think there 
are basical differences.


The problem is that with your current approach you need the sync_obj 
alive for the synchronization to work. That is most likely not a good 
idea.
Indeed, I will fix that. How abount only creating fence array for 
every wait pt when syncobj release? when syncobj release, wait pt must 
have waited the signal opertation, then we can easily condense fences 
for every wait pt. And meantime, we can take timeline based wait pt 
advantage.


That could work, but I don't see how you want to replace the already 
issued fence with a fence_array when the sync object is destroyed.


Additional to that I would rather prefer a consistent handling, e.g. 
without extra rarely used code paths.






Additional to that you enable signaling without a need from the 
waiting side. That is rather bad for implementations which need that 
optimization.
Do you mean increasing timeline based on signal fence is not better? 
only update timeline value when requested by a wait pt?


Yes, exactly.

This way, we will not update timeline value immidiately and cannot 
free signal pt immidiately, and we also need to consider it to CPU 
query and wait.


That is actually the better coding style. We usually try to avoid doing 
things in interrupt handlers as much as possible.


How about this idea:
1. Each signaling point is a fence implementation with an rb node.
2. Each node keeps a reference to the last previously inserted node.
3. Each node is referenced by the sync object itself.
4. Before each signal/wait operation we do a garbage collection and 
remove the first node from the tree as long as it is signaled.


5. When enable_signaling is requested for a node we cascade that to the 
left using rb_prev.
    This ensures that signaling is enabled for the current fence as 
well as all previous fences.


6. A wait just looks into the tree for the signal point lower or equal 
of the requested sequence number.


7. When the sync object is released we use 
rbtree_postorder_for_each_entry_safe() and drop 

[PATCH libdrm] amdgpu: fix typo in function comment

[Qiang Yu]

Signed-off-by: Qiang Yu 
---
 amdgpu/amdgpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index dc51659..e6ec7a8 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -731,7 +731,7 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle);
 void amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
 
 /**
- * Request CPU access to GPU accessable memory
+ * Request CPU access to GPU accessible memory
  *
  * \param   buf_handle - \c [in] Buffer handle
  * \param   cpu- \c [out] CPU address to be used for access
-- 
1.9.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: amdgpu_bo_inc_ref don't return dummy int

[Yu, Qiang]

Sorry for not check this patch carefully, indeed a typo, will revert it.

Regards,
Qiang


From: Christian König 
Sent: Monday, September 3, 2018 6:40:33 PM
To: Michel Dänzer; Yu, Qiang
Cc: Zhang, Jerry; Koenig, Christian; amd-gfx@lists.freedesktop.org; Deng, Hui
Subject: Re: [PATCH libdrm] amdgpu: amdgpu_bo_inc_ref don't return dummy int

Am 03.09.2018 um 12:13 schrieb Michel Dänzer:
> On 2018-09-03 12:06 p.m., Qiang Yu wrote:
>> Signed-off-by: Qiang Yu 
>> ---
>>   amdgpu/amdgpu-symbol-check | 2 +-
>>   amdgpu/amdgpu.h| 5 +
>>   amdgpu/amdgpu_bo.c | 3 +--
>>   3 files changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
>> index 487610e..58646e8 100755
>> --- a/amdgpu/amdgpu-symbol-check
>> +++ b/amdgpu/amdgpu-symbol-check
>> @@ -15,8 +15,8 @@ amdgpu_bo_cpu_map
>>   amdgpu_bo_cpu_unmap
>>   amdgpu_bo_export
>>   amdgpu_bo_free
>> -amdgpu_bo_inc_ref
>>   amdgpu_bo_import
>> +amdgpu_bo_inc_ref
>>   amdgpu_bo_list_create
>>   amdgpu_bo_list_destroy
>>   amdgpu_bo_list_update
>> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
>> index e1f93f8..dc51659 100644
>> --- a/amdgpu/amdgpu.h
>> +++ b/amdgpu/amdgpu.h
>> @@ -725,13 +725,10 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle);
>>*
>>* \param   bo - \c [in]  Buffer object handle to increase the reference 
>> count
>>*
>> - * \return   0 on success\n
>> - *  <0 - Negative POSIX Error code
>> - *
>>* \sa amdgpu_bo_alloc(), amdgpu_bo_free()
>>*
>>   */
>> -int amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
>> +void amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
>>
>>   /**
>>* Request CPU access to GPU accessable memory
>> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
>> index dceab01..6a95929 100644
>> --- a/amdgpu/amdgpu_bo.c
>> +++ b/amdgpu/amdgpu_bo.c
>> @@ -438,10 +438,9 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle)
>>  return 0;
>>   }
>>
>> -int amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
>> +void amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
>>   {
>>  atomic_inc(&bo->refcount);
>> -return 0;
>>   }
>>
>>   int amdgpu_bo_cpu_map(amdgpu_bo_handle bo, void **cpu)
>>
> Reviewed-by: Michel Dänzer 

Added this and my rb as well and pushed it to master repository.

BTW: In the original patch there seems to be an unrelated change:
> - * Request CPU access to GPU accessible memory
...
> + * Request CPU access to GPU accessable memory

That doesn't looks correct to me and we should probably revert that.

Christian.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: amdgpu_bo_inc_ref don't return dummy int

[Christian König]

Am 03.09.2018 um 12:13 schrieb Michel Dänzer:

On 2018-09-03 12:06 p.m., Qiang Yu wrote:

Signed-off-by: Qiang Yu 
---
  amdgpu/amdgpu-symbol-check | 2 +-
  amdgpu/amdgpu.h| 5 +
  amdgpu/amdgpu_bo.c | 3 +--
  3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
index 487610e..58646e8 100755
--- a/amdgpu/amdgpu-symbol-check
+++ b/amdgpu/amdgpu-symbol-check
@@ -15,8 +15,8 @@ amdgpu_bo_cpu_map
  amdgpu_bo_cpu_unmap
  amdgpu_bo_export
  amdgpu_bo_free
-amdgpu_bo_inc_ref
  amdgpu_bo_import
+amdgpu_bo_inc_ref
  amdgpu_bo_list_create
  amdgpu_bo_list_destroy
  amdgpu_bo_list_update
diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index e1f93f8..dc51659 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -725,13 +725,10 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle);
   *
   * \param   bo - \c [in]  Buffer object handle to increase the reference count
   *
- * \return   0 on success\n
- *  <0 - Negative POSIX Error code
- *
   * \sa amdgpu_bo_alloc(), amdgpu_bo_free()
   *
  */
-int amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
+void amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
  
  /**

   * Request CPU access to GPU accessable memory
diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
index dceab01..6a95929 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -438,10 +438,9 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle)
return 0;
  }
  
-int amdgpu_bo_inc_ref(amdgpu_bo_handle bo)

+void amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
  {
atomic_inc(&bo->refcount);
-   return 0;
  }
  
  int amdgpu_bo_cpu_map(amdgpu_bo_handle bo, void **cpu)



Reviewed-by: Michel Dänzer 


Added this and my rb as well and pushed it to master repository.

BTW: In the original patch there seems to be an unrelated change:

- * Request CPU access to GPU accessible memory

...

+ * Request CPU access to GPU accessable memory


That doesn't looks correct to me and we should probably revert that.

Christian.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: amdgpu_bo_inc_ref don't return dummy int

[Michel Dänzer]

On 2018-09-03 12:06 p.m., Qiang Yu wrote:
> Signed-off-by: Qiang Yu 
> ---
>  amdgpu/amdgpu-symbol-check | 2 +-
>  amdgpu/amdgpu.h| 5 +
>  amdgpu/amdgpu_bo.c | 3 +--
>  3 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
> index 487610e..58646e8 100755
> --- a/amdgpu/amdgpu-symbol-check
> +++ b/amdgpu/amdgpu-symbol-check
> @@ -15,8 +15,8 @@ amdgpu_bo_cpu_map
>  amdgpu_bo_cpu_unmap
>  amdgpu_bo_export
>  amdgpu_bo_free
> -amdgpu_bo_inc_ref
>  amdgpu_bo_import
> +amdgpu_bo_inc_ref
>  amdgpu_bo_list_create
>  amdgpu_bo_list_destroy
>  amdgpu_bo_list_update
> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
> index e1f93f8..dc51659 100644
> --- a/amdgpu/amdgpu.h
> +++ b/amdgpu/amdgpu.h
> @@ -725,13 +725,10 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle);
>   *
>   * \param   bo - \c [in]  Buffer object handle to increase the reference 
> count
>   *
> - * \return   0 on success\n
> - *  <0 - Negative POSIX Error code
> - *
>   * \sa amdgpu_bo_alloc(), amdgpu_bo_free()
>   *
>  */
> -int amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
> +void amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
>  
>  /**
>   * Request CPU access to GPU accessable memory
> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
> index dceab01..6a95929 100644
> --- a/amdgpu/amdgpu_bo.c
> +++ b/amdgpu/amdgpu_bo.c
> @@ -438,10 +438,9 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle)
>   return 0;
>  }
>  
> -int amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
> +void amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
>  {
>   atomic_inc(&bo->refcount);
> - return 0;
>  }
>  
>  int amdgpu_bo_cpu_map(amdgpu_bo_handle bo, void **cpu)
> 

Reviewed-by: Michel Dänzer 


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] [RFC]drm: add syncobj timeline support v3

[Chunming Zhou]

在 2018/9/3 16:50, Christian König 写道:

Am 03.09.2018 um 06:13 schrieb Chunming Zhou:



在 2018/8/30 19:32, Christian König 写道:

[SNIP]



+
+struct drm_syncobj_wait_pt {
+    struct drm_syncobj_stub_fence base;
+    u64    value;
+    struct rb_node   node;
+};
+struct drm_syncobj_signal_pt {
+    struct drm_syncobj_stub_fence base;
+    struct dma_fence *signal_fence;
+    struct dma_fence *pre_pt_base;
+    struct dma_fence_cb signal_cb;
+    struct dma_fence_cb pre_pt_cb;
+    struct drm_syncobj *syncobj;
+    u64    value;
+    struct list_head list;
+};


What are those two structures good for

They are used to record wait ops points and signal ops points.
For timeline, they are connected by timeline value, works like:
    a. signal pt increase timeline value to signal_pt value when 
signal_pt is signaled. signal_pt is depending on previous pt 
fence and itself signal fence from signal ops.

    b. wait pt tree checks if timeline value over itself value.

For normal, works like:
    a. signal pt increase 1 for syncobj_timeline->signal_point 
every time when signal ops is performed.
    b. when wait ops is comming, wait pt will fetch above last 
signal pt value as its wait point. wait pt will be only signaled 
by equal point value signal_pt.




and why is the stub fence their base?
Good question, I tried to kzalloc for them as well when I debug 
them, I encountered a problem:
I lookup/find wait_pt or signal_pt successfully, but when I tried 
to use them, they are freed sometimes, and results in NULL point.
and generally, when lookup them, we often need their stub fence 
as well, and in the meantime,  their lifecycle are same.

Above reason, I make stub fence as their base.


That sounds like you only did this because you messed up the 
lifecycle.


Additional to that I don't think you correctly considered the 
lifecycle of the waits and the sync object itself. E.g. blocking 
in drm_syncobj_timeline_fini() until all waits are done is not a 
good idea.


What you should do instead is to create a fence_array object with 
all the fence we need to wait for when a wait point is requested.
Yeah, this is our initial discussion result, but when I tried to do 
that, I found that cannot meet the advance signal requirement:
    a. We need to consider the wait and signal pt value are not 
one-to-one match, it's difficult to find dependent point, at least, 
there is some overhead.


As far as I can see that is independent of using a fence array here. 
See you can either use a ring buffer or an rb-tree, but when you 
want to wait for a specific point we need to condense the not yet 
signaled fences into an array.
again, need to find the range of where the specific point in, we 
should close to timeline semantics, I also refered the sw_sync.c 
timeline, usally wait_pt is signalled by timeline point. And I agree 
we can implement it with several methods, but I don't think there are 
basical differences.


The problem is that with your current approach you need the sync_obj 
alive for the synchronization to work. That is most likely not a good 
idea.
Indeed, I will fix that. How abount only creating fence array for every 
wait pt when syncobj release? when syncobj release, wait pt must have 
waited the signal opertation, then we can easily condense fences for 
every wait pt. And meantime, we can take timeline based wait pt advantage.





Additional to that you enable signaling without a need from the 
waiting side. That is rather bad for implementations which need that 
optimization.
Do you mean increasing timeline based on signal fence is not better? 
only update timeline value when requested by a wait pt? This way, we 
will not update timeline value immidiately and cannot free signal pt 
immidiately, and we also need to consider it to CPU  query and wait.


Thanks,
David Zhou



I suggest to either condense all the fences you need to wait for in an 
array during the wait operation, or reference count the sync_obj and 
only enable the signaling on the fences when requested by a wait.






    b. because we allowed "wait-before-signal", if 
"wait-before-signal" happens, there isn't signal fence which can be 
used to create fence array.


Well, again we DON'T support wait-before-signal here. I will 
certainly NAK any implementation which tries to do this until we 
haven't figured out all the resource management constraints and I 
still don't see how we can do this.
yes, we don't support real wait-before-signal in patch now, just a 
fake wait-before-signal, which still wait on CS submission until 
signal operation coming through wait_event, which is the conclusion 
we disscussed before.


Well in this case we should call it wait-for-signal and not 
wait-before-signal :)







So timeline value is good to resolve that.



Otherwise somebody can easily construct a situation where timeline 
sync obj A waits on B and B waits on A.
Same situation also can happens with fence-array, we only can see 
this is a programming 

[PATCH libdrm] amdgpu: amdgpu_bo_inc_ref don't return dummy int

[Qiang Yu]

Signed-off-by: Qiang Yu 
---
 amdgpu/amdgpu-symbol-check | 2 +-
 amdgpu/amdgpu.h| 5 +
 amdgpu/amdgpu_bo.c | 3 +--
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
index 487610e..58646e8 100755
--- a/amdgpu/amdgpu-symbol-check
+++ b/amdgpu/amdgpu-symbol-check
@@ -15,8 +15,8 @@ amdgpu_bo_cpu_map
 amdgpu_bo_cpu_unmap
 amdgpu_bo_export
 amdgpu_bo_free
-amdgpu_bo_inc_ref
 amdgpu_bo_import
+amdgpu_bo_inc_ref
 amdgpu_bo_list_create
 amdgpu_bo_list_destroy
 amdgpu_bo_list_update
diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index e1f93f8..dc51659 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -725,13 +725,10 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle);
  *
  * \param   bo - \c [in]  Buffer object handle to increase the reference count
  *
- * \return   0 on success\n
- *  <0 - Negative POSIX Error code
- *
  * \sa amdgpu_bo_alloc(), amdgpu_bo_free()
  *
 */
-int amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
+void amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
 
 /**
  * Request CPU access to GPU accessable memory
diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
index dceab01..6a95929 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -438,10 +438,9 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle)
return 0;
 }
 
-int amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
+void amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
 {
atomic_inc(&bo->refcount);
-   return 0;
 }
 
 int amdgpu_bo_cpu_map(amdgpu_bo_handle bo, void **cpu)
-- 
1.9.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: add amdgpu_bo_inc_ref() function.

[Yu, Qiang]

>>  amdgpu_bo_free
>> +amdgpu_bo_inc_ref
>>  amdgpu_bo_import
>>  amdgpu_bo_list_create
>>  amdgpu_bo_list_destroy
>
> Thanks for remembering to add the symbol here, but amdgpu_bo_inc_ref
> goes after amdgpu_bo_import in lexical order. :)
Oh, haven't notice this is in alphabetic order.

>>
>> +int amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
>> +{
>> + atomic_inc(&bo->refcount);
>> + return 0;
>> +}
>
> What's the point of having a non-void return value that's always 0?
Indeed no actual functionality, will prepare a patch for removing it.

Thanks,
Qiang

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: add amdgpu_bo_inc_ref() function.

[Michel Dänzer]

On 2018-09-03 8:55 a.m., Qiang Yu wrote:
> For Pro OGL be able to work with upstream libdrm.
> 
> Signed-off-by: Qiang Yu 
> Reviewed-by: Christian König 
> ---
>  amdgpu/amdgpu-symbol-check |  1 +
>  amdgpu/amdgpu.h| 15 ++-
>  amdgpu/amdgpu_bo.c |  6 ++
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
> index b5e4fe6..487610e 100755
> --- a/amdgpu/amdgpu-symbol-check
> +++ b/amdgpu/amdgpu-symbol-check
> @@ -15,6 +15,7 @@ amdgpu_bo_cpu_map
>  amdgpu_bo_cpu_unmap
>  amdgpu_bo_export
>  amdgpu_bo_free
> +amdgpu_bo_inc_ref
>  amdgpu_bo_import
>  amdgpu_bo_list_create
>  amdgpu_bo_list_destroy

Thanks for remembering to add the symbol here, but amdgpu_bo_inc_ref
goes after amdgpu_bo_import in lexical order. :)


> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
> index a2fc525..dceab01 100644
> --- a/amdgpu/amdgpu_bo.c
> +++ b/amdgpu/amdgpu_bo.c
> @@ -438,6 +438,12 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle)
>   return 0;
>  }
>  
> +int amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
> +{
> + atomic_inc(&bo->refcount);
> + return 0;
> +}

What's the point of having a non-void return value that's always 0?


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] libdrm: Allow dynamic drm majors on linux

[Thomas Hellstrom]

On 08/31/2018 05:30 PM, Thomas Hellstrom wrote:

On 08/31/2018 05:27 PM, Emil Velikov wrote:

On 31 August 2018 at 15:38, Michel Dänzer  wrote:

[ Adding the amd-gfx list ]

On 2018-08-31 3:05 p.m., Thomas Hellstrom wrote:

On 08/31/2018 02:30 PM, Emil Velikov wrote:

On 31 August 2018 at 12:54, Thomas Hellstrom 
wrote:
To determine whether a device node is a drm device node or not, 
the code

currently compares the node's major number to the static drm major
device
number.

This breaks the standalone vmwgfx driver on XWayland dri clients,


Any particular reason why the code doesn't use a fixed node there?
It will make the diff vs the in-kernel driver a bit smaller.

Because then it won't be able to interoperate with other in-tree
drivers, like virtual drm drivers or passthrough usb drm drivers.
There is no clean way to share the minor number allocation with 
in-tree

drm, so standalone vmwgfx is using dynamic major allocation.

I wonder why I haven't heard of any of these issues with the standalone
version of amdgpu shipped in packaged AMD releases. Does that also 
use a

different major number? If yes, maybe it's just that nobody has tried
Xwayland clients with that driver. If no, how does it avoid the other
issues described above?


AFAICT, the difference is that the standalone vmwgfx uses an internal
copy of drm core.
It doesn't reuse the in-kernel drm, hence it cannot know which minor 
it can use.


-Emil


Actually, standalone vmwgfx could perhaps also try to allocate minors 
from 63 and downwards. That might work, but needs some verification.




So unfortuntately this doesn't work since the in-tree drm's file 
operations are registered with the DRM_MAJOR.
So I still think the patch is the way to go. If people are concerned 
that also fbdev file descriptors are allowed, perhaps there are other 
sysfs traits we can look at?


/Thomas





/Thomas



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: improve VM state machine documentation v2

[Zhang, Jerry (Junwei)]

On 09/03/2018 05:08 PM, Christian König wrote:

Since we have a lot of FAQ on the VM state machine try to improve the
documentation by adding functions for each state move.

v2: fix typo in amdgpu_vm_bo_invalidated, use amdgpu_vm_bo_relocated in
 one more place as well.

Signed-off-by: Christian König 

Reviewed-by: Junwei Zhang 


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 141 +
  1 file changed, 109 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 65977e7c94dc..1f79a0ddc78a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -204,6 +204,95 @@ static unsigned amdgpu_vm_bo_size(struct amdgpu_device 
*adev, unsigned level)
return AMDGPU_GPU_PAGE_ALIGN(amdgpu_vm_num_entries(adev, level) * 8);
  }

+/**
+ * amdgpu_vm_bo_evicted - vm_bo is evicted
+ *
+ * @vm_bo: vm_bo which is evicted
+ *
+ * State for PDs/PTs and per VM BOs which are not at the location they should
+ * be.
+ */
+static void amdgpu_vm_bo_evicted(struct amdgpu_vm_bo_base *vm_bo)
+{
+   struct amdgpu_vm *vm = vm_bo->vm;
+   struct amdgpu_bo *bo = vm_bo->bo;
+
+   vm_bo->moved = true;
+   if (bo->tbo.type == ttm_bo_type_kernel)
+   list_move(&vm_bo->vm_status, &vm->evicted);
+   else
+   list_move_tail(&vm_bo->vm_status, &vm->evicted);
+}
+
+/**
+ * amdgpu_vm_bo_relocated - vm_bo is reloacted
+ *
+ * @vm_bo: vm_bo which is relocated
+ *
+ * State for PDs/PTs which needs to update their parent PD.
+ */
+static void amdgpu_vm_bo_relocated(struct amdgpu_vm_bo_base *vm_bo)
+{
+   list_move(&vm_bo->vm_status, &vm_bo->vm->relocated);
+}
+
+/**
+ * amdgpu_vm_bo_moved - vm_bo is moved
+ *
+ * @vm_bo: vm_bo which is moved
+ *
+ * State for per VM BOs which are moved, but that change is not yet reflected
+ * in the page tables.
+ */
+static void amdgpu_vm_bo_moved(struct amdgpu_vm_bo_base *vm_bo)
+{
+   list_move(&vm_bo->vm_status, &vm_bo->vm->moved);
+}
+
+/**
+ * amdgpu_vm_bo_idle - vm_bo is idle
+ *
+ * @vm_bo: vm_bo which is now idle
+ *
+ * State for PDs/PTs and per VM BOs which have gone through the state machine
+ * and are now idle.
+ */
+static void amdgpu_vm_bo_idle(struct amdgpu_vm_bo_base *vm_bo)
+{
+   list_move(&vm_bo->vm_status, &vm_bo->vm->idle);
+   vm_bo->moved = false;
+}
+
+/**
+ * amdgpu_vm_bo_invalidated - vm_bo is invalidated
+ *
+ * @vm_bo: vm_bo which is now invalidated
+ *
+ * State for normal BOs which are invalidated and that change not yet reflected
+ * in the PTs.
+ */
+static void amdgpu_vm_bo_invalidated(struct amdgpu_vm_bo_base *vm_bo)
+{
+   spin_lock(&vm_bo->vm->invalidated_lock);
+   list_move(&vm_bo->vm_status, &vm_bo->vm->invalidated);
+   spin_unlock(&vm_bo->vm->invalidated_lock);
+}
+
+/**
+ * amdgpu_vm_bo_done - vm_bo is done
+ *
+ * @vm_bo: vm_bo which is now done
+ *
+ * State for normal BOs which are invalidated and that change has been updated
+ * in the PTs.
+ */
+static void amdgpu_vm_bo_done(struct amdgpu_vm_bo_base *vm_bo)
+{
+   spin_lock(&vm_bo->vm->invalidated_lock);
+   list_del_init(&vm_bo->vm_status);
+   spin_unlock(&vm_bo->vm->invalidated_lock);
+}
+
  /**
   * amdgpu_vm_bo_base_init - Adds bo to the list of bos associated with the vm
   *
@@ -232,9 +321,9 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base 
*base,

vm->bulk_moveable = false;
if (bo->tbo.type == ttm_bo_type_kernel)
-   list_move(&base->vm_status, &vm->relocated);
+   amdgpu_vm_bo_relocated(base);
else
-   list_move(&base->vm_status, &vm->idle);
+   amdgpu_vm_bo_idle(base);

if (bo->preferred_domains &
amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
@@ -245,8 +334,7 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base 
*base,
 * is currently evicted. add the bo to the evicted list to make sure it
 * is validated on next vm use to avoid fault.
 * */
-   list_move_tail(&base->vm_status, &vm->evicted);
-   base->moved = true;
+   amdgpu_vm_bo_evicted(base);
  }

  /**
@@ -342,7 +430,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, 
struct amdgpu_vm *vm,
break;

if (bo->tbo.type != ttm_bo_type_kernel) {
-   list_move(&bo_base->vm_status, &vm->moved);
+   amdgpu_vm_bo_moved(bo_base);
} else {
if (vm->use_cpu_for_update)
r = amdgpu_bo_kmap(bo, NULL);
@@ -350,7 +438,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, 
struct amdgpu_vm *vm,
r = amdgpu_ttm_alloc_gart(&bo->tbo);
if (r)
break;
-   list_move(&bo_base->vm_status, &vm->relocated);
+ 

Re: [PATCH libdrm] amdgpu: When couldn't find bo, need to return error.

[Zhang, Jerry (Junwei)]

On 09/03/2018 04:44 PM, Christian König wrote:

Am 03.09.2018 um 09:16 schrieb Zhang, Jerry (Junwei):

On 09/03/2018 03:11 PM, Christian König wrote:

About master branch, needs someone's help with correct permission.

I've already took care of that on the weekend.


Thank you again.
BTW, how to apply that permission?


Previously that was done by opening a bugzilla ticket, but since the migration 
to gitlab that might be outdated now.

A good start is to go to https://gitlab.freedesktop.org and register an 
account, then somebody from the admin team needs to add that account to the 
appropriate groups.


Thanks, will give a try.

Regards,
Jerry



Regards,
Christian.



Regards,
Jerry



Regards,
Christian.

Am 03.09.2018 um 03:42 schrieb Zhang, Jerry (Junwei):

On 09/01/2018 04:58 PM, Deng, Emily wrote:

Ok, then just ignore this patch. But seems didn't saw the patch on branch 
amd-staging-hybrid-master20180315.


Thanks to take care of this as well.

I'm waiting some verification, and now push the patch to internal staging branch
mainline will be pushed later for another verification.

About master branch, needs someone's help with correct permission.

Regards,
Jerry


Best wishes
Emily Deng


-Original Message-
From: Christian König 
Sent: Saturday, September 1, 2018 4:17 PM
To: Deng, Emily ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH libdrm] amdgpu: When couldn't find bo, need to return
error.

Am 01.09.2018 um 06:24 schrieb Emily Deng:

The startx will have segmant fault if return success.

SWDEV-163962

Change-Id: I56b189fa26efdcd1d96e5100af3f3e0b1208b0c3
Signed-off-by: Emily Deng 


Jerry already send a much better patch for this.


---
   amdgpu/amdgpu_bo.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c index
f25cacc..7e297fa 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -760,6 +760,7 @@ int

amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,

 uint64_t *offset_in_bo)
   {
   uint32_t i;
+int r = 0;
   struct amdgpu_bo *bo;

   if (cpu == NULL || size == 0)
@@ -787,10 +788,11 @@ int

amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,

   } else {
   *buf_handle = NULL;
   *offset_in_bo = 0;
+r = -errno;


errno doesn't contain any error in this case.


   }
pthread_mutex_unlock(&dev->bo_table_mutex);

-return 0;
+return r;
   }

   int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx






___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: improve VM state machine documentation v2

[Christian König]

Since we have a lot of FAQ on the VM state machine try to improve the
documentation by adding functions for each state move.

v2: fix typo in amdgpu_vm_bo_invalidated, use amdgpu_vm_bo_relocated in
one more place as well.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 141 +
 1 file changed, 109 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 65977e7c94dc..1f79a0ddc78a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -204,6 +204,95 @@ static unsigned amdgpu_vm_bo_size(struct amdgpu_device 
*adev, unsigned level)
return AMDGPU_GPU_PAGE_ALIGN(amdgpu_vm_num_entries(adev, level) * 8);
 }
 
+/**
+ * amdgpu_vm_bo_evicted - vm_bo is evicted
+ *
+ * @vm_bo: vm_bo which is evicted
+ *
+ * State for PDs/PTs and per VM BOs which are not at the location they should
+ * be.
+ */
+static void amdgpu_vm_bo_evicted(struct amdgpu_vm_bo_base *vm_bo)
+{
+   struct amdgpu_vm *vm = vm_bo->vm;
+   struct amdgpu_bo *bo = vm_bo->bo;
+
+   vm_bo->moved = true;
+   if (bo->tbo.type == ttm_bo_type_kernel)
+   list_move(&vm_bo->vm_status, &vm->evicted);
+   else
+   list_move_tail(&vm_bo->vm_status, &vm->evicted);
+}
+
+/**
+ * amdgpu_vm_bo_relocated - vm_bo is reloacted
+ *
+ * @vm_bo: vm_bo which is relocated
+ *
+ * State for PDs/PTs which needs to update their parent PD.
+ */
+static void amdgpu_vm_bo_relocated(struct amdgpu_vm_bo_base *vm_bo)
+{
+   list_move(&vm_bo->vm_status, &vm_bo->vm->relocated);
+}
+
+/**
+ * amdgpu_vm_bo_moved - vm_bo is moved
+ *
+ * @vm_bo: vm_bo which is moved
+ *
+ * State for per VM BOs which are moved, but that change is not yet reflected
+ * in the page tables.
+ */
+static void amdgpu_vm_bo_moved(struct amdgpu_vm_bo_base *vm_bo)
+{
+   list_move(&vm_bo->vm_status, &vm_bo->vm->moved);
+}
+
+/**
+ * amdgpu_vm_bo_idle - vm_bo is idle
+ *
+ * @vm_bo: vm_bo which is now idle
+ *
+ * State for PDs/PTs and per VM BOs which have gone through the state machine
+ * and are now idle.
+ */
+static void amdgpu_vm_bo_idle(struct amdgpu_vm_bo_base *vm_bo)
+{
+   list_move(&vm_bo->vm_status, &vm_bo->vm->idle);
+   vm_bo->moved = false;
+}
+
+/**
+ * amdgpu_vm_bo_invalidated - vm_bo is invalidated
+ *
+ * @vm_bo: vm_bo which is now invalidated
+ *
+ * State for normal BOs which are invalidated and that change not yet reflected
+ * in the PTs.
+ */
+static void amdgpu_vm_bo_invalidated(struct amdgpu_vm_bo_base *vm_bo)
+{
+   spin_lock(&vm_bo->vm->invalidated_lock);
+   list_move(&vm_bo->vm_status, &vm_bo->vm->invalidated);
+   spin_unlock(&vm_bo->vm->invalidated_lock);
+}
+
+/**
+ * amdgpu_vm_bo_done - vm_bo is done
+ *
+ * @vm_bo: vm_bo which is now done
+ *
+ * State for normal BOs which are invalidated and that change has been updated
+ * in the PTs.
+ */
+static void amdgpu_vm_bo_done(struct amdgpu_vm_bo_base *vm_bo)
+{
+   spin_lock(&vm_bo->vm->invalidated_lock);
+   list_del_init(&vm_bo->vm_status);
+   spin_unlock(&vm_bo->vm->invalidated_lock);
+}
+
 /**
  * amdgpu_vm_bo_base_init - Adds bo to the list of bos associated with the vm
  *
@@ -232,9 +321,9 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base 
*base,
 
vm->bulk_moveable = false;
if (bo->tbo.type == ttm_bo_type_kernel)
-   list_move(&base->vm_status, &vm->relocated);
+   amdgpu_vm_bo_relocated(base);
else
-   list_move(&base->vm_status, &vm->idle);
+   amdgpu_vm_bo_idle(base);
 
if (bo->preferred_domains &
amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
@@ -245,8 +334,7 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base 
*base,
 * is currently evicted. add the bo to the evicted list to make sure it
 * is validated on next vm use to avoid fault.
 * */
-   list_move_tail(&base->vm_status, &vm->evicted);
-   base->moved = true;
+   amdgpu_vm_bo_evicted(base);
 }
 
 /**
@@ -342,7 +430,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, 
struct amdgpu_vm *vm,
break;
 
if (bo->tbo.type != ttm_bo_type_kernel) {
-   list_move(&bo_base->vm_status, &vm->moved);
+   amdgpu_vm_bo_moved(bo_base);
} else {
if (vm->use_cpu_for_update)
r = amdgpu_bo_kmap(bo, NULL);
@@ -350,7 +438,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, 
struct amdgpu_vm *vm,
r = amdgpu_ttm_alloc_gart(&bo->tbo);
if (r)
break;
-   list_move(&bo_base->vm_status, &vm->relocated);
+   amdgpu_vm_bo_relocated(bo_base);
}
}
 
@@ -1066,7

Re: [PATCH] drm/amdgpu: fix amdgpu_mn_unlock() in the CS error path

[Zhang, Jerry (Junwei)]

On 09/03/2018 04:53 PM, Christian König wrote:

Avoid unlocking a lock we never locked.

Signed-off-by: Christian König 

Reviewed-by: Junwei Zhang 


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 349dcc37ee64..04a2733b5ccc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1247,10 +1247,10 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
  error_abort:
dma_fence_put(&job->base.s_fence->finished);
job->base.s_fence = NULL;
+   amdgpu_mn_unlock(p->mn);

  error_unlock:
amdgpu_job_free(job);
-   amdgpu_mn_unlock(p->mn);
return r;
  }



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: add amdgpu_bo_inc_ref() function.

[Yu, Qiang]

Thanks.

Regards,
Qiang


From: Christian König 
Sent: Monday, September 3, 2018 4:57:39 PM
To: Yu, Qiang; Zhang, Jerry; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian; Deng, Hui
Subject: Re: [PATCH libdrm] amdgpu: add amdgpu_bo_inc_ref() function.

Done.

Christian.

Am 03.09.2018 um 10:03 schrieb Yu, Qiang:
> Thanks Jerry.
>
> Hi Christian,
>
> This is an old patch back to 2016 reviewed in hybrid list. If you are OK with 
> it,
> would you please submit it to upstream?
>
> Thanks,
> Qiang
>
> 
> From: amd-gfx  on behalf of Zhang, 
> Jerry (Junwei) 
> Sent: Monday, September 3, 2018 3:18:14 PM
> To: Yu, Qiang; amd-gfx@lists.freedesktop.org
> Cc: Koenig, Christian; Deng, Hui
> Subject: Re: [PATCH libdrm] amdgpu: add amdgpu_bo_inc_ref() function.
>
> On 09/03/2018 02:55 PM, Qiang Yu wrote:
>> For Pro OGL be able to work with upstream libdrm.
>>
>> Signed-off-by: Qiang Yu 
>> Reviewed-by: Christian König 
> I'm fine with that, not sure if mesa is going to use that as well.
>
> Reviewed-by: Junwei Zhang 
>
> Regards,
> Jerry
>
>> ---
>>amdgpu/amdgpu-symbol-check |  1 +
>>amdgpu/amdgpu.h| 15 ++-
>>amdgpu/amdgpu_bo.c |  6 ++
>>3 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
>> index b5e4fe6..487610e 100755
>> --- a/amdgpu/amdgpu-symbol-check
>> +++ b/amdgpu/amdgpu-symbol-check
>> @@ -15,6 +15,7 @@ amdgpu_bo_cpu_map
>>amdgpu_bo_cpu_unmap
>>amdgpu_bo_export
>>amdgpu_bo_free
>> +amdgpu_bo_inc_ref
>>amdgpu_bo_import
>>amdgpu_bo_list_create
>>amdgpu_bo_list_destroy
>> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
>> index a8c353c..e1f93f8 100644
>> --- a/amdgpu/amdgpu.h
>> +++ b/amdgpu/amdgpu.h
>> @@ -721,7 +721,20 @@ int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle 
>> dev,
>>int amdgpu_bo_free(amdgpu_bo_handle buf_handle);
>>
>>/**
>> - * Request CPU access to GPU accessible memory
>> + * Increase the reference count of a buffer object
>> + *
>> + * \param   bo - \c [in]  Buffer object handle to increase the reference 
>> count
>> + *
>> + * \return   0 on success\n
>> + *  <0 - Negative POSIX Error code
>> + *
>> + * \sa amdgpu_bo_alloc(), amdgpu_bo_free()
>> + *
>> +*/
>> +int amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
>> +
>> +/**
>> + * Request CPU access to GPU accessable memory
>> *
>> * \param   buf_handle - \c [in] Buffer handle
>> * \param   cpu- \c [out] CPU address to be used for access
>> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
>> index a2fc525..dceab01 100644
>> --- a/amdgpu/amdgpu_bo.c
>> +++ b/amdgpu/amdgpu_bo.c
>> @@ -438,6 +438,12 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle)
>>return 0;
>>}
>>
>> +int amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
>> +{
>> + atomic_inc(&bo->refcount);
>> + return 0;
>> +}
>> +
>>int amdgpu_bo_cpu_map(amdgpu_bo_handle bo, void **cpu)
>>{
>>union drm_amdgpu_gem_mmap args;
>>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: add amdgpu_bo_inc_ref() function.

[Christian König]

Done.

Christian.

Am 03.09.2018 um 10:03 schrieb Yu, Qiang:

Thanks Jerry.

Hi Christian,

This is an old patch back to 2016 reviewed in hybrid list. If you are OK with 
it,
would you please submit it to upstream?

Thanks,
Qiang


From: amd-gfx  on behalf of Zhang, Jerry 
(Junwei) 
Sent: Monday, September 3, 2018 3:18:14 PM
To: Yu, Qiang; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian; Deng, Hui
Subject: Re: [PATCH libdrm] amdgpu: add amdgpu_bo_inc_ref() function.

On 09/03/2018 02:55 PM, Qiang Yu wrote:

For Pro OGL be able to work with upstream libdrm.

Signed-off-by: Qiang Yu 
Reviewed-by: Christian König 

I'm fine with that, not sure if mesa is going to use that as well.

Reviewed-by: Junwei Zhang 

Regards,
Jerry


---
   amdgpu/amdgpu-symbol-check |  1 +
   amdgpu/amdgpu.h| 15 ++-
   amdgpu/amdgpu_bo.c |  6 ++
   3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
index b5e4fe6..487610e 100755
--- a/amdgpu/amdgpu-symbol-check
+++ b/amdgpu/amdgpu-symbol-check
@@ -15,6 +15,7 @@ amdgpu_bo_cpu_map
   amdgpu_bo_cpu_unmap
   amdgpu_bo_export
   amdgpu_bo_free
+amdgpu_bo_inc_ref
   amdgpu_bo_import
   amdgpu_bo_list_create
   amdgpu_bo_list_destroy
diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index a8c353c..e1f93f8 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -721,7 +721,20 @@ int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
   int amdgpu_bo_free(amdgpu_bo_handle buf_handle);

   /**
- * Request CPU access to GPU accessible memory
+ * Increase the reference count of a buffer object
+ *
+ * \param   bo - \c [in]  Buffer object handle to increase the reference count
+ *
+ * \return   0 on success\n
+ *  <0 - Negative POSIX Error code
+ *
+ * \sa amdgpu_bo_alloc(), amdgpu_bo_free()
+ *
+*/
+int amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
+
+/**
+ * Request CPU access to GPU accessable memory
*
* \param   buf_handle - \c [in] Buffer handle
* \param   cpu- \c [out] CPU address to be used for access
diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
index a2fc525..dceab01 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -438,6 +438,12 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle)
   return 0;
   }

+int amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
+{
+ atomic_inc(&bo->refcount);
+ return 0;
+}
+
   int amdgpu_bo_cpu_map(amdgpu_bo_handle bo, void **cpu)
   {
   union drm_amdgpu_gem_mmap args;


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: fix amdgpu_mn_unlock() in the CS error path

[Christian König]

Avoid unlocking a lock we never locked.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 349dcc37ee64..04a2733b5ccc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1247,10 +1247,10 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 error_abort:
dma_fence_put(&job->base.s_fence->finished);
job->base.s_fence = NULL;
+   amdgpu_mn_unlock(p->mn);
 
 error_unlock:
amdgpu_job_free(job);
-   amdgpu_mn_unlock(p->mn);
return r;
 }
 
-- 
2.14.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] [RFC]drm: add syncobj timeline support v3

[Christian König]

Am 03.09.2018 um 06:13 schrieb Chunming Zhou:



在 2018/8/30 19:32, Christian König 写道:

[SNIP]



+
+struct drm_syncobj_wait_pt {
+    struct drm_syncobj_stub_fence base;
+    u64    value;
+    struct rb_node   node;
+};
+struct drm_syncobj_signal_pt {
+    struct drm_syncobj_stub_fence base;
+    struct dma_fence *signal_fence;
+    struct dma_fence *pre_pt_base;
+    struct dma_fence_cb signal_cb;
+    struct dma_fence_cb pre_pt_cb;
+    struct drm_syncobj *syncobj;
+    u64    value;
+    struct list_head list;
+};


What are those two structures good for

They are used to record wait ops points and signal ops points.
For timeline, they are connected by timeline value, works like:
    a. signal pt increase timeline value to signal_pt value when 
signal_pt is signaled. signal_pt is depending on previous pt fence 
and itself signal fence from signal ops.

    b. wait pt tree checks if timeline value over itself value.

For normal, works like:
    a. signal pt increase 1 for syncobj_timeline->signal_point 
every time when signal ops is performed.
    b. when wait ops is comming, wait pt will fetch above last 
signal pt value as its wait point. wait pt will be only signaled 
by equal point value signal_pt.




and why is the stub fence their base?
Good question, I tried to kzalloc for them as well when I debug 
them, I encountered a problem:
I lookup/find wait_pt or signal_pt successfully, but when I tried 
to use them, they are freed sometimes, and results in NULL point.
and generally, when lookup them, we often need their stub fence as 
well, and in the meantime,  their lifecycle are same.

Above reason, I make stub fence as their base.


That sounds like you only did this because you messed up the 
lifecycle.


Additional to that I don't think you correctly considered the 
lifecycle of the waits and the sync object itself. E.g. blocking in 
drm_syncobj_timeline_fini() until all waits are done is not a good 
idea.


What you should do instead is to create a fence_array object with 
all the fence we need to wait for when a wait point is requested.
Yeah, this is our initial discussion result, but when I tried to do 
that, I found that cannot meet the advance signal requirement:
    a. We need to consider the wait and signal pt value are not 
one-to-one match, it's difficult to find dependent point, at least, 
there is some overhead.


As far as I can see that is independent of using a fence array here. 
See you can either use a ring buffer or an rb-tree, but when you want 
to wait for a specific point we need to condense the not yet signaled 
fences into an array.
again, need to find the range of where the specific point in, we 
should close to timeline semantics, I also refered the sw_sync.c 
timeline, usally wait_pt is signalled by timeline point. And I agree 
we can implement it with several methods, but I don't think there are 
basical differences.


The problem is that with your current approach you need the sync_obj 
alive for the synchronization to work. That is most likely not a good idea.


Additional to that you enable signaling without a need from the waiting 
side. That is rather bad for implementations which need that optimization.


I suggest to either condense all the fences you need to wait for in an 
array during the wait operation, or reference count the sync_obj and 
only enable the signaling on the fences when requested by a wait.






    b. because we allowed "wait-before-signal", if 
"wait-before-signal" happens, there isn't signal fence which can be 
used to create fence array.


Well, again we DON'T support wait-before-signal here. I will 
certainly NAK any implementation which tries to do this until we 
haven't figured out all the resource management constraints and I 
still don't see how we can do this.
yes, we don't support real wait-before-signal in patch now, just a 
fake wait-before-signal, which still wait on CS submission until 
signal operation coming through wait_event, which is the conclusion we 
disscussed before.


Well in this case we should call it wait-for-signal and not 
wait-before-signal :)







So timeline value is good to resolve that.



Otherwise somebody can easily construct a situation where timeline 
sync obj A waits on B and B waits on A.
Same situation also can happens with fence-array, we only can see 
this is a programming bug with incorrect use.


No, fence-array is initialized only once with a static list of 
fences. This way it is impossible to add the fence-array to itself 
for example.


E.g. you can't build circle dependencies with that.
we already wait for signal operation event, so never build circle 
dependencies with that. The theory is same.


Yeah, ok that is certainly true.

Regards,
Christian.







Better use rbtree_postorder_for_each_entry_safe() for this.
From the comments, seems we cannot use it here, we need erase node 
here.

Comments:
 * Note, however, that it cannot handle other modifications that 
re-order the
 * rbtree it 

[PATCH libdrm] Add basic CONTRIBUTING file

[Daniel Vetter]

I picked up a bunch of the pieces from wayland's version:

https://gitlab.freedesktop.org/wayland/wayland/blob/master/CONTRIBUTING.md

The weston one is fairly similar. Then I rather massively trimmed it
down since in reality libdrm is a bit a dumping ground with very few
real rules. The commit rights and CoC sections I've copied verbatim
from igt respectively drm-misc. Weston/Wayland only differ in their
pick of how many patches you need (10 instead of 5). I think for
libdrm this is supremely relevant, since most everyone will get their
commit rights by contributing already to the kernel or mesa and having
commit rights there already.

Anyway, I figured this is good to get the rules documented, even if
there's mostly not many rules.

Note: This references maintainers in a MAINTAINERS file, which needs
to be created first.

Note: With the gitlab migration the entire commit rights process is
still a bit up in the air. But gitlab commit rights and roles are
hierarchical, so we can do libdrm-only maintainer/commiter roles
("Owner" and "Developer" in gitlab-speak). This should avoid
conflating libdrm roles with mesa roles, useful for those pushing to
libdrm as primarily kernel contributors.

v2: Comments from Emil:
- Recommend subject prefix.
- Fix copypaste fumbles, this isn't igt/wayland ...

v3: Comments from Marek:
- libdrm moved to mesa, update the document. Atm the entire account
  request situation is entirely not clear for gitlab and mesa
  projects, so that's a bit up in the air. Also, should probably send
  an announcement to dri-devel@, which didn't happen.
- amd folks don't submit their patches to dri-devel, document that.
  Probably applies to other drivers too.

v4: Comments from Rob:
- Also include kernel/userspace in the commit counts criteria, due to
  libdrm's special role as a glue library.

v5: Summarize the irc discussion on gitlab roles in the commit message
a bit.

v6: Some grammer stuff from Eric E.

v7: Use --local in git config (Eric E.)

Cc: Dave Airlie 
Cc: Michel Dänzer 
Cc: Emil Velikov 
Cc: Marek Olšák 
Cc: Rob Clark 
Cc: Eric Engestrom 
Reviewed-by: Rob Clark  (v4)
Reviewed-by: Eric Engestrom  (v6)
Acked-by: Emil Velikov  (v6)
Acked-by: Marek Olšák  (v5)
References: 
https://gitlab.freedesktop.org/wayland/weston/blob/master/CONTRIBUTING.md
References: 
https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-misc.html#commit-rights
References: https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/CONTRIBUTING#n54
Signed-off-by: Daniel Vetter 
---
 CONTRIBUTING | 105 +++
 1 file changed, 105 insertions(+)
 create mode 100644 CONTRIBUTING

diff --git a/CONTRIBUTING b/CONTRIBUTING
new file mode 100644
index ..96f1e4fb0108
--- /dev/null
+++ b/CONTRIBUTING
@@ -0,0 +1,105 @@
+Contributing to libdrm
+==
+
+Submitting Patches
+--
+
+Patches should be sent to dri-de...@lists.freedesktop.org, using git
+send-email. For patches only touching driver specific code one of the driver
+mailing lists (like amd-gfx@lists.freedesktop.org) is also appropriate. See git
+documentation for help:
+
+http://git-scm.com/documentation
+
+Since dri-devel is a very busy mailing list please use --subject-prefix="PATCH
+libdrm" to make it easier to find libdrm patches. This is best done by running
+
+git config --local format.subjectprefix "PATCH libdrm"
+
+The first line of a commit message should contain a prefix indicating what part
+is affected by the patch followed by one sentence that describes the change. 
For
+examples:
+
+amdgpu: Use uint32_t i in amdgpu_find_bo_by_cpu_mapping
+
+The body of the commit message should describe what the patch changes and why,
+and also note any particular side effects. For a recommended reading on
+writing commit messages, see:
+
+http://who-t.blogspot.de/2009/12/on-commit-messages.html
+
+Your patches should also include a Signed-off-by line with your name and email
+address. If you're not the patch's original author, you should also gather
+S-o-b's by them (and/or whomever gave the patch to you.) The significance of
+this is that it certifies that you created the patch, that it was created under
+an appropriate open source license, or provided to you under those terms.  This
+lets us indicate a chain of responsibility for the copyright status of the 
code.
+For more details:
+
+https://developercertificate.org/
+
+We won't reject patches that lack S-o-b, but it is strongly recommended.
+
+Review and Merging
+--
+
+Patches should have at least one positive review (Reviewed-by: tag) or
+indication of approval (Acked-by: tag) before merging. For any code shared
+between drivers this is mandatory.
+
+Please note that kernel/userspace API header files have special rules, see
+include/drm/README.
+
+Coding style in the project loosely follows the CodingStyle of the linux 
kernel:
+
+https://www.kernel.org/doc/html/latest/process/coding-style.html?highligh

Re: [PATCH libdrm] amdgpu: When couldn't find bo, need to return error.

[Christian König]

Am 03.09.2018 um 09:16 schrieb Zhang, Jerry (Junwei):

On 09/03/2018 03:11 PM, Christian König wrote:

About master branch, needs someone's help with correct permission.

I've already took care of that on the weekend.


Thank you again.
BTW, how to apply that permission?


Previously that was done by opening a bugzilla ticket, but since the 
migration to gitlab that might be outdated now.


A good start is to go to https://gitlab.freedesktop.org and register an 
account, then somebody from the admin team needs to add that account to 
the appropriate groups.


Regards,
Christian.



Regards,
Jerry



Regards,
Christian.

Am 03.09.2018 um 03:42 schrieb Zhang, Jerry (Junwei):

On 09/01/2018 04:58 PM, Deng, Emily wrote:
Ok, then just ignore this patch. But seems didn't saw the patch on 
branch amd-staging-hybrid-master20180315.


Thanks to take care of this as well.

I'm waiting some verification, and now push the patch to internal 
staging branch

mainline will be pushed later for another verification.

About master branch, needs someone's help with correct permission.

Regards,
Jerry


Best wishes
Emily Deng


-Original Message-
From: Christian König 
Sent: Saturday, September 1, 2018 4:17 PM
To: Deng, Emily ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH libdrm] amdgpu: When couldn't find bo, need to 
return

error.

Am 01.09.2018 um 06:24 schrieb Emily Deng:

The startx will have segmant fault if return success.

SWDEV-163962

Change-Id: I56b189fa26efdcd1d96e5100af3f3e0b1208b0c3
Signed-off-by: Emily Deng 


Jerry already send a much better patch for this.


---
   amdgpu/amdgpu_bo.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c index
f25cacc..7e297fa 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -760,6 +760,7 @@ int

amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,

 uint64_t *offset_in_bo)
   {
   uint32_t i;
+    int r = 0;
   struct amdgpu_bo *bo;

   if (cpu == NULL || size == 0)
@@ -787,10 +788,11 @@ int

amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,

   } else {
   *buf_handle = NULL;
   *offset_in_bo = 0;
+    r = -errno;


errno doesn't contain any error in this case.


   }
pthread_mutex_unlock(&dev->bo_table_mutex);

-    return 0;
+    return r;
   }

   int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx





___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: add amdgpu_bo_inc_ref() function.

[Yu, Qiang]

Thanks Jerry.

Hi Christian,

This is an old patch back to 2016 reviewed in hybrid list. If you are OK with 
it,
would you please submit it to upstream?

Thanks,
Qiang


From: amd-gfx  on behalf of Zhang, Jerry 
(Junwei) 
Sent: Monday, September 3, 2018 3:18:14 PM
To: Yu, Qiang; amd-gfx@lists.freedesktop.org
Cc: Koenig, Christian; Deng, Hui
Subject: Re: [PATCH libdrm] amdgpu: add amdgpu_bo_inc_ref() function.

On 09/03/2018 02:55 PM, Qiang Yu wrote:
> For Pro OGL be able to work with upstream libdrm.
>
> Signed-off-by: Qiang Yu 
> Reviewed-by: Christian König 

I'm fine with that, not sure if mesa is going to use that as well.

Reviewed-by: Junwei Zhang 

Regards,
Jerry

> ---
>   amdgpu/amdgpu-symbol-check |  1 +
>   amdgpu/amdgpu.h| 15 ++-
>   amdgpu/amdgpu_bo.c |  6 ++
>   3 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
> index b5e4fe6..487610e 100755
> --- a/amdgpu/amdgpu-symbol-check
> +++ b/amdgpu/amdgpu-symbol-check
> @@ -15,6 +15,7 @@ amdgpu_bo_cpu_map
>   amdgpu_bo_cpu_unmap
>   amdgpu_bo_export
>   amdgpu_bo_free
> +amdgpu_bo_inc_ref
>   amdgpu_bo_import
>   amdgpu_bo_list_create
>   amdgpu_bo_list_destroy
> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
> index a8c353c..e1f93f8 100644
> --- a/amdgpu/amdgpu.h
> +++ b/amdgpu/amdgpu.h
> @@ -721,7 +721,20 @@ int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle 
> dev,
>   int amdgpu_bo_free(amdgpu_bo_handle buf_handle);
>
>   /**
> - * Request CPU access to GPU accessible memory
> + * Increase the reference count of a buffer object
> + *
> + * \param   bo - \c [in]  Buffer object handle to increase the reference 
> count
> + *
> + * \return   0 on success\n
> + *  <0 - Negative POSIX Error code
> + *
> + * \sa amdgpu_bo_alloc(), amdgpu_bo_free()
> + *
> +*/
> +int amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
> +
> +/**
> + * Request CPU access to GPU accessable memory
>*
>* \param   buf_handle - \c [in] Buffer handle
>* \param   cpu- \c [out] CPU address to be used for access
> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
> index a2fc525..dceab01 100644
> --- a/amdgpu/amdgpu_bo.c
> +++ b/amdgpu/amdgpu_bo.c
> @@ -438,6 +438,12 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle)
>   return 0;
>   }
>
> +int amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
> +{
> + atomic_inc(&bo->refcount);
> + return 0;
> +}
> +
>   int amdgpu_bo_cpu_map(amdgpu_bo_handle bo, void **cpu)
>   {
>   union drm_amdgpu_gem_mmap args;
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] gpu: drm: drm_mm: Fix a sleep-in-atomic-context bug in show_leaks()

[Daniel Vetter]

On Sat, Sep 01, 2018 at 01:32:54PM +0100, Chris Wilson wrote:
> Quoting Jia-Ju Bai (2018-09-01 13:20:41)
> > The driver may sleep with holding a spinlock.
> > 
> > The function call paths (from bottom to top) in Linux-4.16 are:
> > 
> > [FUNC] kmalloc(GFP_KERNEL)
> > drivers/gpu/drm/drm_mm.c, 130: 
> > kmalloc in show_leaks
> > drivers/gpu/drm/drm_mm.c, 913: 
> > show_leaks in drm_mm_takedown
> > drivers/gpu/drm/drm_vma_manager.c, 107: 
> > drm_mm_takedown in drm_vma_offset_manager_destroy
> > drivers/gpu/drm/drm_vma_manager.c, 106: 
> > _raw_write_lock in drm_vma_offset_manager_destroy
> > 
> > [FUNC] kmalloc(GFP_KERNEL)
> > drivers/gpu/drm/drm_mm.c, 130: 
> > kmalloc in show_leaks
> > drivers/gpu/drm/drm_mm.c, 913: 
> > show_leaks in drm_mm_takedown
> > drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c, 71: 
> > drm_mm_takedown in amdgpu_vram_mgr_fini
> > drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c, 70: 
> > spin_lock in amdgpu_vram_mgr_fini
> > 
> > [FUNC] kmalloc(GFP_KERNEL)
> > drivers/gpu/drm/drm_mm.c, 130: 
> > kmalloc in show_leaks
> > drivers/gpu/drm/drm_mm.c, 913: 
> > show_leaks in drm_mm_takedown
> > drivers/gpu/drm/ttm/ttm_bo_manager.c, 128: 
> > drm_mm_takedown in ttm_bo_man_takedown
> > drivers/gpu/drm/ttm/ttm_bo_manager.c, 126: 
> > spin_lock in ttm_bo_man_takedown
> > 
> > To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC.
> 
> The bug are above, since those spinlocks do not protect the data and
> imply use-after-free.

Adding amdgpu, since that's where the bug seems to be.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 00/13] remove_conflicting_framebuffers() cleanup

[Daniel Vetter]

On Sat, Sep 01, 2018 at 04:08:41PM +0200, Michał Mirosław wrote:
> This series cleans up duplicated code for replacing firmware FB
> driver with proper DRI driver and adds handover support to
> Tegra driver.
> 
> This is a sligtly updated version of a series sent on 24 Nov 2017.
> 
> ---
> v2:
>  - rebased on current drm-next
>  - dropped staging/sm750fb changes
>  - added kernel docs for DRM helpers
> v3:
>  - move kerneldoc to fbdev, where functions are implemented
>  - split kerneldoc for remove_conflicting_framebuffers()

Ah, that's not quite what I had in mind. I think having the docs (also) in
the drm helpers would be good, since that's where drm people will look,
and that's the function they'll call. I just wanted you to split the fbdev
and drm parts into 2 patches (since those are two different maintainers).

Anyway, this is ok too, so imo ready for merging. If you can resurrect the
drm docs (with a patch title of "drm/fb-helper: document fbdev remove
functions" or similar) that would be great.

Only thing we need for merging now is the ack from Bartlomiej.
-Daniel

>  - propagate return value in remove_conflicting_pci_framebuffers()
> 
> ---
> Michał Mirosław (13):
>   fbdev: show fbdev number for debugging
>   fbdev: allow apertures == NULL in remove_conflicting_framebuffers()
>   fbdev: add kerneldoc do remove_conflicting_framebuffers()
>   fbdev: add remove_conflicting_pci_framebuffers()
>   drm/amdgpu: use simpler remove_conflicting_pci_framebuffers()
>   drm/bochs: use simpler remove_conflicting_pci_framebuffers()
>   drm/cirrus: use simpler remove_conflicting_pci_framebuffers()
>   drm/mgag200: use simpler remove_conflicting_pci_framebuffers()
>   drm/radeon: use simpler remove_conflicting_pci_framebuffers()
>   drm/virtio: use simpler remove_conflicting_pci_framebuffers()
>   drm/vc4: use simpler remove_conflicting_framebuffers(NULL)
>   drm/sun4i: use simpler remove_conflicting_framebuffers(NULL)
>   drm/tegra: kick out simplefb
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 24 +
>  drivers/gpu/drm/bochs/bochs_drv.c| 18 +--
>  drivers/gpu/drm/cirrus/cirrus_drv.c  | 23 +
>  drivers/gpu/drm/mgag200/mgag200_drv.c| 21 +---
>  drivers/gpu/drm/mgag200/mgag200_main.c   |  9 
>  drivers/gpu/drm/radeon/radeon_drv.c  | 23 +
>  drivers/gpu/drm/sun4i/sun4i_drv.c| 18 +--
>  drivers/gpu/drm/tegra/drm.c  |  4 ++
>  drivers/gpu/drm/vc4/vc4_drv.c| 20 +---
>  drivers/gpu/drm/virtio/virtgpu_drm_bus.c | 24 ++---
>  drivers/video/fbdev/core/fbmem.c | 63 +++-
>  include/drm/drm_fb_helper.h  | 12 +
>  include/linux/fb.h   |  2 +
>  13 files changed, 89 insertions(+), 172 deletions(-)
> 
> -- 
> 2.18.0
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: add amdgpu_bo_inc_ref() function.

[Zhang, Jerry (Junwei)]

On 09/03/2018 02:55 PM, Qiang Yu wrote:

For Pro OGL be able to work with upstream libdrm.

Signed-off-by: Qiang Yu 
Reviewed-by: Christian König 


I'm fine with that, not sure if mesa is going to use that as well.

Reviewed-by: Junwei Zhang 

Regards,
Jerry


---
  amdgpu/amdgpu-symbol-check |  1 +
  amdgpu/amdgpu.h| 15 ++-
  amdgpu/amdgpu_bo.c |  6 ++
  3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
index b5e4fe6..487610e 100755
--- a/amdgpu/amdgpu-symbol-check
+++ b/amdgpu/amdgpu-symbol-check
@@ -15,6 +15,7 @@ amdgpu_bo_cpu_map
  amdgpu_bo_cpu_unmap
  amdgpu_bo_export
  amdgpu_bo_free
+amdgpu_bo_inc_ref
  amdgpu_bo_import
  amdgpu_bo_list_create
  amdgpu_bo_list_destroy
diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index a8c353c..e1f93f8 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -721,7 +721,20 @@ int amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
  int amdgpu_bo_free(amdgpu_bo_handle buf_handle);

  /**
- * Request CPU access to GPU accessible memory
+ * Increase the reference count of a buffer object
+ *
+ * \param   bo - \c [in]  Buffer object handle to increase the reference count
+ *
+ * \return   0 on success\n
+ *  <0 - Negative POSIX Error code
+ *
+ * \sa amdgpu_bo_alloc(), amdgpu_bo_free()
+ *
+*/
+int amdgpu_bo_inc_ref(amdgpu_bo_handle bo);
+
+/**
+ * Request CPU access to GPU accessable memory
   *
   * \param   buf_handle - \c [in] Buffer handle
   * \param   cpu- \c [out] CPU address to be used for access
diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
index a2fc525..dceab01 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -438,6 +438,12 @@ int amdgpu_bo_free(amdgpu_bo_handle buf_handle)
return 0;
  }

+int amdgpu_bo_inc_ref(amdgpu_bo_handle bo)
+{
+   atomic_inc(&bo->refcount);
+   return 0;
+}
+
  int amdgpu_bo_cpu_map(amdgpu_bo_handle bo, void **cpu)
  {
union drm_amdgpu_gem_mmap args;


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: When couldn't find bo, need to return error.

[Zhang, Jerry (Junwei)]

On 09/03/2018 03:11 PM, Christian König wrote:

About master branch, needs someone's help with correct permission.

I've already took care of that on the weekend.


Thank you again.
BTW, how to apply that permission?

Regards,
Jerry



Regards,
Christian.

Am 03.09.2018 um 03:42 schrieb Zhang, Jerry (Junwei):

On 09/01/2018 04:58 PM, Deng, Emily wrote:

Ok, then just ignore this patch. But seems didn't saw the patch on branch 
amd-staging-hybrid-master20180315.


Thanks to take care of this as well.

I'm waiting some verification, and now push the patch to internal staging branch
mainline will be pushed later for another verification.

About master branch, needs someone's help with correct permission.

Regards,
Jerry


Best wishes
Emily Deng


-Original Message-
From: Christian König 
Sent: Saturday, September 1, 2018 4:17 PM
To: Deng, Emily ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH libdrm] amdgpu: When couldn't find bo, need to return
error.

Am 01.09.2018 um 06:24 schrieb Emily Deng:

The startx will have segmant fault if return success.

SWDEV-163962

Change-Id: I56b189fa26efdcd1d96e5100af3f3e0b1208b0c3
Signed-off-by: Emily Deng 


Jerry already send a much better patch for this.


---
   amdgpu/amdgpu_bo.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c index
f25cacc..7e297fa 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -760,6 +760,7 @@ int

amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,

 uint64_t *offset_in_bo)
   {
   uint32_t i;
+int r = 0;
   struct amdgpu_bo *bo;

   if (cpu == NULL || size == 0)
@@ -787,10 +788,11 @@ int

amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,

   } else {
   *buf_handle = NULL;
   *offset_in_bo = 0;
+r = -errno;


errno doesn't contain any error in this case.


   }
   pthread_mutex_unlock(&dev->bo_table_mutex);

-return 0;
+return r;
   }

   int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

build on CentOS 7.3 hostos: too few arguments to function ‘notify_change’

[Acewind]

After download code from
https://github.com/GPUOpen-LibrariesAndSDKs/MxGPU-Virtualization,  I try to
compile MxGPU-Virtualization-master source on my CentOS 7.3 host, but got
an error.  Do I need to change the kernel of hostos?

[root@myhost drv]# uname -r
3.10.0-514.el7.x86_64

[root@myhost drv]# make
make -C /lib/modules/3.10.0-514.el7.x86_64/build
M=/home/astute/zwj/MxGPU-Virtualization-master/drv modules
make[1]: Entering directory `/usr/src/kernels/3.10.0-514.el7.x86_64'
  CC [M]  /home/astute/zwj/MxGPU-Virtualization-master/drv/gim_file.o
/home/astute/zwj/MxGPU-Virtualization-master/drv/gim_file.c: In function
‘file_truncate’:
/home/astute/zwj/MxGPU-Virtualization-master/drv/gim_file.c:87:2: error:
too few arguments to function ‘notify_change’
  ret = notify_change(file->f_path.dentry, &newattrs);
  ^
In file included from
/home/astute/zwj/MxGPU-Virtualization-master/drv/gim_file.c:23:0:
include/linux/fs.h:2579:12: note: declared here
 extern int notify_change(struct dentry *, struct iattr *, struct inode **);
^
make[2]: *** [/home/astute/zwj/MxGPU-Virtualization-master/drv/gim_file.o]
Error 1
make[1]: *** [_module_/home/astute/zwj/MxGPU-Virtualization-master/drv]
Error 2
make[1]: Leaving directory `/usr/src/kernels/3.10.0-514.el7.x86_64'
make: *** [all] Error 2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm] amdgpu: When couldn't find bo, need to return error.

[Christian König]

About master branch, needs someone's help with correct permission. 

I've already took care of that on the weekend.

Regards,
Christian.

Am 03.09.2018 um 03:42 schrieb Zhang, Jerry (Junwei):

On 09/01/2018 04:58 PM, Deng, Emily wrote:
Ok, then just ignore this patch. But seems didn't saw the patch on 
branch amd-staging-hybrid-master20180315.


Thanks to take care of this as well.

I'm waiting some verification, and now push the patch to internal 
staging branch

mainline will be pushed later for another verification.

About master branch, needs someone's help with correct permission.

Regards,
Jerry


Best wishes
Emily Deng


-Original Message-
From: Christian König 
Sent: Saturday, September 1, 2018 4:17 PM
To: Deng, Emily ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH libdrm] amdgpu: When couldn't find bo, need to 
return

error.

Am 01.09.2018 um 06:24 schrieb Emily Deng:

The startx will have segmant fault if return success.

SWDEV-163962

Change-Id: I56b189fa26efdcd1d96e5100af3f3e0b1208b0c3
Signed-off-by: Emily Deng 


Jerry already send a much better patch for this.


---
   amdgpu/amdgpu_bo.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c index
f25cacc..7e297fa 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -760,6 +760,7 @@ int

amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,

 uint64_t *offset_in_bo)
   {
   uint32_t i;
+    int r = 0;
   struct amdgpu_bo *bo;

   if (cpu == NULL || size == 0)
@@ -787,10 +788,11 @@ int

amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,

   } else {
   *buf_handle = NULL;
   *offset_in_bo = 0;
+    r = -errno;


errno doesn't contain any error in this case.


   }
   pthread_mutex_unlock(&dev->bo_table_mutex);

-    return 0;
+    return r;
   }

   int amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev,


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx