* [PATCH] drm/komeda: Skips the invalid writeback job @ 2019-07-26 8:13 Lowry Li (Arm Technology China) 2019-07-26 14:23 ` Daniel Vetter 0 siblings, 1 reply; 4+ messages in thread From: Lowry Li (Arm Technology China) @ 2019-07-26 8:13 UTC (permalink / raw) To: Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, Brian Starkey Cc: Julien Yin (Arm Technology China), Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, nd 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/komeda: Skips the invalid writeback job 2019-07-26 8:13 [PATCH] drm/komeda: Skips the invalid writeback job Lowry Li (Arm Technology China) @ 2019-07-26 14:23 ` Daniel Vetter 2019-07-26 14:44 ` Brian Starkey 0 siblings, 1 reply; 4+ messages in thread From: Daniel Vetter @ 2019-07-26 14:23 UTC (permalink / raw) To: Lowry Li (Arm Technology China) Cc: Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, Brian Starkey, Ayan Halder, Jonathan Chai (Arm Technology China), linux-kernel, dri-devel, Julien Yin (Arm Technology China), nd 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/komeda: Skips the invalid writeback job 2019-07-26 14:23 ` Daniel Vetter @ 2019-07-26 14:44 ` Brian Starkey 2019-07-26 16:15 ` Daniel Vetter 0 siblings, 1 reply; 4+ messages in thread From: Brian Starkey @ 2019-07-26 14:44 UTC (permalink / raw) To: Lowry Li (Arm Technology China), Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, Ayan Halder, Jonathan Chai (Arm Technology China), linux-kernel, dri-devel, Julien Yin (Arm Technology China), nd 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/komeda: Skips the invalid writeback job 2019-07-26 14:44 ` Brian Starkey @ 2019-07-26 16:15 ` Daniel Vetter 0 siblings, 0 replies; 4+ messages in thread From: Daniel Vetter @ 2019-07-26 16:15 UTC (permalink / raw) To: Brian Starkey Cc: Lowry Li (Arm Technology China), Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, Ayan Halder, Jonathan Chai (Arm Technology China), linux-kernel, dri-devel, Julien Yin (Arm Technology China), nd 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-07-26 16:16 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-26 8:13 [PATCH] drm/komeda: Skips the invalid writeback job Lowry Li (Arm Technology China) 2019-07-26 14:23 ` Daniel Vetter 2019-07-26 14:44 ` Brian Starkey 2019-07-26 16:15 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®