* Re: [PATCH] drm: rcar-du: Re-update the DSYSR register value for start/stop
2018-10-22 11:23 ` Laurent Pinchart
@ 2018-10-23 1:01 ` Hoan
2018-11-22 23:29 ` Laurent Pinchart
2018-10-23 10:10 ` Hoan
2018-10-24 0:16 ` Hoan
2 siblings, 1 reply; 8+ messages in thread
From: Hoan @ 2018-10-23 1:01 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-renesas-soc, geert+renesas, kuninori.morimoto.gx,
yoshihiro.shimoda.uh, h-inayoshi, nv-dung, cv-dong
Dear Laurent-san
Thank you for your reply and comments!
On 2018/10/22 20:23, Laurent Pinchart wrote:
> Hello Hoan,
>
> Thank you for the patch.
>
> On Monday, 22 October 2018 09:30:54 EEST Nguyen An Hoan wrote:
>> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
>>
>> From previous commit 0521ccb "drm: rcar-du: Cache DSYSR value to ensure
>> known initial value"
> What exact commit are you referring to ? The mainline commit that has this
> subject is 9144adc5e5a99577bce0d4ee2ca3615f53b9d296.
Seems I have cited the wrong Commit-ID、it is
9144adc5e5a99577bce0d4ee2ca3615f53b9d296
drm: rcar-du: Cache DSYSR value to ensure known initial value
>> We only need to update DSYSR0, DSYSR2 for start/stop.
>> So using rgrp-> mmio_offset is enough, the change back from rcar_du_crtc ->
>> rcar_du_group -> rcar_du_crtc leading to mmio addresses for DSYSR may be
>> different.
> Is this fixing an actual problem ? If you look at the code, the line
>
> struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
>
> makes sure that we select DU0 or DU2 only, so the register write
>
> rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
> start ? DSYSR_DEN : DSYSR_DRES);
>
> should only access DSYSR0 and DSYSR2.
This seems I think to fix the rcar-du problem with M3N-r8a77965,
With M3N-R8a77965 we have DU0, DU1, DU3
So, when Laurent-san divide objetcs into rcar_du_group, rcar_du_crtc.
DU0, DU1 -> du_group[0] rgrp-> mmio_offset = DU0_REG_OFFSET
DU3->du_group[1] and rgrp-> mmio_offset = DU2_REG_OFFSET, but
rcrtc->mmio_offset=DU3_REG_OFFSET (with M3N)
M3N-R8a77965 not have DU2, So after the command:
struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
So in fact, with M3N we are updating DSYSR3 (In this my TC, this
reference to start/stop DU3-RGB)
This will not affect H3, since the H3 lines always have enough DU0,
DU1,DU2,DU3.
Thank you very much !
Hoan.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: rcar-du: Re-update the DSYSR register value for start/stop
2018-10-23 1:01 ` Hoan
@ 2018-11-22 23:29 ` Laurent Pinchart
0 siblings, 0 replies; 8+ messages in thread
From: Laurent Pinchart @ 2018-11-22 23:29 UTC (permalink / raw)
To: Hoan
Cc: linux-renesas-soc, geert+renesas, kuninori.morimoto.gx,
yoshihiro.shimoda.uh, h-inayoshi, nv-dung, cv-dong
Hello Hoan,
On Tuesday, 23 October 2018 04:01:19 EET Hoan wrote:
> On 2018/10/22 20:23, Laurent Pinchart wrote:
> > On Monday, 22 October 2018 09:30:54 EEST Nguyen An Hoan wrote:
> >> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
> >>
> >> From previous commit 0521ccb "drm: rcar-du: Cache DSYSR value to ensure
> >>
> >> known initial value"
> >
> > What exact commit are you referring to ? The mainline commit that has this
> > subject is 9144adc5e5a99577bce0d4ee2ca3615f53b9d296.
>
> Seems I have cited the wrong Commit-ID、it is
>
> 9144adc5e5a99577bce0d4ee2ca3615f53b9d296
> drm: rcar-du: Cache DSYSR value to ensure known initial value
>
> >> We only need to update DSYSR0, DSYSR2 for start/stop.
> >> So using rgrp-> mmio_offset is enough, the change back from rcar_du_crtc
> >> ->rcar_du_group -> rcar_du_crtc leading to mmio addresses for DSYSR may
> >> be different.
> >
> > Is this fixing an actual problem ? If you look at the code, the line
> >
> > struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
> >
> > makes sure that we select DU0 or DU2 only, so the register write
> >
> > rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
> > start ? DSYSR_DEN : DSYSR_DRES);
> >
> > should only access DSYSR0 and DSYSR2.
>
> This seems I think to fix the rcar-du problem with M3N-r8a77965,
>
> With M3N-R8a77965 we have DU0, DU1, DU3
> So, when Laurent-san divide objetcs into rcar_du_group, rcar_du_crtc.
>
> DU0, DU1 -> du_group[0] rgrp-> mmio_offset = DU0_REG_OFFSET
> DU3->du_group[1] and rgrp-> mmio_offset = DU2_REG_OFFSET, but
> rcrtc->mmio_offset=DU3_REG_OFFSET (with M3N)
>
> M3N-R8a77965 not have DU2, So after the command:
>
> struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
>
> So in fact, with M3N we are updating DSYSR3 (In this my TC, this
> reference to start/stop DU3-RGB)
>
> This will not affect H3, since the H3 lines always have enough DU0,
> DU1,DU2,DU3.
You're absolutely right. I'm sorry for introducing the bug in the first place,
and for failing to understand your patch.
I would however prefer a different fix, as switching back to
rcar_du_group_write() defeats the purpose of the "drm: rcar-du: Cache DSYSR
value to ensure known initial value" patch. I will submit a patch and CC you.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: rcar-du: Re-update the DSYSR register value for start/stop
2018-10-22 11:23 ` Laurent Pinchart
2018-10-23 1:01 ` Hoan
@ 2018-10-23 10:10 ` Hoan
2018-10-24 0:16 ` Hoan
2 siblings, 0 replies; 8+ messages in thread
From: Hoan @ 2018-10-23 10:10 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-renesas-soc, geert+renesas, kuninori.morimoto.gx,
yoshihiro.shimoda.uh, h-inayoshi, nv-dung, cv-dong
[-- Attachment #1: Type: text/plain, Size: 3635 bytes --]
Dear Laurent-san
On 2018/10/22 20:23, Laurent Pinchart wrote:
> Hello Hoan,
>
> Thank you for the patch.
>
> On Monday, 22 October 2018 09:30:54 EEST Nguyen An Hoan wrote:
>> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
>>
>> From previous commit 0521ccb "drm: rcar-du: Cache DSYSR value to ensure
>> known initial value"
> What exact commit are you referring to ? The mainline commit that has this
> subject is 9144adc5e5a99577bce0d4ee2ca3615f53b9d296.
>
>> We only need to update DSYSR0, DSYSR2 for start/stop.
>> So using rgrp-> mmio_offset is enough, the change back from rcar_du_crtc ->
>> rcar_du_group -> rcar_du_crtc leading to mmio addresses for DSYSR may be
>> different.
> Is this fixing an actual problem ? If you look at the code, the line
>
> struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
>
> makes sure that we select DU0 or DU2 only, so the register write
>
> rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
> start ? DSYSR_DEN : DSYSR_DRES);
>
> should only access DSYSR0 and DSYSR2.
Dear Laurent-san
I add information about the current error occurring at M3N-r8a77965
when probe rcar-du driver:
[ 13.027115] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
[CRTC:55:crtc-2] flip_done timed out
[ 23.267103] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CRTC:55:crtc-2] flip_done timed out
[ 33.507102] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CONNECTOR:57:VGA-1] flip_done timed out
[ 43.747100] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[PLANE:30:plane-1] flip_done timed out
[ 53.987100] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
[CRTC:55:crtc-2] flip_done timed out
[ 53.989913] Console: switching to colour frame buffer device 128x48
[ 64.227102] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CRTC:55:crtc-2] flip_done timed out
[ 74.467099] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CONNECTOR:57:VGA-1] flip_done timed out
[ 84.707100] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[PLANE:30:plane-1] flip_done timed out
[ 94.947100] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
[CRTC:55:crtc-2] flip_done timed out
[ 95.040076] rcar-du feb00000.display: fb0: DRM emulated frame
buffer device
[ 95.047747] [drm] Initialized rcar-du 1.0.0 20130110 for
feb00000.display on minor 0
[ 95.055512] [drm] Device feb00000.display probed
[ 95.061252] bd9571mwv 7-0030: Device: BD9571MWV rev. 4
renesas-drivers-2018-10-09-v4.19-rc7
Thank you!
Hoan.
>> Signed-off-by: Hoan Nguyen An <na-hoan@jinso.co.jp>
>> ---
>> drivers/gpu/drm/rcar-du/rcar_du_group.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_group.c index d85f0a1..a5f7eed 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
>> @@ -202,10 +202,9 @@ void rcar_du_group_put(struct rcar_du_group *rgrp)
>>
>> static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool
>> start) {
>> - struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
>> -
>> - rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
>> - start ? DSYSR_DEN : DSYSR_DRES);
>> + rcar_du_group_write(rgrp, DSYSR,
>> + (rcar_du_group_read(rgrp, DSYSR) & ~(DSYSR_DRES | DSYSR_DEN)) |
>> + (start ? DSYSR_DEN : DSYSR_DRES));
>> }
>>
>> void rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
[-- Attachment #2: Type: text/html, Size: 4948 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: rcar-du: Re-update the DSYSR register value for start/stop
2018-10-22 11:23 ` Laurent Pinchart
2018-10-23 1:01 ` Hoan
2018-10-23 10:10 ` Hoan
@ 2018-10-24 0:16 ` Hoan
2 siblings, 0 replies; 8+ messages in thread
From: Hoan @ 2018-10-24 0:16 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-renesas-soc, geert+renesas, kuninori.morimoto.gx,
yoshihiro.shimoda.uh, h-inayoshi, nv-dung, cv-dong
[-- Attachment #1: Type: text/plain, Size: 3620 bytes --]
Dear Laurent-san
On 2018/10/22 20:23, Laurent Pinchart wrote:
> Hello Hoan,
>
> Thank you for the patch.
>
> On Monday, 22 October 2018 09:30:54 EEST Nguyen An Hoan wrote:
>> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
>>
>> From previous commit 0521ccb "drm: rcar-du: Cache DSYSR value to ensure
>> known initial value"
> What exact commit are you referring to ? The mainline commit that has this
> subject is 9144adc5e5a99577bce0d4ee2ca3615f53b9d296.
>
>> We only need to update DSYSR0, DSYSR2 for start/stop.
>> So using rgrp-> mmio_offset is enough, the change back from rcar_du_crtc ->
>> rcar_du_group -> rcar_du_crtc leading to mmio addresses for DSYSR may be
>> different.
> Is this fixing an actual problem ? If you look at the code, the line
>
> struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
>
> makes sure that we select DU0 or DU2 only, so the register write
>
> rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
> start ? DSYSR_DEN : DSYSR_DRES);
>
> should only access DSYSR0 and DSYSR2.
Dear Laurent-san
I add information about the current error occurring at M3N-r8a77965
when probe rcar-du driver:
/[ 13.027115] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
[CRTC:55:crtc-2] flip_done timed out//
//[ 23.267103] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CRTC:55:crtc-2] flip_done timed out//
//[ 33.507102] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CONNECTOR:57:VGA-1] flip_done timed out//
//[ 43.747100] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[PLANE:30:plane-1] flip_done timed out//
//[ 53.987100] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
[CRTC:55:crtc-2] flip_done timed out//
//[ 53.989913] Console: switching to colour frame buffer device 128x48//
//[ 64.227102] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CRTC:55:crtc-2] flip_done timed out//
//[ 74.467099] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[CONNECTOR:57:VGA-1] flip_done timed out//
//[ 84.707100] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR*
[PLANE:30:plane-1] flip_done timed out//
//[ 94.947100] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR*
[CRTC:55:crtc-2] flip_done timed out//
//[ 95.040076] rcar-du feb00000.display: fb0: DRM emulated frame
buffer device//
//[ 95.047747] [drm] Initialized rcar-du 1.0.0 20130110 for
feb00000.display on minor 0//
//[ 95.055512] [drm] Device feb00000.display probed//
//[ 95.061252] bd9571mwv 7-0030: Device: BD9571MWV rev. 4/
on renesas-drivers/renesas-drivers-2018-10-09-v4.19-rc7
Thank you!
Hoan.
>> Signed-off-by: Hoan Nguyen An <na-hoan@jinso.co.jp>
>> ---
>> drivers/gpu/drm/rcar-du/rcar_du_group.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_group.c index d85f0a1..a5f7eed 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
>> @@ -202,10 +202,9 @@ void rcar_du_group_put(struct rcar_du_group *rgrp)
>>
>> static void __rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool
>> start) {
>> - struct rcar_du_crtc *rcrtc = &rgrp->dev->crtcs[rgrp->index * 2];
>> -
>> - rcar_du_crtc_dsysr_clr_set(rcrtc, DSYSR_DRES | DSYSR_DEN,
>> - start ? DSYSR_DEN : DSYSR_DRES);
>> + rcar_du_group_write(rgrp, DSYSR,
>> + (rcar_du_group_read(rgrp, DSYSR) & ~(DSYSR_DRES | DSYSR_DEN)) |
>> + (start ? DSYSR_DEN : DSYSR_DRES));
>> }
>>
>> void rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start)
[-- Attachment #2: Type: text/html, Size: 5343 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread