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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 07 Jan 2022, Lucas De Marchi <lucas.demarchi@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.

>
>>
>>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