Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Kieran Bingham]

Hi Maxime,

On 17/07/17 07:32, Maxime Ripard wrote:
> On Thu, Jul 13, 2017 at 05:25:08PM +0100, Kieran Bingham wrote:
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 82b978a5dae6..c2f382feca07 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct 
> drm_atomic_state *old_state)
>  
>   /* Apply the atomic update. */
>   drm_atomic_helper_commit_modeset_disables(dev, old_state);
> - drm_atomic_helper_commit_modeset_enables(dev, old_state);
>   drm_atomic_helper_commit_planes(dev, old_state,
>   DRM_PLANE_COMMIT_ACTIVE_ONLY);

 Except for DRM_PLANE_COMMIT_ACTIVE_ONLY, this function now looks very much 
 like
 the default drm_atomic_helper_commit_tail() code.

 Reading around other uses /variants of commit_tail() style functions in 
 other
 drivers has left me confused as to how the ordering affects things here.

 Could be worth adding a comment at least to describe why we can't use the
 default helper...
>>>
>>> Or better still ... Use Maxime's new :
>>>
>>> [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for 
>>> runtime_pm users
>>
>> Never mind - I've just looked again, and seen that this new helper function 
>> is
>> the ordering previous to *this* patch, and therefore isn't the same.
>>
>> However - it's worth noting that Maxime's patch converts this function to the
>> new helper - which contradicts this patch's motivations.
> 
> So I guess I should drop the renesas modifications in my serie then?

Yes, Please.

I think we have a few extra modifications in this function as well which will
take it further away from a default implementation.

Regards

Kieran

> 
> Maxime
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Maxime Ripard]

On Thu, Jul 13, 2017 at 05:25:08PM +0100, Kieran Bingham wrote:
> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c 
> >>> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >>> index 82b978a5dae6..c2f382feca07 100644
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >>> @@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct 
> >>> drm_atomic_state *old_state)
> >>>  
> >>>   /* Apply the atomic update. */
> >>>   drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >>> - drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >>>   drm_atomic_helper_commit_planes(dev, old_state,
> >>>   DRM_PLANE_COMMIT_ACTIVE_ONLY);
> >>
> >> Except for DRM_PLANE_COMMIT_ACTIVE_ONLY, this function now looks very much 
> >> like
> >> the default drm_atomic_helper_commit_tail() code.
> >>
> >> Reading around other uses /variants of commit_tail() style functions in 
> >> other
> >> drivers has left me confused as to how the ordering affects things here.
> >>
> >> Could be worth adding a comment at least to describe why we can't use the
> >> default helper...
> > 
> > Or better still ... Use Maxime's new :
> > 
> > [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for 
> > runtime_pm users
> 
> Never mind - I've just looked again, and seen that this new helper function is
> the ordering previous to *this* patch, and therefore isn't the same.
> 
> However - it's worth noting that Maxime's patch converts this function to the
> new helper - which contradicts this patch's motivations.

So I guess I should drop the renesas modifications in my serie then?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Kieran Bingham]

