On 28 November 2015 at 23:56, Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote:
Hi Emil, thank you for review.
use_maskOn 28 November 2015 at 10:38, Xinliang Liu
<xinliang.liu@xxxxxxxxxx> wrote:
> Add crtc funcs and helper funcs for ADE.
>
> Signed-off-by: Xinliang Liu <xinliang.liu@xxxxxxxxxx>
> Signed-off-by: Xinwei Kong <kong.kongxinwei@xxxxxxxxxxxxx>
> Signed-off-by: Andy Green <andy.green@xxxxxxxxxx>
> ---
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hisi_ade_reg.h
> +#define ADE_CTRL (0x4)
> +#define ADE_CTRL1 (0x8C)
> +#define ADE_ROT_SRC_CFG (0x10)
> +#define ADE_DISP_SRC_CFG (0x18)
> +#define ADE_WDMA2_SRC_CFG (0x1C)
> +#define ADE_SEC_OVLY_SRC_CFG (0x20)
> +#define ADE_WDMA3_SRC_CFG (0x24)
> +#define ADE_OVLY1_TRANS_CFG (0x2C)
> +#define ADE_EN (0x100)
> +#define INTR_MASK_CPU_0 (0xC10)
> +#define INTR_MASK_CPU_1 (0xC14)
> +#define ADE_FRM_DISGARD_CTRL (0xA4)
> +/* reset and reload regs */
> +#define ADE_SOFT_RST_SEL0 (0x78)
> +#define ADE_SOFT_RST_SEL1 (0x7C)
> +#define ADE_RELOAD_DIS0 (0xAC)
> +#define ADE_RELOAD_DIS1 (0xB0)
> +#define ADE_CH_RDMA_BIT_OFST (0)
> +#define ADE_CLIP_BIT_OFST (15)
> +#define ADE_SCL_BIT_OFST (21)
> +#define ADE_CTRAN_BIT_OFST (24)
> +#define ADE_OVLY_BIT_OFST (37) /* 32+5 */
Don't think we have any cases in drm where constants are wrapped in
brackets. Is there any benefit of doing that here ?
Seems no any benefit, will remove these brackets in v3.
> +/* channel regs */
> +#define RD_CH_PE(x) (0x1000 + (x) * 0x80)
... and I'm not talking about cases where the macros such as this one.
The struct in the above two unions is missing a level of indentation.
> +union U_LDI_CTRL {
> +struct {
> + unsigned int ldi_en :1;
> + unsigned int disp_mode_buf :1;
> + unsigned int date_gate_en :1;
> + unsigned int bpp :2;
> + unsigned int wait_vsync_en :1;
> + unsigned int corlorbar_width :7;
> + unsigned int bgr :1;
> + unsigned int color_mode :1;
> + unsigned int shutdown :1;
> + unsigned int vactive_line :12;
> + unsigned int ldi_en_self_clr :1;
> + unsigned int reserved_573 :3;
> + } bits;
> + unsigned int u32;
> +};
> +
> +union U_LDI_WORK_MODE {
> +struct {
> + unsigned int work_mode :1;
> + unsigned int wback_en :1;
> + unsigned int colorbar_en :1;
> + unsigned int reserved_577 :29;
> + } bits;
> + unsigned int u32;
> +};
> +
yes, will be fixed in v3.
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hisi_drm_ade.c
> +static void ade_ldi_set_mode(struct ade_crtc *acrtc,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adj_mode)
> +{
> + struct ade_hw_ctx *ctx = acrtc->ctx;
> + void __iomem *base = ctx->base;
> + u32 out_w = mode->hdisplay;
> + u32 out_h = mode->vdisplay;
> + u32 hfp, hbp, hsw, vfp, vbp, vsw;
> + u32 plr_flags;
> + int ret;
> +
> + plr_flags = (mode->flags & DRM_MODE_FLAG_NVSYNC)
> + ? HISI_LDI_FLAG_NVSYNC : 0;
> + plr_flags |= (mode->flags & DRM_MODE_FLAG_NHSYNC)
> + ? HISI_LDI_FLAG_NHSYNC : 0;
> + hfp = mode->hsync_start - mode->hdisplay;
> + hbp = mode->htotal - mode->hsync_end;
> + hsw = mode->hsync_end - mode->hsync_start;
> + vfp = mode->vsync_start - mode->vdisplay;
> + vbp = mode->vtotal - mode->vsync_end;
> + vsw = mode->vsync_end - mode->vsync_start;
> + if (vsw > 15) {
> + DRM_INFO("vsw exceeded 15\n");
DRM_ERROR or DRM_DEBUG_xx perhaps ?
This is not an error hardware still can handle if vsw exceed 15.
You are right maybe
DRM_DEBUG_DRIVER is better.
DItto
> + vsw = 15;
> + }
> +
> + writel((hbp << 20) | (hfp << 0), base + LDI_HRZ_CTRL0);
> + /* p3-73 6220V100 pdf:
> + * "The configured value is the actual width - 1"
> + */
> + writel(hsw - 1, base + LDI_HRZ_CTRL1);
> + writel((vbp << 20) | (vfp << 0), base + LDI_VRT_CTRL0);
> + /* p3-74 6220V100 pdf:
> + * "The configured value is the actual width - 1"
> + */
> + writel(vsw - 1, base + LDI_VRT_CTRL1);
> +
> + /* p3-75 6220V100 pdf:
> + * "The configured value is the actual width - 1"
> + */
> + writel(((out_h - 1) << 20) | ((out_w - 1) << 0),
> + base + LDI_DSP_SIZE);
> + writel(plr_flags, base + LDI_PLR_CTRL);
> +
> + ret = clk_set_rate(ctx->ade_pix_clk, mode->clock * 1000);
> + /* Success should be guaranteed in aotomic_check
> + * failer shouldn't happen here
> + */
> + if (ret)
> + DRM_ERROR("set ade_pixel_clk_rate fail\n");
will use
DRM_DEBUG_DRIVER in v3.
> + adj_mode->clock = clk_get_rate(ctx->ade_pix_clk) / 1000;
> +
> + /* ctran6 setting */
> + writel(1, base + ADE_CTRAN_DIS(ADE_CTRAN6));
> + writel(out_w * out_h - 1, base + ADE_CTRAN_IMAGE_SIZE(ADE_CTRAN6));
> + acrtc->use_mask |= BIT(ADE_CTRAN_BIT_OFST + ADE_CTRAN6);
> + DRM_INFO("set mode: %dx%d\n", out_w, out_h);
> +
DRM_DEBUG_DRIVER ?
will use
DRM_DEBUG_DRIVER in v3.
> + /*
> + * other parameters setting
> + */
> + writel(BIT(0), base + LDI_WORK_MODE);
> + writel((0x3c << 6) | (ADE_OUT_RGB_888 << 3) | BIT(2) | BIT(0),
> + base + LDI_CTRL);
> + set_reg(base + LDI_DE_SPACE_LOW, 0x1, 1, 1);
> +}
> +
> +static int ade_power_up(struct ade_hw_ctx *ctx)
> +{
> + void __iomem *media_base = ctx->media_base;
> + int ret;
> +
> + ret = clk_set_rate(ctx->ade_core_clk, ctx->ade_core_rate);
> + if (ret) {
> + DRM_ERROR("clk_set_rate ade_core_rate error\n");
How about the following (or alike) less cryptic and more informative
message. Other places could use a similar treatment.
"Failed to set rate X clk (%d)\n", ret ?
yes, good advice. will be fixed in v3.
> +static void ade_crtc_enable(struct drm_crtc *crtc)
> +{
> + struct ade_crtc *acrtc = to_ade_crtc(crtc);
> + struct ade_hw_ctx *ctx = acrtc->ctx;
> + int ret;
> +
> + DRM_DEBUG_DRIVER("enter.\n");
Does this and the remainingDEBUG_DRIVER(enter|exit) messages provide
any meaningful input, past the driverdevelopment stage ?
> + if (acrtc->enable)
> + return;
Esp. since we have cases like this where no message is available.
yeap, these
DEBUG_DRIVER(enter|exit) messages are for
development stage.Which forgot to be removed. will be removed in v3.
Thanks,
-xinliang
Regards,
Emil
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
