[PATCH v3 05/32] drm/exynos: hdmi: Implement initialize op for hdmi
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 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
[PATCH v3 05/32] drm/exynos: hdmi: Implement initialize op for hdmi
On Fri, Nov 1, 2013 at 3:56 PM, Tomasz Figa 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 > 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 >
[PATCH v3 05/32] drm/exynos: hdmi: Implement initialize op for hdmi
On Thu, Oct 31, 2013 at 7:53 PM, Tomasz Figa 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
>> ---
>>
>> 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 = >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
>
[PATCH v3 05/32] drm/exynos: hdmi: Implement initialize op for hdmi
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
> ---
>
> 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 = >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
[PATCH v3 05/32] drm/exynos: hdmi: Implement initialize op for hdmi
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
---
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(-)
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,
