Virtualization - May 2019 - [PATCH v3 01/19] drm: Add |struct drm_gem_vram_object| and helpers

Hi Christian,

would you review the whole patch set? Daniel mentioned that he'd prefer
to leave the review to memory-mgmt developers.

Best regards
Thomas

Am 30.04.19 um 11:35 schrieb Koenig, Christian:
> Am 30.04.19 um 11:23 schrieb Sam Ravnborg:
>> [CAUTION: External Email]
>>
>> Hi Thomas.
>>
>>>>> +
>>>>> +/**
>>>>> + * 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.
>> Readability has IMO higher priority than some limit of 80 chars.
>> And it hurts readability (at least my OCD) when style changes
>> as you do with indent here. So my personal preference is to fix
>> indent and accect longer lines.
> 
> In this case the an often used convention (which is also kind of 
> readable) is to add a newline after the return values, but before the 
> function name. E.g. something like this:
> 
> static inline struct drm_gem_vram_object*
> drm_gem_vram_of_bo(struct ttm_buffer_object *bo)
> 
> Regards,
> Christian.
> 
>>
>> But you ask for a preferred style - which I do not think we have in
this
>> case. So it boils down to what you prefer.
>>
>> Enough bikeshedding, thanks for the quick response.
>>
>>          Sam
> 
-- 
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)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20190503/ffe43a7d/attachment.sig>

On Fri, May 3, 2019 at 12:15 PM Thomas Zimmermann <tzimmermann at suse.de>
wrote:
>
> Hi Christian,
>
> would you review the whole patch set? Daniel mentioned that he'd prefer
> to leave the review to memory-mgmt developers.
I think Noralf Tronnes or Gerd Hoffmann would also make good reviewers
for this, fairly close to what they've been working on in the past.
-Daniel
>
> Best regards
> Thomas
>
> Am 30.04.19 um 11:35 schrieb Koenig, Christian:
> > Am 30.04.19 um 11:23 schrieb Sam Ravnborg:
> >> [CAUTION: External Email]
> >>
> >> Hi Thomas.
> >>
> >>>>> +
> >>>>> +/**
> >>>>> + * 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.
> >> Readability has IMO higher priority than some limit of 80 chars.
> >> And it hurts readability (at least my OCD) when style changes
> >> as you do with indent here. So my personal preference is to fix
> >> indent and accect longer lines.
> >
> > In this case the an often used convention (which is also kind of
> > readable) is to add a newline after the return values, but before the
> > function name. E.g. something like this:
> >
> > static inline struct drm_gem_vram_object*
> > drm_gem_vram_of_bo(struct ttm_buffer_object *bo)
> >
> > Regards,
> > Christian.
> >
> >>
> >> But you ask for a preferred style - which I do not think we have
in this
> >> case. So it boils down to what you prefer.
> >>
> >> Enough bikeshedding, thanks for the quick response.
> >>
> >>          Sam
> >
>
> --
> 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)
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Am 03.05.19 um 14:01 schrieb Daniel Vetter:
> [CAUTION: External Email]
>
> On Fri, May 3, 2019 at 12:15 PM Thomas Zimmermann <tzimmermann at
suse.de> wrote:
>> Hi Christian,
>>
>> would you review the whole patch set? Daniel mentioned that he'd
prefer
>> to leave the review to memory-mgmt developers.
> I think Noralf Tronnes or Gerd Hoffmann would also make good reviewers
> for this, fairly close to what they've been working on in the past.
I will try to take another look next week. Busy as usual here.

Christian.
> -Daniel
>
>> Best regards
>> Thomas
>>
>> Am 30.04.19 um 11:35 schrieb Koenig, Christian:
>>> Am 30.04.19 um 11:23 schrieb Sam Ravnborg:
>>>> [CAUTION: External Email]
>>>>
>>>> Hi Thomas.
>>>>
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * 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.
>>>> Readability has IMO higher priority than some limit of 80
chars.
>>>> And it hurts readability (at least my OCD) when style changes
>>>> as you do with indent here. So my personal preference is to fix
>>>> indent and accect longer lines.
>>> In this case the an often used convention (which is also kind of
>>> readable) is to add a newline after the return values, but before
the
>>> function name. E.g. something like this:
>>>
>>> static inline struct drm_gem_vram_object*
>>> drm_gem_vram_of_bo(struct ttm_buffer_object *bo)
>>>
>>> Regards,
>>> Christian.
>>>
>>>> But you ask for a preferred style - which I do not think we
have in this
>>>> case. So it boils down to what you prefer.
>>>>
>>>> Enough bikeshedding, thanks for the quick response.
>>>>
>>>>           Sam
>> --
>> 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)
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch