Hi,
thanks for the feedback.
Am 29.04.19 um 21:58 schrieb Sam Ravnborg:
Hi Thomas.
Some minor things and some bikeshedding too.
One general^Wbikeshedding thing - unint32_t is used in many places. And then s64 in one place. Seems like two concepts are mixed. Maybe be consistent and use u32, s32 everywhere?
The DRM API already has a mixture of such types and I tried to use the type that best fits the current context. But yeah, I don't mind some consistency. I'll see if I can replace some of these instances.
+config DRM_GEM_VRAM_HELPER
- bool
- depends on DRM
- select DRM_VRAM_HELPER
- help
Choose this if you need the GEM VRAM helper functionsI cannot remember how select will deal with symbols whos has a "depends on". But if I recall correct then the "depends on" will be ignored or in best case trigger a warning. In other words - when we have symbols we select they should not have a depends on.
What can be done is something like:
symbol foo bool
symbol bar depends on foo
symbol foobar bool "This is what you need - select me" select foo
So when one chooses "foobar" then we will select "foo" and this will satisfy bar.
But maybe this rambling is irrelevant - maybe check what we do with other selectable symbols in DRM.
It may not strictly be necessary here, but the other helpers' symbols depend on DRM. I'd like to keep it consistent unless there's a strong reason not to.
+/**
- DOC: overview
- This library provides a GEM object that is backed by VRAM. It
- can be used for simple framebuffer devices with dedicated memory.
- */
It is likely only me, but... I could use a short explanation what is GEM and maybe also VRAM.
VRAM as video RAM, but maybe there is more constraints? (When I first looked at DRM I wondered what you used Virtual RAM for. But thats a long time ago so it counts only as a funny story.
OK :)
+/*
- Buffer-object helpers
- */
+/**
- struct drm_gem_vram_object - GEM object backed by VRAM
- @gem: GEM object
- @bo: TTM buffer object
- @kmap: Mapping information for @bo
- @placement: TTM placement information. Supported placements are \
- %TTM_PL_VRAM and %TTM_PL_SYSTEM
- @placements: TTM placement information.
- @pin_count: Pin counter
- The type struct drm_gem_vram_object represents a GEM object that is
- backed by VRAM. It can be used for simple frambuffer devices with
- dedicated memory. The buffer object can be evicted to system memory if
- video memory becomes scarce.
- */
+struct drm_gem_vram_object {
struct drm_gem_object gem;struct ttm_buffer_object bo;struct ttm_bo_kmap_obj kmap;- /* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
struct ttm_placement placement;struct ttm_place placements[3];int pin_count;+};
Use tabs for indent - not spaces. Ask checkpatch if anything similar needs to be adjusted.
Oh well, I should have checked this. Thanks for reporting.
+/**
- Returns the container of type &struct drm_gem_vram_object
- for field bo.
- @bo: the VRAM buffer object
- Returns: The containing GEM VRAM object
- */
+static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
- struct ttm_buffer_object *bo)
+{
- return container_of(bo, struct drm_gem_vram_object, bo);
+}
Indent funny. USe same indent as used in other parts of file for function arguments.
If I put the argument next to the function's name, it will exceed the 80-character limit. From the coding-style document, I could not see what to do in this case. One solution would move the return type to a separate line before the function name. I've not seen that anywhere in the source code, so moving the argument onto a separate line and indenting by one tab appears to be the next best solution. Please let me know if there's if there's a preferred style for cases like this one.
Best regards Thomas
+/**
- Returns the container of type &struct drm_gem_vram_object
- for field gem.
- @gem: the GEM object
- Returns: The containing GEM VRAM object
- */
+static inline struct drm_gem_vram_object* drm_gem_vram_of_gem(
- struct drm_gem_object *gem)
+{
- return container_of(gem, struct drm_gem_vram_object, gem);
+}
ditto
+struct drm_gem_vram_object* drm_gem_vram_create(struct drm_device *dev,
struct ttm_bo_device* bdev,unsigned long size,uint32_t pg_align,bool interruptible);Here is is "normal"