Re: [PATCH v6] drm/kmb: Add support for KeemBay Display

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux