Re: [Nouveau] [PATCH 000/102] Convert drivers to explicit reset API
On Thu, Jul 20, 2017 at 10:46 PM, Dmitry Torokhov <dmitry.torok...@gmail.com> wrote: > On Thu, Jul 20, 2017 at 5:55 AM, Philipp Zabel <p.za...@pengutronix.de> wrote: >>> What about reset_control_get(struct device *, const char *, int flags) >>> to replace all those variants ? >> >> While I like how this looks, unfortunately (devm_)reset_control_get >> already exists without the flags, so we can't change to that with a >> gentle transition. > > This was done for gpiod_get() and its flags argument with horrifying > #define-ry, which thankfully was completely hidden from users. For your reference: commit bae48da237fcedd7ad09569025483b988635efb7 "gpiolib: add gpiod_get() and gpiod_put() functions" commit 39b2bbe3d715cf5013b5c48695ccdd25bd3bf120 "gpio: add flags argument to gpiod_get*() functions" commit 0dbc8b7afef6e4fddcfebcbacbeb269a0a3b06d5 "gpio: move varargs hack outside #ifdef GPIOLIB" commit b17d1bf16cc72a374a48d748940f79d40ff4 "gpio: make flags mandatory for gpiod_get functions" Retrospectively ... was that really a good idea... it was a LOT of trouble to add a flag, maybe it had been better to try and just slam all users in a single go. But it worked. Yours, Linus Walleij ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 04/12] drm: Nuke mode->vrefresh
On Tue, Feb 25, 2020 at 8:27 PM Ville Syrjälä wrote: > OK, so I went ahead a wrote a bit of cocci [1] to find the bad apples. That's impressive :D > Unfortunately it found a lot of strange stuff: I will answer for the weirdness I caused. I have long suspected that a whole bunch of the "simple" displays are not simple but contains a display controller and memory. That means that the speed over the link to the display and actual refresh rate on the actual display is asymmetric because well we are just updating a RAM, the resolution just limits how much data we are sending, the clock limits the speed on the bus over to the RAM on the other side. In most cases I thing the clock is the way to go. > panel-sony-acx424akp.c:51/sony_acx424akp_vid_mode: 60 vs. 727 (.clock=33 > .htotal=480 + 15 + 0 + 15 .vtotal=864 + 14 + 1 + 11) I suspect clock should be adjusted after vfresh = 60 here instead of the other way around. I couldn't quite test the video mode, but the vendor driver (no documentation ) does state 330 MHz which seems a bit high. Just drop vrefresh for now. > panel-sony-acx424akp.c:71/sony_acx424akp_cmd_mode: 60 vs. 711 (.clock=420160 > .htotal=480 + 154 + 16 + 32 .vtotal=864 + 1 + 1 + 1) You can override this ignoring the vrefresh, this is a command-mode only, and in command mode the refresh doesn't come into play, or is very high and limited by a bunch of other overhead than just the resolution. The command mode HS clock is @420+ MHz indeed. Tests showed around 116 Hz for this particular display in practice with continuous updates. > panel-ilitek-ili9322.c:543/srgb_320x240_mode: 60 vs. 10168 (.clock=2453500 > .htotal=320 + 359 + 1 + 241 .vtotal=262) > panel-ilitek-ili9322.c:587/yuv_640x320_mode: 60 vs. 7768 (.clock=2454000 > .htotal=640 + 252 + 1 + 28 .vtotal=320 + 4 + 1 + 18) > panel-ilitek-ili9322.c:616/itu_r_bt_656_640_mode: 60 vs. 5358 (.clock=2454000 > .htotal=640 + 3 + 1 + 272 .vtotal=500) > panel-ilitek-ili9322.c:557/srgb_360x240_mode: 60 vs. 16178 (.clock=270 > .htotal=360 + 35 + 1 + 241 .vtotal=262) > panel-ilitek-ili9322.c:601/yuv_720x360_mode: 60 vs. 7071 (.clock=270 > .htotal=720 + 252 + 1 + 24 .vtotal=360 + 4 + 1 + 18) > panel-ilitek-ili9322.c:631/itu_r_bt_656_720_mode: 60 vs. 5422 (.clock=270 > .htotal=720 + 3 + 1 + 272 .vtotal=500) > panel-ilitek-ili9322.c:572/prgb_320x240_mode: 60 vs. 59725 (.clock=640 > .htotal=320 + 38 + 1 + 50 .vtotal=262) This is the datasheet if you want to take a look: https://dflund.se/~triad/krad/dlink-dir-685/ILI9322DS_V1.12.pdf The one platform using this is using the 8 bit ITU-R BT.656 640Y 320CbCr mode which (I think) is similar to DSI command mode again: there is a stream of data in a burst and you decide how often you want to send it because the panel always has backing memory and the speed out to the physical display is something completely different. You can safely delete vrefresh from all of these sites. > panel-arm-versatile.c:161/versatile_panels[]: 60 vs. 61 (.clock=25000 > .htotal=640 + 24 + 96 + 24 .vtotal=480 + 11 + 2 + 32) > panel-arm-versatile.c:208/versatile_panels[]: 116 vs. 59 (.clock=5400 > .htotal=240 + 10 + 10 + 20 .vtotal=320 + 2 + 2 + 2) > panel-arm-versatile.c:184/versatile_panels[]: 390 vs. 1523 (.clock=62500 > .htotal=176 + 2 + 3 + 3 .vtotal=220 + 0 + 2 + 1) The only driver drivers/gpu/drm/pl111/pl111_display.c Uses mode->clock so just drop vrefresh. Yours, Linus Walleij ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] (no subject)
On Wed, Feb 26, 2020 at 3:34 PM Ville Syrjälä
wrote:
> On Wed, Feb 26, 2020 at 01:08:06PM +0100, Linus Walleij wrote:
> > On Wed, Feb 26, 2020 at 12:57 PM Ville Syrjälä
> > wrote:
> > > On Tue, Feb 25, 2020 at 10:52:25PM +0100, Linus Walleij wrote:
> >
> > > > I have long suspected that a whole bunch of the "simple" displays
> > > > are not simple but contains a display controller and memory.
> > > > That means that the speed over the link to the display and
> > > > actual refresh rate on the actual display is asymmetric because
> > > > well we are just updating a RAM, the resolution just limits how
> > > > much data we are sending, the clock limits the speed on the
> > > > bus over to the RAM on the other side.
> > >
> > > IMO even in command mode mode->clock should probably be the actual
> > > dotclock used by the display. If there's another clock for the bus
> > > speed/etc. it should be stored somewhere else.
> >
> > Good point. For the DSI panels we have the field hs_rate
> > for the HS clock in struct mipi_dsi_device which is based
> > on exactly this reasoning. And that is what I actually use for
> > setting the HS clock.
> >
> > The problem is however that we in many cases have so
> > substandard documentation of these panels that we have
> > absolutely no idea about the dotclock. Maybe we should
> > just set it to 0 in these cases?
>
> Don't you always have a TE interrupt or something like that
> available? Could just measure it from that if no better
> information is available?
Yes and I did exactly that, so that is why this comment is in
the driver:
static const struct drm_display_mode sony_acx424akp_cmd_mode = {
(...)
/*
* Some desired refresh rate, experiments at the maximum "pixel"
* clock speed (HS clock 420 MHz) yields around 117Hz.
*/
.vrefresh = 60,
I got a review comment at the time saying 117 Hz was weird.
We didn't reach a proper conclusion on this:
https://lore.kernel.org/dri-devel/CACRpkdYW3YNPSNMY3A44GQn8DqK-n9TLvr7uipF7LM_DHZ5=l...@mail.gmail.com/
Thierry wasn't sure if 60Hz was good or not, so I just had to
go with something.
We could calculate the resulting pixel clock for ~117 Hz with
this resolution and put that in the clock field but ... don't know
what is the best?
Yours,
Linus Walleij
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] (no subject)
On Wed, Feb 26, 2020 at 12:57 PM Ville Syrjälä wrote: > On Tue, Feb 25, 2020 at 10:52:25PM +0100, Linus Walleij wrote: > > I have long suspected that a whole bunch of the "simple" displays > > are not simple but contains a display controller and memory. > > That means that the speed over the link to the display and > > actual refresh rate on the actual display is asymmetric because > > well we are just updating a RAM, the resolution just limits how > > much data we are sending, the clock limits the speed on the > > bus over to the RAM on the other side. > > IMO even in command mode mode->clock should probably be the actual > dotclock used by the display. If there's another clock for the bus > speed/etc. it should be stored somewhere else. Good point. For the DSI panels we have the field hs_rate for the HS clock in struct mipi_dsi_device which is based on exactly this reasoning. And that is what I actually use for setting the HS clock. The problem is however that we in many cases have so substandard documentation of these panels that we have absolutely no idea about the dotclock. Maybe we should just set it to 0 in these cases? Yours, Linus Walleij ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v5 09/10] dma-buf-map: Add memcpy and pointer-increment interfaces
Overall I like this, just an inline question:
On Tue, Oct 20, 2020 at 2:20 PM Thomas Zimmermann wrote:
> To do framebuffer updates, one needs memcpy from system memory and a
> pointer-increment function. Add both interfaces with documentation.
(...)
> +/**
> + * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
> + * @dst: The dma-buf mapping structure
> + * @src: The source buffer
> + * @len: The number of byte in src
> + *
> + * Copies data into a dma-buf mapping. The source buffer is in system
> + * memory. Depending on the buffer's location, the helper picks the correct
> + * method of accessing the memory.
> + */
> +static inline void dma_buf_map_memcpy_to(struct dma_buf_map *dst, const void
> *src, size_t len)
> +{
> + if (dst->is_iomem)
> + memcpy_toio(dst->vaddr_iomem, src, len);
> + else
> + memcpy(dst->vaddr, src, len);
> +}
Are these going to be really big memcpy() operations?
Some platforms have DMA offload engines that can perform memcpy(),
drivers/dma, include/linux/dmaengine.h
especially if the CPU doesn't really need to touch the contents
and flush caches etc.
An example exist in some MTD drivers that move large quantities of
data off flash memory like this:
drivers/mtd/nand/raw/cadence-nand-controller.c
Notice that DMAengine and DMAbuf does not have much in common,
the names can be deceiving.
The value of this varies with the system architecture. It is not just
a question about performance but also about power and the CPU
being able to do other stuff in parallel for large transfers. So *when*
to use this facility to accelerate memcpy() is a delicate question.
What I'm after here is if these can be really big, do we want
(in the long run, not now) open up to the idea to slot in
hardware-accelerated memcpy() here?
Yours,
Linus Walleij
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [PATCH v1 1/1] treewide: Align match_string() with sysfs_match_string()
On Tue, Jun 4, 2024 at 9:46 AM Jani Nikula wrote: [Maybe slightly off-topic, ranty] > Why do we think it's a good idea to increase and normalize the use of > double-underscore function names across the kernel, like > __match_string() in this case? It should mean "reserved for the > implementation, not to be called directly". > > If it's to be used directly, it should be named accordingly, right? It's a huge mess. "__" prefix is just so ambiguous I think it just shouldn't be used or prolifierated, and it usually breaks Rusty Russells API rules times over. Consider __set_bit() from , used all over the place, in contrast with set_bit() for example, what does "__" represent in this context that makes __set_bit() different from set_bit()? It means "non-atomic"... How does a random contributor know this? Yeah, you guess it. By the token of "everybody knows that". (Grep, google, repeat for the number of contributors to the kernel.) I was considering to send a script to Torvalds to just change all this to set_bit_nonatomic() (etc) but was hesitating because that makes the name unambiguous but long. I think I stayed off it because changing stuff like that all over the place creates churn and churn is bad. Yours, Linus Walleij
