[Intel-gfx] [PATCH v1] drm/i915: Add IOCTL Param to control data port coherency.

[Joonas Lahtinen]

Quoting Lis, Tomasz (2018-06-21 16:47:45)
> On 2018-06-21 08:39, Joonas Lahtinen wrote:
> > Quoting Tomasz Lis (2018-06-20 18:03:07)
> >>   int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
> >>                                      struct drm_file *file)
> >>   {
> >> +       struct drm_i915_private *dev_priv = to_i915(dev);
> >>          struct drm_i915_file_private *file_priv = file->driver_priv;
> >>          struct drm_i915_gem_context_param *args = data;
> >>          struct i915_gem_context *ctx;
> >> @@ -818,6 +837,16 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
> >>          case I915_CONTEXT_PARAM_PRIORITY:
> >>                  args->value = ctx->sched.priority;
> >>                  break;
> >> +       case I915_CONTEXT_PARAM_COHERENCY:
> >> +               /*
> >> +                * ENODEV if the feature is not supported. This removes the need
> >> +                * of separate IS_SUPPORTED parameter.
> >> +                */
> > Code speaks for itself, the comment is not needed.
> I don't think it is a good idea to limit comments. The current look of 
> the code makes it hard for anyone new to work on it, as the only 
> documentation is the history in mailing list.
> I don't think it's the correct approach. I believe comments should be 
> encouraged.

It's not a matter of opinion. Code should be written clear enough that
comments are not needed.

<SNIP>

> >> +       *cs++ = MI_LOAD_REGISTER_IMM(1);
> >> +       *cs++ = i915_mmio_reg_offset(reg);
> >> +       /* Enabling coherency means disabling the bit which forces it off */
> > Code is again very self explanatory without the comment.
> The logic is reversed, so that "enable" does a "disable". I believe the 
> comment does a great job of assuring the reader that this is not just a 
> coding mistake.
> 
> Do we have any official guidelines for limiting comments?

Yes, avoid where humanly possible. And when you can't avoid, it should
explain "why" not what. I don't see such cases in this patch.

<SNIP>

> > I'm thinking we should set this value when it has changed, when we insert the
> > requests into the command stream. So if you change back and forth, while
> > not emitting any requests, nothing really happens. If you change the value and
> > emit a request, we should emit a LRI before the jump to the commands.
> > Similary if you keep setting the value to the value it already was in,
> > nothing will happen, again.
> When I considered that, my way of reasoning was:
> If we execute the flag changing buffer right away, it may be sent to 
> hardware faster if there is no job in progress.
> If we use the lazy way, and trigger the change just before submission -  
> there will be additional conditions in submission code, plus the change 
> will be made when there is another job pending (though it's not a 
> considerable payload to just switch a flag).
> If user space switches the flag back and forth without much sense, then 
> there is something wrong with the user space driver, and it shouldn't be 
> up to kernel to fix that.

A few register writes appended before jumping to the BB should not be a
performance concern. There will definitely be more overhead in sending a
whole separate request, so I'm not sure I follow whole picture.

So I still think it's right thing to do to only emit the commands as a
prelude when needed.

Regards, Joonas