Re: [Freedreno] [PATCH v4 1/1] drm: allow limiting the scatter list size.

2020-09-08 Thread Daniel Vetter
On Tue, Sep 08, 2020 at 07:48:58AM +0200, Gerd Hoffmann wrote:
> On Mon, Sep 07, 2020 at 03:53:02PM +0200, Daniel Vetter wrote:
> > On Mon, Sep 7, 2020 at 1:24 PM Gerd Hoffmann  wrote:
> > >
> > > Add drm_device argument to drm_prime_pages_to_sg(), so we can
> > > call dma_max_mapping_size() to figure the segment size limit
> > > and call into __sg_alloc_table_from_pages() with the correct
> > > limit.
> > >
> > > This fixes virtio-gpu with sev.  Possibly it'll fix other bugs
> > > too given that drm seems to totaly ignore segment size limits
> > > so far ...
> > >
> > > v2: place max_segment in drm driver not gem object.
> > > v3: move max_segment next to the other gem fields.
> > > v4: just use dma_max_mapping_size().
> > >
> > > Signed-off-by: Gerd Hoffmann 
> > 
> > Uh, are you sure this works in all cases for virtio?
> 
> Sure, I've tested it ;)
> 
> > The comments I've found suggest very much not ... Or is that all very
> > old stuff only that no one cares about anymore?
> 
> I think these days it is possible to override dma_ops per device, which
> in turn allows virtio to deal with the quirks without the rest of the
> kernel knowing about these details.
> 
> I also think virtio-gpu can drop the virtio_has_dma_quirk() checks, just
> use the dma api path unconditionally and depend on virtio core having
> setup dma_ops in a way that it JustWorks[tm].  I'll look into that next.

The comment above vring_use_dma_api() suggests that this has not yet
happened, that's why I'm asking. If this has happened then I think it'd be
best if you remove that todo entry and update it, as part of the overall
series to add dma_max_mapping_size and remove the quirks.

Otherwise this all is a bit wtf material :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno


Re: [Freedreno] [PATCH v4 1/1] drm: allow limiting the scatter list size.

2020-09-08 Thread Gerd Hoffmann
> > > The comments I've found suggest very much not ... Or is that all very
> > > old stuff only that no one cares about anymore?
> > 
> > I think these days it is possible to override dma_ops per device, which
> > in turn allows virtio to deal with the quirks without the rest of the
> > kernel knowing about these details.
> > 
> > I also think virtio-gpu can drop the virtio_has_dma_quirk() checks, just
> > use the dma api path unconditionally and depend on virtio core having
> > setup dma_ops in a way that it JustWorks[tm].  I'll look into that next.
> 
> The comment above vring_use_dma_api() suggests that this has not yet
> happened, that's why I'm asking.

Hmm, wading through the code, seems it indeed happen yet, even though my
testing didn't show any issues.  Probably pure luck because devices and
cpus have the same memory view on x86.  Guess I need to try this on
ppc64 to see it actually failing ...

So dropping the virtio_has_dma_quirk() checks isn't going to fly.

Using dma_max_mapping_size() should be fine though.  It might use a
lower limit than needed for virtio, but it should not break things.

take care,
  Gerd

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno


Re: [Freedreno] [PATCH v4 1/1] drm: allow limiting the scatter list size.

2020-09-08 Thread Daniel Vetter
On Tue, Sep 08, 2020 at 12:02:53PM +0200, Gerd Hoffmann wrote:
> > > > The comments I've found suggest very much not ... Or is that all very
> > > > old stuff only that no one cares about anymore?
> > > 
> > > I think these days it is possible to override dma_ops per device, which
> > > in turn allows virtio to deal with the quirks without the rest of the
> > > kernel knowing about these details.
> > > 
> > > I also think virtio-gpu can drop the virtio_has_dma_quirk() checks, just
> > > use the dma api path unconditionally and depend on virtio core having
> > > setup dma_ops in a way that it JustWorks[tm].  I'll look into that next.
> > 
> > The comment above vring_use_dma_api() suggests that this has not yet
> > happened, that's why I'm asking.
> 
> Hmm, wading through the code, seems it indeed happen yet, even though my
> testing didn't show any issues.  Probably pure luck because devices and
> cpus have the same memory view on x86.  Guess I need to try this on
> ppc64 to see it actually failing ...
> 
> So dropping the virtio_has_dma_quirk() checks isn't going to fly.
> 
> Using dma_max_mapping_size() should be fine though.  It might use a
> lower limit than needed for virtio, but it should not break things.

Makes sense. On this patch here:

Reviewed-by: Daniel Vetter 

And I guess would be good if virtio pushes a bit more towards using the
dma api abstraction fully so we can get rid of these hacks. Virtio feels
like a driver that really should be using dma-api and not dig around
behind it because "it' makes stuff 0.5% faster" or so, since being
virtualized it's already not the king of speed anyway :-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno


[Freedreno] Adreno GPU microcode update

2020-09-08 Thread Jordan Crouse
The following changes since commit d5f9eea5a251d43412b07f5295d03e97b89ac4a5:

  wl18xx: update firmware file 8.9.0.0.83 (2020-09-01 08:07:59 -0400)

are available in the Git repository at:

  https://github.com/CosmicPenguin/linux-firmware.git for-master

for you to fetch changes up to f48fec44127f88ce83ea1bcaf5824de4146ca2f9:

  qcom: Add updated a5xx and a6xx microcode (2020-09-08 10:03:36 -0600)


Jordan Crouse (1):
  qcom: Add updated a5xx and a6xx microcode

 qcom/a530_pfp.fw | Bin 15876 -> 16144 bytes
 qcom/a630_sqe.fw | Bin 31316 -> 32056 bytes
 2 files changed, 0 insertions(+), 0 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno


Re: [Freedreno] [PATCH v5] drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets

2020-09-08 Thread Stephen Boyd
Quoting Kuogee Hsieh (2020-09-04 12:54:39)
> add event thread to execute events serially from event queue. Also
> timeout mode is supported  which allow an event be deferred to be
> executed at later time. Both link and phy compliant tests had been
> done successfully.
> 
> Changes in v2:
> - Fix potential deadlock by removing redundant connect_mutex
> - Check and enable link clock during modeset
> - Drop unused code and fix function prototypes.
> - set sink power to normal operation state (D0) before DPCD read
> 
> Changes in v3:
> - push idle pattern at main link before timing generator off
> - add timeout handles for both connect and disconnect
> 
> Changes in v4:
> - add ST_SUSPEND_PENDING to handles suspend/modeset test operations
> - clear dp phy aux interrupt status when ERR_DPPHY_AUX error
> - send segment addr during edid read
> - clear bpp depth before MISC register write
> 
> Changes in v5:
> - add ST_SUSPENDED to fix crash at resume
> 
> Signed-off-by: Kuogee Hsieh 
> 
> This patch depends-on following series:
> https://lore.kernel.org/dri-devel/20200818051137.21478-1-tan...@codeaurora.org/

Can this be based on v12 of the patch series? v4 of this patch was based
on v12, but this has regressed and gone back to v11.

v12 is 
https://lkml.kernel.org/lkml/20200827211658.27479-1-tan...@codeaurora.org/
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno


Re: [Freedreno] [PATCH 1/2] drm/msm/dpu: move vblank events to complete_commit()

2020-09-08 Thread abhinavk

On 2020-09-07 10:04, Rob Clark wrote:

From: Rob Clark 

We could get a vblank event racing with the current atomic commit,
resulting in sending the pageflip event to userspace early, causing
tearing.  On the other hand, complete_commit() ensures that the
pending flush is complete.

Signed-off-by: Rob Clark 


I checked our downstream code as well and yes we are not signaling
page flips inside the vblank_cb and are doing it after 
wait_for_commit_done

This aligns with that.
Hence,
Reviewed-by: Abhinav Kumar 

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index c2729f71e2fa..89c0245b5de5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -297,7 +297,6 @@ void dpu_crtc_vblank_callback(struct drm_crtc 
*crtc)

dpu_crtc->vblank_cb_time = ktime_get();
else
dpu_crtc->vblank_cb_count++;
-   _dpu_crtc_complete_flip(crtc);
drm_crtc_handle_vblank(crtc);
trace_dpu_crtc_vblank_cb(DRMID(crtc));
 }
@@ -402,6 +401,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 
event)

 void dpu_crtc_complete_commit(struct drm_crtc *crtc)
 {
trace_dpu_crtc_complete_commit(DRMID(crtc));
+   _dpu_crtc_complete_flip(crtc);
 }

 static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno


Re: [Freedreno] [PATCH 2/2] drm/msm/dpu: clean up some impossibilities

2020-09-08 Thread abhinavk

On 2020-09-07 10:04, Rob Clark wrote:

From: Rob Clark 

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 81 
 1 file changed, 12 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 89c0245b5de5..ad49b0e17fcb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -265,11 +265,6 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
drm_crtc *crtc)
 {
struct drm_encoder *encoder;

-   if (!crtc) {
-   DPU_ERROR("invalid crtc\n");
-   return INTF_MODE_NONE;
-   }
-
/*
 	 * TODO: This function is called from dpu debugfs and as part of 
atomic

 * check. When called from debugfs, the crtc->mutex must be held to
@@ -500,12 +495,6 @@ static void dpu_crtc_atomic_begin(struct drm_crtc 
*crtc,

struct dpu_crtc_state *cstate;
struct drm_encoder *encoder;
struct drm_device *dev;
-   unsigned long flags;
-
-   if (!crtc) {
-   DPU_ERROR("invalid crtc\n");
-   return;
-   }

if (!crtc->state->enable) {
DPU_DEBUG("crtc%d -> enable %d, skip atomic_begin\n",
@@ -521,15 +510,6 @@ static void dpu_crtc_atomic_begin(struct drm_crtc 
*crtc,


_dpu_crtc_setup_lm_bounds(crtc, crtc->state);

-   if (dpu_crtc->event) {
-   WARN_ON(dpu_crtc->event);
-   } else {
-   spin_lock_irqsave(&dev->event_lock, flags);
-   dpu_crtc->event = crtc->state->event;
-   crtc->state->event = NULL;
-   spin_unlock_irqrestore(&dev->event_lock, flags);
-   }
-
/* encoder will trigger pending mask now */
 	drm_for_each_encoder_mask(encoder, crtc->dev, 
crtc->state->encoder_mask)

dpu_encoder_trigger_kickoff_pending(encoder);
@@ -583,14 +563,11 @@ static void dpu_crtc_atomic_flush(struct drm_crtc 
*crtc,

return;
}

-   if (dpu_crtc->event) {
-   DPU_DEBUG("already received dpu_crtc->event\n");
-   } else {
-   spin_lock_irqsave(&dev->event_lock, flags);
-   dpu_crtc->event = crtc->state->event;
-   crtc->state->event = NULL;
-   spin_unlock_irqrestore(&dev->event_lock, flags);
-   }
+   WARN_ON(dpu_crtc->event);
before the patch "move vblank events to complete_commit()", the dpu_crtc 
events
were consumed in the _dpu_crtc_complete_flip(). So there was a chance 
that if the vblank event
did not come in time before the next commit, dpu_crtc->event will not be 
consumed.


But after the patch, _dpu_crtc_complete_flip() is being signaled in 
dpu_crtc_complete_commit()
which will always happen, so is there any case where we will hit this 
warning at all or will this

catch some other condition?


+   spin_lock_irqsave(&dev->event_lock, flags);
+   dpu_crtc->event = crtc->state->event;
+   crtc->state->event = NULL;
+   spin_unlock_irqrestore(&dev->event_lock, flags);

/*
 * If no mixers has been allocated in dpu_crtc_atomic_check(),
@@ -635,14 +612,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc 
*crtc,

 static void dpu_crtc_destroy_state(struct drm_crtc *crtc,
struct drm_crtc_state *state)
 {
-   struct dpu_crtc_state *cstate;
-
-   if (!crtc || !state) {
-   DPU_ERROR("invalid argument(s)\n");
-   return;
-   }
-
-   cstate = to_dpu_crtc_state(state);
+   struct dpu_crtc_state *cstate = to_dpu_crtc_state(state);

DPU_DEBUG("crtc%d\n", crtc->base.id);

@@ -731,14 +701,8 @@ static void dpu_crtc_reset(struct drm_crtc *crtc)
  */
 static struct drm_crtc_state *dpu_crtc_duplicate_state(struct drm_crtc 
*crtc)

 {
-   struct dpu_crtc_state *cstate, *old_cstate;
-
-   if (!crtc || !crtc->state) {
-   DPU_ERROR("invalid argument(s)\n");
-   return NULL;
-   }
+	struct dpu_crtc_state *cstate, *old_cstate = 
to_dpu_crtc_state(crtc->state);


-   old_cstate = to_dpu_crtc_state(crtc->state);
cstate = kmemdup(old_cstate, sizeof(*old_cstate), GFP_KERNEL);
if (!cstate) {
DPU_ERROR("failed to allocate state\n");
@@ -754,19 +718,12 @@ static struct drm_crtc_state
*dpu_crtc_duplicate_state(struct drm_crtc *crtc)
 static void dpu_crtc_disable(struct drm_crtc *crtc,
 struct drm_crtc_state *old_crtc_state)
 {
-   struct dpu_crtc *dpu_crtc;
-   struct dpu_crtc_state *cstate;
+   struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
+   struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
struct drm_encoder *encoder;
unsigned long flags;
bool release_bandwidth = false;

-   if (!crtc || !crtc->state) {
-   DPU_ERROR("invalid crtc\n");
-   return;
-   }
-   dpu_crtc = to_dpu_crtc(crt

Re: [Freedreno] [PATCH 2/2] drm/msm/dpu: clean up some impossibilities

2020-09-08 Thread Rob Clark
On Tue, Sep 8, 2020 at 12:44 PM  wrote:
>
> On 2020-09-07 10:04, Rob Clark wrote:
> > From: Rob Clark 
> >
> > Signed-off-by: Rob Clark 
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 81 
> >  1 file changed, 12 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 89c0245b5de5..ad49b0e17fcb 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -265,11 +265,6 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
> > drm_crtc *crtc)
> >  {
> >   struct drm_encoder *encoder;
> >
> > - if (!crtc) {
> > - DPU_ERROR("invalid crtc\n");
> > - return INTF_MODE_NONE;
> > - }
> > -
> >   /*
> >* TODO: This function is called from dpu debugfs and as part of
> > atomic
> >* check. When called from debugfs, the crtc->mutex must be held to
> > @@ -500,12 +495,6 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
> > *crtc,
> >   struct dpu_crtc_state *cstate;
> >   struct drm_encoder *encoder;
> >   struct drm_device *dev;
> > - unsigned long flags;
> > -
> > - if (!crtc) {
> > - DPU_ERROR("invalid crtc\n");
> > - return;
> > - }
> >
> >   if (!crtc->state->enable) {
> >   DPU_DEBUG("crtc%d -> enable %d, skip atomic_begin\n",
> > @@ -521,15 +510,6 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
> > *crtc,
> >
> >   _dpu_crtc_setup_lm_bounds(crtc, crtc->state);
> >
> > - if (dpu_crtc->event) {
> > - WARN_ON(dpu_crtc->event);
> > - } else {
> > - spin_lock_irqsave(&dev->event_lock, flags);
> > - dpu_crtc->event = crtc->state->event;
> > - crtc->state->event = NULL;
> > - spin_unlock_irqrestore(&dev->event_lock, flags);
> > - }
> > -
> >   /* encoder will trigger pending mask now */
> >   drm_for_each_encoder_mask(encoder, crtc->dev,
> > crtc->state->encoder_mask)
> >   dpu_encoder_trigger_kickoff_pending(encoder);
> > @@ -583,14 +563,11 @@ static void dpu_crtc_atomic_flush(struct drm_crtc
> > *crtc,
> >   return;
> >   }
> >
> > - if (dpu_crtc->event) {
> > - DPU_DEBUG("already received dpu_crtc->event\n");
> > - } else {
> > - spin_lock_irqsave(&dev->event_lock, flags);
> > - dpu_crtc->event = crtc->state->event;
> > - crtc->state->event = NULL;
> > - spin_unlock_irqrestore(&dev->event_lock, flags);
> > - }
> > + WARN_ON(dpu_crtc->event);
> before the patch "move vblank events to complete_commit()", the dpu_crtc
> events
> were consumed in the _dpu_crtc_complete_flip(). So there was a chance
> that if the vblank event
> did not come in time before the next commit, dpu_crtc->event will not be
> consumed.
>
> But after the patch, _dpu_crtc_complete_flip() is being signaled in
> dpu_crtc_complete_commit()
> which will always happen, so is there any case where we will hit this
> warning at all or will this
> catch some other condition?

The core drm-atomic bits will reject an incoming atomic update if the
previous one has not completed, so other than a coding bug, this
WARN_ON() should not be hit..  it's only purpose is to make it very
obvious if someone breaks something :-)

BR,
-R

>
> > + spin_lock_irqsave(&dev->event_lock, flags);
> > + dpu_crtc->event = crtc->state->event;
> > + crtc->state->event = NULL;
> > + spin_unlock_irqrestore(&dev->event_lock, flags);
> >
> >   /*
> >* If no mixers has been allocated in dpu_crtc_atomic_check(),
> > @@ -635,14 +612,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc
> > *crtc,
> >  static void dpu_crtc_destroy_state(struct drm_crtc *crtc,
> >   struct drm_crtc_state *state)
> >  {
> > - struct dpu_crtc_state *cstate;
> > -
> > - if (!crtc || !state) {
> > - DPU_ERROR("invalid argument(s)\n");
> > - return;
> > - }
> > -
> > - cstate = to_dpu_crtc_state(state);
> > + struct dpu_crtc_state *cstate = to_dpu_crtc_state(state);
> >
> >   DPU_DEBUG("crtc%d\n", crtc->base.id);
> >
> > @@ -731,14 +701,8 @@ static void dpu_crtc_reset(struct drm_crtc *crtc)
> >   */
> >  static struct drm_crtc_state *dpu_crtc_duplicate_state(struct drm_crtc
> > *crtc)
> >  {
> > - struct dpu_crtc_state *cstate, *old_cstate;
> > -
> > - if (!crtc || !crtc->state) {
> > - DPU_ERROR("invalid argument(s)\n");
> > - return NULL;
> > - }
> > + struct dpu_crtc_state *cstate, *old_cstate =
> > to_dpu_crtc_state(crtc->state);
> >
> > - old_cstate = to_dpu_crtc_state(crtc->state);
> >   cstate = kmemdup(old_cstate, sizeof(*old_cstate), GFP_KERNEL);
> >   if (!cstate) {
> >   DPU_ERROR("failed to allocate state\n");
> > @@ -754,19 +718,12 @@ static struct drm_crtc_state
>

Re: [Freedreno] [PATCH 2/2] drm/msm/dpu: clean up some impossibilities

2020-09-08 Thread abhinavk

On 2020-09-08 13:11, Rob Clark wrote:

On Tue, Sep 8, 2020 at 12:44 PM  wrote:


On 2020-09-07 10:04, Rob Clark wrote:
> From: Rob Clark 
>
> Signed-off-by: Rob Clark 


Reviewed-by: Abhinav Kumar 


> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 81 
>  1 file changed, 12 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 89c0245b5de5..ad49b0e17fcb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -265,11 +265,6 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
> drm_crtc *crtc)
>  {
>   struct drm_encoder *encoder;
>
> - if (!crtc) {
> - DPU_ERROR("invalid crtc\n");
> - return INTF_MODE_NONE;
> - }
> -
>   /*
>* TODO: This function is called from dpu debugfs and as part of
> atomic
>* check. When called from debugfs, the crtc->mutex must be held to
> @@ -500,12 +495,6 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
> *crtc,
>   struct dpu_crtc_state *cstate;
>   struct drm_encoder *encoder;
>   struct drm_device *dev;
> - unsigned long flags;
> -
> - if (!crtc) {
> - DPU_ERROR("invalid crtc\n");
> - return;
> - }
>
>   if (!crtc->state->enable) {
>   DPU_DEBUG("crtc%d -> enable %d, skip atomic_begin\n",
> @@ -521,15 +510,6 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
> *crtc,
>
>   _dpu_crtc_setup_lm_bounds(crtc, crtc->state);
>
> - if (dpu_crtc->event) {
> - WARN_ON(dpu_crtc->event);
> - } else {
> - spin_lock_irqsave(&dev->event_lock, flags);
> - dpu_crtc->event = crtc->state->event;
> - crtc->state->event = NULL;
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> - }
> -
>   /* encoder will trigger pending mask now */
>   drm_for_each_encoder_mask(encoder, crtc->dev,
> crtc->state->encoder_mask)
>   dpu_encoder_trigger_kickoff_pending(encoder);
> @@ -583,14 +563,11 @@ static void dpu_crtc_atomic_flush(struct drm_crtc
> *crtc,
>   return;
>   }
>
> - if (dpu_crtc->event) {
> - DPU_DEBUG("already received dpu_crtc->event\n");
> - } else {
> - spin_lock_irqsave(&dev->event_lock, flags);
> - dpu_crtc->event = crtc->state->event;
> - crtc->state->event = NULL;
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> - }
> + WARN_ON(dpu_crtc->event);
before the patch "move vblank events to complete_commit()", the 
dpu_crtc

events
were consumed in the _dpu_crtc_complete_flip(). So there was a chance
that if the vblank event
did not come in time before the next commit, dpu_crtc->event will not 
be

consumed.

But after the patch, _dpu_crtc_complete_flip() is being signaled in
dpu_crtc_complete_commit()
which will always happen, so is there any case where we will hit this
warning at all or will this
catch some other condition?


The core drm-atomic bits will reject an incoming atomic update if the
previous one has not completed, so other than a coding bug, this
WARN_ON() should not be hit..  it's only purpose is to make it very
obvious if someone breaks something :-)

BR,
-R



> + spin_lock_irqsave(&dev->event_lock, flags);
> + dpu_crtc->event = crtc->state->event;
> + crtc->state->event = NULL;
> + spin_unlock_irqrestore(&dev->event_lock, flags);
>
>   /*
>* If no mixers has been allocated in dpu_crtc_atomic_check(),
> @@ -635,14 +612,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc
> *crtc,
>  static void dpu_crtc_destroy_state(struct drm_crtc *crtc,
>   struct drm_crtc_state *state)
>  {
> - struct dpu_crtc_state *cstate;
> -
> - if (!crtc || !state) {
> - DPU_ERROR("invalid argument(s)\n");
> - return;
> - }
> -
> - cstate = to_dpu_crtc_state(state);
> + struct dpu_crtc_state *cstate = to_dpu_crtc_state(state);
>
>   DPU_DEBUG("crtc%d\n", crtc->base.id);
>
> @@ -731,14 +701,8 @@ static void dpu_crtc_reset(struct drm_crtc *crtc)
>   */
>  static struct drm_crtc_state *dpu_crtc_duplicate_state(struct drm_crtc
> *crtc)
>  {
> - struct dpu_crtc_state *cstate, *old_cstate;
> -
> - if (!crtc || !crtc->state) {
> - DPU_ERROR("invalid argument(s)\n");
> - return NULL;
> - }
> + struct dpu_crtc_state *cstate, *old_cstate =
> to_dpu_crtc_state(crtc->state);
>
> - old_cstate = to_dpu_crtc_state(crtc->state);
>   cstate = kmemdup(old_cstate, sizeof(*old_cstate), GFP_KERNEL);
>   if (!cstate) {
>   DPU_ERROR("failed to allocate state\n");
> @@ -754,19 +718,12 @@ static struct drm_crtc_state
> *dpu_crtc_duplicate_state(struct drm_crtc *crtc)
>  static void dpu_crtc_disable(struct drm_crtc *crtc,
>struct drm_crtc_state *old_crtc_state)
>  {