[Intel-gfx] [PATCH] drm/i915: Rename global i915 to i915_modparams

[Joonas Lahtinen]

On Tue, 2017-09-19 at 16:15 +0300, Ville Syrjälä wrote:
> On Tue, Sep 19, 2017 at 04:07:17PM +0300, Joonas Lahtinen wrote:
> > On Tue, 2017-09-19 at 13:22 +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 19, 2017 at 11:24:41AM +0300, Joonas Lahtinen wrote:
> > > > On Mon, 2017-09-18 at 22:07 +0200, Michal Wajdeczko wrote:
> > > > > On Mon, 18 Sep 2017 21:11:40 +0200, Jani Nikula <jani.nikula at intel.com>  
> > > > > wrote:
> > > > > 
> > > > > > On Mon, 18 Sep 2017, Michal Wajdeczko <michal.wajdeczko at intel.com> wrote:
> > > > > > > Our global struct with params is named exactly the same way
> > > > > > > as new preferred name for the drm_i915_private function parameter.
> > > > > > > To avoid such name reuse lets use different name for the global.
> > > > > > > 
> > > > > > > v4: introduction of mkwrite()
> > > > > > 
> > > > > > Why?
> > > > > > 
> > > > > > I don't know what you're trying to achieve with the mkwrite() stuff (the
> > > > > 
> > > > > I was trying to buy at least one more vote, as discussed on IRC
> > > > > 
> > > > > <quote>
> > > > > [14:23:36] <dolphin> I'll be glad to vote for i915_modparams +  
> > > > > i915_modparams_mkwrite()
> > > > > <quote/>
> > > > > 
> > > > > > commit message would be the perfect place to explain that) but no matter
> > > > > > what it should IMO be a separate patch.
> > > > > > 
> > > > > > I think the simple s/i915/i915_modparams/ would be fine, and we could
> > > > > > move on.
> > > > > 
> > > > > Note that it all started with this idea.
> > > > > See https://patchwork.freedesktop.org/patch/176409/
> > > > > 
> > > > 
> > > > I agree with Jani that the pure rename should be its own patch. That'll
> > > > make review much easier. Then have a follow-up that introduces
> > > > _mkwrite() and as a bonus makes the struct const or at least makes
> > > > sparse complain.
> > > 
> > > I know we abuse the const+mkwrite type of thing for the device info, but
> > > I'm not sure how safe that actually is on account of the compiler being
> > > free to assume that const stuff doesn't generally change. I guess if the
> > > mkwrite thing happens at some early controlled point it's going to be OK,
> > > but if it starts happening at some randomish times we might not be so
> > > lucky.
> > 
> > I see this more as a reason to introduce it.
> 
> Introduce what exactly? A bug due to compiler optimizing away some read of
> the variable because it can assume that it didn't change?

"Introduce enforcement of taking special action to acknowledge
consideration before writing to i915_modparams, to avoid bugs due to
writing to variables that are not expected to change."

  -- Joonas Lahtinen, 2017

If somebody is still unclear what I'm after, I'm out of words.

> I think this needs to be well thought out to make sure we don't end up
> with some impossible looking bugs.

That's why we're discussing it. My bet is most of the potentially
troublesome variables belong to to the device state anyway (vs. module
state). It's just a dumping ground of global variables currently.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation