> -----Original Message-----
> From: Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx>
> Sent: Tuesday, April 7, 2020 7:42 PM
> To: Shankar, Uma <uma.shankar@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vehmanen, Kai <kai.vehmanen@xxxxxxxxx>
> Subject: Re: [PATCH v3] drm/i915/display: Enable DP Display Audio WA
>
> 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?
No major reasons as such. Will switch to unsigned int.
> > + 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"..?
Ok Sure, will improve this message.
> > + 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.
Sure, will add comments to make this more clear.
Thanks Kai for the feedback. Will address and send the next version.
Regards,
Uma Shankar
> Br, Kai
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
