[PATCH] drm/komeda: Skips the invalid writeback job

[PATCH] drm/komeda: Skips the invalid writeback job

[Lowry Li (Arm Technology China)]

Current DRM-CORE accepts the writeback_job with a empty fb, but that
is an invalid job for HW, so need to skip it when commit it to HW.

Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_crtc.c         | 2 +-
 drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 2fed1f6..372e99a 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -265,7 +265,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
 		komeda_pipeline_update(slave, old->state);
 
 	conn_st = wb_conn ? wb_conn->base.base.state : NULL;
-	if (conn_st && conn_st->writeback_job)
+	if (conn_st && conn_st->writeback_job && conn_st->writeback_job->fb)
 		drm_writeback_queue_job(&wb_conn->base, conn_st);
 
 	/* step 2: notify the HW to kickoff the update */
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
index 9787745..8e2ef63 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
@@ -52,9 +52,16 @@
 	struct komeda_data_flow_cfg dflow;
 	int err;
 
-	if (!writeback_job || !writeback_job->fb)
+	if (!writeback_job)
 		return 0;
 
+	if (!writeback_job->fb) {
+		if (writeback_job->out_fence)
+			DRM_DEBUG_ATOMIC("Out fence required on a invalid writeback job.\n");
+
+		return writeback_job->out_fence ? -EINVAL : 0;
+	}
+
 	if (!crtc_st->active) {
 		DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n");
 		return -EINVAL;
-- 
1.9.1

[PATCH] drm/komeda: Skips the invalid writeback job

[Lowry Li (Arm Technology China)]

Current DRM-CORE accepts the writeback_job with a empty fb, but that
is an invalid job for HW, so need to skip it when commit it to HW.

Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_crtc.c         | 2 +-
 drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 2fed1f6..372e99a 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -265,7 +265,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
 		komeda_pipeline_update(slave, old->state);
 
 	conn_st = wb_conn ? wb_conn->base.base.state : NULL;
-	if (conn_st && conn_st->writeback_job)
+	if (conn_st && conn_st->writeback_job && conn_st->writeback_job->fb)
 		drm_writeback_queue_job(&wb_conn->base, conn_st);
 
 	/* step 2: notify the HW to kickoff the update */
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
index 9787745..8e2ef63 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
@@ -52,9 +52,16 @@
 	struct komeda_data_flow_cfg dflow;
 	int err;
 
-	if (!writeback_job || !writeback_job->fb)
+	if (!writeback_job)
 		return 0;
 
+	if (!writeback_job->fb) {
+		if (writeback_job->out_fence)
+			DRM_DEBUG_ATOMIC("Out fence required on a invalid writeback job.\n");
+
+		return writeback_job->out_fence ? -EINVAL : 0;
+	}
+
 	if (!crtc_st->active) {
 		DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n");
 		return -EINVAL;
-- 
1.9.1

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

Re: [PATCH] drm/komeda: Skips the invalid writeback job

[Daniel Vetter]

On Fri, Jul 26, 2019 at 08:13:00AM +0000, Lowry Li (Arm Technology China) wrote:
> Current DRM-CORE accepts the writeback_job with a empty fb, but that
> is an invalid job for HW, so need to skip it when commit it to HW.
> 
> Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>

Hm, this sounds a bit like an oversight in core writeback code? Not sure
how this can even happen, setting up a writeback job without an fb sounds
a bit like a bug to me at least ...

If we don't have a good reason for why other hw needs to accept this, then
imo this needs to be rejected in shared code. For consistent behaviour
across all writeback supporting drivers.
-Daniel

> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c         | 2 +-
>  drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 ++++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 2fed1f6..372e99a 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -265,7 +265,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
>  		komeda_pipeline_update(slave, old->state);
>  
>  	conn_st = wb_conn ? wb_conn->base.base.state : NULL;
> -	if (conn_st && conn_st->writeback_job)
> +	if (conn_st && conn_st->writeback_job && conn_st->writeback_job->fb)
>  		drm_writeback_queue_job(&wb_conn->base, conn_st);
>  
>  	/* step 2: notify the HW to kickoff the update */
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> index 9787745..8e2ef63 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> @@ -52,9 +52,16 @@
>  	struct komeda_data_flow_cfg dflow;
>  	int err;
>  
> -	if (!writeback_job || !writeback_job->fb)
> +	if (!writeback_job)
>  		return 0;
>  
> +	if (!writeback_job->fb) {
> +		if (writeback_job->out_fence)
> +			DRM_DEBUG_ATOMIC("Out fence required on a invalid writeback job.\n");
> +
> +		return writeback_job->out_fence ? -EINVAL : 0;
> +	}
> +
>  	if (!crtc_st->active) {
>  		DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n");
>  		return -EINVAL;
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/komeda: Skips the invalid writeback job

[Daniel Vetter]

On Fri, Jul 26, 2019 at 08:13:00AM +0000, Lowry Li (Arm Technology China) wrote:
> Current DRM-CORE accepts the writeback_job with a empty fb, but that
> is an invalid job for HW, so need to skip it when commit it to HW.
> 
> Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>

Hm, this sounds a bit like an oversight in core writeback code? Not sure
how this can even happen, setting up a writeback job without an fb sounds
a bit like a bug to me at least ...

If we don't have a good reason for why other hw needs to accept this, then
imo this needs to be rejected in shared code. For consistent behaviour
across all writeback supporting drivers.
-Daniel

> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c         | 2 +-
>  drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 ++++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 2fed1f6..372e99a 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -265,7 +265,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
>  		komeda_pipeline_update(slave, old->state);
>  
>  	conn_st = wb_conn ? wb_conn->base.base.state : NULL;
> -	if (conn_st && conn_st->writeback_job)
> +	if (conn_st && conn_st->writeback_job && conn_st->writeback_job->fb)
>  		drm_writeback_queue_job(&wb_conn->base, conn_st);
>  
>  	/* step 2: notify the HW to kickoff the update */
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> index 9787745..8e2ef63 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> @@ -52,9 +52,16 @@
>  	struct komeda_data_flow_cfg dflow;
>  	int err;
>  
> -	if (!writeback_job || !writeback_job->fb)
> +	if (!writeback_job)
>  		return 0;
>  
> +	if (!writeback_job->fb) {
> +		if (writeback_job->out_fence)
> +			DRM_DEBUG_ATOMIC("Out fence required on a invalid writeback job.\n");
> +
> +		return writeback_job->out_fence ? -EINVAL : 0;
> +	}
> +
>  	if (!crtc_st->active) {
>  		DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n");
>  		return -EINVAL;
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/komeda: Skips the invalid writeback job

[Brian Starkey]

On Fri, Jul 26, 2019 at 04:23:56PM +0200, Daniel Vetter wrote:
> On Fri, Jul 26, 2019 at 08:13:00AM +0000, Lowry Li (Arm Technology China) wrote:
> > Current DRM-CORE accepts the writeback_job with a empty fb, but that
> > is an invalid job for HW, so need to skip it when commit it to HW.
> >
> > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
>
> Hm, this sounds a bit like an oversight in core writeback code? Not sure
> how this can even happen, setting up a writeback job without an fb sounds
> a bit like a bug to me at least ...
>
> If we don't have a good reason for why other hw needs to accept this, then
> imo this needs to be rejected in shared code. For consistent behaviour
> across all writeback supporting drivers.
> -Daniel

I think it's only this way to simplify the drm_writeback_set_fb()
implementation in the case where the property is set more than once in
the same commit (to something valid, and then 0).

The core could indeed handle it - drm_writeback_set_fb() would check
fb. If it's NULL and there's no writeback job, then it can just early
return. If it's NULL and there's already a writeback job then it
should drop the reference on the existing fb and free that job.

Could lead to the job getting alloc/freed multiple times if userspace
is insane, but meh.

-Brian

>
> > ---
> >  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c         | 2 +-
> >  drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 ++++++++-
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > index 2fed1f6..372e99a 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > @@ -265,7 +265,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
> >  komeda_pipeline_update(slave, old->state);
> >
> >  conn_st = wb_conn ? wb_conn->base.base.state : NULL;
> > -if (conn_st && conn_st->writeback_job)
> > +if (conn_st && conn_st->writeback_job && conn_st->writeback_job->fb)
> >  drm_writeback_queue_job(&wb_conn->base, conn_st);
> >
> >  /* step 2: notify the HW to kickoff the update */
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > index 9787745..8e2ef63 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > @@ -52,9 +52,16 @@
> >  struct komeda_data_flow_cfg dflow;
> >  int err;
> >
> > -if (!writeback_job || !writeback_job->fb)
> > +if (!writeback_job)
> >  return 0;
> >
> > +if (!writeback_job->fb) {
> > +if (writeback_job->out_fence)
> > +DRM_DEBUG_ATOMIC("Out fence required on a invalid writeback job.\n");
> > +
> > +return writeback_job->out_fence ? -EINVAL : 0;
> > +}
> > +
> >  if (!crtc_st->active) {
> >  DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n");
> >  return -EINVAL;
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH] drm/komeda: Skips the invalid writeback job

[Brian Starkey]

On Fri, Jul 26, 2019 at 04:23:56PM +0200, Daniel Vetter wrote:
> On Fri, Jul 26, 2019 at 08:13:00AM +0000, Lowry Li (Arm Technology China) wrote:
> > Current DRM-CORE accepts the writeback_job with a empty fb, but that
> > is an invalid job for HW, so need to skip it when commit it to HW.
> >
> > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
>
> Hm, this sounds a bit like an oversight in core writeback code? Not sure
> how this can even happen, setting up a writeback job without an fb sounds
> a bit like a bug to me at least ...
>
> If we don't have a good reason for why other hw needs to accept this, then
> imo this needs to be rejected in shared code. For consistent behaviour
> across all writeback supporting drivers.
> -Daniel

I think it's only this way to simplify the drm_writeback_set_fb()
implementation in the case where the property is set more than once in
the same commit (to something valid, and then 0).

The core could indeed handle it - drm_writeback_set_fb() would check
fb. If it's NULL and there's no writeback job, then it can just early
return. If it's NULL and there's already a writeback job then it
should drop the reference on the existing fb and free that job.

Could lead to the job getting alloc/freed multiple times if userspace
is insane, but meh.

-Brian

>
> > ---
> >  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c         | 2 +-
> >  drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 ++++++++-
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > index 2fed1f6..372e99a 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > @@ -265,7 +265,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
> >  komeda_pipeline_update(slave, old->state);
> >
> >  conn_st = wb_conn ? wb_conn->base.base.state : NULL;
> > -if (conn_st && conn_st->writeback_job)
> > +if (conn_st && conn_st->writeback_job && conn_st->writeback_job->fb)
> >  drm_writeback_queue_job(&wb_conn->base, conn_st);
> >
> >  /* step 2: notify the HW to kickoff the update */
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > index 9787745..8e2ef63 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > @@ -52,9 +52,16 @@
> >  struct komeda_data_flow_cfg dflow;
> >  int err;
> >
> > -if (!writeback_job || !writeback_job->fb)
> > +if (!writeback_job)
> >  return 0;
> >
> > +if (!writeback_job->fb) {
> > +if (writeback_job->out_fence)
> > +DRM_DEBUG_ATOMIC("Out fence required on a invalid writeback job.\n");
> > +
> > +return writeback_job->out_fence ? -EINVAL : 0;
> > +}
> > +
> >  if (!crtc_st->active) {
> >  DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n");
> >  return -EINVAL;
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Skips the invalid writeback job

[Daniel Vetter]

On Fri, Jul 26, 2019 at 4:44 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
>
> On Fri, Jul 26, 2019 at 04:23:56PM +0200, Daniel Vetter wrote:
> > On Fri, Jul 26, 2019 at 08:13:00AM +0000, Lowry Li (Arm Technology China) wrote:
> > > Current DRM-CORE accepts the writeback_job with a empty fb, but that
> > > is an invalid job for HW, so need to skip it when commit it to HW.
> > >
> > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> >
> > Hm, this sounds a bit like an oversight in core writeback code? Not sure
> > how this can even happen, setting up a writeback job without an fb sounds
> > a bit like a bug to me at least ...
> >
> > If we don't have a good reason for why other hw needs to accept this, then
> > imo this needs to be rejected in shared code. For consistent behaviour
> > across all writeback supporting drivers.
> > -Daniel
>
> I think it's only this way to simplify the drm_writeback_set_fb()
> implementation in the case where the property is set more than once in
> the same commit (to something valid, and then 0).
>
> The core could indeed handle it - drm_writeback_set_fb() would check
> fb. If it's NULL and there's no writeback job, then it can just early
> return. If it's NULL and there's already a writeback job then it
> should drop the reference on the existing fb and free that job.
>
> Could lead to the job getting alloc/freed multiple times if userspace
> is insane, but meh.

Generally these consistency checks need to be in in the atomic_check
phase, not when we set properties. So either somewhere in the helpers,
or in drm_atomic_connector_check() if we want it in core, enforced for
everyone.
-Daniel

>
> -Brian
>
> >
> > > ---
> > >  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c         | 2 +-
> > >  drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 ++++++++-
> > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > index 2fed1f6..372e99a 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > @@ -265,7 +265,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
> > >  komeda_pipeline_update(slave, old->state);
> > >
> > >  conn_st = wb_conn ? wb_conn->base.base.state : NULL;
> > > -if (conn_st && conn_st->writeback_job)
> > > +if (conn_st && conn_st->writeback_job && conn_st->writeback_job->fb)
> > >  drm_writeback_queue_job(&wb_conn->base, conn_st);
> > >
> > >  /* step 2: notify the HW to kickoff the update */
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > index 9787745..8e2ef63 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > @@ -52,9 +52,16 @@
> > >  struct komeda_data_flow_cfg dflow;
> > >  int err;
> > >
> > > -if (!writeback_job || !writeback_job->fb)
> > > +if (!writeback_job)
> > >  return 0;
> > >
> > > +if (!writeback_job->fb) {
> > > +if (writeback_job->out_fence)
> > > +DRM_DEBUG_ATOMIC("Out fence required on a invalid writeback job.\n");
> > > +
> > > +return writeback_job->out_fence ? -EINVAL : 0;
> > > +}
> > > +
> > >  if (!crtc_st->active) {
> > >  DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n");
> > >  return -EINVAL;
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/komeda: Skips the invalid writeback job

[Daniel Vetter]

On Fri, Jul 26, 2019 at 4:44 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
>
> On Fri, Jul 26, 2019 at 04:23:56PM +0200, Daniel Vetter wrote:
> > On Fri, Jul 26, 2019 at 08:13:00AM +0000, Lowry Li (Arm Technology China) wrote:
> > > Current DRM-CORE accepts the writeback_job with a empty fb, but that
> > > is an invalid job for HW, so need to skip it when commit it to HW.
> > >
> > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> >
> > Hm, this sounds a bit like an oversight in core writeback code? Not sure
> > how this can even happen, setting up a writeback job without an fb sounds
> > a bit like a bug to me at least ...
> >
> > If we don't have a good reason for why other hw needs to accept this, then
> > imo this needs to be rejected in shared code. For consistent behaviour
> > across all writeback supporting drivers.
> > -Daniel
>
> I think it's only this way to simplify the drm_writeback_set_fb()
> implementation in the case where the property is set more than once in
> the same commit (to something valid, and then 0).
>
> The core could indeed handle it - drm_writeback_set_fb() would check
> fb. If it's NULL and there's no writeback job, then it can just early
> return. If it's NULL and there's already a writeback job then it
> should drop the reference on the existing fb and free that job.
>
> Could lead to the job getting alloc/freed multiple times if userspace
> is insane, but meh.

Generally these consistency checks need to be in in the atomic_check
phase, not when we set properties. So either somewhere in the helpers,
or in drm_atomic_connector_check() if we want it in core, enforced for
everyone.
-Daniel

>
> -Brian
>
> >
> > > ---
> > >  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c         | 2 +-
> > >  drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 ++++++++-
> > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > index 2fed1f6..372e99a 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > @@ -265,7 +265,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
> > >  komeda_pipeline_update(slave, old->state);
> > >
> > >  conn_st = wb_conn ? wb_conn->base.base.state : NULL;
> > > -if (conn_st && conn_st->writeback_job)
> > > +if (conn_st && conn_st->writeback_job && conn_st->writeback_job->fb)
> > >  drm_writeback_queue_job(&wb_conn->base, conn_st);
> > >
> > >  /* step 2: notify the HW to kickoff the update */
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > index 9787745..8e2ef63 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > @@ -52,9 +52,16 @@
> > >  struct komeda_data_flow_cfg dflow;
> > >  int err;
> > >
> > > -if (!writeback_job || !writeback_job->fb)
> > > +if (!writeback_job)
> > >  return 0;
> > >
> > > +if (!writeback_job->fb) {
> > > +if (writeback_job->out_fence)
> > > +DRM_DEBUG_ATOMIC("Out fence required on a invalid writeback job.\n");
> > > +
> > > +return writeback_job->out_fence ? -EINVAL : 0;
> > > +}
> > > +
> > >  if (!crtc_st->active) {
> > >  DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n");
> > >  return -EINVAL;
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Skips the invalid writeback job

[Lowry Li (Arm Technology China)]

On Fri, Jul 26, 2019 at 06:15:46PM +0200, Daniel Vetter wrote:
> On Fri, Jul 26, 2019 at 4:44 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
> >
> > On Fri, Jul 26, 2019 at 04:23:56PM +0200, Daniel Vetter wrote:
> > > On Fri, Jul 26, 2019 at 08:13:00AM +0000, Lowry Li (Arm Technology China) wrote:
> > > > Current DRM-CORE accepts the writeback_job with a empty fb, but that
> > > > is an invalid job for HW, so need to skip it when commit it to HW.
> > > >
> > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> > >
> > > Hm, this sounds a bit like an oversight in core writeback code? Not sure
> > > how this can even happen, setting up a writeback job without an fb sounds
> > > a bit like a bug to me at least ...
> > >
> > > If we don't have a good reason for why other hw needs to accept this, then
> > > imo this needs to be rejected in shared code. For consistent behaviour
> > > across all writeback supporting drivers.
> > > -Daniel
> >
> > I think it's only this way to simplify the drm_writeback_set_fb()
> > implementation in the case where the property is set more than once in
> > the same commit (to something valid, and then 0).
> >
> > The core could indeed handle it - drm_writeback_set_fb() would check
> > fb. If it's NULL and there's no writeback job, then it can just early
> > return. If it's NULL and there's already a writeback job then it
> > should drop the reference on the existing fb and free that job.
> >
> > Could lead to the job getting alloc/freed multiple times if userspace
> > is insane, but meh.
> 
> Generally these consistency checks need to be in in the atomic_check
> phase, not when we set properties. So either somewhere in the helpers,
> or in drm_atomic_connector_check() if we want it in core, enforced for
> everyone.
> -Daniel

Thanks Daniel and Brian's comments. Agree with it to add the check in
atomic_check phase. Will send an update patch.

-Lowry
> >
> > -Brian
> >
> > >
> > > > ---
> > > >  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c         | 2 +-
> > > >  drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 ++++++++-
> > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > > index 2fed1f6..372e99a 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > > @@ -265,7 +265,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
> > > >  komeda_pipeline_update(slave, old->state);
> > > >
> > > >  conn_st = wb_conn ? wb_conn->base.base.state : NULL;
> > > > -if (conn_st && conn_st->writeback_job)
> > > > +if (conn_st && conn_st->writeback_job && conn_st->writeback_job->fb)
> > > >  drm_writeback_queue_job(&wb_conn->base, conn_st);
> > > >
> > > >  /* step 2: notify the HW to kickoff the update */
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > > index 9787745..8e2ef63 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > > @@ -52,9 +52,16 @@
> > > >  struct komeda_data_flow_cfg dflow;
> > > >  int err;
> > > >
> > > > -if (!writeback_job || !writeback_job->fb)
> > > > +if (!writeback_job)
> > > >  return 0;
> > > >
> > > > +if (!writeback_job->fb) {
> > > > +if (writeback_job->out_fence)
> > > > +DRM_DEBUG_ATOMIC("Out fence required on a invalid writeback job.\n");
> > > > +
> > > > +return writeback_job->out_fence ? -EINVAL : 0;
> > > > +}
> > > > +
> > > >  if (!crtc_st->active) {
> > > >  DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n");
> > > >  return -EINVAL;
> > > > --
> > > > 1.9.1
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Regards,
Lowry
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Skips the invalid writeback job

[Lowry Li (Arm Technology China)]

On Mon, Jul 29, 2019 at 06:11:25PM +0800, Lowry Li wrote:
> On Fri, Jul 26, 2019 at 06:15:46PM +0200, Daniel Vetter wrote:
> > On Fri, Jul 26, 2019 at 4:44 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
> > >
> > > On Fri, Jul 26, 2019 at 04:23:56PM +0200, Daniel Vetter wrote:
> > > > On Fri, Jul 26, 2019 at 08:13:00AM +0000, Lowry Li (Arm Technology China) wrote:
> > > > > Current DRM-CORE accepts the writeback_job with a empty fb, but that
> > > > > is an invalid job for HW, so need to skip it when commit it to HW.
> > > > >
> > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> > > >
> > > > Hm, this sounds a bit like an oversight in core writeback code? Not sure
> > > > how this can even happen, setting up a writeback job without an fb sounds
> > > > a bit like a bug to me at least ...
> > > >
> > > > If we don't have a good reason for why other hw needs to accept this, then
> > > > imo this needs to be rejected in shared code. For consistent behaviour
> > > > across all writeback supporting drivers.
> > > > -Daniel
> > >
> > > I think it's only this way to simplify the drm_writeback_set_fb()
> > > implementation in the case where the property is set more than once in
> > > the same commit (to something valid, and then 0).
> > >
> > > The core could indeed handle it - drm_writeback_set_fb() would check
> > > fb. If it's NULL and there's no writeback job, then it can just early
> > > return. If it's NULL and there's already a writeback job then it
> > > should drop the reference on the existing fb and free that job.
> > >
> > > Could lead to the job getting alloc/freed multiple times if userspace
> > > is insane, but meh.
> > 
> > Generally these consistency checks need to be in in the atomic_check
> > phase, not when we set properties. So either somewhere in the helpers,
> > or in drm_atomic_connector_check() if we want it in core, enforced for
> > everyone.
> > -Daniel
> 
> Thanks Daniel and Brian's comments. Agree with it to add the check in
> atomic_check phase. Will send an update patch.
> 
> -Lowry
The new patchset has been committed at:
https://patchwork.freedesktop.org/series/64493/
Thanks again for the comments and this patch will be droped.

Best regards,
Lowry
> > >
> > > -Brian
> > >
> > > >
> > > > > ---
> > > > >  drivers/gpu/drm/arm/display/komeda/komeda_crtc.c         | 2 +-
> > > > >  drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 ++++++++-
> > > > >  2 files changed, 9 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > > > index 2fed1f6..372e99a 100644
> > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> > > > > @@ -265,7 +265,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
> > > > >  komeda_pipeline_update(slave, old->state);
> > > > >
> > > > >  conn_st = wb_conn ? wb_conn->base.base.state : NULL;
> > > > > -if (conn_st && conn_st->writeback_job)
> > > > > +if (conn_st && conn_st->writeback_job && conn_st->writeback_job->fb)
> > > > >  drm_writeback_queue_job(&wb_conn->base, conn_st);
> > > > >
> > > > >  /* step 2: notify the HW to kickoff the update */
> > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > > > index 9787745..8e2ef63 100644
> > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > > > @@ -52,9 +52,16 @@
> > > > >  struct komeda_data_flow_cfg dflow;
> > > > >  int err;
> > > > >
> > > > > -if (!writeback_job || !writeback_job->fb)
> > > > > +if (!writeback_job)
> > > > >  return 0;
> > > > >
> > > > > +if (!writeback_job->fb) {
> > > > > +if (writeback_job->out_fence)
> > > > > +DRM_DEBUG_ATOMIC("Out fence required on a invalid writeback job.\n");
> > > > > +
> > > > > +return writeback_job->out_fence ? -EINVAL : 0;
> > > > > +}
> > > > > +
> > > > >  if (!crtc_st->active) {
> > > > >  DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n");
> > > > >  return -EINVAL;
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Regards,
> Lowry

-- 
Regards,
Lowry
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel