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