[PATCH v2] drm/gem: fix not to assign error value to gem name
Hello, Chris, On 2013? 06? 27? 17:31, Chris Wilson wrote: > On Thu, Jun 27, 2013 at 08:58:33AM +0900, Seung-Woo Kim wrote: >> From: YoungJun Cho >> >> If idr_alloc() is failed, obj->name can be error value. Also >> it cleans up duplicated flink processing code. >> >> This regression has been introduced in >> >> commit 2e928815c1886fe628ed54623aa98d0889cf5509 >> Author: Tejun Heo >> Date: Wed Feb 27 17:04:08 2013 -0800 >> >> drm: convert to idr_alloc() >> >> Signed-off-by: YoungJun Cho >> Signed-off-by: Seung-Woo Kim >> Signed-off-by: Kyungmin Park > > Reviewed-by: Chris Wilson > > Minor comment inline. > >> --- >> change since v1: >> - Add a regression commit information in commit msg as Chris commented >> >> drivers/gpu/drm/drm_gem.c | 18 +++--- >> 1 file changed, 7 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >> index 4321713..c9d7081 100644 >> --- a/drivers/gpu/drm/drm_gem.c >> +++ b/drivers/gpu/drm/drm_gem.c >> @@ -453,25 +453,21 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data, >> spin_lock(>object_name_lock); >> if (!obj->name) { >> ret = idr_alloc(>object_name_idr, obj, 1, 0, GFP_NOWAIT); >> -obj->name = ret; >> -args->name = (uint64_t) obj->name; >> -spin_unlock(>object_name_lock); >> -idr_preload_end(); >> - >> if (ret < 0) >> goto err; > > Being pedantic, ret == 0 is also an error - but a programming error > leading to an object leak. BUG_ON(ret == 0) ? > -Chris > It seems that idr_alloc() with start id 1 does not return 0, so IMHO, ret == 0 can be ignored here. But if you think it needs to consider programming error, I'll add some assertion. Please let me know. Thanks and Regards, - Seung-Woo Kim -- Seung-Woo Kim Samsung Software R Center --
Re: [PATCH v2] drm/gem: fix not to assign error value to gem name
Hello, Chris, On 2013년 06월 27일 17:31, Chris Wilson wrote: On Thu, Jun 27, 2013 at 08:58:33AM +0900, Seung-Woo Kim wrote: From: YoungJun Cho yj44@samsung.com If idr_alloc() is failed, obj-name can be error value. Also it cleans up duplicated flink processing code. This regression has been introduced in commit 2e928815c1886fe628ed54623aa98d0889cf5509 Author: Tejun Heo t...@kernel.org Date: Wed Feb 27 17:04:08 2013 -0800 drm: convert to idr_alloc() Signed-off-by: YoungJun Cho yj44@samsung.com Signed-off-by: Seung-Woo Kim sw0312@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Minor comment inline. --- change since v1: - Add a regression commit information in commit msg as Chris commented drivers/gpu/drm/drm_gem.c | 18 +++--- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 4321713..c9d7081 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -453,25 +453,21 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data, spin_lock(dev-object_name_lock); if (!obj-name) { ret = idr_alloc(dev-object_name_idr, obj, 1, 0, GFP_NOWAIT); -obj-name = ret; -args-name = (uint64_t) obj-name; -spin_unlock(dev-object_name_lock); -idr_preload_end(); - if (ret 0) goto err; Being pedantic, ret == 0 is also an error - but a programming error leading to an object leak. BUG_ON(ret == 0) ? -Chris It seems that idr_alloc() with start id 1 does not return 0, so IMHO, ret == 0 can be ignored here. But if you think it needs to consider programming error, I'll add some assertion. Please let me know. Thanks and Regards, - Seung-Woo Kim -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 2/3] drm/exynos: add support for exynos5420 mixer
Hello Rahul, This patch is exactly same with v2 2/4 and only rebased on v3 1/3, so my ack is valid for this patch. On 2013? 06? 19? 21:51, Rahul Sharma wrote: > Add support for exynos5420 mixer IP in the drm mixer driver. > > Signed-off-by: Rahul Sharma Acked-by: Seung-Woo Kim Anyway, this patch can be merged after merging [Patch v3 1/3] as like v2. Best Regards, - Seung-Woo Kim > --- > .../devicetree/bindings/video/exynos_mixer.txt |1 + > drivers/gpu/drm/exynos/exynos_mixer.c | 49 > +++- > drivers/gpu/drm/exynos/regs-mixer.h|7 +++ > 3 files changed, 45 insertions(+), 12 deletions(-) > > diff --git a/Documentation/devicetree/bindings/video/exynos_mixer.txt > b/Documentation/devicetree/bindings/video/exynos_mixer.txt > index 9131b99..3334b0a 100644 > --- a/Documentation/devicetree/bindings/video/exynos_mixer.txt > +++ b/Documentation/devicetree/bindings/video/exynos_mixer.txt > @@ -5,6 +5,7 @@ Required properties: > 1) "samsung,exynos5-mixer" > 2) "samsung,exynos4210-mixer" > 3) "samsung,exynos5250-mixer" > + 4) "samsung,exynos5420-mixer" > > - reg: physical base address of the mixer and length of memory mapped > region. > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c > b/drivers/gpu/drm/exynos/exynos_mixer.c > index 6225501..b1280b4 100644 > --- a/drivers/gpu/drm/exynos/exynos_mixer.c > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c > @@ -78,6 +78,7 @@ struct mixer_resources { > enum mixer_version_id { > MXR_VER_0_0_0_16, > MXR_VER_16_0_33_0, > + MXR_VER_128_0_0_184, > }; > > struct mixer_context { > @@ -283,17 +284,19 @@ static void mixer_cfg_scan(struct mixer_context *ctx, > unsigned int height) > val = (ctx->interlace ? MXR_CFG_SCAN_INTERLACE : > MXR_CFG_SCAN_PROGRASSIVE); > > - /* choosing between porper HD and SD mode */ > - if (height <= 480) > - val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; > - else if (height <= 576) > - val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; > - else if (height <= 720) > - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > - else if (height <= 1080) > - val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; > - else > - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > + if (ctx->mxr_ver != MXR_VER_128_0_0_184) { > + /* choosing between proper HD and SD mode */ > + if (height <= 480) > + val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; > + else if (height <= 576) > + val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; > + else if (height <= 720) > + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > + else if (height <= 1080) > + val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; > + else > + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > + } > > mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_SCAN_MASK); > } > @@ -557,6 +560,14 @@ static void mixer_graph_buffer(struct mixer_context > *ctx, int win) > /* setup geometry */ > mixer_reg_write(res, MXR_GRAPHIC_SPAN(win), win_data->fb_width); > > + /* setup display size */ > + if (ctx->mxr_ver == MXR_VER_128_0_0_184 && > + win == MIXER_DEFAULT_WIN) { > + val = MXR_MXR_RES_HEIGHT(win_data->fb_height); > + val |= MXR_MXR_RES_WIDTH(win_data->fb_width); > + mixer_reg_write(res, MXR_RESOLUTION, val); > + } > + > val = MXR_GRP_WH_WIDTH(win_data->crtc_width); > val |= MXR_GRP_WH_HEIGHT(win_data->crtc_height); > val |= MXR_GRP_WH_H_SCALE(x_ratio); > @@ -581,7 +592,8 @@ static void mixer_graph_buffer(struct mixer_context *ctx, > int win) > mixer_cfg_layer(ctx, win, true); > > /* layer update mandatory for mixer 16.0.33.0 */ > - if (ctx->mxr_ver == MXR_VER_16_0_33_0) > + if (ctx->mxr_ver == MXR_VER_16_0_33_0 || > + ctx->mxr_ver == MXR_VER_128_0_0_184) > mixer_layer_update(ctx); > > mixer_run(ctx); > @@ -816,6 +828,7 @@ static void mixer_win_disable(void *ctx, int win) > > static int mixer_check_mode(void *ctx, struct drm_display_mode *mode) > { > + struct mixer_context *mixer_ctx = ctx; > u32 w, h; > > w = mode->hdisplay; > @@ -825,6 +838,10 @@ static int mixer_check_mode(void *ctx, struct > drm_display_mode *mode) > mode->hdisplay, mode->vdisplay, mode->vrefresh, > (mode->flags & DRM_MODE_FLAG_INTERLACE) ? 1 : 0); > > + if (mixer_ctx->mxr_ver == MXR_VER_0_0_0_16 || > + mixer_ctx->mxr_ver == MXR_VER_128_0_0_184) > + return 0; > + > if ((w >= 464 && w <= 720 && h >= 261 && h <= 576) || > (w >= 1024 && w <= 1280 && h >= 576 && h <= 720) || > (w >= 1664 && w <= 1920 && h >= 936 && h
[PATCH v2 2/4] drm/exynos: add support for exynos5420 mixer
On 2013? 06? 19? 15:50, Rahul Sharma wrote: > Add support for exynos5420 mixer IP in the drm mixer driver. > > Signed-off-by: Rahul Sharma Acked-by: Seung-Woo Kim This patch can be merged after merging "[PATCH 1/4] drm/exynos: rename compatible strings for hdmi subsystem". Best Regards, - Seung-Woo Kim > --- > .../devicetree/bindings/video/exynos_mixer.txt |1 + > drivers/gpu/drm/exynos/exynos_mixer.c | 49 > +++- > drivers/gpu/drm/exynos/regs-mixer.h|7 +++ > 3 files changed, 45 insertions(+), 12 deletions(-) > > diff --git a/Documentation/devicetree/bindings/video/exynos_mixer.txt > b/Documentation/devicetree/bindings/video/exynos_mixer.txt > index a8b063f..c64ddc1 100644 > --- a/Documentation/devicetree/bindings/video/exynos_mixer.txt > +++ b/Documentation/devicetree/bindings/video/exynos_mixer.txt > @@ -4,6 +4,7 @@ Required properties: > - compatible: value should be: > 1) "samsung,exynos4210-mixer" > 2) "samsung,exynos5250-mixer" > + 3) "samsung,exynos5420-mixer" > > - reg: physical base address of the mixer and length of memory mapped > region. > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c > b/drivers/gpu/drm/exynos/exynos_mixer.c > index 2fe6d33..d51ff36 100644 > --- a/drivers/gpu/drm/exynos/exynos_mixer.c > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c > @@ -78,6 +78,7 @@ struct mixer_resources { > enum mixer_version_id { > MXR_VER_0_0_0_16, > MXR_VER_16_0_33_0, > + MXR_VER_128_0_0_184, > }; > > struct mixer_context { > @@ -283,17 +284,19 @@ static void mixer_cfg_scan(struct mixer_context *ctx, > unsigned int height) > val = (ctx->interlace ? MXR_CFG_SCAN_INTERLACE : > MXR_CFG_SCAN_PROGRASSIVE); > > - /* choosing between porper HD and SD mode */ > - if (height <= 480) > - val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; > - else if (height <= 576) > - val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; > - else if (height <= 720) > - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > - else if (height <= 1080) > - val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; > - else > - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > + if (ctx->mxr_ver != MXR_VER_128_0_0_184) { > + /* choosing between proper HD and SD mode */ > + if (height <= 480) > + val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; > + else if (height <= 576) > + val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; > + else if (height <= 720) > + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > + else if (height <= 1080) > + val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; > + else > + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > + } > > mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_SCAN_MASK); > } > @@ -557,6 +560,14 @@ static void mixer_graph_buffer(struct mixer_context > *ctx, int win) > /* setup geometry */ > mixer_reg_write(res, MXR_GRAPHIC_SPAN(win), win_data->fb_width); > > + /* setup display size */ > + if (ctx->mxr_ver == MXR_VER_128_0_0_184 && > + win == MIXER_DEFAULT_WIN) { > + val = MXR_MXR_RES_HEIGHT(win_data->fb_height); > + val |= MXR_MXR_RES_WIDTH(win_data->fb_width); > + mixer_reg_write(res, MXR_RESOLUTION, val); > + } > + > val = MXR_GRP_WH_WIDTH(win_data->crtc_width); > val |= MXR_GRP_WH_HEIGHT(win_data->crtc_height); > val |= MXR_GRP_WH_H_SCALE(x_ratio); > @@ -581,7 +592,8 @@ static void mixer_graph_buffer(struct mixer_context *ctx, > int win) > mixer_cfg_layer(ctx, win, true); > > /* layer update mandatory for mixer 16.0.33.0 */ > - if (ctx->mxr_ver == MXR_VER_16_0_33_0) > + if (ctx->mxr_ver == MXR_VER_16_0_33_0 || > + ctx->mxr_ver == MXR_VER_128_0_0_184) > mixer_layer_update(ctx); > > mixer_run(ctx); > @@ -816,6 +828,7 @@ static void mixer_win_disable(void *ctx, int win) > > static int mixer_check_mode(void *ctx, struct drm_display_mode *mode) > { > + struct mixer_context *mixer_ctx = ctx; > u32 w, h; > > w = mode->hdisplay; > @@ -825,6 +838,10 @@ static int mixer_check_mode(void *ctx, struct > drm_display_mode *mode) > mode->hdisplay, mode->vdisplay, mode->vrefresh, > (mode->flags & DRM_MODE_FLAG_INTERLACE) ? 1 : 0); > > + if (mixer_ctx->mxr_ver == MXR_VER_0_0_0_16 || > + mixer_ctx->mxr_ver == MXR_VER_128_0_0_184) > + return 0; > + > if ((w >= 464 && w <= 720 && h >= 261 && h <= 576) || > (w >= 1024 && w <= 1280 && h >= 576 && h <= 720) || > (w >= 1664 && w <= 1920 && h >= 936 && h <= 1080)) > @@ -1115,6 +1132,11 @@ static int vp_resources_init(struct >
[PATCH 1/4] drm/exynos: rename compatible strings for hdmi subsystem
Hi Rahul, It looks good, at least, to me. Best Regards, - Seung-Woo Kim On 2013? 06? 18? 21:49, Rahul Sharma wrote: > This patch renames the combatible strings for hdmi, mixer, ddc > and hdmiphy. It follows the convention of using compatible string > which represent the SoC in which the IP was added for the first > time. > > Signed-off-by: Rahul Sharma > --- > Documentation/devicetree/bindings/video/exynos_hdmi.txt|6 -- > Documentation/devicetree/bindings/video/exynos_hdmiddc.txt |4 ++-- > Documentation/devicetree/bindings/video/exynos_hdmiphy.txt |6 -- > Documentation/devicetree/bindings/video/exynos_mixer.txt |7 +-- > drivers/gpu/drm/exynos/exynos_ddc.c|2 +- > drivers/gpu/drm/exynos/exynos_hdmi.c |2 +- > drivers/gpu/drm/exynos/exynos_hdmiphy.c|4 +++- > drivers/gpu/drm/exynos/exynos_mixer.c | 12 > ++-- > 8 files changed, 26 insertions(+), 17 deletions(-) > > diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt > b/Documentation/devicetree/bindings/video/exynos_hdmi.txt > index 589edee..2ac01ca 100644 > --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt > +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt > @@ -1,7 +1,9 @@ > Device-Tree bindings for drm hdmi driver > > Required properties: > -- compatible: value should be "samsung,exynos5-hdmi". > +- compatible: value should be one among the following: > + 1) "samsung,exynos4210-hdmi" > + 2) "samsung,exynos4212-hdmi" > - reg: physical base address of the hdmi and length of memory mapped > region. > - interrupts: interrupt number to the cpu. > @@ -15,7 +17,7 @@ Required properties: > Example: > > hdmi { > - compatible = "samsung,exynos5-hdmi"; > + compatible = "samsung,exynos4212-hdmi"; > reg = <0x1453 0x10>; > interrupts = <0 95 0>; > hpd-gpio = < 7 0xf 1 3>; > diff --git a/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt > b/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt > index fa166d9..c1bd2ea 100644 > --- a/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt > +++ b/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt > @@ -1,12 +1,12 @@ > Device-Tree bindings for hdmiddc driver > > Required properties: > -- compatible: value should be "samsung,exynos5-hdmiddc". > +- compatible: value should be "samsung,exynos4210-hdmiddc". > - reg: I2C address of the hdmiddc device. > > Example: > > hdmiddc { > - compatible = "samsung,exynos5-hdmiddc"; > + compatible = "samsung,exynos4210-hdmiddc"; > reg = <0x50>; > }; > diff --git a/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt > b/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt > index 858f4f9..e59d793 100644 > --- a/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt > +++ b/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt > @@ -1,12 +1,14 @@ > Device-Tree bindings for hdmiphy driver > > Required properties: > -- compatible: value should be "samsung,exynos5-hdmiphy". > +- compatible: value should be > + 1) "samsung,exynos4210-hdmiphy". > + 2) "samsung,exynos4212-hdmiphy". > - reg: I2C address of the hdmiphy device. > > Example: > > hdmiphy { > - compatible = "samsung,exynos5-hdmiphy"; > + compatible = "samsung,exynos4210-hdmiphy"; > reg = <0x38>; > }; > diff --git a/Documentation/devicetree/bindings/video/exynos_mixer.txt > b/Documentation/devicetree/bindings/video/exynos_mixer.txt > index 9b2ea03..a8b063f 100644 > --- a/Documentation/devicetree/bindings/video/exynos_mixer.txt > +++ b/Documentation/devicetree/bindings/video/exynos_mixer.txt > @@ -1,7 +1,10 @@ > Device-Tree bindings for mixer driver > > Required properties: > -- compatible: value should be "samsung,exynos5-mixer". > +- compatible: value should be: > + 1) "samsung,exynos4210-mixer" > + 2) "samsung,exynos5250-mixer" > + > - reg: physical base address of the mixer and length of memory mapped > region. > - interrupts: interrupt number to the cpu. > @@ -9,7 +12,7 @@ Required properties: > Example: > > mixer { > - compatible = "samsung,exynos5-mixer"; > + compatible = "samsung,exynos5250-mixer"; > reg = <0x1445 0x1>; > interrupts = <0 94 0>; > }; > diff --git a/drivers/gpu/drm/exynos/exynos_ddc.c > b/drivers/gpu/drm/exynos/exynos_ddc.c > index 4e9b5ba..1a0cca1 100644 > --- a/drivers/gpu/drm/exynos/exynos_ddc.c > +++ b/drivers/gpu/drm/exynos/exynos_ddc.c > @@ -51,7 +51,7 @@ static struct i2c_device_id ddc_idtable[] = { > #ifdef CONFIG_OF > static struct of_device_id hdmiddc_match_types[] = { > { > - .compatible = "samsung,exynos5-hdmiddc", > +
[PATCH 3/4] drm/exynos: fix interlace resolutions for exynos5420
Hi Rahul, This patch looks good to me. On 2013? 06? 18? 21:49, Rahul Sharma wrote: > Modified code for calculating hdmi IP register values from drm timing > values. The modification is based on the inputs from hw team and specifically > proposed for 1440x576i and 1440x480i. But same changes holds good for other > interlaced resolutions also. > > Signed-off-by: Rahul Sharma Acked-by: Seung-Woo Kim > --- > drivers/gpu/drm/exynos/exynos_hdmi.c | 14 -- > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c > b/drivers/gpu/drm/exynos/exynos_hdmi.c > index 8752171..2f807d5 100644 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c > @@ -1557,8 +1557,7 @@ static void hdmi_v14_mode_set(struct hdmi_context > *hdata, > (m->vsync_start - m->vdisplay) / 2); > hdmi_set_reg(core->v2_blank, 2, m->vtotal / 2); > hdmi_set_reg(core->v1_blank, 2, (m->vtotal - m->vdisplay) / 2); > - hdmi_set_reg(core->v_blank_f0, 2, (m->vtotal + > - ((m->vsync_end - m->vsync_start) * 4) + 5) / 2); > + hdmi_set_reg(core->v_blank_f0, 2, m->vtotal - m->vdisplay / 2); > hdmi_set_reg(core->v_blank_f1, 2, m->vtotal); > hdmi_set_reg(core->v_sync_line_aft_2, 2, (m->vtotal / 2) + 7); > hdmi_set_reg(core->v_sync_line_aft_1, 2, (m->vtotal / 2) + 2); > @@ -1568,7 +1567,10 @@ static void hdmi_v14_mode_set(struct hdmi_context > *hdata, > (m->htotal / 2) + (m->hsync_start - m->hdisplay)); > hdmi_set_reg(tg->vact_st, 2, (m->vtotal - m->vdisplay) / 2); > hdmi_set_reg(tg->vact_sz, 2, m->vdisplay / 2); > - hdmi_set_reg(tg->vact_st2, 2, 0x249);/* Reset value + 1*/ > + hdmi_set_reg(tg->vact_st2, 2, m->vtotal - m->vdisplay / 2); > + hdmi_set_reg(tg->vsync2, 2, (m->vtotal / 2) + 1); > + hdmi_set_reg(tg->vsync_bot_hdmi, 2, (m->vtotal / 2) + 1); > + hdmi_set_reg(tg->field_bot_hdmi, 2, (m->vtotal / 2) + 1); > hdmi_set_reg(tg->vact_st3, 2, 0x0); > hdmi_set_reg(tg->vact_st4, 2, 0x0); > } else { > @@ -1590,6 +1592,9 @@ static void hdmi_v14_mode_set(struct hdmi_context > *hdata, > hdmi_set_reg(tg->vact_st2, 2, 0x248); /* Reset value */ > hdmi_set_reg(tg->vact_st3, 2, 0x47b); /* Reset value */ > hdmi_set_reg(tg->vact_st4, 2, 0x6ae); /* Reset value */ > + hdmi_set_reg(tg->vsync2, 2, 0x233); /* Reset value */ > + hdmi_set_reg(tg->vsync_bot_hdmi, 2, 0x233); /* Reset value */ > + hdmi_set_reg(tg->field_bot_hdmi, 2, 0x233); /* Reset value */ > } > > /* Following values & calculations are same irrespective of mode type */ > @@ -1621,12 +1626,9 @@ static void hdmi_v14_mode_set(struct hdmi_context > *hdata, > hdmi_set_reg(tg->hact_sz, 2, m->hdisplay); > hdmi_set_reg(tg->v_fsz, 2, m->vtotal); > hdmi_set_reg(tg->vsync, 2, 0x1); > - hdmi_set_reg(tg->vsync2, 2, 0x233); /* Reset value */ > hdmi_set_reg(tg->field_chg, 2, 0x233); /* Reset value */ > hdmi_set_reg(tg->vsync_top_hdmi, 2, 0x1); /* Reset value */ > - hdmi_set_reg(tg->vsync_bot_hdmi, 2, 0x233); /* Reset value */ > hdmi_set_reg(tg->field_top_hdmi, 2, 0x1); /* Reset value */ > - hdmi_set_reg(tg->field_bot_hdmi, 2, 0x233); /* Reset value */ > hdmi_set_reg(tg->tg_3d, 1, 0x0); > } > > -- Seung-Woo Kim Samsung Software R Center --
[PATCH 2/4] drm/exynos: add support for exynos5420 mixer
Hi Rahul, Code part looks good to me but IMHO, binding document for exynos_mixer is also fixed like following because compitable string samsung,exynos5420-mixer is added with this patch. Required properties: - compatible: value should be: 1) "samsung,exynos4210-mixer" 2) "samsung,exynos5250-mixer" + 3) "samsung,exynos5420-mixer" Thanks and Regards, - Seung-Woo Kim On 2013? 06? 18? 21:49, Rahul Sharma wrote: > Add support for exynos5420 mixer IP in the drm mixer driver. > > Signed-off-by: Rahul Sharma > --- > drivers/gpu/drm/exynos/exynos_mixer.c | 49 > + > drivers/gpu/drm/exynos/regs-mixer.h |7 + > 2 files changed, 44 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c > b/drivers/gpu/drm/exynos/exynos_mixer.c > index 2fe6d33..d51ff36 100644 > --- a/drivers/gpu/drm/exynos/exynos_mixer.c > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c > @@ -78,6 +78,7 @@ struct mixer_resources { > enum mixer_version_id { > MXR_VER_0_0_0_16, > MXR_VER_16_0_33_0, > + MXR_VER_128_0_0_184, > }; > > struct mixer_context { > @@ -283,17 +284,19 @@ static void mixer_cfg_scan(struct mixer_context *ctx, > unsigned int height) > val = (ctx->interlace ? MXR_CFG_SCAN_INTERLACE : > MXR_CFG_SCAN_PROGRASSIVE); > > - /* choosing between porper HD and SD mode */ > - if (height <= 480) > - val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; > - else if (height <= 576) > - val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; > - else if (height <= 720) > - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > - else if (height <= 1080) > - val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; > - else > - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > + if (ctx->mxr_ver != MXR_VER_128_0_0_184) { > + /* choosing between proper HD and SD mode */ > + if (height <= 480) > + val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; > + else if (height <= 576) > + val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; > + else if (height <= 720) > + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > + else if (height <= 1080) > + val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; > + else > + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > + } > > mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_SCAN_MASK); > } > @@ -557,6 +560,14 @@ static void mixer_graph_buffer(struct mixer_context > *ctx, int win) > /* setup geometry */ > mixer_reg_write(res, MXR_GRAPHIC_SPAN(win), win_data->fb_width); > > + /* setup display size */ > + if (ctx->mxr_ver == MXR_VER_128_0_0_184 && > + win == MIXER_DEFAULT_WIN) { > + val = MXR_MXR_RES_HEIGHT(win_data->fb_height); > + val |= MXR_MXR_RES_WIDTH(win_data->fb_width); > + mixer_reg_write(res, MXR_RESOLUTION, val); > + } > + > val = MXR_GRP_WH_WIDTH(win_data->crtc_width); > val |= MXR_GRP_WH_HEIGHT(win_data->crtc_height); > val |= MXR_GRP_WH_H_SCALE(x_ratio); > @@ -581,7 +592,8 @@ static void mixer_graph_buffer(struct mixer_context *ctx, > int win) > mixer_cfg_layer(ctx, win, true); > > /* layer update mandatory for mixer 16.0.33.0 */ > - if (ctx->mxr_ver == MXR_VER_16_0_33_0) > + if (ctx->mxr_ver == MXR_VER_16_0_33_0 || > + ctx->mxr_ver == MXR_VER_128_0_0_184) > mixer_layer_update(ctx); > > mixer_run(ctx); > @@ -816,6 +828,7 @@ static void mixer_win_disable(void *ctx, int win) > > static int mixer_check_mode(void *ctx, struct drm_display_mode *mode) > { > + struct mixer_context *mixer_ctx = ctx; > u32 w, h; > > w = mode->hdisplay; > @@ -825,6 +838,10 @@ static int mixer_check_mode(void *ctx, struct > drm_display_mode *mode) > mode->hdisplay, mode->vdisplay, mode->vrefresh, > (mode->flags & DRM_MODE_FLAG_INTERLACE) ? 1 : 0); > > + if (mixer_ctx->mxr_ver == MXR_VER_0_0_0_16 || > + mixer_ctx->mxr_ver == MXR_VER_128_0_0_184) > + return 0; > + > if ((w >= 464 && w <= 720 && h >= 261 && h <= 576) || > (w >= 1024 && w <= 1280 && h >= 576 && h <= 720) || > (w >= 1664 && w <= 1920 && h >= 936 && h <= 1080)) > @@ -1115,6 +1132,11 @@ static int vp_resources_init(struct > exynos_drm_hdmi_context *ctx, > return 0; > } > > +static struct mixer_drv_data exynos5420_mxr_drv_data = { > + .version = MXR_VER_128_0_0_184, > + .is_vp_enabled = 0, > +}; > + > static struct mixer_drv_data exynos5250_mxr_drv_data = { > .version = MXR_VER_16_0_33_0, > .is_vp_enabled = 0, > @@ -1139,6 +1161,9 @@ static struct platform_device_id mixer_driver_types[] = > { > > static struct
Re: [PATCH v2 2/4] drm/exynos: add support for exynos5420 mixer
On 2013년 06월 19일 15:50, Rahul Sharma wrote: Add support for exynos5420 mixer IP in the drm mixer driver. Signed-off-by: Rahul Sharma rahul.sha...@samsung.com Acked-by: Seung-Woo Kim sw0312@samsung.com This patch can be merged after merging [PATCH 1/4] drm/exynos: rename compatible strings for hdmi subsystem. Best Regards, - Seung-Woo Kim --- .../devicetree/bindings/video/exynos_mixer.txt |1 + drivers/gpu/drm/exynos/exynos_mixer.c | 49 +++- drivers/gpu/drm/exynos/regs-mixer.h|7 +++ 3 files changed, 45 insertions(+), 12 deletions(-) diff --git a/Documentation/devicetree/bindings/video/exynos_mixer.txt b/Documentation/devicetree/bindings/video/exynos_mixer.txt index a8b063f..c64ddc1 100644 --- a/Documentation/devicetree/bindings/video/exynos_mixer.txt +++ b/Documentation/devicetree/bindings/video/exynos_mixer.txt @@ -4,6 +4,7 @@ Required properties: - compatible: value should be: 1) samsung,exynos4210-mixer 2) samsung,exynos5250-mixer + 3) samsung,exynos5420-mixer - reg: physical base address of the mixer and length of memory mapped region. diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 2fe6d33..d51ff36 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -78,6 +78,7 @@ struct mixer_resources { enum mixer_version_id { MXR_VER_0_0_0_16, MXR_VER_16_0_33_0, + MXR_VER_128_0_0_184, }; struct mixer_context { @@ -283,17 +284,19 @@ static void mixer_cfg_scan(struct mixer_context *ctx, unsigned int height) val = (ctx-interlace ? MXR_CFG_SCAN_INTERLACE : MXR_CFG_SCAN_PROGRASSIVE); - /* choosing between porper HD and SD mode */ - if (height = 480) - val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; - else if (height = 576) - val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; - else if (height = 720) - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; - else if (height = 1080) - val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; - else - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; + if (ctx-mxr_ver != MXR_VER_128_0_0_184) { + /* choosing between proper HD and SD mode */ + if (height = 480) + val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; + else if (height = 576) + val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; + else if (height = 720) + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; + else if (height = 1080) + val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; + else + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; + } mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_SCAN_MASK); } @@ -557,6 +560,14 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win) /* setup geometry */ mixer_reg_write(res, MXR_GRAPHIC_SPAN(win), win_data-fb_width); + /* setup display size */ + if (ctx-mxr_ver == MXR_VER_128_0_0_184 + win == MIXER_DEFAULT_WIN) { + val = MXR_MXR_RES_HEIGHT(win_data-fb_height); + val |= MXR_MXR_RES_WIDTH(win_data-fb_width); + mixer_reg_write(res, MXR_RESOLUTION, val); + } + val = MXR_GRP_WH_WIDTH(win_data-crtc_width); val |= MXR_GRP_WH_HEIGHT(win_data-crtc_height); val |= MXR_GRP_WH_H_SCALE(x_ratio); @@ -581,7 +592,8 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win) mixer_cfg_layer(ctx, win, true); /* layer update mandatory for mixer 16.0.33.0 */ - if (ctx-mxr_ver == MXR_VER_16_0_33_0) + if (ctx-mxr_ver == MXR_VER_16_0_33_0 || + ctx-mxr_ver == MXR_VER_128_0_0_184) mixer_layer_update(ctx); mixer_run(ctx); @@ -816,6 +828,7 @@ static void mixer_win_disable(void *ctx, int win) static int mixer_check_mode(void *ctx, struct drm_display_mode *mode) { + struct mixer_context *mixer_ctx = ctx; u32 w, h; w = mode-hdisplay; @@ -825,6 +838,10 @@ static int mixer_check_mode(void *ctx, struct drm_display_mode *mode) mode-hdisplay, mode-vdisplay, mode-vrefresh, (mode-flags DRM_MODE_FLAG_INTERLACE) ? 1 : 0); + if (mixer_ctx-mxr_ver == MXR_VER_0_0_0_16 || + mixer_ctx-mxr_ver == MXR_VER_128_0_0_184) + return 0; + if ((w = 464 w = 720 h = 261 h = 576) || (w = 1024 w = 1280 h = 576 h = 720) || (w = 1664 w = 1920 h = 936 h = 1080)) @@ -1115,6 +1132,11 @@ static int vp_resources_init(struct exynos_drm_hdmi_context *ctx, return 0; } +static struct mixer_drv_data exynos5420_mxr_drv_data = { + .version =
Re: [PATCH v3 2/3] drm/exynos: add support for exynos5420 mixer
Hello Rahul, This patch is exactly same with v2 2/4 and only rebased on v3 1/3, so my ack is valid for this patch. On 2013년 06월 19일 21:51, Rahul Sharma wrote: Add support for exynos5420 mixer IP in the drm mixer driver. Signed-off-by: Rahul Sharma rahul.sha...@samsung.com Acked-by: Seung-Woo Kim sw0312@samsung.com Anyway, this patch can be merged after merging [Patch v3 1/3] as like v2. Best Regards, - Seung-Woo Kim --- .../devicetree/bindings/video/exynos_mixer.txt |1 + drivers/gpu/drm/exynos/exynos_mixer.c | 49 +++- drivers/gpu/drm/exynos/regs-mixer.h|7 +++ 3 files changed, 45 insertions(+), 12 deletions(-) diff --git a/Documentation/devicetree/bindings/video/exynos_mixer.txt b/Documentation/devicetree/bindings/video/exynos_mixer.txt index 9131b99..3334b0a 100644 --- a/Documentation/devicetree/bindings/video/exynos_mixer.txt +++ b/Documentation/devicetree/bindings/video/exynos_mixer.txt @@ -5,6 +5,7 @@ Required properties: 1) samsung,exynos5-mixer DEPRECATED 2) samsung,exynos4210-mixer 3) samsung,exynos5250-mixer + 4) samsung,exynos5420-mixer - reg: physical base address of the mixer and length of memory mapped region. diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 6225501..b1280b4 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -78,6 +78,7 @@ struct mixer_resources { enum mixer_version_id { MXR_VER_0_0_0_16, MXR_VER_16_0_33_0, + MXR_VER_128_0_0_184, }; struct mixer_context { @@ -283,17 +284,19 @@ static void mixer_cfg_scan(struct mixer_context *ctx, unsigned int height) val = (ctx-interlace ? MXR_CFG_SCAN_INTERLACE : MXR_CFG_SCAN_PROGRASSIVE); - /* choosing between porper HD and SD mode */ - if (height = 480) - val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; - else if (height = 576) - val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; - else if (height = 720) - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; - else if (height = 1080) - val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; - else - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; + if (ctx-mxr_ver != MXR_VER_128_0_0_184) { + /* choosing between proper HD and SD mode */ + if (height = 480) + val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; + else if (height = 576) + val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; + else if (height = 720) + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; + else if (height = 1080) + val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; + else + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; + } mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_SCAN_MASK); } @@ -557,6 +560,14 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win) /* setup geometry */ mixer_reg_write(res, MXR_GRAPHIC_SPAN(win), win_data-fb_width); + /* setup display size */ + if (ctx-mxr_ver == MXR_VER_128_0_0_184 + win == MIXER_DEFAULT_WIN) { + val = MXR_MXR_RES_HEIGHT(win_data-fb_height); + val |= MXR_MXR_RES_WIDTH(win_data-fb_width); + mixer_reg_write(res, MXR_RESOLUTION, val); + } + val = MXR_GRP_WH_WIDTH(win_data-crtc_width); val |= MXR_GRP_WH_HEIGHT(win_data-crtc_height); val |= MXR_GRP_WH_H_SCALE(x_ratio); @@ -581,7 +592,8 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win) mixer_cfg_layer(ctx, win, true); /* layer update mandatory for mixer 16.0.33.0 */ - if (ctx-mxr_ver == MXR_VER_16_0_33_0) + if (ctx-mxr_ver == MXR_VER_16_0_33_0 || + ctx-mxr_ver == MXR_VER_128_0_0_184) mixer_layer_update(ctx); mixer_run(ctx); @@ -816,6 +828,7 @@ static void mixer_win_disable(void *ctx, int win) static int mixer_check_mode(void *ctx, struct drm_display_mode *mode) { + struct mixer_context *mixer_ctx = ctx; u32 w, h; w = mode-hdisplay; @@ -825,6 +838,10 @@ static int mixer_check_mode(void *ctx, struct drm_display_mode *mode) mode-hdisplay, mode-vdisplay, mode-vrefresh, (mode-flags DRM_MODE_FLAG_INTERLACE) ? 1 : 0); + if (mixer_ctx-mxr_ver == MXR_VER_0_0_0_16 || + mixer_ctx-mxr_ver == MXR_VER_128_0_0_184) + return 0; + if ((w = 464 w = 720 h = 261 h = 576) || (w = 1024 w = 1280 h = 576 h = 720) || (w = 1664 w = 1920 h = 936 h = 1080)) @@ -1115,6 +1132,11 @@ static int vp_resources_init(struct exynos_drm_hdmi_context *ctx, return 0; }
Re: [PATCH 2/4] drm/exynos: add support for exynos5420 mixer
Hi Rahul, Code part looks good to me but IMHO, binding document for exynos_mixer is also fixed like following because compitable string samsung,exynos5420-mixer is added with this patch. Required properties: - compatible: value should be: 1) samsung,exynos4210-mixer 2) samsung,exynos5250-mixer + 3) samsung,exynos5420-mixer Thanks and Regards, - Seung-Woo Kim On 2013년 06월 18일 21:49, Rahul Sharma wrote: Add support for exynos5420 mixer IP in the drm mixer driver. Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- drivers/gpu/drm/exynos/exynos_mixer.c | 49 + drivers/gpu/drm/exynos/regs-mixer.h |7 + 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 2fe6d33..d51ff36 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -78,6 +78,7 @@ struct mixer_resources { enum mixer_version_id { MXR_VER_0_0_0_16, MXR_VER_16_0_33_0, + MXR_VER_128_0_0_184, }; struct mixer_context { @@ -283,17 +284,19 @@ static void mixer_cfg_scan(struct mixer_context *ctx, unsigned int height) val = (ctx-interlace ? MXR_CFG_SCAN_INTERLACE : MXR_CFG_SCAN_PROGRASSIVE); - /* choosing between porper HD and SD mode */ - if (height = 480) - val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; - else if (height = 576) - val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; - else if (height = 720) - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; - else if (height = 1080) - val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; - else - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; + if (ctx-mxr_ver != MXR_VER_128_0_0_184) { + /* choosing between proper HD and SD mode */ + if (height = 480) + val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; + else if (height = 576) + val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; + else if (height = 720) + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; + else if (height = 1080) + val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; + else + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; + } mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_SCAN_MASK); } @@ -557,6 +560,14 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win) /* setup geometry */ mixer_reg_write(res, MXR_GRAPHIC_SPAN(win), win_data-fb_width); + /* setup display size */ + if (ctx-mxr_ver == MXR_VER_128_0_0_184 + win == MIXER_DEFAULT_WIN) { + val = MXR_MXR_RES_HEIGHT(win_data-fb_height); + val |= MXR_MXR_RES_WIDTH(win_data-fb_width); + mixer_reg_write(res, MXR_RESOLUTION, val); + } + val = MXR_GRP_WH_WIDTH(win_data-crtc_width); val |= MXR_GRP_WH_HEIGHT(win_data-crtc_height); val |= MXR_GRP_WH_H_SCALE(x_ratio); @@ -581,7 +592,8 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win) mixer_cfg_layer(ctx, win, true); /* layer update mandatory for mixer 16.0.33.0 */ - if (ctx-mxr_ver == MXR_VER_16_0_33_0) + if (ctx-mxr_ver == MXR_VER_16_0_33_0 || + ctx-mxr_ver == MXR_VER_128_0_0_184) mixer_layer_update(ctx); mixer_run(ctx); @@ -816,6 +828,7 @@ static void mixer_win_disable(void *ctx, int win) static int mixer_check_mode(void *ctx, struct drm_display_mode *mode) { + struct mixer_context *mixer_ctx = ctx; u32 w, h; w = mode-hdisplay; @@ -825,6 +838,10 @@ static int mixer_check_mode(void *ctx, struct drm_display_mode *mode) mode-hdisplay, mode-vdisplay, mode-vrefresh, (mode-flags DRM_MODE_FLAG_INTERLACE) ? 1 : 0); + if (mixer_ctx-mxr_ver == MXR_VER_0_0_0_16 || + mixer_ctx-mxr_ver == MXR_VER_128_0_0_184) + return 0; + if ((w = 464 w = 720 h = 261 h = 576) || (w = 1024 w = 1280 h = 576 h = 720) || (w = 1664 w = 1920 h = 936 h = 1080)) @@ -1115,6 +1132,11 @@ static int vp_resources_init(struct exynos_drm_hdmi_context *ctx, return 0; } +static struct mixer_drv_data exynos5420_mxr_drv_data = { + .version = MXR_VER_128_0_0_184, + .is_vp_enabled = 0, +}; + static struct mixer_drv_data exynos5250_mxr_drv_data = { .version = MXR_VER_16_0_33_0, .is_vp_enabled = 0, @@ -1139,6 +1161,9 @@ static struct platform_device_id mixer_driver_types[] = { static struct of_device_id mixer_match_types[] = { { + .compatible = samsung,exynos5420-mixer, + .data = exynos5420_mxr_drv_data, + }, {
Re: [PATCH 3/4] drm/exynos: fix interlace resolutions for exynos5420
Hi Rahul, This patch looks good to me. On 2013년 06월 18일 21:49, Rahul Sharma wrote: Modified code for calculating hdmi IP register values from drm timing values. The modification is based on the inputs from hw team and specifically proposed for 1440x576i and 1440x480i. But same changes holds good for other interlaced resolutions also. Signed-off-by: Rahul Sharma rahul.sha...@samsung.com Acked-by: Seung-Woo Kim sw0312@samsung.com --- drivers/gpu/drm/exynos/exynos_hdmi.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 8752171..2f807d5 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -1557,8 +1557,7 @@ static void hdmi_v14_mode_set(struct hdmi_context *hdata, (m-vsync_start - m-vdisplay) / 2); hdmi_set_reg(core-v2_blank, 2, m-vtotal / 2); hdmi_set_reg(core-v1_blank, 2, (m-vtotal - m-vdisplay) / 2); - hdmi_set_reg(core-v_blank_f0, 2, (m-vtotal + - ((m-vsync_end - m-vsync_start) * 4) + 5) / 2); + hdmi_set_reg(core-v_blank_f0, 2, m-vtotal - m-vdisplay / 2); hdmi_set_reg(core-v_blank_f1, 2, m-vtotal); hdmi_set_reg(core-v_sync_line_aft_2, 2, (m-vtotal / 2) + 7); hdmi_set_reg(core-v_sync_line_aft_1, 2, (m-vtotal / 2) + 2); @@ -1568,7 +1567,10 @@ static void hdmi_v14_mode_set(struct hdmi_context *hdata, (m-htotal / 2) + (m-hsync_start - m-hdisplay)); hdmi_set_reg(tg-vact_st, 2, (m-vtotal - m-vdisplay) / 2); hdmi_set_reg(tg-vact_sz, 2, m-vdisplay / 2); - hdmi_set_reg(tg-vact_st2, 2, 0x249);/* Reset value + 1*/ + hdmi_set_reg(tg-vact_st2, 2, m-vtotal - m-vdisplay / 2); + hdmi_set_reg(tg-vsync2, 2, (m-vtotal / 2) + 1); + hdmi_set_reg(tg-vsync_bot_hdmi, 2, (m-vtotal / 2) + 1); + hdmi_set_reg(tg-field_bot_hdmi, 2, (m-vtotal / 2) + 1); hdmi_set_reg(tg-vact_st3, 2, 0x0); hdmi_set_reg(tg-vact_st4, 2, 0x0); } else { @@ -1590,6 +1592,9 @@ static void hdmi_v14_mode_set(struct hdmi_context *hdata, hdmi_set_reg(tg-vact_st2, 2, 0x248); /* Reset value */ hdmi_set_reg(tg-vact_st3, 2, 0x47b); /* Reset value */ hdmi_set_reg(tg-vact_st4, 2, 0x6ae); /* Reset value */ + hdmi_set_reg(tg-vsync2, 2, 0x233); /* Reset value */ + hdmi_set_reg(tg-vsync_bot_hdmi, 2, 0x233); /* Reset value */ + hdmi_set_reg(tg-field_bot_hdmi, 2, 0x233); /* Reset value */ } /* Following values calculations are same irrespective of mode type */ @@ -1621,12 +1626,9 @@ static void hdmi_v14_mode_set(struct hdmi_context *hdata, hdmi_set_reg(tg-hact_sz, 2, m-hdisplay); hdmi_set_reg(tg-v_fsz, 2, m-vtotal); hdmi_set_reg(tg-vsync, 2, 0x1); - hdmi_set_reg(tg-vsync2, 2, 0x233); /* Reset value */ hdmi_set_reg(tg-field_chg, 2, 0x233); /* Reset value */ hdmi_set_reg(tg-vsync_top_hdmi, 2, 0x1); /* Reset value */ - hdmi_set_reg(tg-vsync_bot_hdmi, 2, 0x233); /* Reset value */ hdmi_set_reg(tg-field_top_hdmi, 2, 0x1); /* Reset value */ - hdmi_set_reg(tg-field_bot_hdmi, 2, 0x233); /* Reset value */ hdmi_set_reg(tg-tg_3d, 1, 0x0); } -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm/exynos: rename compatible strings for hdmi subsystem
Hi Rahul, It looks good, at least, to me. Best Regards, - Seung-Woo Kim On 2013년 06월 18일 21:49, Rahul Sharma wrote: This patch renames the combatible strings for hdmi, mixer, ddc and hdmiphy. It follows the convention of using compatible string which represent the SoC in which the IP was added for the first time. Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- Documentation/devicetree/bindings/video/exynos_hdmi.txt|6 -- Documentation/devicetree/bindings/video/exynos_hdmiddc.txt |4 ++-- Documentation/devicetree/bindings/video/exynos_hdmiphy.txt |6 -- Documentation/devicetree/bindings/video/exynos_mixer.txt |7 +-- drivers/gpu/drm/exynos/exynos_ddc.c|2 +- drivers/gpu/drm/exynos/exynos_hdmi.c |2 +- drivers/gpu/drm/exynos/exynos_hdmiphy.c|4 +++- drivers/gpu/drm/exynos/exynos_mixer.c | 12 ++-- 8 files changed, 26 insertions(+), 17 deletions(-) diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt index 589edee..2ac01ca 100644 --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt @@ -1,7 +1,9 @@ Device-Tree bindings for drm hdmi driver Required properties: -- compatible: value should be samsung,exynos5-hdmi. +- compatible: value should be one among the following: + 1) samsung,exynos4210-hdmi + 2) samsung,exynos4212-hdmi - reg: physical base address of the hdmi and length of memory mapped region. - interrupts: interrupt number to the cpu. @@ -15,7 +17,7 @@ Required properties: Example: hdmi { - compatible = samsung,exynos5-hdmi; + compatible = samsung,exynos4212-hdmi; reg = 0x1453 0x10; interrupts = 0 95 0; hpd-gpio = gpx3 7 0xf 1 3; diff --git a/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt b/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt index fa166d9..c1bd2ea 100644 --- a/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt +++ b/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt @@ -1,12 +1,12 @@ Device-Tree bindings for hdmiddc driver Required properties: -- compatible: value should be samsung,exynos5-hdmiddc. +- compatible: value should be samsung,exynos4210-hdmiddc. - reg: I2C address of the hdmiddc device. Example: hdmiddc { - compatible = samsung,exynos5-hdmiddc; + compatible = samsung,exynos4210-hdmiddc; reg = 0x50; }; diff --git a/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt b/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt index 858f4f9..e59d793 100644 --- a/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt +++ b/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt @@ -1,12 +1,14 @@ Device-Tree bindings for hdmiphy driver Required properties: -- compatible: value should be samsung,exynos5-hdmiphy. +- compatible: value should be + 1) samsung,exynos4210-hdmiphy. + 2) samsung,exynos4212-hdmiphy. - reg: I2C address of the hdmiphy device. Example: hdmiphy { - compatible = samsung,exynos5-hdmiphy; + compatible = samsung,exynos4210-hdmiphy; reg = 0x38; }; diff --git a/Documentation/devicetree/bindings/video/exynos_mixer.txt b/Documentation/devicetree/bindings/video/exynos_mixer.txt index 9b2ea03..a8b063f 100644 --- a/Documentation/devicetree/bindings/video/exynos_mixer.txt +++ b/Documentation/devicetree/bindings/video/exynos_mixer.txt @@ -1,7 +1,10 @@ Device-Tree bindings for mixer driver Required properties: -- compatible: value should be samsung,exynos5-mixer. +- compatible: value should be: + 1) samsung,exynos4210-mixer + 2) samsung,exynos5250-mixer + - reg: physical base address of the mixer and length of memory mapped region. - interrupts: interrupt number to the cpu. @@ -9,7 +12,7 @@ Required properties: Example: mixer { - compatible = samsung,exynos5-mixer; + compatible = samsung,exynos5250-mixer; reg = 0x1445 0x1; interrupts = 0 94 0; }; diff --git a/drivers/gpu/drm/exynos/exynos_ddc.c b/drivers/gpu/drm/exynos/exynos_ddc.c index 4e9b5ba..1a0cca1 100644 --- a/drivers/gpu/drm/exynos/exynos_ddc.c +++ b/drivers/gpu/drm/exynos/exynos_ddc.c @@ -51,7 +51,7 @@ static struct i2c_device_id ddc_idtable[] = { #ifdef CONFIG_OF static struct of_device_id hdmiddc_match_types[] = { { - .compatible = samsung,exynos5-hdmiddc, + .compatible = samsung,exynos4210-hdmiddc, }, { /* end node */ } diff --git
[PATCH 7/9] drm/exynos: use of_get_named_gpio to get hdmi hpd gpio
Hello Rahul, this patch is not related with others and it looks good to me. On 2013? 06? 11? 23:11, Rahul Sharma wrote: > Cleanup by removing flags variable from drm_hdmi_dt_parse_pdata > which is not used anywhere. Swtiching to of_get_named_gpio instead > of of_get_named_gpio_flags solved this. > > Signed-off-by: Rahul Sharma Acked-by: Seung-Woo Kim > --- > drivers/gpu/drm/exynos/exynos_hdmi.c |3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c > b/drivers/gpu/drm/exynos/exynos_hdmi.c > index 1eb5ffb..fc6a9b0 100644 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c > @@ -2081,7 +2081,6 @@ static struct s5p_hdmi_platform_data > *drm_hdmi_dt_parse_pdata > { > struct device_node *np = dev->of_node; > struct s5p_hdmi_platform_data *pd; > - enum of_gpio_flags flags; > u32 value; > > pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL); > @@ -2095,7 +2094,7 @@ static struct s5p_hdmi_platform_data > *drm_hdmi_dt_parse_pdata > goto err_data; > } > > - pd->hpd_gpio = of_get_named_gpio_flags(np, "hpd-gpio", 0, ); > + pd->hpd_gpio = of_get_named_gpio(np, "hpd-gpio", 0); > > return pd; > > -- Seung-Woo Kim Samsung Software R Center --
[PATCH 5/9] drm/exynos: add support for exynos5420 mixer
Hello Rahul, This patch looks good to me just with mixer part of 2nd patch because there is no hdmiphy related things. On 2013? 06? 11? 23:11, Rahul Sharma wrote: > Add support for exynos5420 mixer IP in the drm mixer driver. > > Signed-off-by: Rahul Sharma > --- > drivers/gpu/drm/exynos/exynos_mixer.c | 49 > + > drivers/gpu/drm/exynos/regs-mixer.h |7 + > 2 files changed, 44 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c > b/drivers/gpu/drm/exynos/exynos_mixer.c > index 58dfd3f..101d5bb 100644 > --- a/drivers/gpu/drm/exynos/exynos_mixer.c > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c > @@ -78,6 +78,7 @@ struct mixer_resources { > enum mixer_version_id { > MXR_VER_0_0_0_16, > MXR_VER_16_0_33_0, > + MXR_VER_128_0_0_184, > }; > > struct mixer_context { > @@ -283,17 +284,19 @@ static void mixer_cfg_scan(struct mixer_context *ctx, > unsigned int height) > val = (ctx->interlace ? MXR_CFG_SCAN_INTERLACE : > MXR_CFG_SCAN_PROGRASSIVE); > > - /* choosing between porper HD and SD mode */ > - if (height <= 480) > - val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; > - else if (height <= 576) > - val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; > - else if (height <= 720) > - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > - else if (height <= 1080) > - val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; > - else > - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > + if (ctx->mxr_ver != MXR_VER_128_0_0_184) { > + /* choosing between proper HD and SD mode */ > + if (height <= 480) > + val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; > + else if (height <= 576) > + val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; > + else if (height <= 720) > + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > + else if (height <= 1080) > + val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; > + else > + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; > + } > > mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_SCAN_MASK); > } > @@ -557,6 +560,14 @@ static void mixer_graph_buffer(struct mixer_context > *ctx, int win) > /* setup geometry */ > mixer_reg_write(res, MXR_GRAPHIC_SPAN(win), win_data->fb_width); > > + /* setup display size */ > + if (ctx->mxr_ver == MXR_VER_128_0_0_184 && > + win == MIXER_DEFAULT_WIN) { > + val = MXR_MXR_RES_HEIGHT(win_data->fb_height); > + val |= MXR_MXR_RES_WIDTH(win_data->fb_width); > + mixer_reg_write(res, MXR_RESOLUTION, val); > + } > + > val = MXR_GRP_WH_WIDTH(win_data->crtc_width); > val |= MXR_GRP_WH_HEIGHT(win_data->crtc_height); > val |= MXR_GRP_WH_H_SCALE(x_ratio); > @@ -581,7 +592,8 @@ static void mixer_graph_buffer(struct mixer_context *ctx, > int win) > mixer_cfg_layer(ctx, win, true); > > /* layer update mandatory for mixer 16.0.33.0 */ > - if (ctx->mxr_ver == MXR_VER_16_0_33_0) > + if (ctx->mxr_ver == MXR_VER_16_0_33_0 || > + ctx->mxr_ver == MXR_VER_128_0_0_184) > mixer_layer_update(ctx); > > mixer_run(ctx); > @@ -822,6 +834,7 @@ static void mixer_win_disable(void *ctx, int win) > > static int mixer_check_mode(void *ctx, struct drm_display_mode *mode) > { > + struct mixer_context *mixer_ctx = ctx; > u32 w, h; > > w = mode->hdisplay; > @@ -831,6 +844,10 @@ static int mixer_check_mode(void *ctx, struct > drm_display_mode *mode) > mode->hdisplay, mode->vdisplay, mode->vrefresh, > (mode->flags & DRM_MODE_FLAG_INTERLACE) ? 1 : 0); > > + if (mixer_ctx->mxr_ver == MXR_VER_0_0_0_16 || > + mixer_ctx->mxr_ver == MXR_VER_128_0_0_184) > + return 0; > + > if ((w >= 464 && w <= 720 && h >= 261 && h <= 576) || > (w >= 1024 && w <= 1280 && h >= 576 && h <= 720) || > (w >= 1664 && w <= 1920 && h >= 936 && h <= 1080)) > @@ -1127,6 +1144,11 @@ static int vp_resources_init(struct > exynos_drm_hdmi_context *ctx, > return 0; > } > > +static struct mixer_drv_data exynos5420_mxr_drv_data = { > + .version = MXR_VER_128_0_0_184, > + .is_vp_enabled = 0, > +}; > + > static struct mixer_drv_data exynos5250_mxr_drv_data = { > .version = MXR_VER_16_0_33_0, > .is_vp_enabled = 0, > @@ -1151,6 +1173,9 @@ static struct platform_device_id mixer_driver_types[] = > { > > static struct of_device_id mixer_match_types[] = { > { > + .compatible = "samsung,exynos5420-mixer", > + .data = _mxr_drv_data, > + }, { > .compatible = "samsung,exynos5250-mixer", > .data = _mxr_drv_data, > }, { > diff
[PATCH 1/9] drm/exynos: use SoC name to identify hdmi version
Hello Rahul, On 2013? 06? 11? 23:11, Rahul Sharma wrote: > Exynos hdmi IP version is named after hdmi specification version i.e. > 1.3 and 1.4. This versioning mechanism is not sufficient to handle > the diversity in the hdmi/phy IPs which are present across the exynos > SoC family. > > This patch changes the hdmi version to the name of the SoC in which > the IP was introduced for the first time. Same version is applicable > to all subsequent SoCs having the same IP version. > > Exynos4210 has 1.3 HDMI, i2c mapped phy with configuration set. > Exynos5250 has 1.4 HDMI, i2c mapped phy with configuration set. > Exynos5420 has 1.4 HDMI, Platform Bus mapped phy with configuration set. > > Based on the HDMI IP version we cannot decide to pick Exynos5250 phy conf > and use i2c for data transfer or Exynos5420 phy confs and platform bus > calls for communication. Considering your other patch to divide hdmi and hdmiphy, how do you think using hdmiphy version parsed from hdmiphy dt binding from phy code instead of using hdmi version for both hdmi and hdmiphy? If that, this SoC identifying hdmi version is not necessary because there is no change at least in hdmi side. And IMO, it seems easy to merge hdmiphy related patch first before merging patch for exynos5420. > > Signed-off-by: Rahul Sharma > --- > drivers/gpu/drm/exynos/exynos_hdmi.c | 249 > +- > drivers/gpu/drm/exynos/regs-hdmi.h | 78 +-- > 2 files changed, 164 insertions(+), 163 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c > b/drivers/gpu/drm/exynos/exynos_hdmi.c > index 75a6bf3..9384ffc 100644 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c > @@ -73,9 +73,9 @@ enum HDMI_PACKET_TYPE { > HDMI_PACKET_TYPE_AUI = HDMI_PACKET_TYPE_INFOFRAME + 4 > }; > > -enum hdmi_type { > - HDMI_TYPE13, > - HDMI_TYPE14, > +enum hdmi_version { > + HDMI_VER_EXYNOS4210, > + HDMI_VER_EXYNOS4212, > }; -- Seung-Woo Kim Samsung Software R Center --
[RFC 0/2] exynos5250/hdmi: replace dummy hdmiphy clock with pmu reg control
Hello Kishon, On 2013? 06? 13? 21:54, Kishon Vijay Abraham I wrote: > Hi, > > On Thursday 13 June 2013 04:51 PM, Inki Dae wrote: >> >> >>> -Original Message- >>> From: Sylwester Nawrocki [mailto:s.nawrocki at samsung.com] >>> Sent: Thursday, June 13, 2013 5:56 PM >>> To: Rahul Sharma >>> Cc: Rahul Sharma; Inki Dae; linux-samsung-soc at vger.kernel.org; >>> devicetree- >>> discuss at lists.ozlabs.org; DRI mailing list; Kukjin Kim; Seung-Woo Kim; >>> Sean Paul; sunil joshi; Kishon Vijay Abraham I; Stephen Warren; >>> grant.likely at linaro.org >>> Subject: Re: [RFC 0/2] exynos5250/hdmi: replace dummy hdmiphy clock with >>> pmu reg control >>> >>> Hi, >>> >>> On 06/13/2013 06:26 AM, Rahul Sharma wrote: Mr. Dae, Thanks for your valuable inputs. I posted it as RFC because, I also have received comments to register hdmiphy as a clock controller. As we always configure it for specific frequency, hdmi-phy looks similar to a PLL. But it really doesn't belong to that class. Secondly prior to exynos5420, it was a i2c device. I am not sure we can register a I2C device as a clock controller. I wanted to discuss and explore this option here. >>> >>> Have you considered using the generic PHY framework for those HDMI >>> PHY devices [1] ? I guess we could add a dedicated group of ops for >>> video PHYs, similarly as is is done with struct v4l2_subdev_ops. For >>> configuring things like the carrier/pixel clock frequency or anything >>> what's common across the video PHYs. >>> >>> Perhaps you could have a look and see if this framework would be >>> useful for HDMI and possibly point out anything what might be missing ? >>> >>> I'm not sure it it really solves the issues specific to the Exynos >>> HDMI but at least with a generic PHY driver the PHY module would be >>> separate from the PHY controller, as often same HDMI DPHY can be used >>> with various types of a HDMI controller. So this would allow to not >>> duplicate the HDMI PHY drivers in the long-term perspective. >> >> Yeah, at least, it seems that we could use PHY module to control PMU >> register, HDMI_PHY_CONTROL. However, PHY module provides only init/on/off >> callbacks. As you may know, HDMIPHY needs i2c interfaces to control >> HDMIPHY >> clock. So with PHY module, HDMIPHY driver could enable PMU more >> generically, >> but also has to use existing i2c stuff to control HDMIPHY clock. I had a >> quick review to Generic PHY Framework[v6] but I didn't see that the PHY >> module could generically support more features such as i2c stuff. > > I don't think PHY framework needs to provide i2c interfaces to program > certain configurations. Instead in one of the callbacks (init/on/off) > PHY driver can program whatever it wants using any of the interfaces it > needs. IMO PHY framework should work independent of the interfaces. In exnoys hdmi case, i2c interface is not the exact issue. In exynos hdmi, hdmiphy should send i2c configuration about video clock information as the video mode information including resolution, bit per pixel, refresh rate passed from drm subsystem. So init/on/off callbacks of phy framework are not enough for exynos hdmiphy and it should have a callback to set video mode. Do you have plan to add driver specific extend callback pointers to phy framework? Currently, hdmi directly calls phy operations, but Rahul's another patch set, mentioned by Inki, divides hdmi and hdmiphy and hdmi and hdmiphy is connected with exynos hdmi own sub driver callback operations. IMHO, if phy framework can support extend callback feature, then this own sub driver callbacks can be replaced with phy framework at long term view. Thanks and Regards, - Seung-Woo Kim > > For example, twl4030 phy driver actually uses i2c to program its > registers but still it uses the PHY framework [1]. > > [1] --> http://www.gossamer-threads.com/lists/linux/kernel/1729414 > > Thanks > Kishon > -- > To unsubscribe from this list: send the line "unsubscribe > linux-samsung-soc" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Seung-Woo Kim Samsung Software R Center --
Re: [RFC 0/2] exynos5250/hdmi: replace dummy hdmiphy clock with pmu reg control
Hello Kishon, On 2013년 06월 13일 21:54, Kishon Vijay Abraham I wrote: Hi, On Thursday 13 June 2013 04:51 PM, Inki Dae wrote: -Original Message- From: Sylwester Nawrocki [mailto:s.nawro...@samsung.com] Sent: Thursday, June 13, 2013 5:56 PM To: Rahul Sharma Cc: Rahul Sharma; Inki Dae; linux-samsung-...@vger.kernel.org; devicetree- disc...@lists.ozlabs.org; DRI mailing list; Kukjin Kim; Seung-Woo Kim; Sean Paul; sunil joshi; Kishon Vijay Abraham I; Stephen Warren; grant.lik...@linaro.org Subject: Re: [RFC 0/2] exynos5250/hdmi: replace dummy hdmiphy clock with pmu reg control Hi, On 06/13/2013 06:26 AM, Rahul Sharma wrote: Mr. Dae, Thanks for your valuable inputs. I posted it as RFC because, I also have received comments to register hdmiphy as a clock controller. As we always configure it for specific frequency, hdmi-phy looks similar to a PLL. But it really doesn't belong to that class. Secondly prior to exynos5420, it was a i2c device. I am not sure we can register a I2C device as a clock controller. I wanted to discuss and explore this option here. Have you considered using the generic PHY framework for those HDMI PHY devices [1] ? I guess we could add a dedicated group of ops for video PHYs, similarly as is is done with struct v4l2_subdev_ops. For configuring things like the carrier/pixel clock frequency or anything what's common across the video PHYs. Perhaps you could have a look and see if this framework would be useful for HDMI and possibly point out anything what might be missing ? I'm not sure it it really solves the issues specific to the Exynos HDMI but at least with a generic PHY driver the PHY module would be separate from the PHY controller, as often same HDMI DPHY can be used with various types of a HDMI controller. So this would allow to not duplicate the HDMI PHY drivers in the long-term perspective. Yeah, at least, it seems that we could use PHY module to control PMU register, HDMI_PHY_CONTROL. However, PHY module provides only init/on/off callbacks. As you may know, HDMIPHY needs i2c interfaces to control HDMIPHY clock. So with PHY module, HDMIPHY driver could enable PMU more generically, but also has to use existing i2c stuff to control HDMIPHY clock. I had a quick review to Generic PHY Framework[v6] but I didn't see that the PHY module could generically support more features such as i2c stuff. I don't think PHY framework needs to provide i2c interfaces to program certain configurations. Instead in one of the callbacks (init/on/off) PHY driver can program whatever it wants using any of the interfaces it needs. IMO PHY framework should work independent of the interfaces. In exnoys hdmi case, i2c interface is not the exact issue. In exynos hdmi, hdmiphy should send i2c configuration about video clock information as the video mode information including resolution, bit per pixel, refresh rate passed from drm subsystem. So init/on/off callbacks of phy framework are not enough for exynos hdmiphy and it should have a callback to set video mode. Do you have plan to add driver specific extend callback pointers to phy framework? Currently, hdmi directly calls phy operations, but Rahul's another patch set, mentioned by Inki, divides hdmi and hdmiphy and hdmi and hdmiphy is connected with exynos hdmi own sub driver callback operations. IMHO, if phy framework can support extend callback feature, then this own sub driver callbacks can be replaced with phy framework at long term view. Thanks and Regards, - Seung-Woo Kim For example, twl4030 phy driver actually uses i2c to program its registers but still it uses the PHY framework [1]. [1] -- http://www.gossamer-threads.com/lists/linux/kernel/1729414 Thanks Kishon -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/9] drm/exynos: use SoC name to identify hdmi version
Hello Rahul, On 2013년 06월 11일 23:11, Rahul Sharma wrote: Exynos hdmi IP version is named after hdmi specification version i.e. 1.3 and 1.4. This versioning mechanism is not sufficient to handle the diversity in the hdmi/phy IPs which are present across the exynos SoC family. This patch changes the hdmi version to the name of the SoC in which the IP was introduced for the first time. Same version is applicable to all subsequent SoCs having the same IP version. Exynos4210 has 1.3 HDMI, i2c mapped phy with configuration set. Exynos5250 has 1.4 HDMI, i2c mapped phy with configuration set. Exynos5420 has 1.4 HDMI, Platform Bus mapped phy with configuration set. Based on the HDMI IP version we cannot decide to pick Exynos5250 phy conf and use i2c for data transfer or Exynos5420 phy confs and platform bus calls for communication. Considering your other patch to divide hdmi and hdmiphy, how do you think using hdmiphy version parsed from hdmiphy dt binding from phy code instead of using hdmi version for both hdmi and hdmiphy? If that, this SoC identifying hdmi version is not necessary because there is no change at least in hdmi side. And IMO, it seems easy to merge hdmiphy related patch first before merging patch for exynos5420. Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- drivers/gpu/drm/exynos/exynos_hdmi.c | 249 +- drivers/gpu/drm/exynos/regs-hdmi.h | 78 +-- 2 files changed, 164 insertions(+), 163 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 75a6bf3..9384ffc 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -73,9 +73,9 @@ enum HDMI_PACKET_TYPE { HDMI_PACKET_TYPE_AUI = HDMI_PACKET_TYPE_INFOFRAME + 4 }; -enum hdmi_type { - HDMI_TYPE13, - HDMI_TYPE14, +enum hdmi_version { + HDMI_VER_EXYNOS4210, + HDMI_VER_EXYNOS4212, }; snip -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 5/9] drm/exynos: add support for exynos5420 mixer
Hello Rahul, This patch looks good to me just with mixer part of 2nd patch because there is no hdmiphy related things. On 2013년 06월 11일 23:11, Rahul Sharma wrote: Add support for exynos5420 mixer IP in the drm mixer driver. Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- drivers/gpu/drm/exynos/exynos_mixer.c | 49 + drivers/gpu/drm/exynos/regs-mixer.h |7 + 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 58dfd3f..101d5bb 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -78,6 +78,7 @@ struct mixer_resources { enum mixer_version_id { MXR_VER_0_0_0_16, MXR_VER_16_0_33_0, + MXR_VER_128_0_0_184, }; struct mixer_context { @@ -283,17 +284,19 @@ static void mixer_cfg_scan(struct mixer_context *ctx, unsigned int height) val = (ctx-interlace ? MXR_CFG_SCAN_INTERLACE : MXR_CFG_SCAN_PROGRASSIVE); - /* choosing between porper HD and SD mode */ - if (height = 480) - val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; - else if (height = 576) - val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; - else if (height = 720) - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; - else if (height = 1080) - val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; - else - val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; + if (ctx-mxr_ver != MXR_VER_128_0_0_184) { + /* choosing between proper HD and SD mode */ + if (height = 480) + val |= MXR_CFG_SCAN_NTSC | MXR_CFG_SCAN_SD; + else if (height = 576) + val |= MXR_CFG_SCAN_PAL | MXR_CFG_SCAN_SD; + else if (height = 720) + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; + else if (height = 1080) + val |= MXR_CFG_SCAN_HD_1080 | MXR_CFG_SCAN_HD; + else + val |= MXR_CFG_SCAN_HD_720 | MXR_CFG_SCAN_HD; + } mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_SCAN_MASK); } @@ -557,6 +560,14 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win) /* setup geometry */ mixer_reg_write(res, MXR_GRAPHIC_SPAN(win), win_data-fb_width); + /* setup display size */ + if (ctx-mxr_ver == MXR_VER_128_0_0_184 + win == MIXER_DEFAULT_WIN) { + val = MXR_MXR_RES_HEIGHT(win_data-fb_height); + val |= MXR_MXR_RES_WIDTH(win_data-fb_width); + mixer_reg_write(res, MXR_RESOLUTION, val); + } + val = MXR_GRP_WH_WIDTH(win_data-crtc_width); val |= MXR_GRP_WH_HEIGHT(win_data-crtc_height); val |= MXR_GRP_WH_H_SCALE(x_ratio); @@ -581,7 +592,8 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win) mixer_cfg_layer(ctx, win, true); /* layer update mandatory for mixer 16.0.33.0 */ - if (ctx-mxr_ver == MXR_VER_16_0_33_0) + if (ctx-mxr_ver == MXR_VER_16_0_33_0 || + ctx-mxr_ver == MXR_VER_128_0_0_184) mixer_layer_update(ctx); mixer_run(ctx); @@ -822,6 +834,7 @@ static void mixer_win_disable(void *ctx, int win) static int mixer_check_mode(void *ctx, struct drm_display_mode *mode) { + struct mixer_context *mixer_ctx = ctx; u32 w, h; w = mode-hdisplay; @@ -831,6 +844,10 @@ static int mixer_check_mode(void *ctx, struct drm_display_mode *mode) mode-hdisplay, mode-vdisplay, mode-vrefresh, (mode-flags DRM_MODE_FLAG_INTERLACE) ? 1 : 0); + if (mixer_ctx-mxr_ver == MXR_VER_0_0_0_16 || + mixer_ctx-mxr_ver == MXR_VER_128_0_0_184) + return 0; + if ((w = 464 w = 720 h = 261 h = 576) || (w = 1024 w = 1280 h = 576 h = 720) || (w = 1664 w = 1920 h = 936 h = 1080)) @@ -1127,6 +1144,11 @@ static int vp_resources_init(struct exynos_drm_hdmi_context *ctx, return 0; } +static struct mixer_drv_data exynos5420_mxr_drv_data = { + .version = MXR_VER_128_0_0_184, + .is_vp_enabled = 0, +}; + static struct mixer_drv_data exynos5250_mxr_drv_data = { .version = MXR_VER_16_0_33_0, .is_vp_enabled = 0, @@ -1151,6 +1173,9 @@ static struct platform_device_id mixer_driver_types[] = { static struct of_device_id mixer_match_types[] = { { + .compatible = samsung,exynos5420-mixer, + .data = exynos5420_mxr_drv_data, + }, { .compatible = samsung,exynos5250-mixer, .data = exynos5250_mxr_drv_data, }, { diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h index 5d8dbc0..4537026 100644 ---
Re: [PATCH 7/9] drm/exynos: use of_get_named_gpio to get hdmi hpd gpio
Hello Rahul, this patch is not related with others and it looks good to me. On 2013년 06월 11일 23:11, Rahul Sharma wrote: Cleanup by removing flags variable from drm_hdmi_dt_parse_pdata which is not used anywhere. Swtiching to of_get_named_gpio instead of of_get_named_gpio_flags solved this. Signed-off-by: Rahul Sharma rahul.sha...@samsung.com Acked-by: Seung-Woo Kim sw0312@samsung.com --- drivers/gpu/drm/exynos/exynos_hdmi.c |3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 1eb5ffb..fc6a9b0 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -2081,7 +2081,6 @@ static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata { struct device_node *np = dev-of_node; struct s5p_hdmi_platform_data *pd; - enum of_gpio_flags flags; u32 value; pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL); @@ -2095,7 +2094,7 @@ static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata goto err_data; } - pd-hpd_gpio = of_get_named_gpio_flags(np, hpd-gpio, 0, flags); + pd-hpd_gpio = of_get_named_gpio(np, hpd-gpio, 0); return pd; -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC][PATCH 1/2] dma-buf: add importer private data to attachment
On 2013? 06? 07? 20:24, Maarten Lankhorst wrote: > Op 07-06-13 04:32, ??? schreef: >> Hello Maarten, >> >> On 2013? 06? 05? 22:23, Maarten Lankhorst wrote: >>> Op 31-05-13 10:54, Seung-Woo Kim schreef: dma-buf attachment has only exporter private data, but importer private data can be useful for importer especially to re-import the same dma-buf. To use importer private data in attachment of the device, the function to search attachment in the attachment list of dma-buf is also added. Signed-off-by: Seung-Woo Kim --- drivers/base/dma-buf.c | 31 +++ include/linux/dma-buf.h |4 2 files changed, 35 insertions(+), 0 deletions(-) diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c index 08fe897..a1eaaf2 100644 --- a/drivers/base/dma-buf.c +++ b/drivers/base/dma-buf.c @@ -259,6 +259,37 @@ err_attach: EXPORT_SYMBOL_GPL(dma_buf_attach); /** + * dma_buf_get_attachment - Get attachment with the device from dma_buf's + * attachments list + * @dmabuf: [in]buffer to find device from. + * @dev: [in]device to be found. + * + * Returns struct dma_buf_attachment * attaching the device; may return + * negative error codes. + * + */ +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf, +struct device *dev) +{ + struct dma_buf_attachment *attach; + + if (WARN_ON(!dmabuf || !dev)) + return ERR_PTR(-EINVAL); + + mutex_lock(>lock); + list_for_each_entry(attach, >attachments, node) { + if (attach->dev == dev) { + mutex_unlock(>lock); + return attach; + } + } + mutex_unlock(>lock); + + return ERR_PTR(-ENODEV); +} +EXPORT_SYMBOL_GPL(dma_buf_get_attachment); >>> NAK in any form.. >>> >>> Spot the race condition between dma_buf_get_attachment and >>> dma_buf_attach >> Both dma_buf_get_attachment and dma_buf_attach has mutet with >> dmabuf->lock, and dma_buf_get_attachment is used for get attachment from >> same device before calling dma_buf_attach. > > hint: what happens if 2 threads do this: > > if (IS_ERR(attach = dma_buf_get_attachment(buf, dev))) > attach = dma_buf_attach(buf, dev); > > There really is no correct usecase for this kind of thing, so please don't do > it. Ok, dma_buf_get_attachment can not prevent attachments from one device. Anyway, do you think that importer side private data, not to allow re-import one dma-buf to a device, has some advantage? If that, I'll add some check_and_attach function, otherwise, I'll find other way. Thanks and Regards, - Seung-Woo Kim > >> While, dma_buf_detach can removes attachment because it does not have >> ref count. So importer should check ref count in its importer private >> data before calling dma_buf_detach if the importer want to use >> dma_buf_get_attachment. >> >> And dma_buf_get_attachment is for the importer, so exporter attach and >> detach callback operation should not call it as like exporter detach >> callback operation should not call dma_buf_attach if you mean this kind >> of race. >> >> If you have other considerations here, please describe more specifically. >> >> Thanks and Best Regards, >> - Seung-Woo Kim >> >>> ~Maarten >>> >>> > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Seung-Woo Kim Samsung Software R Center --
Re: [RFC][PATCH 1/2] dma-buf: add importer private data to attachment
On 2013년 06월 07일 20:24, Maarten Lankhorst wrote: Op 07-06-13 04:32, 김승우 schreef: Hello Maarten, On 2013년 06월 05일 22:23, Maarten Lankhorst wrote: Op 31-05-13 10:54, Seung-Woo Kim schreef: dma-buf attachment has only exporter private data, but importer private data can be useful for importer especially to re-import the same dma-buf. To use importer private data in attachment of the device, the function to search attachment in the attachment list of dma-buf is also added. Signed-off-by: Seung-Woo Kim sw0312@samsung.com --- drivers/base/dma-buf.c | 31 +++ include/linux/dma-buf.h |4 2 files changed, 35 insertions(+), 0 deletions(-) diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c index 08fe897..a1eaaf2 100644 --- a/drivers/base/dma-buf.c +++ b/drivers/base/dma-buf.c @@ -259,6 +259,37 @@ err_attach: EXPORT_SYMBOL_GPL(dma_buf_attach); /** + * dma_buf_get_attachment - Get attachment with the device from dma_buf's + * attachments list + * @dmabuf: [in]buffer to find device from. + * @dev: [in]device to be found. + * + * Returns struct dma_buf_attachment * attaching the device; may return + * negative error codes. + * + */ +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf, +struct device *dev) +{ + struct dma_buf_attachment *attach; + + if (WARN_ON(!dmabuf || !dev)) + return ERR_PTR(-EINVAL); + + mutex_lock(dmabuf-lock); + list_for_each_entry(attach, dmabuf-attachments, node) { + if (attach-dev == dev) { + mutex_unlock(dmabuf-lock); + return attach; + } + } + mutex_unlock(dmabuf-lock); + + return ERR_PTR(-ENODEV); +} +EXPORT_SYMBOL_GPL(dma_buf_get_attachment); NAK in any form.. Spot the race condition between dma_buf_get_attachment and dma_buf_attach Both dma_buf_get_attachment and dma_buf_attach has mutet with dmabuf-lock, and dma_buf_get_attachment is used for get attachment from same device before calling dma_buf_attach. hint: what happens if 2 threads do this: if (IS_ERR(attach = dma_buf_get_attachment(buf, dev))) attach = dma_buf_attach(buf, dev); There really is no correct usecase for this kind of thing, so please don't do it. Ok, dma_buf_get_attachment can not prevent attachments from one device. Anyway, do you think that importer side private data, not to allow re-import one dma-buf to a device, has some advantage? If that, I'll add some check_and_attach function, otherwise, I'll find other way. Thanks and Regards, - Seung-Woo Kim While, dma_buf_detach can removes attachment because it does not have ref count. So importer should check ref count in its importer private data before calling dma_buf_detach if the importer want to use dma_buf_get_attachment. And dma_buf_get_attachment is for the importer, so exporter attach and detach callback operation should not call it as like exporter detach callback operation should not call dma_buf_attach if you mean this kind of race. If you have other considerations here, please describe more specifically. Thanks and Best Regards, - Seung-Woo Kim ~Maarten ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC][PATCH 1/2] dma-buf: add importer private data to attachment
Hello Maarten, On 2013? 06? 05? 22:23, Maarten Lankhorst wrote: > Op 31-05-13 10:54, Seung-Woo Kim schreef: >> dma-buf attachment has only exporter private data, but importer private data >> can be useful for importer especially to re-import the same dma-buf. >> To use importer private data in attachment of the device, the function to >> search attachment in the attachment list of dma-buf is also added. >> >> Signed-off-by: Seung-Woo Kim >> --- >> drivers/base/dma-buf.c | 31 +++ >> include/linux/dma-buf.h |4 >> 2 files changed, 35 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c >> index 08fe897..a1eaaf2 100644 >> --- a/drivers/base/dma-buf.c >> +++ b/drivers/base/dma-buf.c >> @@ -259,6 +259,37 @@ err_attach: >> EXPORT_SYMBOL_GPL(dma_buf_attach); >> >> /** >> + * dma_buf_get_attachment - Get attachment with the device from dma_buf's >> + * attachments list >> + * @dmabuf: [in]buffer to find device from. >> + * @dev:[in]device to be found. >> + * >> + * Returns struct dma_buf_attachment * attaching the device; may return >> + * negative error codes. >> + * >> + */ >> +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf, >> + struct device *dev) >> +{ >> +struct dma_buf_attachment *attach; >> + >> +if (WARN_ON(!dmabuf || !dev)) >> +return ERR_PTR(-EINVAL); >> + >> +mutex_lock(>lock); >> +list_for_each_entry(attach, >attachments, node) { >> +if (attach->dev == dev) { >> +mutex_unlock(>lock); >> +return attach; >> +} >> +} >> +mutex_unlock(>lock); >> + >> +return ERR_PTR(-ENODEV); >> +} >> +EXPORT_SYMBOL_GPL(dma_buf_get_attachment); > NAK in any form.. > > Spot the race condition between dma_buf_get_attachment and dma_buf_attach Both dma_buf_get_attachment and dma_buf_attach has mutet with dmabuf->lock, and dma_buf_get_attachment is used for get attachment from same device before calling dma_buf_attach. While, dma_buf_detach can removes attachment because it does not have ref count. So importer should check ref count in its importer private data before calling dma_buf_detach if the importer want to use dma_buf_get_attachment. And dma_buf_get_attachment is for the importer, so exporter attach and detach callback operation should not call it as like exporter detach callback operation should not call dma_buf_attach if you mean this kind of race. If you have other considerations here, please describe more specifically. Thanks and Best Regards, - Seung-Woo Kim > > ~Maarten > > -- Seung-Woo Kim Samsung Software R Center --
Re: [RFC][PATCH 1/2] dma-buf: add importer private data to attachment
Hello Maarten, On 2013년 06월 05일 22:23, Maarten Lankhorst wrote: Op 31-05-13 10:54, Seung-Woo Kim schreef: dma-buf attachment has only exporter private data, but importer private data can be useful for importer especially to re-import the same dma-buf. To use importer private data in attachment of the device, the function to search attachment in the attachment list of dma-buf is also added. Signed-off-by: Seung-Woo Kim sw0312@samsung.com --- drivers/base/dma-buf.c | 31 +++ include/linux/dma-buf.h |4 2 files changed, 35 insertions(+), 0 deletions(-) diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c index 08fe897..a1eaaf2 100644 --- a/drivers/base/dma-buf.c +++ b/drivers/base/dma-buf.c @@ -259,6 +259,37 @@ err_attach: EXPORT_SYMBOL_GPL(dma_buf_attach); /** + * dma_buf_get_attachment - Get attachment with the device from dma_buf's + * attachments list + * @dmabuf: [in]buffer to find device from. + * @dev:[in]device to be found. + * + * Returns struct dma_buf_attachment * attaching the device; may return + * negative error codes. + * + */ +struct dma_buf_attachment *dma_buf_get_attachment(struct dma_buf *dmabuf, + struct device *dev) +{ +struct dma_buf_attachment *attach; + +if (WARN_ON(!dmabuf || !dev)) +return ERR_PTR(-EINVAL); + +mutex_lock(dmabuf-lock); +list_for_each_entry(attach, dmabuf-attachments, node) { +if (attach-dev == dev) { +mutex_unlock(dmabuf-lock); +return attach; +} +} +mutex_unlock(dmabuf-lock); + +return ERR_PTR(-ENODEV); +} +EXPORT_SYMBOL_GPL(dma_buf_get_attachment); NAK in any form.. Spot the race condition between dma_buf_get_attachment and dma_buf_attach Both dma_buf_get_attachment and dma_buf_attach has mutet with dmabuf-lock, and dma_buf_get_attachment is used for get attachment from same device before calling dma_buf_attach. While, dma_buf_detach can removes attachment because it does not have ref count. So importer should check ref count in its importer private data before calling dma_buf_detach if the importer want to use dma_buf_get_attachment. And dma_buf_get_attachment is for the importer, so exporter attach and detach callback operation should not call it as like exporter detach callback operation should not call dma_buf_attach if you mean this kind of race. If you have other considerations here, please describe more specifically. Thanks and Best Regards, - Seung-Woo Kim ~Maarten -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC][PATCH 0/2] dma-buf: add importer private data for reimporting
On 2013? 06? 04? 21:55, Daniel Vetter wrote: > On Tue, Jun 04, 2013 at 07:42:22PM +0900, ??? wrote: >> >> >> On 2013? 06? 01? 00:29, Daniel Vetter wrote: >>> On Fri, May 31, 2013 at 07:22:24PM +0900, ??? wrote: Hello Daniel, Thanks for your comment. On 2013? 05? 31? 18:14, Daniel Vetter wrote: > On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim samsung.com> wrote: >> importer private data in dma-buf attachment can be used by importer to >> reimport same dma-buf. >> >> Seung-Woo Kim (2): >> dma-buf: add importer private data to attachment >> drm/prime: find gem object from the reimported dma-buf > > Self-import should already work (at least with the latest refcount > fixes merged). At least the tests to check both re-import on the same > drm fd and on a different all work as expected now. Currently, prime works well for all case including self-importing, importing, and reimporting as you describe. Just, importing dma-buf from other driver twice with different drm_fd, each import create its own gem object even two import is done for same buffer because prime_priv is in struct drm_file. This means mapping to the device is done also twice. IMHO, these duplicated creations and maps are not necessary if drm can find previous import in different prime_priv. >>> >>> Well, that's imo a bug with the other driver. If it doesn't export >>> something really simple (e.g. contiguous memory which doesn't require any >>> mmio resources at all) it should have a cache of exported dma_buf fds so >>> that it hands out the same dma_buf every time. >> >> Hm, all existing dma-buf exporter including i915 driver implements its >> map_dma_buf callback as allocating scatter-gather table with pages in >> its buffer and calling dma_map_sg() with the sgt. With different >> drm_fds, importing one dma-buf *twice*, then importer calls >> dma_buf_attach() and dma_buf_map_attachment() twice at least in drm >> importer because re-importing case can only checked with prime_priv in >> drm_file as I described. > > Well, but thanks to all the self-import and re-import checks, it's > _impossible_ to import the same dma_buf twice without noticing (presuming > both importer and exporter are drm devices). No, it is possible. Prime function, drm_gem_prime_fd_to_handle(), checks re-import with following code. ret = drm_prime_lookup_buf_handle(_priv->prime, dma_buf, handle); Unfortunately, file_priv is allocated per each open of drm node so this code can only find re-import within same drm open context. And driver specific import functions, like drm_gem_prime_import(), only check self-import like following code. if (dma_buf->ops == _gem_prime_dmabuf_ops) { obj = dma_buf->priv; if (obj->dev == dev) { /* ... */ } } This means some application like following can make re-import to different gem objects. int drm_fd1, drm_fd2, ret; int dma_buf_fd; struct drm_prime_handle prime1, prime2; drm_fd1 = open(DRM_NODE, O_RDWR, 0); drm_fd2 = open(DRM_NODE, O_RDWR, 0); /* get some dma-buf_fd from other dma-buf exporter */ prime1.fd = dma_buf_fd; prime2.fd = dma_buf_fd; ret = ioctl(drm_fd1, DRM_IOCTL_PRIME_FD_TO_HANDLE, ); ret = ioctl(drm_fd2, DRM_IOCTL_PRIME_FD_TO_HANDLE, ); This will import same dma-buf twice as different GEM object because above checking codes can not check already imported gem object from the dma-buf. >> >>> >>> Or it needs to be more clever in it's dma_buf_attachment_map functions and >>> lookup up a pre-existing iommu mapping. >>> >>> But dealing with this in the importer is just broken. >>> > Second, the dma_buf_attachment is _definitely_ the wrong place to do > this. If you need iommu mapping caching, that should happen at a lower > level (i.e. in the map_attachment callback somewhere of the exporter, > that's what the priv field in the attachment is for). Snatching away > the attachement from some random other import is certainly not the way > to go - attachements are _not_ refcounted! Yes, attachments do not have refcount, so importer should handle and drm case in my patch, importer private data is gem object and it has, of course, refcount. And at current, exporter can not classify map_dma_buf requests of same importer to same buffer with different attachment because dma_buf_attach always makes new attachments. To resolve this exporter should search all different attachment from same importer of dma-buf and it seems more complex than importer private data to me. If I misunderstood something, please let me know. >>> >>> Like I've said above, just fix this in the exporter. If an importer sees >>> two different dma_bufs it can very well presume that it those two indeed >>> point to different backing storage. >> >> Yes, my patch does not break this concept. I just fixed case
[RFC][PATCH 0/2] dma-buf: add importer private data for reimporting
On 2013? 06? 01? 00:29, Daniel Vetter wrote: > On Fri, May 31, 2013 at 07:22:24PM +0900, ??? wrote: >> Hello Daniel, >> >> Thanks for your comment. >> >> On 2013? 05? 31? 18:14, Daniel Vetter wrote: >>> On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim >>> wrote: importer private data in dma-buf attachment can be used by importer to reimport same dma-buf. Seung-Woo Kim (2): dma-buf: add importer private data to attachment drm/prime: find gem object from the reimported dma-buf >>> >>> Self-import should already work (at least with the latest refcount >>> fixes merged). At least the tests to check both re-import on the same >>> drm fd and on a different all work as expected now. >> >> Currently, prime works well for all case including self-importing, >> importing, and reimporting as you describe. Just, importing dma-buf from >> other driver twice with different drm_fd, each import create its own gem >> object even two import is done for same buffer because prime_priv is in >> struct drm_file. This means mapping to the device is done also twice. >> IMHO, these duplicated creations and maps are not necessary if drm can >> find previous import in different prime_priv. > > Well, that's imo a bug with the other driver. If it doesn't export > something really simple (e.g. contiguous memory which doesn't require any > mmio resources at all) it should have a cache of exported dma_buf fds so > that it hands out the same dma_buf every time. Hm, all existing dma-buf exporter including i915 driver implements its map_dma_buf callback as allocating scatter-gather table with pages in its buffer and calling dma_map_sg() with the sgt. With different drm_fds, importing one dma-buf *twice*, then importer calls dma_buf_attach() and dma_buf_map_attachment() twice at least in drm importer because re-importing case can only checked with prime_priv in drm_file as I described. > > Or it needs to be more clever in it's dma_buf_attachment_map functions and > lookup up a pre-existing iommu mapping. > > But dealing with this in the importer is just broken. > >>> Second, the dma_buf_attachment is _definitely_ the wrong place to do >>> this. If you need iommu mapping caching, that should happen at a lower >>> level (i.e. in the map_attachment callback somewhere of the exporter, >>> that's what the priv field in the attachment is for). Snatching away >>> the attachement from some random other import is certainly not the way >>> to go - attachements are _not_ refcounted! >> >> Yes, attachments do not have refcount, so importer should handle and drm >> case in my patch, importer private data is gem object and it has, of >> course, refcount. >> >> And at current, exporter can not classify map_dma_buf requests of same >> importer to same buffer with different attachment because dma_buf_attach >> always makes new attachments. To resolve this exporter should search all >> different attachment from same importer of dma-buf and it seems more >> complex than importer private data to me. >> >> If I misunderstood something, please let me know. > > Like I've said above, just fix this in the exporter. If an importer sees > two different dma_bufs it can very well presume that it those two indeed > point to different backing storage. Yes, my patch does not break this concept. I just fixed case importing _one_ dma-buf twice with different drm_fds. > > This will be even more important if we attach fences two dma_bufs. If your > broken exporter creates multiple dma_bufs each one of them will have their > own fences attached, leading to a complete disasters. Ok, strictly > speaking if you keep the same reservation pointer for each dma_buf it'll > work, but that's just a detail of how you solve this in the exporter. I can not understand about broken exporter you addressed. I don't mean exporter makes dma-bufs from one backing storage. While, my patch prevents not to create drm gem objects from one back storage by importing one dma-buf with different drm-fds. I do not believe the fix of importer is the best way, but at this moment, I have no idea how I can fix the exporter for this issue. Best Regards, - Seung-Woo Kim > > Cheers, Daniel > -- Seung-Woo Kim Samsung Software R Center --
Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting
On 2013년 06월 01일 00:29, Daniel Vetter wrote: On Fri, May 31, 2013 at 07:22:24PM +0900, 김승우 wrote: Hello Daniel, Thanks for your comment. On 2013년 05월 31일 18:14, Daniel Vetter wrote: On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim sw0312@samsung.com wrote: importer private data in dma-buf attachment can be used by importer to reimport same dma-buf. Seung-Woo Kim (2): dma-buf: add importer private data to attachment drm/prime: find gem object from the reimported dma-buf Self-import should already work (at least with the latest refcount fixes merged). At least the tests to check both re-import on the same drm fd and on a different all work as expected now. Currently, prime works well for all case including self-importing, importing, and reimporting as you describe. Just, importing dma-buf from other driver twice with different drm_fd, each import create its own gem object even two import is done for same buffer because prime_priv is in struct drm_file. This means mapping to the device is done also twice. IMHO, these duplicated creations and maps are not necessary if drm can find previous import in different prime_priv. Well, that's imo a bug with the other driver. If it doesn't export something really simple (e.g. contiguous memory which doesn't require any mmio resources at all) it should have a cache of exported dma_buf fds so that it hands out the same dma_buf every time. Hm, all existing dma-buf exporter including i915 driver implements its map_dma_buf callback as allocating scatter-gather table with pages in its buffer and calling dma_map_sg() with the sgt. With different drm_fds, importing one dma-buf *twice*, then importer calls dma_buf_attach() and dma_buf_map_attachment() twice at least in drm importer because re-importing case can only checked with prime_priv in drm_file as I described. Or it needs to be more clever in it's dma_buf_attachment_map functions and lookup up a pre-existing iommu mapping. But dealing with this in the importer is just broken. Second, the dma_buf_attachment is _definitely_ the wrong place to do this. If you need iommu mapping caching, that should happen at a lower level (i.e. in the map_attachment callback somewhere of the exporter, that's what the priv field in the attachment is for). Snatching away the attachement from some random other import is certainly not the way to go - attachements are _not_ refcounted! Yes, attachments do not have refcount, so importer should handle and drm case in my patch, importer private data is gem object and it has, of course, refcount. And at current, exporter can not classify map_dma_buf requests of same importer to same buffer with different attachment because dma_buf_attach always makes new attachments. To resolve this exporter should search all different attachment from same importer of dma-buf and it seems more complex than importer private data to me. If I misunderstood something, please let me know. Like I've said above, just fix this in the exporter. If an importer sees two different dma_bufs it can very well presume that it those two indeed point to different backing storage. Yes, my patch does not break this concept. I just fixed case importing _one_ dma-buf twice with different drm_fds. This will be even more important if we attach fences two dma_bufs. If your broken exporter creates multiple dma_bufs each one of them will have their own fences attached, leading to a complete disasters. Ok, strictly speaking if you keep the same reservation pointer for each dma_buf it'll work, but that's just a detail of how you solve this in the exporter. I can not understand about broken exporter you addressed. I don't mean exporter makes dma-bufs from one backing storage. While, my patch prevents not to create drm gem objects from one back storage by importing one dma-buf with different drm-fds. I do not believe the fix of importer is the best way, but at this moment, I have no idea how I can fix the exporter for this issue. Best Regards, - Seung-Woo Kim Cheers, Daniel -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting
On 2013년 06월 04일 21:55, Daniel Vetter wrote: On Tue, Jun 04, 2013 at 07:42:22PM +0900, 김승우 wrote: On 2013년 06월 01일 00:29, Daniel Vetter wrote: On Fri, May 31, 2013 at 07:22:24PM +0900, 김승우 wrote: Hello Daniel, Thanks for your comment. On 2013년 05월 31일 18:14, Daniel Vetter wrote: On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim sw0312@samsung.com wrote: importer private data in dma-buf attachment can be used by importer to reimport same dma-buf. Seung-Woo Kim (2): dma-buf: add importer private data to attachment drm/prime: find gem object from the reimported dma-buf Self-import should already work (at least with the latest refcount fixes merged). At least the tests to check both re-import on the same drm fd and on a different all work as expected now. Currently, prime works well for all case including self-importing, importing, and reimporting as you describe. Just, importing dma-buf from other driver twice with different drm_fd, each import create its own gem object even two import is done for same buffer because prime_priv is in struct drm_file. This means mapping to the device is done also twice. IMHO, these duplicated creations and maps are not necessary if drm can find previous import in different prime_priv. Well, that's imo a bug with the other driver. If it doesn't export something really simple (e.g. contiguous memory which doesn't require any mmio resources at all) it should have a cache of exported dma_buf fds so that it hands out the same dma_buf every time. Hm, all existing dma-buf exporter including i915 driver implements its map_dma_buf callback as allocating scatter-gather table with pages in its buffer and calling dma_map_sg() with the sgt. With different drm_fds, importing one dma-buf *twice*, then importer calls dma_buf_attach() and dma_buf_map_attachment() twice at least in drm importer because re-importing case can only checked with prime_priv in drm_file as I described. Well, but thanks to all the self-import and re-import checks, it's _impossible_ to import the same dma_buf twice without noticing (presuming both importer and exporter are drm devices). No, it is possible. Prime function, drm_gem_prime_fd_to_handle(), checks re-import with following code. ret = drm_prime_lookup_buf_handle(file_priv-prime, dma_buf, handle); Unfortunately, file_priv is allocated per each open of drm node so this code can only find re-import within same drm open context. And driver specific import functions, like drm_gem_prime_import(), only check self-import like following code. if (dma_buf-ops == drm_gem_prime_dmabuf_ops) { obj = dma_buf-priv; if (obj-dev == dev) { /* ... */ } } This means some application like following can make re-import to different gem objects. int drm_fd1, drm_fd2, ret; int dma_buf_fd; struct drm_prime_handle prime1, prime2; drm_fd1 = open(DRM_NODE, O_RDWR, 0); drm_fd2 = open(DRM_NODE, O_RDWR, 0); /* get some dma-buf_fd from other dma-buf exporter */ prime1.fd = dma_buf_fd; prime2.fd = dma_buf_fd; ret = ioctl(drm_fd1, DRM_IOCTL_PRIME_FD_TO_HANDLE, prime1); ret = ioctl(drm_fd2, DRM_IOCTL_PRIME_FD_TO_HANDLE, prime2); This will import same dma-buf twice as different GEM object because above checking codes can not check already imported gem object from the dma-buf. Or it needs to be more clever in it's dma_buf_attachment_map functions and lookup up a pre-existing iommu mapping. But dealing with this in the importer is just broken. Second, the dma_buf_attachment is _definitely_ the wrong place to do this. If you need iommu mapping caching, that should happen at a lower level (i.e. in the map_attachment callback somewhere of the exporter, that's what the priv field in the attachment is for). Snatching away the attachement from some random other import is certainly not the way to go - attachements are _not_ refcounted! Yes, attachments do not have refcount, so importer should handle and drm case in my patch, importer private data is gem object and it has, of course, refcount. And at current, exporter can not classify map_dma_buf requests of same importer to same buffer with different attachment because dma_buf_attach always makes new attachments. To resolve this exporter should search all different attachment from same importer of dma-buf and it seems more complex than importer private data to me. If I misunderstood something, please let me know. Like I've said above, just fix this in the exporter. If an importer sees two different dma_bufs it can very well presume that it those two indeed point to different backing storage. Yes, my patch does not break this concept. I just fixed case importing _one_ dma-buf twice with different drm_fds. See above, if you have two different struct file * for the same underlying buffer object something is wrong already. drm_fds, I described, are not dma-buf fd but fds from opening DRM_NODE
[RFC][PATCH 0/2] dma-buf: add importer private data for reimporting
Hello Daniel, Thanks for your comment. On 2013? 05? 31? 18:14, Daniel Vetter wrote: > On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim > wrote: >> importer private data in dma-buf attachment can be used by importer to >> reimport same dma-buf. >> >> Seung-Woo Kim (2): >> dma-buf: add importer private data to attachment >> drm/prime: find gem object from the reimported dma-buf > > Self-import should already work (at least with the latest refcount > fixes merged). At least the tests to check both re-import on the same > drm fd and on a different all work as expected now. Currently, prime works well for all case including self-importing, importing, and reimporting as you describe. Just, importing dma-buf from other driver twice with different drm_fd, each import create its own gem object even two import is done for same buffer because prime_priv is in struct drm_file. This means mapping to the device is done also twice. IMHO, these duplicated creations and maps are not necessary if drm can find previous import in different prime_priv. > > Second, the dma_buf_attachment is _definitely_ the wrong place to do > this. If you need iommu mapping caching, that should happen at a lower > level (i.e. in the map_attachment callback somewhere of the exporter, > that's what the priv field in the attachment is for). Snatching away > the attachement from some random other import is certainly not the way > to go - attachements are _not_ refcounted! Yes, attachments do not have refcount, so importer should handle and drm case in my patch, importer private data is gem object and it has, of course, refcount. And at current, exporter can not classify map_dma_buf requests of same importer to same buffer with different attachment because dma_buf_attach always makes new attachments. To resolve this exporter should search all different attachment from same importer of dma-buf and it seems more complex than importer private data to me. If I misunderstood something, please let me know. Best Regards, - Seung-Woo Kim > -Daniel > >> >> drivers/base/dma-buf.c | 31 >> >> drivers/gpu/drm/drm_prime.c| 19 >> drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |1 + >> drivers/gpu/drm/i915/i915_gem_dmabuf.c |1 + >> drivers/gpu/drm/udl/udl_gem.c |1 + >> include/linux/dma-buf.h|4 +++ >> 6 files changed, 52 insertions(+), 5 deletions(-) >> >> -- >> 1.7.4.1 >> > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > -- Seung-Woo Kim Samsung Software R Center --
Re: [RFC][PATCH 0/2] dma-buf: add importer private data for reimporting
Hello Daniel, Thanks for your comment. On 2013년 05월 31일 18:14, Daniel Vetter wrote: On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim sw0312@samsung.com wrote: importer private data in dma-buf attachment can be used by importer to reimport same dma-buf. Seung-Woo Kim (2): dma-buf: add importer private data to attachment drm/prime: find gem object from the reimported dma-buf Self-import should already work (at least with the latest refcount fixes merged). At least the tests to check both re-import on the same drm fd and on a different all work as expected now. Currently, prime works well for all case including self-importing, importing, and reimporting as you describe. Just, importing dma-buf from other driver twice with different drm_fd, each import create its own gem object even two import is done for same buffer because prime_priv is in struct drm_file. This means mapping to the device is done also twice. IMHO, these duplicated creations and maps are not necessary if drm can find previous import in different prime_priv. Second, the dma_buf_attachment is _definitely_ the wrong place to do this. If you need iommu mapping caching, that should happen at a lower level (i.e. in the map_attachment callback somewhere of the exporter, that's what the priv field in the attachment is for). Snatching away the attachement from some random other import is certainly not the way to go - attachements are _not_ refcounted! Yes, attachments do not have refcount, so importer should handle and drm case in my patch, importer private data is gem object and it has, of course, refcount. And at current, exporter can not classify map_dma_buf requests of same importer to same buffer with different attachment because dma_buf_attach always makes new attachments. To resolve this exporter should search all different attachment from same importer of dma-buf and it seems more complex than importer private data to me. If I misunderstood something, please let me know. Best Regards, - Seung-Woo Kim -Daniel drivers/base/dma-buf.c | 31 drivers/gpu/drm/drm_prime.c| 19 drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |1 + drivers/gpu/drm/i915/i915_gem_dmabuf.c |1 + drivers/gpu/drm/udl/udl_gem.c |1 + include/linux/dma-buf.h|4 +++ 6 files changed, 52 insertions(+), 5 deletions(-) -- 1.7.4.1 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/exynos: exynos_hdmi: Pass correct pointer to free_irq()
Good point, On 2013? 05? 21? 02:32, Lars-Peter Clausen wrote: > free_irq() expects the same pointer that was passed to request_threaded_irq(), > otherwise the IRQ is not freed. > > The issue was found using the following coccinelle script: > > > @r1@ > type T; > T devid; > @@ > request_threaded_irq(..., devid) > > @r2@ > type r1.T; > T devid; > position p; > @@ > free_irq at p(..., devid) > > @@ > position p != r2.p; > @@ > *free_irq at p(...) > > > Signed-off-by: Lars-Peter Clausen Acked-by: Seung-Woo Kim > --- > drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c > b/drivers/gpu/drm/exynos/exynos_hdmi.c > index bbfc384..7e99853 100644 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c > @@ -2082,7 +2082,7 @@ static int hdmi_remove(struct platform_device *pdev) > > pm_runtime_disable(dev); > > - free_irq(hdata->irq, hdata); > + free_irq(hdata->irq, ctx); > > > /* hdmiphy i2c driver */ > -- Seung-Woo Kim Samsung Software R Center --
Re: [PATCH] drm/exynos: exynos_hdmi: Pass correct pointer to free_irq()
Good point, On 2013년 05월 21일 02:32, Lars-Peter Clausen wrote: free_irq() expects the same pointer that was passed to request_threaded_irq(), otherwise the IRQ is not freed. The issue was found using the following coccinelle script: smpl @r1@ type T; T devid; @@ request_threaded_irq(..., devid) @r2@ type r1.T; T devid; position p; @@ free_irq@p(..., devid) @@ position p != r2.p; @@ *free_irq@p(...) /smpl Signed-off-by: Lars-Peter Clausen l...@metafoo.de Acked-by: Seung-Woo Kim sw0312@samsung.com --- drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index bbfc384..7e99853 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -2082,7 +2082,7 @@ static int hdmi_remove(struct platform_device *pdev) pm_runtime_disable(dev); - free_irq(hdata-irq, hdata); + free_irq(hdata-irq, ctx); /* hdmiphy i2c driver */ -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4] drm/exynos: hdmi: use drm_display_mode to check the supported modes
Hello Rahul, I looks good to me. On 2013? 04? 29? 20:14, Rahul Sharma wrote: > Exynos hdmi driver is using drm_display_mode for setting timing values > for a supported resolution. Conversion to fb_videomode and then comparing > with the mixer/hdmi/phy limits is not required. Instead, drm_display_mode > fields cane be directly compared. > > v4: > 1) Removed __func__ from DRM_DEBUG_KMS. > > v3: > 1) Replaced check_timing callbacks with check_mode. > 2) Change the type of second parameter of check_mode callback from void > pointer paramenter to struct drm_display_mode pointer. > > v2: > 1) Removed convert_to_video_timing(). > 2) Corrected DRM_DEBUG_KMS to print the resolution properly. > > Signed-off-by: Rahul Sharma Acked-by: Seung-Woo Kim > --- > drivers/gpu/drm/exynos/exynos_drm_connector.c | 38 > ++--- > drivers/gpu/drm/exynos/exynos_drm_drv.h |4 +-- > drivers/gpu/drm/exynos/exynos_drm_fimd.c |4 +-- > drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 17 +-- > drivers/gpu/drm/exynos/exynos_drm_hdmi.h |6 ++-- > drivers/gpu/drm/exynos/exynos_drm_vidi.c |4 +-- > drivers/gpu/drm/exynos/exynos_hdmi.c | 29 +-- > drivers/gpu/drm/exynos/exynos_mixer.c | 17 +-- > 8 files changed, 43 insertions(+), 76 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c > b/drivers/gpu/drm/exynos/exynos_drm_connector.c > index 8bcc13a..ab3c6d4 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c > @@ -58,37 +58,6 @@ convert_to_display_mode(struct drm_display_mode *mode, > mode->flags |= DRM_MODE_FLAG_DBLSCAN; > } > > -/* convert drm_display_mode to exynos_video_timings */ > -static inline void > -convert_to_video_timing(struct fb_videomode *timing, > - struct drm_display_mode *mode) > -{ > - DRM_DEBUG_KMS("%s\n", __FILE__); > - > - memset(timing, 0, sizeof(*timing)); > - > - timing->pixclock = mode->clock * 1000; > - timing->refresh = drm_mode_vrefresh(mode); > - > - timing->xres = mode->hdisplay; > - timing->right_margin = mode->hsync_start - mode->hdisplay; > - timing->hsync_len = mode->hsync_end - mode->hsync_start; > - timing->left_margin = mode->htotal - mode->hsync_end; > - > - timing->yres = mode->vdisplay; > - timing->lower_margin = mode->vsync_start - mode->vdisplay; > - timing->vsync_len = mode->vsync_end - mode->vsync_start; > - timing->upper_margin = mode->vtotal - mode->vsync_end; > - > - if (mode->flags & DRM_MODE_FLAG_INTERLACE) > - timing->vmode = FB_VMODE_INTERLACED; > - else > - timing->vmode = FB_VMODE_NONINTERLACED; > - > - if (mode->flags & DRM_MODE_FLAG_DBLSCAN) > - timing->vmode |= FB_VMODE_DOUBLE; > -} > - > static int exynos_drm_connector_get_modes(struct drm_connector *connector) > { > struct exynos_drm_connector *exynos_connector = > @@ -168,15 +137,12 @@ static int exynos_drm_connector_mode_valid(struct > drm_connector *connector, > to_exynos_connector(connector); > struct exynos_drm_manager *manager = exynos_connector->manager; > struct exynos_drm_display_ops *display_ops = manager->display_ops; > - struct fb_videomode timing; > int ret = MODE_BAD; > > DRM_DEBUG_KMS("%s\n", __FILE__); > > - convert_to_video_timing(, mode); > - > - if (display_ops && display_ops->check_timing) > - if (!display_ops->check_timing(manager->dev, (void *))) > + if (display_ops && display_ops->check_mode) > + if (!display_ops->check_mode(manager->dev, mode)) > ret = MODE_OK; > > return ret; > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h > b/drivers/gpu/drm/exynos/exynos_drm_drv.h > index 4606fac..9650b3b 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h > @@ -142,7 +142,7 @@ struct exynos_drm_overlay { > * @is_connected: check for that display is connected or not. > * @get_edid: get edid modes from display driver. > * @get_panel: get panel object from display driver. > - * @check_timing: check if timing is valid or not. > + * @check_mode: check if mode is valid or not. > * @power_on: display device on or off. > */ > struct exynos_drm_display_ops { > @@ -151,7 +151,7 @@ struct exynos_drm_display_ops { > struct edid *(*get_edid)(struct device *dev, > struct drm_connector *connector); > void *(*get_panel)(struct device *dev); > - int (*check_timing)(struct device *dev, void *timing); > + int (*check_mode)(struct device *dev, struct drm_display_mode *mode); > int (*power_on)(struct device *dev, int mode); > }; > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c > b/drivers/gpu/drm/exynos/exynos_drm_fimd.c > index
[PATCH v2] drm/exynos: hdmi: use drm_display_mode to check the supported modes
Hello Rahul, I agree with basic idea of this patch. However, to avoid confusion, how do you think about fixing all check_timing as check_mode and its second parameter as mode including struct exynos_drm_display_ops? Regards, - Seung-Woo Kim On 2013? 04? 26? 23:03, Rahul Sharma wrote: > Exynos hdmi driver is using drm_display_mode for setting timing values > for a supported resolution. Conversion to fb_videomode and then comparing > with the mixer/hdmi/phy limits is not required. Instead, drm_display_mode > fields cane be directly compared. > > v2: > 1) Removed convert_to_video_timing(). > 2) Corrected DRM_DEBUG_KMS to print the resolution properly. > > Signed-off-by: Rahul Sharma > --- > drivers/gpu/drm/exynos/exynos_drm_connector.c | 36 > + > drivers/gpu/drm/exynos/exynos_drm_hdmi.h |6 ++--- > drivers/gpu/drm/exynos/exynos_hdmi.c | 15 +-- > drivers/gpu/drm/exynos/exynos_mixer.c | 13 - > 4 files changed, 18 insertions(+), 52 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c > b/drivers/gpu/drm/exynos/exynos_drm_connector.c > index 8bcc13a..7259fff 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c > @@ -58,37 +58,6 @@ convert_to_display_mode(struct drm_display_mode *mode, > mode->flags |= DRM_MODE_FLAG_DBLSCAN; > } > > -/* convert drm_display_mode to exynos_video_timings */ > -static inline void > -convert_to_video_timing(struct fb_videomode *timing, > - struct drm_display_mode *mode) > -{ > - DRM_DEBUG_KMS("%s\n", __FILE__); > - > - memset(timing, 0, sizeof(*timing)); > - > - timing->pixclock = mode->clock * 1000; > - timing->refresh = drm_mode_vrefresh(mode); > - > - timing->xres = mode->hdisplay; > - timing->right_margin = mode->hsync_start - mode->hdisplay; > - timing->hsync_len = mode->hsync_end - mode->hsync_start; > - timing->left_margin = mode->htotal - mode->hsync_end; > - > - timing->yres = mode->vdisplay; > - timing->lower_margin = mode->vsync_start - mode->vdisplay; > - timing->vsync_len = mode->vsync_end - mode->vsync_start; > - timing->upper_margin = mode->vtotal - mode->vsync_end; > - > - if (mode->flags & DRM_MODE_FLAG_INTERLACE) > - timing->vmode = FB_VMODE_INTERLACED; > - else > - timing->vmode = FB_VMODE_NONINTERLACED; > - > - if (mode->flags & DRM_MODE_FLAG_DBLSCAN) > - timing->vmode |= FB_VMODE_DOUBLE; > -} > - > static int exynos_drm_connector_get_modes(struct drm_connector *connector) > { > struct exynos_drm_connector *exynos_connector = > @@ -168,15 +137,12 @@ static int exynos_drm_connector_mode_valid(struct > drm_connector *connector, > to_exynos_connector(connector); > struct exynos_drm_manager *manager = exynos_connector->manager; > struct exynos_drm_display_ops *display_ops = manager->display_ops; > - struct fb_videomode timing; > int ret = MODE_BAD; > > DRM_DEBUG_KMS("%s\n", __FILE__); > > - convert_to_video_timing(, mode); > - > if (display_ops && display_ops->check_timing) > - if (!display_ops->check_timing(manager->dev, (void *))) > + if (!display_ops->check_timing(manager->dev, (void *)mode)) > ret = MODE_OK; > > return ret; > diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h > b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h > index 6b70944..fd2ff9f 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h > +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h > @@ -32,11 +32,11 @@ struct exynos_hdmi_ops { > bool (*is_connected)(void *ctx); > struct edid *(*get_edid)(void *ctx, > struct drm_connector *connector); > - int (*check_timing)(void *ctx, struct fb_videomode *timing); > + int (*check_timing)(void *ctx, struct drm_display_mode *mode); > int (*power_on)(void *ctx, int mode); > > /* manager */ > - void (*mode_set)(void *ctx, void *mode); > + void (*mode_set)(void *ctx, struct drm_display_mode *mode); > void (*get_max_resol)(void *ctx, unsigned int *width, > unsigned int *height); > void (*commit)(void *ctx); > @@ -57,7 +57,7 @@ struct exynos_mixer_ops { > void (*win_disable)(void *ctx, int zpos); > > /* display */ > - int (*check_timing)(void *ctx, struct fb_videomode *timing); > + int (*check_timing)(void *ctx, struct drm_display_mode *mode); > }; > > void exynos_hdmi_drv_attach(struct exynos_drm_hdmi_context *ctx); > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c > b/drivers/gpu/drm/exynos/exynos_hdmi.c > index 93b70e9..aeca603 100644 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c > @@ -796,18 +796,17 @@ static int hdmi_find_phy_conf(struct
[RFC][PATCH] drm/prime: fixed to allocate sg table considering contiguous pages
Hello Rahul, Thanks for notifying. As your comment, it is same patch with yours, so just ignore my patch. Besg Regards, - Seung-Woo Kim On 2013? 04? 26? 18:14, Rahul Sharma wrote: > Hi Seung Woo, > > I had posted the same solution at > http://lists.freedesktop.org/archives/dri-devel/2013-January/034119.html. > This has been pulled in drm-intel-next. > > regards, > Rahul Sharma. > > > > On Fri, Apr 26, 2013 at 2:18 PM, Seung-Woo Kim samsung.com>wrote: > >> Allocating scatter table with sg_alloc_table() does not consider >> contiguous pages. Because sg_alloc_table_from_pages() merges >> contigous pages into a signle scatter entry, this patch fixes to >> allocate scatter table with it from drm_prime_pages_to_sg(). >> >> Signed-off-by: Seung-Woo Kim >> --- >> drivers/gpu/drm/drm_prime.c |8 ++-- >> 1 files changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >> index 366910d..8c098a3 100644 >> --- a/drivers/gpu/drm/drm_prime.c >> +++ b/drivers/gpu/drm/drm_prime.c >> @@ -401,21 +401,17 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device >> *dev, void *data, >> struct sg_table *drm_prime_pages_to_sg(struct page **pages, int nr_pages) >> { >> struct sg_table *sg = NULL; >> - struct scatterlist *iter; >> - int i; >> int ret; >> >> sg = kmalloc(sizeof(struct sg_table), GFP_KERNEL); >> if (!sg) >> goto out; >> >> - ret = sg_alloc_table(sg, nr_pages, GFP_KERNEL); >> + ret = sg_alloc_table_from_pages(sg, pages, nr_pages, >> + 0, PAGE_SIZE * nr_pages, GFP_KERNEL); >> if (ret) >> goto out; >> >> - for_each_sg(sg->sgl, iter, nr_pages, i) >> - sg_set_page(iter, pages[i], PAGE_SIZE, 0); >> - >> return sg; >> out: >> kfree(sg); >> -- >> 1.7.4.1 >> >> ___ >> dri-devel mailing list >> dri-devel at lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel >> > > > > > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Seung-Woo Kim Samsung Software R Center --
Re: [PATCH v4] drm/exynos: hdmi: use drm_display_mode to check the supported modes
Hello Rahul, I looks good to me. On 2013년 04월 29일 20:14, Rahul Sharma wrote: Exynos hdmi driver is using drm_display_mode for setting timing values for a supported resolution. Conversion to fb_videomode and then comparing with the mixer/hdmi/phy limits is not required. Instead, drm_display_mode fields cane be directly compared. v4: 1) Removed __func__ from DRM_DEBUG_KMS. v3: 1) Replaced check_timing callbacks with check_mode. 2) Change the type of second parameter of check_mode callback from void pointer paramenter to struct drm_display_mode pointer. v2: 1) Removed convert_to_video_timing(). 2) Corrected DRM_DEBUG_KMS to print the resolution properly. Signed-off-by: Rahul Sharma rahul.sha...@samsung.com Acked-by: Seung-Woo Kim sw0312@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_connector.c | 38 ++--- drivers/gpu/drm/exynos/exynos_drm_drv.h |4 +-- drivers/gpu/drm/exynos/exynos_drm_fimd.c |4 +-- drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 17 +-- drivers/gpu/drm/exynos/exynos_drm_hdmi.h |6 ++-- drivers/gpu/drm/exynos/exynos_drm_vidi.c |4 +-- drivers/gpu/drm/exynos/exynos_hdmi.c | 29 +-- drivers/gpu/drm/exynos/exynos_mixer.c | 17 +-- 8 files changed, 43 insertions(+), 76 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c b/drivers/gpu/drm/exynos/exynos_drm_connector.c index 8bcc13a..ab3c6d4 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c @@ -58,37 +58,6 @@ convert_to_display_mode(struct drm_display_mode *mode, mode-flags |= DRM_MODE_FLAG_DBLSCAN; } -/* convert drm_display_mode to exynos_video_timings */ -static inline void -convert_to_video_timing(struct fb_videomode *timing, - struct drm_display_mode *mode) -{ - DRM_DEBUG_KMS(%s\n, __FILE__); - - memset(timing, 0, sizeof(*timing)); - - timing-pixclock = mode-clock * 1000; - timing-refresh = drm_mode_vrefresh(mode); - - timing-xres = mode-hdisplay; - timing-right_margin = mode-hsync_start - mode-hdisplay; - timing-hsync_len = mode-hsync_end - mode-hsync_start; - timing-left_margin = mode-htotal - mode-hsync_end; - - timing-yres = mode-vdisplay; - timing-lower_margin = mode-vsync_start - mode-vdisplay; - timing-vsync_len = mode-vsync_end - mode-vsync_start; - timing-upper_margin = mode-vtotal - mode-vsync_end; - - if (mode-flags DRM_MODE_FLAG_INTERLACE) - timing-vmode = FB_VMODE_INTERLACED; - else - timing-vmode = FB_VMODE_NONINTERLACED; - - if (mode-flags DRM_MODE_FLAG_DBLSCAN) - timing-vmode |= FB_VMODE_DOUBLE; -} - static int exynos_drm_connector_get_modes(struct drm_connector *connector) { struct exynos_drm_connector *exynos_connector = @@ -168,15 +137,12 @@ static int exynos_drm_connector_mode_valid(struct drm_connector *connector, to_exynos_connector(connector); struct exynos_drm_manager *manager = exynos_connector-manager; struct exynos_drm_display_ops *display_ops = manager-display_ops; - struct fb_videomode timing; int ret = MODE_BAD; DRM_DEBUG_KMS(%s\n, __FILE__); - convert_to_video_timing(timing, mode); - - if (display_ops display_ops-check_timing) - if (!display_ops-check_timing(manager-dev, (void *)timing)) + if (display_ops display_ops-check_mode) + if (!display_ops-check_mode(manager-dev, mode)) ret = MODE_OK; return ret; diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 4606fac..9650b3b 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -142,7 +142,7 @@ struct exynos_drm_overlay { * @is_connected: check for that display is connected or not. * @get_edid: get edid modes from display driver. * @get_panel: get panel object from display driver. - * @check_timing: check if timing is valid or not. + * @check_mode: check if mode is valid or not. * @power_on: display device on or off. */ struct exynos_drm_display_ops { @@ -151,7 +151,7 @@ struct exynos_drm_display_ops { struct edid *(*get_edid)(struct device *dev, struct drm_connector *connector); void *(*get_panel)(struct device *dev); - int (*check_timing)(struct device *dev, void *timing); + int (*check_mode)(struct device *dev, struct drm_display_mode *mode); int (*power_on)(struct device *dev, int mode); }; diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 15e58f5..98bbe1e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c +++
Re: [RFC][PATCH] drm/prime: fixed to allocate sg table considering contiguous pages
Hello Rahul, Thanks for notifying. As your comment, it is same patch with yours, so just ignore my patch. Besg Regards, - Seung-Woo Kim On 2013년 04월 26일 18:14, Rahul Sharma wrote: Hi Seung Woo, I had posted the same solution at http://lists.freedesktop.org/archives/dri-devel/2013-January/034119.html. This has been pulled in drm-intel-next. regards, Rahul Sharma. On Fri, Apr 26, 2013 at 2:18 PM, Seung-Woo Kim sw0312@samsung.comwrote: Allocating scatter table with sg_alloc_table() does not consider contiguous pages. Because sg_alloc_table_from_pages() merges contigous pages into a signle scatter entry, this patch fixes to allocate scatter table with it from drm_prime_pages_to_sg(). Signed-off-by: Seung-Woo Kim sw0312@samsung.com --- drivers/gpu/drm/drm_prime.c |8 ++-- 1 files changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 366910d..8c098a3 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -401,21 +401,17 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, struct sg_table *drm_prime_pages_to_sg(struct page **pages, int nr_pages) { struct sg_table *sg = NULL; - struct scatterlist *iter; - int i; int ret; sg = kmalloc(sizeof(struct sg_table), GFP_KERNEL); if (!sg) goto out; - ret = sg_alloc_table(sg, nr_pages, GFP_KERNEL); + ret = sg_alloc_table_from_pages(sg, pages, nr_pages, + 0, PAGE_SIZE * nr_pages, GFP_KERNEL); if (ret) goto out; - for_each_sg(sg-sgl, iter, nr_pages, i) - sg_set_page(iter, pages[i], PAGE_SIZE, 0); - return sg; out: kfree(sg); -- 1.7.4.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/exynos: hdmi: use drm_display_mode to check the supported modes
Hello Rahul, I agree with basic idea of this patch. However, to avoid confusion, how do you think about fixing all check_timing as check_mode and its second parameter as mode including struct exynos_drm_display_ops? Regards, - Seung-Woo Kim On 2013년 04월 26일 23:03, Rahul Sharma wrote: Exynos hdmi driver is using drm_display_mode for setting timing values for a supported resolution. Conversion to fb_videomode and then comparing with the mixer/hdmi/phy limits is not required. Instead, drm_display_mode fields cane be directly compared. v2: 1) Removed convert_to_video_timing(). 2) Corrected DRM_DEBUG_KMS to print the resolution properly. Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_connector.c | 36 + drivers/gpu/drm/exynos/exynos_drm_hdmi.h |6 ++--- drivers/gpu/drm/exynos/exynos_hdmi.c | 15 +-- drivers/gpu/drm/exynos/exynos_mixer.c | 13 - 4 files changed, 18 insertions(+), 52 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c b/drivers/gpu/drm/exynos/exynos_drm_connector.c index 8bcc13a..7259fff 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c @@ -58,37 +58,6 @@ convert_to_display_mode(struct drm_display_mode *mode, mode-flags |= DRM_MODE_FLAG_DBLSCAN; } -/* convert drm_display_mode to exynos_video_timings */ -static inline void -convert_to_video_timing(struct fb_videomode *timing, - struct drm_display_mode *mode) -{ - DRM_DEBUG_KMS(%s\n, __FILE__); - - memset(timing, 0, sizeof(*timing)); - - timing-pixclock = mode-clock * 1000; - timing-refresh = drm_mode_vrefresh(mode); - - timing-xres = mode-hdisplay; - timing-right_margin = mode-hsync_start - mode-hdisplay; - timing-hsync_len = mode-hsync_end - mode-hsync_start; - timing-left_margin = mode-htotal - mode-hsync_end; - - timing-yres = mode-vdisplay; - timing-lower_margin = mode-vsync_start - mode-vdisplay; - timing-vsync_len = mode-vsync_end - mode-vsync_start; - timing-upper_margin = mode-vtotal - mode-vsync_end; - - if (mode-flags DRM_MODE_FLAG_INTERLACE) - timing-vmode = FB_VMODE_INTERLACED; - else - timing-vmode = FB_VMODE_NONINTERLACED; - - if (mode-flags DRM_MODE_FLAG_DBLSCAN) - timing-vmode |= FB_VMODE_DOUBLE; -} - static int exynos_drm_connector_get_modes(struct drm_connector *connector) { struct exynos_drm_connector *exynos_connector = @@ -168,15 +137,12 @@ static int exynos_drm_connector_mode_valid(struct drm_connector *connector, to_exynos_connector(connector); struct exynos_drm_manager *manager = exynos_connector-manager; struct exynos_drm_display_ops *display_ops = manager-display_ops; - struct fb_videomode timing; int ret = MODE_BAD; DRM_DEBUG_KMS(%s\n, __FILE__); - convert_to_video_timing(timing, mode); - if (display_ops display_ops-check_timing) - if (!display_ops-check_timing(manager-dev, (void *)timing)) + if (!display_ops-check_timing(manager-dev, (void *)mode)) ret = MODE_OK; return ret; diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h index 6b70944..fd2ff9f 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h @@ -32,11 +32,11 @@ struct exynos_hdmi_ops { bool (*is_connected)(void *ctx); struct edid *(*get_edid)(void *ctx, struct drm_connector *connector); - int (*check_timing)(void *ctx, struct fb_videomode *timing); + int (*check_timing)(void *ctx, struct drm_display_mode *mode); int (*power_on)(void *ctx, int mode); /* manager */ - void (*mode_set)(void *ctx, void *mode); + void (*mode_set)(void *ctx, struct drm_display_mode *mode); void (*get_max_resol)(void *ctx, unsigned int *width, unsigned int *height); void (*commit)(void *ctx); @@ -57,7 +57,7 @@ struct exynos_mixer_ops { void (*win_disable)(void *ctx, int zpos); /* display */ - int (*check_timing)(void *ctx, struct fb_videomode *timing); + int (*check_timing)(void *ctx, struct drm_display_mode *mode); }; void exynos_hdmi_drv_attach(struct exynos_drm_hdmi_context *ctx); diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 93b70e9..aeca603 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -796,18 +796,17 @@ static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock) return -EINVAL; } -static int hdmi_check_timing(void *ctx, struct fb_videomode
[PATCH] drivers: gpu: drm: exynos: Replaced kzalloc & memcpy with kmemdup
Good point. Acked-by: Seung-Woo Kim On 2013? 03? 12? 04:25, Alexandru Gheorghiu wrote: > Replaced calls to kzalloc followed by memcpy with call to kmemdup. > Patch found using coccinelle. > > Signed-off-by: Alexandru Gheorghiu > --- > drivers/gpu/drm/exynos/exynos_drm_vidi.c |6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c > b/drivers/gpu/drm/exynos/exynos_drm_vidi.c > index 13ccbd4..9504b0c 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c > @@ -117,13 +117,12 @@ static struct edid *vidi_get_edid(struct device *dev, > } > > edid_len = (1 + ctx->raw_edid->extensions) * EDID_LENGTH; > - edid = kzalloc(edid_len, GFP_KERNEL); > + edid = kmemdup(ctx->raw_edid, edid_len, GFP_KERNEL); > if (!edid) { > DRM_DEBUG_KMS("failed to allocate edid\n"); > return ERR_PTR(-ENOMEM); > } > > - memcpy(edid, ctx->raw_edid, edid_len); > return edid; > } > > @@ -563,12 +562,11 @@ int vidi_connection_ioctl(struct drm_device *drm_dev, > void *data, > return -EINVAL; > } > edid_len = (1 + raw_edid->extensions) * EDID_LENGTH; > - ctx->raw_edid = kzalloc(edid_len, GFP_KERNEL); > + ctx->raw_edid = kmemdup(raw_edid, edid_len, GFP_KERNEL); > if (!ctx->raw_edid) { > DRM_DEBUG_KMS("failed to allocate raw_edid.\n"); > return -ENOMEM; > } > - memcpy(ctx->raw_edid, raw_edid, edid_len); > } else { > /* >* with connection = 0, free raw_edid > -- Seung-Woo Kim Samsung Software R Center --
Re: [PATCH] drivers: gpu: drm: exynos: Replaced kzalloc memcpy with kmemdup
Good point. Acked-by: Seung-Woo Kim sw0312@samsung.com On 2013년 03월 12일 04:25, Alexandru Gheorghiu wrote: Replaced calls to kzalloc followed by memcpy with call to kmemdup. Patch found using coccinelle. Signed-off-by: Alexandru Gheorghiu gheorghiuan...@gmail.com --- drivers/gpu/drm/exynos/exynos_drm_vidi.c |6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c index 13ccbd4..9504b0c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c @@ -117,13 +117,12 @@ static struct edid *vidi_get_edid(struct device *dev, } edid_len = (1 + ctx-raw_edid-extensions) * EDID_LENGTH; - edid = kzalloc(edid_len, GFP_KERNEL); + edid = kmemdup(ctx-raw_edid, edid_len, GFP_KERNEL); if (!edid) { DRM_DEBUG_KMS(failed to allocate edid\n); return ERR_PTR(-ENOMEM); } - memcpy(edid, ctx-raw_edid, edid_len); return edid; } @@ -563,12 +562,11 @@ int vidi_connection_ioctl(struct drm_device *drm_dev, void *data, return -EINVAL; } edid_len = (1 + raw_edid-extensions) * EDID_LENGTH; - ctx-raw_edid = kzalloc(edid_len, GFP_KERNEL); + ctx-raw_edid = kmemdup(raw_edid, edid_len, GFP_KERNEL); if (!ctx-raw_edid) { DRM_DEBUG_KMS(failed to allocate raw_edid.\n); return -ENOMEM; } - memcpy(ctx-raw_edid, raw_edid, edid_len); } else { /* * with connection = 0, free raw_edid -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH RFC] drm/exynos: hdmi: move hdmiphy related code to hdmiphy driver
On 2013? 03? 07? 15:45, Rahul Sharma wrote: > Thanks Seung Woo, Mr. Dae, > > On Thu, Mar 7, 2013 at 10:13 AM, Inki Dae wrote: >> 2013/3/7 ??? : >>> >>> >>> On 2013? 03? 04? 23:05, Rahul Sharma wrote: Thanks Sean, On Wed, Feb 27, 2013 at 9:47 PM, Sean Paul wrote: > On Wed, Feb 27, 2013 at 8:22 AM, Rahul Sharma samsung.com> wrote: >> Right now hdmiphy operations and configs are kept inside hdmi driver. >> hdmiphy related >> code is tightly coupled with hdmi ip driver. Physicaly they are >> different devices and > > s/Physicaly/Physically/ > >> should be instantiated independently. >> >> In terms of versions/mapping/configurations Hdmi and hdmiphy are >> independent of each >> other. It is preferred to isolate them and maintained independently. >> >> This implementations is tested for: >> 1) Resolutions supported by exynos4 and 5 hdmi. >> 2) Runtime PM and S2R scenarions for exynos5. >> > > I don't like the idea of spawning off yet another driver in here. It > adds more globals, more suspend/resume ordering issues, and more > implicit dependencies. I understand, however, that this is the Chosen > Way for the exynos driver, so I will save my rant. > I agree to it. splitting phy to a new driver will complicate the power related scenarios. But in upcoming SoC,s, Phy is changing considerably in terms of config values, mapping (i2c/platform bus) etc. Handling this diversity inside hdmi driver is complicating it with unrelated changes. >>> >>> Basically, I agree with the idea to split hdmiphy from hdmi. And it >>> seems that already existing hdmiphy i2c device is just reused and >>> hdmiphy_power_on is reorganized to hdmiphy dpms operation: even calling >>> flow of power operations is reordered. >>> >>> But I'm not sure exynos_hdmiphy_driver_register() really need to be >>> called from exynos_drm_init() of exynos_drm_drv.c. IMO, it is enough to >>> call exynos_hdmiphy_driver_register() from hdmi_probe() because hdmiphy >>> is only used from hdmi. >>> >> >> I agree with Seung-Woo. The hdmiphy is just one part of HDMI subsystem. >> > > I agree to the Seung Woo's point that hdmi-phy used to be solely accessed by > hdmi driver. But in this RFC, hdmi-phy is not called by hdmi driver > anymore. Phy > ops will be called from drm-common-hdmi platform driver along with mixer and > hdmi ops. Considering this, exynos_drm_hdmi_probe() is more proper position. > > The rational behind my implementation is that I am projecting hdmi-phy as > a device which is peer to hdmi ip and mixer. These 3 devices together makes > DRM HDMI subsystem. > > Even physically hdmi-phy doesn't seems to be a part of hdmi ip though > configurations are listed under hdmi ip user manual. It looks like a > isolated module accessed by i2c. > > Though I don't find anything wrong with Seung Woo suggestion but above > placement of hdmi-phy (parallel to hdmi and mixer) makes more sense > to me. > > Please have a another look at it and let me know your opinion. > > Another things which bothers me is registering mixer, hdmi driver inside > exynos_drm_init(). If we strictly follow the hierarchy inside drm, > exynos_drm_init() > should register drm-common-hdmi only. drm-common-hdmi should register > mixer and hdmi (or hdmi-phy as well). Yes, it makes sense. All real hw blocks for hdmi including mixer, hdmi, and hdmiphy shoulde be registered in exynos_drm_hdmi (drm-common-hdmi for exynos). Thanks and Regards, - Seung-Woo Kim > > regards, > Rahul Sharma. > >> Thanks, >> Inki Dae >> >>> Thanks and Regards, >>> - Seung-Woo Kim >>> I have tested this RFC for Runtime PM / S2R. But if we see any major roadblock we should re-factor this by explicitly calling power related callbacks of mixer, phy, hdmi drivers in a required order. We can call them from exynos-drm-hdmi plf device. AFAIR something like this is already in place in chrome-kernel. > I've made some comments below. > >> This patch is dependent on >> http://www.mail-archive.com/dri-devel at >> lists.freedesktop.org/msg34733.html >> http://www.mail-archive.com/dri-devel at >> lists.freedesktop.org/msg34861.html >> http://www.mail-archive.com/dri-devel at >> lists.freedesktop.org/msg34862.html >> >> Signed-off-by: Rahul Sharma >> --- >> It is based on exynos-drm-next-todo branch at >> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git >> >> drivers/gpu/drm/exynos/exynos_drm_drv.c | 8 + >> drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 + >> drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 58 ++- >> drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 11 + >> drivers/gpu/drm/exynos/exynos_hdmi.c | 375 ++-- >> drivers/gpu/drm/exynos/exynos_hdmi.h | 1 - >>
[PATCH RFC] drm/exynos: hdmi: move hdmiphy related code to hdmiphy driver
On 2013? 03? 04? 23:05, Rahul Sharma wrote: > Thanks Sean, > > On Wed, Feb 27, 2013 at 9:47 PM, Sean Paul wrote: >> On Wed, Feb 27, 2013 at 8:22 AM, Rahul Sharma >> wrote: >>> Right now hdmiphy operations and configs are kept inside hdmi driver. >>> hdmiphy related >>> code is tightly coupled with hdmi ip driver. Physicaly they are different >>> devices and >> >> s/Physicaly/Physically/ >> >>> should be instantiated independently. >>> >>> In terms of versions/mapping/configurations Hdmi and hdmiphy are >>> independent of each >>> other. It is preferred to isolate them and maintained independently. >>> >>> This implementations is tested for: >>> 1) Resolutions supported by exynos4 and 5 hdmi. >>> 2) Runtime PM and S2R scenarions for exynos5. >>> >> >> I don't like the idea of spawning off yet another driver in here. It >> adds more globals, more suspend/resume ordering issues, and more >> implicit dependencies. I understand, however, that this is the Chosen >> Way for the exynos driver, so I will save my rant. >> > > I agree to it. splitting phy to a new driver will complicate the power related > scenarios. But in upcoming SoC,s, Phy is changing considerably in terms of > config values, mapping (i2c/platform bus) etc. Handling this diversity > inside hdmi driver is complicating it with unrelated changes. Basically, I agree with the idea to split hdmiphy from hdmi. And it seems that already existing hdmiphy i2c device is just reused and hdmiphy_power_on is reorganized to hdmiphy dpms operation: even calling flow of power operations is reordered. But I'm not sure exynos_hdmiphy_driver_register() really need to be called from exynos_drm_init() of exynos_drm_drv.c. IMO, it is enough to call exynos_hdmiphy_driver_register() from hdmi_probe() because hdmiphy is only used from hdmi. Thanks and Regards, - Seung-Woo Kim > > I have tested this RFC for Runtime PM / S2R. But if we see any major roadblock > we should re-factor this by explicitly calling power related callbacks > of mixer, phy, > hdmi drivers in a required order. We can call them from exynos-drm-hdmi plf > device. AFAIR something like this is already in place in chrome-kernel. > >> I've made some comments below. >> >>> This patch is dependent on >>> http://www.mail-archive.com/dri-devel at lists.freedesktop.org/msg34733.html >>> http://www.mail-archive.com/dri-devel at lists.freedesktop.org/msg34861.html >>> http://www.mail-archive.com/dri-devel at lists.freedesktop.org/msg34862.html >>> >>> Signed-off-by: Rahul Sharma >>> --- >>> It is based on exynos-drm-next-todo branch at >>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git >>> >>> drivers/gpu/drm/exynos/exynos_drm_drv.c | 8 + >>> drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 + >>> drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 58 ++- >>> drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 11 + >>> drivers/gpu/drm/exynos/exynos_hdmi.c | 375 ++-- >>> drivers/gpu/drm/exynos/exynos_hdmi.h | 1 - >>> drivers/gpu/drm/exynos/exynos_hdmiphy.c | 586 >>> ++- >>> drivers/gpu/drm/exynos/regs-hdmiphy.h| 61 >>> 8 files changed, 738 insertions(+), 368 deletions(-) >>> create mode 100644 drivers/gpu/drm/exynos/regs-hdmiphy.h >>> -- Seung-Woo Kim Samsung Software R Center --
Re: [PATCH RFC] drm/exynos: hdmi: move hdmiphy related code to hdmiphy driver
On 2013년 03월 04일 23:05, Rahul Sharma wrote: Thanks Sean, On Wed, Feb 27, 2013 at 9:47 PM, Sean Paul seanp...@google.com wrote: On Wed, Feb 27, 2013 at 8:22 AM, Rahul Sharma rahul.sha...@samsung.com wrote: Right now hdmiphy operations and configs are kept inside hdmi driver. hdmiphy related code is tightly coupled with hdmi ip driver. Physicaly they are different devices and s/Physicaly/Physically/ should be instantiated independently. In terms of versions/mapping/configurations Hdmi and hdmiphy are independent of each other. It is preferred to isolate them and maintained independently. This implementations is tested for: 1) Resolutions supported by exynos4 and 5 hdmi. 2) Runtime PM and S2R scenarions for exynos5. I don't like the idea of spawning off yet another driver in here. It adds more globals, more suspend/resume ordering issues, and more implicit dependencies. I understand, however, that this is the Chosen Way for the exynos driver, so I will save my rant. I agree to it. splitting phy to a new driver will complicate the power related scenarios. But in upcoming SoC,s, Phy is changing considerably in terms of config values, mapping (i2c/platform bus) etc. Handling this diversity inside hdmi driver is complicating it with unrelated changes. Basically, I agree with the idea to split hdmiphy from hdmi. And it seems that already existing hdmiphy i2c device is just reused and hdmiphy_power_on is reorganized to hdmiphy dpms operation: even calling flow of power operations is reordered. But I'm not sure exynos_hdmiphy_driver_register() really need to be called from exynos_drm_init() of exynos_drm_drv.c. IMO, it is enough to call exynos_hdmiphy_driver_register() from hdmi_probe() because hdmiphy is only used from hdmi. Thanks and Regards, - Seung-Woo Kim I have tested this RFC for Runtime PM / S2R. But if we see any major roadblock we should re-factor this by explicitly calling power related callbacks of mixer, phy, hdmi drivers in a required order. We can call them from exynos-drm-hdmi plf device. AFAIR something like this is already in place in chrome-kernel. I've made some comments below. This patch is dependent on http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg34733.html http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg34861.html http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg34862.html Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- It is based on exynos-drm-next-todo branch at git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git drivers/gpu/drm/exynos/exynos_drm_drv.c | 8 + drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 + drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 58 ++- drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 11 + drivers/gpu/drm/exynos/exynos_hdmi.c | 375 ++-- drivers/gpu/drm/exynos/exynos_hdmi.h | 1 - drivers/gpu/drm/exynos/exynos_hdmiphy.c | 586 ++- drivers/gpu/drm/exynos/regs-hdmiphy.h| 61 8 files changed, 738 insertions(+), 368 deletions(-) create mode 100644 drivers/gpu/drm/exynos/regs-hdmiphy.h snip -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC] drm/exynos: hdmi: move hdmiphy related code to hdmiphy driver
On 2013년 03월 07일 15:45, Rahul Sharma wrote: Thanks Seung Woo, Mr. Dae, On Thu, Mar 7, 2013 at 10:13 AM, Inki Dae inki@samsung.com wrote: 2013/3/7 김승우 sw0312@samsung.com: On 2013년 03월 04일 23:05, Rahul Sharma wrote: Thanks Sean, On Wed, Feb 27, 2013 at 9:47 PM, Sean Paul seanp...@google.com wrote: On Wed, Feb 27, 2013 at 8:22 AM, Rahul Sharma rahul.sha...@samsung.com wrote: Right now hdmiphy operations and configs are kept inside hdmi driver. hdmiphy related code is tightly coupled with hdmi ip driver. Physicaly they are different devices and s/Physicaly/Physically/ should be instantiated independently. In terms of versions/mapping/configurations Hdmi and hdmiphy are independent of each other. It is preferred to isolate them and maintained independently. This implementations is tested for: 1) Resolutions supported by exynos4 and 5 hdmi. 2) Runtime PM and S2R scenarions for exynos5. I don't like the idea of spawning off yet another driver in here. It adds more globals, more suspend/resume ordering issues, and more implicit dependencies. I understand, however, that this is the Chosen Way for the exynos driver, so I will save my rant. I agree to it. splitting phy to a new driver will complicate the power related scenarios. But in upcoming SoC,s, Phy is changing considerably in terms of config values, mapping (i2c/platform bus) etc. Handling this diversity inside hdmi driver is complicating it with unrelated changes. Basically, I agree with the idea to split hdmiphy from hdmi. And it seems that already existing hdmiphy i2c device is just reused and hdmiphy_power_on is reorganized to hdmiphy dpms operation: even calling flow of power operations is reordered. But I'm not sure exynos_hdmiphy_driver_register() really need to be called from exynos_drm_init() of exynos_drm_drv.c. IMO, it is enough to call exynos_hdmiphy_driver_register() from hdmi_probe() because hdmiphy is only used from hdmi. I agree with Seung-Woo. The hdmiphy is just one part of HDMI subsystem. I agree to the Seung Woo's point that hdmi-phy used to be solely accessed by hdmi driver. But in this RFC, hdmi-phy is not called by hdmi driver anymore. Phy ops will be called from drm-common-hdmi platform driver along with mixer and hdmi ops. Considering this, exynos_drm_hdmi_probe() is more proper position. The rational behind my implementation is that I am projecting hdmi-phy as a device which is peer to hdmi ip and mixer. These 3 devices together makes DRM HDMI subsystem. Even physically hdmi-phy doesn't seems to be a part of hdmi ip though configurations are listed under hdmi ip user manual. It looks like a isolated module accessed by i2c. Though I don't find anything wrong with Seung Woo suggestion but above placement of hdmi-phy (parallel to hdmi and mixer) makes more sense to me. Please have a another look at it and let me know your opinion. Another things which bothers me is registering mixer, hdmi driver inside exynos_drm_init(). If we strictly follow the hierarchy inside drm, exynos_drm_init() should register drm-common-hdmi only. drm-common-hdmi should register mixer and hdmi (or hdmi-phy as well). Yes, it makes sense. All real hw blocks for hdmi including mixer, hdmi, and hdmiphy shoulde be registered in exynos_drm_hdmi (drm-common-hdmi for exynos). Thanks and Regards, - Seung-Woo Kim regards, Rahul Sharma. Thanks, Inki Dae Thanks and Regards, - Seung-Woo Kim I have tested this RFC for Runtime PM / S2R. But if we see any major roadblock we should re-factor this by explicitly calling power related callbacks of mixer, phy, hdmi drivers in a required order. We can call them from exynos-drm-hdmi plf device. AFAIR something like this is already in place in chrome-kernel. I've made some comments below. This patch is dependent on http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg34733.html http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg34861.html http://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg34862.html Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- It is based on exynos-drm-next-todo branch at git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git drivers/gpu/drm/exynos/exynos_drm_drv.c | 8 + drivers/gpu/drm/exynos/exynos_drm_drv.h | 6 + drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 58 ++- drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 11 + drivers/gpu/drm/exynos/exynos_hdmi.c | 375 ++-- drivers/gpu/drm/exynos/exynos_hdmi.h | 1 - drivers/gpu/drm/exynos/exynos_hdmiphy.c | 586 ++- drivers/gpu/drm/exynos/regs-hdmiphy.h| 61 8 files changed, 738 insertions(+), 368 deletions(-) create mode 100644 drivers/gpu/drm/exynos/regs-hdmiphy.h snip -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing
[PATCH v2 2/2] drm/exynos: Add device tree based discovery support for G2D
On 2013? 02? 06? 17:51, Inki Dae wrote: > > >> -Original Message- >> From: Sachin Kamat [mailto:sachin.kamat at linaro.org] >> Sent: Wednesday, February 06, 2013 5:03 PM >> To: Inki Dae >> Cc: linux-media at vger.kernel.org; dri-devel at lists.freedesktop.org; >> devicetree-discuss at lists.ozlabs.org; k.debski at samsung.com; >> s.nawrocki at samsung.com; kgene.kim at samsung.com; patches at linaro.org; >> Ajay >> Kumar >> Subject: Re: [PATCH v2 2/2] drm/exynos: Add device tree based discovery >> support for G2D >> >> On 6 February 2013 13:02, Inki Dae wrote: >>> >>> Looks good to me but please add document for it. >> >> Yes. I will. I was planning to send the bindings document patch along >> with the dt patches (adding node entries to dts files). >> Sylwester had suggested adding this to >> Documentation/devicetree/bindings/media/ which contains other media >> IPs. > > I think that it's better to go to gpu than media and we can divide Exynos > IPs into the bellow categories, > > Media : mfc > GPU : g2d, g3d, fimc, gsc > Video : fimd, hdmi, eDP, MIPI-DSI Hm, here is another considering point. Some device can be used as one of two sub-system. For example g2d can be used as V4L2 driver or DRM driver. And more specific case, multiple fimc/gsc deivces can be separately used as both drivers: two fimc devices are used as V4L2 driver and other devices are used as DRM driver. Current discussion, without change of build configuration, device can be only used as one driver. So I want to discuss about how we can bind device and driver just with dts configuration. IMO, there are two options. First, driver usage is set on configurable node. g2d: g2d { compatible = "samsung,exynos4212-g2d"; ... *subsystem = "v4l2"* or *subsystem = "drm"* }; Node name and type is just an example to describe. With this option, driver which is not matched with subsystem node should return with fail during its probing. Second, using dual compatible strings. g2d: g2d { *compatible = "samsung,exynos4212-v4l2-g2d"; or compatible = "samsung,exynos4212-v4l2-g2d";* ... }; String is just an example so don't mind if it is ugly. Actually, with this option, compatible string has non HW information. But this option does not need fail in probing. I'm not sure these options are fit to DT concept. Please let me know if anyone has idea. Best Regards, - Seung-Woo Kim > > And I think that the device-tree describes hardware so possibly, all > documents in .../bindings/drm/exynos/* should be moved to proper place also. > Please give me any opinions. > > Thanks, > Inki Dae > >> >>> >>> To other guys, >>> And is there anyone who know where this document should be added to? >>> I'm not sure that the g2d document should be placed in >>> Documentation/devicetree/bindings/gpu, media, drm/exynos or arm/exynos. >> At >>> least, this document should be shared with the g2d hw relevant drivers >> such >>> as v4l2 and drm. So is ".../bindings/gpu" proper place? >>> >> >> >> -- >> With warm regards, >> Sachin > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >
[PATCH v3 1/3] drm/exynos: Get HDMI version from device tree
On 2013? 02? 06? 09:56, Sean Paul wrote: > On Tue, Feb 5, 2013 at 4:42 PM, Stephen Warren > wrote: >> On 02/05/2013 05:37 PM, Sean Paul wrote: >>> On Tue, Feb 5, 2013 at 4:22 PM, Stephen Warren >>> wrote: n 02/05/2013 04:42 PM, Sean Paul wrote: > Use the compatible string in the device tree to determine which > registers/functions to use in the HDMI driver. Also changes the > references from v13 to 4210 and v14 to 4212 to reflect the IP > block version instead of the HDMI version. > diff --git a/Documentation/devicetree/bindings/drm/exynos/hdmi.txt > b/Documentation/devicetree/bindings/drm/exynos/hdmi.txt Binding looks sane to me. > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c > b/drivers/gpu/drm/exynos/exynos_hdmi.c > #ifdef CONFIG_OF > static struct of_device_id hdmi_match_types[] = { > { > - .compatible = "samsung,exynos5-hdmi", > - .data = (void *)HDMI_TYPE14, > + .compatible = "samsung,exynos4-hdmi", > }, { > /* end node */ > } Why not fill in all the "base" compatible values there (I think you need this anyway so that DTs don't all have to be compatible with samsung,exynos4-hdmi), with .data containing the HDMI_VER_EXYNOS* values, then ... >>> >>> At the moment, all DTs have to be compatible with exynos4-hdmi since >>> it provides the base for the current driver. The driver uses 4210 and >>> 4212 to differentiate between different register addresses and >>> features, but most things are just exynos4-hdmi compatible. >> >> The DT nodes should include only the compatible values that the HW is >> actually compatible with. If the HW isn't compatible with exynos4-hdmi, >> that value shouldn't be in the compatible property, but instead whatever >> the "base" value that the HW really is compatible with. The driver can >> support multiple "base" compatible values from this table. >> > > All devices that use this driver are compatible, at some level, with > exynos4-hdmi, so I think its usage is correct here. > > @@ -2218,17 +2217,18 @@ static int hdmi_probe(struct platform_device > *pdev) > + > + if (of_device_is_compatible(dev->of_node, > "samsung,exynos4210-hdmi")) > + hdata->version |= HDMI_VER_EXYNOS4210; > + if (of_device_is_compatible(dev->of_node, > "samsung,exynos4212-hdmi")) > + hdata->version |= HDMI_VER_EXYNOS4212; > + if (of_device_is_compatible(dev->of_node, > "samsung,exynos5250-hdmi")) > + hdata->version |= HDMI_VER_EXYNOS5250; Instead of that, do roughly: match = of_match_device(hdmi_match_types, >dev); if (match) hdata->version |= (int)match->data; That way, it's all table-based. Any future additions to hdmi_match_types[] won't require another if statement to be added to probe(). >>> >>> I don't think it's that easy. of_match_device returns the first match >>> from the device table, so I'd still need to iterate through the >>> matches. I could still break this out into a table, but I don't think >>> of_match_device is the right way to probe it. >> >> You shouldn't have to iterate over multiple matches. of_match_device() >> is supposed to return the match for the first entry in the compatible >> property, then if there was no match, move on to looking at the next >> entry in the compatible property, etc. In practice, I think it's still >> not implemented quite correctly for this, but you can make it work by >> putting the newest compatible value first in the match table. > > I think the only way that works is if you hardcode the compatible > versions in the driver, like this: > > static struct of_device_id hdmi_match_types[] = { > { > .compatible = "samsung,exynos5250-hdmi", > .data = (void *)(HDMI_VER_EXYNOS5250 | HDMI_VER_EXYNOS4212); > }, { > .compatible = "samsung,exynos4212-hdmi", > .data = (void *)HDMI_VER_EXYNOS4212; > }, { > .compatible = "samsung,exynos4210-hdmi", > .data = (void *)HDMI_VER_EXYNOS4210; > }, { > /* end node */ > } > }; Actually, I can't understand why there is HDMI_VER_EXYNOS5250 because it is not used anywhere in your patch. I'm not sure I missed something from your v2 patch thread, but to me, just hdmi version or hdmi ip version can be used as data field of struct of_device_id as like your v2 patch set. and then of_match_device() can be used without | in data field. If I have missed some point from v2 thread, please let me know. Best Regards, - Seung-Woo Kim > > In that case, it eliminates the benefit of using device tree to > determine the compatible bits. I hope I'm just being thick and missing > something. > > Sean >
Re: [PATCH v2 2/2] drm/exynos: Add device tree based discovery support for G2D
On 2013년 02월 06일 17:51, Inki Dae wrote: -Original Message- From: Sachin Kamat [mailto:sachin.ka...@linaro.org] Sent: Wednesday, February 06, 2013 5:03 PM To: Inki Dae Cc: linux-me...@vger.kernel.org; dri-devel@lists.freedesktop.org; devicetree-disc...@lists.ozlabs.org; k.deb...@samsung.com; s.nawro...@samsung.com; kgene@samsung.com; patc...@linaro.org; Ajay Kumar Subject: Re: [PATCH v2 2/2] drm/exynos: Add device tree based discovery support for G2D On 6 February 2013 13:02, Inki Dae inki@samsung.com wrote: Looks good to me but please add document for it. Yes. I will. I was planning to send the bindings document patch along with the dt patches (adding node entries to dts files). Sylwester had suggested adding this to Documentation/devicetree/bindings/media/ which contains other media IPs. I think that it's better to go to gpu than media and we can divide Exynos IPs into the bellow categories, Media : mfc GPU : g2d, g3d, fimc, gsc Video : fimd, hdmi, eDP, MIPI-DSI Hm, here is another considering point. Some device can be used as one of two sub-system. For example g2d can be used as V4L2 driver or DRM driver. And more specific case, multiple fimc/gsc deivces can be separately used as both drivers: two fimc devices are used as V4L2 driver and other devices are used as DRM driver. Current discussion, without change of build configuration, device can be only used as one driver. So I want to discuss about how we can bind device and driver just with dts configuration. IMO, there are two options. First, driver usage is set on configurable node. g2d: g2d { compatible = samsung,exynos4212-g2d; ... *subsystem = v4l2* or *subsystem = drm* }; Node name and type is just an example to describe. With this option, driver which is not matched with subsystem node should return with fail during its probing. Second, using dual compatible strings. g2d: g2d { *compatible = samsung,exynos4212-v4l2-g2d; or compatible = samsung,exynos4212-v4l2-g2d;* ... }; String is just an example so don't mind if it is ugly. Actually, with this option, compatible string has non HW information. But this option does not need fail in probing. I'm not sure these options are fit to DT concept. Please let me know if anyone has idea. Best Regards, - Seung-Woo Kim And I think that the device-tree describes hardware so possibly, all documents in .../bindings/drm/exynos/* should be moved to proper place also. Please give me any opinions. Thanks, Inki Dae To other guys, And is there anyone who know where this document should be added to? I'm not sure that the g2d document should be placed in Documentation/devicetree/bindings/gpu, media, drm/exynos or arm/exynos. At least, this document should be shared with the g2d hw relevant drivers such as v4l2 and drm. So is .../bindings/gpu proper place? -- With warm regards, Sachin ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/exynos: Add device tree based discovery support for G2D
Hi Inki, On 2013? 02? 05? 12:03, Inki Dae wrote: > 2013/2/4 Sachin Kamat : >> On 1 February 2013 18:28, Inki Dae wrote: >>> >>> >>> >>> >>> 2013. 2. 1. ?? 8:52 Inki Dae ??: >>> > -Original Message- > From: linux-media-owner at vger.kernel.org [mailto:linux-media- > owner at vger.kernel.org] On Behalf Of Sachin Kamat > Sent: Friday, February 01, 2013 8:40 PM > To: Inki Dae > Cc: Sylwester Nawrocki; Kukjin Kim; Sylwester Nawrocki; linux- > media at vger.kernel.org; dri-devel at lists.freedesktop.org; devicetree- > discuss at lists.ozlabs.org; patches at linaro.org > Subject: Re: [PATCH 2/2] drm/exynos: Add device tree based discovery > support for G2D > > On 1 February 2013 17:02, Inki Dae wrote: >> >> How about using like below? >>Compatible = ""samsung,exynos4x12-fimg-2d" /* for Exynos4212, >> Exynos4412 */ >> It looks odd to use "samsung,exynos4212-fimg-2d" saying that this ip is > for >> exynos4212 and exynos4412. > > AFAIK, compatible strings are not supposed to have any wildcard characters. > Compatible string should suggest the first SoC that contained this IP. > Hence IMO 4212 is OK. > >>> >>> Oops, one more thing. AFAIK Exynos4210 also has fimg-2d ip. In this case, >>> we should use "samsung,exynos4210-fimg-2d" as comparible string and add it >>> to exynos4210.dtsi? >> >> Exynos4210 has same g2d IP (v3.0) as C110 or V210; so the same >> comptible string will be used for this one too. >> >>> And please check if exynos4212 and 4412 SoCs have same fimg-2d ip. If it's >>> different, we might need to add ip version property or compatible string to >>> each dtsi file to identify the ip version. >> >> AFAIK, they both have the same IP (v4.1). >> > > Ok, let's use the below, > > For exynos4210 SoC, > compatible = "samsung,exynos4210-g2d" > > For exynos4x12 SoCs, > compatible = "samsung,exynos4212-g2d" > > For exynos5250, 5410 (In case of Exynos5440, I'm not sure that the SoC > has same ip) > compatible = "samsung,exynos5250-g2d" > > To other guys, > The device tree is used by not only v4l2 side but also drm side so we > should reach an arrangement. So please give me ack if you agree to my > opinion. Otherwise please, give me your opinions. This seems good to me. Best Regards, - Seung-Woo Kim > > Thanks, > Inki Dae > > >>> >>> Sorry but give me your opinions. >>> >>> Thanks, >>> Inki Dae >>> >>> Got it. Please post it again. > > -- > With warm regards, > Sachin > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ___ dri-devel mailing list dri-devel at lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> >> >> -- >> With warm regards, >> Sachin > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Seung-Woo Kim Samsung Software R Center --
Re: [PATCH 2/2] drm/exynos: Add device tree based discovery support for G2D
Hi Inki, On 2013년 02월 05일 12:03, Inki Dae wrote: 2013/2/4 Sachin Kamat sachin.ka...@linaro.org: On 1 February 2013 18:28, Inki Dae daei...@gmail.com wrote: 2013. 2. 1. 오후 8:52 Inki Dae inki@samsung.com 작성: -Original Message- From: linux-media-ow...@vger.kernel.org [mailto:linux-media- ow...@vger.kernel.org] On Behalf Of Sachin Kamat Sent: Friday, February 01, 2013 8:40 PM To: Inki Dae Cc: Sylwester Nawrocki; Kukjin Kim; Sylwester Nawrocki; linux- me...@vger.kernel.org; dri-devel@lists.freedesktop.org; devicetree- disc...@lists.ozlabs.org; patc...@linaro.org Subject: Re: [PATCH 2/2] drm/exynos: Add device tree based discovery support for G2D On 1 February 2013 17:02, Inki Dae inki@samsung.com wrote: How about using like below? Compatible = samsung,exynos4x12-fimg-2d /* for Exynos4212, Exynos4412 */ It looks odd to use samsung,exynos4212-fimg-2d saying that this ip is for exynos4212 and exynos4412. AFAIK, compatible strings are not supposed to have any wildcard characters. Compatible string should suggest the first SoC that contained this IP. Hence IMO 4212 is OK. Oops, one more thing. AFAIK Exynos4210 also has fimg-2d ip. In this case, we should use samsung,exynos4210-fimg-2d as comparible string and add it to exynos4210.dtsi? Exynos4210 has same g2d IP (v3.0) as C110 or V210; so the same comptible string will be used for this one too. And please check if exynos4212 and 4412 SoCs have same fimg-2d ip. If it's different, we might need to add ip version property or compatible string to each dtsi file to identify the ip version. AFAIK, they both have the same IP (v4.1). Ok, let's use the below, For exynos4210 SoC, compatible = samsung,exynos4210-g2d For exynos4x12 SoCs, compatible = samsung,exynos4212-g2d For exynos5250, 5410 (In case of Exynos5440, I'm not sure that the SoC has same ip) compatible = samsung,exynos5250-g2d To other guys, The device tree is used by not only v4l2 side but also drm side so we should reach an arrangement. So please give me ack if you agree to my opinion. Otherwise please, give me your opinions. This seems good to me. Best Regards, - Seung-Woo Kim Thanks, Inki Dae Sorry but give me your opinions. Thanks, Inki Dae Got it. Please post it again. -- With warm regards, Sachin -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- With warm regards, Sachin ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 1/3] drm/exynos: Get HDMI version from device tree
On 2013년 02월 06일 09:56, Sean Paul wrote: On Tue, Feb 5, 2013 at 4:42 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 02/05/2013 05:37 PM, Sean Paul wrote: On Tue, Feb 5, 2013 at 4:22 PM, Stephen Warren swar...@wwwdotorg.org wrote: n 02/05/2013 04:42 PM, Sean Paul wrote: Use the compatible string in the device tree to determine which registers/functions to use in the HDMI driver. Also changes the references from v13 to 4210 and v14 to 4212 to reflect the IP block version instead of the HDMI version. diff --git a/Documentation/devicetree/bindings/drm/exynos/hdmi.txt b/Documentation/devicetree/bindings/drm/exynos/hdmi.txt Binding looks sane to me. diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c #ifdef CONFIG_OF static struct of_device_id hdmi_match_types[] = { { - .compatible = samsung,exynos5-hdmi, - .data = (void *)HDMI_TYPE14, + .compatible = samsung,exynos4-hdmi, }, { /* end node */ } Why not fill in all the base compatible values there (I think you need this anyway so that DTs don't all have to be compatible with samsung,exynos4-hdmi), with .data containing the HDMI_VER_EXYNOS* values, then ... At the moment, all DTs have to be compatible with exynos4-hdmi since it provides the base for the current driver. The driver uses 4210 and 4212 to differentiate between different register addresses and features, but most things are just exynos4-hdmi compatible. The DT nodes should include only the compatible values that the HW is actually compatible with. If the HW isn't compatible with exynos4-hdmi, that value shouldn't be in the compatible property, but instead whatever the base value that the HW really is compatible with. The driver can support multiple base compatible values from this table. All devices that use this driver are compatible, at some level, with exynos4-hdmi, so I think its usage is correct here. @@ -2218,17 +2217,18 @@ static int hdmi_probe(struct platform_device *pdev) + + if (of_device_is_compatible(dev-of_node, samsung,exynos4210-hdmi)) + hdata-version |= HDMI_VER_EXYNOS4210; + if (of_device_is_compatible(dev-of_node, samsung,exynos4212-hdmi)) + hdata-version |= HDMI_VER_EXYNOS4212; + if (of_device_is_compatible(dev-of_node, samsung,exynos5250-hdmi)) + hdata-version |= HDMI_VER_EXYNOS5250; Instead of that, do roughly: match = of_match_device(hdmi_match_types, pdev-dev); if (match) hdata-version |= (int)match-data; That way, it's all table-based. Any future additions to hdmi_match_types[] won't require another if statement to be added to probe(). I don't think it's that easy. of_match_device returns the first match from the device table, so I'd still need to iterate through the matches. I could still break this out into a table, but I don't think of_match_device is the right way to probe it. You shouldn't have to iterate over multiple matches. of_match_device() is supposed to return the match for the first entry in the compatible property, then if there was no match, move on to looking at the next entry in the compatible property, etc. In practice, I think it's still not implemented quite correctly for this, but you can make it work by putting the newest compatible value first in the match table. I think the only way that works is if you hardcode the compatible versions in the driver, like this: static struct of_device_id hdmi_match_types[] = { { .compatible = samsung,exynos5250-hdmi, .data = (void *)(HDMI_VER_EXYNOS5250 | HDMI_VER_EXYNOS4212); }, { .compatible = samsung,exynos4212-hdmi, .data = (void *)HDMI_VER_EXYNOS4212; }, { .compatible = samsung,exynos4210-hdmi, .data = (void *)HDMI_VER_EXYNOS4210; }, { /* end node */ } }; Actually, I can't understand why there is HDMI_VER_EXYNOS5250 because it is not used anywhere in your patch. I'm not sure I missed something from your v2 patch thread, but to me, just hdmi version or hdmi ip version can be used as data field of struct of_device_id as like your v2 patch set. and then of_match_device() can be used without | in data field. If I have missed some point from v2 thread, please let me know. Best Regards, - Seung-Woo Kim In that case, it eliminates the benefit of using device tree to determine the compatible bits. I hope I'm just being thick and missing something. Sean ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org
[PATCH] drm/exynos: let drm handle edid allocations
Hi Rahul, On 2012? 12? 28? 16:01, Rahul Sharma wrote: > There's no need to allocate edid twice and do a memcpy when drm helpers > exist to do just that. This patch cleans that interaction up, and > doesn't keep the edid hanging around in the connector. Basically, I agree about this idea. But exynos_drm_vidi also uses display_ops->get_edid(), so vidi should be considered. Best Regards, - Seung-Woo Kim > > Signed-off-by: Sean Paul > Signed-off-by: Rahul Sharma > --- > This patch is based on branch "exynos-drm-next" at > http://git.kernel.org/?p=linux/kernel/git/daeinki/drm-exynos.git > > drivers/gpu/drm/exynos/exynos_drm_connector.c | 36 > ++- > drivers/gpu/drm/exynos/exynos_drm_drv.h | 4 +-- > drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 9 +++ > drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 4 +-- > drivers/gpu/drm/exynos/exynos_hdmi.c | 25 --- > 5 files changed, 37 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c > b/drivers/gpu/drm/exynos/exynos_drm_connector.c > index ab37437..7ee43aa 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c > @@ -96,7 +96,9 @@ static int exynos_drm_connector_get_modes(struct > drm_connector *connector) > to_exynos_connector(connector); > struct exynos_drm_manager *manager = exynos_connector->manager; > struct exynos_drm_display_ops *display_ops = manager->display_ops; > - unsigned int count; > + unsigned int count = 0; > + struct edid *edid = NULL; > + int ret; > > DRM_DEBUG_KMS("%s\n", __FILE__); > > @@ -114,27 +116,25 @@ static int exynos_drm_connector_get_modes(struct > drm_connector *connector) >* because lcd panel has only one mode. >*/ > if (display_ops->get_edid) { > - int ret; > - void *edid; > - > - edid = kzalloc(MAX_EDID, GFP_KERNEL); > - if (!edid) { > - DRM_ERROR("failed to allocate edid\n"); > - return 0; > + edid = display_ops->get_edid(manager->dev, connector); > + if (IS_ERR_OR_NULL(edid)) { > + ret = PTR_ERR(edid); > + edid = NULL; > + DRM_ERROR("Panel operation get_edid failed %d\n", ret); > + goto out; > } > > - ret = display_ops->get_edid(manager->dev, connector, > - edid, MAX_EDID); > - if (ret < 0) { > - DRM_ERROR("failed to get edid data.\n"); > - kfree(edid); > - edid = NULL; > - return 0; > + ret = drm_mode_connector_update_edid_property(connector, edid); > + if (ret) { > + DRM_ERROR("update edid property failed(%d)\n", ret); > + goto out; > } > > - drm_mode_connector_update_edid_property(connector, edid); > count = drm_add_edid_modes(connector, edid); > - kfree(edid); > + if (count < 0) { > + DRM_ERROR("Add edid modes failed %d\n", count); > + goto out; > + } > } else { > struct exynos_drm_panel_info *panel; > struct drm_display_mode *mode = drm_mode_create(connector->dev); > @@ -161,6 +161,8 @@ static int exynos_drm_connector_get_modes(struct > drm_connector *connector) > count = 1; > } > > +out: > + kfree(edid); > return count; > } > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h > b/drivers/gpu/drm/exynos/exynos_drm_drv.h > index b9e51bc..4606fac 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h > @@ -148,8 +148,8 @@ struct exynos_drm_overlay { > struct exynos_drm_display_ops { > enum exynos_drm_output_type type; > bool (*is_connected)(struct device *dev); > - int (*get_edid)(struct device *dev, struct drm_connector *connector, > - u8 *edid, int len); > + struct edid *(*get_edid)(struct device *dev, > + struct drm_connector *connector); > void *(*get_panel)(struct device *dev); > int (*check_timing)(struct device *dev, void *timing); > int (*power_on)(struct device *dev, int mode);
Re: [PATCH] drm/exynos: let drm handle edid allocations
Hi Rahul, On 2012년 12월 28일 16:01, Rahul Sharma wrote: There's no need to allocate edid twice and do a memcpy when drm helpers exist to do just that. This patch cleans that interaction up, and doesn't keep the edid hanging around in the connector. Basically, I agree about this idea. But exynos_drm_vidi also uses display_ops-get_edid(), so vidi should be considered. Best Regards, - Seung-Woo Kim Signed-off-by: Sean Paul seanp...@chromium.org Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- This patch is based on branch exynos-drm-next at http://git.kernel.org/?p=linux/kernel/git/daeinki/drm-exynos.git drivers/gpu/drm/exynos/exynos_drm_connector.c | 36 ++- drivers/gpu/drm/exynos/exynos_drm_drv.h | 4 +-- drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 9 +++ drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 4 +-- drivers/gpu/drm/exynos/exynos_hdmi.c | 25 --- 5 files changed, 37 insertions(+), 41 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c b/drivers/gpu/drm/exynos/exynos_drm_connector.c index ab37437..7ee43aa 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c @@ -96,7 +96,9 @@ static int exynos_drm_connector_get_modes(struct drm_connector *connector) to_exynos_connector(connector); struct exynos_drm_manager *manager = exynos_connector-manager; struct exynos_drm_display_ops *display_ops = manager-display_ops; - unsigned int count; + unsigned int count = 0; + struct edid *edid = NULL; + int ret; DRM_DEBUG_KMS(%s\n, __FILE__); @@ -114,27 +116,25 @@ static int exynos_drm_connector_get_modes(struct drm_connector *connector) * because lcd panel has only one mode. */ if (display_ops-get_edid) { - int ret; - void *edid; - - edid = kzalloc(MAX_EDID, GFP_KERNEL); - if (!edid) { - DRM_ERROR(failed to allocate edid\n); - return 0; + edid = display_ops-get_edid(manager-dev, connector); + if (IS_ERR_OR_NULL(edid)) { + ret = PTR_ERR(edid); + edid = NULL; + DRM_ERROR(Panel operation get_edid failed %d\n, ret); + goto out; } - ret = display_ops-get_edid(manager-dev, connector, - edid, MAX_EDID); - if (ret 0) { - DRM_ERROR(failed to get edid data.\n); - kfree(edid); - edid = NULL; - return 0; + ret = drm_mode_connector_update_edid_property(connector, edid); + if (ret) { + DRM_ERROR(update edid property failed(%d)\n, ret); + goto out; } - drm_mode_connector_update_edid_property(connector, edid); count = drm_add_edid_modes(connector, edid); - kfree(edid); + if (count 0) { + DRM_ERROR(Add edid modes failed %d\n, count); + goto out; + } } else { struct exynos_drm_panel_info *panel; struct drm_display_mode *mode = drm_mode_create(connector-dev); @@ -161,6 +161,8 @@ static int exynos_drm_connector_get_modes(struct drm_connector *connector) count = 1; } +out: + kfree(edid); return count; } diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index b9e51bc..4606fac 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -148,8 +148,8 @@ struct exynos_drm_overlay { struct exynos_drm_display_ops { enum exynos_drm_output_type type; bool (*is_connected)(struct device *dev); - int (*get_edid)(struct device *dev, struct drm_connector *connector, - u8 *edid, int len); + struct edid *(*get_edid)(struct device *dev, + struct drm_connector *connector); void *(*get_panel)(struct device *dev); int (*check_timing)(struct device *dev, void *timing); int (*power_on)(struct device *dev, int mode); snip ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/prime: drop reference on imported dma-buf come from gem
Hi Dave, On 2012? 12? 18? 15:30, Dave Airlie wrote: > On Thu, Sep 27, 2012 at 4:30 PM, Seung-Woo Kim > wrote: >> Increasing ref counts of both dma-buf and gem for imported dma-buf come from >> gem >> makes memory leak. release function of dma-buf cannot be called because >> f_count >> of dma-buf increased by importing gem and gem ref count cannot be decrease >> because of exported dma-buf. >> >> So I add dma_buf_put() for imported gem come from its own gem into each >> drivers >> having prime_import and prime_export capabilities. With this, only gem ref >> count is increased if importing gem exported from gem of same driver. > > Okay its taken me a while to circle around and get back to this, but > yes I admit this is needed, but I hate implementing it like this > > But I think I'll push it and work out a cleaner solution, I should > also go back and look at the older patches. I want to also report some strange thing in dma-buf prime export. In drm_prime_handle_to_fd_ioctl(), flags is cleared to only support DRM_CLOEXEC but in gem_prime_export() callbacks of each driver, it uses 0600 as flags for dma_buf_export() like following. return dma_buf_export(obj, _dmabuf_ops, obj->base.size, 0600); Best Regards, - Seung-Woo Kim > > Dave. > -- Seung-Woo Kim Samsung Software R Center --
Re: [PATCH] drm/prime: drop reference on imported dma-buf come from gem
Hi Dave, On 2012년 12월 18일 15:30, Dave Airlie wrote: On Thu, Sep 27, 2012 at 4:30 PM, Seung-Woo Kim sw0312@samsung.com wrote: Increasing ref counts of both dma-buf and gem for imported dma-buf come from gem makes memory leak. release function of dma-buf cannot be called because f_count of dma-buf increased by importing gem and gem ref count cannot be decrease because of exported dma-buf. So I add dma_buf_put() for imported gem come from its own gem into each drivers having prime_import and prime_export capabilities. With this, only gem ref count is increased if importing gem exported from gem of same driver. Okay its taken me a while to circle around and get back to this, but yes I admit this is needed, but I hate implementing it like this But I think I'll push it and work out a cleaner solution, I should also go back and look at the older patches. I want to also report some strange thing in dma-buf prime export. In drm_prime_handle_to_fd_ioctl(), flags is cleared to only support DRM_CLOEXEC but in gem_prime_export() callbacks of each driver, it uses 0600 as flags for dma_buf_export() like following. return dma_buf_export(obj, i915_dmabuf_ops, obj-base.size, 0600); Best Regards, - Seung-Woo Kim Dave. -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH -next] drm/exynos/iommu: fix return value check in drm_create_iommu_mapping()
On 2012? 12? 07? 21:50, Wei Yongjun wrote: > From: Wei Yongjun > > In case of error, function arm_iommu_create_mapping() returns > ERR_PTR() and never returns NULL. The NULL test in the return > value check should be replaced with IS_ERR(). > > Signed-off-by: Wei Yongjun > --- > drivers/gpu/drm/exynos/exynos_drm_iommu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_iommu.c > b/drivers/gpu/drm/exynos/exynos_drm_iommu.c > index 09db198..3b3d3a6 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_iommu.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_iommu.c > @@ -56,7 +56,7 @@ int drm_create_iommu_mapping(struct drm_device *drm_dev) > mapping = arm_iommu_create_mapping(_bus_type, priv->da_start, > priv->da_space_size, > priv->da_space_order); > - if (!mapping) > + if (IS_ERR(mapping)) > return -ENOMEM; One more fix is needed here. - return -ENOMEM; + return PTR_ERR(mapping); > > dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), > > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Seung-Woo Kim Samsung Software R Center --
Re: [PATCH -next] drm/exynos/iommu: fix return value check in drm_create_iommu_mapping()
On 2012년 12월 07일 21:50, Wei Yongjun wrote: From: Wei Yongjun yongjun_...@trendmicro.com.cn In case of error, function arm_iommu_create_mapping() returns ERR_PTR() and never returns NULL. The NULL test in the return value check should be replaced with IS_ERR(). Signed-off-by: Wei Yongjun yongjun_...@trendmicro.com.cn --- drivers/gpu/drm/exynos/exynos_drm_iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_iommu.c b/drivers/gpu/drm/exynos/exynos_drm_iommu.c index 09db198..3b3d3a6 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_iommu.c +++ b/drivers/gpu/drm/exynos/exynos_drm_iommu.c @@ -56,7 +56,7 @@ int drm_create_iommu_mapping(struct drm_device *drm_dev) mapping = arm_iommu_create_mapping(platform_bus_type, priv-da_start, priv-da_space_size, priv-da_space_order); - if (!mapping) + if (IS_ERR(mapping)) return -ENOMEM; One more fix is needed here. - return -ENOMEM; + return PTR_ERR(mapping); dev-dma_parms = devm_kzalloc(dev, sizeof(*dev-dma_parms), ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: exynos: hdmi: sending AVI and AUI info frames
On 2012? 11? 23? 18:17, Rahul Sharma wrote: > This patch adds code for composing AVI and AUI info frames > and send them every VSYNC. > > This patch is important for hdmi certification. > > Signed-off-by: Rahul Sharma > Signed-off-by: Fahad Kunnathadi > Signed-off-by: Shirish S Acked-by : Seung-Woo Kim > > Based on exynos-drm-next branch of > http://git.kernel.org/?p=linux/kernel/git/daeinki/drm-exynos.git > --- > drivers/gpu/drm/exynos/exynos_hdmi.c | 139 > +- > drivers/gpu/drm/exynos/exynos_hdmi.h | 23 ++ > drivers/gpu/drm/exynos/regs-hdmi.h | 17 - > 3 files changed, 158 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c > b/drivers/gpu/drm/exynos/exynos_hdmi.c > index 59839cc..49332bd 100644 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c > @@ -183,6 +183,7 @@ struct hdmi_v13_conf { > int height; > int vrefresh; > bool interlace; > + int cea_video_id; > const u8 *hdmiphy_data; > const struct hdmi_v13_preset_conf *conf; > }; > @@ -354,15 +355,20 @@ static const struct hdmi_v13_preset_conf > hdmi_v13_conf_1080p60 = { > }; > > static const struct hdmi_v13_conf hdmi_v13_confs[] = { > - { 1280, 720, 60, false, hdmiphy_v13_conf74_25, _v13_conf_720p60 }, > - { 1280, 720, 50, false, hdmiphy_v13_conf74_25, _v13_conf_720p60 }, > - { 720, 480, 60, false, hdmiphy_v13_conf27_027, _v13_conf_480p }, > - { 1920, 1080, 50, true, hdmiphy_v13_conf74_25, _v13_conf_1080i50 }, > - { 1920, 1080, 50, false, hdmiphy_v13_conf148_5, > - _v13_conf_1080p50 }, > - { 1920, 1080, 60, true, hdmiphy_v13_conf74_25, _v13_conf_1080i60 }, > - { 1920, 1080, 60, false, hdmiphy_v13_conf148_5, > - _v13_conf_1080p60 }, > + { 1280, 720, 60, false, 4, hdmiphy_v13_conf74_25, > + _v13_conf_720p60 }, > + { 1280, 720, 50, false, 19, hdmiphy_v13_conf74_25, > + _v13_conf_720p60 }, > + { 720, 480, 60, false, 3, hdmiphy_v13_conf27_027, > + _v13_conf_480p }, > + { 1920, 1080, 50, true, 20, hdmiphy_v13_conf74_25, > + _v13_conf_1080i50 }, > + { 1920, 1080, 50, false, 31, hdmiphy_v13_conf148_5, > + _v13_conf_1080p50 }, > + { 1920, 1080, 60, true, 5, hdmiphy_v13_conf74_25, > + _v13_conf_1080i60 }, > + { 1920, 1080, 60, false, 16, hdmiphy_v13_conf148_5, > + _v13_conf_1080p60 }, > }; > > /* HDMI Version 1.4 */ > @@ -480,6 +486,7 @@ struct hdmi_conf { > int height; > int vrefresh; > bool interlace; > + int cea_video_id; > const u8 *hdmiphy_data; > const struct hdmi_preset_conf *conf; > }; > @@ -935,16 +942,21 @@ static const struct hdmi_preset_conf hdmi_conf_1080p60 > = { > }; > > static const struct hdmi_conf hdmi_confs[] = { > - { 720, 480, 60, false, hdmiphy_conf27_027, _conf_480p60 }, > - { 1280, 720, 50, false, hdmiphy_conf74_25, _conf_720p50 }, > - { 1280, 720, 60, false, hdmiphy_conf74_25, _conf_720p60 }, > - { 1920, 1080, 50, true, hdmiphy_conf74_25, _conf_1080i50 }, > - { 1920, 1080, 60, true, hdmiphy_conf74_25, _conf_1080i60 }, > - { 1920, 1080, 30, false, hdmiphy_conf74_176, _conf_1080p30 }, > - { 1920, 1080, 50, false, hdmiphy_conf148_5, _conf_1080p50 }, > - { 1920, 1080, 60, false, hdmiphy_conf148_5, _conf_1080p60 }, > + { 720, 480, 60, false, 3, hdmiphy_conf27_027, _conf_480p60 }, > + { 1280, 720, 50, false, 19, hdmiphy_conf74_25, _conf_720p50 }, > + { 1280, 720, 60, false, 4, hdmiphy_conf74_25, _conf_720p60 }, > + { 1920, 1080, 50, true, 20, hdmiphy_conf74_25, _conf_1080i50 }, > + { 1920, 1080, 60, true, 5, hdmiphy_conf74_25, _conf_1080i60 }, > + { 1920, 1080, 30, false, 34, hdmiphy_conf74_176, _conf_1080p30 }, > + { 1920, 1080, 50, false, 31, hdmiphy_conf148_5, _conf_1080p50 }, > + { 1920, 1080, 60, false, 16, hdmiphy_conf148_5, _conf_1080p60 }, > }; > > +struct hdmi_infoframe { > + enum HDMI_PACKET_TYPE type; > + u8 ver; > + u8 len; > +}; > > static inline u32 hdmi_reg_read(struct hdmi_context *hdata, u32 reg_id) > { > @@ -1268,6 +1280,85 @@ static int hdmi_conf_index(struct hdmi_context *hdata, > return hdmi_v14_conf_index(mode); > } > > +static u8 hdmi_chksum(struct hdmi_context *hdata, > + u32 start, u8 len, u32 hdr_sum) > +{ > + int i; > + /* hdr_sum : header0 + header1 + header2 > + * start : start address of packet byte1 > + * len : packet bytes - 1 */ > + for (i = 0; i < len; ++i) > + hdr_sum += 0xff & hdmi_reg_read(hdata, start + i * 4); > + > + return (u8)(0x100 - (hdr_sum & 0xff)); > +} > + > +void hdmi_reg_infoframe(struct hdmi_context *hdata, > + struct hdmi_infoframe *infoframe) > +{ > + u32
[PATCH] drm: exynos: hdmi: sending AVI and AUI info frames
Hi Rahul, I think this patch is almost ready just except few trivial check. On 2012? 11? 22? 23:12, Rahul Sharma wrote: > This patch adds code for composing AVI and AUI info frames > and send them every VSYNC. > > This patch is important for hdmi certification. > > Based on exynos-drm-fixes branch of > git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git This comment don't need to apply, so it's better to be moved to below line. > > Signed-off-by: Rahul Sharma > Signed-off-by: Fahad Kunnathadi > Signed-off-by: Shirish S > --- Here. > drivers/gpu/drm/exynos/exynos_hdmi.c | 142 > +- > drivers/gpu/drm/exynos/exynos_hdmi.h | 23 ++ > drivers/gpu/drm/exynos/regs-hdmi.h | 17 - > 3 files changed, 161 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c > b/drivers/gpu/drm/exynos/exynos_hdmi.c > index ee110c9..5ffedc3 100644 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c > @@ -185,6 +185,7 @@ struct hdmi_v13_conf { > int height; > int vrefresh; > bool interlace; > + int cea_video_id; > const u8 *hdmiphy_data; > const struct hdmi_v13_preset_conf *conf; > }; > @@ -356,15 +357,20 @@ static const struct hdmi_v13_preset_conf > hdmi_v13_conf_1080p60 = { > }; > > static const struct hdmi_v13_conf hdmi_v13_confs[] = { > - { 1280, 720, 60, false, hdmiphy_v13_conf74_25, _v13_conf_720p60 }, > - { 1280, 720, 50, false, hdmiphy_v13_conf74_25, _v13_conf_720p60 }, > - { 720, 480, 60, false, hdmiphy_v13_conf27_027, _v13_conf_480p }, > - { 1920, 1080, 50, true, hdmiphy_v13_conf74_25, _v13_conf_1080i50 }, > - { 1920, 1080, 50, false, hdmiphy_v13_conf148_5, > - _v13_conf_1080p50 }, > - { 1920, 1080, 60, true, hdmiphy_v13_conf74_25, _v13_conf_1080i60 }, > - { 1920, 1080, 60, false, hdmiphy_v13_conf148_5, > - _v13_conf_1080p60 }, > + { 1280, 720, 60, false, 4, hdmiphy_v13_conf74_25, > + _v13_conf_720p60 }, > + { 1280, 720, 50, false, 19, hdmiphy_v13_conf74_25, > + _v13_conf_720p60 }, > + { 720, 480, 60, false, 3, hdmiphy_v13_conf27_027, > + _v13_conf_480p }, > + { 1920, 1080, 50, true, 20, hdmiphy_v13_conf74_25, > + _v13_conf_1080i50 }, > + { 1920, 1080, 50, false, 31, hdmiphy_v13_conf148_5, > + _v13_conf_1080p50 }, > + { 1920, 1080, 60, true, 5, hdmiphy_v13_conf74_25, > + _v13_conf_1080i60 }, > + { 1920, 1080, 60, false, 16, hdmiphy_v13_conf148_5, > + _v13_conf_1080p60 }, > }; > > /* HDMI Version 1.4 */ > @@ -482,6 +488,7 @@ struct hdmi_conf { > int height; > int vrefresh; > bool interlace; > + int cea_video_id; > const u8 *hdmiphy_data; > const struct hdmi_preset_conf *conf; > }; > @@ -937,16 +944,21 @@ static const struct hdmi_preset_conf hdmi_conf_1080p60 > = { > }; > > static const struct hdmi_conf hdmi_confs[] = { > - { 720, 480, 60, false, hdmiphy_conf27_027, _conf_480p60 }, > - { 1280, 720, 50, false, hdmiphy_conf74_25, _conf_720p50 }, > - { 1280, 720, 60, false, hdmiphy_conf74_25, _conf_720p60 }, > - { 1920, 1080, 50, true, hdmiphy_conf74_25, _conf_1080i50 }, > - { 1920, 1080, 60, true, hdmiphy_conf74_25, _conf_1080i60 }, > - { 1920, 1080, 30, false, hdmiphy_conf74_176, _conf_1080p30 }, > - { 1920, 1080, 50, false, hdmiphy_conf148_5, _conf_1080p50 }, > - { 1920, 1080, 60, false, hdmiphy_conf148_5, _conf_1080p60 }, > + { 720, 480, 60, false, 3, hdmiphy_conf27_027, _conf_480p60 }, > + { 1280, 720, 50, false, 19, hdmiphy_conf74_25, _conf_720p50 }, > + { 1280, 720, 60, false, 4, hdmiphy_conf74_25, _conf_720p60 }, > + { 1920, 1080, 50, true, 20, hdmiphy_conf74_25, _conf_1080i50 }, > + { 1920, 1080, 60, true, 5, hdmiphy_conf74_25, _conf_1080i60 }, > + { 1920, 1080, 30, false, 34, hdmiphy_conf74_176, _conf_1080p30 }, > + { 1920, 1080, 50, false, 31, hdmiphy_conf148_5, _conf_1080p50 }, > + { 1920, 1080, 60, false, 16, hdmiphy_conf148_5, _conf_1080p60 }, > }; > > +struct hdmi_infoframe { > + enum HDMI_PACKET_TYPE type; > + u8 ver; > + u8 len; > +}; > > static inline u32 hdmi_reg_read(struct hdmi_context *hdata, u32 reg_id) > { > @@ -1270,6 +1282,88 @@ static int hdmi_conf_index(struct hdmi_context *hdata, > return hdmi_v14_conf_index(mode); > } > > +static u8 hdmi_chksum(struct hdmi_context *hdata, > + u32 start, u8 len, u32 hdr_sum) > +{ > + int i; > + /* hdr_sum : header0 + header1 + header2 > + * start : start address of packet byte1 > + * len : packet bytes - 1 */ > + for (i = 0; i < len; ++i) > + hdr_sum += 0xff & hdmi_reg_read(hdata, start + i * 4); > + > + return (u8)(0x100 - (hdr_sum & 0xff)); >
Re: [PATCH] drm: exynos: hdmi: sending AVI and AUI info frames
On 2012년 11월 23일 18:17, Rahul Sharma wrote: This patch adds code for composing AVI and AUI info frames and send them every VSYNC. This patch is important for hdmi certification. Signed-off-by: Rahul Sharma rahul.sha...@samsung.com Signed-off-by: Fahad Kunnathadi faha...@samsung.com Signed-off-by: Shirish S s.shir...@samsung.com Acked-by : Seung-Woo Kim sw0312@samsung.com Based on exynos-drm-next branch of http://git.kernel.org/?p=linux/kernel/git/daeinki/drm-exynos.git --- drivers/gpu/drm/exynos/exynos_hdmi.c | 139 +- drivers/gpu/drm/exynos/exynos_hdmi.h | 23 ++ drivers/gpu/drm/exynos/regs-hdmi.h | 17 - 3 files changed, 158 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 59839cc..49332bd 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -183,6 +183,7 @@ struct hdmi_v13_conf { int height; int vrefresh; bool interlace; + int cea_video_id; const u8 *hdmiphy_data; const struct hdmi_v13_preset_conf *conf; }; @@ -354,15 +355,20 @@ static const struct hdmi_v13_preset_conf hdmi_v13_conf_1080p60 = { }; static const struct hdmi_v13_conf hdmi_v13_confs[] = { - { 1280, 720, 60, false, hdmiphy_v13_conf74_25, hdmi_v13_conf_720p60 }, - { 1280, 720, 50, false, hdmiphy_v13_conf74_25, hdmi_v13_conf_720p60 }, - { 720, 480, 60, false, hdmiphy_v13_conf27_027, hdmi_v13_conf_480p }, - { 1920, 1080, 50, true, hdmiphy_v13_conf74_25, hdmi_v13_conf_1080i50 }, - { 1920, 1080, 50, false, hdmiphy_v13_conf148_5, - hdmi_v13_conf_1080p50 }, - { 1920, 1080, 60, true, hdmiphy_v13_conf74_25, hdmi_v13_conf_1080i60 }, - { 1920, 1080, 60, false, hdmiphy_v13_conf148_5, - hdmi_v13_conf_1080p60 }, + { 1280, 720, 60, false, 4, hdmiphy_v13_conf74_25, + hdmi_v13_conf_720p60 }, + { 1280, 720, 50, false, 19, hdmiphy_v13_conf74_25, + hdmi_v13_conf_720p60 }, + { 720, 480, 60, false, 3, hdmiphy_v13_conf27_027, + hdmi_v13_conf_480p }, + { 1920, 1080, 50, true, 20, hdmiphy_v13_conf74_25, + hdmi_v13_conf_1080i50 }, + { 1920, 1080, 50, false, 31, hdmiphy_v13_conf148_5, + hdmi_v13_conf_1080p50 }, + { 1920, 1080, 60, true, 5, hdmiphy_v13_conf74_25, + hdmi_v13_conf_1080i60 }, + { 1920, 1080, 60, false, 16, hdmiphy_v13_conf148_5, + hdmi_v13_conf_1080p60 }, }; /* HDMI Version 1.4 */ @@ -480,6 +486,7 @@ struct hdmi_conf { int height; int vrefresh; bool interlace; + int cea_video_id; const u8 *hdmiphy_data; const struct hdmi_preset_conf *conf; }; @@ -935,16 +942,21 @@ static const struct hdmi_preset_conf hdmi_conf_1080p60 = { }; static const struct hdmi_conf hdmi_confs[] = { - { 720, 480, 60, false, hdmiphy_conf27_027, hdmi_conf_480p60 }, - { 1280, 720, 50, false, hdmiphy_conf74_25, hdmi_conf_720p50 }, - { 1280, 720, 60, false, hdmiphy_conf74_25, hdmi_conf_720p60 }, - { 1920, 1080, 50, true, hdmiphy_conf74_25, hdmi_conf_1080i50 }, - { 1920, 1080, 60, true, hdmiphy_conf74_25, hdmi_conf_1080i60 }, - { 1920, 1080, 30, false, hdmiphy_conf74_176, hdmi_conf_1080p30 }, - { 1920, 1080, 50, false, hdmiphy_conf148_5, hdmi_conf_1080p50 }, - { 1920, 1080, 60, false, hdmiphy_conf148_5, hdmi_conf_1080p60 }, + { 720, 480, 60, false, 3, hdmiphy_conf27_027, hdmi_conf_480p60 }, + { 1280, 720, 50, false, 19, hdmiphy_conf74_25, hdmi_conf_720p50 }, + { 1280, 720, 60, false, 4, hdmiphy_conf74_25, hdmi_conf_720p60 }, + { 1920, 1080, 50, true, 20, hdmiphy_conf74_25, hdmi_conf_1080i50 }, + { 1920, 1080, 60, true, 5, hdmiphy_conf74_25, hdmi_conf_1080i60 }, + { 1920, 1080, 30, false, 34, hdmiphy_conf74_176, hdmi_conf_1080p30 }, + { 1920, 1080, 50, false, 31, hdmiphy_conf148_5, hdmi_conf_1080p50 }, + { 1920, 1080, 60, false, 16, hdmiphy_conf148_5, hdmi_conf_1080p60 }, }; +struct hdmi_infoframe { + enum HDMI_PACKET_TYPE type; + u8 ver; + u8 len; +}; static inline u32 hdmi_reg_read(struct hdmi_context *hdata, u32 reg_id) { @@ -1268,6 +1280,85 @@ static int hdmi_conf_index(struct hdmi_context *hdata, return hdmi_v14_conf_index(mode); } +static u8 hdmi_chksum(struct hdmi_context *hdata, + u32 start, u8 len, u32 hdr_sum) +{ + int i; + /* hdr_sum : header0 + header1 + header2 + * start : start address of packet byte1 + * len : packet bytes - 1 */ + for (i = 0; i len; ++i) + hdr_sum += 0xff hdmi_reg_read(hdata, start + i * 4); + + return (u8)(0x100 - (hdr_sum 0xff)); +} + +void hdmi_reg_infoframe(struct hdmi_context
[PATCH 2/2] drm: exynos: compose and send avi and aui info frames
On 2012? 11? 21? 20:36, Rahul Sharma wrote: > Hi Seung Woo, > > Thanks for your inputs. Please find my response below. > > On Wed, Nov 21, 2012 at 2:12 PM, ??? wrote: >> Hi Rahul, >> >> Control part seems good, and my comment is below. >> >> On 2012? 11? 10? 01:21, Rahul Sharma wrote: >>> This patch adds code for composing AVI and AUI info frames >>> and send them every VSYNC. >>> >>> This patch is important for hdmi certification. >>> >>> Signed-off-by: Fahad Kunnathadi >>> Signed-off-by: Shirish S >>> Signed-off-by: Rahul Sharma >>> --- >>> drivers/gpu/drm/exynos/exynos_hdmi.c | 97 >>> +- >>> drivers/gpu/drm/exynos/exynos_hdmi.h | 23 >>> drivers/gpu/drm/exynos/regs-hdmi.h | 17 +- >>> 3 files changed, 133 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c >>> b/drivers/gpu/drm/exynos/exynos_hdmi.c >>> index 2c115f8..bb8a045 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c >>> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c >> >> >> >>> @@ -1993,6 +2084,8 @@ static void hdmi_mode_set(void *ctx, void *mode) >>> >>> DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__); >>> >>> + hdata->cur_video_id = drm_match_cea_mode(mode); >>> + >> >> How do you think about using predefined cea video id in struct >> hdmi_conf? drm_mode does not have cea video id, so drm_match_cea_mode() >> compares only mode information. Considering this, IMHO, cea video id can >> be embedded in struct hdmi_conf. >> > > I feel, It will leads to duplication of video id information. In > edid_cea_modes, modes are > strictly arranged in the order of respective cea video ID codes. > "drm_add_edid_modes" > also passes the cea codes (recieved after edid data parsing) as the index to > edid_cea_modes to get mode details. It might be a concern related with your first patch, anyway edid_cea_modes has few pair of exact same modes because struct drm_mode does not have picture ratio. For example, video id 2 and 3 have exact same values for struct drm_mode. So cea video id can be used to get a mode, but a drm_mode is not sufficient to get exact video id. Considering that exynos hdmi does not support video ids with same mode, I suggested video id in struct hdmi_conf. At the point of exynos drm, I can ack this patch. > > Secondly, mode to cea code translation is required by all platforms > for AVI packet > composition. By adding it to hdmi_conf, we are limiting its usage for exynos. I agree with you at this point. I quickly checked i915 and radeon and I found that they use fixed value for avi packet at sw level, but I don't have information hw can properly build avi packet. If they also need video id for building avi packet, video id translation can be used. Best Regards, - Seung-Woo Kim > > regards, > Rahul Sharma. > >>> conf_idx = hdmi_conf_index(hdata, mode); >>> if (conf_idx >= 0) >>> hdata->cur_conf = conf_idx; >> >> >> >> Thanks and Regards, >> - Seung-Woo Kim >> >> -- >> Seung-Woo Kim >> Samsung Software R Center >> -- >> >> ___ >> dri-devel mailing list >> dri-devel at lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel >
[RFC 2/4] drm/exynos: add fimc ipp driver
Hi Eunchul, IMHO, each function for source and destination has quite similar routine and it seems that there are some redundant code. I'm not sure these duplicated code can be removed with mergeing similar part. Some comments are below. On 2012? 10? 29? 22:10, Eunchul Kim wrote: > FIMC is stand for Fully Interfactive Mobile Camera and > supports image scaler/rotator/crop/flip/csc and input/output DMA operations. > input DMA reads image data from the memory. > output DMA writes image data to memory. > FIMC supports image rotation and image effect functions. > also supports writeback and display output operations. > > Signed-off-by: Eunchul Kim > Signed-off-by: Jinyoung Jeon > --- > drivers/gpu/drm/exynos/Kconfig |6 + > drivers/gpu/drm/exynos/Makefile |1 + > drivers/gpu/drm/exynos/exynos_drm_drv.c | 15 + > drivers/gpu/drm/exynos/exynos_drm_drv.h |1 + > drivers/gpu/drm/exynos/exynos_drm_fimc.c | 2041 > ++ > drivers/gpu/drm/exynos/exynos_drm_fimc.h | 35 + > drivers/gpu/drm/exynos/regs-fimc.h | 669 ++ > include/drm/exynos_drm.h | 33 + > 8 files changed, 2801 insertions(+), 0 deletions(-) > create mode 100644 drivers/gpu/drm/exynos/exynos_drm_fimc.c > create mode 100644 drivers/gpu/drm/exynos/exynos_drm_fimc.h > create mode 100644 drivers/gpu/drm/exynos/regs-fimc.h > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c > b/drivers/gpu/drm/exynos/exynos_drm_fimc.c > new file mode 100644 > index 000..3adb452 > --- /dev/null > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c > @@ -0,0 +1,2041 @@ > +/* > + * Copyright (C) 2012 Samsung Electronics Co.Ltd > + * Authors: > + * Eunchul Kim > + * Jinyoung Jeon > + * Sangmin Lee > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + * > + */ > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include "regs-fimc.h" > +#include "exynos_drm_drv.h" > +#include "exynos_drm_gem.h" > +#include "exynos_drm_ipp.h" > +#include "exynos_drm_fimc.h" > + > +/* > + * FIMC is stand for Fully Interactive Mobile Camera and > + * supports image scaler/rotator and input/output DMA operations. > + * input DMA reads image data from the memory. > + * output DMA writes image data to memory. > + * FIMC supports image rotation and image effect functions. > + */ > + > +#define FIMC_MAX_DEVS4 > +#define FIMC_MAX_SRC 2 > +#define FIMC_MAX_DST 32 > +#define FIMC_CLK_RATE16675 > +#define FIMC_CLK_RATE_R2 18000 > +#define FIMC_BUF_STOP1 > +#define FIMC_BUF_START 2 > +#define FIMC_REG_SZ 32 > +#define FIMC_WIDTH_ITU_709 1280 > + > +#define get_fimc_context(dev) > platform_get_drvdata(to_platform_device(dev)) > +#define get_ctx_from_ippdrv(ippdrv) container_of(ippdrv,\ > + struct fimc_context, ippdrv); > +#define fimc_read(offset)readl(ctx->regs + (offset)); > +#define fimc_write(cfg, offset) writel(cfg, ctx->regs + (offset)); > + > +enum fimc_wb { > + FIMC_WB_NONE, > + FIMC_WB_A, > + FIMC_WB_B, > +}; > + > +/* > + * A structure of scaler. > + * > + * @range: narrow, wide. > + * @bypass: unused scaler path. > + * @up_h: horizontal scale up. > + * @up_v: vertical scale up. > + * @hratio: horizontal ratio. > + * @vratio: vertical ratio. > + */ > +struct fimc_scaler { > + boolrange; > + bool bypass; > + bool up_h; > + bool up_v; > + u32 hratio; > + u32 vratio; > +}; > + > +/* > + * A structure of scaler capability. > + * > + * find user manual table 43-1. > + * @in_hori: scaler input horizontal size. > + * @bypass: scaler bypass mode. > + * @dst_h_wo_rot: target horizontal size without output rotation. > + * @dst_h_rot: target horizontal size with output rotation. > + * @rl_w_wo_rot: real width without input rotation. > + * @rl_h_rot: real height without output rotation. > + */ > +struct fimc_capability { > + /* scaler */ > + u32 in_hori; > + u32 bypass; > + /* output rotator */ > + u32 dst_h_wo_rot; > + u32 dst_h_rot; > + /* input rotator */ > + u32 rl_w_wo_rot; > + u32 rl_h_rot; > +}; > + > +/* > + * A structure of fimc context. > + * > + * @ippdrv: prepare initialization using ippdrv. > + * @regs_res: register resources. > + * @regs: memory mapped io registers. > + * @lock: locking of operations. > + * @sclk_fimc_clk: fimc source clock. > + * @fimc_clk: fimc clock. > + * @wb_clk: writeback a clock. > + * @wb_b_clk: writeback b clock. > + * @sc: scaler infomations. > + * @capa: scaler capability. > + * @odr: ordering of YUV. > + * @ver: fimc version. > + * @pol:
Re: [PATCH] drm: exynos: hdmi: sending AVI and AUI info frames
Hi Rahul, I think this patch is almost ready just except few trivial check. On 2012년 11월 22일 23:12, Rahul Sharma wrote: This patch adds code for composing AVI and AUI info frames and send them every VSYNC. This patch is important for hdmi certification. Based on exynos-drm-fixes branch of git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git This comment don't need to apply, so it's better to be moved to below line. Signed-off-by: Rahul Sharma rahul.sha...@samsung.com Signed-off-by: Fahad Kunnathadi faha...@samsung.com Signed-off-by: Shirish S s.shir...@samsung.com --- Here. drivers/gpu/drm/exynos/exynos_hdmi.c | 142 +- drivers/gpu/drm/exynos/exynos_hdmi.h | 23 ++ drivers/gpu/drm/exynos/regs-hdmi.h | 17 - 3 files changed, 161 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index ee110c9..5ffedc3 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -185,6 +185,7 @@ struct hdmi_v13_conf { int height; int vrefresh; bool interlace; + int cea_video_id; const u8 *hdmiphy_data; const struct hdmi_v13_preset_conf *conf; }; @@ -356,15 +357,20 @@ static const struct hdmi_v13_preset_conf hdmi_v13_conf_1080p60 = { }; static const struct hdmi_v13_conf hdmi_v13_confs[] = { - { 1280, 720, 60, false, hdmiphy_v13_conf74_25, hdmi_v13_conf_720p60 }, - { 1280, 720, 50, false, hdmiphy_v13_conf74_25, hdmi_v13_conf_720p60 }, - { 720, 480, 60, false, hdmiphy_v13_conf27_027, hdmi_v13_conf_480p }, - { 1920, 1080, 50, true, hdmiphy_v13_conf74_25, hdmi_v13_conf_1080i50 }, - { 1920, 1080, 50, false, hdmiphy_v13_conf148_5, - hdmi_v13_conf_1080p50 }, - { 1920, 1080, 60, true, hdmiphy_v13_conf74_25, hdmi_v13_conf_1080i60 }, - { 1920, 1080, 60, false, hdmiphy_v13_conf148_5, - hdmi_v13_conf_1080p60 }, + { 1280, 720, 60, false, 4, hdmiphy_v13_conf74_25, + hdmi_v13_conf_720p60 }, + { 1280, 720, 50, false, 19, hdmiphy_v13_conf74_25, + hdmi_v13_conf_720p60 }, + { 720, 480, 60, false, 3, hdmiphy_v13_conf27_027, + hdmi_v13_conf_480p }, + { 1920, 1080, 50, true, 20, hdmiphy_v13_conf74_25, + hdmi_v13_conf_1080i50 }, + { 1920, 1080, 50, false, 31, hdmiphy_v13_conf148_5, + hdmi_v13_conf_1080p50 }, + { 1920, 1080, 60, true, 5, hdmiphy_v13_conf74_25, + hdmi_v13_conf_1080i60 }, + { 1920, 1080, 60, false, 16, hdmiphy_v13_conf148_5, + hdmi_v13_conf_1080p60 }, }; /* HDMI Version 1.4 */ @@ -482,6 +488,7 @@ struct hdmi_conf { int height; int vrefresh; bool interlace; + int cea_video_id; const u8 *hdmiphy_data; const struct hdmi_preset_conf *conf; }; @@ -937,16 +944,21 @@ static const struct hdmi_preset_conf hdmi_conf_1080p60 = { }; static const struct hdmi_conf hdmi_confs[] = { - { 720, 480, 60, false, hdmiphy_conf27_027, hdmi_conf_480p60 }, - { 1280, 720, 50, false, hdmiphy_conf74_25, hdmi_conf_720p50 }, - { 1280, 720, 60, false, hdmiphy_conf74_25, hdmi_conf_720p60 }, - { 1920, 1080, 50, true, hdmiphy_conf74_25, hdmi_conf_1080i50 }, - { 1920, 1080, 60, true, hdmiphy_conf74_25, hdmi_conf_1080i60 }, - { 1920, 1080, 30, false, hdmiphy_conf74_176, hdmi_conf_1080p30 }, - { 1920, 1080, 50, false, hdmiphy_conf148_5, hdmi_conf_1080p50 }, - { 1920, 1080, 60, false, hdmiphy_conf148_5, hdmi_conf_1080p60 }, + { 720, 480, 60, false, 3, hdmiphy_conf27_027, hdmi_conf_480p60 }, + { 1280, 720, 50, false, 19, hdmiphy_conf74_25, hdmi_conf_720p50 }, + { 1280, 720, 60, false, 4, hdmiphy_conf74_25, hdmi_conf_720p60 }, + { 1920, 1080, 50, true, 20, hdmiphy_conf74_25, hdmi_conf_1080i50 }, + { 1920, 1080, 60, true, 5, hdmiphy_conf74_25, hdmi_conf_1080i60 }, + { 1920, 1080, 30, false, 34, hdmiphy_conf74_176, hdmi_conf_1080p30 }, + { 1920, 1080, 50, false, 31, hdmiphy_conf148_5, hdmi_conf_1080p50 }, + { 1920, 1080, 60, false, 16, hdmiphy_conf148_5, hdmi_conf_1080p60 }, }; +struct hdmi_infoframe { + enum HDMI_PACKET_TYPE type; + u8 ver; + u8 len; +}; static inline u32 hdmi_reg_read(struct hdmi_context *hdata, u32 reg_id) { @@ -1270,6 +1282,88 @@ static int hdmi_conf_index(struct hdmi_context *hdata, return hdmi_v14_conf_index(mode); } +static u8 hdmi_chksum(struct hdmi_context *hdata, + u32 start, u8 len, u32 hdr_sum) +{ + int i; + /* hdr_sum : header0 + header1 + header2 + * start : start address of packet byte1 + * len : packet bytes - 1 */ + for (i = 0; i len; ++i) + hdr_sum += 0xff hdmi_reg_read(hdata,
[PATCH 2/2] drm: exynos: compose and send avi and aui info frames
Hi Rahul, Control part seems good, and my comment is below. On 2012? 11? 10? 01:21, Rahul Sharma wrote: > This patch adds code for composing AVI and AUI info frames > and send them every VSYNC. > > This patch is important for hdmi certification. > > Signed-off-by: Fahad Kunnathadi > Signed-off-by: Shirish S > Signed-off-by: Rahul Sharma > --- > drivers/gpu/drm/exynos/exynos_hdmi.c | 97 > +- > drivers/gpu/drm/exynos/exynos_hdmi.h | 23 > drivers/gpu/drm/exynos/regs-hdmi.h | 17 +- > 3 files changed, 133 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c > b/drivers/gpu/drm/exynos/exynos_hdmi.c > index 2c115f8..bb8a045 100644 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c > @@ -1993,6 +2084,8 @@ static void hdmi_mode_set(void *ctx, void *mode) > > DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__); > > + hdata->cur_video_id = drm_match_cea_mode(mode); > + How do you think about using predefined cea video id in struct hdmi_conf? drm_mode does not have cea video id, so drm_match_cea_mode() compares only mode information. Considering this, IMHO, cea video id can be embedded in struct hdmi_conf. > conf_idx = hdmi_conf_index(hdata, mode); > if (conf_idx >= 0) > hdata->cur_conf = conf_idx; Thanks and Regards, - Seung-Woo Kim -- Seung-Woo Kim Samsung Software R Center --
[RESEND][PATCH] drm/prime: drop reference on imported dma-buf come from gem
On 2012? 11? 20? 19:26, Maarten Lankhorst wrote: > Op 20-11-12 02:03, ??? schreef: >> Hi Maarten, >> >> On 2012? 11? 19? 19:27, Maarten Lankhorst wrote: >>> Op 15-11-12 04:52, Seung-Woo Kim schreef: Increasing ref counts of both dma-buf and gem for imported dma-buf come from gem makes memory leak. release function of dma-buf cannot be called because f_count of dma-buf increased by importing gem and gem ref count cannot be decrease because of exported dma-buf. So I add dma_buf_put() for imported gem come from its own gem into each drivers having prime_import and prime_export capabilities. With this, only gem ref count is increased if importing gem exported from gem of same driver. Signed-off-by: Seung-Woo Kim Signed-off-by: Kyungmin.park Cc: Inki Dae Cc: Daniel Vetter Cc: Rob Clark Cc: Alex Deucher --- Mmap failiure in i915 was reported by Jani, and I think it was fixed with Dave's commit "drm/prime: add exported buffers to current fprivs imported buffer list (v2)", so I resend this patch. I can send exynos only, but this issue is common in all drm driver having both prime inport and export, IMHO, it's better to send for all drivers. >>> I fear that this won't solve the issue completely and keeps open a small >>> race. >> I don't believe my patch can fix all issue related with gem prime >> completely. But it seems that it can solve unrecoverable memory leak >> caused by re-importing GEM. Anyway, can you give me an example of other >> race issue? > When the dma_buf is unreffed and refcount drops to 0, work is queued to free > it. > > Until then export_dma_buf member points to something, but refcount is 0 on it. > > Importing to itself, then closing fd then re-export from the new place would > probably trigger it. > Ok, I understood about issue from delayed fput also in your below comment. Anyway, IMO, it is already on current drm prime. >>> I don't like the current destruction path either. It really looks like >>> export_dma_buf >>> should be unset when the exported buffer is closed in the original file. >>> Anything else is racy. >>> Especially setting export_dma_buf to NULL won't work with the current >>> delayed fput semantics. >> I cannot figure out all aspect of delayed fput, but until delayed work >> is called, dma_buf is not released so export_dma_buf is valid. >> Considering this, I can't find possible issue of delayed fput. > I'm fairly sure that when refcount drops to 0, even though the memory may not > be freed yet, > you no longer have a guarantee that the memory is still valid. I missed that delayed fput is triggered after recount drops to 0, and now I understood it can cause invalid access. > > And of course after importing into a different process with its own drm > namespace, how does > file_priv->prime.lock still protect serialization? > > I don't see any hierarchy either. export_dma_buf needs to be set to NULL > before the dma_buf_put > is called that is used for the reference inside export_dma_buf. > > The release function should only release a reference to whatever backing > memory is used. > >>> So to me it seems instead we should do something like this: >>> >>> - dmabuf_release callback is a noop, no ref is held by the dma-buf. >>> - attach and detach ops increase reference by 1*. >>> >>> - when the buffer is exported, export_dma_buf is set by core code and kept >>> around >>> alive until the gem object is closed. >>> >>> - dma_buf_put is not called by import function. This reference removed as >>> part >>> of gem object close instead. >> Considering re-import, where drm file priv is different from exporter's >> file priv, attach and detach are not called because gem object is reused. >> >> How do you think remove checking whether dma_buf is came from driver own >> exporter? Because without this checking, gem object is newly created, >> and then re-import issue will disappear. > The whole refcounting is a circular mess, sadly with no easy solution. :/ > > It seems to me that using gem reference counts is solving this problem at the > wrong layer. > A secondary type of reference count is needed for the underlying memory > backing. > > For those who use ttm, I would recommend that the dma_buf increases refcount > on ttm_bo by one, > so that the gem object can be destroyed and release its reference on the > dma_buf. > > WIthout a secondary refcount you end up in a position where you can't > reliably and race free unref > the gem object and dma_buf at the same time, since they're both independent > interfaces with possibly > different lifetimes. If there is no checking whether dma_buf is came from driver own exporter, gem_object is newly allocated and refcount of it can be a secondary refcount at least form mermoy leak issue. So I mentioned about removing check for exporter. But I prefer processing re-import as gem_open without
Re: [PATCH 2/2] drm: exynos: compose and send avi and aui info frames
Hi Rahul, Control part seems good, and my comment is below. On 2012년 11월 10일 01:21, Rahul Sharma wrote: This patch adds code for composing AVI and AUI info frames and send them every VSYNC. This patch is important for hdmi certification. Signed-off-by: Fahad Kunnathadi faha...@samsung.com Signed-off-by: Shirish S s.shir...@samsung.com Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- drivers/gpu/drm/exynos/exynos_hdmi.c | 97 +- drivers/gpu/drm/exynos/exynos_hdmi.h | 23 drivers/gpu/drm/exynos/regs-hdmi.h | 17 +- 3 files changed, 133 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 2c115f8..bb8a045 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c snip @@ -1993,6 +2084,8 @@ static void hdmi_mode_set(void *ctx, void *mode) DRM_DEBUG_KMS([%d] %s\n, __LINE__, __func__); + hdata-cur_video_id = drm_match_cea_mode(mode); + How do you think about using predefined cea video id in struct hdmi_conf? drm_mode does not have cea video id, so drm_match_cea_mode() compares only mode information. Considering this, IMHO, cea video id can be embedded in struct hdmi_conf. conf_idx = hdmi_conf_index(hdata, mode); if (conf_idx = 0) hdata-cur_conf = conf_idx; snip Thanks and Regards, - Seung-Woo Kim -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm: exynos: compose and send avi and aui info frames
On 2012년 11월 21일 20:36, Rahul Sharma wrote: Hi Seung Woo, Thanks for your inputs. Please find my response below. On Wed, Nov 21, 2012 at 2:12 PM, 김승우 sw0312@samsung.com wrote: Hi Rahul, Control part seems good, and my comment is below. On 2012년 11월 10일 01:21, Rahul Sharma wrote: This patch adds code for composing AVI and AUI info frames and send them every VSYNC. This patch is important for hdmi certification. Signed-off-by: Fahad Kunnathadi faha...@samsung.com Signed-off-by: Shirish S s.shir...@samsung.com Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- drivers/gpu/drm/exynos/exynos_hdmi.c | 97 +- drivers/gpu/drm/exynos/exynos_hdmi.h | 23 drivers/gpu/drm/exynos/regs-hdmi.h | 17 +- 3 files changed, 133 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c index 2c115f8..bb8a045 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c snip @@ -1993,6 +2084,8 @@ static void hdmi_mode_set(void *ctx, void *mode) DRM_DEBUG_KMS([%d] %s\n, __LINE__, __func__); + hdata-cur_video_id = drm_match_cea_mode(mode); + How do you think about using predefined cea video id in struct hdmi_conf? drm_mode does not have cea video id, so drm_match_cea_mode() compares only mode information. Considering this, IMHO, cea video id can be embedded in struct hdmi_conf. I feel, It will leads to duplication of video id information. In edid_cea_modes, modes are strictly arranged in the order of respective cea video ID codes. drm_add_edid_modes also passes the cea codes (recieved after edid data parsing) as the index to edid_cea_modes to get mode details. It might be a concern related with your first patch, anyway edid_cea_modes has few pair of exact same modes because struct drm_mode does not have picture ratio. For example, video id 2 and 3 have exact same values for struct drm_mode. So cea video id can be used to get a mode, but a drm_mode is not sufficient to get exact video id. Considering that exynos hdmi does not support video ids with same mode, I suggested video id in struct hdmi_conf. At the point of exynos drm, I can ack this patch. Secondly, mode to cea code translation is required by all platforms for AVI packet composition. By adding it to hdmi_conf, we are limiting its usage for exynos. I agree with you at this point. I quickly checked i915 and radeon and I found that they use fixed value for avi packet at sw level, but I don't have information hw can properly build avi packet. If they also need video id for building avi packet, video id translation can be used. Best Regards, - Seung-Woo Kim regards, Rahul Sharma. conf_idx = hdmi_conf_index(hdata, mode); if (conf_idx = 0) hdata-cur_conf = conf_idx; snip Thanks and Regards, - Seung-Woo Kim -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RESEND][PATCH] drm/prime: drop reference on imported dma-buf come from gem
Hi Maarten, On 2012? 11? 19? 19:27, Maarten Lankhorst wrote: > Op 15-11-12 04:52, Seung-Woo Kim schreef: >> Increasing ref counts of both dma-buf and gem for imported dma-buf >> come from gem makes memory leak. release function of dma-buf cannot >> be called because f_count of dma-buf increased by importing gem and >> gem ref count cannot be decrease because of exported dma-buf. >> >> So I add dma_buf_put() for imported gem come from its own gem into >> each drivers having prime_import and prime_export capabilities. With >> this, only gem ref count is increased if importing gem exported from >> gem of same driver. >> >> Signed-off-by: Seung-Woo Kim >> Signed-off-by: Kyungmin.park >> Cc: Inki Dae >> Cc: Daniel Vetter >> Cc: Rob Clark >> Cc: Alex Deucher >> --- >> Mmap failiure in i915 was reported by Jani, and I think it was fixed >> with Dave's commit "drm/prime: add exported buffers to current fprivs >> imported buffer list (v2)", so I resend this patch. >> >> I can send exynos only, but this issue is common in all drm driver >> having both prime inport and export, IMHO, it's better to send for >> all drivers. > I fear that this won't solve the issue completely and keeps open a small > race. I don't believe my patch can fix all issue related with gem prime completely. But it seems that it can solve unrecoverable memory leak caused by re-importing GEM. Anyway, can you give me an example of other race issue? > > I don't like the current destruction path either. It really looks like > export_dma_buf > should be unset when the exported buffer is closed in the original file. > Anything else is racy. > Especially setting export_dma_buf to NULL won't work with the current delayed > fput semantics. I cannot figure out all aspect of delayed fput, but until delayed work is called, dma_buf is not released so export_dma_buf is valid. Considering this, I can't find possible issue of delayed fput. > > So to me it seems instead we should do something like this: > > - dmabuf_release callback is a noop, no ref is held by the dma-buf. > - attach and detach ops increase reference by 1*. > > - when the buffer is exported, export_dma_buf is set by core code and kept > around > alive until the gem object is closed. > > - dma_buf_put is not called by import function. This reference removed as part > of gem object close instead. Considering re-import, where drm file priv is different from exporter's file priv, attach and detach are not called because gem object is reused. How do you think remove checking whether dma_buf is came from driver own exporter? Because without this checking, gem object is newly created, and then re-import issue will disappear. Thanks and Regards, - Seung-Woo Kim > > ~Maarten > > *) Lockdep will rightfully hate this as it reopens a locking inversion, some > solution for that is needed. > > Maybe a post detach callback for dma-buf with all locks dropped would be best > here? > Other way would be to schedule delayed work with the sole purpose of > unreffing in the detach op, > similar to how atomic_dec_and_mutex_lock works. > > -- Seung-Woo Kim Samsung Software R Center --
Re: [RESEND][PATCH] drm/prime: drop reference on imported dma-buf come from gem
On 2012년 11월 20일 19:26, Maarten Lankhorst wrote: Op 20-11-12 02:03, 김승우 schreef: Hi Maarten, On 2012년 11월 19일 19:27, Maarten Lankhorst wrote: Op 15-11-12 04:52, Seung-Woo Kim schreef: Increasing ref counts of both dma-buf and gem for imported dma-buf come from gem makes memory leak. release function of dma-buf cannot be called because f_count of dma-buf increased by importing gem and gem ref count cannot be decrease because of exported dma-buf. So I add dma_buf_put() for imported gem come from its own gem into each drivers having prime_import and prime_export capabilities. With this, only gem ref count is increased if importing gem exported from gem of same driver. Signed-off-by: Seung-Woo Kim sw0312@samsung.com Signed-off-by: Kyungmin.park kyungmin.p...@samsung.com Cc: Inki Dae inki@samsung.com Cc: Daniel Vetter daniel.vet...@ffwll.ch Cc: Rob Clark rob.cl...@linaro.org Cc: Alex Deucher alexander.deuc...@amd.com --- Mmap failiure in i915 was reported by Jani, and I think it was fixed with Dave's commit drm/prime: add exported buffers to current fprivs imported buffer list (v2), so I resend this patch. I can send exynos only, but this issue is common in all drm driver having both prime inport and export, IMHO, it's better to send for all drivers. I fear that this won't solve the issue completely and keeps open a small race. I don't believe my patch can fix all issue related with gem prime completely. But it seems that it can solve unrecoverable memory leak caused by re-importing GEM. Anyway, can you give me an example of other race issue? When the dma_buf is unreffed and refcount drops to 0, work is queued to free it. Until then export_dma_buf member points to something, but refcount is 0 on it. Importing to itself, then closing fd then re-export from the new place would probably trigger it. Ok, I understood about issue from delayed fput also in your below comment. Anyway, IMO, it is already on current drm prime. I don't like the current destruction path either. It really looks like export_dma_buf should be unset when the exported buffer is closed in the original file. Anything else is racy. Especially setting export_dma_buf to NULL won't work with the current delayed fput semantics. I cannot figure out all aspect of delayed fput, but until delayed work is called, dma_buf is not released so export_dma_buf is valid. Considering this, I can't find possible issue of delayed fput. I'm fairly sure that when refcount drops to 0, even though the memory may not be freed yet, you no longer have a guarantee that the memory is still valid. I missed that delayed fput is triggered after recount drops to 0, and now I understood it can cause invalid access. And of course after importing into a different process with its own drm namespace, how does file_priv-prime.lock still protect serialization? I don't see any hierarchy either. export_dma_buf needs to be set to NULL before the dma_buf_put is called that is used for the reference inside export_dma_buf. The release function should only release a reference to whatever backing memory is used. So to me it seems instead we should do something like this: - dmabuf_release callback is a noop, no ref is held by the dma-buf. - attach and detach ops increase reference by 1*. - when the buffer is exported, export_dma_buf is set by core code and kept around alive until the gem object is closed. - dma_buf_put is not called by import function. This reference removed as part of gem object close instead. Considering re-import, where drm file priv is different from exporter's file priv, attach and detach are not called because gem object is reused. How do you think remove checking whether dma_buf is came from driver own exporter? Because without this checking, gem object is newly created, and then re-import issue will disappear. The whole refcounting is a circular mess, sadly with no easy solution. :/ It seems to me that using gem reference counts is solving this problem at the wrong layer. A secondary type of reference count is needed for the underlying memory backing. For those who use ttm, I would recommend that the dma_buf increases refcount on ttm_bo by one, so that the gem object can be destroyed and release its reference on the dma_buf. WIthout a secondary refcount you end up in a position where you can't reliably and race free unref the gem object and dma_buf at the same time, since they're both independent interfaces with possibly different lifetimes. If there is no checking whether dma_buf is came from driver own exporter, gem_object is newly allocated and refcount of it can be a secondary refcount at least form mermoy leak issue. So I mentioned about removing check for exporter. But I prefer processing re-import as gem_open without considering dma-buf as current implementation. It would really help if there were hard rules
Re: [RESEND][PATCH] drm/prime: drop reference on imported dma-buf come from gem
Hi Maarten, On 2012년 11월 19일 19:27, Maarten Lankhorst wrote: Op 15-11-12 04:52, Seung-Woo Kim schreef: Increasing ref counts of both dma-buf and gem for imported dma-buf come from gem makes memory leak. release function of dma-buf cannot be called because f_count of dma-buf increased by importing gem and gem ref count cannot be decrease because of exported dma-buf. So I add dma_buf_put() for imported gem come from its own gem into each drivers having prime_import and prime_export capabilities. With this, only gem ref count is increased if importing gem exported from gem of same driver. Signed-off-by: Seung-Woo Kim sw0312@samsung.com Signed-off-by: Kyungmin.park kyungmin.p...@samsung.com Cc: Inki Dae inki@samsung.com Cc: Daniel Vetter daniel.vet...@ffwll.ch Cc: Rob Clark rob.cl...@linaro.org Cc: Alex Deucher alexander.deuc...@amd.com --- Mmap failiure in i915 was reported by Jani, and I think it was fixed with Dave's commit drm/prime: add exported buffers to current fprivs imported buffer list (v2), so I resend this patch. I can send exynos only, but this issue is common in all drm driver having both prime inport and export, IMHO, it's better to send for all drivers. I fear that this won't solve the issue completely and keeps open a small race. I don't believe my patch can fix all issue related with gem prime completely. But it seems that it can solve unrecoverable memory leak caused by re-importing GEM. Anyway, can you give me an example of other race issue? I don't like the current destruction path either. It really looks like export_dma_buf should be unset when the exported buffer is closed in the original file. Anything else is racy. Especially setting export_dma_buf to NULL won't work with the current delayed fput semantics. I cannot figure out all aspect of delayed fput, but until delayed work is called, dma_buf is not released so export_dma_buf is valid. Considering this, I can't find possible issue of delayed fput. So to me it seems instead we should do something like this: - dmabuf_release callback is a noop, no ref is held by the dma-buf. - attach and detach ops increase reference by 1*. - when the buffer is exported, export_dma_buf is set by core code and kept around alive until the gem object is closed. - dma_buf_put is not called by import function. This reference removed as part of gem object close instead. Considering re-import, where drm file priv is different from exporter's file priv, attach and detach are not called because gem object is reused. How do you think remove checking whether dma_buf is came from driver own exporter? Because without this checking, gem object is newly created, and then re-import issue will disappear. Thanks and Regards, - Seung-Woo Kim ~Maarten *) Lockdep will rightfully hate this as it reopens a locking inversion, some solution for that is needed. Maybe a post detach callback for dma-buf with all locks dropped would be best here? Other way would be to schedule delayed work with the sole purpose of unreffing in the detach op, similar to how atomic_dec_and_mutex_lock works. -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/prime: drop reference on imported dma-buf come from gem
Hi Jani, Sorry for late reply. On 2012? 09? 27? 22:43, Jani Nikula wrote: > On Thu, 27 Sep 2012, Seung-Woo Kim wrote: >> Increasing ref counts of both dma-buf and gem for imported dma-buf come from >> gem >> makes memory leak. release function of dma-buf cannot be called because >> f_count >> of dma-buf increased by importing gem and gem ref count cannot be decrease >> because of exported dma-buf. >> >> So I add dma_buf_put() for imported gem come from its own gem into each >> drivers >> having prime_import and prime_export capabilities. With this, only gem ref >> count is increased if importing gem exported from gem of same driver. > > There's a reference leak bug [1] related to prime self import found by > intel-gpu-tools tests. Without much thinking, I just applied this patch > to see if it makes a difference. Instead, it now fails with: > > prime_self_import: prime_self_import.c:61: check_bo: Assertion `ptr1' > failed. > > The assert is at [2]. I think it was possible to re-use a handle imported_buf list which is already deleted, and it was fixed after Dave's commit "drm/prime: add exported buffers to current fprivs imported buffer list (v2)". > > I haven't looked into why this happens at all, so I'm just sharing this > in case you guys find it helpful. If you still have same assert, I think it will be very helpful to let me know exactly where the assert occurs from 5 check_bo() in test application in your link. Thanks and Best Regards, - Seung-Woo Kim > > BR, > Jani. > > > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=54111 > [2] > http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/tests/prime_self_import.c#n61 > > >> >> Signed-off-by: Seung-Woo Kim >> Signed-off-by: Kyungmin.park >> Cc: Inki Dae >> Cc: Daniel Vetter >> Cc: Rob Clark >> Cc: Alex Deucher >> --- >> drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |5 + >> drivers/gpu/drm/i915/i915_gem_dmabuf.c |5 + >> drivers/gpu/drm/nouveau/nouveau_prime.c|1 + >> drivers/gpu/drm/radeon/radeon_prime.c |1 + >> drivers/staging/omapdrm/omap_gem_dmabuf.c |5 + >> 5 files changed, 17 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c >> b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c >> index ae13feb..b0897c9 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c >> @@ -211,7 +211,12 @@ struct drm_gem_object >> *exynos_dmabuf_prime_import(struct drm_device *drm_dev, >> >> /* is it from our device? */ >> if (obj->dev == drm_dev) { >> +/* >> + * Importing dmabuf exported from out own gem increases >> + * refcount on gem itself instead of f_count of dmabuf. >> + */ >> drm_gem_object_reference(obj); >> +dma_buf_put(dma_buf); >> return obj; >> } >> } >> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c >> b/drivers/gpu/drm/i915/i915_gem_dmabuf.c >> index aa308e1..32e6287 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c >> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c >> @@ -188,7 +188,12 @@ struct drm_gem_object *i915_gem_prime_import(struct >> drm_device *dev, >> obj = dma_buf->priv; >> /* is it from our device? */ >> if (obj->base.dev == dev) { >> +/* >> + * Importing dmabuf exported from out own gem increases >> + * refcount on gem itself instead of f_count of dmabuf. >> + */ >> drm_gem_object_reference(>base); >> +dma_buf_put(dma_buf); >> return >base; >> } >> } >> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c >> b/drivers/gpu/drm/nouveau/nouveau_prime.c >> index a25cf2c..bb653c6 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c >> @@ -199,6 +199,7 @@ struct drm_gem_object *nouveau_gem_prime_import(struct >> drm_device *dev, >> if (nvbo->gem) { >> if (nvbo->gem->dev == dev) { >> drm_gem_object_reference(nvbo->gem); >> +dma_buf_put(dma_buf); >> return nvbo->gem; >> } >> } >> diff --git a/drivers/gpu/drm/radeon/radeon_prime.c >> b/drivers/gpu/drm/radeon/radeon_prime.c >> index 6bef46a..d344a3be 100644 >> --- a/drivers/gpu/drm/radeon/radeon_prime.c >> +++ b/drivers/gpu/drm/radeon/radeon_prime.c >> @@ -195,6 +195,7 @@ struct drm_gem_object *radeon_gem_prime_import(struct >> drm_device *dev, >> bo = dma_buf->priv; >> if (bo->gem_base.dev == dev) { >> drm_gem_object_reference(>gem_base); >> +dma_buf_put(dma_buf); >>
Re: [PATCH] drm/prime: drop reference on imported dma-buf come from gem
Hi Jani, Sorry for late reply. On 2012년 09월 27일 22:43, Jani Nikula wrote: On Thu, 27 Sep 2012, Seung-Woo Kim sw0312@samsung.com wrote: Increasing ref counts of both dma-buf and gem for imported dma-buf come from gem makes memory leak. release function of dma-buf cannot be called because f_count of dma-buf increased by importing gem and gem ref count cannot be decrease because of exported dma-buf. So I add dma_buf_put() for imported gem come from its own gem into each drivers having prime_import and prime_export capabilities. With this, only gem ref count is increased if importing gem exported from gem of same driver. There's a reference leak bug [1] related to prime self import found by intel-gpu-tools tests. Without much thinking, I just applied this patch to see if it makes a difference. Instead, it now fails with: prime_self_import: prime_self_import.c:61: check_bo: Assertion `ptr1' failed. The assert is at [2]. I think it was possible to re-use a handle imported_buf list which is already deleted, and it was fixed after Dave's commit drm/prime: add exported buffers to current fprivs imported buffer list (v2). I haven't looked into why this happens at all, so I'm just sharing this in case you guys find it helpful. If you still have same assert, I think it will be very helpful to let me know exactly where the assert occurs from 5 check_bo() in test application in your link. Thanks and Best Regards, - Seung-Woo Kim BR, Jani. [1] https://bugs.freedesktop.org/show_bug.cgi?id=54111 [2] http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/tests/prime_self_import.c#n61 Signed-off-by: Seung-Woo Kim sw0312@samsung.com Signed-off-by: Kyungmin.park kyungmin.p...@samsung.com Cc: Inki Dae inki@samsung.com Cc: Daniel Vetter daniel.vet...@ffwll.ch Cc: Rob Clark rob.cl...@linaro.org Cc: Alex Deucher alexander.deuc...@amd.com --- drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |5 + drivers/gpu/drm/i915/i915_gem_dmabuf.c |5 + drivers/gpu/drm/nouveau/nouveau_prime.c|1 + drivers/gpu/drm/radeon/radeon_prime.c |1 + drivers/staging/omapdrm/omap_gem_dmabuf.c |5 + 5 files changed, 17 insertions(+), 0 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c index ae13feb..b0897c9 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c @@ -211,7 +211,12 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev, /* is it from our device? */ if (obj-dev == drm_dev) { +/* + * Importing dmabuf exported from out own gem increases + * refcount on gem itself instead of f_count of dmabuf. + */ drm_gem_object_reference(obj); +dma_buf_put(dma_buf); return obj; } } diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index aa308e1..32e6287 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -188,7 +188,12 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, obj = dma_buf-priv; /* is it from our device? */ if (obj-base.dev == dev) { +/* + * Importing dmabuf exported from out own gem increases + * refcount on gem itself instead of f_count of dmabuf. + */ drm_gem_object_reference(obj-base); +dma_buf_put(dma_buf); return obj-base; } } diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c index a25cf2c..bb653c6 100644 --- a/drivers/gpu/drm/nouveau/nouveau_prime.c +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c @@ -199,6 +199,7 @@ struct drm_gem_object *nouveau_gem_prime_import(struct drm_device *dev, if (nvbo-gem) { if (nvbo-gem-dev == dev) { drm_gem_object_reference(nvbo-gem); +dma_buf_put(dma_buf); return nvbo-gem; } } diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c index 6bef46a..d344a3be 100644 --- a/drivers/gpu/drm/radeon/radeon_prime.c +++ b/drivers/gpu/drm/radeon/radeon_prime.c @@ -195,6 +195,7 @@ struct drm_gem_object *radeon_gem_prime_import(struct drm_device *dev, bo = dma_buf-priv; if (bo-gem_base.dev == dev) { drm_gem_object_reference(bo-gem_base); +dma_buf_put(dma_buf); return bo-gem_base;
[PATCH] drm/prime: drop reference on imported dma-buf come from gem
Hi Rob, On 2012? 09? 27? 15:52, Rob Clark wrote: > fwiw, I had a similar patch: > > https://patchwork.kernel.org/patch/1229161/ Yes, I already check your patch and even my patch's title is a bit from your patch. I thought locking issue blocks your patch, so I sent simple fixes on current state. How do you think merging bug-fixes at first? Best Regards, - Seung-Woo Kim > > although it was on top of some locking fixes from Daniel (which I > think are also needed): > > https://patchwork.kernel.org/patch/1227251/ > > although that seemed to cause/trigger some explosions which I think > still need to be debugged.. > > BR, > -R > > On Thu, Sep 27, 2012 at 8:30 AM, Seung-Woo Kim > wrote: >> Increasing ref counts of both dma-buf and gem for imported dma-buf come from >> gem >> makes memory leak. release function of dma-buf cannot be called because >> f_count >> of dma-buf increased by importing gem and gem ref count cannot be decrease >> because of exported dma-buf. >> >> So I add dma_buf_put() for imported gem come from its own gem into each >> drivers >> having prime_import and prime_export capabilities. With this, only gem ref >> count is increased if importing gem exported from gem of same driver. >> >> Signed-off-by: Seung-Woo Kim >> Signed-off-by: Kyungmin.park >> Cc: Inki Dae >> Cc: Daniel Vetter >> Cc: Rob Clark >> Cc: Alex Deucher >> --- >> drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |5 + >> drivers/gpu/drm/i915/i915_gem_dmabuf.c |5 + >> drivers/gpu/drm/nouveau/nouveau_prime.c|1 + >> drivers/gpu/drm/radeon/radeon_prime.c |1 + >> drivers/staging/omapdrm/omap_gem_dmabuf.c |5 + >> 5 files changed, 17 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c >> b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c >> index ae13feb..b0897c9 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c >> @@ -211,7 +211,12 @@ struct drm_gem_object >> *exynos_dmabuf_prime_import(struct drm_device *drm_dev, >> >> /* is it from our device? */ >> if (obj->dev == drm_dev) { >> + /* >> +* Importing dmabuf exported from out own gem >> increases >> +* refcount on gem itself instead of f_count of >> dmabuf. >> +*/ >> drm_gem_object_reference(obj); >> + dma_buf_put(dma_buf); >> return obj; >> } >> } >> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c >> b/drivers/gpu/drm/i915/i915_gem_dmabuf.c >> index aa308e1..32e6287 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c >> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c >> @@ -188,7 +188,12 @@ struct drm_gem_object *i915_gem_prime_import(struct >> drm_device *dev, >> obj = dma_buf->priv; >> /* is it from our device? */ >> if (obj->base.dev == dev) { >> + /* >> +* Importing dmabuf exported from out own gem >> increases >> +* refcount on gem itself instead of f_count of >> dmabuf. >> +*/ >> drm_gem_object_reference(>base); >> + dma_buf_put(dma_buf); >> return >base; >> } >> } >> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c >> b/drivers/gpu/drm/nouveau/nouveau_prime.c >> index a25cf2c..bb653c6 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c >> @@ -199,6 +199,7 @@ struct drm_gem_object *nouveau_gem_prime_import(struct >> drm_device *dev, >> if (nvbo->gem) { >> if (nvbo->gem->dev == dev) { >> drm_gem_object_reference(nvbo->gem); >> + dma_buf_put(dma_buf); >> return nvbo->gem; >> } >> } >> diff --git a/drivers/gpu/drm/radeon/radeon_prime.c >> b/drivers/gpu/drm/radeon/radeon_prime.c >> index 6bef46a..d344a3be 100644 >> --- a/drivers/gpu/drm/radeon/radeon_prime.c >> +++ b/drivers/gpu/drm/radeon/radeon_prime.c >> @@ -195,6 +195,7 @@ struct drm_gem_object *radeon_gem_prime_import(struct >> drm_device *dev, >> bo = dma_buf->priv; >> if (bo->gem_base.dev == dev) { >> drm_gem_object_reference(>gem_base); >> + dma_buf_put(dma_buf); >> return >gem_base; >> } >> } >> diff --git a/drivers/staging/omapdrm/omap_gem_dmabuf.c >> b/drivers/staging/omapdrm/omap_gem_dmabuf.c >> index 42728e0..5b50eb6 100644 >> --- a/drivers/staging/omapdrm/omap_gem_dmabuf.c >> +++ b/drivers/staging/omapdrm/omap_gem_dmabuf.c >> @@ -207,7 +207,12
Re: [PATCH] drm/prime: drop reference on imported dma-buf come from gem
Hi Rob, On 2012년 09월 27일 15:52, Rob Clark wrote: fwiw, I had a similar patch: https://patchwork.kernel.org/patch/1229161/ Yes, I already check your patch and even my patch's title is a bit from your patch. I thought locking issue blocks your patch, so I sent simple fixes on current state. How do you think merging bug-fixes at first? Best Regards, - Seung-Woo Kim although it was on top of some locking fixes from Daniel (which I think are also needed): https://patchwork.kernel.org/patch/1227251/ although that seemed to cause/trigger some explosions which I think still need to be debugged.. BR, -R On Thu, Sep 27, 2012 at 8:30 AM, Seung-Woo Kim sw0312@samsung.com wrote: Increasing ref counts of both dma-buf and gem for imported dma-buf come from gem makes memory leak. release function of dma-buf cannot be called because f_count of dma-buf increased by importing gem and gem ref count cannot be decrease because of exported dma-buf. So I add dma_buf_put() for imported gem come from its own gem into each drivers having prime_import and prime_export capabilities. With this, only gem ref count is increased if importing gem exported from gem of same driver. Signed-off-by: Seung-Woo Kim sw0312@samsung.com Signed-off-by: Kyungmin.park kyungmin.p...@samsung.com Cc: Inki Dae inki@samsung.com Cc: Daniel Vetter daniel.vet...@ffwll.ch Cc: Rob Clark rob.cl...@linaro.org Cc: Alex Deucher alexander.deuc...@amd.com --- drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |5 + drivers/gpu/drm/i915/i915_gem_dmabuf.c |5 + drivers/gpu/drm/nouveau/nouveau_prime.c|1 + drivers/gpu/drm/radeon/radeon_prime.c |1 + drivers/staging/omapdrm/omap_gem_dmabuf.c |5 + 5 files changed, 17 insertions(+), 0 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c index ae13feb..b0897c9 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c @@ -211,7 +211,12 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev, /* is it from our device? */ if (obj-dev == drm_dev) { + /* +* Importing dmabuf exported from out own gem increases +* refcount on gem itself instead of f_count of dmabuf. +*/ drm_gem_object_reference(obj); + dma_buf_put(dma_buf); return obj; } } diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index aa308e1..32e6287 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -188,7 +188,12 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, obj = dma_buf-priv; /* is it from our device? */ if (obj-base.dev == dev) { + /* +* Importing dmabuf exported from out own gem increases +* refcount on gem itself instead of f_count of dmabuf. +*/ drm_gem_object_reference(obj-base); + dma_buf_put(dma_buf); return obj-base; } } diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c index a25cf2c..bb653c6 100644 --- a/drivers/gpu/drm/nouveau/nouveau_prime.c +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c @@ -199,6 +199,7 @@ struct drm_gem_object *nouveau_gem_prime_import(struct drm_device *dev, if (nvbo-gem) { if (nvbo-gem-dev == dev) { drm_gem_object_reference(nvbo-gem); + dma_buf_put(dma_buf); return nvbo-gem; } } diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c index 6bef46a..d344a3be 100644 --- a/drivers/gpu/drm/radeon/radeon_prime.c +++ b/drivers/gpu/drm/radeon/radeon_prime.c @@ -195,6 +195,7 @@ struct drm_gem_object *radeon_gem_prime_import(struct drm_device *dev, bo = dma_buf-priv; if (bo-gem_base.dev == dev) { drm_gem_object_reference(bo-gem_base); + dma_buf_put(dma_buf); return bo-gem_base; } } diff --git a/drivers/staging/omapdrm/omap_gem_dmabuf.c b/drivers/staging/omapdrm/omap_gem_dmabuf.c index 42728e0..5b50eb6 100644 --- a/drivers/staging/omapdrm/omap_gem_dmabuf.c +++ b/drivers/staging/omapdrm/omap_gem_dmabuf.c @@ -207,7 +207,12 @@ struct drm_gem_object * omap_gem_prime_import(struct
[PATCH] drm/exynos: fix double call of drm_prime_(init/destroy)_file_private
Hi Mandeep, On 2012? 09? 06? 06:47, Mandeep Singh Baines wrote: > The double invocations are incorrect but seem to be safe so I don't > think this will fix any bugs. > > Before: > > [7.639366] drm_prime_init_file ee3675d0 > [7.639377] drm_prime_init_file ee3675d0 > [7.639507] drm_prime_destroy_file ee3675d0 > [7.639518] drm_prime_destroy_file ee3675d0 > [7.639802] drm_prime_init_file ee372390 > [7.639810] drm_prime_init_file ee372390 > [8.473316] drm_prime_init_file ee356390 > [8.473331] drm_prime_init_file ee356390 > > After: > > [6.363842] drm_prime_init_file edc2e5d0 > [6.363994] drm_prime_destroy_file edc2e5d0 > [6.364260] drm_prime_init_file edc2e750 > [8.004837] drm_prime_init_file ee36ded0 > You are right. prime file_priv is handled by drm_fops.c, so it can be removed from exynos drm. Thanks for your patch. > Signed-off-by: Mandeep Singh Baines > CC: St?phane Marchesin > CC: Pawel Osciak > CC: Inki Dae > CC: Joonyoung Shim > CC: Seung-Woo Kim > CC: Kyungmin Park > CC: David Airlie > CC: dri-devel at lists.freedesktop.org Acked-by: Seung-Woo Kim > --- > drivers/gpu/drm/exynos/exynos_drm_drv.c |2 -- > 1 files changed, 0 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c > b/drivers/gpu/drm/exynos/exynos_drm_drv.c > index ebacec6..a27b8ff 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > @@ -160,7 +160,6 @@ static int exynos_drm_open(struct drm_device *dev, struct > drm_file *file) > if (!file_priv) > return -ENOMEM; > > - drm_prime_init_file_private(>prime); > file->driver_priv = file_priv; > > return exynos_drm_subdrv_open(dev, file); > @@ -184,7 +183,6 @@ static void exynos_drm_preclose(struct drm_device *dev, > e->base.destroy(>base); > } > } > - drm_prime_destroy_file_private(>prime); > spin_unlock_irqrestore(>event_lock, flags); > > exynos_drm_subdrv_close(dev, file); > -- Seung-Woo Kim Samsung Software R Center --
Re: [PATCH] drm/exynos: fix double call of drm_prime_(init/destroy)_file_private
Hi Mandeep, On 2012년 09월 06일 06:47, Mandeep Singh Baines wrote: The double invocations are incorrect but seem to be safe so I don't think this will fix any bugs. Before: [7.639366] drm_prime_init_file ee3675d0 [7.639377] drm_prime_init_file ee3675d0 [7.639507] drm_prime_destroy_file ee3675d0 [7.639518] drm_prime_destroy_file ee3675d0 [7.639802] drm_prime_init_file ee372390 [7.639810] drm_prime_init_file ee372390 [8.473316] drm_prime_init_file ee356390 [8.473331] drm_prime_init_file ee356390 After: [6.363842] drm_prime_init_file edc2e5d0 [6.363994] drm_prime_destroy_file edc2e5d0 [6.364260] drm_prime_init_file edc2e750 [8.004837] drm_prime_init_file ee36ded0 You are right. prime file_priv is handled by drm_fops.c, so it can be removed from exynos drm. Thanks for your patch. Signed-off-by: Mandeep Singh Baines m...@chromium.org CC: Stéphane Marchesin marc...@chromium.org CC: Pawel Osciak posc...@google.com CC: Inki Dae inki@samsung.com CC: Joonyoung Shim jy0922.s...@samsung.com CC: Seung-Woo Kim sw0312@samsung.com CC: Kyungmin Park kyungmin.p...@samsung.com CC: David Airlie airl...@linux.ie CC: dri-devel@lists.freedesktop.org Acked-by: Seung-Woo Kim sw0312@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_drv.c |2 -- 1 files changed, 0 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index ebacec6..a27b8ff 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -160,7 +160,6 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file) if (!file_priv) return -ENOMEM; - drm_prime_init_file_private(file-prime); file-driver_priv = file_priv; return exynos_drm_subdrv_open(dev, file); @@ -184,7 +183,6 @@ static void exynos_drm_preclose(struct drm_device *dev, e-base.destroy(e-base); } } - drm_prime_destroy_file_private(file-prime); spin_unlock_irqrestore(dev-event_lock, flags); exynos_drm_subdrv_close(dev, file); -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/exynos: Update the MAX_EDID value for E-DDC
Hi Daniel, On 2012? 08? 23? 17:50, Daniel Vetter wrote: > On Wed, Aug 22, 2012 at 06:53:33PM +0530, Shirish S wrote: >> From: Shirish Shankarappa >> >> The value of MAX_EDID is now valid only for 2 >> block EDID data which is 256, but to support >> 4 block EDID (E-DDC) this needs to be 512. >> >> Signed-off-by: Shirish Shankarappa >> --- >> drivers/gpu/drm/exynos/exynos_drm_connector.c |2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c >> b/drivers/gpu/drm/exynos/exynos_drm_connector.c >> index d956819..69d02b5 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c >> @@ -32,7 +32,7 @@ >> #include "exynos_drm_drv.h" >> #include "exynos_drm_encoder.h" >> >> -#define MAX_EDID 256 >> +#define MAX_EDID 512 >> #define to_exynos_connector(x) container_of(x, struct >> exynos_drm_connector,\ >> drm_connector) > > Shouldn't this be in a common drm/edid header to begin with? This value is not real maximum length of EDID and it is only used for memory allocation by exynos connector. Actually, this allocation is unnecessary because edid is already allocated by drm_get_edid(). So, I have plan to remove this allocation and the definition once Jani Nikula's patches for removing raw_edid related memory leaks are applied. > -Daniel > Thanks and Regards, - Seung-Woo Kim -- Seung-Woo Kim Samsung Software R Center --
Re: [PATCH] drm/exynos: Update the MAX_EDID value for E-DDC
Hi Daniel, On 2012년 08월 23일 17:50, Daniel Vetter wrote: On Wed, Aug 22, 2012 at 06:53:33PM +0530, Shirish S wrote: From: Shirish Shankarappa s.shir...@samsung.com The value of MAX_EDID is now valid only for 2 block EDID data which is 256, but to support 4 block EDID (E-DDC) this needs to be 512. Signed-off-by: Shirish Shankarappa s.shir...@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_connector.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c b/drivers/gpu/drm/exynos/exynos_drm_connector.c index d956819..69d02b5 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_connector.c +++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c @@ -32,7 +32,7 @@ #include exynos_drm_drv.h #include exynos_drm_encoder.h -#define MAX_EDID 256 +#define MAX_EDID 512 #define to_exynos_connector(x) container_of(x, struct exynos_drm_connector,\ drm_connector) Shouldn't this be in a common drm/edid header to begin with? This value is not real maximum length of EDID and it is only used for memory allocation by exynos connector. Actually, this allocation is unnecessary because edid is already allocated by drm_get_edid(). So, I have plan to remove this allocation and the definition once Jani Nikula's patches for removing raw_edid related memory leaks are applied. -Daniel Thanks and Regards, - Seung-Woo Kim -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH libdrm] libdrm: update drm/drm_fourcc.h from kernel to add multi plane formats
Hi Ville, I skipped explanation about NV12M and other two formats because these formats are already in kernel drm_fourcc.h. I think it is better to add a difference between NV12 and NV12M here. NV12M has Y plane and CbCr plane and these are in non contiguous memory region. Compared with NV12, NV12M's memory shape is like following. NV12 : __(Y)(CbCr)___ NV12M : __(Y)_ . _(CbCr)__ Y and CbCr plane of NV12 can be expressed with one memory address and offset of each plane. but NV12M needs memory address of each plane. On 2012? 03? 30? 19:12, Ville Syrj?l? wrote: > On Fri, Mar 30, 2012 at 11:54:50AM +0900, Seung-Woo Kim wrote: >> Multi buffer plane pixel formats are added as like kernel header. >> >> Signed-off-by: Seung-Woo Kim >> --- >> include/drm/drm_fourcc.h |7 +++ >> 1 files changed, 7 insertions(+), 0 deletions(-) >> >> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h >> index 85facb0..7cfd95a 100644 >> --- a/include/drm/drm_fourcc.h >> +++ b/include/drm/drm_fourcc.h >> @@ -107,6 +107,10 @@ >> #define DRM_FORMAT_NV16fourcc_code('N', 'V', '1', '6') /* 2x1 >> subsampled Cr:Cb plane */ >> #define DRM_FORMAT_NV61fourcc_code('N', 'V', '6', '1') /* 2x1 >> subsampled Cb:Cr plane */ >> >> +/* 2 non contiguous plane YCbCr */ >> +#define DRM_FORMAT_NV12Mfourcc_code('N', 'M', '1', '2') /* 2x2 >> subsampled Cr:Cb plane */ > NAK. DRM_FORMAT_NV12 handles this just fine. > Exynos soc supports two kinds of memory shape explained above, so two different types are need for exynos soc. >> +#define DRM_FORMAT_NV12MT fourcc_code('T', 'M', '1', '2') /* 2x2 >> subsampled Cr:Cb plane 64x32 macroblocks */ > This one is more difficult. Until now tiling was always handled in > driver specific manner. OTOH if this format is really supported by > different devices from multiple vendors, then it would probably > make sense to add it as a standard format. > Exynos soc also supports normal and tiled pixel data and pixel data is shared between hw blocks for example from scaler to hdmi. So driver can not handle it internally. IMHO, support for various shape can be helpful even though only exynos soc family suppors these formats currently. >> + >> /* >>* 3 plane YCbCr >>* index 0: Y plane, [7:0] Y >> @@ -127,4 +131,7 @@ >> #define DRM_FORMAT_YUV444 fourcc_code('Y', 'U', '2', '4') /* >> non-subsampled Cb (1) and Cr (2) planes */ >> #define DRM_FORMAT_YVU444 fourcc_code('Y', 'V', '2', '4') /* >> non-subsampled Cr (1) and Cb (2) planes */ >> >> +/* 3 non contiguous plane YCbCr */ >> +#define DRM_FORMAT_YUV420M fourcc_code('Y', 'M', '1', '2') /* 2x2 >> subsampled Cb (1) and Cr (2) planes */a > NAK. DRM_FORMAT_YUV420 handles this. This is same with case of NV12M. Regards. -- Seung-Woo Kim Samsung Software R Center --
Re: [PATCH libdrm] libdrm: update drm/drm_fourcc.h from kernel to add multi plane formats
Hi Ville, I skipped explanation about NV12M and other two formats because these formats are already in kernel drm_fourcc.h. I think it is better to add a difference between NV12 and NV12M here. NV12M has Y plane and CbCr plane and these are in non contiguous memory region. Compared with NV12, NV12M's memory shape is like following. NV12 : __(Y)(CbCr)___ NV12M : __(Y)_ . _(CbCr)__ Y and CbCr plane of NV12 can be expressed with one memory address and offset of each plane. but NV12M needs memory address of each plane. On 2012년 03월 30일 19:12, Ville Syrjälä wrote: On Fri, Mar 30, 2012 at 11:54:50AM +0900, Seung-Woo Kim wrote: Multi buffer plane pixel formats are added as like kernel header. Signed-off-by: Seung-Woo Kimsw0312@samsung.com --- include/drm/drm_fourcc.h |7 +++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index 85facb0..7cfd95a 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -107,6 +107,10 @@ #define DRM_FORMAT_NV16 fourcc_code('N', 'V', '1', '6') /* 2x1 subsampled Cr:Cb plane */ #define DRM_FORMAT_NV61 fourcc_code('N', 'V', '6', '1') /* 2x1 subsampled Cb:Cr plane */ +/* 2 non contiguous plane YCbCr */ +#define DRM_FORMAT_NV12M fourcc_code('N', 'M', '1', '2') /* 2x2 subsampled Cr:Cb plane */ NAK. DRM_FORMAT_NV12 handles this just fine. Exynos soc supports two kinds of memory shape explained above, so two different types are need for exynos soc. +#define DRM_FORMAT_NV12MT fourcc_code('T', 'M', '1', '2') /* 2x2 subsampled Cr:Cb plane 64x32 macroblocks */ This one is more difficult. Until now tiling was always handled in driver specific manner. OTOH if this format is really supported by different devices from multiple vendors, then it would probably make sense to add it as a standard format. Exynos soc also supports normal and tiled pixel data and pixel data is shared between hw blocks for example from scaler to hdmi. So driver can not handle it internally. IMHO, support for various shape can be helpful even though only exynos soc family suppors these formats currently. + /* * 3 plane YCbCr * index 0: Y plane, [7:0] Y @@ -127,4 +131,7 @@ #define DRM_FORMAT_YUV444 fourcc_code('Y', 'U', '2', '4') /* non-subsampled Cb (1) and Cr (2) planes */ #define DRM_FORMAT_YVU444 fourcc_code('Y', 'V', '2', '4') /* non-subsampled Cr (1) and Cb (2) planes */ +/* 3 non contiguous plane YCbCr */ +#define DRM_FORMAT_YUV420M fourcc_code('Y', 'M', '1', '2') /* 2x2 subsampled Cb (1) and Cr (2) planes */a NAK. DRM_FORMAT_YUV420 handles this. This is same with case of NV12M. Regards. -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel