Andrzej Hajda (6):
drm/exynos/hdmi: fix edid memory leak
drm/exynos/mixer: fix interrupt clearing
drm/exynos/mixer: correct vsync configuration sequence
drm/exynos/mixer: always update INT_EN cache
drm/exynos/mixer: simplify poweron flag
drm/exynos/mixer: replace MXR_INT_EN register cache with flag
drivers/gpu/drm/exynos/exynos_hdmi.c | 7 ++-
drivers/gpu/drm/exynos/exynos_mixer.c | 80 +++++++++++++----------------------
2 files changed, 36 insertions(+), 51 deletions(-)
--
1.9.1
edid returned by drm_get_edid should be freed.
The patch fixes it.
Signed-off-by: Andrzej Hajda <[email protected]>
---
drivers/gpu/drm/exynos/exynos_hdmi.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 99e2864..4a00990 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -1064,6 +1064,7 @@ static int hdmi_get_modes(struct drm_connector *connector)
{
struct hdmi_context *hdata = ctx_from_connector(connector);
struct edid *edid;
+ int ret;
if (!hdata->ddc_adpt)
return -ENODEV;
@@ -1079,7 +1080,11 @@ static int hdmi_get_modes(struct drm_connector *connector)
drm_mode_connector_update_edid_property(connector, edid);
- return drm_add_edid_modes(connector, edid);
+ ret = drm_add_edid_modes(connector, edid);
+
+ kfree(edid);
+
+ return ret;
}
static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
--
1.9.1
The driver used incorrect flags to clear interrupt status.
The patch fixes it.
Signed-off-by: Andrzej Hajda <[email protected]>
---
drivers/gpu/drm/exynos/exynos_mixer.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index cae98db..0686484 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -718,6 +718,9 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
/* handling VSYNC */
if (val & MXR_INT_STATUS_VSYNC) {
+ /* vsync interrupt use different bit for read and clear */
+ val |= MXR_INT_CLEAR_VSYNC;
+
/* interlace scan need to check shadow register */
if (ctx->interlace) {
base = mixer_reg_read(res, MXR_GRAPHIC_BASE(0));
@@ -743,11 +746,6 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
out:
/* clear interrupts */
- if (~val & MXR_INT_EN_VSYNC) {
- /* vsync interrupt use different bit for read and clear */
- val &= ~MXR_INT_EN_VSYNC;
- val |= MXR_INT_CLEAR_VSYNC;
- }
mixer_reg_write(res, MXR_INT_STATUS, val);
spin_unlock(&res->reg_slock);
--
1.9.1
Specification advises to clear vsync indicator before configuring vsync.
Signed-off-by: Andrzej Hajda <[email protected]>
---
drivers/gpu/drm/exynos/exynos_mixer.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 0686484..b338a10 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -905,8 +905,8 @@ static int mixer_enable_vblank(struct exynos_drm_crtc *crtc)
}
/* enable vsync interrupt */
- mixer_reg_writemask(res, MXR_INT_EN, MXR_INT_EN_VSYNC,
- MXR_INT_EN_VSYNC);
+ mixer_reg_writemask(res, MXR_INT_STATUS, ~0, MXR_INT_CLEAR_VSYNC);
+ mixer_reg_writemask(res, MXR_INT_EN, ~0, MXR_INT_EN_VSYNC);
return 0;
}
@@ -917,6 +917,7 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
struct mixer_resources *res = &mixer_ctx->mixer_res;
/* disable vsync interrupt */
+ mixer_reg_writemask(res, MXR_INT_STATUS, ~0, MXR_INT_CLEAR_VSYNC);
mixer_reg_writemask(res, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
}
@@ -1045,6 +1046,8 @@ static void mixer_enable(struct exynos_drm_crtc *crtc)
mixer_reg_writemask(res, MXR_STATUS, ~0, MXR_STATUS_SOFT_RESET);
+ if (ctx->int_en & MXR_INT_EN_VSYNC)
+ mixer_reg_writemask(res, MXR_INT_STATUS, ~0, MXR_INT_CLEAR_VSYNC);
mixer_reg_write(res, MXR_INT_EN, ctx->int_en);
mixer_win_reset(ctx);
}
--
1.9.1
INT_EN cache field was updated only by mixer_enable_vblank.
The patch adds update also by mixer_disable_vblank function.
Signed-off-by: Andrzej Hajda <[email protected]>
---
drivers/gpu/drm/exynos/exynos_mixer.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index b338a10..53bcb70 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -916,6 +916,11 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
struct mixer_context *mixer_ctx = crtc->ctx;
struct mixer_resources *res = &mixer_ctx->mixer_res;
+ if (!mixer_ctx->powered) {
+ mixer_ctx->int_en &= MXR_INT_EN_VSYNC;
+ return;
+ }
+
/* disable vsync interrupt */
mixer_reg_writemask(res, MXR_INT_STATUS, ~0, MXR_INT_CLEAR_VSYNC);
mixer_reg_writemask(res, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
--
1.9.1
The driver uses bool protected by mutex to track power state.
The patch replaces this combo with single bit and atomic bitops.
Signed-off-by: Andrzej Hajda <[email protected]>
---
drivers/gpu/drm/exynos/exynos_mixer.c | 52 ++++++++++-------------------------
1 file changed, 14 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 53bcb70..453b6bb 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -69,6 +69,10 @@ enum mixer_version_id {
MXR_VER_128_0_0_184,
};
+enum mixer_flag_bits {
+ MXR_BIT_POWERED,
+};
+
struct mixer_context {
struct platform_device *pdev;
struct device *dev;
@@ -76,13 +80,12 @@ struct mixer_context {
struct exynos_drm_crtc *crtc;
struct exynos_drm_plane planes[MIXER_WIN_NR];
int pipe;
+ unsigned long flags;
bool interlace;
- bool powered;
bool vp_enabled;
bool has_sclk;
u32 int_en;
- struct mutex mixer_mutex;
struct mixer_resources mixer_res;
enum mixer_version_id mxr_ver;
wait_queue_head_t wait_vsync_queue;
@@ -899,7 +902,7 @@ static int mixer_enable_vblank(struct exynos_drm_crtc *crtc)
struct mixer_context *mixer_ctx = crtc->ctx;
struct mixer_resources *res = &mixer_ctx->mixer_res;
- if (!mixer_ctx->powered) {
+ if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) {
mixer_ctx->int_en |= MXR_INT_EN_VSYNC;
return 0;
}
@@ -916,7 +919,7 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
struct mixer_context *mixer_ctx = crtc->ctx;
struct mixer_resources *res = &mixer_ctx->mixer_res;
- if (!mixer_ctx->powered) {
+ if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) {
mixer_ctx->int_en &= MXR_INT_EN_VSYNC;
return;
}
@@ -932,12 +935,8 @@ static void mixer_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
DRM_DEBUG_KMS("win: %d\n", win);
- mutex_lock(&mixer_ctx->mixer_mutex);
- if (!mixer_ctx->powered) {
- mutex_unlock(&mixer_ctx->mixer_mutex);
+ if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
return;
- }
- mutex_unlock(&mixer_ctx->mixer_mutex);
if (win > 1 && mixer_ctx->vp_enabled)
vp_video_buffer(mixer_ctx, win);
@@ -953,12 +952,8 @@ static void mixer_win_disable(struct exynos_drm_crtc *crtc, unsigned int win)
DRM_DEBUG_KMS("win: %d\n", win);
- mutex_lock(&mixer_ctx->mixer_mutex);
- if (!mixer_ctx->powered) {
- mutex_unlock(&mixer_ctx->mixer_mutex);
+ if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
return;
- }
- mutex_unlock(&mixer_ctx->mixer_mutex);
spin_lock_irqsave(&res->reg_slock, flags);
mixer_vsync_set_update(mixer_ctx, false);
@@ -974,12 +969,8 @@ static void mixer_wait_for_vblank(struct exynos_drm_crtc *crtc)
struct mixer_context *mixer_ctx = crtc->ctx;
int err;
- mutex_lock(&mixer_ctx->mixer_mutex);
- if (!mixer_ctx->powered) {
- mutex_unlock(&mixer_ctx->mixer_mutex);
+ if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
return;
- }
- mutex_unlock(&mixer_ctx->mixer_mutex);
err = drm_vblank_get(mixer_ctx->drm_dev, mixer_ctx->pipe);
if (err < 0) {
@@ -1007,13 +998,8 @@ static void mixer_enable(struct exynos_drm_crtc *crtc)
struct mixer_resources *res = &ctx->mixer_res;
int ret;
- mutex_lock(&ctx->mixer_mutex);
- if (ctx->powered) {
- mutex_unlock(&ctx->mixer_mutex);
+ if (test_bit(MXR_BIT_POWERED, &ctx->flags))
return;
- }
-
- mutex_unlock(&ctx->mixer_mutex);
pm_runtime_get_sync(ctx->dev);
@@ -1045,9 +1031,7 @@ static void mixer_enable(struct exynos_drm_crtc *crtc)
}
}
- mutex_lock(&ctx->mixer_mutex);
- ctx->powered = true;
- mutex_unlock(&ctx->mixer_mutex);
+ set_bit(MXR_BIT_POWERED, &ctx->flags);
mixer_reg_writemask(res, MXR_STATUS, ~0, MXR_STATUS_SOFT_RESET);
@@ -1063,12 +1047,8 @@ static void mixer_disable(struct exynos_drm_crtc *crtc)
struct mixer_resources *res = &ctx->mixer_res;
int i;
- mutex_lock(&ctx->mixer_mutex);
- if (!ctx->powered) {
- mutex_unlock(&ctx->mixer_mutex);
+ if (!test_bit(MXR_BIT_POWERED, &ctx->flags))
return;
- }
- mutex_unlock(&ctx->mixer_mutex);
mixer_stop(ctx);
mixer_regs_dump(ctx);
@@ -1078,9 +1058,7 @@ static void mixer_disable(struct exynos_drm_crtc *crtc)
ctx->int_en = mixer_reg_read(res, MXR_INT_EN);
- mutex_lock(&ctx->mixer_mutex);
- ctx->powered = false;
- mutex_unlock(&ctx->mixer_mutex);
+ clear_bit(MXR_BIT_POWERED, &ctx->flags);
clk_disable_unprepare(res->hdmi);
clk_disable_unprepare(res->mixer);
@@ -1242,8 +1220,6 @@ static int mixer_probe(struct platform_device *pdev)
return -ENOMEM;
}
- mutex_init(&ctx->mixer_mutex);
-
if (dev->of_node) {
const struct of_device_id *match;
--
1.9.1
Driver uses only VSYNC interrupts, so we need to cache VSYNC bit state only.
Signed-off-by: Andrzej Hajda <[email protected]>
---
drivers/gpu/drm/exynos/exynos_mixer.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 453b6bb..7e57409 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -71,6 +71,7 @@ enum mixer_version_id {
enum mixer_flag_bits {
MXR_BIT_POWERED,
+ MXR_BIT_VSYNC,
};
struct mixer_context {
@@ -84,7 +85,6 @@ struct mixer_context {
bool interlace;
bool vp_enabled;
bool has_sclk;
- u32 int_en;
struct mixer_resources mixer_res;
enum mixer_version_id mxr_ver;
@@ -902,10 +902,9 @@ static int mixer_enable_vblank(struct exynos_drm_crtc *crtc)
struct mixer_context *mixer_ctx = crtc->ctx;
struct mixer_resources *res = &mixer_ctx->mixer_res;
- if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) {
- mixer_ctx->int_en |= MXR_INT_EN_VSYNC;
+ __set_bit(MXR_BIT_VSYNC, &mixer_ctx->flags);
+ if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
return 0;
- }
/* enable vsync interrupt */
mixer_reg_writemask(res, MXR_INT_STATUS, ~0, MXR_INT_CLEAR_VSYNC);
@@ -919,10 +918,10 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
struct mixer_context *mixer_ctx = crtc->ctx;
struct mixer_resources *res = &mixer_ctx->mixer_res;
- if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags)) {
- mixer_ctx->int_en &= MXR_INT_EN_VSYNC;
+ __clear_bit(MXR_BIT_VSYNC, &mixer_ctx->flags);
+
+ if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
return;
- }
/* disable vsync interrupt */
mixer_reg_writemask(res, MXR_INT_STATUS, ~0, MXR_INT_CLEAR_VSYNC);
@@ -1035,9 +1034,10 @@ static void mixer_enable(struct exynos_drm_crtc *crtc)
mixer_reg_writemask(res, MXR_STATUS, ~0, MXR_STATUS_SOFT_RESET);
- if (ctx->int_en & MXR_INT_EN_VSYNC)
+ if (test_bit(MXR_BIT_VSYNC, &ctx->flags)) {
mixer_reg_writemask(res, MXR_INT_STATUS, ~0, MXR_INT_CLEAR_VSYNC);
- mixer_reg_write(res, MXR_INT_EN, ctx->int_en);
+ mixer_reg_writemask(res, MXR_INT_EN, ~0, MXR_INT_EN_VSYNC);
+ }
mixer_win_reset(ctx);
}
@@ -1056,8 +1056,6 @@ static void mixer_disable(struct exynos_drm_crtc *crtc)
for (i = 0; i < MIXER_WIN_NR; i++)
mixer_win_disable(crtc, i);
- ctx->int_en = mixer_reg_read(res, MXR_INT_EN);
-
clear_bit(MXR_BIT_POWERED, &ctx->flags);
clk_disable_unprepare(res->hdmi);
--
1.9.1
On 07/09/2015 03:25 PM, Andrzej Hajda wrote:
> The driver used incorrect flags to clear interrupt status.
> The patch fixes it.
>
> Signed-off-by: Andrzej Hajda <[email protected]>
> ---
> drivers/gpu/drm/exynos/exynos_mixer.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index cae98db..0686484 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -718,6 +718,9 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
>
> /* handling VSYNC */
> if (val & MXR_INT_STATUS_VSYNC) {
> + /* vsync interrupt use different bit for read and clear */
> + val |= MXR_INT_CLEAR_VSYNC;
MXR_INT_STATUS_VSYNC bit of MXR_INT_STATUS register is read-only, so it
needs to be clear MXR_INT_STATUS_VSYNC bit of val.
> +
> /* interlace scan need to check shadow register */
> if (ctx->interlace) {
> base = mixer_reg_read(res, MXR_GRAPHIC_BASE(0));
> @@ -743,11 +746,6 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
>
> out:
> /* clear interrupts */
> - if (~val & MXR_INT_EN_VSYNC) {
> - /* vsync interrupt use different bit for read and clear */
> - val &= ~MXR_INT_EN_VSYNC;
> - val |= MXR_INT_CLEAR_VSYNC;
> - }
> mixer_reg_write(res, MXR_INT_STATUS, val);
>
> spin_unlock(&res->reg_slock);
>
Hi Andrzej,
On 07/09/2015 03:25 PM, Andrzej Hajda wrote:
>
>
> Andrzej Hajda (6):
> drm/exynos/hdmi: fix edid memory leak
> drm/exynos/mixer: fix interrupt clearing
> drm/exynos/mixer: correct vsync configuration sequence
> drm/exynos/mixer: always update INT_EN cache
> drm/exynos/mixer: simplify poweron flag
> drm/exynos/mixer: replace MXR_INT_EN register cache with flag
>
> drivers/gpu/drm/exynos/exynos_hdmi.c | 7 ++-
> drivers/gpu/drm/exynos/exynos_mixer.c | 80 +++++++++++++----------------------
> 2 files changed, 36 insertions(+), 51 deletions(-)
>
Looks good to me except one comment of "drm/exynos/mixer: fix interrupt
clearing" patch. Also below my patch can be dropped by this patchset.
http://www.spinics.net/lists/dri-devel/msg85322.html
Reviewed-by: Joonyoung Shim <[email protected]>
The driver used incorrect flags to clear interrupt status.
The patch fixes it.
Signed-off-by: Andrzej Hajda <[email protected]>
---
drivers/gpu/drm/exynos/exynos_mixer.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index cae98db..25f0aac 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -718,6 +718,10 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
/* handling VSYNC */
if (val & MXR_INT_STATUS_VSYNC) {
+ /* vsync interrupt use different bit for read and clear */
+ val |= MXR_INT_CLEAR_VSYNC;
+ val &= ~MXR_INT_STATUS_VSYNC;
+
/* interlace scan need to check shadow register */
if (ctx->interlace) {
base = mixer_reg_read(res, MXR_GRAPHIC_BASE(0));
@@ -743,11 +747,6 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
out:
/* clear interrupts */
- if (~val & MXR_INT_EN_VSYNC) {
- /* vsync interrupt use different bit for read and clear */
- val &= ~MXR_INT_EN_VSYNC;
- val |= MXR_INT_CLEAR_VSYNC;
- }
mixer_reg_write(res, MXR_INT_STATUS, val);
spin_unlock(&res->reg_slock);
--
1.9.1
On 07/09/2015 05:07 PM, Andrzej Hajda wrote:
> The driver used incorrect flags to clear interrupt status.
> The patch fixes it.
>
> Signed-off-by: Andrzej Hajda <[email protected]>
> ---
> drivers/gpu/drm/exynos/exynos_mixer.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index cae98db..25f0aac 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -718,6 +718,10 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
>
> /* handling VSYNC */
> if (val & MXR_INT_STATUS_VSYNC) {
> + /* vsync interrupt use different bit for read and clear */
> + val |= MXR_INT_CLEAR_VSYNC;
> + val &= ~MXR_INT_STATUS_VSYNC;
> +
> /* interlace scan need to check shadow register */
> if (ctx->interlace) {
> base = mixer_reg_read(res, MXR_GRAPHIC_BASE(0));
> @@ -743,11 +747,6 @@ static irqreturn_t mixer_irq_handler(int irq, void *arg)
>
> out:
> /* clear interrupts */
> - if (~val & MXR_INT_EN_VSYNC) {
> - /* vsync interrupt use different bit for read and clear */
> - val &= ~MXR_INT_EN_VSYNC;
> - val |= MXR_INT_CLEAR_VSYNC;
> - }
> mixer_reg_write(res, MXR_INT_STATUS, val);
>
> spin_unlock(&res->reg_slock);
>
Reviewed-by: Joonyoung Shim <[email protected]>
Thanks.