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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
