Hi,
thanks Uma! It's good to see the implementation is this localized and
doesn't need changes elsewhere. Other reviewers already covered most
parts, but a few notes:
On Tue, 7 Apr 2020, Uma Shankar wrote:
> + struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> + enum pipe pipe = crtc->pipe;
> + u32 link_clks_available, link_clks_required;
> + u32 tu_data, tu_line, link_clks_active;
> + u32 hblank_rise, hblank_early_prog;
> + u32 h_active, h_total, hblank_delta, pixel_clk, v_total;
> + u32 fec_coeff, refresh_rate, cdclk;
hmm, minor thing, but why are these u32 and not just unsigned ints?
> + if (!(h_active && crtc_state->port_clock && crtc_state->lane_count &&
> + crtc_state->pipe_bpp && cdclk)) {
> + drm_err(&i915->drm, "Null Parameters received\n");
> + WARN_ON(1);
> + return -EINVAL;
This is still not very informative. "Invalid parameters for
hblank_early"..?
> + if (samples_room < 3) {
> + *val &= ~NUMBER_SAMPLES_PER_LINE_MASK(pipe);
> + *val |= NUMBER_SAMPLES_PER_LINE(pipe, samples_room);
> + } else {
> + *val &= ~NUMBER_SAMPLES_PER_LINE_MASK(pipe);
> + *val |= NUMBER_SAMPLES_PER_LINE(pipe, 0x0);
> + }
This is a bit hard to follow in terms of logic. Maybe worth a comment
that 0x0 means to take all samples from the buffer.
Br, Kai
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
