Hi Anitha.
On Mon, Aug 10, 2020 at 02:53:38PM -0700, Anitha Chrisanthus wrote:
> This is a basic KMS atomic modesetting display driver for KeemBay family of
> SOCs. Driver has no 2D or 3D graphics.It calls into the ADV bridge
> driver at the connector level.
>
> Single CRTC with LCD controller->mipi DSI-> ADV bridge
>
> Only 1080p resolution and single plane is supported at this time.
>
> v2: moved extern to .h, removed license text
> use drm_dev_init, upclassed dev_private, removed HAVE_IRQ.
>
> v3: Squashed all 59 commits to one
>
> v4: review changes from Sam Ravnborg
> renamed dev_p to kmb
> moved clocks under kmb_clock, consolidated clk initializations
> use drmm functions
> use DRM_GEM_CMA_DRIVER_OPS_VMAP
>
> v5: corrected spellings
> v6: corrected checkpatch warnings
I have asked a few persons to review, but they lack time at the moment.
So I will continue this monolouge of review feedback.
I had hoped to provide all feedback in a few itearations, but I continue
to find more stuff.
First part of this round is some feedback on plane stuff
Sam
> diff --git a/drivers/gpu/drm/kmb/kmb_plane.c b/drivers/gpu/drm/kmb/kmb_plane.c
> new file mode 100644
> index 0000000..31bcba0
> --- /dev/null
> +++ b/drivers/gpu/drm/kmb/kmb_plane.c
> @@ -0,0 +1,519 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright © 2018-2020 Intel Corporation
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_data/simplefb.h>
Not used I think.
> +#include <video/videomode.h>
Not used I hope.
> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_managed.h>
> +
> +#include "kmb_crtc.h"
> +#include "kmb_drv.h"
> +#include "kmb_plane.h"
> +#include "kmb_regs.h"
> +
> +struct layer_status plane_status[KMB_MAX_PLANES];
Embed plane_status in struct kmb_plane so you avoid an extra statically
allocated variable here. And it is then together with other relevant
data.
> +const u32 layer_irqs[] = {
> + LCD_INT_VL0,
> + LCD_INT_VL1,
> + LCD_INT_GL0,
> + LCD_INT_GL1
> +};
> +
> +static unsigned int check_pixel_format(struct drm_plane *plane, u32 format)
> +{
> + int i;
> +
> + for (i = 0; i < plane->format_count; i++) {
> + if (plane->format_types[i] == format)
> + return 0;
> + }
> + return -EINVAL;
> +}
> +
> +static int kmb_plane_atomic_check(struct drm_plane *plane,
> + struct drm_plane_state *state)
> +{
> + struct drm_framebuffer *fb;
> + int ret;
> +
> + fb = state->fb;
> + if (!fb || !state->crtc)
> + return 0;
> +
Should drm_atomic_helper_check_plane_state() be called here?
> + ret = check_pixel_format(plane, fb->format->format);
> + if (ret)
> + return ret;
> +
> + if (state->crtc_w > KMB_MAX_WIDTH || state->crtc_h > KMB_MAX_HEIGHT)
> + return -EINVAL;
> + if (state->crtc_w < KMB_MIN_WIDTH || state->crtc_h < KMB_MIN_HEIGHT)
> + return -EINVAL;
> + return 0;
> +}
> +
> +static void kmb_plane_atomic_disable(struct drm_plane *plane,
> + struct drm_plane_state *state)
> +{
> + struct kmb_plane *kmb_plane = to_kmb_plane(plane);
> + int plane_id = kmb_plane->id;
> +
> + switch (plane_id) {
> + case LAYER_0:
> + plane_status[plane_id].ctrl = LCD_CTRL_VL1_ENABLE;
> + break;
> + case LAYER_1:
> + plane_status[plane_id].ctrl = LCD_CTRL_VL2_ENABLE;
> + break;
> + case LAYER_2:
> + plane_status[plane_id].ctrl = LCD_CTRL_GL1_ENABLE;
> + break;
> + case LAYER_3:
> + plane_status[plane_id].ctrl = LCD_CTRL_GL2_ENABLE;
> + break;
> + }
> +
> + plane_status[plane_id].disable = true;
> +}
> +
> +unsigned int set_pixel_format(u32 format)
> +{
> + unsigned int val = 0;
> +
> + switch (format) {
> + /* planar formats */
> + case DRM_FORMAT_YUV444:
> + val = LCD_LAYER_FORMAT_YCBCR444PLAN | LCD_LAYER_PLANAR_STORAGE;
> + break;
> + case DRM_FORMAT_YVU444:
> + val = LCD_LAYER_FORMAT_YCBCR444PLAN | LCD_LAYER_PLANAR_STORAGE
> + | LCD_LAYER_CRCB_ORDER;
> + break;
> + case DRM_FORMAT_YUV422:
> + val = LCD_LAYER_FORMAT_YCBCR422PLAN | LCD_LAYER_PLANAR_STORAGE;
> + break;
> + case DRM_FORMAT_YVU422:
> + val = LCD_LAYER_FORMAT_YCBCR422PLAN | LCD_LAYER_PLANAR_STORAGE
> + | LCD_LAYER_CRCB_ORDER;
> + break;
> + case DRM_FORMAT_YUV420:
> + val = LCD_LAYER_FORMAT_YCBCR420PLAN | LCD_LAYER_PLANAR_STORAGE;
> + break;
> + case DRM_FORMAT_YVU420:
> + val = LCD_LAYER_FORMAT_YCBCR420PLAN | LCD_LAYER_PLANAR_STORAGE
> + | LCD_LAYER_CRCB_ORDER;
> + break;
> + case DRM_FORMAT_NV12:
> + val = LCD_LAYER_FORMAT_NV12 | LCD_LAYER_PLANAR_STORAGE;
> + break;
> + case DRM_FORMAT_NV21:
> + val = LCD_LAYER_FORMAT_NV12 | LCD_LAYER_PLANAR_STORAGE
> + | LCD_LAYER_CRCB_ORDER;
> + break;
> + /* packed formats */
> + /* looks hw requires B & G to be swapped when RGB */
> + case DRM_FORMAT_RGB332:
> + val = LCD_LAYER_FORMAT_RGB332 | LCD_LAYER_BGR_ORDER;
> + break;
> + case DRM_FORMAT_XBGR4444:
> + val = LCD_LAYER_FORMAT_RGBX4444;
> + break;
> + case DRM_FORMAT_ARGB4444:
> + val = LCD_LAYER_FORMAT_RGBA4444 | LCD_LAYER_BGR_ORDER;
> + break;
> + case DRM_FORMAT_ABGR4444:
> + val = LCD_LAYER_FORMAT_RGBA4444;
> + break;
> + case DRM_FORMAT_XRGB1555:
> + val = LCD_LAYER_FORMAT_XRGB1555 | LCD_LAYER_BGR_ORDER;
> + break;
> + case DRM_FORMAT_XBGR1555:
> + val = LCD_LAYER_FORMAT_XRGB1555;
> + break;
> + case DRM_FORMAT_ARGB1555:
> + val = LCD_LAYER_FORMAT_RGBA1555 | LCD_LAYER_BGR_ORDER;
> + break;
> + case DRM_FORMAT_ABGR1555:
> + val = LCD_LAYER_FORMAT_RGBA1555;
> + break;
> + case DRM_FORMAT_RGB565:
> + val = LCD_LAYER_FORMAT_RGB565 | LCD_LAYER_BGR_ORDER;
> + break;
> + case DRM_FORMAT_BGR565:
> + val = LCD_LAYER_FORMAT_RGB565;
> + break;
> + case DRM_FORMAT_RGB888:
> + val = LCD_LAYER_FORMAT_RGB888 | LCD_LAYER_BGR_ORDER;
> + break;
> + case DRM_FORMAT_BGR888:
> + val = LCD_LAYER_FORMAT_RGB888;
> + break;
> + case DRM_FORMAT_XRGB8888:
> + val = LCD_LAYER_FORMAT_RGBX8888 | LCD_LAYER_BGR_ORDER;
> + break;
> + case DRM_FORMAT_XBGR8888:
> + val = LCD_LAYER_FORMAT_RGBX8888;
> + break;
> + case DRM_FORMAT_ARGB8888:
> + val = LCD_LAYER_FORMAT_RGBA8888 | LCD_LAYER_BGR_ORDER;
> + break;
> + case DRM_FORMAT_ABGR8888:
> + val = LCD_LAYER_FORMAT_RGBA8888;
> + break;
> + }
> + DRM_INFO_ONCE("%s : %d format=0x%x val=0x%x\n",
> + __func__, __LINE__, format, val);
> + return val;
> +}
> +
> +unsigned int set_bits_per_pixel(const struct drm_format_info *format)
This is not a set function - nothing is set.
Maybe just rename it to get_*
> +{
> + u32 bpp = 0;
> + unsigned int val = 0;
> +
> + if (format->num_planes > 1) {
> + val = LCD_LAYER_8BPP;
> + return val;
> + }
> +
> + bpp += 8 * format->cpp[0];
> +
> + switch (bpp) {
> + case 8:
> + val = LCD_LAYER_8BPP;
> + break;
> + case 16:
> + val = LCD_LAYER_16BPP;
> + break;
> + case 24:
> + val = LCD_LAYER_24BPP;
> + break;
> + case 32:
> + val = LCD_LAYER_32BPP;
> + break;
> + }
> +
> + DRM_DEBUG("bpp=%d val=0x%x\n", bpp, val);
> + return val;
> +}
> +
> +static void config_csc(struct kmb_drm_private *kmb, int plane_id)
> +{
> + /* YUV to RGB conversion using the fixed matrix csc_coef_lcd */
> + kmb_write_lcd(kmb, LCD_LAYERn_CSC_COEFF11(plane_id), csc_coef_lcd[0]);
> + kmb_write_lcd(kmb, LCD_LAYERn_CSC_COEFF12(plane_id), csc_coef_lcd[1]);
> + kmb_write_lcd(kmb, LCD_LAYERn_CSC_COEFF13(plane_id), csc_coef_lcd[2]);
> + kmb_write_lcd(kmb, LCD_LAYERn_CSC_COEFF21(plane_id), csc_coef_lcd[3]);
> + kmb_write_lcd(kmb, LCD_LAYERn_CSC_COEFF22(plane_id), csc_coef_lcd[4]);
> + kmb_write_lcd(kmb, LCD_LAYERn_CSC_COEFF23(plane_id), csc_coef_lcd[5]);
> + kmb_write_lcd(kmb, LCD_LAYERn_CSC_COEFF31(plane_id), csc_coef_lcd[6]);
> + kmb_write_lcd(kmb, LCD_LAYERn_CSC_COEFF32(plane_id), csc_coef_lcd[7]);
> + kmb_write_lcd(kmb, LCD_LAYERn_CSC_COEFF33(plane_id), csc_coef_lcd[8]);
> + kmb_write_lcd(kmb, LCD_LAYERn_CSC_OFF1(plane_id), csc_coef_lcd[9]);
> + kmb_write_lcd(kmb, LCD_LAYERn_CSC_OFF2(plane_id), csc_coef_lcd[10]);
> + kmb_write_lcd(kmb, LCD_LAYERn_CSC_OFF3(plane_id), csc_coef_lcd[11]);
> +}
> +
> +static void kmb_plane_atomic_update(struct drm_plane *plane,
> + struct drm_plane_state *state)
> +{
> + struct drm_framebuffer *fb;
> + struct kmb_drm_private *kmb;
> + unsigned int width;
> + unsigned int height;
> + unsigned int dma_len;
> + struct kmb_plane *kmb_plane;
> + unsigned int dma_cfg;
> + unsigned int ctrl = 0, val = 0, out_format = 0;
> + unsigned int src_w, src_h, crtc_x, crtc_y;
> + unsigned char plane_id;
> + int num_planes;
> + static dma_addr_t addr[MAX_SUB_PLANES] = { 0, 0, 0 };
I *think* some compilers will choke on this. And the assignment seems
not to be needed, they are all assigned before use as far as I could
tell.
> +
> + if (!plane || !plane->state || !state)
> + return;
> +
> + fb = plane->state->fb;
> + if (!fb)
> + return;
> + num_planes = fb->format->num_planes;
> + kmb_plane = to_kmb_plane(plane);
> + plane_id = kmb_plane->id;
> +
> + kmb = to_kmb(plane->dev);
> +
> + if (kmb_under_flow || kmb_flush_done) {
> + drm_dbg(&kmb->drm, "plane_update:underflow!!!! returning");
> + return;
> + }
> +
> + src_w = (plane->state->src_w >> 16);
> + src_h = plane->state->src_h >> 16;
> + crtc_x = plane->state->crtc_x;
> + crtc_y = plane->state->crtc_y;
> +
> + drm_dbg(&kmb->drm,
> + "src_w=%d src_h=%d, fb->format->format=0x%x fb->flags=0x%x\n",
> + src_w, src_h, fb->format->format, fb->flags);
> +
> + width = fb->width;
> + height = fb->height;
> + dma_len = (width * height * fb->format->cpp[0]);
> + drm_dbg(&kmb->drm, "dma_len=%d ", dma_len);
> + kmb_write_lcd(kmb, LCD_LAYERn_DMA_LEN(plane_id), dma_len);
> + kmb_write_lcd(kmb, LCD_LAYERn_DMA_LEN_SHADOW(plane_id), dma_len);
> + kmb_write_lcd(kmb, LCD_LAYERn_DMA_LINE_VSTRIDE(plane_id),
> + fb->pitches[0]);
> + kmb_write_lcd(kmb, LCD_LAYERn_DMA_LINE_WIDTH(plane_id),
> + (width * fb->format->cpp[0]));
> +
> + addr[Y_PLANE] = drm_fb_cma_get_gem_addr(fb, plane->state, 0);
> + kmb->fb_addr = addr[Y_PLANE];
> + kmb_write_lcd(kmb, LCD_LAYERn_DMA_START_ADDR(plane_id),
> + addr[Y_PLANE] + fb->offsets[0]);
> + val = set_pixel_format(fb->format->format);
> + val |= set_bits_per_pixel(fb->format);
> + /* Program Cb/Cr for planar formats */
> + if (num_planes > 1) {
> + kmb_write_lcd(kmb, LCD_LAYERn_DMA_CB_LINE_VSTRIDE(plane_id),
> + width * fb->format->cpp[0]);
> + kmb_write_lcd(kmb, LCD_LAYERn_DMA_CB_LINE_WIDTH(plane_id),
> + (width * fb->format->cpp[0]));
> +
> + addr[U_PLANE] = drm_fb_cma_get_gem_addr(fb, plane->state,
> + U_PLANE);
> + /* check if Cb/Cr is swapped*/
> + if (num_planes == 3 && (val & LCD_LAYER_CRCB_ORDER))
> + kmb_write_lcd(kmb,
> + LCD_LAYERn_DMA_START_CR_ADR(plane_id),
> + addr[U_PLANE]);
> + else
> + kmb_write_lcd(kmb,
> + LCD_LAYERn_DMA_START_CB_ADR(plane_id),
> + addr[U_PLANE]);
> +
> + if (num_planes == 3) {
> + kmb_write_lcd(kmb,
> + LCD_LAYERn_DMA_CR_LINE_VSTRIDE(plane_id),
> + ((width) * fb->format->cpp[0]));
> +
> + kmb_write_lcd(kmb,
> + LCD_LAYERn_DMA_CR_LINE_WIDTH(plane_id),
> + ((width) * fb->format->cpp[0]));
> +
> + addr[V_PLANE] = drm_fb_cma_get_gem_addr(fb,
> + plane->state,
> + V_PLANE);
> +
> + /* check if Cb/Cr is swapped*/
> + if (val & LCD_LAYER_CRCB_ORDER)
> + kmb_write_lcd(kmb,
> + LCD_LAYERn_DMA_START_CB_ADR(plane_id),
> + addr[V_PLANE]);
> + else
> + kmb_write_lcd(kmb,
> + LCD_LAYERn_DMA_START_CR_ADR(plane_id),
> + addr[V_PLANE]);
> + }
> + }
> +
> + kmb_write_lcd(kmb, LCD_LAYERn_WIDTH(plane_id), src_w - 1);
> + kmb_write_lcd(kmb, LCD_LAYERn_HEIGHT(plane_id), src_h - 1);
> + kmb_write_lcd(kmb, LCD_LAYERn_COL_START(plane_id), crtc_x);
> + kmb_write_lcd(kmb, LCD_LAYERn_ROW_START(plane_id), crtc_y);
> +
> + val |= LCD_LAYER_FIFO_100;
> +
> + if (val & LCD_LAYER_PLANAR_STORAGE) {
> + val |= LCD_LAYER_CSC_EN;
> +
> + /* Enable CSC if input is planar and output is RGB */
> + config_csc(kmb, plane_id);
> + }
> +
> + kmb_write_lcd(kmb, LCD_LAYERn_CFG(plane_id), val);
> +
> + switch (plane_id) {
> + case LAYER_0:
> + ctrl = LCD_CTRL_VL1_ENABLE;
> + break;
> + case LAYER_1:
> + ctrl = LCD_CTRL_VL2_ENABLE;
> + break;
> + case LAYER_2:
> + ctrl = LCD_CTRL_GL1_ENABLE;
> + break;
> + case LAYER_3:
> + ctrl = LCD_CTRL_GL2_ENABLE;
> + break;
> + }
> +
> + ctrl |= LCD_CTRL_PROGRESSIVE | LCD_CTRL_TIM_GEN_ENABLE
> + | LCD_CTRL_CONTINUOUS | LCD_CTRL_OUTPUT_ENABLED;
> +
> + /* LCD is connected to MIPI on kmb
> + * Therefore this bit is required for DSI Tx
> + */
> + ctrl |= LCD_CTRL_VHSYNC_IDLE_LVL;
> +
> + kmb_set_bitmask_lcd(kmb, LCD_CONTROL, ctrl);
> +
> + /* FIXME no doc on how to set output format,these values are
> + * taken from the Myriadx tests
> + */
> + out_format |= LCD_OUTF_FORMAT_RGB888;
> +
> + /* Leave RGB order,conversion mode and clip mode to default */
> + /* do not interleave RGB channels for mipi Tx compatibility */
> + out_format |= LCD_OUTF_MIPI_RGB_MODE;
> + kmb_write_lcd(kmb, LCD_OUT_FORMAT_CFG, out_format);
> +
> + dma_cfg = LCD_DMA_LAYER_ENABLE | LCD_DMA_LAYER_VSTRIDE_EN |
> + LCD_DMA_LAYER_CONT_UPDATE | LCD_DMA_LAYER_AXI_BURST_16;
> +
> + /* Enable DMA */
> + kmb_write_lcd(kmb, LCD_LAYERn_DMA_CFG(plane_id), dma_cfg);
> + drm_dbg(&kmb->drm, "dma_cfg=0x%x LCD_DMA_CFG=0x%x\n", dma_cfg,
> + kmb_read_lcd(kmb, LCD_LAYERn_DMA_CFG(plane_id)));
> +
> + kmb_set_bitmask_lcd(kmb, LCD_INT_CLEAR, LCD_INT_EOF |
> + LCD_INT_DMA_ERR);
> + kmb_set_bitmask_lcd(kmb, LCD_INT_ENABLE, LCD_INT_EOF |
> + LCD_INT_DMA_ERR);
> +}
> +
> +static const struct drm_plane_helper_funcs kmb_plane_helper_funcs = {
> + .atomic_check = kmb_plane_atomic_check,
> + .atomic_update = kmb_plane_atomic_update,
> + .atomic_disable = kmb_plane_atomic_disable
> +};
> +
> +void kmb_plane_destroy(struct drm_plane *plane)
> +{
> + struct kmb_plane *kmb_plane = to_kmb_plane(plane);
> +
> + drm_plane_cleanup(plane);
> + kfree(kmb_plane);
> +}
> +
> +static void kmb_destroy_plane_state(struct drm_plane *plane,
> + struct drm_plane_state *state)
> +{
> + struct kmb_plane_state *kmb_state = to_kmb_plane_state(state);
> +
> + __drm_atomic_helper_plane_destroy_state(state);
> + kfree(kmb_state);
> +}
> +
> +struct drm_plane_state *kmb_plane_duplicate_state(struct drm_plane *plane)
> +{
Use __drm_atomic_helper_plane_duplicate_state() - which requires a few
updates.
See other users.
> + struct drm_plane_state *state;
> + struct kmb_plane_state *kmb_state;
> +
> + kmb_state = kmemdup(plane->state, sizeof(*kmb_state), GFP_KERNEL);
> + if (!kmb_state)
> + return NULL;
> +
> + state = &kmb_state->base_plane_state;
> + __drm_atomic_helper_plane_duplicate_state(plane, state);
> +
> + return state;
> +}
> +
> +static void kmb_plane_reset(struct drm_plane *plane)
> +{
> + struct kmb_plane_state *kmb_state = to_kmb_plane_state(plane->state);
> +
> + if (kmb_state)
> + __drm_atomic_helper_plane_destroy_state
> + (&kmb_state->base_plane_state);
Join lines - this is not readable.
> + kfree(kmb_state);
> +
> + plane->state = NULL;
> + kmb_state = kzalloc(sizeof(*kmb_state), GFP_KERNEL);
Use __drm_atomic_helper_plane_reset()
> + if (kmb_state) {
> + kmb_state->base_plane_state.plane = plane;
> + kmb_state->base_plane_state.rotation = DRM_MODE_ROTATE_0;
> + plane->state = &kmb_state->base_plane_state;
> + kmb_state->no_planes = KMB_MAX_PLANES;
> + }
> +}
> +
> +static const struct drm_plane_funcs kmb_plane_funcs = {
> + .update_plane = drm_atomic_helper_update_plane,
> + .disable_plane = drm_atomic_helper_disable_plane,
> + .destroy = kmb_plane_destroy,
> + .reset = kmb_plane_reset,
> + .atomic_duplicate_state = kmb_plane_duplicate_state,
> + .atomic_destroy_state = kmb_destroy_plane_state,
> +};
> +
> +struct kmb_plane *kmb_plane_init(struct drm_device *drm)
> +{
> + struct kmb_drm_private *lcd = to_kmb(drm);
Name it kmb? s/lcd/kmb/
> + struct kmb_plane *plane = NULL;
> + struct kmb_plane *primary = NULL;
> + int i = 0;
> + int ret = 0;
> + enum drm_plane_type plane_type;
> + const u32 *plane_formats;
> + int num_plane_formats;
> +
> + for (i = 0; i < lcd->n_layers; i++) {
> + plane = drmm_kzalloc(drm, sizeof(*plane), GFP_KERNEL);
> +
> + if (!plane) {
> + drm_err(drm, "Failed to allocate plane\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + plane_type = (i == 0) ? DRM_PLANE_TYPE_PRIMARY :
> + DRM_PLANE_TYPE_OVERLAY;
> + if (i < 2) {
> + plane_formats = kmb_formats_v;
> + num_plane_formats = ARRAY_SIZE(kmb_formats_v);
> + } else {
> + plane_formats = kmb_formats_g;
> + num_plane_formats = ARRAY_SIZE(kmb_formats_g);
> + }
> +
> + ret = drm_universal_plane_init(drm, &plane->base_plane,
> + POSSIBLE_CRTCS, &kmb_plane_funcs,
> + plane_formats, num_plane_formats,
> + NULL, plane_type, "plane %d", i);
> + if (ret < 0) {
> + drm_err(drm, "drm_universal_plane_init failed (ret=%d)",
> + ret);
> + goto cleanup;
> + }
> + drm_dbg(drm, "%s : %d i=%d type=%d",
> + __func__, __LINE__,
> + i, plane_type);
> + drm_plane_helper_add(&plane->base_plane,
> + &kmb_plane_helper_funcs);
> + if (plane_type == DRM_PLANE_TYPE_PRIMARY) {
> + primary = plane;
> + lcd->plane = plane;
> + }
> + drm_dbg(drm, "%s : %d primary=%p\n", __func__, __LINE__,
> + &primary->base_plane);
> + plane->id = i;
> + }
> +
> + return primary;
> +cleanup:
> + kfree(plane);
> + return ERR_PTR(ret);
> +}
> diff --git a/drivers/gpu/drm/kmb/kmb_plane.h b/drivers/gpu/drm/kmb/kmb_plane.h
> new file mode 100644
> index 0000000..48f237f
> --- /dev/null
> +++ b/drivers/gpu/drm/kmb/kmb_plane.h
> @@ -0,0 +1,124 @@
> +/* SPDX-License-Identifier: GPL-2.0-only
> + *
> + * Copyright © 2018-2020 Intel Corporation
> + */
> +
> +#ifndef __KMB_PLANE_H__
> +#define __KMB_PLANE_H__
> +
> +#include "kmb_drv.h"
> +
> +extern int kmb_under_flow;
> +extern int kmb_flush_done;
> +
> +#define LCD_INT_VL0_ERR ((LAYER0_DMA_FIFO_UNDERFLOW) | \
> + (LAYER0_DMA_FIFO_OVERFLOW) | \
> + (LAYER0_DMA_CB_FIFO_OVERFLOW) | \
> + (LAYER0_DMA_CB_FIFO_UNDERFLOW) | \
> + (LAYER0_DMA_CR_FIFO_OVERFLOW) | \
> + (LAYER0_DMA_CR_FIFO_UNDERFLOW))
> +
> +#define LCD_INT_VL1_ERR ((LAYER1_DMA_FIFO_UNDERFLOW) | \
> + (LAYER1_DMA_FIFO_OVERFLOW) | \
> + (LAYER1_DMA_CB_FIFO_OVERFLOW) | \
> + (LAYER1_DMA_CB_FIFO_UNDERFLOW) | \
> + (LAYER1_DMA_CR_FIFO_OVERFLOW) | \
> + (LAYER1_DMA_CR_FIFO_UNDERFLOW))
> +
> +#define LCD_INT_GL0_ERR (LAYER2_DMA_FIFO_OVERFLOW | LAYER2_DMA_FIFO_UNDERFLOW)
> +#define LCD_INT_GL1_ERR (LAYER3_DMA_FIFO_OVERFLOW | LAYER3_DMA_FIFO_UNDERFLOW)
> +#define LCD_INT_VL0 (LAYER0_DMA_DONE | LAYER0_DMA_IDLE | LCD_INT_VL0_ERR)
> +#define LCD_INT_VL1 (LAYER1_DMA_DONE | LAYER1_DMA_IDLE | LCD_INT_VL1_ERR)
> +#define LCD_INT_GL0 (LAYER2_DMA_DONE | LAYER2_DMA_IDLE | LCD_INT_GL0_ERR)
> +#define LCD_INT_GL1 (LAYER3_DMA_DONE | LAYER3_DMA_IDLE | LCD_INT_GL1_ERR)
> +#define LCD_INT_DMA_ERR (LCD_INT_VL0_ERR | LCD_INT_VL1_ERR \
> + | LCD_INT_GL0_ERR | LCD_INT_GL1_ERR)
> +
> +#define POSSIBLE_CRTCS 1
> +#define INITIALIZED 1
Not used I think.
> +#define to_kmb_plane(x) container_of(x, struct kmb_plane, base_plane)
> +
> +#define to_kmb_plane_state(x) \
> + container_of(x, struct kmb_plane_state, base_plane_state)
> +
> +enum layer_id {
> + LAYER_0,
> + LAYER_1,
> + LAYER_2,
> + LAYER_3,
> +// KMB_MAX_PLANES,
> +};
> +
> +#define KMB_MAX_PLANES 1
> +
> +enum sub_plane_id {
> + Y_PLANE,
> + U_PLANE,
> + V_PLANE,
> + MAX_SUB_PLANES,
> +};
> +
> +struct kmb_plane {
> + struct drm_plane base_plane;
> + struct kmb_drm_private kmb_dev;
This is wrong, embedding kmb_drm_private here is not correct.
> + unsigned char id;
> +};
If possible embedding the planes in kmb_drm_private would be preferable.
This is how other drivers do it with success.
The id part seems pretty unique - but maybe other drivers has the same
but name it different.
> +
> +struct kmb_plane_state {
> + struct drm_plane_state base_plane_state;
> + unsigned char no_planes;
no_planes is not used - so the whole kmb_plane_state can be dropped.
And this should kill a few helper functions too.
I know part of this is prepared for multiple planes.
But keep it lean and clean now - maybe addding extra planes later needs
to be done different than what the code tries to prepare for.
> +};
> +
> +/* Graphics layer (layers 2 & 3) formats, only packed formats are supported */
> +static const u32 kmb_formats_g[] = {
> + DRM_FORMAT_RGB332,
> + DRM_FORMAT_XRGB4444, DRM_FORMAT_XBGR4444,
> + DRM_FORMAT_ARGB4444, DRM_FORMAT_ABGR4444,
> + DRM_FORMAT_XRGB1555, DRM_FORMAT_XBGR1555,
> + DRM_FORMAT_ARGB1555, DRM_FORMAT_ABGR1555,
> + DRM_FORMAT_RGB565, DRM_FORMAT_BGR565,
> + DRM_FORMAT_RGB888, DRM_FORMAT_BGR888,
> + DRM_FORMAT_XRGB8888, DRM_FORMAT_XBGR8888,
> + DRM_FORMAT_ARGB8888, DRM_FORMAT_ABGR8888,
> +};
> +
> +#define MAX_FORMAT_G (ARRAY_SIZE(kmb_formats_g))
> +#define MAX_FORMAT_V (ARRAY_SIZE(kmb_formats_v))
> +
> +/* Video layer ( 0 & 1) formats, packed and planar formats are supported */
> +static const u32 kmb_formats_v[] = {
> + /* packed formats */
> + DRM_FORMAT_RGB332,
> + DRM_FORMAT_XRGB4444, DRM_FORMAT_XBGR4444,
> + DRM_FORMAT_ARGB4444, DRM_FORMAT_ABGR4444,
> + DRM_FORMAT_XRGB1555, DRM_FORMAT_XBGR1555,
> + DRM_FORMAT_ARGB1555, DRM_FORMAT_ABGR1555,
> + DRM_FORMAT_RGB565, DRM_FORMAT_BGR565,
> + DRM_FORMAT_RGB888, DRM_FORMAT_BGR888,
> + DRM_FORMAT_XRGB8888, DRM_FORMAT_XBGR8888,
> + DRM_FORMAT_ARGB8888, DRM_FORMAT_ABGR8888,
> + /*planar formats */
> + DRM_FORMAT_YUV420, DRM_FORMAT_YVU420,
> + DRM_FORMAT_YUV422, DRM_FORMAT_YVU422,
> + DRM_FORMAT_YUV444, DRM_FORMAT_YVU444,
> + DRM_FORMAT_NV12, DRM_FORMAT_NV21,
> +};
> +
> +/* Conversion (yuv->rgb) matrix from myriadx */
> +static const u32 csc_coef_lcd[] = {
> + 1024, 0, 1436,
> + 1024, -352, -731,
> + 1024, 1814, 0,
> + -179, 125, -226
> +};
> +
> +struct layer_status {
> + bool disable;
> + u32 ctrl;
> +};
> +
> +extern struct layer_status plane_status[KMB_MAX_PLANES];
> +
> +struct kmb_plane *kmb_plane_init(struct drm_device *drm);
> +void kmb_plane_destroy(struct drm_plane *plane);
> +#endif /* __KMB_PLANE_H__ */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
