Hallo
this rc1 runs here (pure Intel-box) without errors.
Thanks !
An RPC (I'm thinking about since some month)
======
Wouldn't it be better (and not so much add. work) to sort the
Pseudo-Shortlog towards subsystem/driver ?
something like this:
...
usb: gadget: f_ncm: allow using NCM in SuperSpeed Plus gadgets.
usb: cdns3: gadget: free interrupt after gadget has deleted
Lorenzo Colitti <lorenzo(a)google.com>
Peter Chen <peter.chen(a)nxp.com>
...
Think of searching a bugfix in the shortlog.
With the current layout I need to read/"visual grep" the whole log.
With the new layout I'm able to jump to the "buggy" subsystem/driver and
only need to read that part of the log to get the info if the bug is
fixed or not yet
Well, surely there are other ways to get the info I need, e.g.
search-function of my favorite browser, but ...
--
regards
Ronald
On Wed, 28 Oct 2020 at 11:03, Sakari Ailus <sakari.ailus(a)linux.intel.com> wrote:
>
> On Wed, Oct 28, 2020 at 10:56:55AM +0100, Krzysztof Kozlowski wrote:
> > On Wed, 28 Oct 2020 at 10:45, Krzysztof Kozlowski <krzk(a)kernel.org> wrote:
> > >
> > > On Wed, 28 Oct 2020 at 10:43, Yeh, Andy <andy.yeh(a)intel.com> wrote:
> > > >
> > > > But the sensor settings for the original submission is to output GRBG Bayer RAW.
> > > >
> > > > Regards, Andy
> > >
> > > No, not to my knowledge. There are no settings for color output
> > > because it is fixed to GBGB/RGRG. I was looking a lot into this driver
> > > (I have few other problems with it, already few other patches posted)
> > > and I could not find a setting for this in datasheet. If you know the
> > > setting for the other color - can you point me to it?
> >
> > And except the datasheet which mentions the specific format, the
> > testing confirms it. With original color the pictures are pink/purple.
> > With proper color, the pictures are correct (with more green color as
> > expected for bayer).
>
> Quoting the driver's start_streaming function:
>
> /* Set Orientation be 180 degree */
> ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP);
I understand that you think it will replace the lines and columns and
the first line will be RG, instead of GB.... or actually BG because it
flips horizontal and vertical? So why does it not work?
BTW, this nicely points that the comment around
device_property_read_u32() for rotation is a little bit misleading :)
> if (ret) {
> dev_err(&client->dev, "%s failed to set orientation\n",
> __func__);
> return ret;
> }
>
> Could it be you're taking pictures of pink objects? ;-)
I can send a few sample pictures taken with GStreamer (RAW8, not
original RAW10)...
Best regards,
Krzysztof
On Wed, Oct 28, 2020 at 10:14:52AM +0000, Yeh, Andy wrote:
> >-----Original Message-----
> >From: Sakari Ailus <sakari.ailus(a)linux.intel.com>
> >Sent: Wednesday, October 28, 2020 6:03 PM
> >To: Krzysztof Kozlowski <krzk(a)kernel.org>
> >Cc: Yeh, Andy <andy.yeh(a)intel.com>; Mauro Carvalho Chehab
> ><mchehab(a)kernel.org>; Tomasz Figa <tfiga(a)chromium.org>; Jason Chen
> ><jasonx.z.chen(a)intel.com>; Alan Chiang <alanx.chiang(a)intel.com>; linux-
> >media(a)vger.kernel.org; linux-kernel(a)vger.kernel.org;
> >stable(a)vger.kernel.org
> >Subject: Re: [PATCH] media: i2c: imx258: correct mode to GBGB/RGRG
> >
> >On Wed, Oct 28, 2020 at 10:56:55AM +0100, Krzysztof Kozlowski wrote:
> >> On Wed, 28 Oct 2020 at 10:45, Krzysztof Kozlowski <krzk(a)kernel.org>
> >wrote:
> >> >
> >> > On Wed, 28 Oct 2020 at 10:43, Yeh, Andy <andy.yeh(a)intel.com> wrote:
> >> > >
> >> > > But the sensor settings for the original submission is to output GRBG
> >Bayer RAW.
> >> > >
> >> > > Regards, Andy
> >> >
> >> > No, not to my knowledge. There are no settings for color output
> >> > because it is fixed to GBGB/RGRG. I was looking a lot into this
> >> > driver (I have few other problems with it, already few other patches
> >> > posted) and I could not find a setting for this in datasheet. If you
> >> > know the setting for the other color - can you point me to it?
> >>
> >> And except the datasheet which mentions the specific format, the
> >> testing confirms it. With original color the pictures are pink/purple.
> >> With proper color, the pictures are correct (with more green color as
> >> expected for bayer).
> >
> >Quoting the driver's start_streaming function:
> >
> > /* Set Orientation be 180 degree */
> > ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> > IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP);
> > if (ret) {
> > dev_err(&client->dev, "%s failed to set orientation\n",
> > __func__);
> > return ret;
> > }
> >
> >Could it be you're taking pictures of pink objects? ;-)
> >
> >--
> >Sakari Ailus
>
> Sakari is right. IIRC since the default sensor settings outputs in GBRG, and after mirro/flip (it is the original application when submit the driver) the bayer order will be GRBG.
Yes, you are all right. It seems I was using wrong color mode for
Gstreamer, or I messed up something more.
Thanks for help everyone!
The patch can be dropped.
Best regards,
Krzysztof
We've fixed many races in panfrost_job_timedout() but some remain.
Instead of trying to fix it again, let's simplify the logic and move
the reset bits to a separate work scheduled when one of the queue
reports a timeout.
Fixes: 1a11a88cfd9a ("drm/panfrost: Fix job timeout handling")
Cc: <stable(a)vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon(a)collabora.com>
---
drivers/gpu/drm/panfrost/panfrost_device.c | 1 -
drivers/gpu/drm/panfrost/panfrost_device.h | 6 +-
drivers/gpu/drm/panfrost/panfrost_job.c | 130 ++++++++++++---------
3 files changed, 82 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index ea8d31863c50..a83b2ff5837a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -200,7 +200,6 @@ int panfrost_device_init(struct panfrost_device *pfdev)
struct resource *res;
mutex_init(&pfdev->sched_lock);
- mutex_init(&pfdev->reset_lock);
INIT_LIST_HEAD(&pfdev->scheduled_jobs);
INIT_LIST_HEAD(&pfdev->as_lru_list);
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 2e9cbd1c4a58..67f9f66904be 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -105,7 +105,11 @@ struct panfrost_device {
struct panfrost_perfcnt *perfcnt;
struct mutex sched_lock;
- struct mutex reset_lock;
+
+ struct {
+ struct work_struct work;
+ atomic_t pending;
+ } reset;
struct mutex shrinker_lock;
struct list_head shrinker_list;
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index d0469e944143..745ee9563a54 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -20,6 +20,8 @@
#include "panfrost_gpu.h"
#include "panfrost_mmu.h"
+#define JOB_TIMEOUT_MS 500
+
#define job_write(dev, reg, data) writel(data, dev->iomem + (reg))
#define job_read(dev, reg) readl(dev->iomem + (reg))
@@ -382,19 +384,37 @@ static bool panfrost_scheduler_stop(struct panfrost_queue_state *queue,
drm_sched_increase_karma(bad);
queue->stopped = true;
stopped = true;
+
+ /*
+ * Set the timeout to max so the timer doesn't get started
+ * when we return from the timeout handler (restored in
+ * panfrost_scheduler_start()).
+ */
+ queue->sched.timeout = MAX_SCHEDULE_TIMEOUT;
}
mutex_unlock(&queue->lock);
return stopped;
}
+static void panfrost_scheduler_start(struct panfrost_queue_state *queue)
+{
+ if (WARN_ON(!queue->stopped))
+ return;
+
+ mutex_lock(&queue->lock);
+ /* Restore the original timeout before starting the scheduler. */
+ queue->sched.timeout = msecs_to_jiffies(JOB_TIMEOUT_MS);
+ drm_sched_start(&queue->sched, true);
+ queue->stopped = false;
+ mutex_unlock(&queue->lock);
+}
+
static void panfrost_job_timedout(struct drm_sched_job *sched_job)
{
struct panfrost_job *job = to_panfrost_job(sched_job);
struct panfrost_device *pfdev = job->pfdev;
int js = panfrost_job_get_slot(job);
- unsigned long flags;
- int i;
/*
* If the GPU managed to complete this jobs fence, the timeout is
@@ -415,56 +435,9 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
if (!panfrost_scheduler_stop(&pfdev->js->queue[js], sched_job))
return;
- if (!mutex_trylock(&pfdev->reset_lock))
- return;
-
- for (i = 0; i < NUM_JOB_SLOTS; i++) {
- struct drm_gpu_scheduler *sched = &pfdev->js->queue[i].sched;
-
- /*
- * If the queue is still active, make sure we wait for any
- * pending timeouts.
- */
- if (!pfdev->js->queue[i].stopped)
- cancel_delayed_work_sync(&sched->work_tdr);
-
- /*
- * If the scheduler was not already stopped, there's a tiny
- * chance a timeout has expired just before we stopped it, and
- * drm_sched_stop() does not flush pending works. Let's flush
- * them now so the timeout handler doesn't get called in the
- * middle of a reset.
- */
- if (panfrost_scheduler_stop(&pfdev->js->queue[i], NULL))
- cancel_delayed_work_sync(&sched->work_tdr);
-
- /*
- * Now that we cancelled the pending timeouts, we can safely
- * reset the stopped state.
- */
- pfdev->js->queue[i].stopped = false;
- }
-
- spin_lock_irqsave(&pfdev->js->job_lock, flags);
- for (i = 0; i < NUM_JOB_SLOTS; i++) {
- if (pfdev->jobs[i]) {
- pm_runtime_put_noidle(pfdev->dev);
- panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
- pfdev->jobs[i] = NULL;
- }
- }
- spin_unlock_irqrestore(&pfdev->js->job_lock, flags);
-
- panfrost_device_reset(pfdev);
-
- for (i = 0; i < NUM_JOB_SLOTS; i++)
- drm_sched_resubmit_jobs(&pfdev->js->queue[i].sched);
-
- mutex_unlock(&pfdev->reset_lock);
-
- /* restart scheduler after GPU is usable again */
- for (i = 0; i < NUM_JOB_SLOTS; i++)
- drm_sched_start(&pfdev->js->queue[i].sched, true);
+ /* Schedule a reset. */
+ atomic_set(&pfdev->reset.pending, 1);
+ schedule_work(&pfdev->reset.work);
}
static const struct drm_sched_backend_ops panfrost_sched_ops = {
@@ -531,11 +504,62 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
return IRQ_HANDLED;
}
+static void panfrost_reset(struct work_struct *work)
+{
+ struct panfrost_device *pfdev = container_of(work,
+ struct panfrost_device,
+ reset.work);
+ unsigned long flags;
+ unsigned int i;
+
+ if (!atomic_read(&pfdev->reset.pending))
+ return;
+
+ for (i = 0; i < NUM_JOB_SLOTS; i++) {
+ /*
+ * We want pending timeouts to be handled before we attempt
+ * to stop the scheduler. If we don't do that and the timeout
+ * handler is in flight, it might have removed the bad job
+ * from the list, and we'll lose this job if the reset handler
+ * enters the critical section in panfrost_scheduler_stop()
+ * before the timeout handler.
+ *
+ * Timeout is set to max to make sure the timer is not
+ * restarted after the cancellation.
+ */
+ pfdev->js->queue[i].sched.timeout = MAX_SCHEDULE_TIMEOUT;
+ cancel_delayed_work_sync(&pfdev->js->queue[i].sched.work_tdr);
+ panfrost_scheduler_stop(&pfdev->js->queue[i], NULL);
+ }
+
+ /* All timers have been stopped, we can safely reset the pending state. */
+ atomic_set(&pfdev->reset.pending, 0);
+
+ spin_lock_irqsave(&pfdev->js->job_lock, flags);
+ for (i = 0; i < NUM_JOB_SLOTS; i++) {
+ if (pfdev->jobs[i]) {
+ pm_runtime_put_noidle(pfdev->dev);
+ panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
+ pfdev->jobs[i] = NULL;
+ }
+ }
+ spin_unlock_irqrestore(&pfdev->js->job_lock, flags);
+
+ panfrost_device_reset(pfdev);
+
+ for (i = 0; i < NUM_JOB_SLOTS; i++) {
+ drm_sched_resubmit_jobs(&pfdev->js->queue[i].sched);
+ panfrost_scheduler_start(&pfdev->js->queue[i]);
+ }
+}
+
int panfrost_job_init(struct panfrost_device *pfdev)
{
struct panfrost_job_slot *js;
int ret, j, irq;
+ INIT_WORK(&pfdev->reset.work, panfrost_reset);
+
pfdev->js = js = devm_kzalloc(pfdev->dev, sizeof(*js), GFP_KERNEL);
if (!js)
return -ENOMEM;
@@ -558,7 +582,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
ret = drm_sched_init(&js->queue[j].sched,
&panfrost_sched_ops,
- 1, 0, msecs_to_jiffies(500),
+ 1, 0, msecs_to_jiffies(JOB_TIMEOUT_MS),
"pan_js");
if (ret) {
dev_err(pfdev->dev, "Failed to create scheduler: %d.", ret);
--
2.26.2