On 13/07/17 16:51, Kieran Bingham wrote:
> Hi Laurent,
> 
> I've just seen Maxime's latest series "[PATCH 0/4] drm/sun4i: Fix a register
> access bug" and it relates directly to a comment I had in this patch:
> 
> On 12/07/17 17:35, Kieran Bingham wrote:
>> Hi Laurent,
>>
>> On 28/06/17 19:50, Laurent Pinchart wrote:
>>> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
>>> start to CRTC resume") changed the order of the plane commit and CRTC
>>> enable operations to accommodate the runtime PM requirements. However,
>>> this introduced corruption in the first displayed frame, as the CRTC is
>>> now enabled without any plane configured. On Gen2 hardware the first
>>> frame will be black and likely unnoticed, but on Gen3 hardware we end up
>>> starting the display before the VSP compositor, which is more
>>> noticeable.
>>>
>>> To fix this, revert the order of the commit operations back, and handle
>>> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
>>> helper operation handlers.
>>>
>>> Signed-off-by: Laurent Pinchart 
>>
>> I only have code reduction or comment suggestions below - so either with or
>> without those changes, feel free to add my:
>>
>> Reviewed-by: Kieran Bingham 
>>
>>> ---
>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 
>>> --
>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +--
>>>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
>>>  3 files changed, 43 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c 
>>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> index 6b5219ef0ad2..76cdb88b2b8e 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> @@ -448,14 +448,8 @@ static void rcar_du_crtc_wait_page_flip(struct 
>>> rcar_du_crtc *rcrtc)
>>>   * Start/Stop and Suspend/Resume
>>>   */
>>>  
>>> -static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>>> +static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
>>>  {
>>> -   struct drm_crtc *crtc = >crtc;
>>> -   bool interlaced;
>>> -
>>> -   if (rcrtc->started)
>>> -   return;
>>> -
>>> /* Set display off and background to black */
>>> rcar_du_crtc_write(rcrtc, DOOR, DOOR_RGB(0, 0, 0));
>>> rcar_du_crtc_write(rcrtc, BPOR, BPOR_RGB(0, 0, 0));
>>> @@ -467,6 +461,18 @@ static void rcar_du_crtc_start(struct rcar_du_crtc 
>>> *rcrtc)
>>> /* Start with all planes disabled. */
>>> rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
>>>  
>>> +   /* Enable the VSP compositor. */
>>> +   if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>>> +   rcar_du_vsp_enable(rcrtc);
>>> +
>>> +   /* Turn vertical blanking interrupt reporting on. */
>>> +   drm_crtc_vblank_on(>crtc);
>>> +}
>>> +
>>> +static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>>> +{
>>> +   bool interlaced;
>>> +
>>> /* Select master sync mode. This enables display operation in master
>>
>> Are we close enough here to fix this multiline comment style ?
>> (Not worth doing unless the patch is respun for other reasons ...)
>>
>> Actually - there are a lot in this file, so it would be better to do them 
>> all in
>> one hit/patch at a point of least conflicts ...
>>
>>
>>>  * sync mode (with the HSYNC and VSYNC signals configured as outputs and
>>>  * actively driven).
>>> @@ -477,24 +483,12 @@ static void rcar_du_crtc_start(struct rcar_du_crtc 
>>> *rcrtc)
>>>  DSYSR_TVM_MASTER);
>>>  
>>> rcar_du_group_start_stop(rcrtc->group, true);
>>> -
>>> -   /* Enable the VSP compositor. */
>>> -   if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>>> -   rcar_du_vsp_enable(rcrtc);
>>> -
>>> -   /* Turn vertical blanking interrupt reporting back on. */
>>> -   drm_crtc_vblank_on(crtc);
>>> -
>>> -   rcrtc->started = true;
>>>  }
>>>  
>>>  static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>>>  {
>>> struct drm_crtc *crtc = >crtc;
>>>  
>>> -   if (!rcrtc->started)
>>> -   return;
>>> -
>>> /* Disable all planes and wait for the change to take effect. This is
>>>  * required as the DSnPR registers are updated on vblank, and no vblank
>>>  * will occur once the CRTC is stopped. Disabling planes when starting
>>> @@ -525,8 +519,6 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc 
>>> *rcrtc)
>>> rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
>>>  
>>> rcar_du_group_start_stop(rcrtc->group, false);
>>> -
>>> -   rcrtc->started = false;
>>>  }
>>>  
>>>  void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc)
>>> @@ -546,12 +538,10 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
>>> return;
>>>  
>>> rcar_du_crtc_get(rcrtc);
>>> -   rcar_du_crtc_start(rcrtc);
>>> +   rcar_du_crtc_setup(rcrtc);
>>
>> Every call to _setup is 

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Kieran Bingham]

Hi Laurent,

I've just seen Maxime's latest series "[PATCH 0/4] drm/sun4i: Fix a register
access bug" and it relates directly to a comment I had in this patch:

On 12/07/17 17:35, Kieran Bingham wrote:
> Hi Laurent,
> 
> On 28/06/17 19:50, Laurent Pinchart wrote:
>> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
>> start to CRTC resume") changed the order of the plane commit and CRTC
>> enable operations to accommodate the runtime PM requirements. However,
>> this introduced corruption in the first displayed frame, as the CRTC is
>> now enabled without any plane configured. On Gen2 hardware the first
>> frame will be black and likely unnoticed, but on Gen3 hardware we end up
>> starting the display before the VSP compositor, which is more
>> noticeable.
>>
>> To fix this, revert the order of the commit operations back, and handle
>> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
>> helper operation handlers.
>>
>> Signed-off-by: Laurent Pinchart 
> 
> I only have code reduction or comment suggestions below - so either with or
> without those changes, feel free to add my:
> 
> Reviewed-by: Kieran Bingham 
> 
>> ---
>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 
>> --
>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +--
>>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
>>  3 files changed, 43 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c 
>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> index 6b5219ef0ad2..76cdb88b2b8e 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -448,14 +448,8 @@ static void rcar_du_crtc_wait_page_flip(struct 
>> rcar_du_crtc *rcrtc)
>>   * Start/Stop and Suspend/Resume
>>   */
>>  
>> -static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>> +static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
>>  {
>> -struct drm_crtc *crtc = >crtc;
>> -bool interlaced;
>> -
>> -if (rcrtc->started)
>> -return;
>> -
>>  /* Set display off and background to black */
>>  rcar_du_crtc_write(rcrtc, DOOR, DOOR_RGB(0, 0, 0));
>>  rcar_du_crtc_write(rcrtc, BPOR, BPOR_RGB(0, 0, 0));
>> @@ -467,6 +461,18 @@ static void rcar_du_crtc_start(struct rcar_du_crtc 
>> *rcrtc)
>>  /* Start with all planes disabled. */
>>  rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
>>  
>> +/* Enable the VSP compositor. */
>> +if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>> +rcar_du_vsp_enable(rcrtc);
>> +
>> +/* Turn vertical blanking interrupt reporting on. */
>> +drm_crtc_vblank_on(>crtc);
>> +}
>> +
>> +static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
>> +{
>> +bool interlaced;
>> +
>>  /* Select master sync mode. This enables display operation in master
> 
> Are we close enough here to fix this multiline comment style ?
> (Not worth doing unless the patch is respun for other reasons ...)
> 
> Actually - there are a lot in this file, so it would be better to do them all 
> in
> one hit/patch at a point of least conflicts ...
> 
> 
>>   * sync mode (with the HSYNC and VSYNC signals configured as outputs and
>>   * actively driven).
>> @@ -477,24 +483,12 @@ static void rcar_du_crtc_start(struct rcar_du_crtc 
>> *rcrtc)
>>   DSYSR_TVM_MASTER);
>>  
>>  rcar_du_group_start_stop(rcrtc->group, true);
>> -
>> -/* Enable the VSP compositor. */
>> -if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>> -rcar_du_vsp_enable(rcrtc);
>> -
>> -/* Turn vertical blanking interrupt reporting back on. */
>> -drm_crtc_vblank_on(crtc);
>> -
>> -rcrtc->started = true;
>>  }
>>  
>>  static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>>  {
>>  struct drm_crtc *crtc = >crtc;
>>  
>> -if (!rcrtc->started)
>> -return;
>> -
>>  /* Disable all planes and wait for the change to take effect. This is
>>   * required as the DSnPR registers are updated on vblank, and no vblank
>>   * will occur once the CRTC is stopped. Disabling planes when starting
>> @@ -525,8 +519,6 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>>  rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
>>  
>>  rcar_du_group_start_stop(rcrtc->group, false);
>> -
>> -rcrtc->started = false;
>>  }
>>  
>>  void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc)
>> @@ -546,12 +538,10 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
>>  return;
>>  
>>  rcar_du_crtc_get(rcrtc);
>> -rcar_du_crtc_start(rcrtc);
>> +rcar_du_crtc_setup(rcrtc);
> 
> Every call to _setup is immediately prefixed by a call to _get()
> 
> Could the _get() be done in the _setup() for code reduction?
> 
> I'm entirely open to 

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Laurent Pinchart]

Hi Kieran,

On Wednesday 12 Jul 2017 17:35:53 Kieran Bingham wrote:
> On 28/06/17 19:50, Laurent Pinchart wrote:
> > Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> > start to CRTC resume") changed the order of the plane commit and CRTC
> > enable operations to accommodate the runtime PM requirements. However,
> > this introduced corruption in the first displayed frame, as the CRTC is
> > now enabled without any plane configured. On Gen2 hardware the first
> > frame will be black and likely unnoticed, but on Gen3 hardware we end up
> > starting the display before the VSP compositor, which is more
> > noticeable.
> > 
> > To fix this, revert the order of the commit operations back, and handle
> > runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> > helper operation handlers.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> I only have code reduction or comment suggestions below - so either with or
> without those changes, feel free to add my:
> 
> Reviewed-by: Kieran Bingham 
> 
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 +
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +--
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
> >  3 files changed, 43 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 6b5219ef0ad2..76cdb88b2b8e
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c

[snip]

> > @@ -467,6 +461,18 @@ static void rcar_du_crtc_start(struct rcar_du_crtc
> > *rcrtc)
> > /* Start with all planes disabled. */
> > rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
> > 
> > +   /* Enable the VSP compositor. */
> > +   if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> > +   rcar_du_vsp_enable(rcrtc);
> > +
> > +   /* Turn vertical blanking interrupt reporting on. */
> > +   drm_crtc_vblank_on(>crtc);
> > +}
> > +
> > +static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
> > +{
> > +   bool interlaced;
> > +
> > /* Select master sync mode. This enables display operation in master
> 
> Are we close enough here to fix this multiline comment style ?
> (Not worth doing unless the patch is respun for other reasons ...)
> 
> Actually - there are a lot in this file, so it would be better to do them
> all in one hit/patch at a point of least conflicts ...

Done :-) I actually had such a patch in my tree before receiving your comment.

> >  * sync mode (with the HSYNC and VSYNC signals configured as outputs and
> >  * actively driven).

[snip]

> > @@ -546,12 +538,10 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
> > return;
> > 
> > rcar_du_crtc_get(rcrtc);
> > -   rcar_du_crtc_start(rcrtc);
> > +   rcar_du_crtc_setup(rcrtc);
> 
> Every call to _setup is immediately prefixed by a call to _get()
> 
> Could the _get() be done in the _setup() for code reduction?
> 
> I'm entirely open to that not happening here as it might be preferred to
> keep the _get() and _start() separate for style purposes.

Please see below.

> > /* Commit the planes state. */
> > -   if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> > -   rcar_du_vsp_enable(rcrtc);
> > -   } else {
> > +   if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> > for (i = 0; i < rcrtc->group->num_planes; ++i) {
> > struct rcar_du_plane *plane = >group->planes[i];
> > 

[snip]

> > @@ -601,6 +602,19 @@ static void rcar_du_crtc_atomic_begin(struct drm_crtc
> > *crtc,
> >  {
> > struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> > 
> > +   WARN_ON(!crtc->state->enable);
> 
> Is this necessary if it's handled by the rcrtc->initialized flag? or is it
> so that we find out if it happens ?
> 
> (Or is this due to the re-ordering of the _commit_tail() function below?)

It's to find out whether it happens. Before this patch the plane update
occurred after enabling the CRTC (through
drm_atomic_helper_commit_modeset_enables()). With this patch the CRTC can be
disabled at the hardware level, but only if it will be enabled right after
plane update. The state->enable flag should thus always be true, and I added
a WARN_ON to ensure that. I'd like to keep it a bit, we can remove it after
running more tests.

> > +
> > +   /*
> > +* If a mode set is in progress we can be called with the CRTC disabled.
> > +* We then need to first setup the CRTC in order to configure planes.
> > +* The .atomic_enable() handler will notice and skip the CRTC setup.
> > +*/
> 
> I'm assuming this comment is the reason for the WARN_ON above ...
> 
> > +   if (!rcrtc->initialized) {
> > +   rcar_du_crtc_get(rcrtc);
> > +   rcar_du_crtc_setup(rcrtc);
> > +  

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Laurent Pinchart]

Hi Kieran,

On Thursday 13 Jul 2017 16:51:18 Kieran Bingham wrote:
> Hi Laurent,
> 
> I've just seen Maxime's latest series "[PATCH 0/4] drm/sun4i: Fix a register
> access bug" and it relates directly to a comment I had in this patch:
> On 12/07/17 17:35, Kieran Bingham wrote:
>
> > On 28/06/17 19:50, Laurent Pinchart wrote:
> >> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> >> start to CRTC resume") changed the order of the plane commit and CRTC
> >> enable operations to accommodate the runtime PM requirements. However,
> >> this introduced corruption in the first displayed frame, as the CRTC is
> >> now enabled without any plane configured. On Gen2 hardware the first
> >> frame will be black and likely unnoticed, but on Gen3 hardware we end up
> >> starting the display before the VSP compositor, which is more
> >> noticeable.
> >> 
> >> To fix this, revert the order of the commit operations back, and handle
> >> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> >> helper operation handlers.
> >> 
> >> Signed-off-by: Laurent Pinchart
> >> 
> > 
> > I only have code reduction or comment suggestions below - so either with
> > or without those changes, feel free to add my:
> > 
> > Reviewed-by: Kieran Bingham 
> > 
> >> ---
> >> 
> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 +---
> >>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +--
> >>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
> >>  3 files changed, 43 insertions(+), 29 deletions(-)

[snip]

> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 82b978a5dae6..c2f382feca07
> >> 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> >> @@ -255,9 +255,9 @@ static void rcar_du_atomic_commit_tail(struct
> >> drm_atomic_state *old_state)>> 
> >>/* Apply the atomic update. */
> >>drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >> -  drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >>drm_atomic_helper_commit_planes(dev, old_state,
> >>DRM_PLANE_COMMIT_ACTIVE_ONLY);
> > 
> > Except for DRM_PLANE_COMMIT_ACTIVE_ONLY, this function now looks very much
> > like the default drm_atomic_helper_commit_tail() code.
> > 
> > Reading around other uses /variants of commit_tail() style functions in
> > other drivers has left me confused as to how the ordering affects things
> > here.
> > 
> > Could be worth adding a comment at least to describe why we can't use the
> > default helper...
> 
> Or better still ... Use Maxime's new :
> 
> [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for
> runtime_pm users

Note that Maxime's patch implements the commit tail as

drm_atomic_helper_commit_modeset_disables(dev, old_state);
drm_atomic_helper_commit_modeset_enables(dev, old_state);
drm_atomic_helper_commit_planes(dev, old_state,
DRM_PLANE_COMMIT_ACTIVE_ONLY);

while this patches moves the drm_atomic_helper_commit_planes() back between 
drm_atomic_helper_commit_modeset_disables() and 
drm_atomic_helper_commit_modeset_enables().

> >> +  drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >> 
> >>drm_atomic_helper_commit_hw_done(old_state);
> >>drm_atomic_helper_wait_for_vblanks(dev, old_state);

-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Kieran Bingham]

Hi Laurent,

On 28/06/17 19:50, Laurent Pinchart wrote:
> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> start to CRTC resume") changed the order of the plane commit and CRTC
> enable operations to accommodate the runtime PM requirements. However,
> this introduced corruption in the first displayed frame, as the CRTC is
> now enabled without any plane configured. On Gen2 hardware the first
> frame will be black and likely unnoticed, but on Gen3 hardware we end up
> starting the display before the VSP compositor, which is more
> noticeable.
> 
> To fix this, revert the order of the commit operations back, and handle
> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> helper operation handlers.
> 
> Signed-off-by: Laurent Pinchart 

I only have code reduction or comment suggestions below - so either with or
without those changes, feel free to add my:

Reviewed-by: Kieran Bingham 

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 66 
> --
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  4 +--
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
>  3 files changed, 43 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 6b5219ef0ad2..76cdb88b2b8e 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -448,14 +448,8 @@ static void rcar_du_crtc_wait_page_flip(struct 
> rcar_du_crtc *rcrtc)
>   * Start/Stop and Suspend/Resume
>   */
>  
> -static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
> +static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
>  {
> - struct drm_crtc *crtc = >crtc;
> - bool interlaced;
> -
> - if (rcrtc->started)
> - return;
> -
>   /* Set display off and background to black */
>   rcar_du_crtc_write(rcrtc, DOOR, DOOR_RGB(0, 0, 0));
>   rcar_du_crtc_write(rcrtc, BPOR, BPOR_RGB(0, 0, 0));
> @@ -467,6 +461,18 @@ static void rcar_du_crtc_start(struct rcar_du_crtc 
> *rcrtc)
>   /* Start with all planes disabled. */
>   rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
>  
> + /* Enable the VSP compositor. */
> + if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> + rcar_du_vsp_enable(rcrtc);
> +
> + /* Turn vertical blanking interrupt reporting on. */
> + drm_crtc_vblank_on(>crtc);
> +}
> +
> +static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc)
> +{
> + bool interlaced;
> +
>   /* Select master sync mode. This enables display operation in master

Are we close enough here to fix this multiline comment style ?
(Not worth doing unless the patch is respun for other reasons ...)

Actually - there are a lot in this file, so it would be better to do them all in
one hit/patch at a point of least conflicts ...


>* sync mode (with the HSYNC and VSYNC signals configured as outputs and
>* actively driven).
> @@ -477,24 +483,12 @@ static void rcar_du_crtc_start(struct rcar_du_crtc 
> *rcrtc)
>DSYSR_TVM_MASTER);
>  
>   rcar_du_group_start_stop(rcrtc->group, true);
> -
> - /* Enable the VSP compositor. */
> - if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> - rcar_du_vsp_enable(rcrtc);
> -
> - /* Turn vertical blanking interrupt reporting back on. */
> - drm_crtc_vblank_on(crtc);
> -
> - rcrtc->started = true;
>  }
>  
>  static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>  {
>   struct drm_crtc *crtc = >crtc;
>  
> - if (!rcrtc->started)
> - return;
> -
>   /* Disable all planes and wait for the change to take effect. This is
>* required as the DSnPR registers are updated on vblank, and no vblank
>* will occur once the CRTC is stopped. Disabling planes when starting
> @@ -525,8 +519,6 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>   rcar_du_crtc_clr_set(rcrtc, DSYSR, DSYSR_TVM_MASK, DSYSR_TVM_SWITCH);
>  
>   rcar_du_group_start_stop(rcrtc->group, false);
> -
> - rcrtc->started = false;
>  }
>  
>  void rcar_du_crtc_suspend(struct rcar_du_crtc *rcrtc)
> @@ -546,12 +538,10 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc)
>   return;
>  
>   rcar_du_crtc_get(rcrtc);
> - rcar_du_crtc_start(rcrtc);
> + rcar_du_crtc_setup(rcrtc);

Every call to _setup is immediately prefixed by a call to _get()

Could the _get() be done in the _setup() for code reduction?

I'm entirely open to that not happening here as it might be preferred to keep
the _get() and _start() separate for style purposes.

>  
>   /* Commit the planes state. */
> - if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> - rcar_du_vsp_enable(rcrtc);
> - } else {
> + if 

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Laurent Pinchart]

Hi Geert,

On Wednesday 28 Jun 2017 20:52:53 Geert Uytterhoeven wrote:
> On Wed, Jun 28, 2017 at 8:50 PM, Laurent Pinchart wrote:
> > Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> > start to CRTC resume") changed the order of the plane commit and CRTC
> > enable operations to accommodate the runtime PM requirements. However,
> > this introduced corruption in the first displayed frame, as the CRTC is
> > now enabled without any plane configured. On Gen2 hardware the first
> > frame will be black and likely unnoticed, but on Gen3 hardware we end up
> > starting the display before the VSP compositor, which is more
> > noticeable.
> > 
> > To fix this, revert the order of the commit operations back, and handle
> > runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> > helper operation handlers.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> Fixes: ...

I thought about that, but this patch fixes a problem caused by a combination 
of commits. The one mentioned above is probably the easiest to point at, but 
the problem only became, well, problematic, with the introduction of Gen3 
support in v4.6.

Furthermore, while it's probably possible to backport this patch to v4.6, I 
don't think it needs to be included in -stable. For all those reasons, a Fixes 
tag might not be useful. Of course please feel free to disagree :-)

-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: rcar-du: Setup planes before enabling CRTC to avoid flicker

[Geert Uytterhoeven]

On Wed, Jun 28, 2017 at 8:50 PM, Laurent Pinchart
 wrote:
> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> start to CRTC resume") changed the order of the plane commit and CRTC
> enable operations to accommodate the runtime PM requirements. However,
> this introduced corruption in the first displayed frame, as the CRTC is
> now enabled without any plane configured. On Gen2 hardware the first
> frame will be black and likely unnoticed, but on Gen3 hardware we end up
> starting the display before the VSP compositor, which is more
> noticeable.
>
> To fix this, revert the order of the commit operations back, and handle
> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> helper operation handlers.
>
> Signed-off-by: Laurent Pinchart 

Fixes: ...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel