On Tue, Mar 02, 2021 at 03:44:44PM +0300, Dmitry Osipenko wrote:
> Display controller (DC) performs isochronous memory transfers, and thus,
> has a requirement for a minimum memory bandwidth that shall be fulfilled,
> otherwise framebuffer data can't be fetched fast enough and this results
> in a DC's data-FIFO underflow that follows by a visual corruption.
>
> The Memory Controller drivers provide facility for memory bandwidth
> management via interconnect API. Let's wire up the interconnect API
> support to the DC driver in order to fix the distorted display output
> on T30 Ouya, T124 TK1 and other Tegra devices.
I did a read on the code. I have put some thoughts and nits inbetween the
diff, but here are more general questions about the patch:
Is there a reason why the bandwidth is allocated per plane instead of just
using one peak and average for the whole configuration? Or is there a need
to expose all the planes as interconnect channels and allocate their
bandwidth individually?
>From algorithmic part I see that the plane overlaps are calculated twice
(A vs B and later B vs A). The cursor plane is ignored, but nevertheless
its overlap mask is calculated before being thrown away. The bandwidths
are also calculated twice: once for pre-commit and then again for
post-commit. Is setting bandwidth for an interconnect expensive when
re-setting a value that was already set? The code seems to avoid this
case, but maybe unnecessarily?
[...cut the big and interesting part...]
[...]
> @@ -65,7 +75,9 @@ struct tegra_dc_soc_info {
> unsigned int num_overlay_formats;
> const u64 *modifiers;
> bool has_win_a_without_filters;
> + bool has_win_b_vfilter_mem_client;
> bool has_win_c_without_vert_filter;
> + unsigned int plane_tiled_memory_bandwidth_x2;
This looks more like bool in the code using it.
[...]
> --- a/drivers/gpu/drm/tegra/plane.c
> +++ b/drivers/gpu/drm/tegra/plane.c
[...]
> +static int tegra_plane_check_memory_bandwidth(struct drm_plane_state *state)
The function's body looks more like 'update' or 'recalculate' rather
than 'check' the memory bandwidth.
> + struct tegra_plane_state *tegra_state = to_tegra_plane_state(state);
> + unsigned int i, bpp, bpp_plane, dst_w, src_w, src_h, mul;
> + const struct tegra_dc_soc_info *soc;
> + const struct drm_format_info *fmt;
> + struct drm_crtc_state *crtc_state;
> + u32 avg_bandwidth, peak_bandwidth;
> +
> + if (!state->visible)
> + return 0;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(state->state, state->crtc);
> + if (!crtc_state)
> + return -EINVAL;
> +
> + src_w = drm_rect_width(&state->src) >> 16;
> + src_h = drm_rect_height(&state->src) >> 16;
> + dst_w = drm_rect_width(&state->dst);
> +
> + fmt = state->fb->format;
> + soc = to_tegra_dc(state->crtc)->soc;
> +
> + /*
> + * Note that real memory bandwidth vary depending on format and
> + * memory layout, we are not taking that into account because small
> + * estimation error isn't important since bandwidth is rounded up
> + * anyway.
> + */
> + for (i = 0, bpp = 0; i < fmt->num_planes; i++) {
> + bpp_plane = fmt->cpp[i] * 8;
Nit: you could declare the bpp_plane here.
> + /*
> + * Sub-sampling is relevant for chroma planes only and vertical
> + * readouts are not cached, hence only horizontal sub-sampling
> + * matters.
> + */
> + if (i > 0)
> + bpp_plane /= fmt->hsub;
> +
> + bpp += bpp_plane;
> + }
> +
> + /*
> + * Horizontal downscale takes extra bandwidth which roughly depends
> + * on the scaled width.
> + */
> + if (src_w > dst_w)
> + mul = (src_w - dst_w) * bpp / 2048 + 1;
> + else
> + mul = 1;
Does it really need more bandwidth to scale down? Does it read the same
data multiple times just to throw it away?
> + /* average bandwidth in bytes/s */
> + avg_bandwidth = src_w * src_h * bpp / 8 * mul;
> + avg_bandwidth *= drm_mode_vrefresh(&crtc_state->mode);
> +
> + /* mode.clock in kHz, peak bandwidth in kbit/s */
> + peak_bandwidth = crtc_state->mode.clock * bpp * mul;
I would guess that (src_w * bpp / 8) needs rounding up unless the plane
is continuous. Or you could just add the max rounding error here and
get a safe overestimated value. Maybe this would be better done when
summing per-plane widths.
> + /* ICC bandwidth in kbyte/s */
> + peak_bandwidth = kbps_to_icc(peak_bandwidth);
> + avg_bandwidth = Bps_to_icc(avg_bandwidth);
This could be merged with the assignments after the following 'if' block.
> + /*
> + * Tegra30/114 Memory Controller can't interleave DC memory requests
> + * and DC uses 16-bytes atom for the tiled windows, while DDR3 uses 32
> + * bytes atom. Hence there is x2 memory overfetch for tiled framebuffer
> + * and DDR3 on older SoCs.
> + */
> + if (soc->plane_tiled_memory_bandwidth_x2 &&
> + tegra_state->tiling.mode == TEGRA_BO_TILING_MODE_TILED) {
> + peak_bandwidth *= 2;
> + avg_bandwidth *= 2;
> + }
> +
> + tegra_state->peak_memory_bandwidth = peak_bandwidth;
> + tegra_state->avg_memory_bandwidth = avg_bandwidth;
> +
> + return 0;
> +}
[...]
> +static const char * const tegra_plane_icc_names[] = {
> + "wina", "winb", "winc", "", "", "", "cursor",
> +};
> +
> +int tegra_plane_interconnect_init(struct tegra_plane *plane)
> +{
> + const char *icc_name = tegra_plane_icc_names[plane->index];
Is plane->index guaranteed to be <= 6? I would guess so, but maybe
BUILD_BUG_ON(sizeof(icc_names)==TEGRA_DC_LEGACY_PLANES_NUM) or some
other check could document this?
[...]
Best Regards
Michał Mirosław
