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) <[email protected]>
---
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
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) <[email protected]>
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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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) <[email protected]>
>
> 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
> > [email protected]
> > 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.
On Fri, Jul 26, 2019 at 4:44 PM Brian Starkey <[email protected]> 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) <[email protected]>
> >
> > 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
> > > [email protected]
> > > 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch