| Message ID | 1564128758-23553-1-git-send-email-lowry.li@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/komeda: Skips the invalid writeback job | expand |
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
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.
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
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
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
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;
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(-)