Re: [PATCH] component: Move host device to end of device lists on binding
On Mon, May 10, 2021 at 06:05:21PM +0200, Daniel Vetter wrote: > Entirely aside, but an s/master/aggregate/ or similar over the entire > component.c codebase would help a pile in making it easier to understand > which part does what. Or at least I'm always terribly confused about which > bind binds what and all that, so maybe an additional review whether we > have a clear split into aggregate and individual components after that > initial fix is needed. I'm not entirely sure what you mean "which bind binds what". The component helper solves this problem: We have a master or aggregate device representing a collection of individual devices. The aggregate and individual devices may be probed by the device model in any order. The aggregate device is only complete once all individual and aggregate devices have been successfully probed. It does this by tracking which devices are present, and only when they are all present does it call the bind() operation. Conversely, if one happens to be removed, it calls the unbind() operation. To me, that's very simple. When we start talking about PM, the original idea was for the aggregate device to handle that. However, DRM/OF has pushed to change the model a bit such that the aggregate device is created as a platform device when we detect the presence of one of the individual devices. I suspect what we actually want is something that, when the first individual device gets notified of a transition to a lower power mode, we want to place the system formed by all the devices into a low power mode. Please realise that it may not be appropriate for every individual device to be affected by that transition until it receives its own PM call. > One question I have: Why is the bridge component driver not correctly > ordered wrt the i2c driver it needs? The idea is that the aggregate driver > doesn't access any hw itself, but entirely relies on all its components. As far as I'm aware, bridge was never converted to use any component stuff, so I'm not sure what you're referring to. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH] component: Move host device to end of device lists on binding
On Sat, May 08, 2021 at 12:41:18AM -0700, Stephen Boyd wrote:
> Within the component device framework this usually isn't that bad
> because the real driver work is done at bind time via
> component{,master}_ops::bind(). It becomes a problem when the driver
> core, or host driver, wants to operate on the component device outside
> of the bind/unbind functions, e.g. via 'remove' or 'shutdown'. The
> driver core doesn't understand the relationship between the host device
> and the component devices and could possibly try to operate on component
> devices when they're already removed from the system or shut down.
You really are not supposed to be doing anything with component devices
once they have been unbound. You can do stuff with them only between the
bind() and the unbind() callbacks for the host device.
Access to the host devices outside of that is totally undefined and
should not be done.
The shutdown callback should be fine as long as the other devices are
still bound, but there will be implications if the shutdown order
matters.
However, randomly pulling devices around in the DPM list sounds to me
like a very bad idea. What happens if such re-orderings result in a
child device being shutdown after a parent device has been shut down?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [GIT PULL] immutable branch for amba changes targeting v5.12-rc1
On Thu, Feb 04, 2021 at 05:56:50PM +0100, Greg Kroah-Hartman wrote: > On Thu, Feb 04, 2021 at 04:52:24PM +0000, Russell King - ARM Linux admin > wrote: > > On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote: > > > I'm glad to take this through my char/misc tree, as that's where the > > > other coresight changes flow through. So if no one else objects, I will > > > do so... > > > > Greg, did you end up pulling this after all? If not, Uwe produced a v2. > > I haven't merged v2 yet as I don't know what you've done. > > I thought you merged this? I took v1, and put it in a branch I've promised in the past not to rebase/rewind. Uwe is now asking for me to take a v2 or apply a patch on top. The only reason to produce an "immutable" branch is if it's the basis for some dependent work and you need that branch merged into other people's trees... so the whole "lets produce a v2" is really odd workflow... I'm confused about what I should do, and who has to be informed which option I take. I'm rather lost here too. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [GIT PULL] immutable branch for amba changes targeting v5.12-rc1
On Tue, Feb 02, 2021 at 03:06:05PM +0100, Greg Kroah-Hartman wrote: > I'm glad to take this through my char/misc tree, as that's where the > other coresight changes flow through. So if no one else objects, I will > do so... Greg, did you end up pulling this after all? If not, Uwe produced a v2. I haven't merged v2 yet as I don't know what you've done. Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 0/5] amba: minor fix and various cleanups
On Tue, Jan 26, 2021 at 05:58:30PM +0100, Uwe Kleine-König wrote: > From: Uwe Kleine-König > Hello, > > Changes since v2 sent with Message-Id: > 20201124133139.3072124-1-...@kleine-koenig.org: > > - Rebase to v5.11-rc1 (which resulted in a few conflicts in >drivers/hwtracing). > - Add various Acks. > - Send to more maintainers directly (which I think is one of the >reasons why there are so few Acks). > > For my taste patch 4 needs some more acks (drivers/char/hw_random, > drivers/dma, drivers/gpu/drm/pl111, drivers/i2c, drivers/mmc, > drivers/vfio, drivers/watchdog and sound/arm have no maintainer feedback > yet). > > My suggestion is to let this series go in via Russell King (who cares > for amba). Once enough Acks are there I can also provide a tag for > merging into different trees. Just tell me if you prefer this solution. > > Would be great if this could make it for v5.12, but I'm aware it's > already late in the v5.11 cycle so it might have to wait for v5.13. I think you need to have a 6th patch which moves the probe/remove/shutdown methods into the bus_type - if you're setting them for every struct device_driver, then there's no point doing that and they may as well be in the bus_type. Apart from that, it looks good. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/5] amba: Make the remove callback return void
On Tue, Jan 26, 2021 at 06:56:52PM +0100, Uwe Kleine-König wrote:
> I'm surprised to see that the remove callback introduced in 2952ecf5df33
> ("coresight: etm4x: Refactor probing routine") has an __exit annotation.
In general, remove callbacks should not have an __exit annotation.
__exit _can_ be discarded at link time for built-in stuff.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: bridge: dw-hdmi: Avoid resetting force in the detect function
On Sun, Nov 08, 2020 at 10:53:22AM +0100, Sam Ravnborg wrote:
> Russell,
>
> On Sat, Oct 31, 2020 at 07:17:47PM +1100, Jonathan Liu wrote:
> > It has been observed that resetting force in the detect function can
> > result in the PHY being powered down in response to hot-plug detect
> > being asserted, even when the HDMI connector is forced on.
> >
> > Enabling debug messages and adding a call to dump_stack() in
> > dw_hdmi_phy_power_off() shows the following in dmesg:
> > [ 160.637413] dwhdmi-rockchip ff94.hdmi: EVENT=plugin
> > [ 160.637433] dwhdmi-rockchip ff94.hdmi: PHY powered down in 0
> > iterations
> >
> > Call trace:
> > dw_hdmi_phy_power_off
> > dw_hdmi_phy_disable
> > dw_hdmi_update_power
> > dw_hdmi_detect
> > dw_hdmi_connector_detect
> > drm_helper_probe_detect_ctx
> > drm_helper_hpd_irq_event
> > dw_hdmi_irq
> > irq_thread_fn
> > irq_thread
> > kthread
> > ret_from_fork
> >
> > Fixes: 381f05a7a842 ("drm: bridge/dw_hdmi: add connector mode forcing")
> > Signed-off-by: Jonathan Liu
>
> you are the original author of this code - any comments on this patch?
No further comments beyond what has already been discussed, and the
long and short of it is it's been so long that I don't remember why
that code was there. Given that, I'm not even in a position to ack
the change. Sorry.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] gpu/drm/armada: fix unused parameter warning
On Mon, Oct 12, 2020 at 04:57:24AM -0700, Bernard Zhao wrote:
> Functions armada_drm_crtc_atomic_flush &
> armada_drm_crtc_atomic_enable don`t use the second parameter.
> So we may get warning like :
> warning: unused parameter ‘***’ [-Wunused-parameter].
> This change is to fix the compile warning with -Wunused-parameter.
Under what circumstances do we build the kernel with that warning
enabled?
>
> Signed-off-by: Bernard Zhao
> ---
> drivers/gpu/drm/armada/armada_crtc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/armada/armada_crtc.c
> b/drivers/gpu/drm/armada/armada_crtc.c
> index 38dfaa46d306..fc8b922c3e44 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -427,7 +427,7 @@ static int armada_drm_crtc_atomic_check(struct drm_crtc
> *crtc,
> }
>
> static void armada_drm_crtc_atomic_begin(struct drm_crtc *crtc,
> - struct drm_crtc_state *old_crtc_state)
> + struct drm_crtc_state __attribute__((unused))
> *old_crtc_state)
> {
> struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
>
> @@ -441,7 +441,7 @@ static void armada_drm_crtc_atomic_begin(struct drm_crtc
> *crtc,
> }
>
> static void armada_drm_crtc_atomic_flush(struct drm_crtc *crtc,
> - struct drm_crtc_state *old_crtc_state)
> + struct drm_crtc_state __attribute__((unused))
> *old_crtc_state)
> {
> struct armada_crtc *dcrtc = drm_to_armada_crtc(crtc);
>
> --
> 2.28.0
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 02/21] drm/armada: Introduce GEM object functions
On Tue, Sep 15, 2020 at 04:59:39PM +0200, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in armada.
>
> Signed-off-by: Thomas Zimmermann
Acked-by: Russell King
Thanks.
> ---
> drivers/gpu/drm/armada/armada_drv.c | 3 ---
> drivers/gpu/drm/armada/armada_gem.c | 12 +++-
> drivers/gpu/drm/armada/armada_gem.h | 2 --
> 3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/armada/armada_drv.c
> b/drivers/gpu/drm/armada/armada_drv.c
> index 980d3f1f8f16..22247cfce80b 100644
> --- a/drivers/gpu/drm/armada/armada_drv.c
> +++ b/drivers/gpu/drm/armada/armada_drv.c
> @@ -37,13 +37,10 @@ DEFINE_DRM_GEM_FOPS(armada_drm_fops);
>
> static struct drm_driver armada_drm_driver = {
> .lastclose = drm_fb_helper_lastclose,
> - .gem_free_object_unlocked = armada_gem_free_object,
> .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_export = armada_gem_prime_export,
> .gem_prime_import = armada_gem_prime_import,
> .dumb_create= armada_gem_dumb_create,
> - .gem_vm_ops = _gem_vm_ops,
> .major = 1,
> .minor = 0,
> .name = "armada-drm",
> diff --git a/drivers/gpu/drm/armada/armada_gem.c
> b/drivers/gpu/drm/armada/armada_gem.c
> index ecf8a55e93d9..c343fbefe47c 100644
> --- a/drivers/gpu/drm/armada/armada_gem.c
> +++ b/drivers/gpu/drm/armada/armada_gem.c
> @@ -25,7 +25,7 @@ static vm_fault_t armada_gem_vm_fault(struct vm_fault *vmf)
> return vmf_insert_pfn(vmf->vma, vmf->address, pfn);
> }
>
> -const struct vm_operations_struct armada_gem_vm_ops = {
> +static const struct vm_operations_struct armada_gem_vm_ops = {
> .fault = armada_gem_vm_fault,
> .open = drm_gem_vm_open,
> .close = drm_gem_vm_close,
> @@ -184,6 +184,12 @@ armada_gem_map_object(struct drm_device *dev, struct
> armada_gem_object *dobj)
> return dobj->addr;
> }
>
> +static const struct drm_gem_object_funcs armada_gem_object_funcs = {
> + .free = armada_gem_free_object,
> + .export = armada_gem_prime_export,
> + .vm_ops = _gem_vm_ops,
> +};
> +
> struct armada_gem_object *
> armada_gem_alloc_private_object(struct drm_device *dev, size_t size)
> {
> @@ -195,6 +201,8 @@ armada_gem_alloc_private_object(struct drm_device *dev,
> size_t size)
> if (!obj)
> return NULL;
>
> + obj->obj.funcs = _gem_object_funcs;
> +
> drm_gem_private_object_init(dev, >obj, size);
>
> DRM_DEBUG_DRIVER("alloc private obj %p size %zu\n", obj, size);
> @@ -214,6 +222,8 @@ static struct armada_gem_object
> *armada_gem_alloc_object(struct drm_device *dev,
> if (!obj)
> return NULL;
>
> + obj->obj.funcs = _gem_object_funcs;
> +
> if (drm_gem_object_init(dev, >obj, size)) {
> kfree(obj);
> return NULL;
> diff --git a/drivers/gpu/drm/armada/armada_gem.h
> b/drivers/gpu/drm/armada/armada_gem.h
> index de04cc2c8f0e..ffcc7e8dd351 100644
> --- a/drivers/gpu/drm/armada/armada_gem.h
> +++ b/drivers/gpu/drm/armada/armada_gem.h
> @@ -21,8 +21,6 @@ struct armada_gem_object {
> void*update_data;
> };
>
> -extern const struct vm_operations_struct armada_gem_vm_ops;
> -
> #define drm_to_armada_gem(o) container_of(o, struct armada_gem_object, obj)
>
> void armada_gem_free_object(struct drm_gem_object *);
> --
> 2.28.0
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/etnaviv: always start/stop scheduler in timeout processing
On Mon, Aug 24, 2020 at 01:02:48PM +0200, Lucas Stach wrote:
> The drm scheduler currently expects that the stop/start sequence is always
> executed in the timeout handling, as the job at the head of the hardware
> execution list is always removed from the ring mirror before the driver
> function is called and only inserted back into the list when starting the
> scheduler.
>
> This adds some unnecessary overhead if the timeout handler determines
> that the GPU is still executing jobs normally and just wished to extend
> the timeout, but a better solution requires a major rearchitecture of the
> scheduler, which is not applicable as a fix.
>
> Fixes: 135517d3565b drm/scheduler: Avoid accessing freed bad job.)
> Signed-off-by: Lucas Stach
>From a brief test, this seems to fix the problem, thanks.
Tested-by: Russell King
> ---
> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 11 ++-
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> index 4e3e95dce6d8..cd46c882269c 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
> @@ -89,12 +89,15 @@ static void etnaviv_sched_timedout_job(struct
> drm_sched_job *sched_job)
> u32 dma_addr;
> int change;
>
> + /* block scheduler */
> + drm_sched_stop(>sched, sched_job);
> +
> /*
>* If the GPU managed to complete this jobs fence, the timout is
>* spurious. Bail out.
>*/
> if (dma_fence_is_signaled(submit->out_fence))
> - return;
> + goto out_no_timeout;
>
> /*
>* If the GPU is still making forward progress on the front-end (which
> @@ -105,12 +108,9 @@ static void etnaviv_sched_timedout_job(struct
> drm_sched_job *sched_job)
> change = dma_addr - gpu->hangcheck_dma_addr;
> if (change < 0 || change > 16) {
> gpu->hangcheck_dma_addr = dma_addr;
> - return;
> + goto out_no_timeout;
> }
>
> - /* block scheduler */
> - drm_sched_stop(>sched, sched_job);
> -
> if(sched_job)
> drm_sched_increase_karma(sched_job);
>
> @@ -120,6 +120,7 @@ static void etnaviv_sched_timedout_job(struct
> drm_sched_job *sched_job)
>
> drm_sched_resubmit_jobs(>sched);
>
> +out_no_timeout:
> /* restart scheduler after GPU is usable again */
> drm_sched_start(>sched, true);
> }
> --
> 2.20.1
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/etnaviv: fix external abort seen on GC600 rev 0x19
On Sun, Aug 23, 2020 at 09:10:25PM +0200, Christian Gmeiner wrote: > Hi > > > I have formally tested the patch with 5.7.10 - and it doesn't resolve > > the issue - sadly :( > > > > From my testing, the reads on > > VIVS_HI_CHIP_PRODUCT_ID > > VIVS_HI_CHIP_ECO_ID > > need to be conditional - while > > VIVS_HI_CHIP_CUSTOMER_ID > > seems to be okay. > > > > Uhh.. okay.. just send a V2 - thanks for testing :) There is also something else going on with the GC600 - 5.4 worked fine, 5.8 doesn't - my 2D Xorg driver gets stuck waiting on a BO after just a couple of minutes. Looking in debugfs, there's a whole load of BOs that are listed as "active", yet the GPU is idle: 0002: A 0 ( 7) 8294400 0001: I 0 ( 1) 4096 0001: I 0 ( 1) 4096 0001: I 0 ( 1) 327680 0001: A 0 ( 7) 8388608 0001: I 0 ( 1) 8388608 0001: I 0 ( 1) 8388608 0001: A 0 ( 7) 8388608 0001: A 0 ( 3) 8388608 0001: A 0 ( 4) 8388608 0001: A 0 ( 3) 8388608 0001: A 0 ( 3) 8388608 0001: A 0 ( 3) 8388608 0001: A 0 ( 3) 8388608 Total 38 objects, 293842944 bytes My guess is there's something up with the way a job completes that's causing the BOs not to be marked inactive. I haven't yet been able to debug any further. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/imx: fix use after free
On Thu, Jun 11, 2020 at 02:43:31PM +0200, Marco Felsch wrote: > From: Philipp Zabel > > Component driver structures allocated with devm_kmalloc() in bind() are > freed automatically after unbind(). Since the contained drm structures > are accessed afterwards in drm_mode_config_cleanup(), move the > allocation into probe() to extend the driver structure's lifetime to the > lifetime of the device. This should eventually be changed to use drm > resource managed allocations with lifetime of the drm device. You need to be extremely careful doing this. If the allocation is in the probe function, it's lifetime is not just until unbind, but potentitally to the _next_ bind, unbind, bind, unbind. In other words, it's lifetime is from the point that the component is probed to the point that it is later removed. If the driver relies on initialisation of that structure, then that must be _very_ carefully handled - any state in that structure will remain. So, you need to think long and hard about changes like this, and do a thorough review of the lifetime of every structure member. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 503kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
On Wed, May 20, 2020 at 04:04:39PM +0200, Lucas Stach wrote: > Am Mittwoch, den 20.05.2020, 15:38 +0200 schrieb Lubomir Rintel: > > Yes. This means that in fact "core" is the only required clock for all > > implementations of vivante,gc and the common binding needs to be updated > > to reflect that. I'll follow with a patch that does that, unless there > > are strong objections. > > > > If there are implementations that require different clock inputs, then they > > need to use additional compatible string for the particular flavor and the > > binding should have conditionals for them. Something like this: > > > > if: > > properties: > > compatible: > > contains: > > const: fsl,imx6sx-gpu > > then: > > properties: > > clocks: > > minItems: 4 > > The DT binding of a device should describe the hardware of the device, > not the specific integration into a SoC. Now it's a bit hard to make > any definite statements about the Vivante GC GPU module itself, as most > of the information we have is from reverse engineering. It's pretty > clear though that the GPU module has at least 2 clock inputs: axi and > core, as there is a feature bit that tells us if it's okay to gate the > axi clock independently from core. > > I'm not 100% sure about the older cores as found in Dove, but all the > more recent cores allow to clock the shader partition independently of > the core partition, so that's another clock input. > > Now when it comes to a SoC integration, it's totally fine to have all > those GPU module clock inputs fed from the same clock source and behind > a shared gate maybe. But that doesn't change the clock inputs from the > device perspective, it's still 3 independent clock inputs, which then > just point to the same clock source in the DT. > > imx6sx.dtsi is even a precedent of such a setup: all module clock > inputs are fed by a common clock and share a single gate. It's very good to stand on a pedistal, and try to dictate what you want, but what you want is not always possible, and that is the case here. You have made your idea of how you see the hardware quite plain. Getting back to reality, again, what we know is that there is one clock for the GC600 on Dove, since are register settings to control that clock. What we also know is that there is an AHB clock, but how that relates to the rest of the system, we have no documentation. It is that is used for the AHB bus, which incidentally includes the GC600 for access to its register set. I don't remember whether AHB peripherals require the clock or not, that is something that would need to be checked. If they do, then it seems that clock is missing from the binding. Then there's a description of the AXI interface to the GC600 which mentions no clock (see 10.7.2 below). It has a clock, but what that clock is, and how it relates to the rest of the system is not clearly specified. So, again, what you're basically asking for is for someone to guess and throw some ficticious model into DT. No, if it's not known, then we should not be guessing and throwing random garbage into a SoC DT description - once it's in the DT description, it has to be supported going forward, and if it's later found to be incorrect, then we have a problem. So, I don't see how Dove can meet your requirements, and I think you have to compromise on this, and just accept that Dove only has one known clock. If you want to continue being pedantic about this, then in order to support that pedanty, we need to add an AHB clock to all Vivante GC descriptions as well since the AHB interface requires it - there it is called HCLK in the AHB specs. Here is the extract from the Dove documentation for the AHB interface: 10.7.1 AHB Interface The main features of the AHB interface are: n256k addressable register space n32-bit accesses only (no bursts) n32-bit data bus nError response for illegal accesses nAsynchronous interface to the Graphics Core nInterrupt support The interface uses a separate clock that is slower than the AXI Bus clock, thus allowing a more relaxed logic design. Because the Core clock is different from the interface clock, design within the GPU Core ensures that incoming and outgoing data are handled properly when they cross clock boundaries. The GPU block occupies 256 KB (64k 32-bit words) of the system address space. An AHB ERROR response is returned if an illegal access is detected. Only 32-bit reads and writes are permitted. 10.7.2 AXI Interface The main features of the AXI interface are:
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
On Thu, May 14, 2020 at 10:40:58AM +0200, Lucas Stach wrote: > Am Donnerstag, den 14.05.2020, 09:27 +0100 schrieb Russell King - ARM Linux > admin: > > On Thu, May 14, 2020 at 10:18:02AM +0200, Lucas Stach wrote: > > > Am Mittwoch, den 13.05.2020, 23:41 -0300 schrieb Fabio Estevam: > > > > On Wed, May 13, 2020 at 2:09 PM Fabio Estevam > > > > wrote: > > > > > > > > > The binding doc Documentation/devicetree/bindings/gpu/vivante,gc.yaml > > > > > says that only the 'reg' clock could be optional, the others are > > > > > required. > > > > > > > > arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. > > > > arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' > > > > > > > > Maybe the binding needs to be updated and it seems that using > > > > devm_clk_get_optional() like you propose is safe. > > > > > > The binding is correct as-is. We want to require those clocks to be > > > present, but the dove DT was added before the binding was finalized, so > > > the driver still treats the clocks as optional to not break > > > compatibility with old DTs. Maybe this warrants a comment in the > > > code... > > > > The binding doc in mainline says: > > > > clocks: > > items: > > - description: AXI/master interface clock > > - description: GPU core clock > > - description: Shader clock (only required if GPU has feature PIPE_3D) > > - description: AHB/slave interface clock (only required if GPU can > > gate slave interface independently) > > minItems: 1 > > maxItems: 4 > > > > clock-names: > > items: > > enum: [ bus, core, shader, reg ] > > minItems: 1 > > maxItems: 4 > > > > which looks correct to me - and means that Dove is compliant with that. > > The YAML binding actually did loose something in translation here, > which I didn't notice. Previously all those clocks were listed under > "Required properties", with the exceptions listed in parenthesis. So > the Dove GPU, which is a combined 2D/3D core should have axi, core and > shader clocks specified. That may be your desire, but that is impossible without knowing that (a) it has the clocks (b) what those clocks are connected to I guess we could "make something up" but as DT is supposed to describe hardware, I don't see how we can satisfy that and your requirement. The only thing that is known from the documentation is that there is one clock for the GPU on Dove. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
On Thu, May 14, 2020 at 10:18:02AM +0200, Lucas Stach wrote: > Am Mittwoch, den 13.05.2020, 23:41 -0300 schrieb Fabio Estevam: > > On Wed, May 13, 2020 at 2:09 PM Fabio Estevam wrote: > > > > > The binding doc Documentation/devicetree/bindings/gpu/vivante,gc.yaml > > > says that only the 'reg' clock could be optional, the others are > > > required. > > > > arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. > > arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' > > > > Maybe the binding needs to be updated and it seems that using > > devm_clk_get_optional() like you propose is safe. > > The binding is correct as-is. We want to require those clocks to be > present, but the dove DT was added before the binding was finalized, so > the driver still treats the clocks as optional to not break > compatibility with old DTs. Maybe this warrants a comment in the > code... The binding doc in mainline says: clocks: items: - description: AXI/master interface clock - description: GPU core clock - description: Shader clock (only required if GPU has feature PIPE_3D) - description: AHB/slave interface clock (only required if GPU can gate slave interface independently) minItems: 1 maxItems: 4 clock-names: items: enum: [ bus, core, shader, reg ] minItems: 1 maxItems: 4 which looks correct to me - and means that Dove is compliant with that. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: decruft the vmalloc API
On Wed, Apr 08, 2020 at 01:58:58PM +0200, Christoph Hellwig wrote: > Hi all, > > Peter noticed that with some dumb luck you can toast the kernel address > space with exported vmalloc symbols. > > I used this as an opportunity to decruft the vmalloc.c API and make it > much more systematic. This also removes any chance to create vmalloc > mappings outside the designated areas or using executable permissions > from modules. Besides that it removes more than 300 lines of code. I haven't read all your patches yet. Have you tested it on 32-bit ARM, where the module area is located _below_ PAGE_OFFSET and outside of the vmalloc area? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] Update my email address in various drivers
On Mon, Mar 30, 2020 at 08:04:44PM +0200, Sam Ravnborg wrote: > Hi Russell. > > On Sun, Mar 29, 2020 at 11:19:10AM +0100, Russell King wrote: > > Globally update my email address in six files scattered through the > > tree. > > > > Signed-off-by: Russell King > > --- > > drivers/gpu/drm/armada/armada_drv.c | 2 +- > > drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 2 +- > > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 2 +- > > drivers/media/cec/cec-notifier.c| 2 +- > > drivers/net/phy/swphy.c | 2 +- > > include/media/cec-notifier.h| 2 +- > > 6 files changed, 6 insertions(+), 6 deletions(-) > > This changes all cases of: > >rmk+ker...@arm.linux.org.uk > > to > > rmk+ker...@armlinux.org.uk or no mail address. Correct. This is the address I sign off all my commits with, and this is the one I use to associate with authorship because it uses my initials. > But I am confused. > > The new address does not appear anywhere in MAINTAINERS and is used > only in three other files. MAINTAINERS lists the addresses I prefer email for the day to day maintanence, which is my linux@ accounts. The above addresses also fall into _this_ mailbox too, rather than my rmk@ mailbox. So, ultimately all that email comes to the same place. However, the plain rmk@ address doesn't. > And there are a few other mail addresses that would reach you. > But no matter how confused I am the patch looks fine so: > > Acked-by: Sam Ravnborg > > And if the change is for private reaons then I do not have to know > anyway so feel free to ignore my confusion. The reason for the change is so I can drop the routing information rmk+ker...@arm.linux.org.uk, thereby causing that address to start bouncing, rather than being a spam inlet. Sure, the new one will be as well, but the point is that keeping both around indefinitely gives a bigger attack surface for spam ingress. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 18/51] drm/: Use drmm_add_final_kfree
On Fri, Feb 21, 2020 at 10:02:46PM +0100, Daniel Vetter wrote: > These are the leftover drivers that didn't have a ->release hook that > needed to be updated. > > Signed-off-by: Daniel Vetter > Cc: "James (Qian) Wang" > Cc: Liviu Dudau > Cc: Mihail Atanassov > Cc: Russell King > Cc: Hans de Goede > --- > drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 2 ++ > drivers/gpu/drm/armada/armada_drv.c | 2 ++ > drivers/gpu/drm/vboxvideo/vbox_drv.c| 2 ++ > 3 files changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/armada/armada_drv.c > b/drivers/gpu/drm/armada/armada_drv.c > index 197dca3fc84c..dd9ed71ed942 100644 > --- a/drivers/gpu/drm/armada/armada_drv.c > +++ b/drivers/gpu/drm/armada/armada_drv.c > @@ -12,6 +12,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -103,6 +104,7 @@ static int armada_drm_bind(struct device *dev) > kfree(priv); > return ret; > } > + drmm_add_final_kfree(>drm, priv); > > /* Remove early framebuffers */ > ret = drm_fb_helper_remove_conflicting_framebuffers(NULL, I have no visibility of what the changes behind this are, so I can't ack this change. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/etnaviv: only reject timeouts with tv_nsec >= 2 seconds
On Wed, Jan 22, 2020 at 11:30:34AM +0100, Guido Günther wrote:
> Hi,
> On Tue, Jan 21, 2020 at 08:05:27PM +0100, Arnd Bergmann wrote:
> > On Tue, Jan 21, 2020 at 5:10 PM Lucas Stach wrote:
> > >
> > > Hi Guido,
> > >
> > > On Di, 2020-01-21 at 13:55 +0100, Guido Günther wrote:
> > > > Hi,
> > > > On Tue, Jan 21, 2020 at 12:45:25PM +0100, Arnd Bergmann wrote:
> > > > > As Guido Günther reported, get_abs_timeout() in the etnaviv user space
> > > > > sometimes passes timeouts with nanosecond values larger than
> > > > > 10,
> > > > > which gets rejected after my first patch.
> > > > >
> > > > > To avoid breaking this, while also not allowing completely arbitrary
> > > > > values, set the limit to 19 and use
> > > > > set_normalized_timespec64()
> > > > > to get the correct format before comparing it.
> > > >
> > > > I'm seeing values up to 5 seconds so I need
> > > >
> > > > if (args->timeout.tv_nsec > (5 * NSEC_PER_SEC))
> > > >
> > > > to unbreak rendering. Which seems to match what mesa's get_abs_timeout()
> > > > does and how it's invoked.
> > >
> > > I have not tested this myself yet, only looked at the code. From the
> > > code I quoted earlier, I don't see how we end up with 5 * NSEC_PER_SEC
> > > in the tv_nsec member, even if the timeout passed to get_abs_timeout()
> > > is 5 seconds.
> >
> > I can think of two different ways you'd end up with around five seconds
> > here:
> >
> > a) you have a completely arbitrary 32-bit number through truncation,
> > which is up to 4.2 seconds
> > b) you have the same kind of 32-bit number, but add up to another 9
> > nanoseconds, so you get up to 5.2 seconds in the 64-bit field.
>
> I've dumped out some values tv_nsec values with current mesa git on arm64:
>
> [ 33.699652] etnaviv_ioctl_gem_cpu_prep: 4990449401
> [ 33.813081] etnaviv_ioctl_gem_cpu_prep: 5103872445
> [ 33.822936] etnaviv_ioctl_gem_cpu_prep: 5113731286
> [ 33.840963] etnaviv_ioctl_gem_cpu_prep: 5131762726
> [ 33.854120] etnaviv_ioctl_gem_cpu_prep: 5144920127
> [ 33.861426] etnaviv_ioctl_gem_cpu_prep: 5152227527
> [ 33.872666] etnaviv_ioctl_gem_cpu_prep: 5163466968
> [ 33.879485] etnaviv_ioctl_gem_cpu_prep: 5170286808
>
> The problem is that in mesa/libdrm
>
> static inline void get_abs_timeout(struct drm_etnaviv_timespec *tv, uint64_t
> ns)
> {
> struct timespec t;
> uint32_t s = ns / 10;
> clock_gettime(CLOCK_MONOTONIC, );
> tv->tv_sec = t.tv_sec + s;
> tv->tv_nsec = t.tv_nsec + ns - (s * 10);
> ^^^
>this overflows (since `s` is `uint_32t` and hence we substract a way
>too small value with ns = 50 which mesa uses in
>etna_bo_cpu_prep.
> }
>
> So with current mesa/libdrm (which needs to be fixed) we'd have a maximum
>
> t.tv_nsec + ns - (s_max * 10)
>
> 9 + 50 - 705032704= 5294967295
>
> Does that make sense? If so that'd be the possible upper bound for the
> kernel. Note that this only applies to etnaviv_ioctl_gem_cpu_prep. While
> etnaviv_ioctl_wait_fence and etnaviv_ioctl_gem_wait are affected too
> i've not yet seen user space passing in larger values.
Except the fact that the calculation being done above is buggy.
Not only do we end up with tv_sec incremented by 5 seconds, but
we also end up with tv_nsec containing around 5 seconds in
nanoseconds, which means we end up with about a 10 second timeout.
I think it would probably be better for the kernel to print a
warning once when noticing over-large nsec values, suggesting a
userspace upgrade is in order, but continue the existing behaviour.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/etnaviv: only reject timeouts with tv_nsec >= 2 seconds
On Tue, Jan 21, 2020 at 01:55:46PM +0100, Guido Günther wrote: > Hi, > On Tue, Jan 21, 2020 at 12:45:25PM +0100, Arnd Bergmann wrote: > > As Guido Günther reported, get_abs_timeout() in the etnaviv user space > > sometimes passes timeouts with nanosecond values larger than 10, > > which gets rejected after my first patch. > > > > To avoid breaking this, while also not allowing completely arbitrary > > values, set the limit to 19 and use set_normalized_timespec64() > > to get the correct format before comparing it. > > I'm seeing values up to 5 seconds so I need > > if (args->timeout.tv_nsec > (5 * NSEC_PER_SEC)) I assume you're looking at 64-bit, but I suspect userspace needs looking at considering 32-bit. If userspace uses a 32-bit tv_nsec anywhere in the path that it attempts to pass up to 5 seconds in tv_nsec, then this will fail to pass the correct timeout. If that is the case, userspace is buggy, and needs fixing not to pass such large values through tv_nsec irrespective of this issue. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH for-5.6 0/2] drm/bridge: dw-hdmi: PCM API updates
On Thu, Jan 09, 2020 at 10:10:09AM +0100, Takashi Iwai wrote: > On Tue, 10 Dec 2019 16:45:34 +0100, > Takashi Iwai wrote: > > > > Hi, > > > > this is a patch set for updating ALSA PCM API usages in dw-hdmi > > driver. I already tried to "fix" the driver some time ago but it was > > utterly wrong. So this is a combination of the revised patch and > > another cleanup patch. > > > > The first one is to change the buffer allocation mechanism in the > > driver to the manual allocation of the h/w buffer and the automatic > > allocation of PCM stream buffers via the new standard API. The > > significant change is that size of the h/w buffer isn't no longer > > controlled via ALSA preallocation proc file but rather via the new > > module option (if any). > > > > The second one is a oneliner patch just to remove the superfluous PCM > > ops. > > > > Both need the ALSA PCM core changes in 5.5-rc1, so please apply them > > on top of 5.5-rc1 or later. Or, just let me know if I should apply > > them through sound git tree. > > > > > > thanks, > > > > Takashi > > > > === > > > > Takashi Iwai (2): > > drm/bridge: dw-hdmi: Follow the standard ALSA memalloc way > > drm/bridge: dw-hdmi: Drop superfluous ioctl PCM ops > > Any chance for reviewing these patches? > > Since this driver is the only one who is still using the old ALSA > vmalloc API, I'd like to change it and drop the old API in 5.6. It isn't something I can even test at the moment; I have the platforms but no TV to connect them to. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/30] drm/bridge: Add device links for lifetime control
What happened with the patches I posted doing exactly this? On Tue, Nov 26, 2019 at 01:15:58PM +, Mihail Atanassov wrote: > Hi all, > > This series adds device links support to drm_bridge. The motivation > behind it is that a drm_bridge in a module could get removed under the > feet of the bridge user without warning, so we need a way to remove and > reprobe the client as needed to avoid peering into the void. > > 1: Add a drm_bridge_init() function which wraps all initialisation of > the structure prior to calling drm_bridge_add(). > > 2-26,28: Apply the drm_bridge_init() refactor to every bridge that uses > drm_bridge_add(). > > 27: Minor cleanup in rcar-du. > > 29: Add of_drm_find_bridge_devlink() which functions the same as > of_drm_find_bridge() plus adds a device device link from the owning > drm_device to the bridge device. > > 30: As a motivating example, convert komeda to exclusively use > drm_bridge for its pipe outputs; this isn't a regression in usability > any more since device links bring the same automatic remove/reprobe > feature as components. > > Mihail Atanassov (29): > drm: Introduce drm_bridge_init() > drm/bridge: adv7511: Use drm_bridge_init() > drm/bridge: anx6345: Use drm_bridge_init() > drm/bridge: anx78xx: Use drm_bridge_init() > drm/bridge: cdns: Use drm_bridge_init() > drm/bridge: dumb-vga-dac: Use drm_bridge_init() > drm/bridge: lvds-encoder: Use drm_bridge_init() > drm/bridge: megachips-stdp-ge-b850v3-fw: Use drm_bridge_init() > drm/bridge: nxp-ptn3460: Use drm_bridge_init() > drm/bridge: panel: Use drm_bridge_init() > drm/bridge: ps8622: Use drm_bridge_init() > drm/bridge: sii902x: Use drm_bridge_init() > gpu: drm: bridge: sii9234: Use drm_bridge_init() > drm/bridge: sil_sii8620: Use drm_bridge_init() > drm/bridge: dw-hdmi: Use drm_bridge_init() > drm/bridge/synopsys: dsi: Use drm_bridge_init() > drm/bridge: tc358764: Use drm_bridge_init() > drm/bridge: tc358767: Use drm_bridge_init() > drm/bridge: thc63: Use drm_bridge_init() > drm/bridge: ti-sn65dsi86: Use drm_bridge_init() > drm/bridge: ti-tfp410: Use drm_bridge_init() > drm/exynos: mic: Use drm_bridge_init() > drm/i2c: tda998x: Use drm_bridge_init() > drm/mcde: dsi: Use drm_bridge_init() > drm/mediatek: hdmi: Use drm_bridge_init() > drm: rcar-du: lvds: Use drm_bridge_init() > drm: rcar-du: lvds: Don't set drm_bridge private pointer > drm/sti: sti_vdo: Use drm_bridge_init() > drm/komeda: Use drm_bridge interface for pipe outputs > > Russell King (1): > drm/bridge: add support for device links to bridge > > .../gpu/drm/arm/display/komeda/komeda_drv.c | 54 ++--- > .../gpu/drm/arm/display/komeda/komeda_kms.c | 77 -- > .../gpu/drm/arm/display/komeda/komeda_kms.h | 2 + > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 +- > .../drm/bridge/analogix/analogix-anx6345.c| 5 +- > .../drm/bridge/analogix/analogix-anx78xx.c| 8 +- > drivers/gpu/drm/bridge/cdns-dsi.c | 4 +- > drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 +- > drivers/gpu/drm/bridge/lvds-encoder.c | 7 +- > .../bridge/megachips-stdp-ge-b850v3-fw.c | 4 +- > drivers/gpu/drm/bridge/nxp-ptn3460.c | 4 +- > drivers/gpu/drm/bridge/panel.c| 7 +- > drivers/gpu/drm/bridge/parade-ps8622.c| 3 +- > drivers/gpu/drm/bridge/sii902x.c | 5 +- > drivers/gpu/drm/bridge/sii9234.c | 3 +- > drivers/gpu/drm/bridge/sil-sii8620.c | 3 +- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 7 +- > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 7 +- > drivers/gpu/drm/bridge/tc358764.c | 4 +- > drivers/gpu/drm/bridge/tc358767.c | 3 +- > drivers/gpu/drm/bridge/thc63lvd1024.c | 7 +- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 +- > drivers/gpu/drm/bridge/ti-tfp410.c| 5 +- > drivers/gpu/drm/drm_bridge.c | 78 +++ > drivers/gpu/drm/exynos/exynos_drm_mic.c | 8 +- > drivers/gpu/drm/i2c/tda998x_drv.c | 6 +- > drivers/gpu/drm/mcde/mcde_dsi.c | 3 +- > drivers/gpu/drm/mediatek/mtk_hdmi.c | 4 +- > drivers/gpu/drm/rcar-du/rcar_lvds.c | 5 +- > drivers/gpu/drm/sti/sti_dvo.c | 4 +- > include/drm/drm_bridge.h | 8 ++ > 31 files changed, 217 insertions(+), 134 deletions(-) > > -- > 2.23.0 > > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 05/15] drm/armada: Delete dma_buf->k(un)map implemenation
On Mon, Nov 25, 2019 at 10:44:43PM +0100, Daniel Vetter wrote:
> On Mon, Nov 18, 2019 at 11:35:26AM +0100, Daniel Vetter wrote:
> > It's a dummy anyway.
> >
> > Signed-off-by: Daniel Vetter
> > Cc: Russell King
>
> I merged the entire series except this one and the final patch, sill
> waiting a bit more for an ack on this perhaps.
Acked-by: Russell King
I thought drm trees closed around -rc6?
> -Daniel
>
> > ---
> > drivers/gpu/drm/armada/armada_gem.c | 12
> > 1 file changed, 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/armada/armada_gem.c
> > b/drivers/gpu/drm/armada/armada_gem.c
> > index 93cf8b8bfcff..976685f2939e 100644
> > --- a/drivers/gpu/drm/armada/armada_gem.c
> > +++ b/drivers/gpu/drm/armada/armada_gem.c
> > @@ -461,16 +461,6 @@ static void armada_gem_prime_unmap_dma_buf(struct
> > dma_buf_attachment *attach,
> > kfree(sgt);
> > }
> >
> > -static void *armada_gem_dmabuf_no_kmap(struct dma_buf *buf, unsigned long
> > n)
> > -{
> > - return NULL;
> > -}
> > -
> > -static void
> > -armada_gem_dmabuf_no_kunmap(struct dma_buf *buf, unsigned long n, void
> > *addr)
> > -{
> > -}
> > -
> > static int
> > armada_gem_dmabuf_mmap(struct dma_buf *buf, struct vm_area_struct *vma)
> > {
> > @@ -481,8 +471,6 @@ static const struct dma_buf_ops
> > armada_gem_prime_dmabuf_ops = {
> > .map_dma_buf= armada_gem_prime_map_dma_buf,
> > .unmap_dma_buf = armada_gem_prime_unmap_dma_buf,
> > .release= drm_gem_dmabuf_release,
> > - .map= armada_gem_dmabuf_no_kmap,
> > - .unmap = armada_gem_dmabuf_no_kunmap,
> > .mmap = armada_gem_dmabuf_mmap,
> > };
> >
> > --
> > 2.24.0
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Incorrect buffer handling in dw-hdmi bridge audio
On Tue, Nov 05, 2019 at 05:44:26PM +0100, Takashi Iwai wrote: > On Tue, 05 Nov 2019 17:33:44 +0100, > Takashi Iwai wrote: > > > > On Tue, 05 Nov 2019 17:02:15 +0100, > > Russell King - ARM Linux admin wrote: > > > > > > On Tue, Nov 05, 2019 at 09:07:43AM +0100, Neil Armstrong wrote: > > > > Hi, > > > > > > > > On 05/11/2019 08:55, Takashi Iwai wrote: > > > > > Hi, > > > > > > > > > > while recently working on the ALSA memory allocator API cleanup, I > > > > > noticed that dw-hdmi bridge driver seems doing weird about the buffer > > > > > management. It pre-allocates the usual device buffers fully at the > > > > > probe time, while each stream allocates the buffer via the vmalloc > > > > > helpers and replaces with it at each open. > > > > > > > > > > I guess it's no expected behavior? It's basically a full waste of > > > > > resources, and the vmalloc buffer isn't guaranteed to work well for > > > > > mmap on every architecture. > > > > > > > > > > Below is the patch to address it. Can anyone check whether this still > > > > > works? > > > > > > > > I don't have the setup to check, but this has been pushed by Russell I > > > > Added in CC. > > > > > > > > I also added the imx maintainer since it's (AFAIK) only used on iMX > > > > SoCs. > > > > > > > > Neil > > > > > > > > > > > > > > Since I have a cleanup series and this is involved, I'd like to take > > > > > the fix through my tree once after it's confirmed (and get ACK if > > > > > possible). > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > Takashi > > > > > > > > > > -- 8< -- > > > > > From: Takashi Iwai > > > > > Subject: [PATCH] drm/bridge: dw-hdmi: Fix the incorrect buffer > > > > > allocations > > > > > > > > > > The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV, > > > > > while it re-allocates and releases vmalloc pages. This is not only a > > > > > lot of waste resources but also causes the mmap malfunction. > > > > > > > > > > Change / drop the vmalloc-related code and use the standard buffer > > > > > allocation / release code instead. > > > > > > I think getting rid of the vmalloc code here is a mistake - I seem to > > > remember using the standard buffer allocation causes failures, due to > > > memory fragmentation. Since the hardware is limited to DMA from at > > > most one page, there is no reason for this driver to require contiguous > > > pages, hence why it's using - and should use - vmalloc pages. vmalloc > > > is way kinder to the MM subsystem than trying to request large order > > > contiguous pages. > > > > > > So, NAK on this patch. > > > > OK, then we should do other way round, rather drop the buffer > > preallocation instead. Currently vmalloc buffer is always allocated > > at each open and overrides the preallocated buffer, so the whole 64k > > and more are wasted for no use. > > > > (BTW, the current code has this snippet: > > > > /* Limit the buffer size to the size of the preallocated buffer */ > > ret = snd_pcm_hw_constraint_minmax(runtime, > >SNDRV_PCM_HW_PARAM_BUFFER_SIZE, > >0, substream->dma_buffer.bytes); > > > > ... and this would have to limit the buffer size only to the > > preallocated size -- which essentially makes the argument above > > invalid. However, this check looks actually bogus, the constraint > > parameter should be SNDRV_PCM_HW_PARAM_BUFFER_BYTES, not _SIZE. It > > might be the reason it worked somehow...) > > > > So below is the revised patch. Could you guys check it again? > > > > There I copied the comment as is, although the 512k mentioned there > > looks inconsistent with the actual code. Should it be 1M? > > ... and reading the patch again, I found that the hw constraint call > can be dropped as well. The dw_hdmi_hw definition already contains > the max buffer size. > > Below is the re-revised patch. Please check it. I was slightly wrong - sorry. It's been a long time since I looked at t
Re: Incorrect buffer handling in dw-hdmi bridge audio
On Tue, Nov 05, 2019 at 09:07:43AM +0100, Neil Armstrong wrote:
> Hi,
>
> On 05/11/2019 08:55, Takashi Iwai wrote:
> > Hi,
> >
> > while recently working on the ALSA memory allocator API cleanup, I
> > noticed that dw-hdmi bridge driver seems doing weird about the buffer
> > management. It pre-allocates the usual device buffers fully at the
> > probe time, while each stream allocates the buffer via the vmalloc
> > helpers and replaces with it at each open.
> >
> > I guess it's no expected behavior? It's basically a full waste of
> > resources, and the vmalloc buffer isn't guaranteed to work well for
> > mmap on every architecture.
> >
> > Below is the patch to address it. Can anyone check whether this still
> > works?
>
> I don't have the setup to check, but this has been pushed by Russell I Added
> in CC.
>
> I also added the imx maintainer since it's (AFAIK) only used on iMX SoCs.
>
> Neil
>
> >
> > Since I have a cleanup series and this is involved, I'd like to take
> > the fix through my tree once after it's confirmed (and get ACK if
> > possible).
> >
> >
> > Thanks!
> >
> > Takashi
> >
> > -- 8< --
> > From: Takashi Iwai
> > Subject: [PATCH] drm/bridge: dw-hdmi: Fix the incorrect buffer allocations
> >
> > The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV,
> > while it re-allocates and releases vmalloc pages. This is not only a
> > lot of waste resources but also causes the mmap malfunction.
> >
> > Change / drop the vmalloc-related code and use the standard buffer
> > allocation / release code instead.
I think getting rid of the vmalloc code here is a mistake - I seem to
remember using the standard buffer allocation causes failures, due to
memory fragmentation. Since the hardware is limited to DMA from at
most one page, there is no reason for this driver to require contiguous
pages, hence why it's using - and should use - vmalloc pages. vmalloc
is way kinder to the MM subsystem than trying to request large order
contiguous pages.
So, NAK on this patch.
> >
> > Signed-off-by: Takashi Iwai
> > ---
> > drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 6 ++
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
> > index 2b7539701b42..8fe7a6e8ff94 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
> > @@ -384,15 +384,14 @@ static int dw_hdmi_close(struct snd_pcm_substream
> > *substream)
> >
> > static int dw_hdmi_hw_free(struct snd_pcm_substream *substream)
> > {
> > - return snd_pcm_lib_free_vmalloc_buffer(substream);
> > + return snd_pcm_lib_free_pages(substream);
> > }
> >
> > static int dw_hdmi_hw_params(struct snd_pcm_substream *substream,
> > struct snd_pcm_hw_params *params)
> > {
> > /* Allocate the PCM runtime buffer, which is exposed to userspace. */
> > - return snd_pcm_lib_alloc_vmalloc_buffer(substream,
> > - params_buffer_bytes(params));
> > + return snd_pcm_lib_malloc_pages(substream, params_buffer_bytes(params));
> > }
> >
> > static int dw_hdmi_prepare(struct snd_pcm_substream *substream)
> > @@ -511,7 +510,6 @@ static const struct snd_pcm_ops snd_dw_hdmi_ops = {
> > .prepare = dw_hdmi_prepare,
> > .trigger = dw_hdmi_trigger,
> > .pointer = dw_hdmi_pointer,
> > - .page = snd_pcm_lib_get_vmalloc_page,
> > };
> >
> > static int snd_dw_hdmi_probe(struct platform_device *pdev)
> >
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Tue, Oct 22, 2019 at 03:42:07PM +0100, Russell King - ARM Linux admin wrote: > On Tue, Oct 22, 2019 at 10:50:42AM +0200, Daniel Vetter wrote: > > On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin > > wrote: > > > I had a patches, which is why I raised the problem with the core: > > > > > > 6961edfee26d bridge hacks using device links > > > > > > but it never went further than an experiment at the time because of the > > > problems in the core. As it was a hack, it never got posted. Seems > > > that kernel tree (for the cubox) is still 5.2 based, so has a lot of > > > patches and might need updating to a more recent base before anything > > > can be tested. > > > > > > For reference, the panel patch: > > > > https://patchwork.kernel.org/patch/10364873/ > > > > And the huge discussion around bridges, that resulted in Rafael > > Wyzocki fixing all the core issues: > > > > https://www.spinics.net/lists/dri-devel/msg201927.html > > > > James, do you want to look into this for bridges? > > I can now confirm that it does work. Something like this - it's based off an older kernel, so may be missing some of the bridge drivers, but should be sufficient for people to test with. 8< From: Russell King Subject: [PATCH] drm/bridge: add support for device links to bridge Bridge devices have been a potential for kernel oops as their lifetime is independent of the DRM device that they are bound to. Hence, if a bridge device is unbound while the parent DRM device is using them, the parent happily continues to use the bridge device, calling the driver and accessing its objects that have been freed. This can cause kernel memory corruption and kernel oops. To control this, use device links to ensure that the parent DRM device is unbound when the bridge device is unbound, and when the bridge device is re-bound, automatically rebind the parent DRM device. Signed-off-by: Russell King --- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 1 + drivers/gpu/drm/bridge/analogix-anx78xx.c | 1 + drivers/gpu/drm/bridge/dumb-vga-dac.c | 1 + drivers/gpu/drm/bridge/lvds-encoder.c | 1 + .../bridge/megachips-stdp-ge-b850v3-fw.c | 1 + drivers/gpu/drm/bridge/nxp-ptn3460.c | 1 + drivers/gpu/drm/bridge/panel.c| 1 + drivers/gpu/drm/bridge/parade-ps8622.c| 1 + drivers/gpu/drm/bridge/sii902x.c | 1 + drivers/gpu/drm/bridge/sii9234.c | 1 + drivers/gpu/drm/bridge/sil-sii8620.c | 1 + drivers/gpu/drm/bridge/tc358767.c | 1 + drivers/gpu/drm/bridge/ti-tfp410.c| 1 + drivers/gpu/drm/drm_bridge.c | 48 ++- drivers/gpu/drm/i2c/tda998x_drv.c | 1 + include/drm/drm_bridge.h | 4 ++ 16 files changed, 53 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index f6d2681f6927..6a5906da58ea 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1217,6 +1217,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) goto err_unregister_cec; adv7511->bridge.funcs = _bridge_funcs; + adv7511->bridge.device = dev; adv7511->bridge.of_node = dev->of_node; drm_bridge_add(>bridge); diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c index 3c7cc5af735c..77ff17c38037 100644 --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c @@ -1323,6 +1323,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client, mutex_init(>lock); + anx78xx->bridge.device = >dev; #if IS_ENABLED(CONFIG_OF) anx78xx->bridge.of_node = client->dev.of_node; #endif diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c index d32885b906ae..40169920560e 100644 --- a/drivers/gpu/drm/bridge/dumb-vga-dac.c +++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c @@ -202,6 +202,7 @@ static int dumb_vga_probe(struct platform_device *pdev) } vga->bridge.funcs = _vga_bridge_funcs; + vga->bridge.device = >dev; vga->bridge.of_node = pdev->dev.of_node; vga->bridge.timings = of_device_get_match_data(>dev); diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c index 2ab2c234f26c..5012be35a5fb 100644 --- a/drivers/gpu/drm/bridge/lvds-encoder.c +++ b/drivers/gpu/drm/bridge/lvds-encoder.c @@ -115,6 +115,7 @@ static int lvds_encoder_probe(struct platform_device *pdev) * to look up. */ lvds_encoder-&
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Tue, Oct 22, 2019 at 10:50:42AM +0200, Daniel Vetter wrote: > On Tue, Oct 22, 2019 at 10:48 AM Russell King - ARM Linux admin > wrote: > > I had a patches, which is why I raised the problem with the core: > > > > 6961edfee26d bridge hacks using device links > > > > but it never went further than an experiment at the time because of the > > problems in the core. As it was a hack, it never got posted. Seems > > that kernel tree (for the cubox) is still 5.2 based, so has a lot of > > patches and might need updating to a more recent base before anything > > can be tested. > > > For reference, the panel patch: > > https://patchwork.kernel.org/patch/10364873/ > > And the huge discussion around bridges, that resulted in Rafael > Wyzocki fixing all the core issues: > > https://www.spinics.net/lists/dri-devel/msg201927.html > > James, do you want to look into this for bridges? I can now confirm that it does work. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Tue, Oct 22, 2019 at 10:42:10AM +0200, Daniel Vetter wrote: > On Thu, Oct 17, 2019 at 12:41:37PM +0100, Russell King - ARM Linux admin > wrote: > > On Thu, Oct 17, 2019 at 10:48:12AM +, Brian Starkey wrote: > > > On Thu, Oct 17, 2019 at 10:21:03AM +, james qian wang (Arm Technology > > > China) wrote: > > > > On Thu, Oct 17, 2019 at 08:20:56AM +, Brian Starkey wrote: > > > > > On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm > > > > > Technology China) wrote: > > > > > > On Wed, Oct 16, 2019 at 04:22:07PM +, Brian Starkey wrote: > > > > > > > > > > > > > > If James is strongly against merging this, maybe we just swap > > > > > > > wholesale to bridge? But for me, the pragmatic approach would be > > > > > > > this > > > > > > > stop-gap. > > > > > > > > > > > > > > > > > > > This is a good idea, and I vote +ULONG_MAX :) > > > > > > > > > > > > and I also checked tda998x driver, it supports bridge. so swap the > > > > > > wholesale to brige is perfect. :) > > > > > > > > > > > > > > > > Well, as Mihail wrote, it's definitely not perfect. > > > > > > > > > > Today, if you rmmod tda998x with the DPU driver still loaded, > > > > > everything will be unbound gracefully. > > > > > > > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge > > > > > driver the DPU is using) with the DPU driver still loaded will result > > > > > in a crash. > > > > > > > > I haven't read the bridge code, but seems this is a bug of drm_bridge, > > > > since if the bridge is still in using by others, the rmmod should fail > > > > > > > > > > Correct, but there's no fix for that today. You can also take a look > > > at the thread linked from Mihail's cover letter. > > > > > > > And personally opinion, if the bridge doesn't handle the dependence. > > > > for us: > > > > > > > > - add such support to bridge > > > > > > That would certainly be helpful. I don't know if there's consensus on > > > how to do that. > > > > > > > or > > > > - just do the insmod/rmmod in correct order. > > > > > > > > > So, there really are proper benefits to sticking with the component > > > > > code for tda998x, which is why I'd like to understand why you're so > > > > > against this patch? > > > > > > > > > > > > > This change handles two different connectors in komeda internally, > > > > compare > > > > with one interface, it increases the complexity, more risk of bug and > > > > more > > > > cost of maintainance. > > > > > > > > > > Well, it's only about how to bind the drivers - two different methods > > > of binding, not two different connectors. I would argue that carrying > > > our out-of-tree patches to support both platforms is a larger > > > maintenance burden. > > > > > > Honestly this looks like a win-win to me. We get the superior approach > > > when its supported, and still get to support bridges which are more > > > common. > > > > > > As/when improvements are made to the bridge code we can remove the > > > component bits and not lose anything. > > > > There was an idea a while back about using the device links code to > > solve the bridge issue - but at the time the device links code wasn't > > up to the job. I think that's been resolved now, but I haven't been > > able to confirm it. I did propose some patches for bridge at the > > time but they probably need updating. > > I think the only patches that existed where for panel, and we only > discussed the bridge case. At least I can only find patches for panel,not > bridge, but might be missing something. I had a patches, which is why I raised the problem with the core: 6961edfee26d bridge hacks using device links but it never went further than an experiment at the time because of the problems in the core. As it was a hack, it never got posted. Seems that kernel tree (for the cubox) is still 5.2 based, so has a lot of patches and might need updating to a more recent base before anything can be tested. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints
On Thu, Oct 17, 2019 at 10:48:12AM +, Brian Starkey wrote: > On Thu, Oct 17, 2019 at 10:21:03AM +, james qian wang (Arm Technology > China) wrote: > > On Thu, Oct 17, 2019 at 08:20:56AM +, Brian Starkey wrote: > > > On Thu, Oct 17, 2019 at 03:07:59AM +, james qian wang (Arm Technology > > > China) wrote: > > > > On Wed, Oct 16, 2019 at 04:22:07PM +, Brian Starkey wrote: > > > > > > > > > > If James is strongly against merging this, maybe we just swap > > > > > wholesale to bridge? But for me, the pragmatic approach would be this > > > > > stop-gap. > > > > > > > > > > > > > This is a good idea, and I vote +ULONG_MAX :) > > > > > > > > and I also checked tda998x driver, it supports bridge. so swap the > > > > wholesale to brige is perfect. :) > > > > > > > > > > Well, as Mihail wrote, it's definitely not perfect. > > > > > > Today, if you rmmod tda998x with the DPU driver still loaded, > > > everything will be unbound gracefully. > > > > > > If we swap to bridge, then rmmod'ing tda998x (or any other bridge > > > driver the DPU is using) with the DPU driver still loaded will result > > > in a crash. > > > > I haven't read the bridge code, but seems this is a bug of drm_bridge, > > since if the bridge is still in using by others, the rmmod should fail > > > > Correct, but there's no fix for that today. You can also take a look > at the thread linked from Mihail's cover letter. > > > And personally opinion, if the bridge doesn't handle the dependence. > > for us: > > > > - add such support to bridge > > That would certainly be helpful. I don't know if there's consensus on > how to do that. > > > or > > - just do the insmod/rmmod in correct order. > > > > > So, there really are proper benefits to sticking with the component > > > code for tda998x, which is why I'd like to understand why you're so > > > against this patch? > > > > > > > This change handles two different connectors in komeda internally, compare > > with one interface, it increases the complexity, more risk of bug and more > > cost of maintainance. > > > > Well, it's only about how to bind the drivers - two different methods > of binding, not two different connectors. I would argue that carrying > our out-of-tree patches to support both platforms is a larger > maintenance burden. > > Honestly this looks like a win-win to me. We get the superior approach > when its supported, and still get to support bridges which are more > common. > > As/when improvements are made to the bridge code we can remove the > component bits and not lose anything. There was an idea a while back about using the device links code to solve the bridge issue - but at the time the device links code wasn't up to the job. I think that's been resolved now, but I haven't been able to confirm it. I did propose some patches for bridge at the time but they probably need updating. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv8 2/2] drm: tda998x: set the connector info
On Thu, Oct 17, 2019 at 11:26:38AM +0200, Dariusz Marcinkiewicz wrote:
> Hi Russel.
>
> On Wed, Oct 16, 2019 at 6:22 PM Russell King - ARM Linux admin
> wrote:
> >
> ...
> > > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > @@ -1337,6 +1337,8 @@ static int tda998x_connector_init(struct
> > > tda998x_priv *priv,
> > > struct drm_device *drm)
> > > {
> > > struct drm_connector *connector = >connector;
> > > + struct cec_connector_info conn_info;
> > > + struct cec_notifier *notifier;
> > > int ret;
> > >
> > > connector->interlace_allowed = 1;
> > > @@ -1353,6 +1355,17 @@ static int tda998x_connector_init(struct
> > > tda998x_priv *priv,
> > > if (ret)
> > > return ret;
> > >
> > > + cec_fill_conn_info_from_drm(_info, connector);
> > > +
> > > + notifier = cec_notifier_conn_register(priv->cec_glue.parent,
> > > + NULL, _info);
> > > + if (!notifier)
> > > + return -ENOMEM;
> > > +
> > > + mutex_lock(>cec_notify_mutex);
> > > + priv->cec_notify = notifier;
> > > + mutex_unlock(>cec_notify_mutex);
> >
> > As per my previous comments, this is a single-copy atomic operation.
> > Either priv->cec_notify is set or it isn't; there is no intermediate
> > value. It can't be set to a value until cec_notifier_conn_register()
> > has completed. So the lock doesn't help.
> >
>
> > > +
> > > drm_connector_attach_encoder(>connector,
> > >priv->bridge.encoder);
> > >
> > > @@ -1372,6 +1385,11 @@ static void tda998x_bridge_detach(struct
> > > drm_bridge *bridge)
> ...
> > > + mutex_lock(>cec_notify_mutex);
> > > + cec_notifier_conn_unregister(priv->cec_notify);
> > > + priv->cec_notify = NULL;
> > > + mutex_unlock(>cec_notify_mutex);
> >
> > This is the only case where the lock makes sense - to ensure that any
> > of the cec_notifier_set_phys_addr*() functions aren't called
> > concurrently with it. However, there's no locking around the instance
> > in tda998x_connector_get_modes(), so have you ensured that that
> > function can't be called concurrently?
> >
> I assumed that tda998x_connector_get_modes does not need to be
> synchronized as it belongs to the connector that gets cleaned up here.
> But, on a closer look, I don't think that this assumption necessarily
> holds.
>
> If this patch is to be merged, I can send an update that:
> - strips locking from tda998x_connector_init,
> - in tda998x_connector_get_modes calls cec_notifier* with the lock held.
Okay, I'd suggest a comment in the code describing precisely what the
lock is doing would be a good idea, as it may not be obvious in the
future.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCHv9 2/2] drm: tda998x: set the connector info
On Thu, Oct 17, 2019 at 09:28:42AM +0200, Hans Verkuil wrote:
> From: Dariusz Marcinkiewicz
>
> Fill in the cec_connector_info when calling cec_notifier_conn_register().
>
> Signed-off-by: Dariusz Marcinkiewicz
> Tested-by: Hans Verkuil
> Signed-off-by: Hans Verkuil
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 33 +++
> 1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index dde8decb52a6..b0620842fa3a 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -82,6 +82,9 @@ struct tda998x_priv {
> u8 audio_port_enable[AUDIO_ROUTE_NUM];
> struct tda9950_glue cec_glue;
> struct gpio_desc *calib;
> +
> + /* protect cec_notify */
> + struct mutex cec_notify_mutex;
> struct cec_notifier *cec_notify;
> };
>
> @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void
> *data)
> tda998x_edid_delay_start(priv);
> } else {
> schedule_work(>detect_work);
> +
> + mutex_lock(>cec_notify_mutex);
> cec_notifier_phys_addr_invalidate(
> priv->cec_notify);
> + mutex_unlock(>cec_notify_mutex);
> }
>
> handled = true;
> @@ -1331,6 +1337,8 @@ static int tda998x_connector_init(struct tda998x_priv
> *priv,
> struct drm_device *drm)
> {
> struct drm_connector *connector = >connector;
> + struct cec_connector_info conn_info;
> + struct cec_notifier *notifier;
> int ret;
>
> connector->interlace_allowed = 1;
> @@ -1347,6 +1355,17 @@ static int tda998x_connector_init(struct tda998x_priv
> *priv,
> if (ret)
> return ret;
>
> + cec_fill_conn_info_from_drm(_info, connector);
> +
> + notifier = cec_notifier_conn_register(priv->cec_glue.parent,
> + NULL, _info);
> + if (!notifier)
> + return -ENOMEM;
> +
> + mutex_lock(>cec_notify_mutex);
> + priv->cec_notify = notifier;
> + mutex_unlock(>cec_notify_mutex);
You haven't taken on board what I said about the mutex in this
instance.
> +
> drm_connector_attach_encoder(>connector,
>priv->bridge.encoder);
>
> @@ -1366,6 +1385,11 @@ static void tda998x_bridge_detach(struct drm_bridge
> *bridge)
> {
> struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
>
> + mutex_lock(>cec_notify_mutex);
> + cec_notifier_conn_unregister(priv->cec_notify);
> + priv->cec_notify = NULL;
> + mutex_unlock(>cec_notify_mutex);
> +
> drm_connector_cleanup(>connector);
> }
>
> @@ -1789,8 +1813,6 @@ static void tda998x_destroy(struct device *dev)
> cancel_work_sync(>detect_work);
>
> i2c_unregister_device(priv->cec);
> -
> - cec_notifier_conn_unregister(priv->cec_notify);
> }
>
> static int tda998x_create(struct device *dev)
> @@ -1811,6 +1833,7 @@ static int tda998x_create(struct device *dev)
> mutex_init(>mutex); /* protect the page access */
> mutex_init(>audio_mutex); /* protect access from audio thread */
> mutex_init(>edid_mutex);
> + mutex_init(>cec_notify_mutex);
> INIT_LIST_HEAD(>bridge.list);
> init_waitqueue_head(>edid_delay_waitq);
> timer_setup(>edid_delay_timer, tda998x_edid_delay_done, 0);
> @@ -1915,12 +1938,6 @@ static int tda998x_create(struct device *dev)
> cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
> }
>
> - priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL);
> - if (!priv->cec_notify) {
> - ret = -ENOMEM;
> - goto fail;
> - }
> -
> priv->cec_glue.parent = dev;
> priv->cec_glue.data = priv;
> priv->cec_glue.init = tda998x_cec_hook_init;
> --
> 2.23.0
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCHv8 2/2] drm: tda998x: set the connector info
On Wed, Oct 16, 2019 at 03:39:16PM +0200, Hans Verkuil wrote:
> From: Dariusz Marcinkiewicz
>
> Fill in the cec_connector_info when calling cec_notifier_conn_register().
>
> Signed-off-by: Dariusz Marcinkiewicz
> Tested-by: Hans Verkuil
> Signed-off-by: Hans Verkuil
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 31 ++-
> 1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 8262b44b776e..b0620842fa3a 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1337,6 +1337,8 @@ static int tda998x_connector_init(struct tda998x_priv
> *priv,
> struct drm_device *drm)
> {
> struct drm_connector *connector = >connector;
> + struct cec_connector_info conn_info;
> + struct cec_notifier *notifier;
> int ret;
>
> connector->interlace_allowed = 1;
> @@ -1353,6 +1355,17 @@ static int tda998x_connector_init(struct tda998x_priv
> *priv,
> if (ret)
> return ret;
>
> + cec_fill_conn_info_from_drm(_info, connector);
> +
> + notifier = cec_notifier_conn_register(priv->cec_glue.parent,
> + NULL, _info);
> + if (!notifier)
> + return -ENOMEM;
> +
> + mutex_lock(>cec_notify_mutex);
> + priv->cec_notify = notifier;
> + mutex_unlock(>cec_notify_mutex);
As per my previous comments, this is a single-copy atomic operation.
Either priv->cec_notify is set or it isn't; there is no intermediate
value. It can't be set to a value until cec_notifier_conn_register()
has completed. So the lock doesn't help.
> +
> drm_connector_attach_encoder(>connector,
>priv->bridge.encoder);
>
> @@ -1372,6 +1385,11 @@ static void tda998x_bridge_detach(struct drm_bridge
> *bridge)
> {
> struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
>
> + mutex_lock(>cec_notify_mutex);
> + cec_notifier_conn_unregister(priv->cec_notify);
> + priv->cec_notify = NULL;
> + mutex_unlock(>cec_notify_mutex);
This is the only case where the lock makes sense - to ensure that any
of the cec_notifier_set_phys_addr*() functions aren't called
concurrently with it. However, there's no locking around the instance
in tda998x_connector_get_modes(), so have you ensured that that
function can't be called concurrently?
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv8 1/2] drm: tda998x: use cec_notifier_conn_(un)register
On Wed, Oct 16, 2019 at 03:39:15PM +0200, Hans Verkuil wrote:
> From: Dariusz Marcinkiewicz
>
> Use the new cec_notifier_conn_(un)register() functions to
> (un)register the notifier for the HDMI connector.
>
> Signed-off-by: Dariusz Marcinkiewicz
> Signed-off-by: Hans Verkuil
Please explain in detail what this mutex actually achieves.
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 21 -
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 84c6d4c91c65..8262b44b776e 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -82,6 +82,9 @@ struct tda998x_priv {
> u8 audio_port_enable[AUDIO_ROUTE_NUM];
> struct tda9950_glue cec_glue;
> struct gpio_desc *calib;
> +
> + /* protect cec_notify */
> + struct mutex cec_notify_mutex;
> struct cec_notifier *cec_notify;
> };
>
> @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void
> *data)
> tda998x_edid_delay_start(priv);
> } else {
> schedule_work(>detect_work);
> - cec_notifier_set_phys_addr(priv->cec_notify,
> -CEC_PHYS_ADDR_INVALID);
> +
> + mutex_lock(>cec_notify_mutex);
> + cec_notifier_phys_addr_invalidate(
> + priv->cec_notify);
> + mutex_unlock(>cec_notify_mutex);
> }
>
> handled = true;
> @@ -1790,8 +1796,10 @@ static void tda998x_destroy(struct device *dev)
>
> i2c_unregister_device(priv->cec);
>
> - if (priv->cec_notify)
> - cec_notifier_put(priv->cec_notify);
> + mutex_lock(>cec_notify_mutex);
> + cec_notifier_conn_unregister(priv->cec_notify);
> + priv->cec_notify = NULL;
> + mutex_unlock(>cec_notify_mutex);
By the time we get here:
1) The interrupt has been freed (which is a synchronous operation)
tda998x_irq_thread() can't be called and can't be running, and
therefore cec_notifier_phys_addr_invalidate() also can't be called
or be running.
2) You don't touch the cec_notifier_set_phys_addr_from_edid() site;
if there's any case that _might_ possibly conflict, it is that one.
3) tda998x_destroy() and tda998x_create() can't be called concurrently
in any case; the driver model guarantees that ->probe and ->remove
for the same device are serialised.
> }
>
> static int tda998x_create(struct device *dev)
> @@ -1812,6 +1820,7 @@ static int tda998x_create(struct device *dev)
> mutex_init(>mutex); /* protect the page access */
> mutex_init(>audio_mutex); /* protect access from audio thread */
> mutex_init(>edid_mutex);
> + mutex_init(>cec_notify_mutex);
> INIT_LIST_HEAD(>bridge.list);
> init_waitqueue_head(>edid_delay_waitq);
> timer_setup(>edid_delay_timer, tda998x_edid_delay_done, 0);
> @@ -1916,7 +1925,9 @@ static int tda998x_create(struct device *dev)
> cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
> }
>
> - priv->cec_notify = cec_notifier_get(dev);
> + mutex_lock(>cec_notify_mutex);
> + priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL);
> + mutex_unlock(>cec_notify_mutex);
and:
4) priv->cec_notify will be NULL here until such time that
cec_notifier_conn_register() has returned. If the mutex is trying
to protect something, it's very unclear what it is.
Trying to guess what it's protecting against:
- Is it protecting against NULL priv->cec_notify? No, because it can
be NULL just before we take the lock.
- Is it protecting the internals of cec_notifier_conn_register()
against the other calls - no, because priv->cec_notify will be NULL
until the function has finished.
- Is it protecting the write to priv->cec_notify? Maybe, but that
doesn't need any protection - architectures are single-copy atomic,
which means that a pointer is either written or it is not written.
Therefore, it will either be NULL (the state before the call is made)
or it will be set correctly (after the call has completed.)
So, all in all, I don't see what this lock is doing, and I think it
should be removed.
If it's necessary for a future change (which may or may not be merged)
then the lock should be part of that future change, because the change
proposed by this patch certainly does not need it.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH v2] drm: bridge/dw_hdmi: add audio sample channel status setting
On Thu, Sep 05, 2019 at 05:43:25PM +0800, Cheng-Yi Chiang wrote:
> From: Yakir Yang
>
> When transmitting IEC60985 linear PCM audio, we configure the
> Aduio Sample Channel Status information of all the channel
> status bits in the IEC60958 frame.
> Refer to 60958-3 page 10 for frequency, original frequency, and
> wordlength setting.
>
> This fix the issue that audio does not come out on some monitors
> (e.g. LG 22CV241)
>
> Note that these registers are only for interfaces:
> I2S audio interface, General Purpose Audio (GPA), or AHB audio DMA
> (AHBAUDDMA).
> For S/PDIF interface this information comes from the stream.
>
> Currently this function dw_hdmi_set_channel_status is only called
> from dw-hdmi-i2s-audio in I2S setup.
>
> Signed-off-by: Yakir Yang
> Signed-off-by: Cheng-Yi Chiang
> ---
> Original patch by Yakir Yang is at
>
> https://lore.kernel.org/patchwork/patch/539653/
>
> Change from v1 to v2:
> 1. Remove the version check because this will only be called by
> dw-hdmi-i2s-audio, and the registers are available in I2S setup.
> 2. Set these registers in dw_hdmi_i2s_hw_params.
> 3. Fix the sample width setting so it can use 16 or 24 bits.
>
> .../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 1 +
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 70 +++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 20 ++
> include/drm/bridge/dw_hdmi.h | 2 +
> 4 files changed, 93 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> index 34d8e837555f..b801a28b0f17 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> @@ -102,6 +102,7 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void
> *data,
> }
>
> dw_hdmi_set_sample_rate(hdmi, hparms->sample_rate);
> + dw_hdmi_set_channel_status(hdmi, hparms->sample_width);
> dw_hdmi_set_channel_count(hdmi, hparms->channels);
> dw_hdmi_set_channel_allocation(hdmi, hparms->cea.channel_allocation);
>
dw_hdmi_i2s_hw_params() is passed the channel status data in
hparams->iec.status Rather than re-creating it afresh in the driver,
I'd recommend programming the already supplied channel status data
into the registers.
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index bd65d0479683..d1daa369c8ae 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -582,6 +582,76 @@ static unsigned int hdmi_compute_n(unsigned int freq,
> unsigned long pixel_clk)
> return n;
> }
>
> +/*
> + * When transmitting IEC60958 linear PCM audio, these registers allow to
> + * configure the channel status information of all the channel status
> + * bits in the IEC60958 frame. For the moment this configuration is only
> + * used when the I2S audio interface, General Purpose Audio (GPA),
> + * or AHB audio DMA (AHBAUDDMA) interface is active
> + * (for S/PDIF interface this information comes from the stream).
> + */
> +void dw_hdmi_set_channel_status(struct dw_hdmi *hdmi,
> + unsigned int sample_width)
> +{
> + u8 aud_schnl_samplerate;
> + u8 aud_schnl_8;
> + u8 word_length_bits;
> +
> + switch (hdmi->sample_rate) {
> + case 32000:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_32K;
> + break;
> + case 44100:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_44K1;
> + break;
> + case 48000:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_48K;
> + break;
> + case 88200:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_88K2;
> + break;
> + case 96000:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_96K;
> + break;
> + case 176400:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_176K4;
> + break;
> + case 192000:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_192K;
> + break;
> + case 768000:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_768K;
> + break;
> + default:
> + dev_warn(hdmi->dev, "Unsupported audio sample rate (%u)\n",
> + hdmi->sample_rate);
> + return;
> + }
> +
> + /* set channel status register */
> + hdmi_modb(hdmi, aud_schnl_samplerate, HDMI_FC_AUDSCHNLS7_SMPRATE_MASK,
> + HDMI_FC_AUDSCHNLS7);
> +
> + /*
> + * Set original frequency to be the same as frequency.
> + * Use one-complement value as stated in IEC60958-3 page 13.
> + */
> + aud_schnl_8 = (~aud_schnl_samplerate) <<
> + HDMI_FC_AUDSCHNLS8_ORIGSAMPFREQ_OFFSET;
> +
> + /*
> + * Refer to IEC60958-3 page 12. We
Re: [PATCH] drm: bridge/dw_hdmi: add audio sample channel status setting
On Wed, Sep 04, 2019 at 05:45:20PM +0800, Cheng-yi Chiang wrote:
> On Wed, Sep 4, 2019 at 5:28 PM Neil Armstrong wrote:
> >
> > Hi,
> >
> > On 04/09/2019 11:09, Cheng-yi Chiang wrote:
> > > Hi,
> > >
> > > On Tue, Sep 3, 2019 at 5:53 PM Neil Armstrong
> > > wrote:
> > >>
> > >> Hi,
> > >>
> > >> On 03/09/2019 07:51, Cheng-Yi Chiang wrote:
> > >>> From: Yakir Yang
> > >>>
> > >>> When transmitting IEC60985 linear PCM audio, we configure the
> > >>> Audio Sample Channel Status information of all the channel
> > >>> status bits in the IEC60958 frame.
> > >>> Refer to 60958-3 page 10 for frequency, original frequency, and
> > >>> wordlength setting.
> > >>>
> > >>> This fix the issue that audio does not come out on some monitors
> > >>> (e.g. LG 22CV241)
> > >>>
> > >>> Signed-off-by: Yakir Yang
> > >>> Signed-off-by: Cheng-Yi Chiang
> > >>> ---
> > >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 59 +++
> > >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 20
> > >>> 2 files changed, 79 insertions(+)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > >>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > >>> index bd65d0479683..34d46e25d610 100644
> > >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > >>> @@ -582,6 +582,63 @@ static unsigned int hdmi_compute_n(unsigned int
> > >>> freq, unsigned long pixel_clk)
> > >>> return n;
> > >>> }
> > >>>
> > >>> +static void hdmi_set_schnl(struct dw_hdmi *hdmi)
> > >>> +{
> > >>> + u8 aud_schnl_samplerate;
> > >>> + u8 aud_schnl_8;
> > >>> +
> > >>> + /* These registers are on RK3288 using version 2.0a. */
> > >>> + if (hdmi->version != 0x200a)
> > >>> + return;
> > >>
> > >> Are these limited to the 2.0a version *in* RK3288, or 2.0a version on all
> > >> SoCs ?
> > >>
> > >
> > > In the original patch by Yakir,
> > >
> > > https://lore.kernel.org/patchwork/patch/539653/ (sorry, I should
> > > have added this link in the "after the cut" note)
> > >
> > > The fix is limited to version 2.0.
> > > Since I am only testing on RK3288 with 2.0a, I change the check to 2.0a
> > > only.
> > > I can not test 2.0a version on other SoCs.
> > > The databook I have at hand is 2.0a (not specific to RK3288) so I
> > > think all 2.0a should have this register.
> > >
> > > As for other version like version 1.3 on iMX6, there is no such
> > > register, as stated by Russell
> > >
> > > http://lkml.iu.edu/hypermail/linux/kernel/1501.3/06268.html.
> >
> > Russell assumes the registers are not present on the iMX6 IP not having
> > the I2S registers, but they are present on the IPs configured with I2S
> > at any versions.
>
> I see. Thanks for the check.
>
> >
> > My databook version (1.40.a) specifies :
> >
> > fc_audschnls0 to fc_audschnls8
> >
> > ```
> > When transmitting IEC60958 linear PCM audio, this registers allow to
> > configure the channel status
> > information of all the channel status bits in the IEC60958 frame. For the
> > moment this configuration is only
> > used when the I2S audio interface, General Purpose Audio (GPA), or AHB
> > audio DMA (AHBAUDDMA)
> > interface is active (for S/PDIF interface this information comes from the
> > stream).
> > ```
> >
> > But the databook Revision History shows these registers were present since
> > version 1.10a.
> >
> > I propose you remove the version check, but you only setup these registers
> > when I2S is enabled:
> > (hdmi_readb(hdmi, HDMI_CONFIG0_ID) & HDMI_CONFIG0_I2S) until a AHBAUDDMA
> > user needs this,
> > with a small comment explaining the situation.
>
> I see. Sound like a good plan.
> In sum, I will add
> set_channel_status()
> in dw_hdmi.c
> And in the beginning of this function,
> check it is using I2S
> (hdmi_readb(hdmi, HDMI_CONFIG0_ID) & HDMI_CONFIG0_I2S)
> with a comment explaining the situation.
>
> And let dw-hdmi-audio-i2s dw_hdmi_i2s_hw_params() to call this
> function after dw_hdmi_set_sample_rate, with word length (or
> sample_bit) passed it as argument.
If you're going to do it this way, there's little point having the
check. The dw-hdmi-audio-i2s device will only be created if that
ID bit is already set. So, provided set_channel_status() (which
should probably be dw_hdmi_set_channel_status()) is only called from
the I2S driver, then everything should be fine without such a check.
However, it is worth noting in the docbook comments above the function
that it is only for I2S setups.
> I have not tested setting this register here as in the original patch
> it was set in hdmi_set_clk_regenerator.
> I will test it and update in my v2.
>
> Thanks again to everyone for the prompt reply and help.
>
> >
> > Neil
> >
> > >
> > > So at least we should check the version.
> > > Maybe we can set the criteria as version 2.0 or above to make it a safe
> > > patch ?
> > > If there is the same need on other SoC with version < 2.0, it can be
> > >
Re: [PATCH] drm: bridge/dw_hdmi: add audio sample channel status setting
On Wed, Sep 04, 2019 at 05:09:29PM +0800, Cheng-yi Chiang wrote:
> Hi,
>
> On Tue, Sep 3, 2019 at 5:53 PM Neil Armstrong wrote:
> >
> > Hi,
> >
> > On 03/09/2019 07:51, Cheng-Yi Chiang wrote:
> > > From: Yakir Yang
> > >
> > > When transmitting IEC60985 linear PCM audio, we configure the
> > > Audio Sample Channel Status information of all the channel
> > > status bits in the IEC60958 frame.
> > > Refer to 60958-3 page 10 for frequency, original frequency, and
> > > wordlength setting.
> > >
> > > This fix the issue that audio does not come out on some monitors
> > > (e.g. LG 22CV241)
> > >
> > > Signed-off-by: Yakir Yang
> > > Signed-off-by: Cheng-Yi Chiang
> > > ---
> > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 59 +++
> > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 20
> > > 2 files changed, 79 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > > index bd65d0479683..34d46e25d610 100644
> > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > > @@ -582,6 +582,63 @@ static unsigned int hdmi_compute_n(unsigned int
> > > freq, unsigned long pixel_clk)
> > > return n;
> > > }
> > >
> > > +static void hdmi_set_schnl(struct dw_hdmi *hdmi)
> > > +{
> > > + u8 aud_schnl_samplerate;
> > > + u8 aud_schnl_8;
> > > +
> > > + /* These registers are on RK3288 using version 2.0a. */
> > > + if (hdmi->version != 0x200a)
> > > + return;
> >
> > Are these limited to the 2.0a version *in* RK3288, or 2.0a version on all
> > SoCs ?
> >
>
> In the original patch by Yakir,
>
> https://lore.kernel.org/patchwork/patch/539653/ (sorry, I should
> have added this link in the "after the cut" note)
>
> The fix is limited to version 2.0.
> Since I am only testing on RK3288 with 2.0a, I change the check to 2.0a only.
> I can not test 2.0a version on other SoCs.
> The databook I have at hand is 2.0a (not specific to RK3288) so I
> think all 2.0a should have this register.
>
> As for other version like version 1.3 on iMX6, there is no such
> register, as stated by Russell
>
> http://lkml.iu.edu/hypermail/linux/kernel/1501.3/06268.html.
It's likely more to do with how the IP is configured rather than the
version. The big difference between dw-hdmi used in iMX6 and elsewhere
is that iMX6 uses a built-in AHB audio interface and not I2S. Elsewhere
uses I2S.
I2S does not have the capability to convey channel status information
(which is required by HDMI). With AHB, it is encoded into the data in
memory.
So, I think this setup should be done in the I2S driver and not in the
core driver.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 3/8] tda9950: use cec_notifier_cec_adap_(un)register
On Tue, Aug 13, 2019 at 01:02:35PM +0200, Dariusz Marcinkiewicz wrote:
> Use the new cec_notifier_cec_adap_(un)register() functions to
> (un)register the notifier for the CEC adapter.
>
> Signed-off-by: Dariusz Marcinkiewicz
> Signed-off-by: Hans Verkuil
> Tested-by: Hans Verkuil
> ---
> drivers/gpu/drm/i2c/tda9950.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda9950.c b/drivers/gpu/drm/i2c/tda9950.c
> index 8039fc0d83db4..a5a75bdeb7a5f 100644
> --- a/drivers/gpu/drm/i2c/tda9950.c
> +++ b/drivers/gpu/drm/i2c/tda9950.c
> @@ -420,7 +420,8 @@ static int tda9950_probe(struct i2c_client *client,
> priv->hdmi = glue->parent;
>
> priv->adap = cec_allocate_adapter(_cec_ops, priv, "tda9950",
> - CEC_CAP_DEFAULTS,
> + CEC_CAP_DEFAULTS |
> + CEC_CAP_CONNECTOR_INFO,
> CEC_MAX_LOG_ADDRS);
> if (IS_ERR(priv->adap))
> return PTR_ERR(priv->adap);
> @@ -457,13 +458,14 @@ static int tda9950_probe(struct i2c_client *client,
> if (ret < 0)
> return ret;
>
> - priv->notify = cec_notifier_get(priv->hdmi);
> + priv->notify = cec_notifier_cec_adap_register(priv->hdmi, NULL,
> + priv->adap);
> if (!priv->notify)
> return -ENOMEM;
>
> ret = cec_register_adapter(priv->adap, priv->hdmi);
> if (ret < 0) {
> - cec_notifier_put(priv->notify);
> + cec_notifier_cec_adap_unregister(priv->notify);
> return ret;
> }
>
> @@ -473,8 +475,6 @@ static int tda9950_probe(struct i2c_client *client,
>*/
> devm_remove_action(dev, tda9950_cec_del, priv);
>
> - cec_register_cec_notifier(priv->adap, priv->notify);
> -
> return 0;
> }
>
> @@ -482,8 +482,8 @@ static int tda9950_remove(struct i2c_client *client)
> {
> struct tda9950_priv *priv = i2c_get_clientdata(client);
>
> + cec_notifier_cec_adap_unregister(priv->notify);
> cec_unregister_adapter(priv->adap);
> - cec_notifier_put(priv->notify);
It looks weird to have an unexpectedly different ordering of
unregistration from the registration path - normally, unregistration
is the reverse order of initialisation.
In the initialisation path, it seems that we register the notifier
and _then_ the adapter. Here, we unregister the notifier and then
the adapter rather than what would normally be expected. Why is
this? I suspect there will be drivers created that do this the
"normal" way round, so if this is a requirement, it needs to be made
plainly obvious.
>
> return 0;
> }
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH v6 4/8] drm: tda998x: use cec_notifier_conn_(un)register
On Tue, Aug 13, 2019 at 01:02:36PM +0200, Dariusz Marcinkiewicz wrote:
> Use the new cec_notifier_conn_(un)register() functions to
> (un)register the notifier for the HDMI connector, and fill
> in the cec_connector_info.
>
> Changes since v2:
> - cec_notifier_phys_addr_invalidate where appropriate,
> - don't check for NULL notifier before calling
> cec_notifier_conn_unregister.
> Changes since v1:
> Add memory barrier to make sure that the notifier
> becomes visible to the irq thread once it is
> fully constructed.
>
> Signed-off-by: Dariusz Marcinkiewicz
> Tested-by: Hans Verkuil
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 33 +--
> 1 file changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 61e042918a7fc..19a63ee1b3f53 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -804,9 +804,14 @@ static irqreturn_t tda998x_irq_thread(int irq, void
> *data)
> if (lvl & CEC_RXSHPDLEV_HPD) {
> tda998x_edid_delay_start(priv);
> } else {
> + struct cec_notifier *notify;
> +
> schedule_work(>detect_work);
> - cec_notifier_set_phys_addr(priv->cec_notify,
> -CEC_PHYS_ADDR_INVALID);
> +
> + notify = READ_ONCE(priv->cec_notify);
> + if (notify)
> + cec_notifier_phys_addr_invalidate(
> + notify);
> }
>
> handled = true;
> @@ -1331,6 +1336,8 @@ static int tda998x_connector_init(struct tda998x_priv
> *priv,
> struct drm_device *drm)
> {
> struct drm_connector *connector = >connector;
> + struct cec_connector_info conn_info;
> + struct cec_notifier *notifier;
> int ret;
>
> connector->interlace_allowed = 1;
> @@ -1347,6 +1354,19 @@ static int tda998x_connector_init(struct tda998x_priv
> *priv,
> if (ret)
> return ret;
>
> + cec_fill_conn_info_from_drm(_info, connector);
> +
> + notifier = cec_notifier_conn_register(priv->cec_glue.parent,
> + NULL, _info);
> + if (!notifier)
> + return -ENOMEM;
> + /*
> + * Make sure that tda998x_irq_thread does see the notifier
> + * when it fully constructed.
> + */
> + smp_wmb();
> + priv->cec_notify = notifier;
To nitpick, this comment and the following code do not go together.
I think what you actually mean is:
* Make sure that tda998x_irq_thread sees the notifier
* only after it is fully constructed.
> +
> drm_connector_attach_encoder(>connector,
>priv->bridge.encoder);
>
> @@ -1790,8 +1810,7 @@ static void tda998x_destroy(struct device *dev)
>
> i2c_unregister_device(priv->cec);
>
> - if (priv->cec_notify)
> - cec_notifier_put(priv->cec_notify);
> + cec_notifier_conn_unregister(priv->cec_notify);
This also doesn't make sense: tda998x_destroy() is the opposite of
tda998x_create(). However, tda998x_connector_destroy() is the
opposite of tda998x_connector_create().
By moving the CEC creation code into tda998x_connector_create(), you
are creating the possibility for the following sequence to mess up
CEC and leak:
tda998x_create()
tda998x_connector_create()
tda998x_connector_destroy()
tda998x_connector_create()
tda998x_connector_destroy()
tda998x_destroy()
Anything you create in tda998x_connector_create() must be cleaned up
by tda998x_connector_destroy().
> }
>
> static int tda998x_create(struct device *dev)
> @@ -1916,12 +1935,6 @@ static int tda998x_create(struct device *dev)
> cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
> }
>
> - priv->cec_notify = cec_notifier_get(dev);
> - if (!priv->cec_notify) {
> - ret = -ENOMEM;
> - goto fail;
> - }
> -
> priv->cec_glue.parent = dev;
> priv->cec_glue.data = priv;
> priv->cec_glue.init = tda998x_cec_hook_init;
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH v3 1/5] ASoC: hdmi-codec: Add an op to set callback function for plug event
On Fri, Jul 12, 2019 at 06:04:39PM +0800, Cheng-Yi Chiang wrote:
> Add an op in hdmi_codec_ops so codec driver can register callback
> function to handle plug event.
>
> Driver in DRM can use this callback function to report connector status.
>
> Signed-off-by: Cheng-Yi Chiang
> ---
> include/sound/hdmi-codec.h| 16 +
> sound/soc/codecs/hdmi-codec.c | 45 +++
> 2 files changed, 61 insertions(+)
>
> diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
> index 7fea496f1f34..9a8661680256 100644
> --- a/include/sound/hdmi-codec.h
> +++ b/include/sound/hdmi-codec.h
> @@ -47,6 +47,9 @@ struct hdmi_codec_params {
> int channels;
> };
>
> +typedef void (*hdmi_codec_plugged_cb)(struct device *dev,
> + bool plugged);
> +
I'd like to pose a question for people to think about.
Firstly, typedefs are generally shunned in the kernel. However, for
these cases it seems to make sense.
However, should the "pointer"-ness be part of the typedef or not? To
see what I mean, consider:
typedef void (*hdmi_foo)(void);
int register_foo(hdmi_foo foo);
vs
typedef void hdmi_foo(void);
int register_foo(hdmi_foo *foo);
which is more in keeping with how we code non-typedef'd code - it's
obvious that foo is a pointer while reading the code.
It seems to me that the latter better matches what is in the kernel's
coding style, which states:
In general, a pointer, or a struct that has elements that can
reasonably be directly accessed should **never** be a typedef.
or maybe Documentation/process/coding-style.rst needs updating?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH v3 2/5] drm: bridge: dw-hdmi: Report connector status using callback
On Fri, Jul 12, 2019 at 06:04:40PM +0800, Cheng-Yi Chiang wrote:
> Allow codec driver register callback function for plug event.
>
> The callback registration flow:
> dw-hdmi <--- hw-hdmi-i2s-audio <--- hdmi-codec
>
> dw-hdmi-i2s-audio implements hook_plugged_cb op
> so codec driver can register the callback.
>
> dw-hdmi implements set_plugged_cb op so platform device can register the
> callback.
>
> When connector plug/unplug event happens, report this event using the
> callback.
>
> Make sure that audio and drm are using the single source of truth for
> connector status.
>
> Signed-off-by: Cheng-Yi Chiang
> ---
> .../gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 3 +
> .../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 10
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 55 ++-
> 3 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
> index 63b5756f463b..f523c590984e 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
> @@ -2,6 +2,8 @@
> #ifndef DW_HDMI_AUDIO_H
> #define DW_HDMI_AUDIO_H
>
> +#include
> +
> struct dw_hdmi;
>
> struct dw_hdmi_audio_data {
> @@ -17,6 +19,7 @@ struct dw_hdmi_i2s_audio_data {
>
> void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
> u8 (*read)(struct dw_hdmi *hdmi, int offset);
> + int (*set_plugged_cb)(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb fn);
> };
>
> #endif
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> index 5cbb71a866d5..7b93cf05c985 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> @@ -104,10 +104,20 @@ static int dw_hdmi_i2s_get_dai_id(struct
> snd_soc_component *component,
> return -EINVAL;
> }
>
> +static int dw_hdmi_i2s_hook_plugged_cb(struct device *dev, void *data,
> +hdmi_codec_plugged_cb fn)
> +{
> + struct dw_hdmi_i2s_audio_data *audio = data;
> + struct dw_hdmi *hdmi = audio->hdmi;
> +
> + return audio->set_plugged_cb(hdmi, fn);
> +}
> +
> static struct hdmi_codec_ops dw_hdmi_i2s_ops = {
> .hw_params = dw_hdmi_i2s_hw_params,
> .audio_shutdown = dw_hdmi_i2s_audio_shutdown,
> .get_dai_id = dw_hdmi_i2s_get_dai_id,
> + .hook_plugged_cb = dw_hdmi_i2s_hook_plugged_cb,
> };
>
> static int snd_dw_hdmi_probe(struct platform_device *pdev)
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 045b1b13fd0e..ce6646067472 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -26,6 +26,8 @@
> #include
> #include
>
> +#include
> +
> #include
> #include
>
> @@ -185,6 +187,9 @@ struct dw_hdmi {
> void (*disable_audio)(struct dw_hdmi *hdmi);
>
> struct cec_notifier *cec_notifier;
> +
> + hdmi_codec_plugged_cb plugged_cb;
> + enum drm_connector_status last_connector_result;
> };
>
> #define HDMI_IH_PHY_STAT0_RX_SENSE \
> @@ -209,6 +214,40 @@ static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int
> offset)
> return val;
> }
>
> +static void handle_plugged_change(struct dw_hdmi *hdmi, bool plugged)
> +{
> + struct platform_device *codec_pdev;
> +
> + if (!hdmi->audio || IS_ERR(hdmi->audio))
> + return;
> + codec_pdev = platform_get_drvdata(hdmi->audio);
> + if (!codec_pdev || IS_ERR(codec_pdev))
> + return;
This looks fragile to me, poking about in another device's driver data
from another driver is really not a good design decision. I think this
can be simplified if the registration function took the function
pointer and the struct device pointer, and then you only need one test
below:
> + if (!hdmi->plugged_cb)
> + return;
> +
> + hdmi->plugged_cb(_pdev->dev, plugged);
> +}
> +
> +static int hdmi_set_plugged_cb(struct dw_hdmi *hdmi, hdmi_codec_plugged_cb
> fn)
> +{
> + bool plugged;
> + struct platform_device *codec_pdev;
> +
> + if (!hdmi->audio || IS_ERR(hdmi->audio))
> + return -EINVAL;
Given the current code structure, how can this ever be true when the
function is called?
> + codec_pdev = platform_get_drvdata(hdmi->audio);
> + if (!codec_pdev || IS_ERR(codec_pdev))
> + return -EINVAL;
This doesn't seem like a good idea as I've pointed out above.
> +
> + mutex_lock(>mutex);
> + hdmi->plugged_cb = fn;
> + plugged = hdmi->last_connector_result == connector_status_connected;
> + handle_plugged_change(hdmi, plugged);
> + mutex_unlock(>mutex);
Should be a blank line here for readability.
> + return 0;
> +}
> +
> static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg)
>
Re: [PATCH 00/12] treewide: Fix GENMASK misuses
On Wed, Jul 10, 2019 at 11:17:31AM +0200, Johannes Berg wrote: > On Tue, 2019-07-09 at 22:04 -0700, Joe Perches wrote: > > These GENMASK uses are inverted argument order and the > > actual masks produced are incorrect. Fix them. > > > > Add checkpatch tests to help avoid more misuses too. > > > > Joe Perches (12): > > checkpatch: Add GENMASK tests > > IMHO this doesn't make a lot of sense as a checkpatch test - just throw > in a BUILD_BUG_ON()? My personal take on this is that GENMASK() is really not useful, it's just pure obfuscation and leads to exactly these kinds of mistakes. Yes, I fully understand the argument that you can just specify the start and end bits, and it _in theory_ makes the code more readable. However, the problem is when writing code. GENMASK(a, b). Is a the starting bit or ending bit? Is b the number of bits? It's confusing and causes mistakes resulting in incorrect code. A BUILD_BUG_ON() can catch some of the cases, but not all of them. For example: GENMASK(6, 2) would satisify the requirement that a > b, so a BUILD_BUG_ON() will not trigger, but was the author meaning 0x3c or 0xc0? Personally, I've decided I am _not_ going to use GENMASK() in my code because I struggle to get the macro arguments correct - I'm _much_ happier, and it is way more reliable for me to write the mask in hex notation. I think this is where use of a ternary operator would come in use. The normal way of writing a number of bits tends to be "a:b", so if GENMASK took something like GENMASK(6:2), then I'd have less issue with it, because it's argument is then in a familiar notation. Yes, I'm sure that someone will point out that the GENMASK arguments are just in the same order, but that doesn't prevent _me_ frequently getting it wrong - and that's the point. The macro seems to me to cause more problems than it solves. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [GIT PULL] ARM (for-airlie-armada branch)
The subject should've been "Armada updates for merge window". Sorry. On Tue, Jul 02, 2019 at 10:13:13AM +0100, Russell King wrote: > Hi David, > > The following changes since commit e93c9c99a629c61837d5a7fc2120cd2b6c70dbdd: > > Linux 5.1 (2019-05-05 17:42:58 -0700) > > are available in the git repository at: > > git://git.armlinux.org.uk/~rmk/linux-arm.git for-airlie-armada > > for you to fetch changes up to 837567c1e9d587c0b438263c9cfd32de46640e16: > > drm/armada: no need to check parent of remote (2019-06-28 14:50:07 +0100) > > > Armada DRM updates: > - Fix interlace support. > - use __drm_atomic_helper_plane_reset in overlay reset. > - since the overlay and video planes use essentially the same format > registers, precompute their values while validating. > - fix a long-standing deficiency with overlay planes and interlace modes > - calculate plane starting address at atomic_check stage rather than > when we're programming the registers. > - add gamma support. > - ensure mode adjustments made by other components are properly handled > in the driver and applied to the CRTC-programmed mode. > - add and use register definitions for the "REG4F" register. > - use drm_atomic_helper_shutdown() when tearing down to ensure that the > hardware is properly shutdown. > - add CRTC-level mode validation to ensure that we don't allow a mode > that the CRTC-level hardware can not support. > - improve the clocking selection for Armada 510 support. > - move CRTC debugfs files into the crtc-specific directory, using the > DRM helper to create these files. > - patch from Lubomir Rintel to replace a simple framebuffer. > - use the OF graph walker rather than open-coding this. > - eliminate a useless check for the availability of the remote's parent > which isn't required. > > > Lubomir Rintel (1): > drm/armada: replace the simple-framebuffer > > Russell King (17): > drm/armada: fix crtc interlace > drm/armada: use __drm_atomic_helper_plane_reset in overlay reset > drm/armada: add plane size/location accessors > drm/armada: fix plane location and size for interlace > drm/armada: add missing interlaced support for overlay frame > drm/armada: move plane address and pitch calculation to atomic_check > drm/armada: add support for setting gamma > drm/armada: add comments about HWC32 cursor colour format > drm/armada: add drm_mode_set_crtcinfo() mode fixup > drm/armada: add and use definitions for RDREG4F > drm/armada: add drm_atomic_helper_shutdown() call in tear-down > drm/armada: add CRTC mode validation > drm/armada: improve Dove clock selection > drm/armada: use mode_valid to validate the adjusted mode > drm/armada: redo CRTC debugfs files > drm/armada: use for_each_endpoint_of_node() to walk crtc endpoints > drm/armada: no need to check parent of remote > > drivers/gpu/drm/armada/armada_510.c | 130 +-- > drivers/gpu/drm/armada/armada_crtc.c| 214 > ++-- > drivers/gpu/drm/armada/armada_crtc.h| 21 +++- > drivers/gpu/drm/armada/armada_debugfs.c | 98 ++- > drivers/gpu/drm/armada/armada_drm.h | 1 + > drivers/gpu/drm/armada/armada_drv.c | 38 +++--- > drivers/gpu/drm/armada/armada_hw.h | 29 +++-- > drivers/gpu/drm/armada/armada_overlay.c | 56 - > drivers/gpu/drm/armada/armada_plane.c | 124 -- > drivers/gpu/drm/armada/armada_plane.h | 23 > 10 files changed, 522 insertions(+), 212 deletions(-) > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/armada: fix debugfs link error
On Fri, Jun 28, 2019 at 12:33:40PM +0200, Arnd Bergmann wrote:
> Debugfs can be disabled at compile time, causing a link error
> with the newly restructured code:
>
> drivers/gpu/drm/armada/armada_crtc.o: In function
> `armada_drm_crtc_late_register':
> armada_crtc.c:(.text+0x974): undefined reference to
> `armada_drm_crtc_debugfs_init'
>
> Make the code into the debugfs init function conditional.
Thanks Arnd, mind if I roll this into the original commit?
> Fixes: 05ec8bd524ba ("drm/armada: redo CRTC debugfs files")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/gpu/drm/armada/armada_crtc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/armada/armada_crtc.c
> b/drivers/gpu/drm/armada/armada_crtc.c
> index e3a5964d8a65..03d3fd00fe00 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -773,7 +773,9 @@ static void armada_drm_crtc_destroy(struct drm_crtc *crtc)
>
> static int armada_drm_crtc_late_register(struct drm_crtc *crtc)
> {
> - armada_drm_crtc_debugfs_init(drm_to_armada_crtc(crtc));
> + if (IS_ENABLED(CONFIG_DEBUG_FS))
> + armada_drm_crtc_debugfs_init(drm_to_armada_crtc(crtc));
> +
> return 0;
> }
>
> --
> 2.20.0
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [REGRESSION] drm/etnaviv: command buffer outside valid memory window
On Thu, Jun 27, 2019 at 04:49:30PM +0200, Lucas Stach wrote: > Am Donnerstag, den 27.06.2019, 15:32 +0100 schrieb Russell King - ARM Linux > admin: > > On Thu, Jun 27, 2019 at 11:04:17AM +0100, Russell King - ARM Linux admin > > wrote: > > > On Thu, Jun 27, 2019 at 11:20:15AM +0200, Lucas Stach wrote: > > > > Am Samstag, den 22.06.2019, 17:16 +0100 schrieb Russell King - ARM > > > > Linux admin: > > > > > While updating my various systems for the TCP SACK issue, I notice > > > > > that while most platforms are happy, the Cubox-i4 is not. During > > > > > boot, we get: > > > > > > > > > > [0.00] cma: Reserved 256 MiB at 0x3000 > > > > > ... > > > > > [0.00] Kernel command line: console=ttymxc0,115200n8 > > > > > console=tty1 video=mxcfb0:dev=hdmi root=/dev/nfs rw cma=256M > > > > > ahci_imx.hotplug=1 splash resume=/dev/sda1 > > > > > [0.00] Dentry cache hash table entries: 131072 (order: 7, > > > > > 524288 bytes) > > > > > [0.00] Inode-cache hash table entries: 65536 (order: 6, > > > > > 262144 bytes) > > > > > [0.00] Memory: 1790972K/2097152K available (8471K kernel > > > > > code, 693K rwdata, 2844K rodata, 500K init, 8062K bss, 44036K > > > > > reserved, 262144K cma-reserved, 1310720K highmem) > > > > > ... > > > > > [ 13.101098] etnaviv-gpu 13.gpu: command buffer outside valid > > > > > memory window > > > > > [ 13.171963] etnaviv-gpu 134000.gpu: command buffer outside valid > > > > > memory window > > > > > > > > Yes, that's a regression due to different default CMA area placement > > > > and etnaviv not being smart enough to move the linear window to the > > > > right offset. > > > > > > As it's a user visible regression, it needs fixing, either by reverting > > > the changes that caused it or by some other issue. In the kernel, the > > > policy is "if a bug fix causes a regression, the bug fix was itself > > > wrong". We don't fix one person's bug if it causes a regression for > > > someone else. > > > > > > Please resolve the acknowledged regression. > > The regression is caused due to a different CMA placement, which is > outside of the control of etnaviv. If you can point to the commit > causing this change in placement we could work with the > authors/maintainers of this code to get rid of the regression. > Currently I don't have the bandwidth to pinpoint the offending code > change. Ok, thanks for the explanation. Well, the problem has become weirder. I'm unable to reproduce the hang now - the only change has been to add your patch for the unload issue, as well as temporarily disabling lightdm's startup at boot (which is now back as it was.) Odd. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/etnaviv: add missing failure path to destroy suballoc
On Thu, Jun 27, 2019 at 04:44:38PM +0200, Lucas Stach wrote:
> When something goes wrong in the GPU init after the cmdbuf suballocator
> has been constructed, we fail to destory it properly. This causes havok
> later when the GPU is unbound due to a module unload or similar.
>
> Signed-off-by: Lucas Stach
Tested-by: Russell King
Thanks.
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 72d01e873160..5418a1a87b2c 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -760,7 +760,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
> if (IS_ERR(gpu->cmdbuf_suballoc)) {
> dev_err(gpu->dev, "Failed to create cmdbuf suballocator\n");
> ret = PTR_ERR(gpu->cmdbuf_suballoc);
> - goto fail;
> + goto destroy_iommu;
> }
>
> /* Create buffer: */
> @@ -768,7 +768,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
> PAGE_SIZE);
> if (ret) {
> dev_err(gpu->dev, "could not create command buffer\n");
> - goto destroy_iommu;
> + goto destroy_suballoc;
> }
>
> if (gpu->mmu->version == ETNAVIV_IOMMU_V1 &&
> @@ -800,6 +800,9 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
> free_buffer:
> etnaviv_cmdbuf_free(>buffer);
> gpu->buffer.suballoc = NULL;
> +destroy_suballoc:
> + etnaviv_cmdbuf_suballoc_destroy(gpu->cmdbuf_suballoc);
> + gpu->cmdbuf_suballoc = NULL;
> destroy_iommu:
> etnaviv_iommu_destroy(gpu->mmu);
> gpu->mmu = NULL;
> --
> 2.20.1
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [REGRESSION] drm/etnaviv: command buffer outside valid memory window
On Thu, Jun 27, 2019 at 11:04:17AM +0100, Russell King - ARM Linux admin wrote: > On Thu, Jun 27, 2019 at 11:20:15AM +0200, Lucas Stach wrote: > > Am Samstag, den 22.06.2019, 17:16 +0100 schrieb Russell King - ARM Linux > > admin: > > > While updating my various systems for the TCP SACK issue, I notice > > > that while most platforms are happy, the Cubox-i4 is not. During > > > boot, we get: > > > > > > [0.00] cma: Reserved 256 MiB at 0x3000 > > > ... > > > [0.00] Kernel command line: console=ttymxc0,115200n8 console=tty1 > > > video=mxcfb0:dev=hdmi root=/dev/nfs rw cma=256M ahci_imx.hotplug=1 splash > > > resume=/dev/sda1 > > > [0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 > > > bytes) > > > [0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 > > > bytes) > > > [0.00] Memory: 1790972K/2097152K available (8471K kernel code, > > > 693K rwdata, 2844K rodata, 500K init, 8062K bss, 44036K reserved, 262144K > > > cma-reserved, 1310720K highmem) > > > ... > > > [ 13.101098] etnaviv-gpu 13.gpu: command buffer outside valid > > > memory window > > > [ 13.171963] etnaviv-gpu 134000.gpu: command buffer outside valid > > > memory window > > > > Yes, that's a regression due to different default CMA area placement > > and etnaviv not being smart enough to move the linear window to the > > right offset. > > As it's a user visible regression, it needs fixing, either by reverting > the changes that caused it or by some other issue. In the kernel, the > policy is "if a bug fix causes a regression, the bug fix was itself > wrong". We don't fix one person's bug if it causes a regression for > someone else. > > Please resolve the acknowledged regression. > > > > and shortly after the login prompt appears, the entire SoC appears to > > > lock up - it becomes unresponsive on the network, or via serial console > > > to sysrq requests. > > > > > > I suspect the GPU ends up scribbling over the CPU's vector page/kernel > > > as a result of the above two etnaviv errors when Xorg attempts to start > > > using the GPU. > > > > This should not be possible. The driver notices that the command buffer > > isn't accessible to the GPU, which aborts the GPU init. While the > > etnaviv DRM device is still accessible, it will not expose any > > enumerable GPU cores to userspace. So there is no way for userspace to > > actually submit GPU commands. > > Yep, I came to that conclusion. Nevertheless, if I allow Xorg to start > with 5.1, the system totally hangs shortly thereafter. I need to try > without etnaviv loaded at all. Well, it seems to get worse. I just tried to unload etnaviv, and was greeted by this oops. It's another regression; etnaviv used to unload perfectly fine. Please can you add module unload testing to your workflow? Unable to handle kernel NULL pointer dereference at virtual address 0008 pgd = da59c000 [0008] *pgd=8fc0f831 Internal error: Oops: 17 [#1] SMP ARM Modules linked in: ip6table_filter ip6_tables ipt_REJECT nf_reject_ipv4 xt_tcpudp xt_owner xt_multiport iptable_filter ip_tables x_tables bnep rfcomm bluetooth ecdh_generic nfsd rc_cec snd_soc_fsl_spdif nvmem_imx_ocotp imx_pcm_dma imx_sdma virt_dma coda v4l2_mem2mem imx_vdoa dw_hdmi_ahb_audio dw_hdmi_cec videobuf2_dma_contig etnaviv(-) gpu_sched imx_thermal snd_soc_imx_spdif imx6q_cpufreq caamrng caam_jr caam error CPU: 1 PID: 2898 Comm: rmmod Not tainted 5.1.0+ #319 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) PC is at etnaviv_iommu_put_suballoc_va+0x10/0x68 [etnaviv] LR is at etnaviv_cmdbuf_suballoc_destroy+0x20/0x48 [etnaviv] pc : []lr : []psr: a00f0013 sp : d9f2be40 ip : 01b0 fp : r10: 0081 r9 : d9f2a000 r8 : c00091c4 r7 : dc993800 r6 : r5 : dd4c6810 r4 : r3 : b00c r2 : 0004 r1 : dd4c6810 r0 : dc991840 Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 2a59c04a DAC: 0051 Process rmmod (pid: 2898, stack limit = 0xd9f2a218) Stack: (0xd9f2be40 to 0xd9f2c000) be40: dd4c6800 dd5e9b40 bf04a664 dd5e9b40 be60: dc991840 bf04e4d0 bf04e458 dd5e93c0 dd5e9b40 c04aa2e0 0018 dc993800 be80: c00091c4 dd5e9b40 0001 c04aa3b4 dc993800 dd0f9410 dd5a4000 bea0: bf04a97c dd5e9b40 dd0f9410 bf05295c c04aa9bc dd5e9b40 c04aaf6c bec0: dd0f9410 bf055260 bf04a950 bf04a93c c04b1f00 c04b1edc dd0f9410 bee0: c04b0798 c0c493a8 de8af44c dd0f9410 c0c493a8 c0c49408 c04af450 bf00: dd0f9444 dd0f9410 000120a8 c04ac02c c0bf
Re: [REGRESSION] drm/etnaviv: command buffer outside valid memory window
On Thu, Jun 27, 2019 at 11:20:15AM +0200, Lucas Stach wrote: > Am Samstag, den 22.06.2019, 17:16 +0100 schrieb Russell King - ARM Linux > admin: > > While updating my various systems for the TCP SACK issue, I notice > > that while most platforms are happy, the Cubox-i4 is not. During > > boot, we get: > > > > [0.00] cma: Reserved 256 MiB at 0x3000 > > ... > > [0.00] Kernel command line: console=ttymxc0,115200n8 console=tty1 > > video=mxcfb0:dev=hdmi root=/dev/nfs rw cma=256M ahci_imx.hotplug=1 splash > > resume=/dev/sda1 > > [0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 > > bytes) > > [0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 > > bytes) > > [0.00] Memory: 1790972K/2097152K available (8471K kernel code, 693K > > rwdata, 2844K rodata, 500K init, 8062K bss, 44036K reserved, 262144K > > cma-reserved, 1310720K highmem) > > ... > > [ 13.101098] etnaviv-gpu 13.gpu: command buffer outside valid memory > > window > > [ 13.171963] etnaviv-gpu 134000.gpu: command buffer outside valid memory > > window > > Yes, that's a regression due to different default CMA area placement > and etnaviv not being smart enough to move the linear window to the > right offset. As it's a user visible regression, it needs fixing, either by reverting the changes that caused it or by some other issue. In the kernel, the policy is "if a bug fix causes a regression, the bug fix was itself wrong". We don't fix one person's bug if it causes a regression for someone else. Please resolve the acknowledged regression. > > and shortly after the login prompt appears, the entire SoC appears to > > lock up - it becomes unresponsive on the network, or via serial console > > to sysrq requests. > > > > I suspect the GPU ends up scribbling over the CPU's vector page/kernel > > as a result of the above two etnaviv errors when Xorg attempts to start > > using the GPU. > > This should not be possible. The driver notices that the command buffer > isn't accessible to the GPU, which aborts the GPU init. While the > etnaviv DRM device is still accessible, it will not expose any > enumerable GPU cores to userspace. So there is no way for userspace to > actually submit GPU commands. Yep, I came to that conclusion. Nevertheless, if I allow Xorg to start with 5.1, the system totally hangs shortly thereafter. I need to try without etnaviv loaded at all. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/2] Associate ddc adapters with connectors
On Tue, Jun 25, 2019 at 04:07:55PM +0200, Daniel Vetter wrote: > Otherwise I like this. Biggest problem I'm seeing here is rolling this out > everywhere, this is a lot of work. And without widespread adoptions it's > not terribly useful for userspace. There will be cases where it's not possible, because the I2C bus is hidden behind a chip that doesn't give you direct access to the DDC bus. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/2] Associate ddc adapters with connectors
On Tue, Jun 25, 2019 at 12:14:27PM +0200, Andrzej Pietrasiewicz wrote: > Hi Russell, > > W dniu 25.06.2019 o 12:03, Russell King - ARM Linux admin pisze: > > On Tue, Jun 25, 2019 at 11:46:34AM +0200, Andrzej Pietrasiewicz wrote: > > > It is difficult for a user to know which of the i2c adapters is for which > > > drm connector. This series addresses this problem. > > > > > > The idea is to have a symbolic link in connector's sysfs directory, e.g.: > > > > > > ls -l /sys/class/drm/card0-HDMI-A-1/i2c-2 > > > lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/i2c-2 > > > \ > > > -> ../../../../soc/1388.i2c/i2c-2 > > > > Don't you want the symlink name to be "i2c" or something fixed, rather > > than the name of the i2c adapter? Otherwise, you seem to be encumbering > > userspace with searching the directory to try and find the symlink. > > > > Thank you for your comment. So you imagine something on the lines of: > > lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \ > -> ../../../../soc/1388.i2c/i2c-2 > > ? Exactly. > This makes sense to me, I will send a v2. Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH 0/2] Associate ddc adapters with connectors
On Tue, Jun 25, 2019 at 11:46:34AM +0200, Andrzej Pietrasiewicz wrote: > It is difficult for a user to know which of the i2c adapters is for which > drm connector. This series addresses this problem. > > The idea is to have a symbolic link in connector's sysfs directory, e.g.: > > ls -l /sys/class/drm/card0-HDMI-A-1/i2c-2 > lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/i2c-2 \ > -> ../../../../soc/1388.i2c/i2c-2 Don't you want the symlink name to be "i2c" or something fixed, rather than the name of the i2c adapter? Otherwise, you seem to be encumbering userspace with searching the directory to try and find the symlink. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
[REGRESSION] drm/etnaviv: command buffer outside valid memory window
While updating my various systems for the TCP SACK issue, I notice that while most platforms are happy, the Cubox-i4 is not. During boot, we get: [0.00] cma: Reserved 256 MiB at 0x3000 ... [0.00] Kernel command line: console=ttymxc0,115200n8 console=tty1 video=mxcfb0:dev=hdmi root=/dev/nfs rw cma=256M ahci_imx.hotplug=1 splash resume=/dev/sda1 [0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes) [0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes) [0.00] Memory: 1790972K/2097152K available (8471K kernel code, 693K rwdata, 2844K rodata, 500K init, 8062K bss, 44036K reserved, 262144K cma-reserved, 1310720K highmem) ... [ 13.101098] etnaviv-gpu 13.gpu: command buffer outside valid memory window [ 13.171963] etnaviv-gpu 134000.gpu: command buffer outside valid memory window and shortly after the login prompt appears, the entire SoC appears to lock up - it becomes unresponsive on the network, or via serial console to sysrq requests. I suspect the GPU ends up scribbling over the CPU's vector page/kernel as a result of the above two etnaviv errors when Xorg attempts to start using the GPU. This used to work, so its a regression. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 1/2] drm/i2c: tda998x: access chip registers via a regmap
On Fri, Jun 21, 2019 at 05:13:33PM -0400, Sven Van Asbroeck wrote:
> On Fri, Jun 21, 2019 at 11:15 AM Russell King - ARM Linux admin
> wrote:
> >
> > Another con is the need to keep the functions that detail the register
> > properties up to date, which if they're wrong may result in unexpected
> > behaviour.
> >
> > I subscribe to the "keep it simple" approach, and regmap, although
> > useful, seems like a giant sledgehammer for this.
> >
>
> Thank you for the review !
>
> I added this back when I was debugging audio artifacts related to this
> chip. The regmap's debugfs binding was extremely useful. So I
> dressed it up a bit in the hope that it would have some general use.
>
> But if the cons outweigh the pros, then this is as far as this patch
> will go...
I won't disagree that debugfs access to the registers is useful,
especially as I keep this patch locally for that exact purpose:
8<===
From: Russell King
Subject: [PATCH] drm/i2c: tda998x: debugfs register access
Add support for dumping the register contents via debugfs, with a
mechanism to write to the TDA998x registers to allow experimentation.
Signed-off-by: Russell King
---
drivers/gpu/drm/i2c/tda998x_drv.c | 114 ++
1 file changed, 114 insertions(+)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 5312fde8b624..6ad1fd1d28a5 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -16,10 +16,13 @@
*/
#include
+#include
+#include
#include
#include
#include
#include
+#include
#include
#include
#include
@@ -1239,6 +1242,116 @@ static int tda998x_audio_codec_init(struct tda998x_priv
*priv,
}
/* DRM connector functions */
+static u8 tda998x_debugfs_pages[] = {
+ 0x00, 0x01, 0x02, 0x09, 0x10, 0x11, 0x12, 0x13
+};
+
+static void *tda998x_regs_start(struct seq_file *s, loff_t *pos)
+{
+ return *pos < ARRAY_SIZE(tda998x_debugfs_pages) ? pos : NULL;
+}
+
+static void *tda998x_regs_next(struct seq_file *s, void *v, loff_t *pos)
+{
+ (*pos)++;
+ return *pos < ARRAY_SIZE(tda998x_debugfs_pages) ? pos : NULL;
+}
+
+static void tda998x_regs_stop(struct seq_file *s, void *v)
+{
+}
+
+static int tda998x_regs_show(struct seq_file *s, void *v)
+{
+ struct tda998x_priv *priv = s->private;
+ u8 page = tda998x_debugfs_pages[*(loff_t *)v];
+ unsigned int i, j;
+ u8 buf[16];
+
+ seq_printf(s, " page %02x
==\n", page);
+ seq_printf(s, " 0 1 2 3 4 5 6 7 8 9 a b c d e f\n");
+ for (i = 0; i < 256; i += sizeof(buf)) {
+ reg_read_range(priv, REG(page, i), buf, sizeof(buf));
+
+ seq_printf(s, "%02x:", i);
+ for (j = 0; j < sizeof(buf); j++)
+ seq_printf(s, " %02x", buf[j]);
+ seq_printf(s, " : ");
+ for (j = 0; j < sizeof(buf); j++)
+ seq_printf(s, "%c",
+ isascii(buf[j]) && isprint(buf[j]) ?
+ buf[j] : '.');
+ seq_putc(s, '\n');
+ }
+ return 0;
+}
+
+static const struct seq_operations tda998x_regs_sops = {
+ .start = tda998x_regs_start,
+ .next = tda998x_regs_next,
+ .stop = tda998x_regs_stop,
+ .show = tda998x_regs_show,
+};
+
+static int tda998x_regs_open(struct inode *inode, struct file *file)
+{
+ int ret = seq_open(file, _regs_sops);
+ if (ret < 0)
+ return ret;
+
+ ((struct seq_file *)file->private_data)->private = inode->i_private;
+
+ return 0;
+}
+
+static ssize_t tda998x_regs_write(struct file *file, const char __user *buf,
+ size_t count, loff_t *offset)
+{
+ struct tda998x_priv *priv =
+ ((struct seq_file *)file->private_data)->private;
+ unsigned int page, addr, mask, val;
+ unsigned char rval;
+ char buffer[16];
+
+ memset(buffer, 0, sizeof(buffer));
+ if (count > sizeof(buffer) - 1)
+ count = sizeof(buffer) - 1;
+ if (copy_from_user(buffer, buf, count))
+ return -EFAULT;
+
+ if (sscanf(buffer, "%x %x %x %x", , , , ) != 4)
+ return -EINVAL;
+ if (page > 0xff || addr > 0xff || mask > 0xff || val > 0xff)
+ return -ERANGE;
+
+ rval = reg_read(priv, REG(page, addr));
+ rval &= ~mask;
+ rval |= val & mask;
+ reg_write(priv, REG(page, addr), rval);
+
+ printk("i2c write %02x @ page:%02x address:%02x\n", rval, page, addr);
+
+ return count;
+}
+
+static const struct file_operations tda998x_regs_fop
Re: [PATCH v1 2/2] drm/i2c: tda998x: remove indirect reg_read/_write() calls
On Mon, May 27, 2019 at 03:15:52PM -0400, Sven Van Asbroeck wrote:
> -static void
> -reg_set(struct tda998x_priv *priv, u16 reg, u8 val)
> +static int
> +reg_set(struct regmap *regmap, u16 reg, u8 val)
I don't see the point of making this return an 'int' - you don't modify
any of the callsites to check the returned value, so returning a value
is not useful.
> {
> - regmap_update_bits(priv->regmap, reg, val, val);
> + return regmap_update_bits(regmap, reg, val, val);
> }
>
> -static void
> -reg_clear(struct tda998x_priv *priv, u16 reg, u8 val)
> +static int
> +reg_clear(struct regmap *regmap, u16 reg, u8 val)
Same here.
> @@ -685,16 +655,18 @@ static void tda998x_detect_work(struct work_struct
> *work)
> static irqreturn_t tda998x_irq_thread(int irq, void *data)
> {
> struct tda998x_priv *priv = data;
> - u8 sta, cec, lvl, flag0, flag1, flag2;
> + struct regmap *regmap = priv->regmap;
> + u8 sta, cec, lvl;
> + unsigned int flag0, flag1, flag2;
> bool handled = false;
>
> sta = cec_read(priv, REG_CEC_INTSTATUS);
> if (sta & CEC_INTSTATUS_HDMI) {
> cec = cec_read(priv, REG_CEC_RXSHPDINT);
> lvl = cec_read(priv, REG_CEC_RXSHPDLEV);
> - flag0 = reg_read(priv, REG_INT_FLAGS_0);
> - flag1 = reg_read(priv, REG_INT_FLAGS_1);
> - flag2 = reg_read(priv, REG_INT_FLAGS_2);
> + regmap_read(regmap, REG_INT_FLAGS_0, );
> + regmap_read(regmap, REG_INT_FLAGS_1, );
> + regmap_read(regmap, REG_INT_FLAGS_2, );
Not particularly enamoured by this...
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 1/2] drm/i2c: tda998x: access chip registers via a regmap
On Mon, May 27, 2019 at 03:15:51PM -0400, Sven Van Asbroeck wrote:
> The tda988x i2c chip registers are accessed through a
> paged register scheme. The kernel regmap abstraction
> supports paged accesses. Replace this driver's
> dedicated i2c access functions with a standard i2c
> regmap.
>
> Pros:
> - dedicated i2c access code disappears: accesses now
> go through well-maintained regmap code
> - page atomicity requirements now handled by regmap
> - ro/wo/rw access modes are now explicitly defined:
> any inappropriate register accesses (e.g. write to a
> read-only register) will now be explicitly rejected
> by the regmap core
> - all tda988x registers are now visible via debugfs
>
> Cons:
> - this will probably require extensive testing
Another con is the need to keep the functions that detail the register
properties up to date, which if they're wrong may result in unexpected
behaviour.
I subscribe to the "keep it simple" approach, and regmap, although
useful, seems like a giant sledgehammer for this.
>
> Tested on a TDA19988 using a Freescale/NXP imx6q.
>
> Signed-off-by: Sven Van Asbroeck
> ---
>
> I originally hacked this together while debugging an incompatibility between
> the tda988x's audio input and the audio codec I was driving it with.
> That required me to have debug access to the chip's register values.
> A regmap did the trick, it has good debugfs support.
>
> drivers/gpu/drm/i2c/tda998x_drv.c | 350 --
> 1 file changed, 234 insertions(+), 116 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 7f34601bb515..8153e2e19e18 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -43,10 +44,9 @@ struct tda998x_audio_port {
> struct tda998x_priv {
> struct i2c_client *cec;
> struct i2c_client *hdmi;
> - struct mutex mutex;
> + struct regmap *regmap;
> u16 rev;
> u8 cec_addr;
> - u8 current_page;
> bool is_on;
> bool supports_infoframes;
> bool sink_has_audio;
> @@ -88,12 +88,10 @@ struct tda998x_priv {
> /* The TDA9988 series of devices use a paged register scheme.. to simplify
> * things we encode the page # in upper bits of the register #. To read/
> * write a given register, we need to make sure CURPAGE register is set
> - * appropriately. Which implies reads/writes are not atomic. Fun!
> + * appropriately.
> */
>
> #define REG(page, addr) (((page) << 8) | (addr))
> -#define REG2ADDR(reg) ((reg) & 0xff)
> -#define REG2PAGE(reg) (((reg) >> 8) & 0xff)
>
> #define REG_CURPAGE 0xff/* write */
>
> @@ -285,8 +283,9 @@ struct tda998x_priv {
>
>
> /* Page 09h: EDID Control */
> +/* EDID_DATA consists of 128 successive registers read */
> #define REG_EDID_DATA_0 REG(0x09, 0x00) /* read */
> -/* next 127 successive registers are the EDID block */
> +#define REG_EDID_DATA_127 REG(0x09, 0x7f) /* read */
> #define REG_EDID_CTRL REG(0x09, 0xfa) /* read/write */
> #define REG_DDC_ADDR REG(0x09, 0xfb) /* read/write */
> #define REG_DDC_OFFS REG(0x09, 0xfc) /* read/write */
> @@ -295,11 +294,17 @@ struct tda998x_priv {
>
>
> /* Page 10h: information frames and packets */
> +/* REG_IF1_HB consists of 32 successive registers read/write */
> #define REG_IF1_HB0 REG(0x10, 0x20) /* read/write */
> +/* REG_IF2_HB consists of 32 successive registers read/write */
> #define REG_IF2_HB0 REG(0x10, 0x40) /* read/write */
> +/* REG_IF3_HB consists of 32 successive registers read/write */
> #define REG_IF3_HB0 REG(0x10, 0x60) /* read/write */
> +/* REG_IF4_HB consists of 32 successive registers read/write */
> #define REG_IF4_HB0 REG(0x10, 0x80) /* read/write */
> +/* REG_IF5_HB consists of 32 successive registers read/write */
> #define REG_IF5_HB0 REG(0x10, 0xa0) /* read/write */
> +#define REG_IF5_HB31 REG(0x10, 0xbf) /* read/write */
>
>
> /* Page 11h: audio settings and content info packets */
> @@ -542,93 +547,29 @@ static void tda998x_cec_hook_release(void *data)
> cec_enamods(priv, CEC_ENAMODS_EN_CEC_CLK | CEC_ENAMODS_EN_CEC, false);
> }
>
> -static int
> -set_page(struct tda998x_priv *priv, u16 reg)
> -{
> - if (REG2PAGE(reg) != priv->current_page) {
> - struct i2c_client *client = priv->hdmi;
> - u8 buf[] = {
> - REG_CURPAGE, REG2PAGE(reg)
> - };
> - int ret = i2c_master_send(client, buf, sizeof(buf));
> - if (ret < 0) {
> - dev_err(>dev, "%s %04x err %d\n", __func__,
> - reg, ret);
>
Re: [PATCH v2 00/13] tda998x updates
On Thu, Jun 13, 2019 at 03:51:06PM -0400, Sven Van Asbroeck wrote: > On Thu, Jun 13, 2019 at 10:29 AM Russell King - ARM Linux admin > wrote: > > > > This series represents development work collected over the last six > > months to improve the TDA998x driver, particularly for the audio > > side. These patches can be found in my "drm-tda998x-devel" branch > > at git://git.armlinux.org.uk/~rmk/linux-arm.git > > For the whole patchset, after 'hacking' the bclk_ratio to correspond with > my imx6q ssi's wire format: > > Tested-by: Sven Van Asbroeck Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 01/13] drm/i2c: tda998x: introduce tda998x_audio_settings
On Thu, Jun 13, 2019 at 02:48:47PM -0400, Sven Van Asbroeck wrote: > Nit: checkpatch.pl appears unhappy with this patch: > > WARNING: line over 80 characters > #222: FILE: drivers/gpu/drm/i2c/tda998x_drv.c:1011: > + audio.params.config = priv->audio_port[i].config; > > WARNING: line over 80 characters > #230: FILE: drivers/gpu/drm/i2c/tda998x_drv.c:1017: > + audio.params.config = priv->audio_port[i].config; > > total: 0 errors, 2 warnings, 178 lines checked Considering that code is going away in the very next patch, those warnings are not something I'm concerned about - the extra hassle of formatting the code so checkpatch is happy only to remove it in the next patch is not worth the effort. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] armada: no need to check return value of debugfs_create functions
On Thu, Jun 13, 2019 at 06:01:14PM +0200, Greg Kroah-Hartman wrote: > On Thu, Jun 13, 2019 at 03:36:00PM +0100, Russell King - ARM Linux admin > wrote: > > On Thu, Jun 13, 2019 at 03:28:50PM +0200, Greg Kroah-Hartman wrote: > > > When calling debugfs functions, there is no need to ever check the > > > return value. The function can work or not, but the code logic should > > > never do something different based on this. > > > > Please don't merge this patch - I have a change that conflicts with this > > which switches us over to using drm_debugfs_create_files(), thereby > > eliminating this code. > > Isn't it "first received, first applied?" That's how it is for my > subsystems... When I started working on the kernel in the 1990s, it was "the most technically correct approach of competing approaches". If we've now switched to "first received, first applied" that can only be harmful and demotivating to those who wish to do a good job. If someone has a better approach ready to go, why should the inferior approach be applied and then the better approach have to be rebased on top of the inferior approach? This makes no sense. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 00/18] Armada DRM updates
Hi, This series updates Armada DRM: - Fix interlace support. - use __drm_atomic_helper_plane_reset in overlay reset. - since the overlay and video planes use essentially the same format registers, precompute their values while validating. - fix a long-standing deficiency with overlay planes and interlace modes - calculate plane starting address at atomic_check stage rather than when we're programming the registers. - add gamma support. - ensure mode adjustments made by other components are properly handled in the driver and applied to the CRTC-programmed mode. - add and use register definitions for the "REG4F" register. - use drm_atomic_helper_shutdown() when tearing down to ensure that the hardware is properly shutdown. - add CRTC-level mode validation to ensure that we don't allow a mode that the CRTC-level hardware can not support. - improve the clocking selection for Armada 510 support. - move CRTC debugfs files into the crtc-specific directory, using the DRM helper to create these files. - patch from Lubomir Rintel to replace a simple framebuffer. - use the OF graph walker rather than open-coding this. - eliminate a useless check for the availability of the remote's parent which isn't required. drivers/gpu/drm/armada/armada_510.c | 130 ++-- drivers/gpu/drm/armada/armada_crtc.c| 212 ++-- drivers/gpu/drm/armada/armada_crtc.h| 21 +++- drivers/gpu/drm/armada/armada_debugfs.c | 98 ++- drivers/gpu/drm/armada/armada_drm.h | 1 + drivers/gpu/drm/armada/armada_drv.c | 38 +++--- drivers/gpu/drm/armada/armada_hw.h | 29 +++-- drivers/gpu/drm/armada/armada_overlay.c | 56 - drivers/gpu/drm/armada/armada_plane.c | 124 +-- drivers/gpu/drm/armada/armada_plane.h | 23 10 files changed, 520 insertions(+), 212 deletions(-) -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] armada: no need to check return value of debugfs_create functions
On Thu, Jun 13, 2019 at 03:28:50PM +0200, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value. The function can work or not, but the code logic should
> never do something different based on this.
Please don't merge this patch - I have a change that conflicts with this
which switches us over to using drm_debugfs_create_files(), thereby
eliminating this code.
>
> Cc: Russell King
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/gpu/drm/armada/armada_debugfs.c | 14 --
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/armada/armada_debugfs.c
> b/drivers/gpu/drm/armada/armada_debugfs.c
> index 6758c3a83de2..aec1e7372371 100644
> --- a/drivers/gpu/drm/armada/armada_debugfs.c
> +++ b/drivers/gpu/drm/armada/armada_debugfs.c
> @@ -109,7 +109,6 @@ static struct drm_info_list armada_debugfs_list[] = {
>
> int armada_drm_debugfs_init(struct drm_minor *minor)
> {
> - struct dentry *de;
> int ret;
>
> ret = drm_debugfs_create_files(armada_debugfs_list,
> @@ -118,15 +117,10 @@ int armada_drm_debugfs_init(struct drm_minor *minor)
> if (ret)
> return ret;
>
> - de = debugfs_create_file("reg", S_IFREG | S_IRUSR,
> - minor->debugfs_root, minor->dev, _reg_r);
> - if (!de)
> - return -ENOMEM;
> -
> - de = debugfs_create_file("reg_wr", S_IFREG | S_IWUSR,
> - minor->debugfs_root, minor->dev, _reg_w);
> - if (!de)
> - return -ENOMEM;
> + debugfs_create_file("reg", S_IFREG | S_IRUSR, minor->debugfs_root,
> + minor->dev, _reg_r);
> + debugfs_create_file("reg_wr", S_IFREG | S_IWUSR, minor->debugfs_root,
> + minor->dev, _reg_w);
>
> return 0;
> }
> --
> 2.22.0
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 00/13] tda998x updates
This series represents development work collected over the last six months to improve the TDA998x driver, particularly for the audio side. These patches can be found in my "drm-tda998x-devel" branch at git://git.armlinux.org.uk/~rmk/linux-arm.git - Introduce an audio_settings structure so we can store the derived register settings independently of the audio parameters. - Add support for the different I2S flavours. - Improve the calculation of the audio clock divisor, the old code seemed to prevent combinations of video mode and audio sample rate from working. Document what we don't know, what assumptions we are making, what has been found through experimentation, and what we are actually doing. - Add calculation of the CTS n and k values depending on the bit clock to sample rate ratio (bclk_ratio) - we assume a bclk_ratio appropriate for the bus format that gives us the values we are using prior to this change to maintain compatibility. - Move the "ena_ap" register value into the audio_settings structure. - Eliminate the audio port array and repetitive searching of the array, instead looking up the "ena_ap" value by format - the DT binding only supports one setting per format, so old approach was not a good design. - There is no need to set the two fields of the AIP_CLKSEL register independently of each other, so just write the register once while setting up audio. - Deal with the format specific audio routing configuration when the audio settings are configured, rather than when programming the TDA998x, which means we can do all the validation at configuration time, rather than spreading it into other paths like modeset, where failures can't be reported. - Since tda998x_configure_audio() is called from paths where failure is not an option, and we have eliminated the configuration dependent failures, we can make this functions return type void. This allows simplification of tda998x_audio_hw_params() within the mutex protected region. - We no longer need to store the full audio params within the audio settings structure, so eliminate it, only storing in the audio settings structure what we need to actually program the hardware. - Add support for pixel repeated modes, tested on a Panasonic TV that supports these formats. - Add support for selecting the appropriate RGB quantisation range depending on the sink capabilities, and avoid sending full range RGB when the sink only supports limited range RGB for the mode. We do this by enabling the TDA998x's colour scaling matrix and applying the appropriate constants. - Add support for setting the vendor specific infoframe, which ensures that we supply the information the sink expects for 3D modes (we support a htotal/vtotal up to 8191x2047, which allows for at least side-by-side 3D.) drivers/gpu/drm/i2c/tda998x_drv.c | 450 +- 1 file changed, 302 insertions(+), 148 deletions(-) -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/13] tda998x updates
On Wed, Jun 12, 2019 at 11:40:43AM -0400, Sven Van Asbroeck wrote: > On Tue, Jun 11, 2019 at 7:01 AM Russell King - ARM Linux admin > wrote: > > > > This series represents development work collected over the last six > > months to improve the TDA998x driver, particularly for the audio > > side. These patches can be found in my "drm-tda998x-devel" branch > > at git://git.armlinux.org.uk/~rmk/linux-arm.git > > Thank you Russell !! > > I tested this patch set on my platform: imx6q ssi codec driving a tda19988. > No issues as far as I can tell. > > Note that I still have to 'hack' the bclk_ratio in the tda driver to > correspond > with the wire format that the imx6q ssi is generating. But that's a known > issue. Yep, I avoided posting those patches so that this set can get merged, which moves us towards supporting more I2S formats. Once we have agreement with these (which should be easy) we can then resume trying to resolve the hdmi-codec side of the problem. I'll repost the patch set later today, I'm intending to drop the bridge timing patch from the set for now, but add the patch for the vendor-specific infoframe that can go upstream in its place. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 12/13] drm/i2c: tda998x: add bridge timing information
On Wed, Jun 12, 2019 at 11:38:06AM -0400, Sven Van Asbroeck wrote:
> On Tue, Jun 11, 2019 at 7:04 AM Russell King
> wrote:
> >
> > Add bridge timing information so that bridge users can figure out the
> > timing parameters that are necessary for TDA998x.
> >
> > Signed-off-by: Russell King
> > ---
>
> +static const struct drm_bridge_timings tda9989_timings = {
> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> + .setup_time_ps = 1500,
> + .hold_time_ps = 1000,
> +};
> +
> +static const struct drm_bridge_timings tda19988_timings = {
> + .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
> + .setup_time_ps = 1600,
> + .hold_time_ps = 1200,
> +};
>
> Need to port these to 5.1 kernel: sampling_edge -> input_bus_flags ?
5.1 still has it as "sampling_edge", but 5.2-rc changes this, so
I'll drop this patch from this series for now. Thanks for pointing
that out.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/13] drm/i2c: tda998x: introduce tda998x_audio_settings
On Wed, Jun 12, 2019 at 11:24:46AM -0400, Sven Van Asbroeck wrote:
> On Tue, Jun 11, 2019 at 7:01 AM Russell King
> wrote:
> >
> > Introduce a structure to hold the register values to be programmed while
> > programming the TDA998x audio settings. This is currently a stub
> > structure, which will be populated in subsequent commits.
> >
> > When we initialise this from the platform data, only do so if there is a
> > valid audio format specification.
> >
> > Signed-off-by: Russell King
> > ---
>
> Nit:
> Fix compile errors? Note that these errors disappear if patch 2/13
> is applied, but maybe we want to make sure that every patch compiles
> so git bisect does not break?
>
> drivers/gpu/drm/i2c/tda998x_drv.c: In function ‘tda998x_audio_hw_params’:
> drivers/gpu/drm/i2c/tda998x_drv.c:1011:10: error: ‘struct
> tda998x_audio_settings’ has no member named ‘config’
> audio.config = priv->audio_port[i].config;
> ^
> drivers/gpu/drm/i2c/tda998x_drv.c:1012:8: error: ‘struct
> tda998x_audio_settings’ has no member named ‘format’
>audio.format = AFMT_I2S;
> ^
> drivers/gpu/drm/i2c/tda998x_drv.c:1017:10: error: ‘struct
> tda998x_audio_settings’ has no member named ‘config’
> audio.config = priv->audio_port[i].config;
> ^
> drivers/gpu/drm/i2c/tda998x_drv.c:1018:8: error: ‘struct
> tda998x_audio_settings’ has no member named ‘format’
>audio.format = AFMT_SPDIF;
> ^
> drivers/gpu/drm/i2c/tda998x_drv.c:1025:11: error: ‘struct
> tda998x_audio_settings’ has no member named ‘config’
> if (audio.config == 0) {
>^
Thanks for reporting. All fixed.
However, I'm concerned that the 0-day builder never found these despite
this code being published for many weeks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 04/13] drm/i2c: tda998x: derive CTS_N value from aclk sample rate ratio
On Wed, Jun 12, 2019 at 12:37:57PM -0400, Sven Van Asbroeck wrote: > On Wed, Jun 12, 2019 at 12:28 PM Russell King - ARM Linux admin > wrote: > > > > The platform data path has never supported the HDMI codec way of doing > > things, so that really isn't a concern here. The platform data way > > was sufficient to allow SPDIF streams to work with a static setup of > > the TDA998x, and has never been intended to support anything beyond > > that. > > Thank you, I am not using the platform data path, so I had no idea. > > Wouldn't the current code always fail on the platform data path though? > > create() calls tda998x_set_config() in the platform data case. > If the audio_params.format is used (i.e. the platform data configures > audio), the function then returns the return value of tda998x_derive_cts_n(), > which is a positive divider (can be 0 if /1). I think you're confusing tda998x_derive_cts_n() and tda998x_get_adiv(). tda998x_derive_cts_n() only returns 0 or -EINVAL. > > Then in create(): if (ret) goto fail; > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/13] drm/i2c: tda998x: move audio routing configuration
On Wed, Jun 12, 2019 at 11:36:59AM -0400, Sven Van Asbroeck wrote:
> On Tue, Jun 11, 2019 at 7:02 AM Russell King
> wrote:
> >
> > Move the mux and clocking selection out of tda998x_configure_audio()
> > into the parent functions, so we can validate this when parameters
> > are set outside of the audio mutex.
> >
> > Signed-off-by: Russell King
> > ---
>
> +/* Configure the TDA998x audio data and clock routing. */
> +static int tda998x_derive_routing(struct tda998x_priv *priv,
> + struct tda998x_audio_settings *s,
> + unsigned int route)
> +{
> + s->route = _audio_route[route];
> + s->ena_ap = priv->audio_port_enable[route];
> + if (s->ena_ap == 0) {
> + dev_err(>hdmi->dev, "no audio configuration found\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
>
> Nit: priv is nearly unused in this function.
> Maybe delegate the error log to the caller, in that case we could just pass
> route and const audio_port_enable to the function. Instead of passing in the
> 'kitchen sink' priv ?
I don't think that's worth doing. This way, compilers are free to
emit code to bounds-check the audio_port_enable access since they
know that it is a defined size. Passing in a const pointer ca
mean that check has to be avoided.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 04/13] drm/i2c: tda998x: derive CTS_N value from aclk sample rate ratio
On Wed, Jun 12, 2019 at 11:27:16AM -0400, Sven Van Asbroeck wrote:
> On Tue, Jun 11, 2019 at 7:02 AM Russell King
> wrote:
> >
> > The TDA998x derives the CTS value using the supplied I2S bit clock
> > (ACLK, in TDA998x parlence) rather than 128·fs. TDA998x uses two
> > constants named m and k in the CTS generator such that we have this
> > relationship between the I2S source ACLK and the sink fs:
> >
> > 128·fs_sink = ACLK·m / k
> >
> > Where ACLK = aclk_ratio·fs_source.
> >
> > When audio support was originally added, we supported a fixed ratio
> > of 64·fs, intending to support the Kirkwood I2S on Dove. However,
> > when hdmi-codec support was added, this was changed to scale the
> > ratio with the sample width, which would've broken its use with
> > Kirkwood I2S.
> >
> > We are now starting to see other users whose I2S blocks send at 64·fs
> > for 16-bit samples, so we need to reinstate the support for the fixed
> > ratio I2S bit clock.
> >
> > This commit takes a step towards supporting these configurations by
> > selecting the CTS_N register m and k values based on the bit clock
> > ratio. However, as the driver is not given the bit clock ratio from
> > ALSA, continue deriving this from the sample width. This will be
> > addressed in a later commit.
> >
> > Signed-off-by: Russell King
> > ---
>
> @@ -1657,9 +1701,17 @@ static void tda998x_set_config(struct tda998x_priv
> *priv,
> (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
>
> if (p->audio_params.format != AFMT_UNUSED) {
> + unsigned int ratio;
> + bool spdif = p->audio_params.format == AFMT_SPDIF;
> +
> priv->audio.params = p->audio_params;
> priv->audio.i2s_format = I2S_FORMAT_PHILIPS;
> +
> + ratio = spdif ? 64 : p->audio_params.sample_width * 2;
> + return tda998x_derive_cts_n(priv, >audio, ratio);
>
> Won't this make the platform_data path fail all the time?
> Also, the platform_data path doesn't appear to instantiate the HDMI_CODEC,
> so tda audio support would be completely missing in this case?
The platform data path has never supported the HDMI codec way of doing
things, so that really isn't a concern here. The platform data way
was sufficient to allow SPDIF streams to work with a static setup of
the TDA998x, and has never been intended to support anything beyond
that.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 03/13] drm/i2c: tda998x: improve programming of audio divisor
On Wed, Jun 12, 2019 at 11:25:59AM -0400, Sven Van Asbroeck wrote:
> On Tue, Jun 11, 2019 at 7:02 AM Russell King
> wrote:
> >
> > Improve the selection of the audio clock divisor so that more modes
> > and sample rates work.
> >
> > Signed-off-by: Russell King
> > ---
>
> +static u8 tda998x_get_adiv(struct tda998x_priv *priv, unsigned int fs)
> +{
> + unsigned long min_audio_clk = fs * 128;
> + unsigned long ser_clk = priv->tmds_clock * 1000;
> + u8 adiv;
> +
> + for (adiv = AUDIO_DIV_SERCLK_32; adiv != AUDIO_DIV_SERCLK_1; adiv--)
> + if (ser_clk > min_audio_clk << adiv)
> + break;
> +
> + dev_dbg(>hdmi->dev,
> + "ser_clk=%luHz fs=%uHz min_aclk=%luHz adiv=%d\n",
> + ser_clk, fs, min_audio_clk, adiv);
> +
> + return adiv;
>
> Doesn't this function need an error return in case min_audio_clk > ser_clk ?
> Or is that a situation that can never arise?
It's possible it could arise. For example, if we have a 192kHz sample
rate, and a tmds clock slower than 24.576MHz.
In such a case, we will select AUDIO_DIV_SERCLK_1 as the divisor, which
is a legal value. The result _may_ be audio not working (which is what
already happens today when we get this setting wrong.)
If we were to return an error, there's no way to handle that error, so
what's the point of returning an error?
The results of this function match what the Philips firmware uses for
this register for all standard TMDS clocks and sample rates, so it's
not a problem that I'm particularly concerned about at this point.
The only way around this would be to increase the TMDS clock, which
means using pixel doubling tricks, and therefore a mode set. I don't
think any HDMI driver has the capability to deal with that yet.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 00/13] tda998x updates
This series represents development work collected over the last six months to improve the TDA998x driver, particularly for the audio side. These patches can be found in my "drm-tda998x-devel" branch at git://git.armlinux.org.uk/~rmk/linux-arm.git - Introduce an audio_settings structure so we can store the derived register settings independently of the audio parameters. - Add support for the different I2S flavours. - Improve the calculation of the audio clock divisor, the old code seemed to prevent combinations of video mode and audio sample rate from working. Document what we don't know, what assumptions we are making, what has been found through experimentation, and what we are actually doing. - Add calculation of the CTS n and k values depending on the bit clock to sample rate ratio (bclk_ratio) - we assume a bclk_ratio appropriate for the bus format that gives us the values we are using prior to this change to maintain compatibility. - Move the "ena_ap" register value into the audio_settings structure. - Eliminate the audio port array and repetitive searching of the array, instead looking up the "ena_ap" value by format - the DT binding only supports one setting per format, so old approach was not a good design. - There is no need to set the two fields of the AIP_CLKSEL register independently of each other, so just write the register once while setting up audio. - Deal with the format specific audio routing configuration when the audio settings are configured, rather than when programming the TDA998x, which means we can do all the validation at configuration time, rather than spreading it into other paths like modeset, where failures can't be reported. - Since tda998x_configure_audio() is called from paths where failure is not an option, and we have eliminated the configuration dependent failures, we can make this functions return type void. This allows simplification of tda998x_audio_hw_params() within the mutex protected region. - We no longer need to store the full audio params within the audio settings structure, so eliminate it, only storing in the audio settings structure what we need to actually program the hardware. - Add support for pixel repeated modes, tested on a Panasonic TV that supports these formats. - Add bridge timing specifications for TDA9989 and TDA19988 devices, although we have no current users of this data it may be useful to complete the information passed from the bridge driver, especially where the pixel clock edge needs configuration. - Add support for selecting the appropriate RGB quantisation range depending on the sink capabilities, and avoid sending full range RGB when the sink only supports limited range RGB for the mode. We do this by enabling the TDA998x's colour scaling matrix and applying the appropriate constants. drivers/gpu/drm/i2c/tda998x_drv.c | 459 ++ 1 file changed, 311 insertions(+), 148 deletions(-) -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/etnaviv: lock MMU while dumping core
On Wed, May 22, 2019 at 11:55:14AM +0200, Lucas Stach wrote:
> The devcoredump needs to operate on a stable state of the MMU while
> it is writing the MMU state to the coredump. The missing lock
> allowed both the userspace submit, as well as the GPU job finish
> paths to mutate the MMU state while a coredump is under way.
This locking does little to solve this problem. We actually rely on the
GPU being deadlocked at this point - we aren't expecting the GPU to make
any progress. The only thing that can realistically happen is for
userspace to submit a new job, but adding this locking does little to
avoid that.
> Fixes: a8c21a5451d8 (drm/etnaviv: add initial etnaviv DRM driver)
> Reported-by: David Jander
> Signed-off-by: Lucas Stach
> Tested-by: David Jander
> ---
> drivers/gpu/drm/etnaviv/etnaviv_dump.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> index 33854c94cb85..515515ef24f9 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> @@ -125,6 +125,8 @@ void etnaviv_core_dump(struct etnaviv_gpu *gpu)
> return;
> etnaviv_dump_core = false;
>
> + mutex_lock(>mmu->lock);
> +
> mmu_size = etnaviv_iommu_dump_size(gpu->mmu);
>
> /* We always dump registers, mmu, ring and end marker */
> @@ -167,6 +169,7 @@ void etnaviv_core_dump(struct etnaviv_gpu *gpu)
> iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_NOWARN |
> __GFP_NORETRY,
> PAGE_KERNEL);
> if (!iter.start) {
> + mutex_unlock(>mmu->lock);
> dev_warn(gpu->dev, "failed to allocate devcoredump file\n");
> return;
> }
> @@ -234,6 +237,8 @@ void etnaviv_core_dump(struct etnaviv_gpu *gpu)
>obj->base.size);
> }
>
> + mutex_unlock(>mmu->lock);
> +
All that this lock does is prevent the lists from changing while we
build up what we're going to write out. At this point, you drop the
lock, before we've written anything out to the coredump. Things
can now change, including the ring buffer.
> etnaviv_core_dump_header(, ETDUMP_BUF_END, iter.data);
>
> dev_coredumpv(gpu->dev, iter.start, iter.data - iter.start, GFP_KERNEL);
Here we write out the data, which includes the command buffers, ring
buffers, BOs, etc. The data in the ring buffer can still change
because the lock has been dropped.
However, all those objects should be pinned, so none of them should
go away: things like the command buffers that have been submitted
should be immutable at this point (if they aren't, it could well be
a reason why the GPU has gone awol.)
It is also not nice to hold the lock over the _big_ vmalloc() which
may take some time.
Have you actually seen problems here, or is this just theoretical?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: etnaviv: avoid DMA API warning when importing buffers
On Wed, May 22, 2019 at 12:04:27PM +0200, Lucas Stach wrote: > Hi Russell, > > Am Samstag, den 18.05.2019, 22:37 +0100 schrieb Russell King - ARM > Linux admin: > > On Sat, May 18, 2019 at 06:04:42PM -0300, Fabio Estevam wrote: > > > Hi Russell, > > > > > > On Sat, May 18, 2019 at 2:51 PM Russell King - ARM Linux admin > > > wrote: > > > > > > > > Ping. > > > > > > This patch is present in Lucas' pull request: > > > https://lists.freedesktop.org/archives/etnaviv/2019-May/002490.html > > > > I'm wondering why it didn't make 5.1 since it's a regression. > > I didn't see the importance to put this into fixes, as it's getting rid > of a warning which will only be present when a debug option is enabled. > So it should be invisible to most users and it doesn't regress > functionality. That depends on your point of view, how you use the kernel, and what you're using the kernel for. If you're trying to use dma debugging (which should always be enabled when doing driver development) and you have a subsystem that keeps triggering it, then it is a serious problem. Given that we want developers to have such options on, having false complaints is counter-productive. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/etnaviv: lock MMU while dumping core
Hi Lucas,
Seems I'm not getting a reply, so I'm hijacking one of your more
recent patches in the hope of attracting your attention.
A while back I sent a fix for a regression that recently occurred
with etnaviv, where the kernel spat out a warning when importing
buffers into etnaviv. You apparently merged this as "development",
queuing it up for the last merge window.
Since it is a regression (although not directly attributable to
etnaviv), please ensure that it is merged into stable kernels.
Thanks.
On Wed, May 22, 2019 at 11:55:14AM +0200, Lucas Stach wrote:
> The devcoredump needs to operate on a stable state of the MMU while
> it is writing the MMU state to the coredump. The missing lock
> allowed both the userspace submit, as well as the GPU job finish
> paths to mutate the MMU state while a coredump is under way.
>
> Fixes: a8c21a5451d8 (drm/etnaviv: add initial etnaviv DRM driver)
> Reported-by: David Jander
> Signed-off-by: Lucas Stach
> Tested-by: David Jander
> ---
> drivers/gpu/drm/etnaviv/etnaviv_dump.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> index 33854c94cb85..515515ef24f9 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> @@ -125,6 +125,8 @@ void etnaviv_core_dump(struct etnaviv_gpu *gpu)
> return;
> etnaviv_dump_core = false;
>
> + mutex_lock(>mmu->lock);
> +
> mmu_size = etnaviv_iommu_dump_size(gpu->mmu);
>
> /* We always dump registers, mmu, ring and end marker */
> @@ -167,6 +169,7 @@ void etnaviv_core_dump(struct etnaviv_gpu *gpu)
> iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_NOWARN |
> __GFP_NORETRY,
> PAGE_KERNEL);
> if (!iter.start) {
> + mutex_unlock(>mmu->lock);
> dev_warn(gpu->dev, "failed to allocate devcoredump file\n");
> return;
> }
> @@ -234,6 +237,8 @@ void etnaviv_core_dump(struct etnaviv_gpu *gpu)
>obj->base.size);
> }
>
> + mutex_unlock(>mmu->lock);
> +
> etnaviv_core_dump_header(, ETDUMP_BUF_END, iter.data);
>
> dev_coredumpv(gpu->dev, iter.start, iter.data - iter.start, GFP_KERNEL);
> --
> 2.20.1
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: etnaviv: avoid DMA API warning when importing buffers
On Sat, May 18, 2019 at 06:04:42PM -0300, Fabio Estevam wrote: > Hi Russell, > > On Sat, May 18, 2019 at 2:51 PM Russell King - ARM Linux admin > wrote: > > > > Ping. > > This patch is present in Lucas' pull request: > https://lists.freedesktop.org/archives/etnaviv/2019-May/002490.html I'm wondering why it didn't make 5.1 since it's a regression. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: etnaviv: avoid DMA API warning when importing buffers
Ping.
On Mon, Feb 25, 2019 at 10:54:23AM +, Russell King - ARM Linux admin wrote:
> On Mon, Feb 25, 2019 at 10:51:30AM +, Russell King wrote:
> > During boot, I get this kernel warning:
> >
> > WARNING: CPU: 0 PID: 19001 at kernel/dma/debug.c:1301
> > debug_dma_map_sg+0x284/0x3dc
> > etnaviv etnaviv: DMA-API: mapping sg segment longer than device claims to
> > support [len=3145728] [max=65536]
> > Modules linked in: ip6t_REJECT nf_reject_ipv6 ip6t_rpfilter xt_tcpudp
> > ipt_REJECT nf_reject_ipv4 xt_conntrack ip_set nfnetlink ebtable_broute
> > ebtable_nat ip6table_raw ip6table_nat nf_nat_ipv6 ip6table_mangle
> > iptable_raw iptable_nat nf_nat_ipv4 nf_nat nf_conntrack nf_defrag_ipv4
> > nf_defrag_ipv6 libcrc32c iptable_mangle ebtable_filter ebtables
> > ip6table_filter ip6_tables iptable_filter caam_jr error snd_soc_imx_spdif
> > imx_thermal snd_soc_imx_audmux nvmem_imx_ocotp snd_soc_sgtl5000
> > caam imx_sdma virt_dma coda rc_cec v4l2_mem2mem snd_soc_fsl_ssi
> > snd_soc_fsl_spdif imx_vdoa imx_pcm_dma videobuf2_dma_contig etnaviv
> > dw_hdmi_cec gpu_sched dw_hdmi_ahb_audio imx6q_cpufreq nfsd sch_fq_codel
> > ip_tables x_tables
> > CPU: 0 PID: 19001 Comm: Xorg Not tainted 4.20.0+ #307
> > Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> > [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> > [] (show_stack) from [] (dump_stack+0x9c/0xd4)
> > [] (dump_stack) from [] (__warn+0xf8/0x124)
> > [] (__warn) from [] (warn_slowpath_fmt+0x38/0x48)
> > [] (warn_slowpath_fmt) from []
> > (debug_dma_map_sg+0x284/0x3dc)
> > [] (debug_dma_map_sg) from []
> > (drm_gem_map_dma_buf+0xc4/0x13c)
> > [] (drm_gem_map_dma_buf) from []
> > (dma_buf_map_attachment+0x38/0x5c)
> > [] (dma_buf_map_attachment) from []
> > (drm_gem_prime_import_dev+0x74/0x104)
> > [] (drm_gem_prime_import_dev) from []
> > (drm_gem_prime_fd_to_handle+0x84/0x17c)
> > [] (drm_gem_prime_fd_to_handle) from []
> > (drm_prime_fd_to_handle_ioctl+0x38/0x4c)
> > [] (drm_prime_fd_to_handle_ioctl) from []
> > (drm_ioctl_kernel+0x90/0xc8)
> > [] (drm_ioctl_kernel) from [] (drm_ioctl+0x1e0/0x3b0)
> > [] (drm_ioctl) from [] (do_vfs_ioctl+0x90/0xa48)
> > [] (do_vfs_ioctl) from [] (ksys_ioctl+0x34/0x60)
> > [] (ksys_ioctl) from [] (ret_fast_syscall+0x0/0x28)
> > Exception stack(0xd81a9fa8 to 0xd81a9ff0)
> > 9fa0: b6c69c88 bec613f8 0009 c00c642e bec613f8
> > b86c4600
> > 9fc0: b6c69c88 bec613f8 c00c642e 0036 012762e0 01276348 0300
> > 012d91f8
> > 9fe0: b6989f18 bec613dc b697185c b667be5c
> > irq event stamp: 47905
> > hardirqs last enabled at (47913): [] console_unlock+0x46c/0x680
> > hardirqs last disabled at (47922): [] console_unlock+0xb8/0x680
> > softirqs last enabled at (47754): [] __do_softirq+0x344/0x540
> > softirqs last disabled at (47701): [] irq_exit+0x124/0x144
> > ---[ end trace af477747acbcc642 ]---
> >
> > The reason is the contiguous buffer exceeds the default maximum segment
> > size of 64K as specified by dma_get_max_seg_size() in
> > linux/dma-mapping.h. Fix this by providing our own segment size, which
> > is set to 2GiB to cover the window found in MMUv1 GPUs.
> >
> > Signed-off-by: Russell King
>
> Fixes: 78c47830a5cb ("dma-debug: check scatterlist segments")
>
> not really that there's a problem with that commit, but it is where the
> warning was introduced.
>
> > ---
> > Patch against v4.20.
> >
> > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 4
> > drivers/gpu/drm/etnaviv/etnaviv_drv.h | 1 +
> > 2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > index 4713885012ab..e414f284f424 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > @@ -527,6 +527,9 @@ static int etnaviv_bind(struct device *dev)
> > }
> > drm->dev_private = priv;
> >
> > + dev->dma_parms = >dma_parms;
> > + dma_set_max_seg_size(dev, SZ_2G);
> > +
> > mutex_init(>gem_lock);
> > INIT_LIST_HEAD(>gem_list);
> > priv->num_gpus = 0;
> > @@ -564,6 +567,7 @@ static void etnaviv_unbind(struct device *dev)
> >
> > component_unbind_all(dev, drm);
> >
> > + dev->dma_parms = NULL;
> > drm->dev_private = NULL;
> > kfree(priv);
> >
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> > b/drivers/gpu/drm/etna
Re: [PATCH] dma-buf: add struct dma_buf_attach_info v2
On Tue, Apr 30, 2019 at 01:10:02PM +0200, Christian König wrote:
> Add a structure for the parameters of dma_buf_attach, this makes it much
> easier
> to add new parameters later on.
I don't understand this reasoning. What are the "new parameters" that
are being proposed, and why do we need to put them into memory to pass
them across this interface?
If the intention is to make it easier to change the interface, passing
parameters in this manner mean that it's easy for the interface to
change and drivers not to notice the changes, since the compiler will
not warn (unless some member of the structure that the driver is using
gets removed, in which case it will error.)
Additions to the structure will go unnoticed by drivers - what if the
caller is expecting some different kind of behaviour, and the driver
ignores that new addition?
This doesn't seem to me like a good idea.
>
> v2: rebase cleanup and fix all new implementations as well
>
> Signed-off-by: Christian König
> ---
> drivers/dma-buf/dma-buf.c | 13 +++--
> drivers/gpu/drm/armada/armada_gem.c | 6 +-
> drivers/gpu/drm/drm_prime.c | 6 +-
> drivers/gpu/drm/i915/i915_gem_dmabuf.c | 6 +-
> drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 6 +-
> drivers/gpu/drm/tegra/gem.c | 6 +-
> drivers/gpu/drm/udl/udl_dmabuf.c| 6 +-
> .../common/videobuf2/videobuf2-dma-contig.c | 6 +-
> .../media/common/videobuf2/videobuf2-dma-sg.c | 6 +-
> drivers/misc/fastrpc.c | 6 +-
> drivers/staging/media/tegra-vde/tegra-vde.c | 6 +-
> drivers/xen/gntdev-dmabuf.c | 4
> include/linux/dma-buf.h | 17 +++--
> 13 files changed, 76 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 3ae6c0c2cc02..e295e76a8c57 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -535,8 +535,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
> /**
> * dma_buf_attach - Add the device to dma_buf's attachments list; optionally,
> * calls attach() of dma_buf_ops to allow device-specific attach
> functionality
> - * @dmabuf: [in]buffer to attach device to.
> - * @dev: [in]device to be attached.
> + * @info:[in]holds all the attach related information provided
> + * by the importer. see dma_buf_attach_info
> + * for further details.
> *
> * Returns struct dma_buf_attachment pointer for this attachment. Attachments
> * must be cleaned up by calling dma_buf_detach().
> @@ -550,20 +551,20 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
> * accessible to @dev, and cannot be moved to a more suitable place. This is
> * indicated with the error code -EBUSY.
> */
> -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
> - struct device *dev)
> +struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info
> *info)
> {
> + struct dma_buf *dmabuf = info->dmabuf;
> struct dma_buf_attachment *attach;
> int ret;
>
> - if (WARN_ON(!dmabuf || !dev))
> + if (WARN_ON(!dmabuf || !info->dev))
> return ERR_PTR(-EINVAL);
>
> attach = kzalloc(sizeof(*attach), GFP_KERNEL);
> if (!attach)
> return ERR_PTR(-ENOMEM);
>
> - attach->dev = dev;
> + attach->dev = info->dev;
> attach->dmabuf = dmabuf;
>
> mutex_lock(>lock);
> diff --git a/drivers/gpu/drm/armada/armada_gem.c
> b/drivers/gpu/drm/armada/armada_gem.c
> index 642d0e70d0f8..19c47821032f 100644
> --- a/drivers/gpu/drm/armada/armada_gem.c
> +++ b/drivers/gpu/drm/armada/armada_gem.c
> @@ -501,6 +501,10 @@ armada_gem_prime_export(struct drm_device *dev, struct
> drm_gem_object *obj,
> struct drm_gem_object *
> armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
> {
> + struct dma_buf_attach_info attach_info = {
> + .dev = dev->dev,
> + .dmabuf = buf
> + };
> struct dma_buf_attachment *attach;
> struct armada_gem_object *dobj;
>
> @@ -516,7 +520,7 @@ armada_gem_prime_import(struct drm_device *dev, struct
> dma_buf *buf)
> }
> }
>
> - attach = dma_buf_attach(buf, dev->dev);
> + attach = dma_buf_attach(_info);
> if (IS_ERR(attach))
> return ERR_CAST(attach);
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index dc079efb3b0f..1dd70fc095ee 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -710,6 +710,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct
> drm_device *dev,
> struct dma_buf *dma_buf,
> struct device *attach_dev)
> {
> + struct
Re: remove NULL struct device support in the DMA API
On Thu, Mar 21, 2019 at 03:52:28PM -0700, Christoph Hellwig wrote: > We still have a few drivers which pass a NULL struct device pointer > to DMA API functions, which generally is a bad idea as the API > implementations rely on the device not only for ops selection, but > also the dma mask and various other attributes, and many implementations > have been broken for NULL device support for a while. I think I must be missing something, but... My understanding is that ISA DMA is normally limited to 24 bits of address - indeed, the x86 version only programs 24 bits of DMA address. Looking through this series, it appears that the conversions mean that the DMA mask for ISA becomes the full all-ones DMA mask, which would of course lead to memory corruption if only 24 bits of the address end up being programmed into the hardware. Maybe you could say why you think this series is safe in regard to ISA DMA? > > This series removes the few remaning users that weren't picked up in > the last merge window and then removes core support for this "feature". > > A git tree is also available at: > > git://git.infradead.org/users/hch/misc.git dma-remove-NULL-dev-support > > Gitweb: > > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-remove-NULL-dev-support > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: etnaviv: fix strncpy sizeof argument
On Mon, Mar 18, 2019 at 10:57:55PM -0400, Bo YU wrote: > Calling strncpy with a maximum size argument of 64 bytes on destination > array "domain->name" of size 64 bytes might leave the destination string > unterminated. > > Detected by CoverityScan, CID# 1443992: Memory - illegal accesses > (BUFFER_SIZE_WARNING) > > Fixes: 9e2c2e2730126 (drm/etnaviv: add infrastructure to query perf counter) > Signed-off-by: Bo YU > --- > drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c > b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c > index 4227a4006c34..08ca3c44be48 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c > @@ -414,7 +414,7 @@ int etnaviv_pm_query_dom(struct etnaviv_gpu *gpu, > > domain->id = domain->iter; > domain->nr_signals = dom->nr_signals; > - strncpy(domain->name, dom->name, sizeof(domain->name)); > + strncpy(domain->name, dom->name, sizeof(dom->name)); This introduces an overflow bug if sizeof(dom->name) > sizeof(domain->name). If both sizes are the same, then there is no effect. strlcpy() would be a better replacement, it guarantees that the destination will be correctly terminated. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [TWO BUGs] etnaviv crashes overnight
On Tue, Feb 26, 2019 at 11:02:48AM +0100, Lucas Stach wrote:
> Hi Russell,
>
> Am Dienstag, den 26.02.2019, 09:24 + schrieb Russell King - ARM Linux
> admin:
> > I'm not sure when this happened, only that it happened sometime
> > overnight. It was left running an Xfce desktop having only logged in,
> > but with the monitor disconnected. Fedora 23 plus xf86-video-armada.
> >
> > I don't have any more information than that and the kernel messages
> > below.
>
> Thanks for the report! Both of those issues are caused by the GPU
> scheduler failing to put the scheduler fence callback execution into a
> work item, like it normally does, in the dying sched_entity cleanup.
> This causes multiple code paths which expect to be called from a clean
> process context to be called from the same IRQ context with a number of
> locks potentially already held.
>
> It will take me some time to work through the corners of this code, but
> I should have a patch fixing this later today.
Hi Lucas,
Did you get a chance to patch this bug?
Thanks.
>
> Regards,
> Lucas
>
> > [51328.909729]
> > [51328.915044] WARNING: possible recursive locking detected
> > [51328.920361] 4.20.0+ #307 Not tainted
> > [51328.923939]
> > [51328.929254] Xorg/5379 is trying to acquire lock:
> > [51328.933874] cd12d5e4 (&(>lock)->rlock){-.-.}, at:
> > dma_fence_signal+0x9c/0x1d4
> > [51328.941638]
> > [51328.941638] but task is already holding lock:
> > [51328.947474] cd12d6a4 (&(>lock)->rlock){-.-.}, at:
> > dma_fence_signal+0x9c/0x1d4
> > [51328.955226]
> > [51328.955226] other info that might help us debug this:
> > [51328.961756] Possible unsafe locking scenario:
> > [51328.961756]
> > [51328.967678]CPU0
> > [51328.970127]
> > [51328.976761] lock(&(>lock)->rlock);
> > [51328.980948]
> > [51328.980948] *** DEADLOCK ***
> > [51328.980948]
> > [51328.986871] May be due to missing lock nesting notation
> > [51328.986871]
> > [51328.993663] 4 locks held by Xorg/5379:
> > [51328.997414] #0: d8c6bdd0 (reservation_ww_class_acquire){+.+.}, at:
> > drm_ioctl_kernel+0x90/0xc8
> > [51329.006040] #1: d879e2ac (>lock){+.+.}, at:
> > etnaviv_cmdbuf_init+0x5c/0x16c [etnaviv]
> > [51329.014784] #2: d900dd88 (&(>fence_spinlock)->rlock){-.-.}, at:
> > dma_fence_signal+0x9c/0x1d4
> > [51329.023668] #3: cd12d6a4 (&(>lock)->rlock){-.-.}, at:
> > dma_fence_signal+0x9c/0x1d4
> > [51329.031854]
> > [51329.031854] stack backtrace:
> > [51329.036218] CPU: 0 PID: 5379 Comm: Xorg Not tainted 4.20.0+ #307
> > [51329.042227] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> > [51329.048772] [] (unwind_backtrace) from []
> > (show_stack+0x10/0x14)
> > [51329.056527] [] (show_stack) from []
> > (dump_stack+0x9c/0xd4)
> > [51329.063763] [] (dump_stack) from []
> > (__lock_acquire+0x1270/0x19b8)
> > [51329.071689] [] (__lock_acquire) from []
> > (lock_acquire+0xc4/0x1dc)
> > [51329.079529] [] (lock_acquire) from []
> > (_raw_spin_lock_irqsave+0x44/0x58)
> > [51329.087975] [] (_raw_spin_lock_irqsave) from []
> > (dma_fence_signal+0x9c/0x1d4)
> > [51329.096872] [] (dma_fence_signal) from []
> > (drm_sched_entity_kill_jobs_cb+0x14/0x78 [gpu_sched])
> > [51329.107327] [] (drm_sched_entity_kill_jobs_cb [gpu_sched])
> > from [] (dma_fence_signal+0xe0/0x1d4)
> > [51329.117866] [] (dma_fence_signal) from []
> > (drm_sched_process_job+0x60/0x1a4 [gpu_sched])
> > [51329.127710] [] (drm_sched_process_job [gpu_sched]) from
> > [] (dma_fence_signal+0xe0/0x1d4)
> > [51329.137569] [] (dma_fence_signal) from []
> > (irq_handler+0xc8/0x1d8 [etnaviv])
> > [51329.146389] [] (irq_handler [etnaviv]) from []
> > (__handle_irq_event_percpu+0x98/0x378)
> > [51329.155966] [] (__handle_irq_event_percpu) from []
> > (handle_irq_event_percpu+0x1c/0x58)
> > [51329.165629] [] (handle_irq_event_percpu) from []
> > (handle_irq_event+0x38/0x5c)
> > [51329.174511] [] (handle_irq_event) from []
> > (handle_fasteoi_irq+0x94/0x124)
> > [51329.183044] [] (handle_fasteoi_irq) from []
> > (generic_handle_irq+0x18/0x28)
> > [51329.191665] [] (generic_handle_irq) from []
> > (__handle_domain_irq+0x54/0xb0)
> > [51329.200374] [] (__handle_domain_irq) from []
> > (gic_handle_irq+0x48/0x98)
> > [51329.208735] [] (gic_handle_irq) from []
> > (__irq_svc+0x70/0x98)
> > [513
Re: [PATCH 0/5] tda998x updates
On Fri, Jan 25, 2019 at 09:40:38AM +, Russell King - ARM Linux admin wrote: > Hi, > > This series adds support for programming the SPD and vendor infoframes. > > It also adds support for pixel repeated modes - we were not rejecting > these modes, but we also didn't have the implementation to support > them. As their implementation is simple, add it rather than rejecting > the modes. > > Support is also added for the bridge timing information, that upstream > components may wish to use to adjust their output appropriately. > > Lastly, rather than merely passing through the full-range RGB from the > CRTC, adapt the RGB range to the capabilities of the display and the > default range for the mode. This means that if the display does not > support the Q bit in the video infoframe, and the mode is defined to > have limited range RGB, we will compress the output RGB range to > limited range. > > Tested on 4.20 with a Panasonic TV. > > drm/i2c: tda998x: add support for pixel repeated modes > drm/i2c: tda998x: add bridge timing information > drm/i2c: tda998x: add support for writing SPD > drm/i2c: tda998x: add vendor specific infoframe support > drm/i2c: tda998x: improve correctness of quantisation range > > drivers/gpu/drm/i2c/tda998x_drv.c | 120 > +- > 1 file changed, 105 insertions(+), 15 deletions(-) Hi, From what I can tell, patches 1 and 2 have not received any comments. Should I assume that these are fine to go to David? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] ASoC: simple-card: add support for bclk_ratio
On Tue, Feb 26, 2019 at 11:31:15AM -0500, Sven Van Asbroeck wrote:
> On Tue, Feb 26, 2019 at 10:45 AM Russell King - ARM Linux admin
> wrote:
> >
> >
> > I can't see how you'd extend a single I2S setup to support multi-
> > channel audio without either adding more I2S data lines or adding
> > additional WS signals (so making it e.g., a binary number).
> >
>
> That's a very good point too. In light of this, I struggle to understand how
> the ssl_ssi can specify this:
>
> static struct snd_soc_dai_driver fsl_ssi_dai_template = {
> .playback = {
> .stream_name = "CPU-Playback",
> .channels_min = 1,
> .channels_max = 32,
> },
>
> There is talk in the manual about "network mode", which could work by changing
> the LRCLK only at the first slot - thereby allowing clients to receive all
> slots just by counting, as long as they know the slot size?
>
> LRCLK _/-\_/-
> DATASLOT1|SLOT2|SLOT3|SLOT4|SLOT1
From what I gather, these are described using SND_SOC_DAIFMT_DSP_A
and SND_SOC_DAIFMT_DSP_B dai formats, and the parameters are
controlled not through snd_soc_dai_set_bclk_ratio() but via
snd_soc_dai_set_tdm_slot().
So, IMHO, the TDM formats should be disregarded from consideration
here. Mark, ack?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] ASoC: simple-card: add support for bclk_ratio
On Tue, Feb 26, 2019 at 09:53:22AM -0500, Sven Van Asbroeck wrote: > I notice that hdmi-codec.c supports up to 8 channels in hdmi multi-channel > playback mode. If we had a _theoretical_ hdmi xmitter with 8chan support, > would the bclk_ratio not be 8 x slot_size - or 8 x 32 if using an fsl_ssi > in master mode? > > This will of course never happen with the tda998x, because > .max_i2s_channels = 2, > but we are thinking about a generic solution here. The way TDA998x supports multichannel audio with I2S is as follows: "The TDA9983B supports the NXP I2S-bus format. There are four I2S-bus stereo input channels (AP1 to AP4), which enable 8 uncompressed audio channels to be carried." There is only one WS input and one SCK (bclk) input, which are common to each of the I2S buses. The TDA19988 reduces this down to two I2S buses, which means it supports only up to 4 uncompressed channels. hdmi-codec doesn't take account of these restrictions, and just assumes the maximal number of channels are always available. So, given this parallel bus architecture, it means that whether we have 2, 4, 6, or 8 channels is irrelevant to the number of bitclocks per sample - the number of bitclocks would be the same. I can't see how you'd extend a single I2S setup to support multi- channel audio without either adding more I2S data lines or adding additional WS signals (so making it e.g., a binary number). Adding more WS signals makes the bus deviate from the I2S standard, thereby making it impossible to connect a set of standard DACs to such a source, whereas adding more I2S data lines, you just connect each DAC to each I2S data line and common up the bit clock and WS signals across all. In other words, the TDA998x approach is really the only sane way forward. Now, as far as transmitter support, I believe TI Davinci SoCs use this - my Onkyo TX-NR609 AV receiver uses a DA830 SoC as a DSP to do the surround decode, which feeds multi-channel audio out to a set of DACs over a parallel I2S bus. The "mcasp" audio driver has multiple serialisers to cope with this - see Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[TWO BUGs] etnaviv crashes overnight
I'm not sure when this happened, only that it happened sometime
overnight. It was left running an Xfce desktop having only logged in,
but with the monitor disconnected. Fedora 23 plus xf86-video-armada.
I don't have any more information than that and the kernel messages
below.
[51328.909729]
[51328.915044] WARNING: possible recursive locking detected
[51328.920361] 4.20.0+ #307 Not tainted
[51328.923939]
[51328.929254] Xorg/5379 is trying to acquire lock:
[51328.933874] cd12d5e4 (&(>lock)->rlock){-.-.}, at:
dma_fence_signal+0x9c/0x1d4
[51328.941638]
[51328.941638] but task is already holding lock:
[51328.947474] cd12d6a4 (&(>lock)->rlock){-.-.}, at:
dma_fence_signal+0x9c/0x1d4
[51328.955226]
[51328.955226] other info that might help us debug this:
[51328.961756] Possible unsafe locking scenario:
[51328.961756]
[51328.967678]CPU0
[51328.970127]
[51328.976761] lock(&(>lock)->rlock);
[51328.980948]
[51328.980948] *** DEADLOCK ***
[51328.980948]
[51328.986871] May be due to missing lock nesting notation
[51328.986871]
[51328.993663] 4 locks held by Xorg/5379:
[51328.997414] #0: d8c6bdd0 (reservation_ww_class_acquire){+.+.}, at:
drm_ioctl_kernel+0x90/0xc8
[51329.006040] #1: d879e2ac (>lock){+.+.}, at:
etnaviv_cmdbuf_init+0x5c/0x16c [etnaviv]
[51329.014784] #2: d900dd88 (&(>fence_spinlock)->rlock){-.-.}, at:
dma_fence_signal+0x9c/0x1d4
[51329.023668] #3: cd12d6a4 (&(>lock)->rlock){-.-.}, at:
dma_fence_signal+0x9c/0x1d4
[51329.031854]
[51329.031854] stack backtrace:
[51329.036218] CPU: 0 PID: 5379 Comm: Xorg Not tainted 4.20.0+ #307
[51329.042227] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[51329.048772] [] (unwind_backtrace) from []
(show_stack+0x10/0x14)
[51329.056527] [] (show_stack) from []
(dump_stack+0x9c/0xd4)
[51329.063763] [] (dump_stack) from []
(__lock_acquire+0x1270/0x19b8)
[51329.071689] [] (__lock_acquire) from []
(lock_acquire+0xc4/0x1dc)
[51329.079529] [] (lock_acquire) from []
(_raw_spin_lock_irqsave+0x44/0x58)
[51329.087975] [] (_raw_spin_lock_irqsave) from []
(dma_fence_signal+0x9c/0x1d4)
[51329.096872] [] (dma_fence_signal) from []
(drm_sched_entity_kill_jobs_cb+0x14/0x78 [gpu_sched])
[51329.107327] [] (drm_sched_entity_kill_jobs_cb [gpu_sched]) from
[] (dma_fence_signal+0xe0/0x1d4)
[51329.117866] [] (dma_fence_signal) from []
(drm_sched_process_job+0x60/0x1a4 [gpu_sched])
[51329.127710] [] (drm_sched_process_job [gpu_sched]) from
[] (dma_fence_signal+0xe0/0x1d4)
[51329.137569] [] (dma_fence_signal) from []
(irq_handler+0xc8/0x1d8 [etnaviv])
[51329.146389] [] (irq_handler [etnaviv]) from []
(__handle_irq_event_percpu+0x98/0x378)
[51329.155966] [] (__handle_irq_event_percpu) from []
(handle_irq_event_percpu+0x1c/0x58)
[51329.165629] [] (handle_irq_event_percpu) from []
(handle_irq_event+0x38/0x5c)
[51329.174511] [] (handle_irq_event) from []
(handle_fasteoi_irq+0x94/0x124)
[51329.183044] [] (handle_fasteoi_irq) from []
(generic_handle_irq+0x18/0x28)
[51329.191665] [] (generic_handle_irq) from []
(__handle_domain_irq+0x54/0xb0)
[51329.200374] [] (__handle_domain_irq) from []
(gic_handle_irq+0x48/0x98)
[51329.208735] [] (gic_handle_irq) from []
(__irq_svc+0x70/0x98)
[51329.216221] Exception stack(0xd8c6bc28 to 0xd8c6bc70)
[51329.221279] bc20: 0001 0011 d8c6bc78 c2005940
d879e2ac
[51329.229462] bc40: c0c2c580 600b0013 c0b98568
c13bb508 d8c6bc78
[51329.237643] bc60: c0085804 c0087860 800b0013
[51329.242704] [] (__irq_svc) from []
(lock_acquire+0xdc/0x1dc)
[51329.250111] [] (lock_acquire) from []
(__mutex_lock+0x50/0x8b8)
[51329.25] [] (__mutex_lock) from []
(mutex_lock_nested+0x1c/0x24)
[51329.265812] [] (mutex_lock_nested) from []
(etnaviv_cmdbuf_init+0x5c/0x16c [etnaviv])
[51329.275424] [] (etnaviv_cmdbuf_init [etnaviv]) from []
(etnaviv_ioctl_gem_submit+0x61c/0x1210 [etnaviv])
[51329.286668] [] (etnaviv_ioctl_gem_submit [etnaviv]) from
[] (drm_ioctl_kernel+0x90/0xc8)
[51329.296503] [] (drm_ioctl_kernel) from []
(drm_ioctl+0x1e0/0x3b0)
[51329.304344] [] (drm_ioctl) from []
(do_vfs_ioctl+0x90/0xa48)
[51329.311750] [] (do_vfs_ioctl) from []
(ksys_ioctl+0x34/0x60)
[51329.319155] [] (ksys_ioctl) from []
(ret_fast_syscall+0x0/0x28)
[51329.326813] Exception stack(0xd8c6bfa8 to 0xd8c6bff0)
[51329.331870] bfa0: 00b0e120 be9f7b28 0009 c0306446
be9f7b28 c0306400
[51329.340053] bfc0: 00b0e120 be9f7b28 c0306446 0036 be9f7b84 be9f7b28
00e19a30 00e460c0
[51329.348234] bfe0: b6d19f18 be9f7ae4 b6d0185c b6a0be5c
[51329.353376] [ cut here ]
[51329.358004] kernel BUG at /home/rmk/git/linux-rmk/mm/vmalloc.c:1612!
[51329.364365] Internal error: Oops - BUG: 0 [#1] SMP ARM
[51329.369510] Modules linked in: ip6t_REJECT nf_reject_ipv6 ip6t_rpfilter
xt_tcpudp ipt_REJECT nf_reject_ipv4 xt_conntrack ip_set nfnetlink
Re: [RFC PATCH 1/2] ASoC: simple-card: add support for bclk_ratio
On Tue, Feb 26, 2019 at 09:35:47AM +0900, Kuninori Morimoto wrote:
> > @@ -97,7 +98,7 @@ static int asoc_simple_card_hw_params(struct
> > snd_pcm_substream *substream,
> > struct simple_card_data *priv = snd_soc_card_get_drvdata(rtd->card);
> > struct simple_dai_props *dai_props =
> > simple_priv_to_props(priv, rtd->num);
> > - unsigned int mclk, mclk_fs = 0;
> > + unsigned int mclk, bclk_ratio, mclk_fs = 0, bclk_slot_ratio = 0;
> > int ret = 0;
> >
> > if (dai_props->mclk_fs)
> (snip)
> > + if (bclk_slot_ratio) {
> > + /* FIXME do we need to care about TDM slots here ? */
> > + bclk_ratio = snd_soc_calc_frame_size(bclk_slot_ratio,
> > + params_channels(params), 1);
> > +
> > + ret = snd_soc_dai_set_bclk_ratio(codec_dai, bclk_ratio);
> > + if (ret && ret != -ENOTSUPP)
> > + goto err;
> > +
> > + ret = snd_soc_dai_set_bclk_ratio(cpu_dai, bclk_ratio);
> > + if (ret && ret != -ENOTSUPP)
> > + goto err;
> > + }
>
> Not a big deal, but "bclk_ratio" is used only here.
> We can define it here
This code actually requires a lot more thought - while it may solve
Sven's issue, it isn't generic. So far, I have this table put
together from various sources of information:
bclk_ratio
sample widthcurrent mcasp fslssi kirkwood
16 32 32 64 64
24 48 48 64 64
32 64 64 64 64
Let's also consider whether it should depend on the number of channels.
I2S uses a WS/LRCLK signal to differentiate each channel and to demark
where the MSB bit is.
If userspace negotiates one channel, what happens - if WS becomes
static, then there is no signal indicating where the MSB bit is in
the stream, so there is no way for a receiver to synchronise. So,
it is highly unlikely that bclk_ratio = channels * sample_width.
If the signal continues toggling, what does it do for the inactive
channel - is that just one BCLK period long or does it assume there
is still the second channel? If the former, it means we could end
up with bclk_ratios of 17, 25, 33 which imho is unlikely, and
would mess up TDA998x's CTS measurement.
What about setups where we have multiple I2S data signals (to support
multi-channel audio) - if we have four channels transmitted on two
data lines (as would be required by the TDA998x), that doesn't mean
BCLK becomes sample_width * 4.
So, I don't think the number of channels comes into the bclk_ratio
calculation at all. Given the above code, it effectively means we'd
always specify channels = 2 to snd_soc_calc_frame_size().
Given that it is normal to talk about I2S being clocked at "64fs",
"32fs" etc, wouldn't it just be much neater to specify _this_ value
in DT, rather than half that value?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 1/3] drm/i2c: tda998x: implement different I2S flavours
On Mon, Feb 25, 2019 at 03:28:51PM +0200, Peter Ujfalusi wrote: > hi Russell, > > On 22/02/2019 23.27, Russell King wrote: > > Add support for the left and right justified I2S formats as well as the > > more tranditional "Philips" I2S format. > > First of all, thank you for the patch, it works. > > Tested-by: Peter Ujfalusi > > There is however one thing I'm not sure about. > the 3.8 kernel configured the page0:0xfc register [1]: > /* select I2S format, and datasize */ > reg_write(encoder, REG_I2S_FORMAT, 0x0a); > > In theory this should select left_j and set bit3 which does something. > It looks like that the McASP is configured to I2S mode in 3.8 kernel > which would result channel swap at least there (I2S vs left_j). > > Do you know what the bit3 is configuring and to what? Bits 2 and 3 are something to do with "data size" which is as much information as I have on those two bits. Maybe they apply to the right justified mode as that would certainly need to know the width of the supplied I2S sample data. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: etnaviv: avoid DMA API warning when importing buffers
On Mon, Feb 25, 2019 at 10:51:30AM +, Russell King wrote:
> During boot, I get this kernel warning:
>
> WARNING: CPU: 0 PID: 19001 at kernel/dma/debug.c:1301
> debug_dma_map_sg+0x284/0x3dc
> etnaviv etnaviv: DMA-API: mapping sg segment longer than device claims to
> support [len=3145728] [max=65536]
> Modules linked in: ip6t_REJECT nf_reject_ipv6 ip6t_rpfilter xt_tcpudp
> ipt_REJECT nf_reject_ipv4 xt_conntrack ip_set nfnetlink ebtable_broute
> ebtable_nat ip6table_raw ip6table_nat nf_nat_ipv6 ip6table_mangle iptable_raw
> iptable_nat nf_nat_ipv4 nf_nat nf_conntrack nf_defrag_ipv4 nf_defrag_ipv6
> libcrc32c iptable_mangle ebtable_filter ebtables ip6table_filter ip6_tables
> iptable_filter caam_jr error snd_soc_imx_spdif imx_thermal snd_soc_imx_audmux
> nvmem_imx_ocotp snd_soc_sgtl5000
> caam imx_sdma virt_dma coda rc_cec v4l2_mem2mem snd_soc_fsl_ssi
> snd_soc_fsl_spdif imx_vdoa imx_pcm_dma videobuf2_dma_contig etnaviv
> dw_hdmi_cec gpu_sched dw_hdmi_ahb_audio imx6q_cpufreq nfsd sch_fq_codel
> ip_tables x_tables
> CPU: 0 PID: 19001 Comm: Xorg Not tainted 4.20.0+ #307
> Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [] (unwind_backtrace) from [] (show_stack+0x10/0x14)
> [] (show_stack) from [] (dump_stack+0x9c/0xd4)
> [] (dump_stack) from [] (__warn+0xf8/0x124)
> [] (__warn) from [] (warn_slowpath_fmt+0x38/0x48)
> [] (warn_slowpath_fmt) from []
> (debug_dma_map_sg+0x284/0x3dc)
> [] (debug_dma_map_sg) from []
> (drm_gem_map_dma_buf+0xc4/0x13c)
> [] (drm_gem_map_dma_buf) from []
> (dma_buf_map_attachment+0x38/0x5c)
> [] (dma_buf_map_attachment) from []
> (drm_gem_prime_import_dev+0x74/0x104)
> [] (drm_gem_prime_import_dev) from []
> (drm_gem_prime_fd_to_handle+0x84/0x17c)
> [] (drm_gem_prime_fd_to_handle) from []
> (drm_prime_fd_to_handle_ioctl+0x38/0x4c)
> [] (drm_prime_fd_to_handle_ioctl) from []
> (drm_ioctl_kernel+0x90/0xc8)
> [] (drm_ioctl_kernel) from [] (drm_ioctl+0x1e0/0x3b0)
> [] (drm_ioctl) from [] (do_vfs_ioctl+0x90/0xa48)
> [] (do_vfs_ioctl) from [] (ksys_ioctl+0x34/0x60)
> [] (ksys_ioctl) from [] (ret_fast_syscall+0x0/0x28)
> Exception stack(0xd81a9fa8 to 0xd81a9ff0)
> 9fa0: b6c69c88 bec613f8 0009 c00c642e bec613f8 b86c4600
> 9fc0: b6c69c88 bec613f8 c00c642e 0036 012762e0 01276348 0300 012d91f8
> 9fe0: b6989f18 bec613dc b697185c b667be5c
> irq event stamp: 47905
> hardirqs last enabled at (47913): [] console_unlock+0x46c/0x680
> hardirqs last disabled at (47922): [] console_unlock+0xb8/0x680
> softirqs last enabled at (47754): [] __do_softirq+0x344/0x540
> softirqs last disabled at (47701): [] irq_exit+0x124/0x144
> ---[ end trace af477747acbcc642 ]---
>
> The reason is the contiguous buffer exceeds the default maximum segment
> size of 64K as specified by dma_get_max_seg_size() in
> linux/dma-mapping.h. Fix this by providing our own segment size, which
> is set to 2GiB to cover the window found in MMUv1 GPUs.
>
> Signed-off-by: Russell King
Fixes: 78c47830a5cb ("dma-debug: check scatterlist segments")
not really that there's a problem with that commit, but it is where the
warning was introduced.
> ---
> Patch against v4.20.
>
> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 4
> drivers/gpu/drm/etnaviv/etnaviv_drv.h | 1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 4713885012ab..e414f284f424 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -527,6 +527,9 @@ static int etnaviv_bind(struct device *dev)
> }
> drm->dev_private = priv;
>
> + dev->dma_parms = >dma_parms;
> + dma_set_max_seg_size(dev, SZ_2G);
> +
> mutex_init(>gem_lock);
> INIT_LIST_HEAD(>gem_list);
> priv->num_gpus = 0;
> @@ -564,6 +567,7 @@ static void etnaviv_unbind(struct device *dev)
>
> component_unbind_all(dev, drm);
>
> + dev->dma_parms = NULL;
> drm->dev_private = NULL;
> kfree(priv);
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> index 8d02d1b7dcf5..b2930d1fe97c 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> @@ -43,6 +43,7 @@ struct etnaviv_file_private {
>
> struct etnaviv_drm_private {
> int num_gpus;
> + struct device_dma_parameters dma_parms;
> struct etnaviv_gpu *gpu[ETNA_MAX_PIPES];
>
> /* list of GEM objects: */
> --
> 2.7.4
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation
On Fri, Feb 22, 2019 at 04:18:33PM -0500, Sven Van Asbroeck wrote:
> Russell, thank you so much for your patience, help and explanation, I really
> appreciate it !
>
> Yes, that would keep the current users in business without them having to
> change anything.
>
> Of course, poor souls like myself would have to patch, say, simple-card so we
> could provide a bclk ratio in the devicetree. Which would then propagate down
> to the tda998x via hdmi-codec. Which is fine by me.
It probably would be better to try and find some generic way to deal
with this.
After all, the I2S source probably knows which ratios it supports.
Given that many sinks support a limited set of values as well, if
ASoC core knew the supported set at each end of an I2S DAI format
link, it could probably select a working bclk ratio automatically.
> Combining your two previous suggestions, I get the following. Now sample_width
> can be removed from tda998x_audio_params, as it's no longer used.
>
> Would this be a good start?
There's actually two threads of conversation going, and I recently had
a reply from the maintainer of hdmi-codec suggesting a way forward - so
I've coded that up as the three RFC patches you should have just
received.
That should allow you to merely add a snd_soc_dai_set_bclk_ratio() call
to set the bclk ratio while avoiding any breakage for existing users.
It does still contain my "FIXME" comment, so it isn't the final
solution yet. One of the down-sides to the hdmi-codec shim is that it
doesn't know which DAI formats nor which bclk ratios the hdmi-codec
it's attached to supports, which makes validation against the codec
capabilities rather awkward.
Sorry to have cut across your patch below.
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index a7c39f39793f..a306994ecc79 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -893,19 +893,25 @@ tda998x_configure_audio(struct tda998x_priv *priv,
> reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
> clksel_aip = AIP_CLKSEL_AIP_I2S;
> clksel_fs = AIP_CLKSEL_FS_ACLK;
> - switch (params->sample_width) {
> + switch (params->bclk_ratio) {
> case 16:
> + cts_n = CTS_N_M(3) | CTS_N_K(0);
> + break;
> + case 32:
> cts_n = CTS_N_M(3) | CTS_N_K(1);
> break;
> - case 18:
> - case 20:
> - case 24:
> + case 48:
> cts_n = CTS_N_M(3) | CTS_N_K(2);
> break;
> - default:
> - case 32:
> + case 64:
> cts_n = CTS_N_M(3) | CTS_N_K(3);
> break;
> + case 128:
> + cts_n = CTS_N_M(0) | CTS_N_K(0);
> + break;
> + default:
> + dev_err(>hdmi->dev, "unsupported I2S bclk
> ratio\n");
> + return -EINVAL;
> }
> break;
>
> @@ -982,7 +988,7 @@ static int tda998x_audio_hw_params(struct device *dev,
> void *data,
> struct tda998x_priv *priv = dev_get_drvdata(dev);
> int i, ret;
> struct tda998x_audio_params audio = {
> - .sample_width = params->sample_width,
> + .bclk_ratio = daifmt->bclk_ratio,
> .sample_rate = params->sample_rate,
> .cea = params->cea,
> };
> @@ -1004,6 +1010,23 @@ static int tda998x_audio_hw_params(struct device *dev,
> void *data,
> if (priv->audio_port[i].format == AFMT_I2S)
> audio.config = priv->audio_port[i].config;
> audio.format = AFMT_I2S;
> + if (!audio.bclk_ratio) {
> + /* compatibility */
> + switch (params->sample_width) {
> + case 16:
> + audio.bclk_ratio = 32;
> + break;
> + case 18:
> + case 20:
> + case 24:
> + audio.bclk_ratio = 48;
> + break;
> + default:
> + case 32:
> + audio.bclk_ratio = 64;
> + break;
> + }
> + }
> break;
> case HDMI_SPDIF:
> for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
> diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
> index 3cb25ccbe5e6..039e1d9af2e0 100644
> --- a/include/drm/i2c/tda998x.h
> +++ b/include/drm/i2c/tda998x.h
> @@ -14,7 +14,7 @@ enum {
> struct tda998x_audio_params {
> u8 config;
> u8 format;
> - unsigned sample_width;
> + u8 bclk_ratio;
> unsigned sample_rate;
>
[PATCH RFC 0/3] tda998x updates for DAI formats and bclk_ratio
This series addresses two issues with TDA998x that have been identified: 1) Peter found that the I2S format was not being explicitly set, and retains its value from whatever was previously running on the platform. Work around this by implementing support for setting the I2S format from the DAI format, rather than merely defaulting the register back to its power-on value. 2) Sven found that TDA998x does not work on his Freescale platform, which always uses a 64·fs bitclock. The TDA998x driver was deriving this information from the sample width, which, while it works for Beagle Bone Black, does not allow the driver to be used with other I2S sources that may have different behaviours. To work around that, we implement support for snd_soc_dai_set_bclk_ratio() in hdmi-codec, and propagate its value to TDA998x and other HDMI codecs via a new member. However, since snd_soc_dai_set_bclk_ratio() is never called, we need to avoid breaking any existing users, so we detect the lack of call by an impossible zero value, and subsitute a value corresponding with the TDA998x's old behaviour. It is hoped that snd_soc_dai_set_bclk_ratio() will see more adoption in ASoC, and the TDA998x specific defaulting can be removed. drivers/gpu/drm/i2c/tda998x_drv.c | 75 ++- include/drm/i2c/tda998x.h | 12 +-- include/sound/hdmi-codec.h| 1 + sound/soc/codecs/hdmi-codec.c | 45 +-- 4 files changed, 104 insertions(+), 29 deletions(-) -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation
On Fri, Feb 22, 2019 at 04:27:35PM +, Russell King - ARM Linux admin wrote:
> On Fri, Feb 22, 2019 at 10:47:35AM -0500, Sven Van Asbroeck wrote:
> > The config structure which you need to fill in to init the audio has a
> > "i2s qualifier" field, where you have the choice between 16 and 32 bits.
> > This then maps to a "Clock Time Stamp factor x" called CTSX, which maps to
> > the following CTS_N register settings:
> >
> > CTSX -> CTS_N (m,k)
> > ---
> > 16 -> (3,0)
> > 32 -> (3,1) (i2s qualifier = 16 bits)
> > 48 -> (3,2)
> > 64 -> (3,3) (i2s qualifier = 32 bits)
> > 128 -> (0,0)
Okay, this is my hypothesis about how the TDA998x works:
In the HDMI spec, the CTS value is generated using:
128*fS divide ---> CTS counter -> CTS value
(clock)^ ^
| |
fTMDS_clock -+> TMDS clock
|
N value ---+--> N value
What this does is count the number of TMDS clocks for every output of
the divider to produce a value for CTS, effectively implementing
CTS = fTMDS_clock·N / 128·fS_source
The sink regenerates the 128·fS clock at the sink by reversing the
process - this is the equation given in the HDMI spec:
128·fS_sink = fTMDS_clock·N / CTS
Using the "actual" values for 'm' and 'k' rather than the register
values, if we subsitute BCLK for the 128·fS input, assume that instead
of a CTS counter it is the mts counter which has to be divided by 'm'
to get the CTS value, and take account of 'k' as an extra prescaler,
what we end up with is:
mts = fTMDS_clock·N·k / BCLK
CTS = fTMDS_clock·N·k / (BCLK·m)
Throw this into the reverse process, and we end up with:
128·fS_sink = BCLK·m / k
From the table of values you've given above, the CTSX value looks very
much like the BCLK fS ratio. For the 64·fS ratio, we have m=8 (reg
value 3) k=4 (reg value 3):
BCLK ratio m k 128·fS_sink is in terms of fS_source
16 8 1 BCLK·8 128·fS_source
32 8 2 BCLK·4 128·fS_source
48 8 3 BCLK·8/3128·fS_source
64 8 4 BCLK·2 128·fS_source
128 1 1 BCLK128·fS_source
What this shows is that we end up with the same sample rate on the sink
as the source despite the different BCLK ratios with this assumption.
What we also know is that SPDIF uses a 64·fS clock, and is programmed
with m=8 k=4, which corresponds nicely with the above.
In light of that, what about this, which rejigs the driver to use a
bclk_ratio rather than the sample width. We then just need to work out
what to do about getting the bclk_ratio value into the driver in a way
that we don't end up breaking existing users.
A possible solution to that would be for hdmi-codec to default that to
zero unless it's been definitively provided by the ASoC "card", which
would allow the old behaviour of selecting the CTS_N M/K values
depending on the sample width, which we know works for some people.
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 9300469dbec0..3d5eb5024b2c 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -930,21 +930,26 @@ tda998x_configure_audio(struct tda998x_priv *priv,
reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
clksel_aip = AIP_CLKSEL_AIP_I2S;
clksel_fs = AIP_CLKSEL_FS_ACLK;
- switch (params->sample_width) {
+ switch (params->bclk_ratio) {
case 16:
+ cts_n = CTS_N_M(3) | CTS_N_K(0);
+ break;
+ case 32:
cts_n = CTS_N_M(3) | CTS_N_K(1);
break;
- case 18:
- case 20:
- case 24:
+ case 48:
cts_n = CTS_N_M(3) | CTS_N_K(2);
break;
- default:
- case 32:
+ case 64:
cts_n = CTS_N_M(3) | CTS_N_K(3);
break;
+ case 128:
+ cts_n = CTS_N_M(0) | CTS_N_K(0);
+ break;
+ default:
+ dev_err(>hdmi->dev, "unsupported I2S bclk
ratio\n");
+ return -EINVAL;
}
-
switch (params->format & AFMT_I2S_MASK) {
case AFMT_I2S_LEFT_J:
i2s_fmt = I2S_FORMAT_LEFT_J;
@@ -1040,6 +1045,22 @@ static int tda998x_audio_hw_params(struct device *dev,
void *data,
memcpy(audio.status, pa
Re: [RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default
On Fri, Feb 22, 2019 at 03:27:43PM +, Russell King - ARM Linux admin wrote: > On Fri, Feb 22, 2019 at 05:20:15PM +0200, Peter Ujfalusi wrote: > > Hi Russell, > > > > On 22/02/2019 16.35, Russell King - ARM Linux admin wrote: > > > It would also be good to know what Fs value(s) BBB uses, and what > > > sample sizes you have tested. > > > > On BBB McASP is the clock master and as I recall I have tested 44.1, 48 > > KHz at least with 16 and 24 bits. > > Sorry, I wasn't clear enough... is the bus clocked at 32Fs for 16bit > samples and 64Fs for 24bit samples, or is it 64Fs for both? To be clear, the reason I'm asking for this information is that Sven Van Asbroeck is trying to use TDA998x, and he has a problem with the current implementation that adjusts CTS_N_M and CTS_N_K parameters according to the _sample_ size. This appears to be wrong, these settings should be set according to the BCLK ratio (Fs multiplier) and not the sample size. If you say that the existing code works for you, but your device always produces a bclk at 64fs, then we have a problem - it means that to add support for Sven's platform, we're probably going to end up causing a regression for your platform. The next issue is with snd_soc_dai_set_bclk_ratio(). Today, no one calls that function, so none of the DAI .set_bclk_ratio implementations are used (and probably completely untested.) If your CPU DAI changes the bclk ratio depending on the sample size, I will need something to call that function at the appropriate time to set the clocking ratio. I suspect most codecs don't care about it, but TDA998x does, because it _looks_ like it uses the BCLK to generate the CTS value to be passed to the HDMI sink. Since CTS = f(tmds) * N / (128 * fs), using BCLK to derive the CTS value requires TDA998x to know the BCLK ratio. So, knowing this information ahead of time is very advantageous. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation
(Adding Mark, ASoC maintainer.)
On Fri, Feb 22, 2019 at 10:47:35AM -0500, Sven Van Asbroeck wrote:
> On Fri, Feb 22, 2019 at 8:21 AM Russell King - ARM Linux admin
> wrote:
> >
> > On Thu, Feb 21, 2019 at 01:18:13PM -0500, Sven Van Asbroeck wrote:
> >
> > > [SNDRV_PCM_FORMAT_S24_LE] = {
> > > .width = 24, .phys = 32, .le = 1, .signd = 1,
> > > .silence = {},
> > > },
> >
> > The above table describes the memory format, not the wire format.
> > Look further down for SNDRV_PCM_FORMAT_S24_3LE, which is 24-bit
> > packed into three bytes (see include/uapi/sound/asound.h for
> > the comment specifying that.)
> >
> > ASoC uses DAIFMT to specify the on-wire format in connection with
> > the above.
> >
>
> Interesting ! So you're saying that currently, nobody strictly defines the
> layout of the on-wire format, correct? I'm not sure how this works in
> practice,
> because codec and cpu dai should agree on the on-wire format? Except if the
> formats used have enough flexibility so you don't have to care.
SNDRV_PCM_FORMAT_xxx more defines the in-memory format, rather than
the on-wire format.
As I've said, the on-wire format is defined in ASoC using a completely
different mechanism, using the definitions in include/sound/soc-dai.h.
This describes parameters such as the polarity of clocks on the i2s
bus, the justification of the data, etc.
Bear in mind that SNDRV_PCM_FORMAT_S24_LE in memory may be right
justified (using the least significant 3 bytes of every 32-bit word),
but on the wire may be left justified - using the most significant
bits.
SND_SOC_DAIFMT_I2S defines the format to be "Philips" format, where
the MSB bit is sent one BCLK _after_ the LRCLK signal changes state.
There is also SND_SOC_DAIFMT_LEFT_J, where the MSB bit is sent with
no delay, and extra padding zeros are sent in the "LSB" bits.
SND_SOC_DAIFMT_RIGHT_J is similar, but the padding is in the "MSB"
bits.
Then there's the polarity of the BCLK and LRCLK (frame) signals.
Finally, there's whether the codec (TDA998x in this case) is the
origin of the LRCLK and/or BCLK.
This information is passed via a call to snd_soc_dai_set_fmt()
which takes the DAI and the format - this calls into hdmi-codec.c
hdmi_codec_set_fmt(). This will be handled by the core ASoC code
if the DAI has a .dai_fmt member set (which can be set by DT -
see snd_soc_of_parse_daifmt().)
Then there is snd_soc_dai_set_bclk_ratio() which sets the BCLK
to sample-rate ratio, as I explained earlier. hdmi-codec doesn't
have an implementation for this, and afaics no one calls this
function. So, it seems assumptions are made throughout ASoC
on that point (probably because most codecs don't care.)
> > This doesn't really help in terms of working out what the correct
> > settings should be, and other information I have laying around does not
> > provide any further enlightenment.
>
> I have access to the NXP software library shipped with the tda19988.
Yes, I'm aware of it.
> The library's release notes have the following entry:
>
> . "I2S audio does not work, CTS value is not good"
> Check the audio I2S format
> CTS is automatically computed by the TDA accordingly to the audio input
> so accordingly to the upstream settings (like an OMAP ;)
> For example, I2S 16 bits or 32 bits do not produce the same CTS value
>
> The config structure which you need to fill in to init the audio has a
> "i2s qualifier" field, where you have the choice between 16 and 32 bits.
> This then maps to a "Clock Time Stamp factor x" called CTSX, which maps to
> the following CTS_N register settings:
>
> CTSX -> CTS_N (m,k)
> ---
> 16 -> (3,0)
> 32 -> (3,1) (i2s qualifier = 16 bits)
> 48 -> (3,2)
> 64 -> (3,3) (i2s qualifier = 32 bits)
> 128 -> (0,0)
>
> Does this information bring us any closer to our assumption that CTS_N needs
> to be calculated off the bclk to sample rate ratio ?
I'm aware of other users of TDA998x, and I'm attempting to get out of
them what ratios their implementations use - they've said that they
have confirmed that 16bit and 24bit works for them, but that's rather
incomplete in terms of what I wanted to know... waiting for another
response!
> I'd love to take a shot at this, but first I'd like to understand what you're
> suggesting :)
>
> Currently there is set_bclk_ratio() support, but no-one is actually using it.
> If hdmi-codec is to retrieve the ratio, wouldn't we need to add .GET_blk_ratio
> to snd_soc_dai_ops ?
>
> I could add this to fsl_ssi in master mode, but what if somebody connects the
> tda to a cpu dai for which no-one implemented .GET_bclk_r
Re: [RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default
On Fri, Feb 22, 2019 at 05:20:15PM +0200, Peter Ujfalusi wrote:
> > and I think we should implement at least setting the I2S
> > register format from the hdmi_codec_daifmt data.
>
> Yes, that needs to be done for sure, but without data sheet with
> register descriptions I would not attempt to do that.
How about this, although it's not particularly pretty.
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index a7c39f39793f..c53128bb40fd 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -242,7 +242,9 @@ struct tda998x_priv {
# define HVF_CNTRL_1_SEMI_PLANAR (1 << 6)
#define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */
#define REG_I2S_FORMATREG(0x00, 0xfc) /* read/write */
-# define I2S_FORMAT(x)(((x) & 3) << 0)
+# define I2S_FORMAT_PHILIPS (0 << 0)
+# define I2S_FORMAT_LEFT_J(2 << 0)
+# define I2S_FORMAT_RIGHT_J (3 << 0)
#define REG_AIP_CLKSELREG(0x00, 0xfd) /* write */
# define AIP_CLKSEL_AIP_SPDIF(0 << 3)
# define AIP_CLKSEL_AIP_I2S (1 << 3)
@@ -872,14 +874,14 @@ static int
tda998x_configure_audio(struct tda998x_priv *priv,
struct tda998x_audio_params *params)
{
- u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv;
+ u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv, i2s_fmt;
u32 n;
/* Enable audio ports */
reg_write(priv, REG_ENA_AP, params->config);
/* Set audio input source */
- switch (params->format) {
+ switch (params->format & AFMT_MASK) {
case AFMT_SPDIF:
reg_write(priv, REG_ENA_ACLK, 0);
reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF);
@@ -907,6 +909,19 @@ tda998x_configure_audio(struct tda998x_priv *priv,
cts_n = CTS_N_M(3) | CTS_N_K(3);
break;
}
+
+ switch (params->format & AFMT_I2S_MASK) {
+ case AFMT_I2S_LEFT_J:
+ i2s_fmt = I2S_FORMAT_LEFT_J;
+ break;
+ case AFMT_I2S_RIGHT_J:
+ i2s_fmt = I2S_FORMAT_RIGHT_J;
+ break;
+ default:
+ i2s_fmt = I2S_FORMAT_PHILIPS;
+ break;
+ }
+ reg_write(priv, REG_I2S_FORMAT, i2s_fmt);
break;
default:
@@ -992,23 +1007,15 @@ static int tda998x_audio_hw_params(struct device *dev,
void *data,
switch (daifmt->fmt) {
case HDMI_I2S:
- if (daifmt->bit_clk_inv || daifmt->frame_clk_inv ||
- daifmt->bit_clk_master || daifmt->frame_clk_master) {
- dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__,
- daifmt->bit_clk_inv, daifmt->frame_clk_inv,
- daifmt->bit_clk_master,
- daifmt->frame_clk_master);
- return -EINVAL;
- }
- for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
- if (priv->audio_port[i].format == AFMT_I2S)
- audio.config = priv->audio_port[i].config;
- audio.format = AFMT_I2S;
+ audio.format = AFMT_I2S | AFMT_I2S_PHILIPS;
+ break;
+ case HDMI_LEFT_J:
+ audio.format = AFMT_I2S | AFMT_I2S_LEFT_J;
+ break;
+ case HDMI_RIGHT_J:
+ audio.format = AFMT_I2S | AFMT_I2S_RIGHT_J;
break;
case HDMI_SPDIF:
- for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
- if (priv->audio_port[i].format == AFMT_SPDIF)
- audio.config = priv->audio_port[i].config;
audio.format = AFMT_SPDIF;
break;
default:
@@ -1016,11 +1023,25 @@ static int tda998x_audio_hw_params(struct device *dev,
void *data,
return -EINVAL;
}
+ for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
+ if (priv->audio_port[i].format == (audio.format & AFMT_MASK))
+ audio.config = priv->audio_port[i].config;
+
if (audio.config == 0) {
dev_err(dev, "%s: No audio configuration found\n", __func__);
return -EINVAL;
}
+ if ((audio.format & AFMT_MASK) == HDMI_I2S &&
+ (daifmt->bit_clk_inv || daifmt->frame_clk_inv ||
+daifmt->bit_clk_master || daifmt->frame_clk_master)) {
+ dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__,
+ daifmt->bit_clk_inv, daifmt->frame_clk_inv,
+ daifmt->bit_clk_master,
+ daifmt->frame_clk_master);
+ return -EINVAL;
+ }
+
mutex_lock(>audio_mutex);
if (priv->supports_infoframes &&
Re: [RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default
On Fri, Feb 22, 2019 at 05:20:15PM +0200, Peter Ujfalusi wrote: > Hi Russell, > > On 22/02/2019 16.35, Russell King - ARM Linux admin wrote: > > On Fri, Feb 22, 2019 at 03:47:14PM +0200, Peter Ujfalusi wrote: > >> Hi, > >> > >> the original version was sent 14.04.2018: > >> https://patchwork.kernel.org/patch/10344403/ > >> > >> Changes since then: > >> - rebased on currentl drm/next > >> > >> The reset value of the register is 0, the soft reset does not reset this > >> register and if other kernel changed this the audio is going to be > >> distorted. > >> > >> It was observed when - accidentally - booted the kernel from eMMC on BBB > >> which is 3.8.13-bone79 and it sets this register to 0x0a. After reboot and > >> tda998x_reset() it remains 0x0a. > > > > Have you checked whether the input I2S stream is Philips or Left > > Justified? This is controlled by the LSB two bits. > > The am335x-boneblack-common.dtsi configures the link to i2s, which > corresponds to Philips format (the default > > > > > It appears that 3.8.13-bone79 configures the TDA998x for left- > > justified, whereas re-setting these two bits to zero will configure > > it for Philips. > > The chip reset value for the register is 0 and software reset will not > reset it if it was modified. So I wonder why 3.8.13-bone79 configures it for left-justified. > > Bits 3:2 control the data size, but I have no information what their > > values correspond to. > > I can not find the register descriptions, can not tell what are the bits > in there. > > > Can we nail down what is required for BBB and what it doesn't care > > about. > > atm the REG_I2S_FORMAT register needs to be reset to 0. > > > and I think we should implement at least setting the I2S > > register format from the hdmi_codec_daifmt data. > > Yes, that needs to be done for sure, but without data sheet with > register descriptions I would not attempt to do that. > > > It would also be good to know what Fs value(s) BBB uses, and what > > sample sizes you have tested. > > On BBB McASP is the clock master and as I recall I have tested 44.1, 48 > KHz at least with 16 and 24 bits. Sorry, I wasn't clear enough... is the bus clocked at 32Fs for 16bit samples and 64Fs for 24bit samples, or is it 64Fs for both? > >> Signed-off-by: Peter Ujfalusi > >> --- > >> drivers/gpu/drm/i2c/tda998x_drv.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c > >> b/drivers/gpu/drm/i2c/tda998x_drv.c > >> index 7f34601bb515..72f93802d209 100644 > >> --- a/drivers/gpu/drm/i2c/tda998x_drv.c > >> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > >> @@ -722,6 +722,9 @@ tda998x_reset(struct tda998x_priv *priv) > >> > >>/* Write the default value MUX register */ > >>reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24); > >> + > >> + /* Write the default to I2S_FORMAT register */ > >> + reg_write(priv, REG_I2S_FORMAT, 0x00); > >> } > >> > >> /* > >> -- > >> Peter > >> > >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > >> > >> > > > > - Péter > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default
On Fri, Feb 22, 2019 at 03:47:14PM +0200, Peter Ujfalusi wrote: > Hi, > > the original version was sent 14.04.2018: > https://patchwork.kernel.org/patch/10344403/ > > Changes since then: > - rebased on currentl drm/next > > The reset value of the register is 0, the soft reset does not reset this > register and if other kernel changed this the audio is going to be > distorted. > > It was observed when - accidentally - booted the kernel from eMMC on BBB > which is 3.8.13-bone79 and it sets this register to 0x0a. After reboot and > tda998x_reset() it remains 0x0a. Have you checked whether the input I2S stream is Philips or Left Justified? This is controlled by the LSB two bits. It appears that 3.8.13-bone79 configures the TDA998x for left- justified, whereas re-setting these two bits to zero will configure it for Philips. Bits 3:2 control the data size, but I have no information what their values correspond to. Can we nail down what is required for BBB and what it doesn't care about - and I think we should implement at least setting the I2S register format from the hdmi_codec_daifmt data. It would also be good to know what Fs value(s) BBB uses, and what sample sizes you have tested. > > Signed-off-by: Peter Ujfalusi > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c > b/drivers/gpu/drm/i2c/tda998x_drv.c > index 7f34601bb515..72f93802d209 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -722,6 +722,9 @@ tda998x_reset(struct tda998x_priv *priv) > > /* Write the default value MUX register */ > reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24); > + > + /* Write the default to I2S_FORMAT register */ > + reg_write(priv, REG_I2S_FORMAT, 0x00); > } > > /* > -- > Peter > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation
On Thu, Feb 21, 2019 at 01:18:13PM -0500, Sven Van Asbroeck wrote:
> My cpu dai driving the tda998x in master mode outputs
> SNDRV_PCM_FORMAT_S24_LE, i2s left justified, two channels:
>
> [SNDRV_PCM_FORMAT_S24_LE] = {
> .width = 24, .phys = 32, .le = 1, .signd = 1,
> .silence = {},
> },
>
> This format has a sample width of 24 bits, but a physical
> size of 32 bits per channel. The redundant bits are padded
> with zeros. This gives a total frame width of 64 bits.
The above table describes the memory format, not the wire format.
Look further down for SNDRV_PCM_FORMAT_S24_3LE, which is 24-bit
packed into three bytes (see include/uapi/sound/asound.h for
the comment specifying that.)
ASoC uses DAIFMT to specify the on-wire format in connection with
the above.
However, having 32-clocks per sample is quite normal with I2S.
> According to the tda19988 datasheet, this format is supported:
>
> "Audio samples with a precision better than 24-bit are
> truncated to 24-bit. [...] If the input clock has a
> frequency of 64fs and is left justified or Philips,
> the audio word is truncated to 24-bit format and other
> bits padded with zeros."
> (tda19988 product datasheet Rev. 3 - 21 July 2011)
>
> However, supplying this format to the chip results in distorted
> audio. I noticed that the audio problem disappears when the
> CTS_N pre-divider is calculated from the physical width, _not_
> the sample width.
It is not correct to use the physical width - as can be seen from the
table, if you check the 3-byte memory format, you'll see that it has
a physical width of 24. That doesn't mean that the bus has 24 clocks
per sample.
Looking at kirkwood-i2s, which is one of the setups that the I2S mode
was apparently originally tested (not by me), it seems to do the same
thing as the FSL SSI - 32 clocks per sample with BCLK being 64Fs -
there is a comment "I2S always uses 32 bits per channel" which implies
64Fs.
When I2S mode patches appeared, I did wonder about that code which
depends on the sample width rather than the Fs value, but I assumed
that it had been tested with all different formats, etc. Maybe that
was a false assumption to make.
As it is possible to have 16-bit samples at 64Fs or 32Fs, and if the
TDA998x is counting BCLKs, knowing the Fs value is necessary to know
how to program the TDA998x to generate the CTS value.
Now, ASoC has a bunch of functions that allows the wire format to be
controlled. E.g., snd_soc_dai_set_fmt() sets which end provides the
master clock, the polarity of the clocks, whether the samples are left
or right justified. There is also snd_soc_dai_set_bclk_ratio(), which
is documented as:
/**
* snd_soc_dai_set_bclk_ratio - configure BCLK to sample rate ratio.
* @dai: DAI
* @ratio: Ratio of BCLK to Sample rate.
*
* Configures the DAI for a preset BCLK to sample rate ratio.
*/
which would have ratio=64 for 64Fs. The problem is, though, that no
one appears to call this function, and fewer implement the method
(hdmi-codec being one of those which does not.)
> This patch adjusts the CTS_N calculation so that the audio
> distortion goes away.
>
> Caveats:
> - I am only capable of generating audio with a frame width of
> 64fs. So I cannot test if 32fs or 48fs audio formats still
> work. Such as S16_LE or S20_LE.
> - I have no access to a datasheet which accurately describes
> the CTS_N register. So I cannot check if my assumption is
> correct.
Don't think that any of the information that is available is much
better! There's a few register descriptions but nothing that really
describes how all the various register settings hang together.
For example, the CTS_N register M and K values are:
M select: postdivider mts (measured time stamp)
0 CTS = mts
1 CTS = mts / 2
2 CTS = mts / 4
3 CTS = mts / 8
K select: predivider (scales n)
0 k=1
1 k=2
2 k=3
3 k=4
4+k=8
Then there's the SEL_FS field in the AIP_CLKSEL register:
select fs: CTS reference
which is surely the mts reference, if CTS is derived from mts.
This doesn't really help in terms of working out what the correct
settings should be, and other information I have laying around does not
provide any further enlightenment.
I think what I'd like to see is passing of the Fs value into the driver
from hdmi-codec, but I suspect that requires a bit of work in multiple
drivers.
>
> Tested with an imx6 ssi.
>
> Cc: Peter Rosin
> Signed-off-by: Sven Van Asbroeck
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 4 ++--
> include/drm/i2c/tda998x.h | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index a7c39f39793f..ba9f55176e1b 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -893,7 +893,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
> reg_write(priv, REG_MUX_AP,
Re: [PATCH 2/4] components: multiple components for a device
On Fri, Feb 08, 2019 at 12:27:57AM +0100, Daniel Vetter wrote:
> Component framework is extended to support multiple components for
> a struct device. These will be matched with different masters based on
> its sub component value.
>
> We are introducing this, as I915 needs two different components
> with different subcomponent value, which will be matched to two
> different component masters(Audio and HDCP) based on the subcomponent
> values.
>
> v2: Add documenation.
>
> v3: Rebase on top of updated documenation.
>
> v4: Review from Rafael:
> - Remove redundant "This" from kerneldoc (also in the previous patch)
> - Streamline the logic in find_component() a bit.
>
> Signed-off-by: Daniel Vetter (v1 code)
> Signed-off-by: Ramalingam C (v1 commit message)
> Cc: Ramalingam C
> Cc: Greg Kroah-Hartman
> Cc: Russell King
> Cc: Rafael J. Wysocki
> Cc: Jaroslav Kysela
> Cc: Takashi Iwai
> Cc: Rodrigo Vivi
> Cc: Jani Nikula
> Signed-off-by: Daniel Vetter
> ---
> drivers/base/component.c | 158 +-
> include/linux/component.h | 10 ++-
> 2 files changed, 129 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 1624c2a892a5..7dbc41cccd58 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -47,6 +47,7 @@ struct component;
> struct component_match_array {
> void *data;
> int (*compare)(struct device *, void *);
> + int (*compare_typed)(struct device *, int, void *);
> void (*release)(struct device *, void *);
> struct component *component;
> bool duplicate;
> @@ -74,6 +75,7 @@ struct component {
> bool bound;
>
> const struct component_ops *ops;
> + int subcomponent;
> struct device *dev;
> };
>
> @@ -158,7 +160,7 @@ static struct master *__master_find(struct device *dev,
> }
>
> static struct component *find_component(struct master *master,
> - int (*compare)(struct device *, void *), void *compare_data)
> + struct component_match_array *mc)
> {
> struct component *c;
>
> @@ -166,7 +168,11 @@ static struct component *find_component(struct master
> *master,
> if (c->master && c->master != master)
> continue;
>
> - if (compare(c->dev, compare_data))
> + if (mc->compare && mc->compare(c->dev, mc->data))
> + return c;
> +
> + if (mc->compare_typed &&
> + mc->compare_typed(c->dev, c->subcomponent, mc->data))
> return c;
> }
>
> @@ -192,7 +198,7 @@ static int find_components(struct master *master)
> if (match->compare[i].component)
> continue;
>
> - c = find_component(master, mc->compare, mc->data);
> + c = find_component(master, mc);
> if (!c) {
> ret = -ENXIO;
> break;
> @@ -327,29 +333,12 @@ static int component_match_realloc(struct device *dev,
> return 0;
> }
>
> -/**
> - * component_match_add_release - add a component match with release callback
> - * @master: device with the aggregate driver
> - * @matchptr: pointer to the list of component matches
> - * @release: release function for @compare_data
> - * @compare: compare function to match against all components
> - * @compare_data: opaque pointer passed to the @compare function
> - *
> - * Adds a new component match to the list stored in @matchptr, which the
> @master
> - * aggregate driver needs to function. The list of component matches pointed
> to
> - * by @matchptr must be initialized to NULL before adding the first match.
> - *
> - * The allocated match list in @matchptr is automatically released using devm
> - * actions, where upon @release will be called to free any references held by
> - * @compare_data, e.g. when @compare_data is a _node that must be
> - * released with of_node_put().
> - *
> - * See also component_match_add().
> - */
> -void component_match_add_release(struct device *master,
> +static void __component_match_add(struct device *master,
> struct component_match **matchptr,
> void (*release)(struct device *, void *),
> - int (*compare)(struct device *, void *), void *compare_data)
> + int (*compare)(struct device *, void *),
> + int (*compare_typed)(struct device *, int, void *),
> + void *compare_data)
> {
> struct component_match *match = *matchptr;
>
> @@ -381,13 +370,69 @@ void component_match_add_release(struct device *master,
> }
>
> match->compare[match->num].compare = compare;
> + match->compare[match->num].compare_typed = compare_typed;
> match->compare[match->num].release = release;
> match->compare[match->num].data = compare_data;
> match->compare[match->num].component = NULL;
> match->num++;
> }
> +
> +/**
> + * component_match_add_release - add a component match with
Re: [PATCH] component: Add documentation
On Tue, Feb 05, 2019 at 05:21:07PM +0100, Daniel Vetter wrote: > Someone owes me a beer ... I find that deeply offensive - it is clearly directed at me personally as author of the component helper. There are double-standards in the kernel ecosystem with respect to documentation - there are entire subsystems way more complicated than the component *helper* which are lacking in documentation, and the subsystem authors response to requests to change that basically get ignored, or the response is "write the documentation yourself". Why does there seem to be one rule for me and one rule for everyone else? Please remove this line. > > While typing these I think doing an s/component_master/aggregate/ > would be useful: > - it's shorter :-) > - I think component/aggregate is much more meaningful naming than > component/puppetmaster or something like that. At least to my > English ear "aggregate" emphasizes much more the "assemble a pile of > things into something bigger" aspect, and there's not really much > of a control hierarchy between aggregate and constituing components. > > But that's way more than a quick doc typing exercise ... > > Thanks to Ram for commenting on an initial draft of these docs. > > v2: Review from Rafael: > - git add Documenation/driver-api/component.rst > - lots of polish to the wording + spelling fixes. > > Cc: "C, Ramalingam" > Cc: Greg Kroah-Hartman > Cc: Russell King > Cc: Rafael J. Wysocki > Cc: Jaroslav Kysela > Cc: Takashi Iwai > Cc: Rodrigo Vivi > Cc: Jani Nikula > Signed-off-by: Daniel Vetter > --- > Documentation/driver-api/component.rst | 17 > Documentation/driver-api/device_link.rst | 3 + > Documentation/driver-api/index.rst | 1 + > drivers/base/component.c | 107 ++- > include/linux/component.h| 70 +++ > 5 files changed, 195 insertions(+), 3 deletions(-) > create mode 100644 Documentation/driver-api/component.rst > > diff --git a/Documentation/driver-api/component.rst > b/Documentation/driver-api/component.rst > new file mode 100644 > index ..3407ff0424b9 > --- /dev/null > +++ b/Documentation/driver-api/component.rst > @@ -0,0 +1,17 @@ > += > +Component Framework for Aggregate Drivers > += > + > +.. kernel-doc:: drivers/base/component.c > + :doc: overview > + > + > +API > +=== > + > +.. kernel-doc:: include/linux/component.h > + :internal: > + > +.. kernel-doc:: drivers/base/component.c > + :export: > + > diff --git a/Documentation/driver-api/device_link.rst > b/Documentation/driver-api/device_link.rst > index d6763272e747..2d5919b2b337 100644 > --- a/Documentation/driver-api/device_link.rst > +++ b/Documentation/driver-api/device_link.rst > @@ -1,6 +1,9 @@ > .. |struct dev_pm_domain| replace:: :c:type:`struct dev_pm_domain > ` > .. |struct generic_pm_domain| replace:: :c:type:`struct generic_pm_domain > ` > > + > +.. _device_link: > + > > Device links > > diff --git a/Documentation/driver-api/index.rst > b/Documentation/driver-api/index.rst > index ab38ced66a44..c0b600ed9961 100644 > --- a/Documentation/driver-api/index.rst > +++ b/Documentation/driver-api/index.rst > @@ -22,6 +22,7 @@ available subsections can be seen below. > device_connection > dma-buf > device_link > + component > message-based > sound > frame-buffer > diff --git a/drivers/base/component.c b/drivers/base/component.c > index ddcea8739c12..4851e1006f11 100644 > --- a/drivers/base/component.c > +++ b/drivers/base/component.c > @@ -16,6 +16,33 @@ > #include > #include > > +/** > + * DOC: overview > + * > + * The component framework allows drivers to collect a pile of sub-devices, Helper. > + * including their bound drivers, into an aggregate driver. Various > subsystems > + * already provide functions to get hold of such components, e.g. > + * of_clk_get_by_name(). The component framework can be used when such a helper > + * subsystem-specific way to find a device is not available: The component > + * framework fills the niche of aggregate drivers for specific hardware, > where helper > + * further standardization into a subsystem would not be practical. The > common > + * example is when a logical device (e.g. a DRM display driver) is spread > around > + * the SoC on various component (scanout engines, blending blocks, > transcoders > + * for various outputs and so on). > + * > + * The component framework also doesn't solve runtime dependencies, e.g. for helper > + * system suspend and resume operations. See also :ref:`device > + * links`. > + * > + * Components are registered using component_add() and unregistered with > + * component_del(), usually from the driver's probe and disconnect functions. > + * > + * Aggregate drivers first assemble a component match list of what they need > + * using
Re: [PATCH 5/5] drm/i2c: tda998x: improve correctness of quantisation range
On Wed, Jan 30, 2019 at 03:53:04PM +, Brian Starkey wrote:
> Hi Russell,
>
> On Fri, Jan 25, 2019 at 09:43:29AM +, Russell King wrote:
> > CEA-861 says: "A Source shall not send a non-zero Q value that does
> > not correspond to the default RGB Quantization Range for the
> > transmitted Picture unless the Sink indicates support for the Q bit
> > in a Video Capabilities Data Block."
> >
> > Make TDA998x compliant by using the helper to set the quantisation
> > range in the infoframe, and using the TDA998x's colour scaling to
> > appropriately adjust the RGB values sent to the monitor.
> >
> > This ensures that monitors that do not support the Q bit are sent
> > RGB values that are within the expected range. Monitors with
> > support for the Q bit will be sent full-range RGB.
> >
> > Signed-off-by: Russell King
> > ---
> > drivers/gpu/drm/i2c/tda998x_drv.c | 39
> > ++-
> > 1 file changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > b/drivers/gpu/drm/i2c/tda998x_drv.c
> > index b0ed2ef49c62..7d9aea79aff2 100644
> > --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > @@ -50,6 +50,8 @@ struct tda998x_priv {
> > bool is_on;
> > bool supports_infoframes;
> > bool sink_has_audio;
> > + bool has_rgb_quant;
> > + enum hdmi_quantization_range rgb_quant_range;
> > u8 vip_cntrl_0;
> > u8 vip_cntrl_1;
> > u8 vip_cntrl_2;
> > @@ -869,7 +871,9 @@ tda998x_write_avi(struct tda998x_priv *priv, const
> > struct drm_display_mode *mode
> >
> > drm_hdmi_avi_infoframe_from_display_mode(,
> > >connector, mode);
> > - frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_FULL;
> > + drm_hdmi_avi_infoframe_quant_range(, mode,
> > + priv->rgb_quant_range,
> > + priv->has_rgb_quant, false);
> >
> > tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, );
> > }
> > @@ -1259,6 +1263,7 @@ static int tda998x_connector_get_modes(struct
> > drm_connector *connector)
> > mutex_lock(>audio_mutex);
> > n = drm_add_edid_modes(connector, edid);
> > priv->sink_has_audio = drm_detect_monitor_audio(edid);
> > + priv->has_rgb_quant = drm_rgb_quant_range_selectable(edid);
> > mutex_unlock(>audio_mutex);
> >
> > kfree(edid);
> > @@ -1385,6 +1390,15 @@ static void tda998x_bridge_mode_set(struct
> > drm_bridge *bridge,
> > u8 reg, div, rep, sel_clk;
> >
> > /*
> > +* Since we are "computer" like, our source invariably produces
> > +* full-range RGB. If the monitor supports full-range, then use
> > +* it, otherwise reduce to limited-range.
> > +*/
> > + priv->rgb_quant_range = priv->has_rgb_quant ?
> > + HDMI_QUANTIZATION_RANGE_FULL :
> > + drm_default_rgb_quant_range(adjusted_mode);
> > +
> > + /*
> > * Internally TDA998x is using ITU-R BT.656 style sync but
> > * we get VESA style sync. TDA998x is using a reference pixel
> > * relative to ITU to sync to the input frame and for output
> > @@ -1499,10 +1513,25 @@ static void tda998x_bridge_mode_set(struct
> > drm_bridge *bridge,
> > reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) |
> > PLL_SERIAL_2_SRL_PR(rep));
> >
> > - /* set color matrix bypass flag: */
> > - reg_write(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP |
> > - MAT_CONTRL_MAT_SC(1));
> > - reg_set(priv, REG_FEAT_POWERDOWN, FEAT_POWERDOWN_CSC);
> > + /* set color matrix according to output rgb quant range */
> > + if (priv->rgb_quant_range == HDMI_QUANTIZATION_RANGE_LIMITED) {
> > + static u8 tda998x_full_to_limited_range[] = {
> > + MAT_CONTRL_MAT_SC(2),
> > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > + 0x03, 0x6f, 0x00, 0x00, 0x00, 0x00,
> > + 0x00, 0x00, 0x03, 0x6f, 0x00, 0x00,
> > + 0x00, 0x00, 0x00, 0x00, 0x03, 0x6f,
> > + 0x00, 0x40, 0x00, 0x40, 0x00, 0x40
> > + };
>
> I couldn't figure out from the datasheet I have what the expected data
> bit-depth is here, but I assume from this offset that it's 12-bit?
No, the data sheet makes it clear that (eg) MAT_OI1_MSB contains
OFFSET_IN1[10:8] and MAT_OI1_LSB contains OFFSET_IN1[7:0] - so that's
11 bits of offset - which looks like from the information I have that
it's twos complement. Similar applies to the output offsets.
The above is the list of register values starting at MAT_CONTRL (0x80),
with the input offsets in the first line, then the G/Y output
coefficients, R/CR coefficients, B/CB coefficients and finally the
output offsets on the last line.
Each line is a pair of MSB, LSB values, starting at G/Y input, R/CR
input, B/CB input.
The above is equivalent to:
GY_OUT = (GY_IN + 0) * 0x36f / 0x400
Re: [PATCH 3/5] drm/i2c: tda998x: add support for writing SPD
On Wed, Jan 30, 2019 at 03:41:04PM +, Brian Starkey wrote:
> Hi Russell,
>
> These did eventually reach me on Saturday evening.
>
> On Fri, Jan 25, 2019 at 09:43:19AM +, Russell King wrote:
> > Add support for writing the SPD infoframe to the TDA998x. Identify us
> > as "Generic" vendor "PC" product, and as "PC general" source device
> > information.
> >
> > Signed-off-by: Russell King
> > ---
>
> As this infoframe is optional, and is intended to provide a "useful"
> name to the user, I wonder if there's really much value in just
> sending "Generic"/"PC"? It seems that it might be better to just not
> send the SPD infoframe until we have a way to put something more
> useful there (e.g. specified by the host driver).
It's along the lines of what other drivers do - are you suggesting
that other drivers should not send the SPD infoframe either?
E.g.
static void vc4_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
{
union hdmi_infoframe frame;
int ret;
ret = hdmi_spd_infoframe_init(, "Broadcom", "Videocore");
===
mtk_hdmi_setup_spd_infoframe(hdmi, "mediatek", "On-chip HDMI");
===
ret = hdmi_spd_infoframe_init(, "Intel", "Integrated gfx");
None of these convey a "useful" name to the user, unless the user
knows what is inside their device - eg, "it's a mediatek SoC" or
"it's a Broadcom SoC".
I could send instead "Philips" "TDA998x" which would be on-par with
these strings.
Maybe there should be a way to set these from DT and/or userspace?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/5] tda998x updates
(Removed what I assume is a typo on the Cc line - n...@arm.com) On Fri, Jan 25, 2019 at 11:45:10AM +, Brian Starkey wrote: > Hi Russell, > > On Fri, Jan 25, 2019 at 09:40:38AM +, Russell King - ARM Linux admin > wrote: > > Hi, > > > > This series adds support for programming the SPD and vendor infoframes. > > > > It also adds support for pixel repeated modes - we were not rejecting > > these modes, but we also didn't have the implementation to support > > them. As their implementation is simple, add it rather than rejecting > > the modes. > > > > Support is also added for the bridge timing information, that upstream > > components may wish to use to adjust their output appropriately. > > > > Lastly, rather than merely passing through the full-range RGB from the > > CRTC, adapt the RGB range to the capabilities of the display and the > > default range for the mode. This means that if the display does not > > support the Q bit in the video infoframe, and the mode is defined to > > have limited range RGB, we will compress the output RGB range to > > limited range. > > > > Tested on 4.20 with a Panasonic TV. > > > > drm/i2c: tda998x: add support for pixel repeated modes > > drm/i2c: tda998x: add bridge timing information > > drm/i2c: tda998x: add support for writing SPD > > drm/i2c: tda998x: add vendor specific infoframe support > > drm/i2c: tda998x: improve correctness of quantisation range > > Only this cover letter made it to my inbox (and the dri-devel > archives, for what they're worth). I think leave it a while longer - the series emails were accepted by gabe.freedesktop.org at 09:51 UTC, maybe it's still thinking about them. If not, I don't think there's anything I can do about it. > Is there somewhere I can take a look at the patches? http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=drm-tda998x-devel contains very similar patches, but based on 4.20. The series I posted were those patches rebased on the linux-drm drm-next branch - and there were some minor conflicts. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/5] tda998x updates
Hi, This series adds support for programming the SPD and vendor infoframes. It also adds support for pixel repeated modes - we were not rejecting these modes, but we also didn't have the implementation to support them. As their implementation is simple, add it rather than rejecting the modes. Support is also added for the bridge timing information, that upstream components may wish to use to adjust their output appropriately. Lastly, rather than merely passing through the full-range RGB from the CRTC, adapt the RGB range to the capabilities of the display and the default range for the mode. This means that if the display does not support the Q bit in the video infoframe, and the mode is defined to have limited range RGB, we will compress the output RGB range to limited range. Tested on 4.20 with a Panasonic TV. drm/i2c: tda998x: add support for pixel repeated modes drm/i2c: tda998x: add bridge timing information drm/i2c: tda998x: add support for writing SPD drm/i2c: tda998x: add vendor specific infoframe support drm/i2c: tda998x: improve correctness of quantisation range drivers/gpu/drm/i2c/tda998x_drv.c | 120 +- 1 file changed, 105 insertions(+), 15 deletions(-) -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/armada: add mmp2 support
On Mon, Jan 21, 2019 at 07:03:49AM +0100, Lubomir Rintel wrote:
> Heavily based on the Armada 510 (Dove) support. Like with 510 support, this
> also just supports a single source clock -- the "Display 1" clock as
> generated by the APMU. This one was chosen because the OLPC XO 1.75 laptop
> uses it for its internal panel.
>
> If anyone uses this to drive a MIPI or HDMI encoder, they may want to
> extend this to choose a different source for the pixel clock -- it should
> be a reasonably straightforward thing to do.
>
> The data sheet is not available, but James Cameron of OLPC kindly
> provided some details about the LCD_SCLK_DIV register.
>
> Link:
> https://lists.freedesktop.org/archives/dri-devel/2018-December/201021.html
> Signed-off-by: Lubomir Rintel
> ---
> drivers/gpu/drm/armada/Makefile | 1 +
> drivers/gpu/drm/armada/armada_610.c | 93
> drivers/gpu/drm/armada/armada_crtc.c | 4 ++
> drivers/gpu/drm/armada/armada_drm.h | 1 +
> drivers/gpu/drm/armada/armada_hw.h | 10 +++
> 5 files changed, 109 insertions(+)
> create mode 100644 drivers/gpu/drm/armada/armada_610.c
>
> diff --git a/drivers/gpu/drm/armada/Makefile b/drivers/gpu/drm/armada/Makefile
> index 9bc3c3213724..5bbf86324cda 100644
> --- a/drivers/gpu/drm/armada/Makefile
> +++ b/drivers/gpu/drm/armada/Makefile
> @@ -2,6 +2,7 @@
> armada-y := armada_crtc.o armada_drv.o armada_fb.o armada_fbdev.o \
> armada_gem.o armada_overlay.o armada_plane.o armada_trace.o
> armada-y += armada_510.o
> +armada-y += armada_610.o
> armada-$(CONFIG_DEBUG_FS) += armada_debugfs.o
>
> obj-$(CONFIG_DRM_ARMADA) := armada.o
> diff --git a/drivers/gpu/drm/armada/armada_610.c
> b/drivers/gpu/drm/armada/armada_610.c
> new file mode 100644
> index ..278b204038ea
> --- /dev/null
> +++ b/drivers/gpu/drm/armada/armada_610.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2012 Russell King
> + * Copyright (C) 2018,2019 Lubomir Rintel
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Armada MMP2 variant support
> + */
> +#include
> +#include
> +#include
> +#include "armada_crtc.h"
> +#include "armada_drm.h"
> +#include "armada_hw.h"
> +
> +static int armada610_crtc_init(struct armada_crtc *dcrtc, struct device *dev)
> +{
> + struct clk *clk;
> +
> + clk = devm_clk_get(dev, "disp0");
> + if (IS_ERR(clk))
> + return PTR_ERR(clk) == -ENOENT ? -EPROBE_DEFER : PTR_ERR(clk);
> +
> + dcrtc->extclk[0] = clk;
I've been reworking the clocking support for Armada, you can find the
current code in my git tree's drm-armada-devel branch (as mentioned
in MAINTAINERS). You'll need to update to that before I can apply
this.
The clocks are named in Dove's TRM as:
0 = AXIbus: Select AXI bus clock as pixel clock source.
1 = EXT_REF_CLK0: LCD_EXT_REF_CLK[0]
2 = PLLDivider: Select PLL divider input clock as pixel clock source.
3 = EXT_REF_CLK1: LCD_EXT_REF_CLK[1]
So I chose to use these neumonics in the Armada 510. Please can we
keep to naming the clock inputs as per documented names please?
Also, have a look at how Armada 510 gets its clocks from DT - note
that the array they're placed in is ordered by priority (iow, if we
have an external clock, we use that in preference to the more
restricted axibus and plldivider clocks.)
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
