[Intel-gfx] [PATCH 1/1] drm/i915: Drop unused register definitions

[Lucas De Marchi]

On Mon, Jan 10, 2022 at 12:55:36PM +0200, Jani Nikula wrote:
>On Fri, 07 Jan 2022, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
>> On Fri, Jan 07, 2022 at 12:46:48PM +0200, Jani Nikula wrote:
>>>I still tell people to 1) split out register definitions to separate
>>>patches, and 2) add macros for the entire feature and full contents for
>>>each register, even if they remain unused.
>>>
>>>One of the main reasons is review economics. It's pretty easy to look at
>>>a patch adding register contents, review it against the bspec and be
>>>done with it. And while you have the right bspec page open, it only
>>>takes a very short time to add and review the entire register, instead
>>>of having to return to it later. A patch adding just the registers could
>>>get reviewed and merged at v1. It's one less patch the developer has to
>>>keep carrying and rebasing, and it's one less portion the reviewer has
>>>to worry about.
>>
>> this failed multiple times though, so I'm on the other side of the fence
>> and think this actually makes things worse. Main reasons is because we
>> have several registers doing things like:
>>
>> 	#define BLA(idx)	REG_BIT(idx * 2 + 1)
>>
>> And it's easy to get this wrong when it was not tested. It may be for
>> example that one phy or port doesn't follow the same logic.
>> When I review code I prefer reviewing code people actually tested.
>>
>> It's less of an issue when it's a 1:1 map from bspec, but for a lot of
>> registers we need just 1 or 2 bits, e.g. for workarounds. Being able to
>> filter out workarounds we don't need because we don't even define the
>> register/bit is also another con to defining the complete register in a
>> separate patch.
>>
>>
>> So from my POV, the cons outweigtht the pros.
>
>Okay, let's not forget this part of the conversation, but let's also not
>let this block the cleanup. We don't have to do a blanket removal of
>unused stuff before splitting the file up, and we don't have to decide
>how we how we approach this in the future before that either, i.e. let's
>get the uncontroversial stuff merged.

agreed,

Lucas De Marchi

>
>>
>>>
>>>Overall it's about getting the easy stuff done and behind you first. And
>>>that's a huge part of my whole approach to kernel development, and what
>>>I try to tell others to follow.
>>>
>>>I also think the documentation aspect is still valid, and especially so
>>>for older hardware. It may be fine to remove some of the accumulated
>>>cruft, *after* the register macros have been split up to smaller files
>>>by functionality. But I don't think it should be an indiscriminate mass
>>>removal of macros. For example, I don't think I want any of the sideband
>>>or VGA or PCI register macros removed.
>>>
>>>Bottom line, I don't mind adding or having unused register macros at
>>>all. Heck, I'd be on board for switching to auto-generated register
>>>macros with absolutely everything.
>>
>> if it can be generated.... Then we'd have some additional headers for
>> the accessor functions that deal with index math to the to the right
>> bits or the  right register instance.
>
>Yeah, it's somewhat of an unicorn I guess.
>
>BR,
>Jani.
>
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center