On 01/15/2013 12:05 AM, Thierry Reding wrote:
> Add support for the B and C planes which support RGB and YUV pixel
> formats and can be used as overlays or hardware cursor.
I think "hardware cursor" has specific meaning for Tegra(e.g: Tegra30
has a 32x32 24bpp or 64x64 2bpp hardware cursor). So you may change it
to "hardware accelerated cursor"?
>
> Signed-off-by: Thierry Reding <thierry.reding at avionic-design.de>
> ---
[...]
> +
> +static const uint32_t plane_formats[] = {
> + DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_YUV422,
I haven't found something related with YUV format in this patch set. For
example, "tegra_dc_format" also doesn't take YUV into consideration. So
remove this line.
> +};
> +
> +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
> +{
> + unsigned int i;
> + int err = 0;
> +
> + for (i = 0; i < 2; i++) {
> + struct tegra_plane *plane;
> +
> + plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
> + if (!plane)
> + return -ENOMEM;
> +
> + plane->index = i;
I suggest to change this line to: "plane->index = i + 1;". This makes
the plane's index be consistent with Tegra's windows number. And also we
don't need to worry about passing "plane->index + 1" to some functions
which need to know which window is operating on.
> +
> + err = drm_plane_init(drm, &plane->base, 1 << dc->pipe,
> + &tegra_plane_funcs, plane_formats,
> + ARRAY_SIZE(plane_formats), false);
> + if (err < 0)
> + return err;
> + }
> +
> + return 0;
> +}
> +
[...]
>
> +int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index,
> + const struct tegra_dc_window *window)
> +{
> + unsigned h_offset, v_offset, h_size, v_size, h_dda, v_dda, bpp;
> + unsigned long value;
> +
> + bpp = window->bits_per_pixel / 8;
> +
> + value = WINDOW_A_SELECT << index;
> + tegra_dc_writel(dc, value, DC_CMD_DISPLAY_WINDOW_HEADER);
> +
> + tegra_dc_writel(dc, window->format, DC_WIN_COLOR_DEPTH);
> + tegra_dc_writel(dc, 0, DC_WIN_BYTE_SWAP);
> +
> + value = V_POSITION(window->dst.y) | H_POSITION(window->dst.x);
> + tegra_dc_writel(dc, value, DC_WIN_POSITION);
> +
> + value = V_SIZE(window->dst.h) | H_SIZE(window->dst.w);
> + tegra_dc_writel(dc, value, DC_WIN_SIZE);
> +
> + h_offset = window->src.x * bpp;
> + v_offset = window->src.y;
> + h_size = window->src.w * bpp;
> + v_size = window->src.h;
> +
> + value = V_PRESCALED_SIZE(v_size) | H_PRESCALED_SIZE(h_size);
> + tegra_dc_writel(dc, value, DC_WIN_PRESCALED_SIZE);
> +
> + h_dda = compute_dda_inc(window->src.w, window->dst.w, false, bpp);
> + v_dda = compute_dda_inc(window->src.h, window->dst.h, true, bpp);
> +
> + value = V_DDA_INC(v_dda) | H_DDA_INC(h_dda);
> + tegra_dc_writel(dc, value, DC_WIN_DDA_INC);
> +
> + h_dda = compute_initial_dda(window->src.x);
> + v_dda = compute_initial_dda(window->src.y);
> +
In current implementation, "compute_initial_dda" always returns zero. So
why we need it? Although according to TRM, typically we set H/V initial
dda to zero.
> + tegra_dc_writel(dc, h_dda, DC_WIN_H_INITIAL_DDA);
> + tegra_dc_writel(dc, v_dda, DC_WIN_V_INITIAL_DDA);
> +
> + tegra_dc_writel(dc, 0, DC_WIN_UV_BUF_STRIDE);
> + tegra_dc_writel(dc, 0, DC_WIN_BUF_STRIDE);
> +
> + tegra_dc_writel(dc, window->base, DC_WINBUF_START_ADDR);
> + tegra_dc_writel(dc, window->stride, DC_WIN_LINE_STRIDE);
> + tegra_dc_writel(dc, h_offset, DC_WINBUF_ADDR_H_OFFSET);
> + tegra_dc_writel(dc, v_offset, DC_WINBUF_ADDR_V_OFFSET);
> +
> + value = WIN_ENABLE;
> +
> + if (bpp < 24)
> + value |= COLOR_EXPAND;
> +
> + tegra_dc_writel(dc, value, DC_WIN_WIN_OPTIONS);
> +
> + /*
> + * Disable blending and assume Window A is the bottom-most window,
> + * Window C is the top-most window and Window B is in the middle.
> + */
> + tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_NOKEY);
> + tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_1WIN);
> +
> + switch (index) {
> + case 0:
> + tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_2WIN_X);
> + tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_2WIN_Y);
> + tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_3WIN_XY);
> + break;
> +
> + case 1:
> + tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_2WIN_X);
> + tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_2WIN_Y);
> + tegra_dc_writel(dc, 0x000000, DC_WIN_BLEND_3WIN_XY);
> + break;
> +
> + case 2:
> + tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_2WIN_X);
> + tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_2WIN_Y);
> + tegra_dc_writel(dc, 0xffff00, DC_WIN_BLEND_3WIN_XY);
> + break;
> + }
> +
> + value = (WIN_A_ACT_REQ << index) | (WIN_A_UPDATE << index);
> + tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL);
> +
> + return 0;
> +}
> +
> +unsigned int tegra_dc_format(uint32_t format)
> +{
> + switch (format) {
> + case DRM_FORMAT_XRGB8888:
> + return WIN_COLOR_DEPTH_B8G8R8A8;
> +
Just for curious, why we choose "WIN_COLOR_DEPTH_B8G8R8A8" while not
"WIN_COLOR_DEPTH_R8G8B8A8" here? I recall you and Stephen talked about
this last year but I still don't know the reason.
> + case DRM_FORMAT_RGB565:
> + return WIN_COLOR_DEPTH_B5G6R5;
> +
> + default:
> + break;
> + }
> +
> + WARN(1, "unsupported pixel format %u, using default\n", format);
> + return WIN_COLOR_DEPTH_B8G8R8A8;
> +}
> +
[...]
> +/* from dc.c */
> +extern unsigned int tegra_dc_format(uint32_t format);
> +extern int tegra_dc_setup_window(struct tegra_dc *dc, unsigned int index,
> + const struct tegra_dc_window *window);
> +
> struct tegra_output_ops {
> int (*enable)(struct tegra_output *output);
> int (*disable)(struct tegra_output *output);
>
