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
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel