diff mbox

[v3,05/32] drm/exynos: hdmi: Implement initialize op for hdmi

Message ID 1383063198-10526-6-git-send-email-seanpaul@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sean Paul Oct. 29, 2013, 4:12 p.m. UTC
This patch implements the initialize callback in the hdmi and mixer
manager. This allows us to get rid of drm_dev in the drm_hdmi level and
track it in the mixer and hdmi drivers. This is one of the things
holding back the complete removal of the drm_hdmi layer.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---

Changes in v2: None
Changes in v3: None

 drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 35 ++++++++++++++++++++++++++------
 drivers/gpu/drm/exynos/exynos_drm_hdmi.h |  3 ++-
 drivers/gpu/drm/exynos/exynos_hdmi.c     | 18 ++++++++++++----
 drivers/gpu/drm/exynos/exynos_mixer.c    | 35 +++++++++++++++++++-------------
 4 files changed, 66 insertions(+), 25 deletions(-)

Comments

Tomasz Figa Oct. 31, 2013, 11:53 p.m. UTC | #1
Hi Sean,

On Tuesday 29 of October 2013 12:12:51 Sean Paul wrote:
> This patch implements the initialize callback in the hdmi and mixer
> manager. This allows us to get rid of drm_dev in the drm_hdmi level and
> track it in the mixer and hdmi drivers. This is one of the things
> holding back the complete removal of the drm_hdmi layer.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> 
> Changes in v2: None
> Changes in v3: None
> 
>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 35
> ++++++++++++++++++++++++++------
> drivers/gpu/drm/exynos/exynos_drm_hdmi.h |  3 ++-
>  drivers/gpu/drm/exynos/exynos_hdmi.c     | 18 ++++++++++++----
>  drivers/gpu/drm/exynos/exynos_mixer.c    | 35
> +++++++++++++++++++------------- 4 files changed, 66 insertions(+), 25
> deletions(-)
[snip]
> @@ -985,8 +991,7 @@ static struct exynos_mixer_ops mixer_ops = {
> 
>  static irqreturn_t mixer_irq_handler(int irq, void *arg)
>  {
> -	struct exynos_drm_hdmi_context *drm_hdmi_ctx = arg;
> -	struct mixer_context *ctx = drm_hdmi_ctx->ctx;
> +	struct mixer_context *ctx = arg;
>  	struct mixer_resources *res = &ctx->mixer_res;
>  	u32 val, base, shadow;
> 
> @@ -995,6 +1000,9 @@ static irqreturn_t mixer_irq_handler(int irq, void
> *arg) /* read interrupt status for handling and clearing flags for
> VSYNC */ val = mixer_reg_read(res, MXR_INT_STATUS);
> 
> +	if (!ctx->drm_dev)
> +		goto out;

The patch looks fine, but I'd like you to explain me in what conditions 
can this condition evaluate to true.

Best regards,
Tomasz
Sean Paul Nov. 1, 2013, 7:54 p.m. UTC | #2
On Thu, Oct 31, 2013 at 7:53 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Sean,
>
> On Tuesday 29 of October 2013 12:12:51 Sean Paul wrote:
>> This patch implements the initialize callback in the hdmi and mixer
>> manager. This allows us to get rid of drm_dev in the drm_hdmi level and
>> track it in the mixer and hdmi drivers. This is one of the things
>> holding back the complete removal of the drm_hdmi layer.
>>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> ---
>>
>> Changes in v2: None
>> Changes in v3: None
>>
>>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 35
>> ++++++++++++++++++++++++++------
>> drivers/gpu/drm/exynos/exynos_drm_hdmi.h |  3 ++-
>>  drivers/gpu/drm/exynos/exynos_hdmi.c     | 18 ++++++++++++----
>>  drivers/gpu/drm/exynos/exynos_mixer.c    | 35
>> +++++++++++++++++++------------- 4 files changed, 66 insertions(+), 25
>> deletions(-)
> [snip]
>> @@ -985,8 +991,7 @@ static struct exynos_mixer_ops mixer_ops = {
>>
>>  static irqreturn_t mixer_irq_handler(int irq, void *arg)
>>  {
>> -     struct exynos_drm_hdmi_context *drm_hdmi_ctx = arg;
>> -     struct mixer_context *ctx = drm_hdmi_ctx->ctx;
>> +     struct mixer_context *ctx = arg;
>>       struct mixer_resources *res = &ctx->mixer_res;
>>       u32 val, base, shadow;
>>
>> @@ -995,6 +1000,9 @@ static irqreturn_t mixer_irq_handler(int irq, void
>> *arg) /* read interrupt status for handling and clearing flags for
>> VSYNC */ val = mixer_reg_read(res, MXR_INT_STATUS);
>>
>> +     if (!ctx->drm_dev)
>> +             goto out;
>
> The patch looks fine, but I'd like you to explain me in what conditions
> can this condition evaluate to true.
>

This can happen if there's a mixer interrupt before the intialize()
hook is called.

Sean


> Best regards,
> Tomasz
>
Tomasz Figa Nov. 1, 2013, 7:56 p.m. UTC | #3
Hi Sean,

On Friday 01 of November 2013 15:54:31 Sean Paul wrote:
> On Thu, Oct 31, 2013 at 7:53 PM, Tomasz Figa <tomasz.figa@gmail.com> 
wrote:
> > Hi Sean,
> > 
> > On Tuesday 29 of October 2013 12:12:51 Sean Paul wrote:
[snip]
> >> @@ -995,6 +1000,9 @@ static irqreturn_t mixer_irq_handler(int irq,
> >> void
> >> *arg) /* read interrupt status for handling and clearing flags for
> >> VSYNC */ val = mixer_reg_read(res, MXR_INT_STATUS);
> >> 
> >> +     if (!ctx->drm_dev)
> >> +             goto out;
> > 
> > The patch looks fine, but I'd like you to explain me in what
> > conditions
> > can this condition evaluate to true.
> 
> This can happen if there's a mixer interrupt before the intialize()
> hook is called.

What about making the driver enable the interrupt (or even all the 
hardware) after this hook is called then?

Best regards,
Tomasz
Sean Paul Nov. 1, 2013, 8:08 p.m. UTC | #4
On Fri, Nov 1, 2013 at 3:56 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Sean,
>
> On Friday 01 of November 2013 15:54:31 Sean Paul wrote:
>> On Thu, Oct 31, 2013 at 7:53 PM, Tomasz Figa <tomasz.figa@gmail.com>
> wrote:
>> > Hi Sean,
>> >
>> > On Tuesday 29 of October 2013 12:12:51 Sean Paul wrote:
> [snip]
>> >> @@ -995,6 +1000,9 @@ static irqreturn_t mixer_irq_handler(int irq,
>> >> void
>> >> *arg) /* read interrupt status for handling and clearing flags for
>> >> VSYNC */ val = mixer_reg_read(res, MXR_INT_STATUS);
>> >>
>> >> +     if (!ctx->drm_dev)
>> >> +             goto out;
>> >
>> > The patch looks fine, but I'd like you to explain me in what
>> > conditions
>> > can this condition evaluate to true.
>>
>> This can happen if there's a mixer interrupt before the intialize()
>> hook is called.
>
> What about making the driver enable the interrupt (or even all the
> hardware) after this hook is called then?
>

Sure, I can do that. This is one of the reasons that a unified driver
model would be useful.

Sean



> Best regards,
> Tomasz
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
index a1ef3c9..aebcc0e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
@@ -97,6 +97,18 @@  void exynos_mixer_ops_register(struct exynos_mixer_ops *ops)
 		mixer_ops = ops;
 }
 
+static int drm_hdmi_display_initialize(struct device *dev,
+		struct drm_device *drm_dev)
+{
+	struct drm_hdmi_context *ctx = to_context(dev);
+
+	if (hdmi_ops && hdmi_ops->initialize)
+		return hdmi_ops->initialize(ctx->hdmi_ctx->ctx, drm_dev);
+
+	return 0;
+}
+
+
 static bool drm_hdmi_is_connected(struct device *dev)
 {
 	struct drm_hdmi_context *ctx = to_context(dev);
@@ -153,6 +165,7 @@  static int drm_hdmi_power_on(struct device *dev, int mode)
 
 static struct exynos_drm_display_ops drm_hdmi_display_ops = {
 	.type = EXYNOS_DISPLAY_TYPE_HDMI,
+	.initialize = drm_hdmi_display_initialize,
 	.is_connected = drm_hdmi_is_connected,
 	.get_edid = drm_hdmi_get_edid,
 	.check_mode = drm_hdmi_check_mode,
@@ -257,6 +270,21 @@  static void drm_hdmi_commit(struct device *subdrv_dev)
 		hdmi_ops->commit(ctx->hdmi_ctx->ctx);
 }
 
+static int drm_hdmi_mgr_initialize(struct device *subdrv_dev,
+		struct drm_device *drm_dev)
+{
+	struct drm_hdmi_context *ctx = to_context(subdrv_dev);
+	int ret = 0;
+
+	if (mixer_ops && mixer_ops->initialize)
+		ret = mixer_ops->initialize(ctx->mixer_ctx->ctx, drm_dev);
+
+	if (mixer_ops->iommu_on)
+		mixer_ops->iommu_on(ctx->mixer_ctx->ctx, true);
+
+	return ret;
+}
+
 static void drm_hdmi_dpms(struct device *subdrv_dev, int mode)
 {
 	struct drm_hdmi_context *ctx = to_context(subdrv_dev);
@@ -326,6 +354,7 @@  static void drm_mixer_win_disable(struct device *subdrv_dev, int zpos)
 }
 
 static struct exynos_drm_manager_ops drm_hdmi_manager_ops = {
+	.initialize = drm_hdmi_mgr_initialize,
 	.dpms = drm_hdmi_dpms,
 	.apply = drm_hdmi_apply,
 	.enable_vblank = drm_hdmi_enable_vblank,
@@ -372,12 +401,6 @@  static int hdmi_subdrv_probe(struct drm_device *drm_dev,
 	ctx->hdmi_ctx = hdmi_ctx;
 	ctx->mixer_ctx = mixer_ctx;
 
-	ctx->hdmi_ctx->drm_dev = drm_dev;
-	ctx->mixer_ctx->drm_dev = drm_dev;
-
-	if (mixer_ops->iommu_on)
-		mixer_ops->iommu_on(ctx->mixer_ctx->ctx, true);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
index 724cab1..cf7b1da 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
@@ -23,12 +23,12 @@ 
  *	this context should be hdmi_context or mixer_context.
  */
 struct exynos_drm_hdmi_context {
-	struct drm_device	*drm_dev;
 	void			*ctx;
 };
 
 struct exynos_hdmi_ops {
 	/* display */
+	int (*initialize)(void *ctx, struct drm_device *drm_dev);
 	bool (*is_connected)(void *ctx);
 	struct edid *(*get_edid)(void *ctx,
 			struct drm_connector *connector);
@@ -45,6 +45,7 @@  struct exynos_hdmi_ops {
 
 struct exynos_mixer_ops {
 	/* manager */
+	int (*initialize)(void *ctx, struct drm_device *drm_dev);
 	int (*iommu_on)(void *ctx, bool enable);
 	int (*enable_vblank)(void *ctx, int pipe);
 	void (*disable_vblank)(void *ctx);
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index fcfa23a..00704e9 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -741,6 +741,15 @@  static void hdmi_reg_infoframe(struct hdmi_context *hdata,
 	}
 }
 
+static int hdmi_initialize(void *ctx, struct drm_device *drm_dev)
+{
+	struct hdmi_context *hdata = ctx;
+
+	hdata->drm_dev = drm_dev;
+
+	return 0;
+}
+
 static bool hdmi_is_connected(void *ctx)
 {
 	struct hdmi_context *hdata = ctx;
@@ -1746,6 +1755,7 @@  static void hdmi_dpms(void *ctx, int mode)
 
 static struct exynos_hdmi_ops hdmi_ops = {
 	/* display */
+	.initialize	= hdmi_initialize,
 	.is_connected	= hdmi_is_connected,
 	.get_edid	= hdmi_get_edid,
 	.check_mode	= hdmi_check_mode,
@@ -1766,8 +1776,8 @@  static irqreturn_t hdmi_irq_thread(int irq, void *arg)
 	hdata->hpd = gpio_get_value(hdata->hpd_gpio);
 	mutex_unlock(&hdata->hdmi_mutex);
 
-	if (ctx->drm_dev)
-		drm_helper_hpd_irq_event(ctx->drm_dev);
+	if (hdata->drm_dev)
+		drm_helper_hpd_irq_event(hdata->drm_dev);
 
 	return IRQ_HANDLED;
 }
@@ -2020,8 +2030,8 @@  static int hdmi_suspend(struct device *dev)
 	disable_irq(hdata->irq);
 
 	hdata->hpd = false;
-	if (ctx->drm_dev)
-		drm_helper_hpd_irq_event(ctx->drm_dev);
+	if (hdata->drm_dev)
+		drm_helper_hpd_irq_event(hdata->drm_dev);
 
 	if (pm_runtime_suspended(dev)) {
 		DRM_DEBUG_KMS("Already suspended\n");
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 9634188..2f204c1 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -685,20 +685,25 @@  static void mixer_win_reset(struct mixer_context *ctx)
 	spin_unlock_irqrestore(&res->reg_slock, flags);
 }
 
+static int mixer_initialize(void *ctx, struct drm_device *drm_dev)
+{
+	struct mixer_context *mixer_ctx = ctx;
+
+	mixer_ctx->drm_dev = drm_dev;
+
+	return 0;
+}
+
 static int mixer_iommu_on(void *ctx, bool enable)
 {
-	struct exynos_drm_hdmi_context *drm_hdmi_ctx;
 	struct mixer_context *mdata = ctx;
-	struct drm_device *drm_dev;
-
-	drm_hdmi_ctx = mdata->parent_ctx;
-	drm_dev = drm_hdmi_ctx->drm_dev;
 
-	if (is_drm_iommu_supported(drm_dev)) {
+	if (is_drm_iommu_supported(mdata->drm_dev)) {
 		if (enable)
-			return drm_iommu_attach_device(drm_dev, mdata->dev);
+			return drm_iommu_attach_device(mdata->drm_dev,
+					mdata->dev);
 
-		drm_iommu_detach_device(drm_dev, mdata->dev);
+		drm_iommu_detach_device(mdata->drm_dev, mdata->dev);
 	}
 	return 0;
 }
@@ -970,6 +975,7 @@  static void mixer_dpms(void *ctx, int mode)
 
 static struct exynos_mixer_ops mixer_ops = {
 	/* manager */
+	.initialize		= mixer_initialize,
 	.iommu_on		= mixer_iommu_on,
 	.enable_vblank		= mixer_enable_vblank,
 	.disable_vblank		= mixer_disable_vblank,
@@ -985,8 +991,7 @@  static struct exynos_mixer_ops mixer_ops = {
 
 static irqreturn_t mixer_irq_handler(int irq, void *arg)
 {
-	struct exynos_drm_hdmi_context *drm_hdmi_ctx = arg;
-	struct mixer_context *ctx = drm_hdmi_ctx->ctx;
+	struct mixer_context *ctx = arg;
 	struct mixer_resources *res = &ctx->mixer_res;
 	u32 val, base, shadow;
 
@@ -995,6 +1000,9 @@  static irqreturn_t mixer_irq_handler(int irq, void *arg)
 	/* read interrupt status for handling and clearing flags for VSYNC */
 	val = mixer_reg_read(res, MXR_INT_STATUS);
 
+	if (!ctx->drm_dev)
+		goto out;
+
 	/* handling VSYNC */
 	if (val & MXR_INT_STATUS_VSYNC) {
 		/* interlace scan need to check shadow register */
@@ -1010,9 +1018,8 @@  static irqreturn_t mixer_irq_handler(int irq, void *arg)
 				goto out;
 		}
 
-		drm_handle_vblank(drm_hdmi_ctx->drm_dev, ctx->pipe);
-		exynos_drm_crtc_finish_pageflip(drm_hdmi_ctx->drm_dev,
-				ctx->pipe);
+		drm_handle_vblank(ctx->drm_dev, ctx->pipe);
+		exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
 
 		/* set wait vsync event to zero and wake up queue. */
 		if (atomic_read(&ctx->wait_vsync_event)) {
@@ -1077,7 +1084,7 @@  static int mixer_resources_init(struct exynos_drm_hdmi_context *ctx,
 	}
 
 	ret = devm_request_irq(dev, res->start, mixer_irq_handler,
-							0, "drm_mixer", ctx);
+						0, "drm_mixer", mixer_ctx);
 	if (ret) {
 		dev_err(dev, "request interrupt failed.\n");
 		return ret;