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 functions
>> +
> I 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"
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
