[PATCH RFC 00/12] tda998x updates
On Tue, 2016-11-08 at 18:24 +, Russell King - ARM Linux wrote:
> On Tue, Nov 08, 2016 at 05:20:36PM +0000, Jon Medhurst (Tixy) wrote:
> > On Tue, 2016-11-08 at 13:34 +, Russell King - ARM Linux wrote:
> > > On Tue, Nov 08, 2016 at 01:32:15PM +, Russell King - ARM Linux wrote:
> > > > Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> > > > these patches it's the original set of 10 patches. I've not pushed
> > > > these ones out to that branch yet, as I've three additional patches on
> > > > top of these which aren't "ready" for pushing out.
> > >
> > > Here's the delta between the branch and what I just posted:
> > >
> > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > b/drivers/gpu/drm/i2c/tda998x_drv.c
> > [...]
> >
> > I have a working setup for HDMI audio on Juno an would like to test this
> > series but am struggling to work out which patches to apply in what
> > order to what branch, can you be specific? (I've tried various
> > combinations of patches series from the list, drm-tda998x-devel, and the
> > diff you posted)
>
> Hmm, I guess this is going to be annoyingly rather difficult then.
> The structure of my git tree is:
>
> v4.8 mali patch -- merge --- these patches
> v4.7 -- tda998x audio patches (up to df0bd1e8f3c5) --^
>
> which makes it rather difficult to send out a series that people can
> apply as patches without first replicating that merge. I guess the
> answer is... use the _patches_ for review, and I'll push out the
> changes into drm-tda998x-devel... should be there soon. Look for
> commit hash d61fa2e50f2a. (Bah, slow 'net connections.)
Testing gets more complicated as I'm using 4.9-rc? which has a DMA fix
needed for audio [1] and breaks hdmi-codec which I hope I fixed [2].
Anyway, I merged in drm-tda998x-devel and audio continued to work on my
HDMI connected monitor. So I guess that's
Tested-by: Jon Medhurst
I also reviewed the patches in this series. They look like a mostly
mechanical code organisation improvement, and whilst I'm not very
familiar with the driver and DRM, the other actual code changes look OK
too. So, FWIW:
Acked-by: Jon Medhurst
[1] Commit d64e9a2c7509 ("dmaengine: pl330: fix residual for non-running BUSY
descriptors")
[2] https://patchwork.kernel.org/patch/9401485/
--
Tixy
[PATCH RFC 00/12] tda998x updates
On Tue, 2016-11-08 at 13:34 +, Russell King - ARM Linux wrote: > On Tue, Nov 08, 2016 at 01:32:15PM +, Russell King - ARM Linux wrote: > > Unfortunately, my drm-tda998x-devel branch is slightly out of date with > > these patches it's the original set of 10 patches. I've not pushed > > these ones out to that branch yet, as I've three additional patches on > > top of these which aren't "ready" for pushing out. > > Here's the delta between the branch and what I just posted: > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c > b/drivers/gpu/drm/i2c/tda998x_drv.c [...] I have a working setup for HDMI audio on Juno an would like to test this series but am struggling to work out which patches to apply in what order to what branch, can you be specific? (I've tried various combinations of patches series from the list, drm-tda998x-devel, and the diff you posted) Thanks -- Tixy
Problem with component helpers and probe deferral in 4.5-rc1
On Wed, 2016-01-27 at 09:18 +, Jon Medhurst (Tixy) wrote: > On Tue, 2016-01-26 at 22:35 +, Russell King - ARM Linux wrote: > > On Tue, Jan 26, 2016 at 05:59:13PM +0000, Jon Medhurst (Tixy) wrote: > > > I believe I've found a problem with the component helpers and/or how > > > drivers use them. I discovered this whilst trying to get ARM's HDLCD > > > driver [1] working on 4.5-rc1, however I believe that code is following > > > a pattern used by drivers already in 4.5 and the problem isn't specific > > > to it. This is what I have observed... > > > > Hmm, it all looks plausible, and I'm again left wondering how the code > > passed testing over the last year (I've been running this code for > > ages both on iMX6 and Dove, where deferred probing does happen.) > > It depends on the order of things. To go wrong, components must already > be there before the master is added, then the master needs to defer > probing from it's bind callback. So, components deferring won't trigger > the issue, Actually, looks like drm drivers for armada and imx will end up calling component_bind_all from their bind callpath, so if components return -EPROBE_DEFER from their bind function, then the master will bail out with that, triggering my failure scenario. Hmmm... -- Tixy
Problem with component helpers and probe deferral in 4.5-rc1
On Tue, 2016-01-26 at 22:35 +, Russell King - ARM Linux wrote: > On Tue, Jan 26, 2016 at 05:59:13PM +0000, Jon Medhurst (Tixy) wrote: > > I believe I've found a problem with the component helpers and/or how > > drivers use them. I discovered this whilst trying to get ARM's HDLCD > > driver [1] working on 4.5-rc1, however I believe that code is following > > a pattern used by drivers already in 4.5 and the problem isn't specific > > to it. This is what I have observed... > > Hmm, it all looks plausible, and I'm again left wondering how the code > passed testing over the last year (I've been running this code for > ages both on iMX6 and Dove, where deferred probing does happen.) It depends on the order of things. To go wrong, components must already be there before the master is added, then the master needs to defer probing from it's bind callback. So, components deferring won't trigger the issue, neither will master deferring before any components have been added. > Your patch looks like the right thing to do, so I'll add it to the > component tree shortly - it should end up in linux-next in a few days > time. Thanks. I wasn't sure if that patch was correct as I hadn't looked at any of this code before this week. -- Tixy
[PATCH v2 1/4] drm: arm: Add DT bindings documentation for HDLCD driver.
On Thu, 2015-11-12 at 10:42 +, Liviu Dudau wrote: > > This is on-chip RAM or nornal system RAM? We already have bindings > for > > both. > > Juno has a set of TLX (ThinLinks) connectors on the board where an > FPGA can be attached. On r1 > the code running on FPGA can even participate as an AXI master with > full coherency. The FPGA > has local memory that we want to share with the HDLCD to be used as a > framebuffer. The HDLCD on the Juno chip or one implemented in the FPGA? I assume you mean the latter but just wanted to check. -- Tixy
[PATCH 2/4] drm: Add support for ARM's HDLCD controller.
I haven't reviewed the code in detail, just had one comment I alluded to
in a private email the other day...
On Wed, 2015-08-05 at 15:28 +0100, Liviu Dudau wrote:
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c
> b/drivers/gpu/drm/arm/hdlcd_crtc.c
[...]
> +void hdlcd_set_scanout(struct hdlcd_drm_private *hdlcd)
> +{
> + struct drm_framebuffer *fb = hdlcd->crtc.primary->fb;
> + struct drm_gem_cma_object *gem;
> + unsigned int depth, bpp;
> + dma_addr_t scanout_start;
> +
> + drm_fb_get_bpp_depth(fb->pixel_format, &depth, &bpp);
> + gem = drm_fb_cma_get_gem_obj(fb, 0);
> +
> + scanout_start = gem->paddr + fb->offsets[0] +
> + (hdlcd->crtc.y * fb->pitches[0]) + (hdlcd->crtc.x * bpp/8);
> +
> + hdlcd_write(hdlcd, HDLCD_REG_FB_BASE, scanout_start);
> +}
> +
The above function accesses various pointers and values, which
presumably all need to be valid and consistent. However...
[...]
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
[...]
> +static irqreturn_t hdlcd_irq(int irq, void *arg)
> +{
> + struct drm_device *dev = arg;
> + struct hdlcd_drm_private *hdlcd = dev->dev_private;
> + unsigned long irq_status;
> +
> + irq_status = hdlcd_read(hdlcd, HDLCD_REG_INT_STATUS);
> +
> +#ifdef CONFIG_DEBUG_FS
> + if (irq_status & HDLCD_INTERRUPT_UNDERRUN) {
> + atomic_inc(&hdlcd->buffer_underrun_count);
> + }
> + if (irq_status & HDLCD_INTERRUPT_DMA_END) {
> + atomic_inc(&hdlcd->dma_end_count);
> + }
> + if (irq_status & HDLCD_INTERRUPT_BUS_ERROR) {
> + atomic_inc(&hdlcd->bus_error_count);
> + }
> + if (irq_status & HDLCD_INTERRUPT_VSYNC) {
> + atomic_inc(&hdlcd->vsync_count);
> + }
> +#endif
> + if (irq_status & HDLCD_INTERRUPT_VSYNC) {
> + struct drm_pending_vblank_event *event;
> + unsigned long flags;
> +
> + hdlcd_set_scanout(hdlcd);
hdlcd_set_scanout is being called on every vsync interrupt, can we
guarantee that is safe? What if we're in the middle of a page flip or
panning operation? Seems to me that there is at least scope for
incorrect addresses being calculated leading to a nasty glitch on the
screen for one frame. And in the worst case, possibly invalid pointer
being dereferenced.
So, if scanout needs to be set on every vsync, would it not be safer
(and more efficient) to have a single variable storing the value to use
during interrupts, and recalculate that value outside of interrupts
whenever things are changed?
--
Tixy
[RFC PATCH] drm.h: Fix DRM compilation with bare-metal toolchain.
On Tue, 2013-04-16 at 12:50 +0200, Arnd Bergmann wrote: > On Tuesday 16 April 2013 12:48:28 Paul Sokolovsky wrote: > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > > index 8d1e2bb..73a99e4 100644 > > > --- a/include/uapi/drm/drm.h > > > +++ b/include/uapi/drm/drm.h > > > @@ -36,7 +36,7 @@ > > > #ifndef _DRM_H_ > > > #define _DRM_H_ > > > > > > -#if defined(__linux__) > > > +#if defined(__KERNEL__) || defined(__linux__) > > > > > > #include > > > #include > > This is still completely bogus, the __KERNEL__ symbol has no significance > here. > Either make the compiler define __linux__, or remove this #ifdef completely. I don't have the full background here but thought I would point out that a similar issue seems to have just cropped up with fuse.h ... http://lkml.org/lkml/2013/4/15/487 -- Tixy
Re: [RFC PATCH] drm.h: Fix DRM compilation with bare-metal toolchain.
On Tue, 2013-04-16 at 12:50 +0200, Arnd Bergmann wrote: > On Tuesday 16 April 2013 12:48:28 Paul Sokolovsky wrote: > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > > index 8d1e2bb..73a99e4 100644 > > > --- a/include/uapi/drm/drm.h > > > +++ b/include/uapi/drm/drm.h > > > @@ -36,7 +36,7 @@ > > > #ifndef _DRM_H_ > > > #define _DRM_H_ > > > > > > -#if defined(__linux__) > > > +#if defined(__KERNEL__) || defined(__linux__) > > > > > > #include > > > #include > > This is still completely bogus, the __KERNEL__ symbol has no significance > here. > Either make the compiler define __linux__, or remove this #ifdef completely. I don't have the full background here but thought I would point out that a similar issue seems to have just cropped up with fuse.h ... http://lkml.org/lkml/2013/4/15/487 -- Tixy ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
