[PATCH 1/3] drm/omap: fix missing disable for unused encoder

[PATCH 1/3] drm/omap: fix missing disable for unused encoder

[Tomi Valkeinen]

When an encoder is no longer connected to a crtc, the driver will leave
the encoder enabled.

This patch adds code to track the encoder used for a crtc, and when the
encoder changes, the old one is disabled.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 4f624c59a660..beccff2ccf84 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -33,6 +33,7 @@ struct omap_crtc {
 	int pipe;
 	enum omap_channel channel;
 	struct omap_overlay_manager_info info;
+	struct drm_encoder *current_encoder;
 
 	/*
 	 * Temporary: eventually this will go away, but it is needed
@@ -593,6 +594,11 @@ static void omap_crtc_pre_apply(struct omap_drm_apply *apply)
 		}
 	}
 
+	if (omap_crtc->current_encoder && encoder != omap_crtc->current_encoder)
+		omap_encoder_set_enabled(omap_crtc->current_encoder, false);
+
+	omap_crtc->current_encoder = encoder;
+
 	if (!omap_crtc->enabled) {
 		set_enabled(&omap_crtc->base, false);
 		if (encoder)
-- 
1.8.3.2

[PATCH 2/3] drm/omap: fix enabling/disabling of video pipeline

[Tomi Valkeinen]

At the moment the omap_crtc_pre_apply() handles the enabling, disabling
and configuring of encoders and panels separately from the CRTC (i.e.
the overlay manager).

However, this doesn't work correctly. The encoder driver has to be in
control of its video input (i.e. the crtc) for correct operation.

This problem causes bugs with (at least) HDMI: the HDMI encoder supplies
pixel clock for DISPC, and DISPC supplies video stream for HDMI. The
current code first enables the HDMI encoder, and CRTC after that.
However, the encoder expects the video stream to start during the
encoder's enable, and if it doesn't, there will be sync lost errors.

The encoder enables its video source by calling src->enable(), and this
call goes to omapdrm (omap_crtc_enable), but omapdrm doesn't do anything
in that function. Similarly for disable, which goes to
omap_crtc_disable().

This patch moves the code to setup and enable/disable the crtc to
omap_crtc_enable. and omap_crtc_disable().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index beccff2ccf84..90916abb66ef 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -121,13 +121,25 @@ static void omap_crtc_start_update(struct omap_overlay_manager *mgr)
 {
 }
 
+static void set_enabled(struct drm_crtc *crtc, bool enable);
+
 static int omap_crtc_enable(struct omap_overlay_manager *mgr)
 {
+	struct omap_crtc *omap_crtc = omap_crtcs[mgr->id];
+
+	dispc_mgr_setup(omap_crtc->channel, &omap_crtc->info);
+	dispc_mgr_set_timings(omap_crtc->channel,
+			&omap_crtc->timings);
+	set_enabled(&omap_crtc->base, true);
+
 	return 0;
 }
 
 static void omap_crtc_disable(struct omap_overlay_manager *mgr)
 {
+	struct omap_crtc *omap_crtc = omap_crtcs[mgr->id];
+
+	set_enabled(&omap_crtc->base, false);
 }
 
 static void omap_crtc_set_timings(struct omap_overlay_manager *mgr,
@@ -600,7 +612,6 @@ static void omap_crtc_pre_apply(struct omap_drm_apply *apply)
 	omap_crtc->current_encoder = encoder;
 
 	if (!omap_crtc->enabled) {
-		set_enabled(&omap_crtc->base, false);
 		if (encoder)
 			omap_encoder_set_enabled(encoder, false);
 	} else {
@@ -609,13 +620,7 @@ static void omap_crtc_pre_apply(struct omap_drm_apply *apply)
 			omap_encoder_update(encoder, omap_crtc->mgr,
 					&omap_crtc->timings);
 			omap_encoder_set_enabled(encoder, true);
-			omap_crtc->full_update = false;
 		}
-
-		dispc_mgr_setup(omap_crtc->channel, &omap_crtc->info);
-		dispc_mgr_set_timings(omap_crtc->channel,
-				&omap_crtc->timings);
-		set_enabled(&omap_crtc->base, true);
 	}
 
 	omap_crtc->full_update = false;
-- 
1.8.3.2

Re: [PATCH 2/3] drm/omap: fix enabling/disabling of video pipeline

[Rob Clark]

On Thu, Apr 3, 2014 at 9:45 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> At the moment the omap_crtc_pre_apply() handles the enabling, disabling
> and configuring of encoders and panels separately from the CRTC (i.e.
> the overlay manager).
>
> However, this doesn't work correctly. The encoder driver has to be in
> control of its video input (i.e. the crtc) for correct operation.
>
> This problem causes bugs with (at least) HDMI: the HDMI encoder supplies
> pixel clock for DISPC, and DISPC supplies video stream for HDMI. The
> current code first enables the HDMI encoder, and CRTC after that.
> However, the encoder expects the video stream to start during the
> encoder's enable, and if it doesn't, there will be sync lost errors.
>
> The encoder enables its video source by calling src->enable(), and this
> call goes to omapdrm (omap_crtc_enable), but omapdrm doesn't do anything
> in that function. Similarly for disable, which goes to
> omap_crtc_disable().
>
> This patch moves the code to setup and enable/disable the crtc to
> omap_crtc_enable. and omap_crtc_disable().
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

I had to go back to look at the code to realize the extra
'omap_crtc->full_update = false' removal didn't actually matter (it
was redundant).  So

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index beccff2ccf84..90916abb66ef 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -121,13 +121,25 @@ static void omap_crtc_start_update(struct omap_overlay_manager *mgr)
>  {
>  }
>
> +static void set_enabled(struct drm_crtc *crtc, bool enable);
> +
>  static int omap_crtc_enable(struct omap_overlay_manager *mgr)
>  {
> +       struct omap_crtc *omap_crtc = omap_crtcs[mgr->id];
> +
> +       dispc_mgr_setup(omap_crtc->channel, &omap_crtc->info);
> +       dispc_mgr_set_timings(omap_crtc->channel,
> +                       &omap_crtc->timings);
> +       set_enabled(&omap_crtc->base, true);
> +
>         return 0;
>  }
>
>  static void omap_crtc_disable(struct omap_overlay_manager *mgr)
>  {
> +       struct omap_crtc *omap_crtc = omap_crtcs[mgr->id];
> +
> +       set_enabled(&omap_crtc->base, false);
>  }
>
>  static void omap_crtc_set_timings(struct omap_overlay_manager *mgr,
> @@ -600,7 +612,6 @@ static void omap_crtc_pre_apply(struct omap_drm_apply *apply)
>         omap_crtc->current_encoder = encoder;
>
>         if (!omap_crtc->enabled) {
> -               set_enabled(&omap_crtc->base, false);
>                 if (encoder)
>                         omap_encoder_set_enabled(encoder, false);
>         } else {
> @@ -609,13 +620,7 @@ static void omap_crtc_pre_apply(struct omap_drm_apply *apply)
>                         omap_encoder_update(encoder, omap_crtc->mgr,
>                                         &omap_crtc->timings);
>                         omap_encoder_set_enabled(encoder, true);
> -                       omap_crtc->full_update = false;
>                 }
> -
> -               dispc_mgr_setup(omap_crtc->channel, &omap_crtc->info);
> -               dispc_mgr_set_timings(omap_crtc->channel,
> -                               &omap_crtc->timings);
> -               set_enabled(&omap_crtc->base, true);
>         }
>
>         omap_crtc->full_update = false;
> --
> 1.8.3.2
>

Re: [PATCH 2/3] drm/omap: fix enabling/disabling of video pipeline

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 3986 bytes --]

On 03/04/14 17:58, Rob Clark wrote:
> On Thu, Apr 3, 2014 at 9:45 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> At the moment the omap_crtc_pre_apply() handles the enabling, disabling
>> and configuring of encoders and panels separately from the CRTC (i.e.
>> the overlay manager).
>>
>> However, this doesn't work correctly. The encoder driver has to be in
>> control of its video input (i.e. the crtc) for correct operation.
>>
>> This problem causes bugs with (at least) HDMI: the HDMI encoder supplies
>> pixel clock for DISPC, and DISPC supplies video stream for HDMI. The
>> current code first enables the HDMI encoder, and CRTC after that.
>> However, the encoder expects the video stream to start during the
>> encoder's enable, and if it doesn't, there will be sync lost errors.
>>
>> The encoder enables its video source by calling src->enable(), and this
>> call goes to omapdrm (omap_crtc_enable), but omapdrm doesn't do anything
>> in that function. Similarly for disable, which goes to
>> omap_crtc_disable().
>>
>> This patch moves the code to setup and enable/disable the crtc to
>> omap_crtc_enable. and omap_crtc_disable().
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> I had to go back to look at the code to realize the extra
> 'omap_crtc->full_update = false' removal didn't actually matter (it
> was redundant).  So

The final 'omap_crtc->full_update = false;' is visible in the patch
below also, so you didn't need to go look at the code ;)

Looking at it, that removal is actually extra, not exactly part of this fix.

> Reviewed-by: Rob Clark <robdclark@gmail.com>

Thanks!

 Tom

> 
>> ---
>>  drivers/gpu/drm/omapdrm/omap_crtc.c | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> index beccff2ccf84..90916abb66ef 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>> @@ -121,13 +121,25 @@ static void omap_crtc_start_update(struct omap_overlay_manager *mgr)
>>  {
>>  }
>>
>> +static void set_enabled(struct drm_crtc *crtc, bool enable);
>> +
>>  static int omap_crtc_enable(struct omap_overlay_manager *mgr)
>>  {
>> +       struct omap_crtc *omap_crtc = omap_crtcs[mgr->id];
>> +
>> +       dispc_mgr_setup(omap_crtc->channel, &omap_crtc->info);
>> +       dispc_mgr_set_timings(omap_crtc->channel,
>> +                       &omap_crtc->timings);
>> +       set_enabled(&omap_crtc->base, true);
>> +
>>         return 0;
>>  }
>>
>>  static void omap_crtc_disable(struct omap_overlay_manager *mgr)
>>  {
>> +       struct omap_crtc *omap_crtc = omap_crtcs[mgr->id];
>> +
>> +       set_enabled(&omap_crtc->base, false);
>>  }
>>
>>  static void omap_crtc_set_timings(struct omap_overlay_manager *mgr,
>> @@ -600,7 +612,6 @@ static void omap_crtc_pre_apply(struct omap_drm_apply *apply)
>>         omap_crtc->current_encoder = encoder;
>>
>>         if (!omap_crtc->enabled) {
>> -               set_enabled(&omap_crtc->base, false);
>>                 if (encoder)
>>                         omap_encoder_set_enabled(encoder, false);
>>         } else {
>> @@ -609,13 +620,7 @@ static void omap_crtc_pre_apply(struct omap_drm_apply *apply)
>>                         omap_encoder_update(encoder, omap_crtc->mgr,
>>                                         &omap_crtc->timings);
>>                         omap_encoder_set_enabled(encoder, true);
>> -                       omap_crtc->full_update = false;
>>                 }
>> -
>> -               dispc_mgr_setup(omap_crtc->channel, &omap_crtc->info);
>> -               dispc_mgr_set_timings(omap_crtc->channel,
>> -                               &omap_crtc->timings);
>> -               set_enabled(&omap_crtc->base, true);
>>         }
>>
>>         omap_crtc->full_update = false;
>> --
>> 1.8.3.2
>>



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] OMAPDSS: HDMI: fix interlace output

[Tomi Valkeinen]

The HDMI output video format's yres needs to be divided by two for
interlace. Fix it.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/hdmi_wp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/omap2/dss/hdmi_wp.c b/drivers/video/omap2/dss/hdmi_wp.c
index cd620c6e43a0..f5f4ccf50d90 100644
--- a/drivers/video/omap2/dss/hdmi_wp.c
+++ b/drivers/video/omap2/dss/hdmi_wp.c
@@ -171,6 +171,8 @@ void hdmi_wp_init_vid_fmt_timings(struct hdmi_video_format *video_fmt,
 	video_fmt->packing_mode = HDMI_PACK_10b_RGB_YUV444;
 	video_fmt->y_res = param->timings.y_res;
 	video_fmt->x_res = param->timings.x_res;
+	if (param->timings.interlace)
+		video_fmt->y_res /= 2;
 
 	timings->hbp = param->timings.hbp;
 	timings->hfp = param->timings.hfp;
-- 
1.8.3.2

Re: [PATCH 1/3] drm/omap: fix missing disable for unused encoder

[Rob Clark]

On Thu, Apr 3, 2014 at 9:45 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> When an encoder is no longer connected to a crtc, the driver will leave
> the encoder enabled.
>
> This patch adds code to track the encoder used for a crtc, and when the
> encoder changes, the old one is disabled.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>


> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 4f624c59a660..beccff2ccf84 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -33,6 +33,7 @@ struct omap_crtc {
>         int pipe;
>         enum omap_channel channel;
>         struct omap_overlay_manager_info info;
> +       struct drm_encoder *current_encoder;
>
>         /*
>          * Temporary: eventually this will go away, but it is needed
> @@ -593,6 +594,11 @@ static void omap_crtc_pre_apply(struct omap_drm_apply *apply)
>                 }
>         }
>
> +       if (omap_crtc->current_encoder && encoder != omap_crtc->current_encoder)
> +               omap_encoder_set_enabled(omap_crtc->current_encoder, false);
> +
> +       omap_crtc->current_encoder = encoder;
> +
>         if (!omap_crtc->enabled) {
>                 set_enabled(&omap_crtc->base, false);
>                 if (encoder)
> --
> 1.8.3.2
>