Nouveau - Jan 2022 - [RFC PATCH v3 2/3] drm: add support modifiers for drivers whose planes only support linear layout

On Mon, Jan 17, 2022 at 02:15:48PM +0900, Esaki Tomohito
wrote:
> On 2022/01/14 23:16, Andy Shevchenko wrote:
> > On Fri, Jan 14, 2022 at 07:17:52PM +0900, Tomohito Esaki wrote:
> > > The LINEAR modifier is advertised as default if a driver
doesn't specify
> > > modifiers.
> > 
> > ...
> > 
> > > +	const uint64_t default_modifiers[] = {
> > > +		DRM_FORMAT_MOD_LINEAR,
> > > +		DRM_FORMAT_MOD_INVALID
> > 
> > + Comma?
> 
> There is no mention in the coding style about adding/removing a comma to
the
> last element of an array. Is there a policy in drm driver?
> 
> I think the advantage of adding a comma to the last element of an array is
> that diff is only one line when an element is added to the end.
> However since INVALID is always the last element in the modifiers array, I
> think it can be either in this case.
> If there is a policy, I will match it.
Indeed, but there is a common sense. The idea behind (multi-line) definitions
that when next time somebody will add an element in the array, there are will
be:

a) no additional churn (like in case of this patch, if the item will be added
   at the bottom;

b) an element that may not be added behind the terminator, which will look
   weird.

That said, the question is if the element is terminator one or not, if not,
comma is better than no comma and vise versa.

-- 
With Best Regards,
Andy Shevchenko

On 2022/01/18 18:53, Andy Shevchenko wrote:
> On Mon, Jan 17, 2022 at 02:15:48PM +0900, Esaki Tomohito wrote:
>> On 2022/01/14 23:16, Andy Shevchenko wrote:
>>> On Fri, Jan 14, 2022 at 07:17:52PM +0900, Tomohito Esaki wrote:
>>>> The LINEAR modifier is advertised as default if a driver
doesn't specify
>>>> modifiers.
>>>
>>> ...
>>>
>>>> +	const uint64_t default_modifiers[] = {
>>>> +		DRM_FORMAT_MOD_LINEAR,
>>>> +		DRM_FORMAT_MOD_INVALID
>>>
>>> + Comma?
>>
>> There is no mention in the coding style about adding/removing a comma
to the
>> last element of an array. Is there a policy in drm driver?
>>
>> I think the advantage of adding a comma to the last element of an array
is
>> that diff is only one line when an element is added to the end.
>> However since INVALID is always the last element in the modifiers
array, I
>> think it can be either in this case.
>> If there is a policy, I will match it.
> 
> Indeed, but there is a common sense. The idea behind (multi-line)
definitions
> that when next time somebody will add an element in the array, there are
will
> be:
> 
> a) no additional churn (like in case of this patch, if the item will be
added
>     at the bottom;
> 
> b) an element that may not be added behind the terminator, which will look
>     weird.
> 
> That said, the question is if the element is terminator one or not, if not,
> comma is better than no comma and vise versa.
> 
Ah I see. In this case, DRM_FORMAT_MOD_INVALID is terminator, so it
should not have a comma.

Thanks
Tomohito Esaki

On Wed, Jan 19, 2022 at 11:35:22AM +0900, Esaki Tomohito
wrote:
> On 2022/01/18 18:53, Andy Shevchenko wrote:
> > On Mon, Jan 17, 2022 at 02:15:48PM +0900, Esaki Tomohito wrote:
> > > On 2022/01/14 23:16, Andy Shevchenko wrote:
> > > > On Fri, Jan 14, 2022 at 07:17:52PM +0900, Tomohito Esaki
wrote:
> > > > > The LINEAR modifier is advertised as default if a
driver doesn't specify
> > > > > modifiers.
> > > > 
> > > > ...
> > > > 
> > > > > +	const uint64_t default_modifiers[] = {
> > > > > +		DRM_FORMAT_MOD_LINEAR,
> > > > > +		DRM_FORMAT_MOD_INVALID
> > > > 
> > > > + Comma?
> > > 
> > > There is no mention in the coding style about adding/removing a
comma to the
> > > last element of an array. Is there a policy in drm driver?
> > > 
> > > I think the advantage of adding a comma to the last element of an
array is
> > > that diff is only one line when an element is added to the end.
> > > However since INVALID is always the last element in the modifiers
array, I
> > > think it can be either in this case.
> > > If there is a policy, I will match it.
> > 
> > Indeed, but there is a common sense. The idea behind (multi-line)
definitions
> > that when next time somebody will add an element in the array, there
are will
> > be:
> > 
> > a) no additional churn (like in case of this patch, if the item will
be added
> >     at the bottom;
> > 
> > b) an element that may not be added behind the terminator, which will
look
> >     weird.
> > 
> > That said, the question is if the element is terminator one or not, if
not,
> > comma is better than no comma and vise versa.
> 
> Ah I see. In this case, DRM_FORMAT_MOD_INVALID is terminator, so it
> should not have a comma.
Thanks for pointing this out. In this case we are good and any new item, AFAIU,
must be added before _INVALID one.

-- 
With Best Regards,
Andy Shevchenko