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

Open Source and information security mailing list archives

Message-ID: <20190726144428.tfwoaniidijchblt@DESKTOP-E1NTVVP.localdomain>
Date:   Fri, 26 Jul 2019 14:44:30 +0000
From:   Brian Starkey <Brian.Starkey@....com>
To:     "Lowry Li (Arm Technology China)" <Lowry.Li@....com>,
        Liviu Dudau <Liviu.Dudau@....com>,
        "james qian wang (Arm Technology China)" <james.qian.wang@....com>,
        "maarten.lankhorst@...ux.intel.com" 
        <maarten.lankhorst@...ux.intel.com>,
        "seanpaul@...omium.org" <seanpaul@...omium.org>,
        "airlied@...ux.ie" <airlied@...ux.ie>,
        Ayan Halder <Ayan.Halder@....com>,
        "Jonathan Chai (Arm Technology China)" <Jonathan.Chai@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "Julien Yin (Arm Technology China)" <Julien.Yin@....com>,
        nd <nd@....com>
Subject: Re: [PATCH] drm/komeda: Skips the invalid writeback job

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@....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@...ts.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.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives