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)