Re: [PATCH 3/6] drm/xen-front: Add YUYV to supported formats

[Noralf Trønnes]

Den 31.07.2020 14.51, skrev Oleksandr Andrushchenko:
> From: Oleksandr Andrushchenko 
> 
> Add YUYV to supported formats, so the frontend can work with the
> formats used by cameras and other HW.
> 
> Signed-off-by: Oleksandr Andrushchenko 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH 5/6] drm/xen-front: Pass dumb buffer data offset to the backend

[Noralf Trønnes]

Den 31.07.2020 14.51, skrev Oleksandr Andrushchenko:
> From: Oleksandr Andrushchenko 
> 
> While importing a dmabuf it is possible that the data of the buffer
> is put with offset which is indicated by the SGT offset.
> Respect the offset value and forward it to the backend.
> 
> Signed-off-by: Oleksandr Andrushchenko 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v5 6/8] drm/panel: Add ilitek ili9341 panel driver

[Noralf Trønnes]

Den 26.05.2020 11.08, skrev dillon min:
> Hi Andy,
> 
> Thanks for input.
> 
> On Tue, May 26, 2020 at 3:46 PM Andy Shevchenko
>  wrote:
>>
>> On Mon, May 25, 2020 at 6:46 AM  wrote:
>>>
>>> From: dillon min 
>>>
>>> This driver combine tiny/ili9341.c mipi_dbi_interface driver
>>> with mipi_dpi_interface driver, can support ili9341 with serial
>>> mode or parallel rgb interface mode by register configuration.
>>
>> Noralf told once that this driver should be unified with mi0283qt.c.
>>
>> So, what should we do here?
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
> 
> from sam's suggestion, we can't setup two drivers to support one panel
> in the tree. so, i copy the mipi dbi part from tiny/ili9341.c. to this driver
> from register settings and dts binding is keep the same to tiny/ili9341.c.
> 
> so, in my opinion if tiny/ili9341.c is unified with mi0283qt.c, this
> driver should be
> too.
> 

There's a discussion about MIPI DBI panels here:

MIPI DSI, DBI, and tinydrm drivers
https://lists.freedesktop.org/archives/dri-devel/2020-May/267031.html

Noralf.

> thanks.
> 
> best regards,
> 
> Dillon,
> 

Re: [PATCH] drm/tiny: Kconfig: Remove always-y THERMAL dep. from TINYDRM_REPAPER

[Noralf Trønnes]

Den 01.10.2019 12.58, skrev Noralf Trønnes:
> 
> 
> Den 27.09.2019 19.42, skrev Ulf Magnusson:
>> Commit 554b3529fe01 ("thermal/drivers/core: Remove the module Kconfig's
>> option") changed the type of THERMAL from tristate to bool, so
>> THERMAL || !THERMAL is now always y. Remove the redundant dependency.
>>
>> Discovered through Kconfiglib detecting a dependency loop. The C tools
>> simplify the expression to y before running dependency loop detection,
>> and so don't see it. Changing the type of THERMAL back to tristate makes
>> the C tools detect the same loop.
>>
>> Not sure if running dep. loop detection after simplification can be
>> called a bug. Fixing this nit unbreaks Kconfiglib on the kernel at
>> least.
>>
>> Signed-off-by: Ulf Magnusson 
>> ---
> 
> Thanks, applied to drm-misc-next.
> 

This has now been queued for the next -rc pull.

Discussion: https://patchwork.freedesktop.org/patch/319826/

Noralf.

Re: 5.2-rc1 on droid4: spi crash

[Noralf Trønnes]

Den 2019-05-27 07:53, skrev Tony Lindgren:

Hi,

* Sebastian Reichel  [190523 09:33]:

Hi,

On Thu, May 23, 2019 at 11:09:26AM +0200, Pavel Machek wrote:
> This was greeting me overnight... I don't yet know how reproducible it
> is, it happened once so far.

Please pipe the stacktrace into ./scripts/decode_stacktrace.sh
to get a readable stacktrace, otherwise this is pretty much useless.
FWIW the only SPI device in the Droid 4 is the PMIC.


I've seen this too, and looks like reverting commit c9ba7a16d0f1
("spi: Release spi_res after finalizing message") fixes it based
several days of testing.

Noralf and Mark, any ideas what needs to be fixed here?


Mark has a revert in his for-5.2 branch:
spi: Fix Raspberry Pi breakage
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/commit/?h=for-5.2=0ed56252c9567351344cb7b5cff6140e1bcec943

I don't know when or if he has sent a pull request.
Sorry about the breakage.

Noralf.



Below is the stacktrace I see without c9ba7a16d0f1 reverted,
not sure how to reproduce but it seems to happen within about
one to two days of uptime.

Regards,

Tony

8< -
Unable to handle kernel NULL pointer dereference at virtual address 
0008

pgd = 829f0a5b
[0008] *pgd=
Internal error: Oops: 8005 [#1] SMP ARM
...
CPU: 0 PID: 71 Comm: spi0 Tainted: GW 5.2.0-rc1+ #5983
Hardware name: Generic OMAP4 (Flattened Device Tree)
PC is at 0x8
LR is at spi_res_release+0x54/0x80
pc : [<0008>]lr : []psr: 2113
sp : ed6e3e88  ip : ed6e3eb0  fp : ed6e3eac
r10: c0b9eca8  r9 : 0100  r8 : 0200
r7 : ed65bc00  r6 : ed6e5d3c  r5 : ed6e5d0c  r4 : c0d05254
r3 : 0008  r2 : c0d05264  r1 : ed6e5d0c  r0 : ed65bc00
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: abf3c04a  DAC: 0051
Process spi0 (pid: 71, stack limit = 0x0ef66f65)
Stack: (0xed6e3e88 to 0xed6e4000)
3e80:   ed6e5cd0 ed6e5d0c ed65bc00 c0daf080  
ed510410
3ea0: ed6e3eec ed6e3eb0 c06fd8c4 c06faa00 ed65b800  ed65ba20 
ed65bee0
3ec0: ed6e3eec ed65bc00 ed6e5cd0 ed6e5d0c  ed510410 ed510410 
0001
3ee0: ed6e3f2c ed6e3ef0 c06fdcd4 c06fd560 0004 c0170948 ed6e3f20 
ed65bdfc
3f00: e000 ed65be68 ed65be44 e000 c0dc7734 ed65be48 c0166f88 

3f20: ed6e3f3c ed6e3f30 c06fe10c c06fd9a4 ed6e3f74 ed6e3f40 c0166f54 
c06fe0f8
3f40: ed6e3f74 6eb8f9ff c0166780  ed3bccc0 ed659c00 ed6e2000 
ed65be44
3f60: c0166eac ed115c44 ed6e3fac ed6e3f78 c0166e58 c0166eb8 ed3bccdc 
ed3bccdc
3f80: ed6e3fac ed659c00 c0166cf8     

3fa0:  ed6e3fb0 c01010e8 c0166d04    

3fc0:        

3fe0:     0013   


Backtrace:
[] (spi_res_release) from []
(spi_transfer_one_message+0x370/0x444)
 r9:ed510410 r8: r7:c0daf080 r6:ed65bc00 r5:ed6e5d0c 
r4:ed6e5cd0

[] (spi_transfer_one_message) from []
(__spi_pump_messages+0x33c/0x754)
 r10:0001 r9:ed510410 r8:ed510410 r7: r6:ed6e5d0c 
r5:ed6e5cd0

 r4:ed65bc00
[] (__spi_pump_messages) from []
(spi_pump_messages+0x20/0x24)
 r10: r9:c0166f88 r8:ed65be48 r7:c0dc7734 r6:e000 
r5:ed65be44

 r4:ed65be68
[] (spi_pump_messages) from []
(kthread_worker_fn+0xa8/0x268)
[] (kthread_worker_fn) from [] 
(kthread+0x160/0x178)
 r10:ed115c44 r9:c0166eac r8:ed65be44 r7:ed6e2000 r6:ed659c00 
r5:ed3bccc0

 r4:
[] (kthread) from [] (ret_from_fork+0x14/0x2c)
Exception stack(0xed6e3fb0 to 0xed6e3ff8)
3fa0:    

3fc0:        


3fe0:     0013 
 r10: r9: r8: r7: r6: 
r5:c0166cf8

 r4:ed659c00
Code: bad PC value
---[ end trace a8011e9722dfda5e ]---

Re: [drm/i915/fbdev] 09ded8af57: dmesg.RIP:drm_setup_crtcs[drm_kms_helper]

[Noralf Trønnes]

Den 28.04.2019 11.51, skrev kernel test robot:
> FYI, we noticed the following commit (built with gcc-7):
> 
> commit: 09ded8af57bcef7287b8242087d3e7556380de62 ("drm/i915/fbdev: Move 
> intel_fb_initial_config() to fbdev helper")
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git master
> 
> in testcase: plzip
> with following parameters:
> 
>   nr_threads: 100%
>   cpufreq_governor: performance
>   ucode: 0x12
> 
> 
> 
> on test machine: 144 threads Intel(R) Xeon(R) CPU E7-8890 v3 @ 2.50GHz with 
> 512G memory
> 
> caused below changes (please refer to attached dmesg/kmsg for entire 
> log/backtrace):
> 
> 
> +-+++
> | | e33898a207 | 09ded8af57 |
> +-+++
> | boot_successes  | 14 | 4  |
> | boot_failures   | 3  ||
> | BUG:kernel_reboot-without-warning_in_test_stage | 3  ||
> | dmesg.RIP:drm_setup_crtcs[drm_kms_helper]   | 0  | 4  |
> +-+++
> 
> 
> If you fix the issue, kindly add following tag
> Reported-by: kernel test robot 
> 
> 
> [   35.625371] BUG: unable to handle kernel NULL pointer dereference at 
> 0010
> [   35.634549] #PF error: [normal kernel read fault]
> [   35.639800] PGD 0 P4D 0 
> [   35.639805] Oops:  [#1] SMP PTI
> [   35.646527] CPU: 53 PID: 1179 Comm: systemd-udevd Not tainted 
> 5.1.0-rc2-01104-g09ded8a #1
> [   35.655659] Hardware name: Intel Corporation BRICKLAND/BRICKLAND, BIOS 
> BRHSXSD1.86B.0067.R02.1507221722 07/22/2015
> [   35.667239] RIP: 0010:drm_setup_crtcs+0x3eb/0x1020 [drm_kms_helper]
> [   35.674236] Code: 44 24 60 48 8b 04 24 4c 01 f0 80 38 00 0f 84 a3 07 00 00 
> 41 8b 8d 1c 03 00 00 83 f9 01 0f 84 5f 09 00 00 49 8b 95 e0 03 00 00 <48> 83 
> 7a 10 00 0f 84 e0 01 00 00 48 8b 52 08 48 85 d2 0f 84 d1 01
> [   35.695199] RSP: :c9000f9938c0 EFLAGS: 00010297
> [   35.701035] RAX: 88c083da2198 RBX: 88c083da2180 RCX: 
> 
> [   35.709000] RDX:  RSI:  RDI: 
> 88c083da21a1
> [   35.716966] RBP:  R08: 88e083ab7d48 R09: 
> 
> [   35.724934] R10:  R11: 889fed53ec18 R12: 
> 0001
> [   35.732894] R13: 888103a92800 R14:  R15: 
> 0001
> [   35.740863] FS:  7fe6402648c0() GS:88dfff64() 
> knlGS:
> [   35.749898] CS:  0010 DS:  ES:  CR0: 80050033
> [   35.756312] CR2: 0010 CR3: 00807c142003 CR4: 
> 001606e0
> [   35.764278] DR0:  DR1:  DR2: 
> 
> [   35.772235] DR3:  DR6: fffe0ff0 DR7: 
> 0400
> [   35.780201] Call Trace:
> [   35.782946]  __drm_fb_helper_initial_config_and_unlock+0x46/0x540 
> [drm_kms_helper]
> [   35.791404]  mgag200_fbdev_init+0xc6/0xe0 [mgag200]
> [   35.796855]  mgag200_modeset_init+0x150/0x1b0 [mgag200]
> [   35.802694]  mgag200_driver_load+0x359/0x4d0 [mgag200]
> [   35.808470]  drm_dev_register+0x11c/0x1b0 [drm]
> [   35.813547]  drm_get_pci_dev+0x9d/0x180 [drm]
> [   35.814299] ata5: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> [   35.818417]  local_pci_probe+0x42/0x90
> [   35.818427]  ? _cond_resched+0x19/0x30
> [   35.827168] ata5.00: ATAPI: TEACDV-W28S-W, 1.0A, max UDMA/100
> [   35.829511]  pci_device_probe+0x141/0x1b0
> [   35.829521]  really_probe+0xf8/0x3e0
> [   35.835771] ata5.00: configured for UDMA/100
> [   35.840505]  driver_probe_device+0x10f/0x120
> [   35.840511]  device_driver_attach+0x50/0x60
> [   35.847061] scsi 5:0:0:0: CD-ROMTEAC DV-W28S-W1.0A 
> PQ: 0 ANSI: 5
> [   35.848979]  __driver_attach+0x9a/0x140
> [   35.848983]  ? device_driver_attach+0x60/0x60
> [   35.848987]  bus_for_each_dev+0x76/0xc0
> [   35.848996]  ? klist_add_tail+0x3b/0x70
> [   35.889929]  bus_add_driver+0x141/0x210
> [   35.894214]  ? 0xc078d000
> [   35.897914]  driver_register+0x5b/0xe0
> [   35.902093]  ? 0xc078d000
> [   35.905805]  do_one_initcall+0x46/0x1e4
> [   35.910089]  ? _cond_resched+0x19/0x30
> [   35.914279]  ? kmem_cache_alloc_trace+0x3b/0x1d0
> [   35.919439]  do_init_module+0x5b/0x210
> [   35.923630]  load_module+0x1838/0x1f00
> [   35.927822]  ? ima_post_read_file+0xe2/0x120
> [   35.932592]  ? __do_sys_finit_module+0xe9/0x110
> [   35.937650]  __do_sys_finit_module+0xe9/0x110
> [   35.942508]  do_syscall_64+0x5b/0x1a0
> [   35.946595]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [   35.952226] RIP: 0033:0x7fe63f0e1229
> [   35.956210] Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 
> f8 48 89 f7 48 89 d6 48 89 ca 4d 

Re: [PATCH v3 4/5] drm: add drm_fb_xrgb8888_to_rgb888_dstclip()

[Noralf Trønnes]

Den 05.04.2019 11.52, skrev Gerd Hoffmann:
> Simliar to drm_fb_xrgb_to_rgb565_dstclip() but converts to rgb888

Simliar -> Similar

> instead of rgb565.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Reviewed-by: Noralf Trønnes 

Re: [PATCH v3 5/5] drm/virtio: rework resource creation workflow.

[Noralf Trønnes]

Den 18.03.2019 12.33, skrev Gerd Hoffmann:
> This patch moves the virtio_gpu_cmd_create_resource() call (which
> notifies the host about the new resource created) into the
> virtio_gpu_object_create() function.  That way we can call
> virtio_gpu_cmd_create_resource() before ttm_bo_init(), so the host
> already knows about the object when ttm initializes the object and calls
> our driver callbacks.
> 
> Specifically the object is already created when the
> virtio_gpu_ttm_tt_bind() callback invokes virtio_gpu_object_attach(),
> so the extra virtio_gpu_object_attach() calls done after
> virtio_gpu_object_create() are not needed any more.
> 
> The fence support for the create ioctl becomes a bit more tricky though.
> The code moved into virtio_gpu_object_create() too.  We first submit the
> (fenced) virtio_gpu_cmd_create_resource() command, then initialize the
> ttm object, and finally attach just created object to the fence for the
> command in case it didn't finish yet.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 2/5] drm/virtio: use struct to pass params to virtio_gpu_object_create()

[Noralf Trønnes]

Den 18.03.2019 12.33, skrev Gerd Hoffmann:
> Create virtio_gpu_object_params, use that to pass object parameters to
> virtio_gpu_object_create.  This is just the first step, followup patches
> will add more parameters to the struct.  The plan is to use the struct
> for all object parameters.
> 
> Drop unused "kernel" parameter for virtio_gpu_alloc_object(), it is
> unused and always false.
> 
> Also drop "pinned" parameter.  virtio-gpu doesn't shuffle around
> objects, so effecively they all are pinned anyway.  Hardcode
> TTM_PL_FLAG_NO_EVICT so ttm knows.  Doesn't change much for the moment
> as virtio-gpu supports TTM_PL_FLAG_TT only so there is no opportunity to
> move around objects.  That'll probably change in the future though.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH 3/3] drm/virtio: implement prime export

[Noralf Trønnes]

Den 27.02.2019 15.44, skrev Gerd Hoffmann:
> Just run drm_prime_pages_to_sg() on the ttm pages list to get an
> sg_table for export.  The pages list is created at object initialization
> time, so there should be no need to handle an unpopulated page list.
> Add a sanity check nevertheless.
> 
> Signed-off-by: Gerd Hoffmann 

Acked-by: Noralf Trønnes 

Re: [PATCH 1/3] drm/virtio: implement prime mmap

[Noralf Trønnes]

Den 27.02.2019 15.44, skrev Gerd Hoffmann:
> Sync gem vm_node.start with ttm vm_node.start,
> then we can just call drm_gem_prime_mmap().
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH 2/3] drm/virtio: implement prime pin/unpin

[Noralf Trønnes]

Den 27.02.2019 15.44, skrev Gerd Hoffmann:
> virtio-gpu objects never move around, so effectively they are pinned
> all the time.  This makes the the implementation pretty easy ;)
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/virtio/virtgpu_prime.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c 
> b/drivers/gpu/drm/virtio/virtgpu_prime.c
> index b4c9199349e7..0fcae0e46abd 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
> @@ -30,13 +30,13 @@
>  
>  int virtgpu_gem_prime_pin(struct drm_gem_object *obj)
>  {
> - WARN_ONCE(1, "not implemented");
> - return -ENODEV;
> + /* nothing: all virtio-gpu objects are pinned all the time */
> + return 0;
>  }
>  
>  void virtgpu_gem_prime_unpin(struct drm_gem_object *obj)
>  {
> - WARN_ONCE(1, "not implemented");
> + /* nothing */
>  }

You can just remove these dummies the callbacks are optional. See
drm_gem_pin().

With that:

Reviewed-by: Noralf Trønnes 

>  
>  void *virtgpu_gem_prime_vmap(struct drm_gem_object *obj)
> 

Re: [PATCH v2 1/2] drm: move i915_kick_out_vgacon to drm_fb_helper

[Noralf Trønnes]

Den 21.02.2019 12.35, skrev Gerd Hoffmann:
> It'll be useful for other drivers too, so move it to drm_fb_helper.c
> (and rename it of course).  Also add docs.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  include/drm/drm_fb_helper.h |  2 ++
>  drivers/gpu/drm/drm_fb_helper.c | 39 +++
>  drivers/gpu/drm/i915/i915_drv.c | 35 +--
>  3 files changed, 42 insertions(+), 34 deletions(-)
> 
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index bb9acea613..a401ba47ad 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -649,4 +649,6 @@ drm_fb_helper_remove_conflicting_pci_framebuffers(struct 
> pci_dev *pdev,
>  #endif
>  }
>  
> +int drm_fb_helper_kick_out_vgacon(void);
> +

Don't you need a dummy version as well for this one, like how it's done
for the other functions, to cover the case when DRM_FBDEV_EMULATION is
not selected?

Noralf.

>  #endif
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0e9349ff2d..a2d7e5bc51 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -35,6 +35,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -3353,3 +3354,41 @@ int __init drm_fb_helper_modinit(void)
>   return 0;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_modinit);
> +
> +/**
> + * drm_fb_helper_kick_out_vgacon - deactivate vgacon driver.
> + *
> + * Deactivate vgacon driver so it stops accessing vga io ports.
> + * Should be called after
> + * drm_fb_helper_remove_conflicting_pci_framebuffers().
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_fb_helper_kick_out_vgacon(void)
> +{
> +#if !defined(CONFIG_VGA_CONSOLE)
> +return 0;
> +#elif !defined(CONFIG_DUMMY_CONSOLE)
> +return -ENODEV;
> +#else
> +int ret = 0;
> +
> +DRM_INFO("Replacing VGA console driver\n");
> +
> +console_lock();
> +if (con_is_bound(_con))
> +ret = do_take_over_console(_con, 0, MAX_NR_CONSOLES - 
> 1, 1);
> +if (ret == 0) {
> +ret = do_unregister_con_driver(_con);
> +
> +/* Ignore "already unregistered". */
> +if (ret == -ENODEV)
> +ret = 0;
> +}
> +console_unlock();
> +
> +return ret;
> +#endif
> +}
> +EXPORT_SYMBOL(drm_fb_helper_kick_out_vgacon);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6630212f2f..3edd4d7d55 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -757,39 +757,6 @@ static int i915_kick_out_firmware_fb(struct 
> drm_i915_private *dev_priv)
>   return ret;
>  }
>  
> -#if !defined(CONFIG_VGA_CONSOLE)
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> - return 0;
> -}
> -#elif !defined(CONFIG_DUMMY_CONSOLE)
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> - return -ENODEV;
> -}
> -#else
> -static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
> -{
> - int ret = 0;
> -
> - DRM_INFO("Replacing VGA console driver\n");
> -
> - console_lock();
> - if (con_is_bound(_con))
> - ret = do_take_over_console(_con, 0, MAX_NR_CONSOLES - 1, 
> 1);
> - if (ret == 0) {
> - ret = do_unregister_con_driver(_con);
> -
> - /* Ignore "already unregistered". */
> - if (ret == -ENODEV)
> - ret = 0;
> - }
> - console_unlock();
> -
> - return ret;
> -}
> -#endif
> -
>  static void intel_init_dpio(struct drm_i915_private *dev_priv)
>  {
>   /*
> @@ -1420,7 +1387,7 @@ static int i915_driver_init_hw(struct drm_i915_private 
> *dev_priv)
>   goto err_ggtt;
>   }
>  
> - ret = i915_kick_out_vgacon(dev_priv);
> + ret = drm_fb_helper_kick_out_vgacon();
>   if (ret) {
>   DRM_ERROR("failed to remove conflicting VGA console\n");
>   goto err_ggtt;
> 

Re: [PATCH v2 4/6] drm/virtio: params struct for virtio_gpu_cmd_create_resource_3d()

[Noralf Trønnes]

Den 01.02.2019 09.01, skrev Gerd Hoffmann:
> On Thu, Jan 31, 2019 at 11:47:38AM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 30.01.2019 10.43, skrev Gerd Hoffmann:
>>> Add 3d resource parameters to virtio_gpu_object_params struct.  With
>>> that in place we can use it for virtio_gpu_cmd_resource_create_3d()
>>> calls.
>>>
>>> Signed-off-by: Gerd Hoffmann 
>>> ---
>>
>> You don't remove the struct virtio_gpu_resource_create_3d definition,
>> but it looks like there's no users left?
> 
> virtio_gpu_cmd_resource_create_3d() still uses it (and has to, it is
> part of the guest <-> host protocol).
> 

Acked-by: Noralf Trønnes 

Re: [PATCH v2 6/6] drm/virtio: move virtio_gpu_cmd_create_resource call into virtio_gpu_object_create

[Noralf Trønnes]

Den 30.01.2019 10.43, skrev Gerd Hoffmann:
> Specifically call virtio_gpu_object_create() before ttm_bo_init(), so
> the object is already created when ttm calls the
> virtio_gpu_ttm_tt_bind() callback (which in turn calls
> virtio_gpu_object_attach()).
> 
> With that in place virtio_gpu_object_attach() will never be called with
> an object which is not yet created, so the extra
> virtio_gpu_object_attach() calls done after
> virtio_gpu_cmd_create_resource() is not needed any more.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v2 5/6] drm/virtio: drop fencing in virtio_gpu_resource_create_ioctl

[Noralf Trønnes]

Den 30.01.2019 10.43, skrev Gerd Hoffmann:
> There is no need to wait for completion here.
> 
> The host will process commands in submit order, so commands can
> reference the new resource just fine even when queued up before
> completion.
> 
> On the guest side there is no need to wait for completion too.  Which
> btw is different from resource destroy, where we have to make sure the
> host has seen the destroy and thus doesn't use it any more before
> releasing the pages backing the resource.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v2 4/6] drm/virtio: params struct for virtio_gpu_cmd_create_resource_3d()

[Noralf Trønnes]

Den 30.01.2019 10.43, skrev Gerd Hoffmann:
> Add 3d resource parameters to virtio_gpu_object_params struct.  With
> that in place we can use it for virtio_gpu_cmd_resource_create_3d()
> calls.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

You don't remove the struct virtio_gpu_resource_create_3d definition,
but it looks like there's no users left?

Noralf.

>  drivers/gpu/drm/virtio/virtgpu_drv.h   | 10 +-
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 25 ++---
>  drivers/gpu/drm/virtio/virtgpu_vq.c| 16 +---
>  3 files changed, 32 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
> b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index a40215c10e..3265e62725 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -56,6 +56,14 @@ struct virtio_gpu_object_params {
>   uint32_t height;
>   unsigned long size;
>   bool pinned;
> + /* 3d */
> + uint32_t target;
> + uint32_t bind;
> + uint32_t depth;
> + uint32_t array_size;
> + uint32_t last_level;
> + uint32_t nr_samples;
> + uint32_t flags;
>  };
>  
>  struct virtio_gpu_object {
> @@ -310,7 +318,7 @@ void virtio_gpu_cmd_transfer_to_host_3d(struct 
> virtio_gpu_device *vgdev,
>  void
>  virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
> struct virtio_gpu_object *bo,
> -   struct virtio_gpu_resource_create_3d *rc_3d);
> +   struct virtio_gpu_object_params *params);
>  void virtio_gpu_ctrl_ack(struct virtqueue *vq);
>  void virtio_gpu_cursor_ack(struct virtqueue *vq);
>  void virtio_gpu_fence_ack(struct virtqueue *vq);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c 
> b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 84c2216fd4..431e5d767e 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -283,7 +283,6 @@ static int virtio_gpu_resource_create_ioctl(struct 
> drm_device *dev, void *data,
>   struct ttm_validate_buffer mainbuf;
>   struct virtio_gpu_fence *fence = NULL;
>   struct ww_acquire_ctx ticket;
> - struct virtio_gpu_resource_create_3d rc_3d;
>   struct virtio_gpu_object_params params = { 0 };
>  
>   if (vgdev->has_virgl_3d == false) {
> @@ -307,7 +306,15 @@ static int virtio_gpu_resource_create_ioctl(struct 
> drm_device *dev, void *data,
>   params.width = rc->width;
>   params.height = rc->height;
>   params.size = rc->size;
> -
> + if (vgdev->has_virgl_3d) {
> + params.target = rc->target;
> + params.bind = rc->bind;
> + params.depth = rc->depth;
> + params.array_size = rc->array_size;
> + params.last_level = rc->last_level;
> + params.nr_samples = rc->nr_samples;
> + params.flags = rc->flags;
> + }
>   /* allocate a single page size object */
>   if (params.size == 0)
>   params.size = PAGE_SIZE;
> @@ -333,25 +340,13 @@ static int virtio_gpu_resource_create_ioctl(struct 
> drm_device *dev, void *data,
>   goto fail_unref;
>   }
>  
> - rc_3d.resource_id = cpu_to_le32(qobj->hw_res_handle);
> - rc_3d.target = cpu_to_le32(rc->target);
> - rc_3d.format = cpu_to_le32(rc->format);
> - rc_3d.bind = cpu_to_le32(rc->bind);
> - rc_3d.width = cpu_to_le32(rc->width);
> - rc_3d.height = cpu_to_le32(rc->height);
> - rc_3d.depth = cpu_to_le32(rc->depth);
> - rc_3d.array_size = cpu_to_le32(rc->array_size);
> - rc_3d.last_level = cpu_to_le32(rc->last_level);
> - rc_3d.nr_samples = cpu_to_le32(rc->nr_samples);
> - rc_3d.flags = cpu_to_le32(rc->flags);
> -
>   fence = virtio_gpu_fence_alloc(vgdev);
>   if (!fence) {
>   ret = -ENOMEM;
>   goto fail_backoff;
>   }
>  
> - virtio_gpu_cmd_resource_create_3d(vgdev, qobj, _3d);
> + virtio_gpu_cmd_resource_create_3d(vgdev, qobj, );
>   ret = virtio_gpu_object_attach(vgdev, qobj, fence);
>   if (ret) {
>   dma_fence_put(>f);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c 
> b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index 363b8b8577..ca93ec6ca3 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -826,7 +826,7 @@ void virtio_gpu_cmd_context_detach_resource(struct 
> virtio_gpu_device *vgdev,
>  void
>  virtio_gpu_cmd_resource_create_3d(struct virtio_gpu_device *vgdev,
> struct virtio_gpu_object *bo,
> -   struct virtio_gpu_resource_create_3d *rc_3d)
> +   struct virtio_gpu_object_params *params)
>  {
>   struct 

Re: [PATCH v2 3/6] drm/virtio: params struct for virtio_gpu_cmd_create_resource()

[Noralf Trønnes]

Den 30.01.2019 10.43, skrev Gerd Hoffmann:
> Add format, width and height fields to the virtio_gpu_object_params
> struct.  With that in place we can use the parameter struct for
> virtio_gpu_cmd_create_resource() calls too.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v2 2/6] drm/virtio: use struct to pass params to virtio_gpu_object_create()

[Noralf Trønnes]

gt; + obj = virtio_gpu_alloc_object(dev, params);
>   if (IS_ERR(obj))
>   return PTR_ERR(obj);
>  
> @@ -85,6 +85,7 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
>   struct virtio_gpu_device *vgdev = dev->dev_private;
>   struct drm_gem_object *gobj;
>   struct virtio_gpu_object *obj;
> + struct virtio_gpu_object_params params = { 0 };
>   int ret;
>   uint32_t pitch;
>   uint32_t format;
> @@ -96,7 +97,9 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
>   args->size = pitch * args->height;
>   args->size = ALIGN(args->size, PAGE_SIZE);
>  
> - ret = virtio_gpu_gem_create(file_priv, dev, args->size, ,
> + params.pinned = false,

You have a comma here, but assigning to false isn't really necessary
since the struct is zeroed. Same goes for the same assignment further down.

With this fixed in some way:

Acked-by: Noralf Trønnes 

> + params.size = args->size;
> + ret = virtio_gpu_gem_create(file_priv, dev, , ,
>   >handle);
>   if (ret)
>   goto fail;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c 
> b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 14ce8188c0..fa7b958ca2 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -279,12 +279,12 @@ static int virtio_gpu_resource_create_ioctl(struct 
> drm_device *dev, void *data,
>   struct virtio_gpu_object *qobj;
>   struct drm_gem_object *obj;
>   uint32_t handle = 0;
> - uint32_t size;
>   struct list_head validate_list;
>   struct ttm_validate_buffer mainbuf;
>   struct virtio_gpu_fence *fence = NULL;
>   struct ww_acquire_ctx ticket;
>   struct virtio_gpu_resource_create_3d rc_3d;
> + struct virtio_gpu_object_params params = { 0 };
>  
>   if (vgdev->has_virgl_3d == false) {
>   if (rc->depth > 1)
> @@ -302,13 +302,14 @@ static int virtio_gpu_resource_create_ioctl(struct 
> drm_device *dev, void *data,
>   INIT_LIST_HEAD(_list);
>   memset(, 0, sizeof(struct ttm_validate_buffer));
>  
> - size = rc->size;
> + params.pinned = false,
> + params.size = rc->size;
>  
>   /* allocate a single page size object */
> - if (size == 0)
> - size = PAGE_SIZE;
> + if (params.size == 0)
> + params.size = PAGE_SIZE;
>  
> - qobj = virtio_gpu_alloc_object(dev, size, false, false);
> + qobj = virtio_gpu_alloc_object(dev, );
>   if (IS_ERR(qobj))
>   return PTR_ERR(qobj);
>   obj = >gem_base;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c 
> b/drivers/gpu/drm/virtio/virtgpu_object.c
> index f39a183d59..62367e3f80 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -79,21 +79,16 @@ static void virtio_gpu_init_ttm_placement(struct 
> virtio_gpu_object *vgbo,
>  }
>  
>  int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
> -  unsigned long size, bool kernel, bool pinned,
> +  struct virtio_gpu_object_params *params,
>struct virtio_gpu_object **bo_ptr)
>  {
>   struct virtio_gpu_object *bo;
> - enum ttm_bo_type type;
>   size_t acc_size;
>   int ret;
>  
> - if (kernel)
> - type = ttm_bo_type_kernel;
> - else
> - type = ttm_bo_type_device;
>   *bo_ptr = NULL;
>  
> - acc_size = ttm_bo_dma_acc_size(>mman.bdev, size,
> + acc_size = ttm_bo_dma_acc_size(>mman.bdev, params->size,
>  sizeof(struct virtio_gpu_object));
>  
>   bo = kzalloc(sizeof(struct virtio_gpu_object), GFP_KERNEL);
> @@ -104,19 +99,20 @@ int virtio_gpu_object_create(struct virtio_gpu_device 
> *vgdev,
>   kfree(bo);
>   return ret;
>   }
> - size = roundup(size, PAGE_SIZE);
> - ret = drm_gem_object_init(vgdev->ddev, >gem_base, size);
> + params->size = roundup(params->size, PAGE_SIZE);
> + ret = drm_gem_object_init(vgdev->ddev, >gem_base, params->size);
>   if (ret != 0) {
>   virtio_gpu_resource_id_put(vgdev, bo->hw_res_handle);
>   kfree(bo);
>   return ret;
>   }
>   bo->dumb = false;
> - virtio_gpu_init_ttm_placement(bo, pinned);
> + virtio_gpu_init_ttm_placement(bo, params->pinned);
>  
> - ret = ttm_bo_init(>mman.bdev, >tbo, size, type,
> -   >placement, 0, !kernel, acc_size,
> -   NULL, NULL, _gpu_ttm_bo_destroy);
> + ret = ttm_bo_init(>mman.bdev, >tbo, params->size,
> +   ttm_bo_type_device, >placement, 0,
> +   true, acc_size, NULL, NULL,
> +   _gpu_ttm_bo_destroy);
>   /* ttm_bo_init failure will call the destroy */
>   if (ret != 0)
>   return ret;
> 

Re: [PATCH v2 1/6] drm/virtio: move virtio_gpu_object_{attach, detach} calls.

[Noralf Trønnes]

Den 30.01.2019 10.43, skrev Gerd Hoffmann:
> Drop the dummy ttm backend implementation, add a real one for
> TTM_PL_FLAG_TT objects.  The bin/unbind callbacks will call
> virtio_gpu_object_{attach,detach}, to update the object state
> on the host side, instead of invoking those calls from the
> move_notify() callback.
> 
> With that in place the move and move_notify callbacks are not
> needed any more, so drop them.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v2] drm/qxl: use ttm_tt

[Noralf Trønnes]

Den 29.01.2019 09.25, skrev Gerd Hoffmann:
> qxl device will not dma, so we don't need ttm_dma_tt.  Go use ttm_tt
> instead, to avoid wasting resources (swiotlb bounce buffers for
> example).
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 17/23] drm/qxl: use generic fbdev emulation

[Noralf Trønnes]

Den 28.01.2019 09.59, skrev Gerd Hoffmann:
> On Fri, Jan 25, 2019 at 06:25:27PM +0100, Noralf Trønnes wrote:
>>
>>
>> Den 18.01.2019 13.20, skrev Gerd Hoffmann:
>>> Switch qxl over to the new generic fbdev emulation.
>>>
>>> Signed-off-by: Gerd Hoffmann 
>>> ---
>>>  drivers/gpu/drm/qxl/qxl_display.c | 7 ---
>>>  drivers/gpu/drm/qxl/qxl_drv.c | 2 ++
>>>  2 files changed, 2 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
>>> b/drivers/gpu/drm/qxl/qxl_display.c
>>> index ef832f98ab..9c751f01e3 100644
>>> --- a/drivers/gpu/drm/qxl/qxl_display.c
>>> +++ b/drivers/gpu/drm/qxl/qxl_display.c
>>> @@ -1221,18 +1221,11 @@ int qxl_modeset_init(struct qxl_device *qdev)
>>> qxl_display_read_client_monitors_config(qdev);
>>>  
>>> drm_mode_config_reset(>ddev);
>>> -
>>> -   /* primary surface must be created by this point, to allow
>>> -* issuing command queue commands and having them read by
>>> -* spice server. */
>>> -   qxl_fbdev_init(qdev);
>>> return 0;
>>>  }
>>>  
>>>  void qxl_modeset_fini(struct qxl_device *qdev)
>>>  {
>>> -   qxl_fbdev_fini(qdev);
>>> -
>>> qxl_destroy_monitors_object(qdev);
>>> drm_mode_config_cleanup(>ddev);
>>>  }
>>> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
>>> index 13c8a662f9..3fce7d16df 100644
>>> --- a/drivers/gpu/drm/qxl/qxl_drv.c
>>> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
>>> @@ -93,6 +93,8 @@ qxl_pci_probe(struct pci_dev *pdev, const struct 
>>> pci_device_id *ent)
>>> if (ret)
>>> goto modeset_cleanup;
>>>  
>>> +   drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
>>
>> I couldn't find that this was part of old fbdev code, so it would be
>> nice to mention it in the commit message.
> 
> It actually is, but a bit hidden because it doesn't use a helper you can
> easily grep for.  Instead sets fb_info->apertures->ranges[0] in
> qxlfb_create(), which has the same effect.
> 

Indeed,

Acked-by: Noralf Trønnes 

Re: [PATCH v3 10/23] drm/qxl: move qxl_primary_apply_cursor to correct place

[Noralf Trønnes]

Den 28.01.2019 09.10, skrev Gerd Hoffmann:
>>> The cursor must be set again after creating the primary surface.
>>> Also drop the error message.
> 
>>> if (!bo->is_primary) {
>>> -   if (!same_shadow)
>>> +   if (!same_shadow) {
>>> qxl_io_create_primary(qdev, 0, bo);
>>> +   qxl_primary_apply_cursor(plane);
>>> +   }
>>> bo->is_primary = true;
>>> }
>>>  
>>>
>>
>> I don't see how the commit message matches what you're doing. It gives
>> the impression that it must be applied under yet another condition, but
>> the condition for applying the cursor is changed from bo_old->is_primary
>> to !bo->is_primary.
> 
> The qxl device ties the cursor to the primary surface.  Therefore
> calling qxl_io_destroy_primary() and qxl_io_create_primary() to switch
> the framebuffer causes the cursor information being lost and the driver
> must re-apply it.
> 
> The correct call order to do that is qxl_io_destroy_primary() +
> qxl_io_create_primary() + qxl_primary_apply_cursor().
> 
> The old code did qxl_io_destroy_primary() + qxl_primary_apply_cursor() +
> qxl_io_create_primary().  Due to qxl_primary_apply_cursor request being
> queued in a ringbuffer and qxl_io_create_primary() trapping to the
> hypervisor instantly there is a high chance that qxl_io_create_primary()
> is processed first even with the wrong call order.  But it's racy and
> thus not reliable.
> 
>> It probably makes sense to someone that knows the driver.
> 
> If the above explains things better to you I should probably replace the
> commit message with that.
> 

This is actually my first review of a driver that I'm not familiar with.
I'm not quite sure how much in depth understanding that is required to
put my ack on it. Going further into the patchset I realised that
there's no way that I can verify the logic without being intimate with
the driver. So I have tried to verify things from a kms point of view.

I liked your expanded explanation better.

Noralf.

>> Acked-by: Noralf Trønnes 
> 
> thanks,
>   Gerd
> 

Re: [PATCH v3 18/23] drm/qxl: remove dead qxl fbdev emulation code

[Noralf Trønnes]

Den 25.01.2019 19.10, skrev Sam Ravnborg:
> Hi Noralf.
> 
>>> Lovely diffstat, thanks to the new generic fbdev emulation.
>>>
>>>  drm/qxl/Makefile   |2
>>>  drm/qxl/qxl_draw.c |  232 
>>>  drm/qxl/qxl_drv.h  |   21 ---
>>>  drm/qxl/qxl_fb.c   |  300 
>>> -
>>>
>>> Signed-off-by: Gerd Hoffmann 
>>> ---
>>>  drivers/gpu/drm/qxl/qxl_drv.h  |  21 ---
>>>  drivers/gpu/drm/qxl/qxl_draw.c | 232 ---
>>>  drivers/gpu/drm/qxl/qxl_fb.c   | 300 
>>> -
>>>  drivers/gpu/drm/qxl/Makefile   |   2 +-
>>>  4 files changed, 1 insertion(+), 554 deletions(-)
>>>  delete mode 100644 drivers/gpu/drm/qxl/qxl_fb.c
>>>
>>
>> Nice!
>>
>> Acked-by: Noralf Trønnes 
> It must be a very satisfying experience to see such benefits
> of the many hours of works you have put into the genric
> fbdev emulation code.

Indeed and having more users of the code increases the chance of
detecting any bugs lurking underneath the surface.

> Well done, and indeed a nice patch from Gerd.
> 
>   Sam
> 

Re: [PATCH v3 22/23] drm/qxl: use kernel mode db

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Add all standard modes from the kernel's video mode data base.
> Keep a few non-standard modes in the qxl mode list.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 21/23] drm/qxl: add qxl_add_mode helper function

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Add a helper function to add custom video modes to a connector.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 20/23] drm/qxl: add mode/framebuffer check functions

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Add a helper functions to check video modes.  Also add a helper to check
> framebuffer buffer objects, using the former for consistency.  That way
> we should not fail in qxl_primary_atomic_check() because video modes
> which are too big will not be added to the mode list in the first place.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 19/23] drm/qxl: implement qxl_gem_prime_(un)pin

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 18/23] drm/qxl: remove dead qxl fbdev emulation code

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Lovely diffstat, thanks to the new generic fbdev emulation.
> 
>  drm/qxl/Makefile   |2
>  drm/qxl/qxl_draw.c |  232 
>  drm/qxl/qxl_drv.h  |   21 ---
>  drm/qxl/qxl_fb.c   |  300 
> -
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/qxl/qxl_drv.h  |  21 ---
>  drivers/gpu/drm/qxl/qxl_draw.c | 232 ---
>  drivers/gpu/drm/qxl/qxl_fb.c   | 300 
> -
>  drivers/gpu/drm/qxl/Makefile   |   2 +-
>  4 files changed, 1 insertion(+), 554 deletions(-)
>  delete mode 100644 drivers/gpu/drm/qxl/qxl_fb.c
> 

Nice!

Acked-by: Noralf Trønnes 

Re: [PATCH v3 17/23] drm/qxl: use generic fbdev emulation

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Switch qxl over to the new generic fbdev emulation.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 7 ---
>  drivers/gpu/drm/qxl/qxl_drv.c | 2 ++
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
> b/drivers/gpu/drm/qxl/qxl_display.c
> index ef832f98ab..9c751f01e3 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -1221,18 +1221,11 @@ int qxl_modeset_init(struct qxl_device *qdev)
>   qxl_display_read_client_monitors_config(qdev);
>  
>   drm_mode_config_reset(>ddev);
> -
> - /* primary surface must be created by this point, to allow
> -  * issuing command queue commands and having them read by
> -  * spice server. */
> - qxl_fbdev_init(qdev);
>   return 0;
>  }
>  
>  void qxl_modeset_fini(struct qxl_device *qdev)
>  {
> - qxl_fbdev_fini(qdev);
> -
>   qxl_destroy_monitors_object(qdev);
>   drm_mode_config_cleanup(>ddev);
>  }
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index 13c8a662f9..3fce7d16df 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -93,6 +93,8 @@ qxl_pci_probe(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
>   if (ret)
>   goto modeset_cleanup;
>  
> + drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");

I couldn't find that this was part of old fbdev code, so it would be
nice to mention it in the commit message.

Acked-by: Noralf Trønnes 


> + drm_fbdev_generic_setup(>ddev, 32);
>   return 0;
>  
>  modeset_cleanup:
> 

Re: [PATCH v3 16/23] drm/qxl: implement prime kmap/kunmap

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Generic fbdev emulation needs this.  Also: We must keep track of the
> number of mappings now, so we don't unmap early in case two users want a
> kmap of the same bo.  Add a sanity check to destroy callback to make
> sure kmap/kunmap is balanced.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Just a note: You catch the one-to-many kmap type of unbalance, but not
the one-too-many kunmap situation.

Acked-by: Noralf Trønnes 


>  drivers/gpu/drm/qxl/qxl_drv.h|  1 +
>  drivers/gpu/drm/qxl/qxl_object.c |  6 ++
>  drivers/gpu/drm/qxl/qxl_prime.c  | 17 +
>  3 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> index 43c6df9cf9..8c3af1cdbe 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -84,6 +84,7 @@ struct qxl_bo {
>   struct ttm_bo_kmap_obj  kmap;
>   unsigned int pin_count;
>   void*kptr;
> + unsigned intmap_count;
>   int type;
>  
>   /* Constant after initialization */
> diff --git a/drivers/gpu/drm/qxl/qxl_object.c 
> b/drivers/gpu/drm/qxl/qxl_object.c
> index 024c8dd317..4928fa6029 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.c
> +++ b/drivers/gpu/drm/qxl/qxl_object.c
> @@ -36,6 +36,7 @@ static void qxl_ttm_bo_destroy(struct ttm_buffer_object 
> *tbo)
>   qdev = (struct qxl_device *)bo->gem_base.dev->dev_private;
>  
>   qxl_surface_evict(qdev, bo, false);
> + WARN_ON_ONCE(bo->map_count > 0);
>   mutex_lock(>gem.mutex);
>   list_del_init(>list);
>   mutex_unlock(>gem.mutex);
> @@ -131,6 +132,7 @@ int qxl_bo_kmap(struct qxl_bo *bo, void **ptr)
>   if (bo->kptr) {
>   if (ptr)
>   *ptr = bo->kptr;
> + bo->map_count++;
>   return 0;
>   }
>   r = ttm_bo_kmap(>tbo, 0, bo->tbo.num_pages, >kmap);
> @@ -139,6 +141,7 @@ int qxl_bo_kmap(struct qxl_bo *bo, void **ptr)
>   bo->kptr = ttm_kmap_obj_virtual(>kmap, _iomem);
>   if (ptr)
>   *ptr = bo->kptr;
> + bo->map_count = 1;
>   return 0;
>  }
>  
> @@ -180,6 +183,9 @@ void qxl_bo_kunmap(struct qxl_bo *bo)
>  {
>   if (bo->kptr == NULL)
>   return;
> + bo->map_count--;
> + if (bo->map_count > 0)
> + return;
>   bo->kptr = NULL;
>   ttm_bo_kunmap(>kmap);
>  }
> diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
> index a55dece118..708378844c 100644
> --- a/drivers/gpu/drm/qxl/qxl_prime.c
> +++ b/drivers/gpu/drm/qxl/qxl_prime.c
> @@ -22,7 +22,7 @@
>   * Authors: Andreas Pokorny
>   */
>  
> -#include "qxl_drv.h"
> +#include "qxl_object.h"
>  
>  /* Empty Implementations as there should not be any other driver for a 
> virtual
>   * device that might share buffers with qxl */
> @@ -54,13 +54,22 @@ struct drm_gem_object *qxl_gem_prime_import_sg_table(
>  
>  void *qxl_gem_prime_vmap(struct drm_gem_object *obj)
>  {
> - WARN_ONCE(1, "not implemented");
> - return ERR_PTR(-ENOSYS);
> + struct qxl_bo *bo = gem_to_qxl_bo(obj);
> + void *ptr;
> + int ret;
> +
> + ret = qxl_bo_kmap(bo, );
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + return ptr;
>  }
>  
>  void qxl_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
>  {
> - WARN_ONCE(1, "not implemented");
> + struct qxl_bo *bo = gem_to_qxl_bo(obj);
> +
> + qxl_bo_kunmap(bo);
>  }
>  
>  int qxl_gem_prime_mmap(struct drm_gem_object *obj,
> 

Re: [PATCH v3 15/23] drm/qxl: use qxl_num_crtc directly

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> qdev->monitors_config->max_allowed is effectively set by the
> qxl.num_heads module parameter, stored in the qxl_num_crtc variable.
> Lets get rid of the indirection and use the variable qxl_num_crtc
> directly.  The kernel doesn't need to dereference pointers each time it
> needs the value, and when reading the code you don't have to trace where
> and why qdev->monitors_config->max_allowed is set.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 14/23] drm/qxl: cover all crtcs in shadow bo.

[Noralf Trønnes]

xl_bo_create(qdev, user_bo->gem_base.size,
> -   true, true, QXL_GEM_DOMAIN_SURFACE, NULL,
> -   _bo->shadow);
> - user_bo->shadow->surf = user_bo->surf;
> + if (user_bo->shadow != qdev->dumb_shadow_bo) {
> + if (user_bo->shadow) {
> + drm_gem_object_put_unlocked
> + (_bo->shadow->gem_base);
> + user_bo->shadow = NULL;
> + }
> + drm_gem_object_get(>dumb_shadow_bo->gem_base);
> + user_bo->shadow = qdev->dumb_shadow_bo;
>   }
>   }
>  
> @@ -773,7 +838,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,
>   user_bo = gem_to_qxl_bo(obj);
>   qxl_bo_unpin(user_bo);
>  
> - if (user_bo->shadow && !user_bo->shadow->is_primary) {
> + if (old_state->fb != plane->state->fb && user_bo->shadow) {
>   drm_gem_object_put_unlocked(_bo->shadow->gem_base);
>   user_bo->shadow = NULL;
>   }
> @@ -1106,6 +1171,8 @@ int qxl_create_monitors_object(struct qxl_device *qdev)
>  
>   memset(qdev->monitors_config, 0, monitors_config_size);
>   qdev->monitors_config->max_allowed = max_allowed;
> +
> + qdev->dumb_heads = kcalloc(max_allowed, sizeof(qdev->dumb_heads[0]), 
> GFP_KERNEL);

Needs an allocation failure check. With that:

Acked-by: Noralf Trønnes 


>   return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
> index c408bb83c7..5313ad21c1 100644
> --- a/drivers/gpu/drm/qxl/qxl_draw.c
> +++ b/drivers/gpu/drm/qxl/qxl_draw.c
> @@ -267,7 +267,8 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
>  struct qxl_bo *bo,
>  unsigned int flags, unsigned int color,
>  struct drm_clip_rect *clips,
> -unsigned int num_clips, int inc)
> +unsigned int num_clips, int inc,
> +uint32_t dumb_shadow_offset)
>  {
>   /*
>* TODO: if flags & DRM_MODE_FB_DIRTY_ANNOTATE_FILL then we should
> @@ -295,6 +296,9 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
>   if (ret)
>   return;
>  
> + clips->x1 += dumb_shadow_offset;
> + clips->x2 += dumb_shadow_offset;
> +
>   left = clips->x1;
>   right = clips->x2;
>   top = clips->y1;
> @@ -342,7 +346,8 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
>   goto out_release_backoff;
>  
>   ret = qxl_image_init(qdev, release, dimage, surface_base,
> -  left, top, width, height, depth, stride);
> +  left - dumb_shadow_offset,
> +  top, width, height, depth, stride);
>   qxl_bo_kunmap(bo);
>   if (ret)
>   goto out_release_backoff;
> 

Re: [PATCH v3 13/23] drm/qxl: use shadow bo directly

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Pass the shadow bo to qxl_io_create_primary() instead of expecting
> qxl_io_create_primary to check bo->shadow.  Set is_primary flag on the
> shadow bo.  Move the is_primary tracking into qxl_io_create_primary()
> and qxl_io_destroy_primary() functions.
> 
> That simplifies primary surface tracking and the workflow in
> qxl_primary_atomic_update().
> 
> Signed-off-by: Gerd Hoffmann 
> 
> qxl_io_create/destroy_primary: primary_bo tracking [fixup]
> ---

I'm unable to follow the logic, but I didn't spot anything suspicious
looking.

Acked-by: Noralf Trønnes 

Re: [PATCH v3 12/23] drm/qxl: track primary bo

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Track which bo is used as primary surface.  With that in place we don't
> need the primary_created flag any more, we can just check the primary bo
> pointer instead.
> 
> Also verify we don't already have a primary surface in
> qxl_io_create_primary().
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 11/23] drm/qxl: drop unused offset parameter from qxl_io_create_primary()

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 10/23] drm/qxl: move qxl_primary_apply_cursor to correct place

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> The cursor must be set again after creating the primary surface.
> Also drop the error message.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c 
> b/drivers/gpu/drm/qxl/qxl_display.c
> index 86bfc19bea..1b700ef503 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -533,7 +533,6 @@ static void qxl_primary_atomic_update(struct drm_plane 
> *plane,
>   .x2 = plane->state->fb->width,
>   .y2 = plane->state->fb->height
>   };
> - int ret;
>   bool same_shadow = false;
>  
>   if (old_state->fb) {
> @@ -554,16 +553,13 @@ static void qxl_primary_atomic_update(struct drm_plane 
> *plane,
>   if (!same_shadow)
>   qxl_io_destroy_primary(qdev);
>   bo_old->is_primary = false;
> -
> - ret = qxl_primary_apply_cursor(plane);
> - if (ret)
> - DRM_ERROR(
> - "could not set cursor after creating primary");
>   }
>  
>   if (!bo->is_primary) {
> - if (!same_shadow)
> + if (!same_shadow) {
>   qxl_io_create_primary(qdev, 0, bo);
> + qxl_primary_apply_cursor(plane);
> + }
>   bo->is_primary = true;
>   }
>  
> 

I don't see how the commit message matches what you're doing. It gives
the impression that it must be applied under yet another condition, but
the condition for applying the cursor is changed from bo_old->is_primary
to !bo->is_primary.
It probably makes sense to someone that knows the driver.

Acked-by: Noralf Trønnes 

Re: [PATCH v3 08/23] drm/qxl: use QXL_GEM_DOMAIN_SURFACE for shadow bo.

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> The shadow bo is used as qxl surface, so allocate it as
> QXL_GEM_DOMAIN_SURFACE.  Should usually be allocated in
> PRIV ttm domain then, so this reduces VRAM memory pressure.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 07/23] drm/qxl: allow both PRIV and VRAM placement for QXL_GEM_DOMAIN_SURFACE

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> qxl surfaces (used for framebuffers and gem objects) can live in both
> VRAM and PRIV ttm domains.  Update placement setup to include both.
> Put PRIV first in the list so it is preferred, so VRAM will have more
> room for objects which must be allocated there.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 09/23] drm/qxl: use QXL_GEM_DOMAIN_SURFACE for dumb gem objects

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> dumb buffers are used as qxl surfaces, so allocate them as
> QXL_GEM_DOMAIN_SURFACE.  Should usually be allocated in
> PRIV ttm domain then, so this reduces VRAM memory pressure.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 06/23] drm/qxl: use separate offset spaces for the two slots / ttm memory types.

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Without that ttm offsets are not unique, they can refer to objects
> in both VRAM and PRIV memory (aka main and surfaces slot).
> 
> One of those "why things didn't blow up without this" moments.
> Probably offset conflicts are rare enough by pure luck.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 05/23] drm/qxl: drop unused fields from struct qxl_device

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> slot_id_bits and slot_gen_bits can be read directly from qxlrom instead.
> va_slot_mask is never used anywhere.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 04/23] drm/qxl: change the way slot is detected

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> From: Frediano Ziglio 
> 
> Instead of relaying on surface type use the actual placement.
> This allow to have different placement for a single type of
> surface.
> 
> Signed-off-by: Frediano Ziglio 
> 
> [ kraxel: rebased, adapted to upstream changes ]
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 03/23] drm/qxl: simplify slot management

[Noralf Trønnes]

Den 18.01.2019 13.20, skrev Gerd Hoffmann:
> Drop pointless indirection, remove the mem_slots array and index
> variables, drop dynamic allocation.  Store memslots in qxl_device
> instead.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Looks sane:

Acked-by: Noralf Trønnes 

Re: [PATCH v3 02/23] drm/qxl: drop unused qxl_fb_virtual_address

[Noralf Trønnes]

Den 18.01.2019 13.19, skrev Gerd Hoffmann:
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v3 01/23] drm/qxl: drop ttm_mem_reg arg from qxl_hw_surface_alloc()

[Noralf Trønnes]

Den 18.01.2019 13.19, skrev Gerd Hoffmann:
> Not used, is always NULL.
> 
> Signed-off-by: Gerd Hoffmann 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH V7 2/2] drm/panel: Add Sitronix ST7701 panel driver

[Noralf Trønnes]

Den 11.01.2019 22.19, skrev Sam Ravnborg:
> Hi Jagan.
> 
> Gave this another more detailed read - triggered some additional comments.
> Depite the comments it looks good, and this is all more or
> less cosmetic improvements.
> 
>   Sam
> 
>> +struct st7701_panel_desc {
>> +const struct drm_display_mode *mode;
>> +unsigned int lanes;
>> +unsigned long flags;
>> +enum mipi_dsi_pixel_format format;
>> +const char *const *supply_names;
>> +unsigned int num_supplies;
>> +unsigned int panel_sleep_delay;
>> +};
>> +
>> +struct st7701 {
>> +struct drm_panel panel;
>> +struct mipi_dsi_device *dsi;
>> +const struct st7701_panel_desc *desc;
>> +
>> +struct backlight_device *backlight;
>> +struct regulator_bulk_data *supplies;
>> +unsigned int num_supplies;
> I cannot see that num_supplies in this struct are used?
> 
> 
>> +struct gpio_desc *reset;
>> +unsigned int sleep_delay;
>> +};
>> +
>> +static inline struct st7701 *panel_to_st7701(struct drm_panel *panel)
>> +{
>> +return container_of(panel, struct st7701, panel);
>> +}
>> +
>> +static inline int st7701_dsi_write(struct st7701 *st7701, const void *seq,
>> +   size_t len)
>> +{
>> +return mipi_dsi_dcs_write_buffer(st7701->dsi, seq, len);
>> +}
>> +
> 
> 
>> +static int st7701_prepare(struct drm_panel *panel)
>> +{
>> +struct st7701 *st7701 = panel_to_st7701(panel);
>> +int ret;
>> +
>> +gpiod_set_value(st7701->reset, 0);
>> +msleep(20);
>> +
>> +ret = regulator_bulk_enable(st7701->desc->num_supplies,
>> +st7701->supplies);
>> +if (ret < 0)
>> +return ret;
>> +msleep(20);
>> +
>> +gpiod_set_value(st7701->reset, 1);
>> +msleep(20);
>> +
>> +gpiod_set_value(st7701->reset, 0);
>> +msleep(30);
>> +
>> +gpiod_set_value(st7701->reset, 1);
>> +msleep(150);
>> +
>> +st7701_init_sequence(st7701);
>> +
>> +return 0;
>> +}
>> +
> 
>> +static int st7701_unprepare(struct drm_panel *panel)
>> +{
>> +struct st7701 *st7701 = panel_to_st7701(panel);
>> +
>> +ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00);
>> +
>> +msleep(st7701->sleep_delay);
>> +
>> +gpiod_set_value(st7701->reset, 0);
>> +
>> +gpiod_set_value(st7701->reset, 1);
>> +
>> +gpiod_set_value(st7701->reset, 0);
> No timing constrains here? In prepare there are sleeps intermixed.
> 
>> +
>> +regulator_bulk_disable(st7701->desc->num_supplies, st7701->supplies);
>> +
>> +return 0;
>> +}
>> +
>> +static int st7701_get_modes(struct drm_panel *panel)
>> +{
>> +struct st7701 *st7701 = panel_to_st7701(panel);
>> +const struct drm_display_mode *desc_mode = st7701->desc->mode;
>> +struct drm_display_mode *mode;
>> +
>> +mode = drm_mode_duplicate(panel->drm, desc_mode);
>> +if (!mode) {
>> +dev_err(>dsi->dev, "failed to add mode %ux%ux@%u\n",
>> +desc_mode->hdisplay, desc_mode->vdisplay,
>> +desc_mode->vrefresh);
> Use something like:
>   DRM_DEV_ERROR(st7701->dev, "failed to add mode" DRM_MODE_FMT ",
> DRM_MODE_ARG(desc_mode));
> 
>> +return -ENOMEM;
>> +}
>> +
>> +drm_mode_set_name(mode);
>> +drm_mode_probed_add(panel->connector, mode);
>> +
>> +panel->connector->display_info.width_mm = desc_mode->width_mm;
>> +panel->connector->display_info.height_mm = desc_mode->height_mm;
>> +
>> +return 1;
>> +}
>> +
> 
>> +static const struct st7701_panel_desc ts8550b_desc = {
>> +.mode = _mode,
>> +.lanes = 2,
>> +.flags = MIPI_DSI_MODE_VIDEO,
>> +.format = MIPI_DSI_FMT_RGB888,
>> +.supply_names = ts8550b_supply_names,
>> +.num_supplies = ARRAY_SIZE(ts8550b_supply_names),
>> +.panel_sleep_delay = 80, /* panel need extra 80ms for sleep out cmd */
> In the only place this is used there is added 120 ms too.
> Looks inconsistent - do we have the same delay twice?
> 
> 
>> +
>> +static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
>> +{
>> +const struct st7701_panel_desc *desc;
>> +struct device_node *np;
>> +struct st7701 *st7701;
>> +int ret, i;
>> +
>> +st7701 = devm_kzalloc(>dev, sizeof(*st7701), GFP_KERNEL);
>> +if (!st7701)
>> +return -ENOMEM;
>> +
>> +desc = of_device_get_match_data(>dev);
>> +dsi->mode_flags = desc->flags;
>> +dsi->format = desc->format;
>> +dsi->lanes = desc->lanes;
>> +
>> +st7701->supplies = devm_kcalloc(>dev, desc->num_supplies,
>> +sizeof(*st7701->supplies),
>> +GFP_KERNEL);
>> +if (!st7701->supplies)
>> +return -ENOMEM;
>> +
>> +for (i = 0; i < desc->num_supplies; i++)
>> +st7701->supplies[i].supply = desc->supply_names[i];
>> +
>> +ret = devm_regulator_bulk_get(>dev, desc->num_supplies,
>> +  

Re: [PATCH] drm/mediatek: Add MTK Framebuffer-Device (mt7623)

[Noralf Trønnes]

Den 11.01.2019 11.25, skrev Daniel Vetter:
> On Fri, Jan 11, 2019 at 05:49:15PM +0800, CK Hu wrote:
>> Hi, Daniel:
>>
>> On Fri, 2019-01-11 at 10:20 +0100, Daniel Vetter wrote:
>>> On Fri, Jan 11, 2019 at 11:20:09AM +0800, CK Hu wrote:
 Hi, Daniel:

 On Thu, 2019-01-10 at 21:02 +0100, Daniel Vetter wrote:
> On Thu, Jan 10, 2019 at 08:01:37PM +0100, Frank Wunderlich wrote:
>> Hi Daniel,
>>
 Would be good to use the new generic fbdev emulation code here, for 
 even
 less code. Or at least know why this isn't possible to use for mtk (and
 maybe address that in the core code). Hand-rolling fbdev code 
 shouldn't be
 needed anymore.
>>>
>>> Back on the mailing list, no private replies please:
>>
>> i don't wanted to spam all people with dumb questions ;)
>
> There's no dumb questions, only insufficient documentation :-)
>
>>> For examples please grep for drm_fbdev_generic_setup(). There's also a
>>> still in-flight series from Gerd Hoffmann to convert over bochs. That,
>>> plus all the kerneldoc linked from there should get you started.
>>> -Daniel
>>
>> this is one of google best founds if i search for 
>> drm_fbdev_generic_setup:
>>
>> https://lkml.org/lkml/2018/12/19/305
>>
>> not very helpful...
>>
>> so i tried kernel-doc
>>
>> https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html?highlight=drm_fbdev_generic_setup#c.drm_fbdev_generic_setup
>>
>> which is nice function-reference but i've found no generic workflow
>>
>> as the posted driver is "only" a driver ported from kernel 4.4 by 
>> Alexander, i don't know if this new framework can be used and which 
>> parts need to be changed. I only try to bring his code Mainline
>> Maybe CK Hu can help here because driver is originally from him and he 
>> knows internals. Or maybe you can help here?
>>
>> i personally make my first steps as spare-time kernel-developer :)
>
> There's a ton of in-kernel users of that function already, I meant you can
> use those to serve as examples ... If those + the kerneldoc aren't
> good enough, then we need to improve them.
> -Daniel

 I've tried with drm_fbdev_generic_setup() and it works fine with simple
 modification. The patch is

 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
 +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
 @@ -16,6 +16,7 @@
  #include 
  #include 
  #include 
 +#include 
  #include 
  #include 
  #include 
 @@ -379,6 +380,7 @@ static void mtk_drm_kms_deinit(struct drm_device
 *drm)
 .gem_prime_get_sg_table = mtk_gem_prime_get_sg_table,
 .gem_prime_import_sg_table = mtk_gem_prime_import_sg_table,
 .gem_prime_mmap = mtk_drm_gem_mmap_buf,
 +   .gem_prime_vmap = mtk_drm_gem_prime_vmap,
 .ioctls = mtk_ioctls,
 .num_ioctls = ARRAY_SIZE(mtk_ioctls),
 .fops = _drm_fops,
 @@ -416,6 +418,8 @@ static int mtk_drm_bind(struct device *dev)
 if (ret < 0)
 goto err_deinit;

 +   drm_fbdev_generic_setup(drm, 32);
 +
 return 0;


 But I implement .gem_prime_vmap with a workaround,


 --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
 +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
 @@ -280,3 +280,8 @@ int mtk_gem_create_ioctl(struct drm_device *dev,
 void *data,
 mtk_drm_gem_free_object(_gem->base);
 return ret;
  }
 +
 +void *mtk_drm_gem_prime_vmap(struct drm_gem_object *obj)
 +{
 +   return (void *)1;
 +}

 Current drm_fb_helper depends on drm_client to create fbdev. When
 drm_client create drm_client_buffer, it need to vmap to get kernel
 vaddr. But I think for fbdev, the vaddr is useless. Do you agree that I
 temporarily implement .gem_prime_vmap in such way?
>>>
>>> The vmap is used by fbcon, without that fbdev won't really work I think.
>>> So I'm rather confused on what you tested ...
>>>
>>> Adding Noralf, he's the expert here.
>>> -Daniel
>>
>> I test with fb_test [1], it is a user space program just open /dev/fb0
>> and mmap it to user space. I does not turn on fbcon so everything looks
>> well. If support fbcon is essential, I would implement vmap correctly.
>> If it's not essential, I think I could return 0 when
>> CONFIG_FRAMBUFFER_CONSOLE is defined.
> 
> I think fbdev emulation without working fbcon is fairly useless. And it
> shouldn't be that much work to make it work, we have quite a few more
> helers for gem bo nowadays.

If the virtual address is not set, fbcon can't be used and that means it
has to be disabled using something like this in the driver Kconfig:
depends on !FRAMEBUFFER_CONSOLE

Otherwise it will crash if someone tries to use it.

The problem here is 

Re: [PATCH v3 14/15] drm/bochs: drop old fbdev emulation code

[Noralf Trønnes]

Den 10.01.2019 11.16, skrev Gerd Hoffmann:
>   Hi,
> 
>>> -   drm_fb_helper_set_suspend_unlocked(>fb.helper, 1);
>>
>> The generic fbdev emulation doesn't take care of suspend/resume. You
>> could do this:
>> drm_fb_helper_set_suspend_unlocked(drm_dev->fb_helper, 1);
> 
> Additional to drm_mode_config_helper_suspend() I assume?

No, sorry for the confusion. I didn't know if you could use
drm_mode_config_helper_suspend() or not. That function takes care of
fbdev suspend as well:

int drm_mode_config_helper_suspend(struct drm_device *dev)
{
<...>
drm_kms_helper_poll_disable(dev);
drm_fb_helper_set_suspend_unlocked(dev->fb_helper, 1);
state = drm_atomic_helper_suspend(dev);
<...>
}


> Does the call order matter?
> 
>>> drm_helper_resume_force_mode(drm_dev);
>>
>> The docs for this function has this to say:
>> * This function is deprecated. New drivers should implement atomic mode-
>> * setting and use the atomic suspend/resume helpers.
>>
>> Maybe you can use drm_mode_config_helper_suspend/resume() instead like
>> suggested in the drm_fbdev_generic_setup() docs.
> 
> That should work, yes.  The driver is simple enough ;)
> 
> cheers,
>   Gerd
> 

Re: [PATCH v3 14/15] drm/bochs: drop old fbdev emulation code

[Noralf Trønnes]

Den 10.01.2019 09.28, skrev Gerd Hoffmann:
> Not needed any more, bochs uses the generic emulation now.
> 
> Signed-off-by: Gerd Hoffmann 
> Reviewed-by: Oleksandr Andrushchenko 
> Acked-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/bochs/bochs.h   |   9 ---
>  drivers/gpu/drm/bochs/bochs_drv.c   |   6 --
>  drivers/gpu/drm/bochs/bochs_fbdev.c | 129 
> 
>  3 files changed, 144 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
> index ede22beb85..03711394f1 100644
> --- a/drivers/gpu/drm/bochs/bochs.h
> +++ b/drivers/gpu/drm/bochs/bochs.h
> @@ -80,12 +80,6 @@ struct bochs_device {
>   struct ttm_bo_device bdev;
>   bool initialized;
>   } ttm;
> -
> - /* fbdev */
> - struct {
> - struct drm_framebuffer *fb;
> - struct drm_fb_helper helper;
> - } fb;
>  };
>  
>  struct bochs_bo {
> @@ -157,7 +151,4 @@ int bochs_kms_init(struct bochs_device *bochs);
>  void bochs_kms_fini(struct bochs_device *bochs);
>  
>  /* bochs_fbdev.c */
> -int bochs_fbdev_init(struct bochs_device *bochs);
> -void bochs_fbdev_fini(struct bochs_device *bochs);
> -
>  extern const struct drm_mode_config_funcs bochs_mode_funcs;
> diff --git a/drivers/gpu/drm/bochs/bochs_drv.c 
> b/drivers/gpu/drm/bochs/bochs_drv.c
> index 1d86c0fb5f..a8968cd8d5 100644
> --- a/drivers/gpu/drm/bochs/bochs_drv.c
> +++ b/drivers/gpu/drm/bochs/bochs_drv.c
> @@ -102,12 +102,9 @@ static int bochs_pm_suspend(struct device *dev)
>  {
>   struct pci_dev *pdev = to_pci_dev(dev);
>   struct drm_device *drm_dev = pci_get_drvdata(pdev);
> - struct bochs_device *bochs = drm_dev->dev_private;
>  
>   drm_kms_helper_poll_disable(drm_dev);
>  
> - drm_fb_helper_set_suspend_unlocked(>fb.helper, 1);

The generic fbdev emulation doesn't take care of suspend/resume. You
could do this:
drm_fb_helper_set_suspend_unlocked(drm_dev->fb_helper, 1);

> -
>   return 0;
>  }
>  
> @@ -115,12 +112,9 @@ static int bochs_pm_resume(struct device *dev)
>  {
>   struct pci_dev *pdev = to_pci_dev(dev);
>   struct drm_device *drm_dev = pci_get_drvdata(pdev);
> - struct bochs_device *bochs = drm_dev->dev_private;
>  
>   drm_helper_resume_force_mode(drm_dev);

The docs for this function has this to say:
* This function is deprecated. New drivers should implement atomic mode-
* setting and use the atomic suspend/resume helpers.

Maybe you can use drm_mode_config_helper_suspend/resume() instead like
suggested in the drm_fbdev_generic_setup() docs.

Noralf.

>  
> - drm_fb_helper_set_suspend_unlocked(>fb.helper, 0);
> -
>   drm_kms_helper_poll_enable(drm_dev);
>   return 0;
>  }
> diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c 
> b/drivers/gpu/drm/bochs/bochs_fbdev.c
> index ccf783b038..7cac3f5253 100644
> --- a/drivers/gpu/drm/bochs/bochs_fbdev.c
> +++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
> @@ -11,124 +11,6 @@
>  
>  /* -- */
>  
> -static int bochsfb_mmap(struct fb_info *info,
> - struct vm_area_struct *vma)
> -{
> - struct drm_fb_helper *fb_helper = info->par;
> - struct bochs_bo *bo = gem_to_bochs_bo(fb_helper->fb->obj[0]);
> -
> - return ttm_fbdev_mmap(vma, >bo);
> -}
> -
> -static struct fb_ops bochsfb_ops = {
> - .owner = THIS_MODULE,
> - DRM_FB_HELPER_DEFAULT_OPS,
> - .fb_fillrect = drm_fb_helper_cfb_fillrect,
> - .fb_copyarea = drm_fb_helper_cfb_copyarea,
> - .fb_imageblit = drm_fb_helper_cfb_imageblit,
> - .fb_mmap = bochsfb_mmap,
> -};
> -
> -static int bochsfb_create_object(struct bochs_device *bochs,
> -  const struct drm_mode_fb_cmd2 *mode_cmd,
> -  struct drm_gem_object **gobj_p)
> -{
> - struct drm_device *dev = bochs->dev;
> - struct drm_gem_object *gobj;
> - u32 size;
> - int ret = 0;
> -
> - size = mode_cmd->pitches[0] * mode_cmd->height;
> - ret = bochs_gem_create(dev, size, true, );
> - if (ret)
> - return ret;
> -
> - *gobj_p = gobj;
> - return ret;
> -}
> -
> -static int bochsfb_create(struct drm_fb_helper *helper,
> -   struct drm_fb_helper_surface_size *sizes)
> -{
> - struct bochs_device *bochs =
> - container_of(helper, struct bochs_device, fb.helper);
> - struct fb_info *info;
> - struct drm_framebuffer *fb;
> - struct drm_mode_fb_cmd2 mode_cmd;
> - struct drm_gem_object *gobj = NULL;
> - struct bochs_bo *bo = NULL;
> - int size, ret;
> -
> - if (sizes->surface_bpp != 32)
> - return -EINVAL;
> -
> - mode_cmd.width = sizes->surface_width;
> - mode_cmd.height = sizes->surface_height;
> - mode_cmd.pitches[0] = sizes->surface_width * 4;
> - mode_cmd.pixel_format = DRM_FORMAT_HOST_XRGB;
> - size = mode_cmd.pitches[0] * mode_cmd.height;
> -
> - /* 

Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup

[Noralf Trønnes]

Den 05.01.2019 19.25, skrev Noralf Trønnes:
> 
> 
> Den 24.12.2018 16.03, skrev Peter Wu:
>> On Mon, Dec 24, 2018 at 03:52:55PM +0100, Noralf Trønnes wrote:
>>>
>>>
>>> Den 24.12.2018 00.10, skrev Peter Wu:
>>>> On Sun, Dec 23, 2018 at 02:55:52PM +0100, Noralf Trønnes wrote:
>>>>>
>>>>>
>>>>> Den 23.12.2018 01.55, skrev Peter Wu:
>>>>>> After drm_fb_helper_fbdev_setup calls drm_fb_helper_init,
>>>>>> "dev->fb_helper" will be initialized (and thus drm_fb_helper_fini
>>>>>> will
>>>>>> have some effect). After that, drm_fb_helper_initial_config is called
>>>>>> which may call the "fb_probe" driver callback.
>>>>>>
>>>>>> This driver callback may call drm_fb_helper_defio_init (as is done by
>>>>>> drm_fb_helper_generic_probe) or set a framebuffer (as is done by
>>>>>> bochs)
>>>>>> as documented. These are normally cleaned up on exit by
>>>>>> drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini.
>>>>>>
>>>>>> If an error occurs after "fb_probe", but before setup is complete,
>>>>>> then
>>>>>> calling just drm_fb_helper_fini will leak resources. This was
>>>>>> triggered
>>>>>> by df2052cc922 ("bochs: convert to
>>>>>> drm_fb_helper_fbdev_setup/teardown"):
>>>>>>
>>>>>>    [   50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN
>>>>>> to support this framebuffer
>>>>>>    [   50.009436] bochs-drm :00:02.0:
>>>>>> [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set
>>>>>> configuration (ret=-38)
>>>>>>    [   50.011456] [drm] Initialized bochs-drm 1.0.0 20130925
>>>>>> for :00:02.0 on minor 2
>>>>>>    [   50.013604] WARNING: CPU: 1 PID: 1 at
>>>>>> drivers/gpu/drm/drm_mode_config.c:477
>>>>>> drm_mode_config_cleanup+0x280/0x2a0
>>>>>>    [   50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted:
>>>>>> G    T 4.20.0-rc7 #1
>>>>>>    [   50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0
>>>>>>    ...
>>>>>>    [   50.023155] Call Trace:
>>>>>>    [   50.023155]  ? bochs_kms_fini+0x1e/0x30
>>>>>>    [   50.023155]  ? bochs_unload+0x18/0x40
>>>>>>
>>>>>> This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n.
>>>>>>
>>>>>> Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian
>>>>>> Link: https://lkml.kernel.org/r/20181223004315.GA11455@al
>>>>>> Fixes: 8741216396b2 ("drm/fb-helper: Add
>>>>>> drm_fb_helper_fbdev_setup/teardown()")
>>>>>> Reported-by: kernel test robot 
>>>>>> Cc: Noralf Trønnes 
>>>>>> Signed-off-by: Peter Wu 
>>>>>> ---
>>>>>>     drivers/gpu/drm/drm_fb_helper.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>>>>>> b/drivers/gpu/drm/drm_fb_helper.c
>>>>>> index 9d64f874f965..432e0f3b9267 100644
>>>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>>>> @@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct
>>>>>> drm_device *dev,
>>>>>>     return 0;
>>>>>>     err_drm_fb_helper_fini:
>>>>>> -    drm_fb_helper_fini(fb_helper);
>>>>>> +    drm_fb_helper_fbdev_teardown(dev);
>>>>>
>>>>> This change will break the error path for drm_fbdev_generic_setup()
>>>>> because drm_fb_helper_generic_probe() cleans up on error but doesn't
>>>>> clear drm_fb_helper->fb resulting in a double
>>>>> drm_framebuffer_remove().
>>>>
>>>> This should probably considered a bug of drm_fb_helper_generic_probe.
>>>> Ownership of fb_helper should remain with the caller. The caller can
>>>> detect an error and act accordingly.
>>>>
>>>>> My assumption has been that the drm_fb_helper_funcs->fb_probe callback
>>

Re: [PATCH v2 13/14] drm/tinydrm: do not reply on drmP.h from drm_gem_cma_helper.h

[Noralf Trønnes]

Den 30.12.2018 18.48, skrev Sam Ravnborg:
> drmP.h was the only header file in the past and a lot
> of files rely on that drmP.h defines everything.
> The goal is to one day to delete drmP.h and
> as a step towards this it will no longer be included in the
> headers files in include/drm/
> 
> To prepare tinydrm/ for this add dependencies that
> othwewise was pulled in by drmP.h from drm_gem_cma_helper.h
> 
> To avoid that tinydrm.h became "include everything",
> push include files to the individual drivers.
> 
> Signed-off-by: Sam Ravnborg 
> Cc: "Noralf Trønnes" 
> Cc: David Airlie 
> Cc: Eric Anholt 
> Cc: David Lechner 
> Cc: Daniel Vetter 
> ---

Thanks for doing this dep fix series:

Reviewed-by: Noralf Trønnes 

Re: [PATCH v2 14/14] drm: remove drmP.h from drm_gem_cma_helper.h

[Noralf Trønnes]

Den 30.12.2018 18.48, skrev Sam Ravnborg:
> With all dependencies fixed we can now remove
> drmP.h from drm_gem_cma_helper.h.
> It is replaced by the include files required,
> or forward declarations as appropritate.
> 
> Signed-off-by: Sam Ravnborg 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Sean Paul 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v2 12/14] drm/stm: do not reply on drmP.h from drm_gem_cma_helper.h

[Noralf Trønnes]

Den 30.12.2018 18.48, skrev Sam Ravnborg:
> drmP.h was the only header file in the past and a lot
> of files rely on that drmP.h defines everything.
> The goal is to one day to delete drmP.h and
> as a step towards this it will no longer be included in the
> headers files in include/drm/
> 
> To prepare stm/ for this add dependencies that
> othwewise was pulled in by drmP.h from drm_gem_cma_helper.h
> 
> Sort the include list when we anyway modify it.
> 
> Signed-off-by: Sam Ravnborg 
> Cc: Yannick Fertre 
> Cc: Philippe Cornu 
> Cc: Benjamin Gaignard 
> Cc: Vincent Abriou 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v2 11/14] drm/arc: do not reply on drmP.h from drm_gem_cma_helper.h

[Noralf Trønnes]

Den 30.12.2018 18.48, skrev Sam Ravnborg:
> drmP.h was the only header file in the past and a lot
> of files rely on that drmP.h defines everything.
> The goal is to one day to delete drmP.h and
> as a step towards this it will no longer be included in the
> headers files in include/drm/
> 
> To prepare arc/ for this add dependencies that
> othwewise was pulled in by drmP.h from drm_gem_cma_helper.h
> 
> Signed-off-by: Sam Ravnborg 
> Cc: Alexey Brodkin 
> Cc: Daniel Vetter 
> Cc: David Airlie 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v2 09/14] drm: remove include of drmP.h from drm_encoder_slave.h

[Noralf Trønnes]

Den 30.12.2018 18.48, skrev Sam Ravnborg:
> No further changes required.
> 
> Signed-off-by: Sam Ravnborg 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Sean Paul 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v2 05/14] drm: make drm_file.h self contained

[Noralf Trønnes]

Den 30.12.2018 18.48, skrev Sam Ravnborg:
> drm_file.h embed struct idr, so this file need to know
> the full type definition.
> 
> With this change users of drm_file.h are no longer forced
> to include idr.h - a file they usually get from drmP.h
> 
> This makes it simpler to remove drmP.h includes
> 
> Signed-off-by: Sam Ravnborg 
> Cc: Jani Nikula 
> Cc: Daniel Vetter 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Sean Paul 
> Cc: David Airlie 
> ---

Acked-by: Noralf Trønnes 

Re: [PATCH v2 04/14] drm: make drm_framebuffer.h self contained

[Noralf Trønnes]

Den 30.12.2018 18.48, skrev Sam Ravnborg:
> Add forward declaration and pull in include
> file to make drm_framebuffer.h self contained.
> 
> While add it order include files alphabetically.
> 
> The use of TASK_COMM_LEN is the reason for including sched.h.
> I could not see any good way to avoid this dependency,
> and users of drm_framebuffer.comm already use
> TASK_COMM_LEN to check for length etc.

We can't avoid including it, the macro is used here after all.

> 
> Signed-off-by: Sam Ravnborg 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Sean Paul 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> ---
>  include/drm/drm_framebuffer.h | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index c94acedfb08e..f639ed527943 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -23,13 +23,16 @@
>  #ifndef __DRM_FRAMEBUFFER_H__
>  #define __DRM_FRAMEBUFFER_H__
>  
> -#include 
>  #include 
> +#include 
> +#include 
> +
>  #include 
>  
>  struct drm_framebuffer;
>  struct drm_file;
>  struct drm_device;
> +struct drm_clip_rect;

I think you can add drm_gem_object to this list.

>  
>  /**
>   * struct drm_framebuffer_funcs - framebuffer hooks
> 

Acked-by: Noralf Trønnes 

Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup

[Noralf Trønnes]

Den 24.12.2018 16.03, skrev Peter Wu:

On Mon, Dec 24, 2018 at 03:52:55PM +0100, Noralf Trønnes wrote:



Den 24.12.2018 00.10, skrev Peter Wu:

On Sun, Dec 23, 2018 at 02:55:52PM +0100, Noralf Trønnes wrote:



Den 23.12.2018 01.55, skrev Peter Wu:

After drm_fb_helper_fbdev_setup calls drm_fb_helper_init,
"dev->fb_helper" will be initialized (and thus drm_fb_helper_fini will
have some effect). After that, drm_fb_helper_initial_config is called
which may call the "fb_probe" driver callback.

This driver callback may call drm_fb_helper_defio_init (as is done by
drm_fb_helper_generic_probe) or set a framebuffer (as is done by bochs)
as documented. These are normally cleaned up on exit by
drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini.

If an error occurs after "fb_probe", but before setup is complete, then
calling just drm_fb_helper_fini will leak resources. This was triggered
by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown"):

   [   50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support 
this framebuffer
   [   50.009436] bochs-drm :00:02.0: [drm:drm_fb_helper_fbdev_setup] 
*ERROR* fbdev: Failed to set configuration (ret=-38)
   [   50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for 
:00:02.0 on minor 2
   [   50.013604] WARNING: CPU: 1 PID: 1 at 
drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0
   [   50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: GT 
4.20.0-rc7 #1
   [   50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0
   ...
   [   50.023155] Call Trace:
   [   50.023155]  ? bochs_kms_fini+0x1e/0x30
   [   50.023155]  ? bochs_unload+0x18/0x40

This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n.

Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian
Link: https://lkml.kernel.org/r/20181223004315.GA11455@al
Fixes: 8741216396b2 ("drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()")
Reported-by: kernel test robot 
Cc: Noralf Trønnes 
Signed-off-by: Peter Wu 
---
drivers/gpu/drm/drm_fb_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 9d64f874f965..432e0f3b9267 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct drm_device *dev,
return 0;
err_drm_fb_helper_fini:
-   drm_fb_helper_fini(fb_helper);
+   drm_fb_helper_fbdev_teardown(dev);


This change will break the error path for drm_fbdev_generic_setup()
because drm_fb_helper_generic_probe() cleans up on error but doesn't
clear drm_fb_helper->fb resulting in a double drm_framebuffer_remove().


This should probably considered a bug of drm_fb_helper_generic_probe.
Ownership of fb_helper should remain with the caller. The caller can
detect an error and act accordingly.


My assumption has been that the drm_fb_helper_funcs->fb_probe callback
cleans up its resources on error. Clearly this is not the case for bochs, so
my take on this is that bochsfb_create() needs to clean up on error.


That assumption still holds for bochs. The problem is this sequence:
- drm_fb_helper_fbdev_setup is called.
- fb_probe succeeds (this is crucial).
- register_framebuffer fails.
- error path of setup is triggered.

As fb_helper is fully setup by drivers, the drm_fb_helper core should
fully deallocate it again on the error path or else a leak occurs.


Gerd has a patchset that switches bochs over to the generic fbdev
emulation, but ofc that doesn't help with 4.20:
https://patchwork.freedesktop.org/series/54269/


And that does not help with other users of the drm_fb_helper who use
functions like drm_fb_helper_defio_init. They will likely run in the
same problem.

I don't have a way to test tinydrm or other drivers, but if you force
register_framebuffer to fail, you should be able to reproduce the
problem with drm_fb_helper_generic_probe.



Now I understand. I have looked at the drivers that use drm_fb_helper
and no one seem to handle the case where register_framebuffer() is
failing.

Here's what drivers do when drm_fb_helper_initial_config() fails:

Doesn't check:
amdgpu
virtio

Calls drm_fb_helper_fini():
armada
ast
exynos
gma500
hisilicon
mgag200
msm
nouveau
omap
radeon
rockchip
tegra
udl
bochs - Uses drm_fb_helper_fbdev_setup()
qxl - Uses drm_fb_helper_fbdev_setup()
vboxvideo - Uses drm_fb_helper_fbdev_setup()

Might clean up, not sure:
cirrus

Looks suspicious:
i915

I looked at bochs before it switched to drm_fb_helper_fbdev_setup() and
it also just called drm_fb_helper_fini().

It looks like you've uncovered something no one has though about (or
not implemented at least).

It's not just the framebuffer that's not destroyed, the buffer object
is also leaked. drm_mode_config_cleanup() yells about the framebuffer
(and fr

Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup

[Noralf Trønnes]

Den 24.12.2018 00.10, skrev Peter Wu:

On Sun, Dec 23, 2018 at 02:55:52PM +0100, Noralf Trønnes wrote:



Den 23.12.2018 01.55, skrev Peter Wu:

After drm_fb_helper_fbdev_setup calls drm_fb_helper_init,
"dev->fb_helper" will be initialized (and thus drm_fb_helper_fini will
have some effect). After that, drm_fb_helper_initial_config is called
which may call the "fb_probe" driver callback.

This driver callback may call drm_fb_helper_defio_init (as is done by
drm_fb_helper_generic_probe) or set a framebuffer (as is done by bochs)
as documented. These are normally cleaned up on exit by
drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini.

If an error occurs after "fb_probe", but before setup is complete, then
calling just drm_fb_helper_fini will leak resources. This was triggered
by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown"):

  [   50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this 
framebuffer
  [   50.009436] bochs-drm :00:02.0: [drm:drm_fb_helper_fbdev_setup] 
*ERROR* fbdev: Failed to set configuration (ret=-38)
  [   50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for 
:00:02.0 on minor 2
  [   50.013604] WARNING: CPU: 1 PID: 1 at 
drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0
  [   50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: GT 
4.20.0-rc7 #1
  [   50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0
  ...
  [   50.023155] Call Trace:
  [   50.023155]  ? bochs_kms_fini+0x1e/0x30
  [   50.023155]  ? bochs_unload+0x18/0x40

This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n.

Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian
Link: https://lkml.kernel.org/r/20181223004315.GA11455@al
Fixes: 8741216396b2 ("drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()")
Reported-by: kernel test robot 
Cc: Noralf Trønnes 
Signed-off-by: Peter Wu 
---
   drivers/gpu/drm/drm_fb_helper.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 9d64f874f965..432e0f3b9267 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct drm_device *dev,
return 0;
   err_drm_fb_helper_fini:
-   drm_fb_helper_fini(fb_helper);
+   drm_fb_helper_fbdev_teardown(dev);


This change will break the error path for drm_fbdev_generic_setup()
because drm_fb_helper_generic_probe() cleans up on error but doesn't
clear drm_fb_helper->fb resulting in a double drm_framebuffer_remove().


This should probably considered a bug of drm_fb_helper_generic_probe.
Ownership of fb_helper should remain with the caller. The caller can
detect an error and act accordingly.


My assumption has been that the drm_fb_helper_funcs->fb_probe callback
cleans up its resources on error. Clearly this is not the case for bochs, so
my take on this is that bochsfb_create() needs to clean up on error.


That assumption still holds for bochs. The problem is this sequence:
- drm_fb_helper_fbdev_setup is called.
- fb_probe succeeds (this is crucial).
- register_framebuffer fails.
- error path of setup is triggered.

As fb_helper is fully setup by drivers, the drm_fb_helper core should
fully deallocate it again on the error path or else a leak occurs.


Gerd has a patchset that switches bochs over to the generic fbdev
emulation, but ofc that doesn't help with 4.20:
https://patchwork.freedesktop.org/series/54269/


And that does not help with other users of the drm_fb_helper who use
functions like drm_fb_helper_defio_init. They will likely run in the
same problem.

I don't have a way to test tinydrm or other drivers, but if you force
register_framebuffer to fail, you should be able to reproduce the
problem with drm_fb_helper_generic_probe.



Now I understand. I have looked at the drivers that use drm_fb_helper
and no one seem to handle the case where register_framebuffer() is
failing.

Here's what drivers do when drm_fb_helper_initial_config() fails:

Doesn't check:
amdgpu
virtio

Calls drm_fb_helper_fini():
armada
ast
exynos
gma500
hisilicon
mgag200
msm
nouveau
omap
radeon
rockchip
tegra
udl
bochs - Uses drm_fb_helper_fbdev_setup()
qxl - Uses drm_fb_helper_fbdev_setup()
vboxvideo - Uses drm_fb_helper_fbdev_setup()

Might clean up, not sure:
cirrus

Looks suspicious:
i915

I looked at bochs before it switched to drm_fb_helper_fbdev_setup() and
it also just called drm_fb_helper_fini().

It looks like you've uncovered something no one has though about (or
not implemented at least).

It's not just the framebuffer that's not destroyed, the buffer object
is also leaked. drm_mode_config_cleanup() yells about the framebuffer
(and frees it), but says nothing about the buffer object. It might be
that it can't even be made to detect that since some d

Re: [PATCH] drm/fb-helper: fix leaks in error path of drm_fb_helper_fbdev_setup

[Noralf Trønnes]

Den 23.12.2018 01.55, skrev Peter Wu:

After drm_fb_helper_fbdev_setup calls drm_fb_helper_init,
"dev->fb_helper" will be initialized (and thus drm_fb_helper_fini will
have some effect). After that, drm_fb_helper_initial_config is called
which may call the "fb_probe" driver callback.

This driver callback may call drm_fb_helper_defio_init (as is done by
drm_fb_helper_generic_probe) or set a framebuffer (as is done by bochs)
as documented. These are normally cleaned up on exit by
drm_fb_helper_fbdev_teardown which also calls drm_fb_helper_fini.

If an error occurs after "fb_probe", but before setup is complete, then
calling just drm_fb_helper_fini will leak resources. This was triggered
by df2052cc922 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown"):

 [   50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this 
framebuffer
 [   50.009436] bochs-drm :00:02.0: [drm:drm_fb_helper_fbdev_setup] 
*ERROR* fbdev: Failed to set configuration (ret=-38)
 [   50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for :00:02.0 
on minor 2
 [   50.013604] WARNING: CPU: 1 PID: 1 at 
drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0
 [   50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: GT 
4.20.0-rc7 #1
 [   50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0
 ...
 [   50.023155] Call Trace:
 [   50.023155]  ? bochs_kms_fini+0x1e/0x30
 [   50.023155]  ? bochs_unload+0x18/0x40

This can be reproduced with QEMU and CONFIG_FB_LITTLE_ENDIAN=n.

Link: https://lkml.kernel.org/r/20181221083226.GI23332@shao2-debian
Link: https://lkml.kernel.org/r/20181223004315.GA11455@al
Fixes: 8741216396b2 ("drm/fb-helper: Add drm_fb_helper_fbdev_setup/teardown()")
Reported-by: kernel test robot 
Cc: Noralf Trønnes 
Signed-off-by: Peter Wu 
---
  drivers/gpu/drm/drm_fb_helper.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 9d64f874f965..432e0f3b9267 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2860,7 +2860,7 @@ int drm_fb_helper_fbdev_setup(struct drm_device *dev,
return 0;
  
  err_drm_fb_helper_fini:

-   drm_fb_helper_fini(fb_helper);
+   drm_fb_helper_fbdev_teardown(dev);


This change will break the error path for drm_fbdev_generic_setup()
because drm_fb_helper_generic_probe() cleans up on error but doesn't
clear drm_fb_helper->fb resulting in a double drm_framebuffer_remove().

My assumption has been that the drm_fb_helper_funcs->fb_probe callback
cleans up its resources on error. Clearly this is not the case for 
bochs, so my take on this is that bochsfb_create() needs to clean up on 
error.


Gerd has a patchset that switches bochs over to the generic fbdev
emulation, but ofc that doesn't help with 4.20:
https://patchwork.freedesktop.org/series/54269/

Noralf.

  
  	return ret;

  }

Re: [PATCH] drm/xen-front: Make shmem backed display buffer coherent

[Noralf Trønnes]

Den 19.12.2018 09.18, skrev Oleksandr Andrushchenko:

On 12/18/18 9:20 PM, Noralf Trønnes wrote:


Den 27.11.2018 11.32, skrev Oleksandr Andrushchenko:

From: Oleksandr Andrushchenko 

When GEM backing storage is allocated with drm_gem_get_pages
the backing pages may be cached, thus making it possible that
the backend sees only partial content of the buffer which may
lead to screen artifacts. Make sure that the frontend's
memory is coherent and the backend always sees correct display
buffer content.

Fixes: c575b7eeb89f ("drm/xen-front: Add support for Xen PV display 
frontend")


Signed-off-by: Oleksandr Andrushchenko 


---
  drivers/gpu/drm/xen/xen_drm_front_gem.c | 62 
+++--

  1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c

index 47ff019d3aef..c592735e49d2 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -33,8 +33,11 @@ struct xen_gem_object {
  /* set for buffers allocated by the backend */
  bool be_alloc;
  -    /* this is for imported PRIME buffer */
-    struct sg_table *sgt_imported;
+    /*
+ * this is for imported PRIME buffer or the one allocated via
+ * drm_gem_get_pages.
+ */
+    struct sg_table *sgt;
  };
    static inline struct xen_gem_object *
@@ -77,10 +80,21 @@ static struct xen_gem_object 
*gem_create_obj(struct drm_device *dev,

  return xen_obj;
  }
  +struct sg_table *xen_drm_front_gem_get_sg_table(struct 
drm_gem_object *gem_obj)

+{
+    struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
+
+    if (!xen_obj->pages)
+    return ERR_PTR(-ENOMEM);
+
+    return drm_prime_pages_to_sg(xen_obj->pages, xen_obj->num_pages);
+}
+
  static struct xen_gem_object *gem_create(struct drm_device *dev, 
size_t size)

  {
  struct xen_drm_front_drm_info *drm_info = dev->dev_private;
  struct xen_gem_object *xen_obj;
+    struct address_space *mapping;
  int ret;
    size = round_up(size, PAGE_SIZE);
@@ -113,10 +127,14 @@ static struct xen_gem_object 
*gem_create(struct drm_device *dev, size_t size)

  xen_obj->be_alloc = true;
  return xen_obj;
  }
+
  /*
   * need to allocate backing pages now, so we can share those
   * with the backend
   */



Let's see if I understand what you're doing:

Here you say that the pages should be DMA accessible for devices that 
can

only see 4GB.


Yes, your understanding is correct. As we are a para-virtualized 
device we


do not have strict requirements for 32-bit DMA. But, via dma-buf export,

the buffer we create can be used by real HW, e.g. one can pass-through

real HW devices into a guest domain and they can import our buffer (yes,

they can be IOMMU backed and other conditions may apply).

So, this is why we are limiting to DMA32 here, just to allow more 
possible


use-cases




+    mapping = xen_obj->base.filp->f_mapping;
+    mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32);
+
  xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
  xen_obj->pages = drm_gem_get_pages(_obj->base);
  if (IS_ERR_OR_NULL(xen_obj->pages)) {
@@ -125,8 +143,27 @@ static struct xen_gem_object *gem_create(struct 
drm_device *dev, size_t size)

  goto fail;
  }
  +    xen_obj->sgt = xen_drm_front_gem_get_sg_table(_obj->base);
+    if (IS_ERR_OR_NULL(xen_obj->sgt)){
+    ret = PTR_ERR(xen_obj->sgt);
+    xen_obj->sgt = NULL;
+    goto fail_put_pages;
+    }
+
+    if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
+    DMA_BIDIRECTIONAL)) {



Are you using the DMA streaming API as a way to flush the caches?

Yes

Does this mean that GFP_USER isn't making the buffer coherent?


No, it didn't help. I had a question [1] if there are any other better 
way


to achieve the same, but didn't have any response yet. So, I implemented

it via DMA API which helped.


As Gerd says asking on the arm list is probably the best way of finding a
future proof solution and understanding what's going on.

But if you don't get any help there and you end up with the present
solution I suggest you add a comment that this is for flushing the caches
on arm. With the current code one can be led to believe that the driver
uses the dma address somewhere.

What about x86, does the problem exist there?

I wonder if you can call dma_unmap_sg() right away since the flushing has
already happened. That would contain this flushing "hack" inside the
gem_create function.

I also suggest calling drm_prime_pages_to_sg() directly to increase
readability, since the check in xen_drm_front_gem_get_sg_table() isn't
necessary for this use case.

Noralf.






Noralf.


+    ret = -EFAULT;
+    goto fail_free_sgt;
+    }
+
  return xen_obj;
  +fail_free_sgt:
+    sg_free_table(xen_obj->sgt);
+    x

Re: [PATCH] drm/xen-front: Make shmem backed display buffer coherent

[Noralf Trønnes]

Den 27.11.2018 11.32, skrev Oleksandr Andrushchenko:

From: Oleksandr Andrushchenko 

When GEM backing storage is allocated with drm_gem_get_pages
the backing pages may be cached, thus making it possible that
the backend sees only partial content of the buffer which may
lead to screen artifacts. Make sure that the frontend's
memory is coherent and the backend always sees correct display
buffer content.

Fixes: c575b7eeb89f ("drm/xen-front: Add support for Xen PV display frontend")

Signed-off-by: Oleksandr Andrushchenko 
---
  drivers/gpu/drm/xen/xen_drm_front_gem.c | 62 +++--
  1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c 
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 47ff019d3aef..c592735e49d2 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -33,8 +33,11 @@ struct xen_gem_object {
/* set for buffers allocated by the backend */
bool be_alloc;
  
-	/* this is for imported PRIME buffer */

-   struct sg_table *sgt_imported;
+   /*
+* this is for imported PRIME buffer or the one allocated via
+* drm_gem_get_pages.
+*/
+   struct sg_table *sgt;
  };
  
  static inline struct xen_gem_object *

@@ -77,10 +80,21 @@ static struct xen_gem_object *gem_create_obj(struct 
drm_device *dev,
return xen_obj;
  }
  
+struct sg_table *xen_drm_front_gem_get_sg_table(struct drm_gem_object *gem_obj)

+{
+   struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
+
+   if (!xen_obj->pages)
+   return ERR_PTR(-ENOMEM);
+
+   return drm_prime_pages_to_sg(xen_obj->pages, xen_obj->num_pages);
+}
+
  static struct xen_gem_object *gem_create(struct drm_device *dev, size_t size)
  {
struct xen_drm_front_drm_info *drm_info = dev->dev_private;
struct xen_gem_object *xen_obj;
+   struct address_space *mapping;
int ret;
  
  	size = round_up(size, PAGE_SIZE);

@@ -113,10 +127,14 @@ static struct xen_gem_object *gem_create(struct 
drm_device *dev, size_t size)
xen_obj->be_alloc = true;
return xen_obj;
}
+
/*
 * need to allocate backing pages now, so we can share those
 * with the backend
 */



Let's see if I understand what you're doing:

Here you say that the pages should be DMA accessible for devices that can
only see 4GB.


+   mapping = xen_obj->base.filp->f_mapping;
+   mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32);
+
xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
xen_obj->pages = drm_gem_get_pages(_obj->base);
if (IS_ERR_OR_NULL(xen_obj->pages)) {
@@ -125,8 +143,27 @@ static struct xen_gem_object *gem_create(struct drm_device 
*dev, size_t size)
goto fail;
}
  
+	xen_obj->sgt = xen_drm_front_gem_get_sg_table(_obj->base);

+   if (IS_ERR_OR_NULL(xen_obj->sgt)){
+   ret = PTR_ERR(xen_obj->sgt);
+   xen_obj->sgt = NULL;
+   goto fail_put_pages;
+   }
+
+   if (!dma_map_sg(dev->dev, xen_obj->sgt->sgl, xen_obj->sgt->nents,
+   DMA_BIDIRECTIONAL)) {



Are you using the DMA streaming API as a way to flush the caches?
Does this mean that GFP_USER isn't making the buffer coherent?

Noralf.


+   ret = -EFAULT;
+   goto fail_free_sgt;
+   }
+
return xen_obj;
  
+fail_free_sgt:

+   sg_free_table(xen_obj->sgt);
+   xen_obj->sgt = NULL;
+fail_put_pages:
+   drm_gem_put_pages(_obj->base, xen_obj->pages, true, false);
+   xen_obj->pages = NULL;
  fail:
DRM_ERROR("Failed to allocate buffer with size %zu\n", size);
return ERR_PTR(ret);
@@ -149,7 +186,7 @@ void xen_drm_front_gem_free_object_unlocked(struct 
drm_gem_object *gem_obj)
struct xen_gem_object *xen_obj = to_xen_gem_obj(gem_obj);
  
  	if (xen_obj->base.import_attach) {

-   drm_prime_gem_destroy(_obj->base, xen_obj->sgt_imported);
+   drm_prime_gem_destroy(_obj->base, xen_obj->sgt);
gem_free_pages_array(xen_obj);
} else {
if (xen_obj->pages) {
@@ -158,6 +195,13 @@ void xen_drm_front_gem_free_object_unlocked(struct 
drm_gem_object *gem_obj)
xen_obj->pages);
gem_free_pages_array(xen_obj);
} else {
+   if (xen_obj->sgt) {
+   dma_unmap_sg(xen_obj->base.dev->dev,
+xen_obj->sgt->sgl,
+xen_obj->sgt->nents,
+DMA_BIDIRECTIONAL);
+   sg_free_table(xen_obj->sgt);
+   }

Re: [Xen-devel][PATCH v2 2/3] drm/xen-front: Use Xen common shared buffer implementation

[Noralf Trønnes]

Den 30.11.2018 08.42, skrev Oleksandr Andrushchenko:

From: Oleksandr Andrushchenko 

Use page directory based shared buffer implementation
now available as common code for Xen frontend drivers.

Remove flushing of shared buffer on page flip as this
workaround needs a proper fix.

Signed-off-by: Oleksandr Andrushchenko 
---


Reviewed-by: Noralf Trønnes 

Re: [PATCH] drm/imx: fix build failure without CONFIG_DRM_FBDEV_EMULATION

[Noralf Trønnes]

Den 26.09.2018 21.38, skrev Arnd Bergmann:

The variable is declared in an #ifdef section, but the user is
now unconditional, which leads to a build failure:

drivers/gpu/drm/imx/imx-drm-core.c: In function 'imx_drm_bind':
drivers/gpu/drm/imx/imx-drm-core.c:264:6: error: 'legacyfb_depth' undeclared 
(first use in this function); did you mean 'lockdep_depth'?

Remove the remaining #ifdef as well.

Fixes: f53705fd9803 ("drm/imx: Use drm_fbdev_generic_setup()")
Signed-off-by: Arnd Bergmann 
---


Thanks for fixing my mistakes Arnd.
Applied to drm-misc-next.

Noralf.


  drivers/gpu/drm/imx/imx-drm-core.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c 
b/drivers/gpu/drm/imx/imx-drm-core.c
index a70f3131a377..0e6942f21a4e 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -35,10 +35,8 @@
  
  #define MAX_CRTC	4
  
-#if IS_ENABLED(CONFIG_DRM_FBDEV_EMULATION)

  static int legacyfb_depth = 16;
  module_param(legacyfb_depth, int, 0444);
-#endif
  
  DEFINE_DRM_GEM_CMA_FOPS(imx_drm_driver_fops);
  

Re: [PATCH] drm/imx: fix build failure without CONFIG_DRM_FBDEV_EMULATION

[Noralf Trønnes]

Den 26.09.2018 21.38, skrev Arnd Bergmann:

The variable is declared in an #ifdef section, but the user is
now unconditional, which leads to a build failure:

drivers/gpu/drm/imx/imx-drm-core.c: In function 'imx_drm_bind':
drivers/gpu/drm/imx/imx-drm-core.c:264:6: error: 'legacyfb_depth' undeclared 
(first use in this function); did you mean 'lockdep_depth'?

Remove the remaining #ifdef as well.

Fixes: f53705fd9803 ("drm/imx: Use drm_fbdev_generic_setup()")
Signed-off-by: Arnd Bergmann 
---


Thanks for fixing my mistakes Arnd.
Applied to drm-misc-next.

Noralf.


  drivers/gpu/drm/imx/imx-drm-core.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c 
b/drivers/gpu/drm/imx/imx-drm-core.c
index a70f3131a377..0e6942f21a4e 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -35,10 +35,8 @@
  
  #define MAX_CRTC	4
  
-#if IS_ENABLED(CONFIG_DRM_FBDEV_EMULATION)

  static int legacyfb_depth = 16;
  module_param(legacyfb_depth, int, 0444);
-#endif
  
  DEFINE_DRM_GEM_CMA_FOPS(imx_drm_driver_fops);
  

Re: [PATCH v1 5/5] tinydrm: add winstar wg160160 driver

[Noralf Trønnes]

Den 08.08.2018 10.32, skrev Sam Ravnborg:

Hi Noralf.

On Tue, Aug 07, 2018 at 07:35:30PM +0200, Noralf Trønnes wrote:

Den 02.08.2018 21.45, skrev Sam Ravnborg:

Add driver for the winstar wg160160 display.
The driver utilises pardata-dbi that
again utilise the pardata subsystem.

Signed-off-by: Sam Ravnborg 
---
  MAINTAINERS|   5 +
  drivers/gpu/drm/tinydrm/Kconfig|  10 ++
  drivers/gpu/drm/tinydrm/Makefile   |   1 +
  drivers/gpu/drm/tinydrm/wg160160.c | 298 +
  4 files changed, 314 insertions(+)
  create mode 100644 drivers/gpu/drm/tinydrm/wg160160.c


[...]


+
+/**
+ * write_reg - Write instruction on parallel bus to controller
+ *
+ * Check BUSY flag and write instruction
+ *
+ * @pdd: pardata data
+ * @reg: The register to write
+ * @value: The value of the register
+ *
+ * Returns:
+ * Zero on success, negative error code on failure
+ */
+int write_reg(struct pardata_data *pdd, unsigned int reg, unsigned int value)
+{
+   int ins[PIN_NUM];
+   int val[PIN_NUM];
+   int i;
+
+   for (i = 0; i < PIN_NUM; i++)
+   ins[PIN_DB0 + i] = !!BIT(reg);
+
+   for (i = 0; i < PIN_NUM; i++)
+   val[PIN_DB0 + i] = !!(value & BIT(i));
+
+   gpiod_set_value_cansleep(pdd->bus->pin_rs, 1);
+   gpiod_set_array_value_cansleep(PIN_NUM, pdd->bus->data_pins->desc, ins);
+   wait_busy(pdd);
+   pardata_strobe_write(pdd);
+
+   gpiod_set_value_cansleep(pdd->bus->pin_rs, 0);
+   gpiod_set_array_value_cansleep(PIN_NUM, pdd->bus->data_pins->desc, val);
+   wait_busy(pdd);
+   pardata_strobe_write(pdd);
+
+   return 0;
+}

If this controller has normal registers, you could do a regmap
implementation for pardata: drivers/base/regmap.

If I understand you correct then you suggest to add a regmap implementation
on top of the registers replacing the current gpio support?

So we in regmap can optimize access to the registers better.
Or did you have something else in mind?


I was thinking of a regmap-pardata.c like drivers/base/regmap/regmap-i2c.c.
It's not much code an we would avoid having generic register access
functions in DRM. It's not about speed or caching, but just to try and
put code where it logically belongs.

Here is one experiment where I have created a regmap on a 8080 bus:
https://github.com/notro/tinydrm/blob/master/tinydrm-regmap.c
Used here: https://github.com/notro/tinydrm/blob/master/fb_ili9325.c

I don't advocate this particular approach, I mention it just as an
example of using regmap in a tinydrm driver.

Noralf.


As speed is not the main challenge here I will likely wait with this
and continue using gpio.

Sam

Re: [PATCH v1 5/5] tinydrm: add winstar wg160160 driver

[Noralf Trønnes]

Den 08.08.2018 10.32, skrev Sam Ravnborg:

Hi Noralf.

On Tue, Aug 07, 2018 at 07:35:30PM +0200, Noralf Trønnes wrote:

Den 02.08.2018 21.45, skrev Sam Ravnborg:

Add driver for the winstar wg160160 display.
The driver utilises pardata-dbi that
again utilise the pardata subsystem.

Signed-off-by: Sam Ravnborg 
---
  MAINTAINERS|   5 +
  drivers/gpu/drm/tinydrm/Kconfig|  10 ++
  drivers/gpu/drm/tinydrm/Makefile   |   1 +
  drivers/gpu/drm/tinydrm/wg160160.c | 298 +
  4 files changed, 314 insertions(+)
  create mode 100644 drivers/gpu/drm/tinydrm/wg160160.c


[...]


+
+/**
+ * write_reg - Write instruction on parallel bus to controller
+ *
+ * Check BUSY flag and write instruction
+ *
+ * @pdd: pardata data
+ * @reg: The register to write
+ * @value: The value of the register
+ *
+ * Returns:
+ * Zero on success, negative error code on failure
+ */
+int write_reg(struct pardata_data *pdd, unsigned int reg, unsigned int value)
+{
+   int ins[PIN_NUM];
+   int val[PIN_NUM];
+   int i;
+
+   for (i = 0; i < PIN_NUM; i++)
+   ins[PIN_DB0 + i] = !!BIT(reg);
+
+   for (i = 0; i < PIN_NUM; i++)
+   val[PIN_DB0 + i] = !!(value & BIT(i));
+
+   gpiod_set_value_cansleep(pdd->bus->pin_rs, 1);
+   gpiod_set_array_value_cansleep(PIN_NUM, pdd->bus->data_pins->desc, ins);
+   wait_busy(pdd);
+   pardata_strobe_write(pdd);
+
+   gpiod_set_value_cansleep(pdd->bus->pin_rs, 0);
+   gpiod_set_array_value_cansleep(PIN_NUM, pdd->bus->data_pins->desc, val);
+   wait_busy(pdd);
+   pardata_strobe_write(pdd);
+
+   return 0;
+}

If this controller has normal registers, you could do a regmap
implementation for pardata: drivers/base/regmap.

If I understand you correct then you suggest to add a regmap implementation
on top of the registers replacing the current gpio support?

So we in regmap can optimize access to the registers better.
Or did you have something else in mind?


I was thinking of a regmap-pardata.c like drivers/base/regmap/regmap-i2c.c.
It's not much code an we would avoid having generic register access
functions in DRM. It's not about speed or caching, but just to try and
put code where it logically belongs.

Here is one experiment where I have created a regmap on a 8080 bus:
https://github.com/notro/tinydrm/blob/master/tinydrm-regmap.c
Used here: https://github.com/notro/tinydrm/blob/master/fb_ili9325.c

I don't advocate this particular approach, I mention it just as an
example of using regmap in a tinydrm driver.

Noralf.


As speed is not the main challenge here I will likely wait with this
and continue using gpio.

Sam

Re: [PATCH 3/3] drm/tinydrm: new driver for ILI9341 display panels

[Noralf Trønnes]

Den 15.05.2018 23.43, skrev Andy Shevchenko:

On Tue, May 15, 2018 at 4:43 AM, David Lechner <da...@lechnology.com> wrote:

This adds a new driver for display panels that use the Ilitek ILI9341
controller. It currently supports a single display panel, namely
the YX240QV29-T (e.g. Adafruit 2.4" TFT).

The init sequence is from the Adafruit Python library for the ILI9341
controller. https://github.com/adafruit/Adafruit_Python_ILI9341

Some minor style nitpicks, otherwise LGTM

Reviewed-by: Andy Shevchenko <andy.shevche...@gmail.com>


Signed-off-by: David Lechner <da...@lechnology.com>
---
  MAINTAINERS   |   6 +
  drivers/gpu/drm/tinydrm/Kconfig   |  10 ++
  drivers/gpu/drm/tinydrm/Makefile  |   1 +
  drivers/gpu/drm/tinydrm/ili9341.c | 239 ++
  4 files changed, 256 insertions(+)
  create mode 100644 drivers/gpu/drm/tinydrm/ili9341.c

diff --git a/MAINTAINERS b/MAINTAINERS
index bc219de9cbee..ffa099abbd79 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4480,6 +4480,12 @@ S:   Maintained
  F: drivers/gpu/drm/tinydrm/ili9225.c
  F: Documentation/devicetree/bindings/display/ilitek,ili9225.txt

+DRM DRIVER FOR ILITEK ILI9341 PANELS
+M: David Lechner <da...@lechnology.com>
+S: Maintained
+F: drivers/gpu/drm/tinydrm/ili9341.c
+F: Documentation/devicetree/bindings/display/ilitek,ili9341.txt
+
  DRM DRIVER FOR INTEL I810 VIDEO CARDS
  S: Orphan / Obsolete
  F: drivers/gpu/drm/i810/
diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
index 4592a5e3f20b..7a8008b0783f 100644
--- a/drivers/gpu/drm/tinydrm/Kconfig
+++ b/drivers/gpu/drm/tinydrm/Kconfig
@@ -20,6 +20,16 @@ config TINYDRM_ILI9225

   If M is selected the module will be called ili9225.

+config TINYDRM_ILI9341
+   tristate "DRM support for ILI9341 display panels"
+   depends on DRM_TINYDRM && SPI

Can't we do something like

if SPI

...

endif

?


+   select TINYDRM_MIPI_DBI
+   help
+ DRM driver for the following Ilitek ILI9341 panels:
+ * YX240QV29-T 2.4" 240x320 TFT (Adafruit 2.4")
+
+ If M is selected the module will be called ili9341.
+
  config TINYDRM_MI0283QT
 tristate "DRM support for MI0283QT"
 depends on DRM_TINYDRM && SPI
diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
index 49a111929724..14d99080665a 100644
--- a/drivers/gpu/drm/tinydrm/Makefile
+++ b/drivers/gpu/drm/tinydrm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_TINYDRM_MIPI_DBI)  += mipi-dbi.o

  # Displays
  obj-$(CONFIG_TINYDRM_ILI9225)  += ili9225.o
+obj-$(CONFIG_TINYDRM_ILI9341)  += ili9341.o
  obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
  obj-$(CONFIG_TINYDRM_REPAPER)  += repaper.o
  obj-$(CONFIG_TINYDRM_ST7586)   += st7586.o
diff --git a/drivers/gpu/drm/tinydrm/ili9341.c 
b/drivers/gpu/drm/tinydrm/ili9341.c
new file mode 100644
index ..2ce4244a68c3
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/ili9341.c
@@ -0,0 +1,239 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * DRM driver for Ilitek ILI9341 panels
+ *
+ * Copyright 2018 David Lechner <da...@lechnology.com>
+ *
+ * Based on mi0283qt.c:
+ * Copyright 2016 Noralf Trønnes
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 

Can it be in order?


+#include 
+#include 
+#include 
+
+#define ILI9341_FRMCTR10xb1
+#define ILI9341_DISCTRL0xb6
+#define ILI9341_ETMOD  0xb7
+
+#define ILI9341_PWCTRL10xc0
+#define ILI9341_PWCTRL20xc1
+#define ILI9341_VMCTRL10xc5
+#define ILI9341_VMCTRL20xc7
+#define ILI9341_PWCTRLA0xcb
+#define ILI9341_PWCTRLB0xcf
+
+#define ILI9341_PGAMCTRL   0xe0
+#define ILI9341_NGAMCTRL   0xe1
+#define ILI9341_DTCTRLA0xe8
+#define ILI9341_DTCTRLB0xea
+#define ILI9341_PWRSEQ 0xed
+
+#define ILI9341_EN3GAM 0xf2
+#define ILI9341_PUMPCTRL   0xf7
+
+#define ILI9341_MADCTL_BGR BIT(3)
+#define ILI9341_MADCTL_MV  BIT(5)
+#define ILI9341_MADCTL_MX  BIT(6)
+#define ILI9341_MADCTL_MY  BIT(7)
+
+static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
+struct drm_crtc_state *crtc_state,
+struct drm_plane_state *plane_state)
+{
+   struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+   struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+   u8 addr_mode;
+   int ret;
+
+   DRM_DEBUG_KMS("\n");
+
+   ret = mipi_dbi_poweron_conditional_reset(mipi);
+   if (ret < 0)
+   return;
+   if (ret == 1)
+   goto out_enable;
+
+   mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
+
+ 

Re: [PATCH 3/3] drm/tinydrm: new driver for ILI9341 display panels

[Noralf Trønnes]

Den 15.05.2018 23.43, skrev Andy Shevchenko:

On Tue, May 15, 2018 at 4:43 AM, David Lechner  wrote:

This adds a new driver for display panels that use the Ilitek ILI9341
controller. It currently supports a single display panel, namely
the YX240QV29-T (e.g. Adafruit 2.4" TFT).

The init sequence is from the Adafruit Python library for the ILI9341
controller. https://github.com/adafruit/Adafruit_Python_ILI9341

Some minor style nitpicks, otherwise LGTM

Reviewed-by: Andy Shevchenko 


Signed-off-by: David Lechner 
---
  MAINTAINERS   |   6 +
  drivers/gpu/drm/tinydrm/Kconfig   |  10 ++
  drivers/gpu/drm/tinydrm/Makefile  |   1 +
  drivers/gpu/drm/tinydrm/ili9341.c | 239 ++
  4 files changed, 256 insertions(+)
  create mode 100644 drivers/gpu/drm/tinydrm/ili9341.c

diff --git a/MAINTAINERS b/MAINTAINERS
index bc219de9cbee..ffa099abbd79 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4480,6 +4480,12 @@ S:   Maintained
  F: drivers/gpu/drm/tinydrm/ili9225.c
  F: Documentation/devicetree/bindings/display/ilitek,ili9225.txt

+DRM DRIVER FOR ILITEK ILI9341 PANELS
+M: David Lechner 
+S: Maintained
+F: drivers/gpu/drm/tinydrm/ili9341.c
+F: Documentation/devicetree/bindings/display/ilitek,ili9341.txt
+
  DRM DRIVER FOR INTEL I810 VIDEO CARDS
  S: Orphan / Obsolete
  F: drivers/gpu/drm/i810/
diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
index 4592a5e3f20b..7a8008b0783f 100644
--- a/drivers/gpu/drm/tinydrm/Kconfig
+++ b/drivers/gpu/drm/tinydrm/Kconfig
@@ -20,6 +20,16 @@ config TINYDRM_ILI9225

   If M is selected the module will be called ili9225.

+config TINYDRM_ILI9341
+   tristate "DRM support for ILI9341 display panels"
+   depends on DRM_TINYDRM && SPI

Can't we do something like

if SPI

...

endif

?


+   select TINYDRM_MIPI_DBI
+   help
+ DRM driver for the following Ilitek ILI9341 panels:
+ * YX240QV29-T 2.4" 240x320 TFT (Adafruit 2.4")
+
+ If M is selected the module will be called ili9341.
+
  config TINYDRM_MI0283QT
 tristate "DRM support for MI0283QT"
 depends on DRM_TINYDRM && SPI
diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
index 49a111929724..14d99080665a 100644
--- a/drivers/gpu/drm/tinydrm/Makefile
+++ b/drivers/gpu/drm/tinydrm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_TINYDRM_MIPI_DBI)  += mipi-dbi.o

  # Displays
  obj-$(CONFIG_TINYDRM_ILI9225)  += ili9225.o
+obj-$(CONFIG_TINYDRM_ILI9341)  += ili9341.o
  obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
  obj-$(CONFIG_TINYDRM_REPAPER)  += repaper.o
  obj-$(CONFIG_TINYDRM_ST7586)   += st7586.o
diff --git a/drivers/gpu/drm/tinydrm/ili9341.c 
b/drivers/gpu/drm/tinydrm/ili9341.c
new file mode 100644
index ..2ce4244a68c3
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/ili9341.c
@@ -0,0 +1,239 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * DRM driver for Ilitek ILI9341 panels
+ *
+ * Copyright 2018 David Lechner 
+ *
+ * Based on mi0283qt.c:
+ * Copyright 2016 Noralf Trønnes
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 

Can it be in order?


+#include 
+#include 
+#include 
+
+#define ILI9341_FRMCTR10xb1
+#define ILI9341_DISCTRL0xb6
+#define ILI9341_ETMOD  0xb7
+
+#define ILI9341_PWCTRL10xc0
+#define ILI9341_PWCTRL20xc1
+#define ILI9341_VMCTRL10xc5
+#define ILI9341_VMCTRL20xc7
+#define ILI9341_PWCTRLA0xcb
+#define ILI9341_PWCTRLB0xcf
+
+#define ILI9341_PGAMCTRL   0xe0
+#define ILI9341_NGAMCTRL   0xe1
+#define ILI9341_DTCTRLA0xe8
+#define ILI9341_DTCTRLB0xea
+#define ILI9341_PWRSEQ 0xed
+
+#define ILI9341_EN3GAM 0xf2
+#define ILI9341_PUMPCTRL   0xf7
+
+#define ILI9341_MADCTL_BGR BIT(3)
+#define ILI9341_MADCTL_MV  BIT(5)
+#define ILI9341_MADCTL_MX  BIT(6)
+#define ILI9341_MADCTL_MY  BIT(7)
+
+static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
+struct drm_crtc_state *crtc_state,
+struct drm_plane_state *plane_state)
+{
+   struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+   struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+   u8 addr_mode;
+   int ret;
+
+   DRM_DEBUG_KMS("\n");
+
+   ret = mipi_dbi_poweron_conditional_reset(mipi);
+   if (ret < 0)
+   return;
+   if (ret == 1)
+   goto out_enable;
+
+   mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
+
+   mipi_dbi_command(mipi, ILI9341_PWCTRLB, 0x00, 0xc1, 0x30);
+   mipi_dbi_command(mipi, ILI9341_PWRSEQ, 0x64, 0x03, 0x12, 0x81);

Re: [PATCH 2/3] dt-bindings: new binding for Ilitek ILI9341 display panels

[Noralf Trønnes]

Den 15.05.2018 03.43, skrev David Lechner:

This adds a new binding for Ilitek ILI9341 display panels. It includes
a compatible string for one display (more can be added in the future).

The vendor prefix "noname" is used because the vendor is not known.
The YX240QV29-T panel[1] is found, for example, in an Adafruit breakout
board[2] and in Mindsensors' PiStorms[3].

[1]: 
https://cdn-learn.adafruit.com/assets/assets/000/046/879/original/SPEC-YX240QV29-T_Rev.A__1_.pdf
[2]: https://www.adafruit.com/product/2478
[3]: 
http://www.mindsensors.com/stem-with-robotics/13-pistorms-v2-base-kit-raspberry-pi-brain-for-lego-robot

Signed-off-by: David Lechner 
---
  .../bindings/display/ilitek,ili9341.txt   | 27 +++
  1 file changed, 27 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/display/ilitek,ili9341.txt

diff --git a/Documentation/devicetree/bindings/display/ilitek,ili9341.txt 
b/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
new file mode 100644
index ..0fc90b2dd732
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
@@ -0,0 +1,27 @@
+Ilitek ILI9341 display panels
+
+This binding is for display panels using an Ilitek ILI9341 controller in SPI
+mode.
+
+Required properties:
+- compatible:  "noname,yx240qv29", "ilitek,ili9341"


Calling a vendor 'noname' looks a bit odd and AFAICT it isn't used by
any other binding. A google search always mentions Adafruit in
connection with this panel, so I suggest we use Adafruit as vendor.

I don't think we should use "ilitek,ili9341" as an option/fallback,
because panels varies in resolution (rarely) and initialization. A
generic ili9341 driver would probably not work with a random new panel.


+- dc-gpios:D/C pin
+- reset-gpios: Reset pin
+
+The node for this driver must be a child node of a SPI controller, hence
+all mandatory properties described in ../spi/spi-bus.txt must be specified.
+
+Optional properties:
+- rotation:panel rotation in degrees counter clockwise (0,90,180,270)
+- backlight:   phandle of the backlight device attached to the panel


You forgot to mention the regulator that you support in the driver.

Noralf.


+
+Example:
+   display@0{
+   compatible = "noname,yx240qv29", "ilitek,ili9341";
+   reg = <0>;
+   spi-max-frequency = <3200>;
+   dc-gpios = < 9 GPIO_ACTIVE_HIGH>;
+   reset-gpios = < 8 GPIO_ACTIVE_HIGH>;
+   rotation = <270>;
+   backlight = <>;
+   };

Re: [PATCH 2/3] dt-bindings: new binding for Ilitek ILI9341 display panels

[Noralf Trønnes]

Den 15.05.2018 03.43, skrev David Lechner:

This adds a new binding for Ilitek ILI9341 display panels. It includes
a compatible string for one display (more can be added in the future).

The vendor prefix "noname" is used because the vendor is not known.
The YX240QV29-T panel[1] is found, for example, in an Adafruit breakout
board[2] and in Mindsensors' PiStorms[3].

[1]: 
https://cdn-learn.adafruit.com/assets/assets/000/046/879/original/SPEC-YX240QV29-T_Rev.A__1_.pdf
[2]: https://www.adafruit.com/product/2478
[3]: 
http://www.mindsensors.com/stem-with-robotics/13-pistorms-v2-base-kit-raspberry-pi-brain-for-lego-robot

Signed-off-by: David Lechner 
---
  .../bindings/display/ilitek,ili9341.txt   | 27 +++
  1 file changed, 27 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/display/ilitek,ili9341.txt

diff --git a/Documentation/devicetree/bindings/display/ilitek,ili9341.txt 
b/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
new file mode 100644
index ..0fc90b2dd732
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ilitek,ili9341.txt
@@ -0,0 +1,27 @@
+Ilitek ILI9341 display panels
+
+This binding is for display panels using an Ilitek ILI9341 controller in SPI
+mode.
+
+Required properties:
+- compatible:  "noname,yx240qv29", "ilitek,ili9341"


Calling a vendor 'noname' looks a bit odd and AFAICT it isn't used by
any other binding. A google search always mentions Adafruit in
connection with this panel, so I suggest we use Adafruit as vendor.

I don't think we should use "ilitek,ili9341" as an option/fallback,
because panels varies in resolution (rarely) and initialization. A
generic ili9341 driver would probably not work with a random new panel.


+- dc-gpios:D/C pin
+- reset-gpios: Reset pin
+
+The node for this driver must be a child node of a SPI controller, hence
+all mandatory properties described in ../spi/spi-bus.txt must be specified.
+
+Optional properties:
+- rotation:panel rotation in degrees counter clockwise (0,90,180,270)
+- backlight:   phandle of the backlight device attached to the panel


You forgot to mention the regulator that you support in the driver.

Noralf.


+
+Example:
+   display@0{
+   compatible = "noname,yx240qv29", "ilitek,ili9341";
+   reg = <0>;
+   spi-max-frequency = <3200>;
+   dc-gpios = < 9 GPIO_ACTIVE_HIGH>;
+   reset-gpios = < 8 GPIO_ACTIVE_HIGH>;
+   rotation = <270>;
+   backlight = <>;
+   };

Re: [PATCH 1/3] MAINTAINERS: fix path to ilitek,ili9225 device tree bindings

[Noralf Trønnes]

Den 15.05.2018 03.43, skrev David Lechner:

This fixes the path to the ilitek,ili9225 device tree binding file.

Signed-off-by: David Lechner <da...@lechnology.com>
---


Reviewed-by: Noralf Trønnes <nor...@tronnes.org>


  MAINTAINERS | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 334a00350922..bc219de9cbee 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4478,7 +4478,7 @@ DRM DRIVER FOR ILITEK ILI9225 PANELS
  M:David Lechner <da...@lechnology.com>
  S:Maintained
  F:drivers/gpu/drm/tinydrm/ili9225.c
-F: Documentation/devicetree/bindings/display/ili9225.txt
+F: Documentation/devicetree/bindings/display/ilitek,ili9225.txt
  
  DRM DRIVER FOR INTEL I810 VIDEO CARDS

  S:Orphan / Obsolete

Re: [PATCH 1/3] MAINTAINERS: fix path to ilitek,ili9225 device tree bindings

[Noralf Trønnes]

Den 15.05.2018 03.43, skrev David Lechner:

This fixes the path to the ilitek,ili9225 device tree binding file.

Signed-off-by: David Lechner 
---


Reviewed-by: Noralf Trønnes 


  MAINTAINERS | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 334a00350922..bc219de9cbee 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4478,7 +4478,7 @@ DRM DRIVER FOR ILITEK ILI9225 PANELS
  M:David Lechner 
  S:Maintained
  F:drivers/gpu/drm/tinydrm/ili9225.c
-F: Documentation/devicetree/bindings/display/ili9225.txt
+F: Documentation/devicetree/bindings/display/ilitek,ili9225.txt
  
  DRM DRIVER FOR INTEL I810 VIDEO CARDS

  S:Orphan / Obsolete

Re: [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Noralf Trønnes]

Den 04.04.2018 19.56, skrev Daniel Vetter:

On Wed, Apr 4, 2018 at 7:41 PM, Noralf Trønnes <nor...@tronnes.org> wrote:


Den 04.04.2018 00.42, skrev Rob Clark:

Add an atomic helper to implement dirtyfb support.  This is needed to
support DSI command-mode panels with x11 userspace (ie. when we can't
rely on pageflips to trigger a flush to the panel).

To signal to the driver that the async atomic update needs to
synchronize with fences, even though the fb didn't change, the
drm_atomic_state::dirty flag is added.

Signed-off-by: Rob Clark <robdcl...@gmail.com>
---
Background: there are a number of different folks working on getting
upstream kernel working on various different phones/tablets with qcom
SoC's.. many of them have command mode panels, so we kind of need a
way to support the legacy dirtyfb ioctl for x11 support.

I know there is work on a proprer non-legacy atomic property for
userspace to communicate dirty-rect(s) to the kernel, so this can
be improved from triggering a full-frame flush once that is in
place.  But we kinda needa a stop-gap solution.

I had considered an in-driver solution for this, but things get a
bit tricky if userspace ands up combining dirtyfb ioctls with page-
flips, because we need to synchronize setting various CTL.FLUSH bits
with setting the CTL.START bit.  (ie. really all we need to do for
cmd mode panels is bang CTL.START, but is this ends up racing with
pageflips setting FLUSH bits, then bad things.)  The easiest soln
is to wrap this up as an atomic commit and rely on the worker to
serialize things.  Hence adding an atomic dirtyfb helper.

I guess at least the helper, with some small addition to translate
and pass-thru the dirty rect(s) is useful to the final atomic dirty-
rect property solution.  Depending on how far off that is, a stop-
gap solution could be useful.


I have been waiting for someone to address this since I'm not skilled
enough myself to tackle the atomic machinery. It would be be nice to do
all flushing during commit since that would mean that the tinydrm drivers
could be driven solely through drm_simple_display_pipe_funcs. I wouldn't
have to wire through the dirty callback like I do now.

I see that you use a nonblocking commit. What happens if a new dirtyfb
comes in before the previous is done?

We could reuse the same trick we're doing in the fbdev code, of
pushing the commit to a worker and doing it in a blocking fashion. Or
at least wait for it to finish (can be done with the crtc_state->event
stuff). That way userspace can pile us up in dirtyfb calls, but we'd
do at most one per frame. More isn't really useful anyway.


If we could save the dirty clips, then I could use this in tinydrm.
A full flush over SPI takes ~50ms so I need to shave off where I can.

This works for me in my tinydrm world:

One thing I thought you've had around somewhere is some additional
tracking code, so we don't have to take all the plane mutexes in the
->dirtyfb callback. Does that exist, or was that just a figment of my
imagination?


I haven't looked at this at all since I know way too little about how
atomic works and after the discussion started with damage on page flips,
I knew I just had to wait for someone other than me to do this. And I
hardly know anything about how the multitude of userspace operates.
So I'm sorry, but I can't add much to this discussion, I fall off rather
quickly when the details and corner cases are discussed.
All I can do is state the needs of tinydrm :-)

Noralf.

Re: [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Noralf Trønnes]

Den 04.04.2018 19.56, skrev Daniel Vetter:

On Wed, Apr 4, 2018 at 7:41 PM, Noralf Trønnes  wrote:


Den 04.04.2018 00.42, skrev Rob Clark:

Add an atomic helper to implement dirtyfb support.  This is needed to
support DSI command-mode panels with x11 userspace (ie. when we can't
rely on pageflips to trigger a flush to the panel).

To signal to the driver that the async atomic update needs to
synchronize with fences, even though the fb didn't change, the
drm_atomic_state::dirty flag is added.

Signed-off-by: Rob Clark 
---
Background: there are a number of different folks working on getting
upstream kernel working on various different phones/tablets with qcom
SoC's.. many of them have command mode panels, so we kind of need a
way to support the legacy dirtyfb ioctl for x11 support.

I know there is work on a proprer non-legacy atomic property for
userspace to communicate dirty-rect(s) to the kernel, so this can
be improved from triggering a full-frame flush once that is in
place.  But we kinda needa a stop-gap solution.

I had considered an in-driver solution for this, but things get a
bit tricky if userspace ands up combining dirtyfb ioctls with page-
flips, because we need to synchronize setting various CTL.FLUSH bits
with setting the CTL.START bit.  (ie. really all we need to do for
cmd mode panels is bang CTL.START, but is this ends up racing with
pageflips setting FLUSH bits, then bad things.)  The easiest soln
is to wrap this up as an atomic commit and rely on the worker to
serialize things.  Hence adding an atomic dirtyfb helper.

I guess at least the helper, with some small addition to translate
and pass-thru the dirty rect(s) is useful to the final atomic dirty-
rect property solution.  Depending on how far off that is, a stop-
gap solution could be useful.


I have been waiting for someone to address this since I'm not skilled
enough myself to tackle the atomic machinery. It would be be nice to do
all flushing during commit since that would mean that the tinydrm drivers
could be driven solely through drm_simple_display_pipe_funcs. I wouldn't
have to wire through the dirty callback like I do now.

I see that you use a nonblocking commit. What happens if a new dirtyfb
comes in before the previous is done?

We could reuse the same trick we're doing in the fbdev code, of
pushing the commit to a worker and doing it in a blocking fashion. Or
at least wait for it to finish (can be done with the crtc_state->event
stuff). That way userspace can pile us up in dirtyfb calls, but we'd
do at most one per frame. More isn't really useful anyway.


If we could save the dirty clips, then I could use this in tinydrm.
A full flush over SPI takes ~50ms so I need to shave off where I can.

This works for me in my tinydrm world:

One thing I thought you've had around somewhere is some additional
tracking code, so we don't have to take all the plane mutexes in the
->dirtyfb callback. Does that exist, or was that just a figment of my
imagination?


I haven't looked at this at all since I know way too little about how
atomic works and after the discussion started with damage on page flips,
I knew I just had to wait for someone other than me to do this. And I
hardly know anything about how the multitude of userspace operates.
So I'm sorry, but I can't add much to this discussion, I fall off rather
quickly when the details and corner cases are discussed.
All I can do is state the needs of tinydrm :-)

Noralf.

Re: [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Noralf Trønnes]

Den 04.04.2018 00.42, skrev Rob Clark:

Add an atomic helper to implement dirtyfb support.  This is needed to
support DSI command-mode panels with x11 userspace (ie. when we can't
rely on pageflips to trigger a flush to the panel).

To signal to the driver that the async atomic update needs to
synchronize with fences, even though the fb didn't change, the
drm_atomic_state::dirty flag is added.

Signed-off-by: Rob Clark 
---
Background: there are a number of different folks working on getting
upstream kernel working on various different phones/tablets with qcom
SoC's.. many of them have command mode panels, so we kind of need a
way to support the legacy dirtyfb ioctl for x11 support.

I know there is work on a proprer non-legacy atomic property for
userspace to communicate dirty-rect(s) to the kernel, so this can
be improved from triggering a full-frame flush once that is in
place.  But we kinda needa a stop-gap solution.

I had considered an in-driver solution for this, but things get a
bit tricky if userspace ands up combining dirtyfb ioctls with page-
flips, because we need to synchronize setting various CTL.FLUSH bits
with setting the CTL.START bit.  (ie. really all we need to do for
cmd mode panels is bang CTL.START, but is this ends up racing with
pageflips setting FLUSH bits, then bad things.)  The easiest soln
is to wrap this up as an atomic commit and rely on the worker to
serialize things.  Hence adding an atomic dirtyfb helper.

I guess at least the helper, with some small addition to translate
and pass-thru the dirty rect(s) is useful to the final atomic dirty-
rect property solution.  Depending on how far off that is, a stop-
gap solution could be useful.


I have been waiting for someone to address this since I'm not skilled
enough myself to tackle the atomic machinery. It would be be nice to do
all flushing during commit since that would mean that the tinydrm drivers
could be driven solely through drm_simple_display_pipe_funcs. I wouldn't
have to wire through the dirty callback like I do now.

I see that you use a nonblocking commit. What happens if a new dirtyfb
comes in before the previous is done?

If we could save the dirty clips, then I could use this in tinydrm.
A full flush over SPI takes ~50ms so I need to shave off where I can.

This works for me in my tinydrm world:

diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index c64225274807..218cb36757fa 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -28,6 +28,7 @@
 #include 
 #include 

+struct drm_clip_rect;
 struct drm_crtc;
 struct drm_printer;
 struct drm_modeset_acquire_ctx;
@@ -85,6 +86,9 @@ struct drm_plane_state {
 */
    bool dirty;

+   struct drm_clip_rect *dirty_clips;
+   unsigned int num_dirty_clips;
+
    /**
 * @fence:
 *
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c

index 8066c508ab50..e00b8247b7c5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3521,6 +3521,8 @@ void 
__drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,

    drm_framebuffer_get(state->fb);

    state->dirty = false;
+   state->dirty_clips = NULL;
+   state->num_dirty_clips = 0;
    state->fence = NULL;
    state->commit = NULL;
 }
@@ -3567,6 +3569,8 @@ void 
__drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)


    if (state->commit)
    drm_crtc_commit_put(state->commit);
+
+   kfree(state->dirty_clips);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);

@@ -3877,8 +3881,20 @@ int drm_atomic_helper_dirtyfb(struct 
drm_framebuffer *fb,

    struct drm_modeset_acquire_ctx ctx;
    struct drm_atomic_state *state;
    struct drm_plane *plane;
+   bool fb_found = false;
    int ret = 0;

+   /* fbdev can flush even when we don't want it to */
+   drm_for_each_plane(plane, fb->dev) {
+   if (plane->state->fb == fb) {
+   fb_found = true;
+   break;
+   }
+   }
+
+   if (!fb_found)
+   return 0;
+
    /*
 * When called from ioctl, we are interruptable, but not when
 * called internally (ie. defio worker)
@@ -3907,6 +3923,25 @@ int drm_atomic_helper_dirtyfb(struct 
drm_framebuffer *fb,

    }

    plane_state->dirty = true;
+   if (clips && num_clips)
+   plane_state->dirty_clips = kmemdup(clips, 
num_clips * sizeof(*clips),

+ GFP_KERNEL);
+   else
+   plane_state->dirty_clips = 
kzalloc(sizeof(*clips), GFP_KERNEL);

+
+   if (!plane_state->dirty_clips) {
+   ret = -ENOMEM;
+   goto out;
+   }
+
+   if (clips && num_clips) {
+   plane_state->num_dirty_clips = 

Re: [RFC] drm/atomic+msm: add helper to implement legacy dirtyfb

[Noralf Trønnes]

Den 04.04.2018 00.42, skrev Rob Clark:

Add an atomic helper to implement dirtyfb support.  This is needed to
support DSI command-mode panels with x11 userspace (ie. when we can't
rely on pageflips to trigger a flush to the panel).

To signal to the driver that the async atomic update needs to
synchronize with fences, even though the fb didn't change, the
drm_atomic_state::dirty flag is added.

Signed-off-by: Rob Clark 
---
Background: there are a number of different folks working on getting
upstream kernel working on various different phones/tablets with qcom
SoC's.. many of them have command mode panels, so we kind of need a
way to support the legacy dirtyfb ioctl for x11 support.

I know there is work on a proprer non-legacy atomic property for
userspace to communicate dirty-rect(s) to the kernel, so this can
be improved from triggering a full-frame flush once that is in
place.  But we kinda needa a stop-gap solution.

I had considered an in-driver solution for this, but things get a
bit tricky if userspace ands up combining dirtyfb ioctls with page-
flips, because we need to synchronize setting various CTL.FLUSH bits
with setting the CTL.START bit.  (ie. really all we need to do for
cmd mode panels is bang CTL.START, but is this ends up racing with
pageflips setting FLUSH bits, then bad things.)  The easiest soln
is to wrap this up as an atomic commit and rely on the worker to
serialize things.  Hence adding an atomic dirtyfb helper.

I guess at least the helper, with some small addition to translate
and pass-thru the dirty rect(s) is useful to the final atomic dirty-
rect property solution.  Depending on how far off that is, a stop-
gap solution could be useful.


I have been waiting for someone to address this since I'm not skilled
enough myself to tackle the atomic machinery. It would be be nice to do
all flushing during commit since that would mean that the tinydrm drivers
could be driven solely through drm_simple_display_pipe_funcs. I wouldn't
have to wire through the dirty callback like I do now.

I see that you use a nonblocking commit. What happens if a new dirtyfb
comes in before the previous is done?

If we could save the dirty clips, then I could use this in tinydrm.
A full flush over SPI takes ~50ms so I need to shave off where I can.

This works for me in my tinydrm world:

diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index c64225274807..218cb36757fa 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -28,6 +28,7 @@
 #include 
 #include 

+struct drm_clip_rect;
 struct drm_crtc;
 struct drm_printer;
 struct drm_modeset_acquire_ctx;
@@ -85,6 +86,9 @@ struct drm_plane_state {
 */
    bool dirty;

+   struct drm_clip_rect *dirty_clips;
+   unsigned int num_dirty_clips;
+
    /**
 * @fence:
 *
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c

index 8066c508ab50..e00b8247b7c5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3521,6 +3521,8 @@ void 
__drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,

    drm_framebuffer_get(state->fb);

    state->dirty = false;
+   state->dirty_clips = NULL;
+   state->num_dirty_clips = 0;
    state->fence = NULL;
    state->commit = NULL;
 }
@@ -3567,6 +3569,8 @@ void 
__drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)


    if (state->commit)
    drm_crtc_commit_put(state->commit);
+
+   kfree(state->dirty_clips);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);

@@ -3877,8 +3881,20 @@ int drm_atomic_helper_dirtyfb(struct 
drm_framebuffer *fb,

    struct drm_modeset_acquire_ctx ctx;
    struct drm_atomic_state *state;
    struct drm_plane *plane;
+   bool fb_found = false;
    int ret = 0;

+   /* fbdev can flush even when we don't want it to */
+   drm_for_each_plane(plane, fb->dev) {
+   if (plane->state->fb == fb) {
+   fb_found = true;
+   break;
+   }
+   }
+
+   if (!fb_found)
+   return 0;
+
    /*
 * When called from ioctl, we are interruptable, but not when
 * called internally (ie. defio worker)
@@ -3907,6 +3923,25 @@ int drm_atomic_helper_dirtyfb(struct 
drm_framebuffer *fb,

    }

    plane_state->dirty = true;
+   if (clips && num_clips)
+   plane_state->dirty_clips = kmemdup(clips, 
num_clips * sizeof(*clips),

+ GFP_KERNEL);
+   else
+   plane_state->dirty_clips = 
kzalloc(sizeof(*clips), GFP_KERNEL);

+
+   if (!plane_state->dirty_clips) {
+   ret = -ENOMEM;
+   goto out;
+   }
+
+   if (clips && num_clips) {
+   plane_state->num_dirty_clips = num_clips;
+   } 

Re: [PATCH] drm/tinydrm: Allocate dummy SPI RX buffer if needed

[Noralf Trønnes]

Den 20.02.2018 00.21, skrev David Lechner:

This adds a new feature to allocate a dummy RX buffer for SPI controllers
that require an RX buffer even when not receiving. If an RX buffer is not
supplied, the SPI controller will reallocate a new buffer on each
transfer.

On systems with limited memory (e.g. 64MB and CONFIG_SWAP=n), this
reallocation will occasionally fail, which causes the display to stop
working. By allocating this buffer once and reusing it, we can prevent
this problem.

This patch also introduces some helper functions for calculating the
size of this buffer, so these functions are re-used where possible
(e.g. in mipi_dbi_init()).

Cc: Noralf Trønnes <nor...@tronnes.org>
Suggested-by: Noralf Trønnes <nor...@tronnes.org>
Signed-off-by: David Lechner <da...@lechnology.com>
---

This is a follow up from the mail thread "tinydrm: page allocation failure" [1].
I actually got an allocation failure a few times even when I had swap enabled,
so I think this change is needed.

[1]: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg203657.html


I was hoping that we could avoid touching drivers with this change.

In the current situation it would be enough to allocate a buffer the size
of what tinydrm_spi_max_transfer_size() returns, because that's the
maximun transfers length used in tinydrm_spi_transfer().

However Meghana is trying to remove the transfer buffer splitting [1],
so I think we should wait and see the outcome of that first.

If in the end we do send the entire buffer, maybe we can defer
allocating the rx buffer until the first time it is used so we can
know the buffer size without asking the drivers.

If we add this and set it in mipi_dbi_init():

 struct mipi_dbi {
    u16 *tx_buf;
+    size_t tx_len;
 };

We can now allocate the rx buffer in mipi_dbi_typec3_command():

static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
                   u8 *par, size_t num)
{

+    if (!mipi->rx_buf && spi->controller->flags & SPI_CONTROLLER_MUST_RX) {
+        mipi->rx_buf = devm_kmalloc(dev, mipi->tx_len, GFP_KERNEL);
+        if (!mipi->rx_buf)
+            return -ENOMEM;
+    }

}

Option 1 has it's own transfer buffer and size:

static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
                   u8 *parameters, size_t num)
{

+        mipi->rx_buf = devm_kmalloc(dev, mipi->tx_buf9_len, GFP_KERNEL);

}


Hopefully before summer I have switched to vmalloc backing for the
framebuffers (which means we can PRIME import from GPU's with shmem
buffers). This means that we can just use vmalloc transfer buffers as
well. The SPI core can dma map vmalloc buffers (spi_map_buf), it just
makes an SG table with an entry per PAGE.

[1]: [PATCH v2 0/2] Chunk splitting of spi transfers
https://lists.freedesktop.org/archives/dri-devel/2018-February/167181.html

Noralf.


  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c |  9 -
  drivers/gpu/drm/tinydrm/ili9225.c  |  8 +---
  drivers/gpu/drm/tinydrm/mi0283qt.c |  3 ++-
  drivers/gpu/drm/tinydrm/mipi-dbi.c | 20 
  drivers/gpu/drm/tinydrm/st7586.c   | 13 +++--
  drivers/gpu/drm/tinydrm/st7735r.c  |  3 ++-
  include/drm/tinydrm/mipi-dbi.h | 16 +++-
  include/drm/tinydrm/tinydrm-helpers.h  |  2 +-
  8 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c 
b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index bf96072..f5c175e 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -434,6 +434,7 @@ EXPORT_SYMBOL(_tinydrm_dbg_spi_message);
   * @header: Optional header transfer
   * @bpw: Bits per word
   * @buf: Buffer to transfer
+ * @rx_buf: Optional dummy buffer
   * @len: Buffer length
   *
   * This SPI transfer helper breaks up the transfer of @buf into chunks which
@@ -442,16 +443,22 @@ EXPORT_SYMBOL(_tinydrm_dbg_spi_message);
   * does a 8-bit transfer.
   * If @header is set, it is prepended to each SPI message.
   *
+ * Some SPI controllers need an RX buffer even though we are not receiving
+ * anything useful. @rx_buf can be provided so that the SPI controller does not
+ * have to reallocate this buffer on each transfer. This is useful for large
+ * transfers, e.g. when updating the GRAM.
+ *
   * Returns:
   * Zero on success, negative error code on failure.
   */
  int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
 struct spi_transfer *header, u8 bpw, const void *buf,
-size_t len)
+void *rx_buf, size_t len)
  {
struct spi_transfer tr = {
.bits_per_word = bpw,
.speed_hz = speed_hz,
+   .rx_buf = rx_buf,
};
struct spi_message m;
u16 *swap_buf =

Re: [PATCH] drm/tinydrm: Allocate dummy SPI RX buffer if needed

[Noralf Trønnes]

Den 20.02.2018 00.21, skrev David Lechner:

This adds a new feature to allocate a dummy RX buffer for SPI controllers
that require an RX buffer even when not receiving. If an RX buffer is not
supplied, the SPI controller will reallocate a new buffer on each
transfer.

On systems with limited memory (e.g. 64MB and CONFIG_SWAP=n), this
reallocation will occasionally fail, which causes the display to stop
working. By allocating this buffer once and reusing it, we can prevent
this problem.

This patch also introduces some helper functions for calculating the
size of this buffer, so these functions are re-used where possible
(e.g. in mipi_dbi_init()).

Cc: Noralf Trønnes 
Suggested-by: Noralf Trønnes 
Signed-off-by: David Lechner 
---

This is a follow up from the mail thread "tinydrm: page allocation failure" [1].
I actually got an allocation failure a few times even when I had swap enabled,
so I think this change is needed.

[1]: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg203657.html


I was hoping that we could avoid touching drivers with this change.

In the current situation it would be enough to allocate a buffer the size
of what tinydrm_spi_max_transfer_size() returns, because that's the
maximun transfers length used in tinydrm_spi_transfer().

However Meghana is trying to remove the transfer buffer splitting [1],
so I think we should wait and see the outcome of that first.

If in the end we do send the entire buffer, maybe we can defer
allocating the rx buffer until the first time it is used so we can
know the buffer size without asking the drivers.

If we add this and set it in mipi_dbi_init():

 struct mipi_dbi {
    u16 *tx_buf;
+    size_t tx_len;
 };

We can now allocate the rx buffer in mipi_dbi_typec3_command():

static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
                   u8 *par, size_t num)
{

+    if (!mipi->rx_buf && spi->controller->flags & SPI_CONTROLLER_MUST_RX) {
+        mipi->rx_buf = devm_kmalloc(dev, mipi->tx_len, GFP_KERNEL);
+        if (!mipi->rx_buf)
+            return -ENOMEM;
+    }

}

Option 1 has it's own transfer buffer and size:

static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
                   u8 *parameters, size_t num)
{

+        mipi->rx_buf = devm_kmalloc(dev, mipi->tx_buf9_len, GFP_KERNEL);

}


Hopefully before summer I have switched to vmalloc backing for the
framebuffers (which means we can PRIME import from GPU's with shmem
buffers). This means that we can just use vmalloc transfer buffers as
well. The SPI core can dma map vmalloc buffers (spi_map_buf), it just
makes an SG table with an entry per PAGE.

[1]: [PATCH v2 0/2] Chunk splitting of spi transfers
https://lists.freedesktop.org/archives/dri-devel/2018-February/167181.html

Noralf.


  drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c |  9 -
  drivers/gpu/drm/tinydrm/ili9225.c  |  8 +---
  drivers/gpu/drm/tinydrm/mi0283qt.c |  3 ++-
  drivers/gpu/drm/tinydrm/mipi-dbi.c | 20 
  drivers/gpu/drm/tinydrm/st7586.c   | 13 +++--
  drivers/gpu/drm/tinydrm/st7735r.c  |  3 ++-
  include/drm/tinydrm/mipi-dbi.h | 16 +++-
  include/drm/tinydrm/tinydrm-helpers.h  |  2 +-
  8 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c 
b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index bf96072..f5c175e 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -434,6 +434,7 @@ EXPORT_SYMBOL(_tinydrm_dbg_spi_message);
   * @header: Optional header transfer
   * @bpw: Bits per word
   * @buf: Buffer to transfer
+ * @rx_buf: Optional dummy buffer
   * @len: Buffer length
   *
   * This SPI transfer helper breaks up the transfer of @buf into chunks which
@@ -442,16 +443,22 @@ EXPORT_SYMBOL(_tinydrm_dbg_spi_message);
   * does a 8-bit transfer.
   * If @header is set, it is prepended to each SPI message.
   *
+ * Some SPI controllers need an RX buffer even though we are not receiving
+ * anything useful. @rx_buf can be provided so that the SPI controller does not
+ * have to reallocate this buffer on each transfer. This is useful for large
+ * transfers, e.g. when updating the GRAM.
+ *
   * Returns:
   * Zero on success, negative error code on failure.
   */
  int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz,
 struct spi_transfer *header, u8 bpw, const void *buf,
-size_t len)
+void *rx_buf, size_t len)
  {
struct spi_transfer tr = {
.bits_per_word = bpw,
.speed_hz = speed_hz,
+   .rx_buf = rx_buf,
};
struct spi_message m;
u16 *swap_buf = NULL;
diff --git a/drivers/gpu/drm/tinydrm/ili9225.c 
b/drivers/gpu/drm/t

Re: [PATCH v19 10/10] drm/omapdrm: Use of_find_backlight helper

[Noralf Trønnes]

Den 24.01.2018 17.41, skrev Meghana Madhyastha:

Replace of_find_backlight_by_node and of the code around it
with of_find_backlight helper to avoid repetition of code.

Signed-off-by: Meghana Madhyastha <meghana.madhyas...@gmail.com>
---


Reviewed-by: Noralf Trønnes <nor...@tronnes.org>


Changes in v19:
-Changed to devm version of of_find_backlight in omapdrm (patch 10)
-removed assigning pdev->dev to variable dev in omapdrm (patch 10)

  drivers/gpu/drm/omapdrm/displays/panel-dpi.c | 25 +
  1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c 
b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
index ac9596251..b4a4006a2 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
@@ -157,7 +157,6 @@ static int panel_dpi_probe_of(struct platform_device *pdev)
  {
struct panel_drv_data *ddata = platform_get_drvdata(pdev);
struct device_node *node = pdev->dev.of_node;
-   struct device_node *bl_node;
struct omap_dss_device *in;
int r;
struct display_timing timing;
@@ -183,19 +182,15 @@ static int panel_dpi_probe_of(struct platform_device 
*pdev)
if (IS_ERR(ddata->vcc_supply))
return PTR_ERR(ddata->vcc_supply);
  
-	bl_node = of_parse_phandle(node, "backlight", 0);

-   if (bl_node) {
-   ddata->backlight = of_find_backlight_by_node(bl_node);
-   of_node_put(bl_node);
+   ddata->backlight = devm_of_find_backlight(>dev);
  
-		if (!ddata->backlight)

-   return -EPROBE_DEFER;
-   }
+   if (IS_ERR(ddata->backlight))
+   return PTR_ERR(ddata->backlight);
  
  	r = of_get_display_timing(node, "panel-timing", );

if (r) {
dev_err(>dev, "failed to get video timing\n");
-   goto error_free_backlight;
+   return r;
}
  
  	videomode_from_timing(, >vm);

@@ -203,19 +198,12 @@ static int panel_dpi_probe_of(struct platform_device 
*pdev)
in = omapdss_of_find_source_for_first_ep(node);
if (IS_ERR(in)) {
dev_err(>dev, "failed to find video source\n");
-   r = PTR_ERR(in);
-   goto error_free_backlight;
+   return PTR_ERR(in);
}
  
  	ddata->in = in;
  
  	return 0;

-
-error_free_backlight:
-   if (ddata->backlight)
-   put_device(>backlight->dev);
-
-   return r;
  }
  
  static int panel_dpi_probe(struct platform_device *pdev)

@@ -270,9 +258,6 @@ static int __exit panel_dpi_remove(struct platform_device 
*pdev)
  
  	omap_dss_put_device(in);
  
-	if (ddata->backlight)

-   put_device(>backlight->dev);
-
return 0;
  }
  

Re: [PATCH v19 10/10] drm/omapdrm: Use of_find_backlight helper

[Noralf Trønnes]

Den 24.01.2018 17.41, skrev Meghana Madhyastha:

Replace of_find_backlight_by_node and of the code around it
with of_find_backlight helper to avoid repetition of code.

Signed-off-by: Meghana Madhyastha 
---


Reviewed-by: Noralf Trønnes 


Changes in v19:
-Changed to devm version of of_find_backlight in omapdrm (patch 10)
-removed assigning pdev->dev to variable dev in omapdrm (patch 10)

  drivers/gpu/drm/omapdrm/displays/panel-dpi.c | 25 +
  1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c 
b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
index ac9596251..b4a4006a2 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
@@ -157,7 +157,6 @@ static int panel_dpi_probe_of(struct platform_device *pdev)
  {
struct panel_drv_data *ddata = platform_get_drvdata(pdev);
struct device_node *node = pdev->dev.of_node;
-   struct device_node *bl_node;
struct omap_dss_device *in;
int r;
struct display_timing timing;
@@ -183,19 +182,15 @@ static int panel_dpi_probe_of(struct platform_device 
*pdev)
if (IS_ERR(ddata->vcc_supply))
return PTR_ERR(ddata->vcc_supply);
  
-	bl_node = of_parse_phandle(node, "backlight", 0);

-   if (bl_node) {
-   ddata->backlight = of_find_backlight_by_node(bl_node);
-   of_node_put(bl_node);
+   ddata->backlight = devm_of_find_backlight(>dev);
  
-		if (!ddata->backlight)

-   return -EPROBE_DEFER;
-   }
+   if (IS_ERR(ddata->backlight))
+   return PTR_ERR(ddata->backlight);
  
  	r = of_get_display_timing(node, "panel-timing", );

if (r) {
dev_err(>dev, "failed to get video timing\n");
-   goto error_free_backlight;
+   return r;
}
  
  	videomode_from_timing(, >vm);

@@ -203,19 +198,12 @@ static int panel_dpi_probe_of(struct platform_device 
*pdev)
in = omapdss_of_find_source_for_first_ep(node);
if (IS_ERR(in)) {
dev_err(>dev, "failed to find video source\n");
-   r = PTR_ERR(in);
-   goto error_free_backlight;
+   return PTR_ERR(in);
}
  
  	ddata->in = in;
  
  	return 0;

-
-error_free_backlight:
-   if (ddata->backlight)
-   put_device(>backlight->dev);
-
-   return r;
  }
  
  static int panel_dpi_probe(struct platform_device *pdev)

@@ -270,9 +258,6 @@ static int __exit panel_dpi_remove(struct platform_device 
*pdev)
  
  	omap_dss_put_device(in);
  
-	if (ddata->backlight)

-   put_device(>backlight->dev);
-
return 0;
  }
  

Re: [PATCH v18 10/10] drm/omapdrm: Use of_find_backlight helper

[Noralf Trønnes]

Den 23.01.2018 18.41, skrev Noralf Trønnes:


Den 23.01.2018 17.55, skrev Meghana Madhyastha:

On Tue, Jan 23, 2018 at 03:37:38PM +0100, Noralf Trønnes wrote:

Den 22.01.2018 15.56, skrev Meghana Madhyastha:

Replace of_find_backlight_by_node and of the code around it
with of_find_backlight helper to avoid repetition of code.

Signed-off-by: Meghana Madhyastha <meghana.madhyas...@gmail.com>
---
Changes in v18:
-Fixed warnings resulting from passing device_node* to 
of_find_backlight.

  Fixed it by passing struct device* to of_find_backlight

  drivers/gpu/drm/omapdrm/displays/panel-dpi.c | 33 
++--

  1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c 
b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c

index ac9596251..93b7a176d 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
@@ -156,14 +156,14 @@ static struct omap_dss_driver panel_dpi_ops = {
  static int panel_dpi_probe_of(struct platform_device *pdev)
  {
  struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+    struct device *dev = >dev;
  struct device_node *node = pdev->dev.of_node;
-    struct device_node *bl_node;
  struct omap_dss_device *in;
  int r;
  struct display_timing timing;
  struct gpio_desc *gpio;
-    gpio = devm_gpiod_get_optional(>dev, "enable", 
GPIOD_OUT_LOW);

+    gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);

Please don't make unrelated changes like this. It clutters the patch.
You can just as well use >dev when getting the backlight also.

I had made the change in order to be more consistent with how the other
drivers were doing this. Most of them had a variable struct device *dev.
However, I can undo this if necessary.


It's best to be consistent with the coding style in the driver you're
changing. If you make an extra dev variable or not isn't that important,
unless the driver maintainer have a strict coding style for their driver.

I try to stay on the safe side, change as little as possible and do thing
the way it's done in the driver to increase the change of getting the
patch accepted as-is the first time around.

The important feedback from me is to remove the unrelated changes.



and the use of the devm_ version ofc.


Noralf.




  if (IS_ERR(gpio))
  return PTR_ERR(gpio);
@@ -175,47 +175,36 @@ static int panel_dpi_probe_of(struct 
platform_device *pdev)
   * timing and order relative to the enable gpio. So for now 
it's just

   * ensured that the reset line isn't active.
   */
-    gpio = devm_gpiod_get_optional(>dev, "reset", 
GPIOD_OUT_LOW);

+    gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
  if (IS_ERR(gpio))
  return PTR_ERR(gpio);
-    ddata->vcc_supply = devm_regulator_get(>dev, "vcc");
+    ddata->vcc_supply = devm_regulator_get(dev, "vcc");
  if (IS_ERR(ddata->vcc_supply))
  return PTR_ERR(ddata->vcc_supply);
-    bl_node = of_parse_phandle(node, "backlight", 0);
-    if (bl_node) {
-    ddata->backlight = of_find_backlight_by_node(bl_node);
-    of_node_put(bl_node);
+    ddata->backlight = of_find_backlight(dev);

Any reason you don't use the devm_ version here?
You do remove error_free_backlight...

With the devm_ version remember to drop the put_device in
panel_dpi_remove().

Noralf.


-    if (!ddata->backlight)
-    return -EPROBE_DEFER;
-    }
+    if (IS_ERR(ddata->backlight))
+    return PTR_ERR(ddata->backlight);
  r = of_get_display_timing(node, "panel-timing", );
  if (r) {
-    dev_err(>dev, "failed to get video timing\n");
-    goto error_free_backlight;
+    dev_err(dev, "failed to get video timing\n");
+    return r;
  }
  videomode_from_timing(, >vm);
  in = omapdss_of_find_source_for_first_ep(node);
  if (IS_ERR(in)) {
-    dev_err(>dev, "failed to find video source\n");
-    r = PTR_ERR(in);
-    goto error_free_backlight;
+    dev_err(dev, "failed to find video source\n");
+    return PTR_ERR(in);
  }
  ddata->in = in;
  return 0;
-
-error_free_backlight:
-    if (ddata->backlight)
-    put_device(>backlight->dev);
-
-    return r;
  }
  static int panel_dpi_probe(struct platform_device *pdev)


___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v18 10/10] drm/omapdrm: Use of_find_backlight helper

[Noralf Trønnes]

Den 23.01.2018 18.41, skrev Noralf Trønnes:


Den 23.01.2018 17.55, skrev Meghana Madhyastha:

On Tue, Jan 23, 2018 at 03:37:38PM +0100, Noralf Trønnes wrote:

Den 22.01.2018 15.56, skrev Meghana Madhyastha:

Replace of_find_backlight_by_node and of the code around it
with of_find_backlight helper to avoid repetition of code.

Signed-off-by: Meghana Madhyastha 
---
Changes in v18:
-Fixed warnings resulting from passing device_node* to 
of_find_backlight.

  Fixed it by passing struct device* to of_find_backlight

  drivers/gpu/drm/omapdrm/displays/panel-dpi.c | 33 
++--

  1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c 
b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c

index ac9596251..93b7a176d 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
@@ -156,14 +156,14 @@ static struct omap_dss_driver panel_dpi_ops = {
  static int panel_dpi_probe_of(struct platform_device *pdev)
  {
  struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+    struct device *dev = >dev;
  struct device_node *node = pdev->dev.of_node;
-    struct device_node *bl_node;
  struct omap_dss_device *in;
  int r;
  struct display_timing timing;
  struct gpio_desc *gpio;
-    gpio = devm_gpiod_get_optional(>dev, "enable", 
GPIOD_OUT_LOW);

+    gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);

Please don't make unrelated changes like this. It clutters the patch.
You can just as well use >dev when getting the backlight also.

I had made the change in order to be more consistent with how the other
drivers were doing this. Most of them had a variable struct device *dev.
However, I can undo this if necessary.


It's best to be consistent with the coding style in the driver you're
changing. If you make an extra dev variable or not isn't that important,
unless the driver maintainer have a strict coding style for their driver.

I try to stay on the safe side, change as little as possible and do thing
the way it's done in the driver to increase the change of getting the
patch accepted as-is the first time around.

The important feedback from me is to remove the unrelated changes.



and the use of the devm_ version ofc.


Noralf.




  if (IS_ERR(gpio))
  return PTR_ERR(gpio);
@@ -175,47 +175,36 @@ static int panel_dpi_probe_of(struct 
platform_device *pdev)
   * timing and order relative to the enable gpio. So for now 
it's just

   * ensured that the reset line isn't active.
   */
-    gpio = devm_gpiod_get_optional(>dev, "reset", 
GPIOD_OUT_LOW);

+    gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
  if (IS_ERR(gpio))
  return PTR_ERR(gpio);
-    ddata->vcc_supply = devm_regulator_get(>dev, "vcc");
+    ddata->vcc_supply = devm_regulator_get(dev, "vcc");
  if (IS_ERR(ddata->vcc_supply))
  return PTR_ERR(ddata->vcc_supply);
-    bl_node = of_parse_phandle(node, "backlight", 0);
-    if (bl_node) {
-    ddata->backlight = of_find_backlight_by_node(bl_node);
-    of_node_put(bl_node);
+    ddata->backlight = of_find_backlight(dev);

Any reason you don't use the devm_ version here?
You do remove error_free_backlight...

With the devm_ version remember to drop the put_device in
panel_dpi_remove().

Noralf.


-    if (!ddata->backlight)
-    return -EPROBE_DEFER;
-    }
+    if (IS_ERR(ddata->backlight))
+    return PTR_ERR(ddata->backlight);
  r = of_get_display_timing(node, "panel-timing", );
  if (r) {
-    dev_err(>dev, "failed to get video timing\n");
-    goto error_free_backlight;
+    dev_err(dev, "failed to get video timing\n");
+    return r;
  }
  videomode_from_timing(, >vm);
  in = omapdss_of_find_source_for_first_ep(node);
  if (IS_ERR(in)) {
-    dev_err(>dev, "failed to find video source\n");
-    r = PTR_ERR(in);
-    goto error_free_backlight;
+    dev_err(dev, "failed to find video source\n");
+    return PTR_ERR(in);
  }
  ddata->in = in;
  return 0;
-
-error_free_backlight:
-    if (ddata->backlight)
-    put_device(>backlight->dev);
-
-    return r;
  }
  static int panel_dpi_probe(struct platform_device *pdev)


___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v18 10/10] drm/omapdrm: Use of_find_backlight helper

[Noralf Trønnes]

Den 23.01.2018 17.55, skrev Meghana Madhyastha:

On Tue, Jan 23, 2018 at 03:37:38PM +0100, Noralf Trønnes wrote:

Den 22.01.2018 15.56, skrev Meghana Madhyastha:

Replace of_find_backlight_by_node and of the code around it
with of_find_backlight helper to avoid repetition of code.

Signed-off-by: Meghana Madhyastha <meghana.madhyas...@gmail.com>
---
Changes in v18:
-Fixed warnings resulting from passing device_node* to of_find_backlight.
  Fixed it by passing struct device* to of_find_backlight

  drivers/gpu/drm/omapdrm/displays/panel-dpi.c | 33 ++--
  1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c 
b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
index ac9596251..93b7a176d 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
@@ -156,14 +156,14 @@ static struct omap_dss_driver panel_dpi_ops = {
  static int panel_dpi_probe_of(struct platform_device *pdev)
  {
struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+   struct device *dev = >dev;
struct device_node *node = pdev->dev.of_node;
-   struct device_node *bl_node;
struct omap_dss_device *in;
int r;
struct display_timing timing;
struct gpio_desc *gpio;
-   gpio = devm_gpiod_get_optional(>dev, "enable", GPIOD_OUT_LOW);
+   gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);

Please don't make unrelated changes like this. It clutters the patch.
You can just as well use >dev when getting the backlight also.

I had made the change in order to be more consistent with how the other
drivers were doing this. Most of them had a variable struct device *dev.
However, I can undo this if necessary.


It's best to be consistent with the coding style in the driver you're
changing. If you make an extra dev variable or not isn't that important,
unless the driver maintainer have a strict coding style for their driver.

I try to stay on the safe side, change as little as possible and do thing
the way it's done in the driver to increase the change of getting the
patch accepted as-is the first time around.

The important feedback from me is to remove the unrelated changes.

Noralf.




if (IS_ERR(gpio))
return PTR_ERR(gpio);
@@ -175,47 +175,36 @@ static int panel_dpi_probe_of(struct platform_device 
*pdev)
 * timing and order relative to the enable gpio. So for now it's just
 * ensured that the reset line isn't active.
 */
-   gpio = devm_gpiod_get_optional(>dev, "reset", GPIOD_OUT_LOW);
+   gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(gpio))
return PTR_ERR(gpio);
-   ddata->vcc_supply = devm_regulator_get(>dev, "vcc");
+   ddata->vcc_supply = devm_regulator_get(dev, "vcc");
if (IS_ERR(ddata->vcc_supply))
return PTR_ERR(ddata->vcc_supply);
-   bl_node = of_parse_phandle(node, "backlight", 0);
-   if (bl_node) {
-   ddata->backlight = of_find_backlight_by_node(bl_node);
-   of_node_put(bl_node);
+   ddata->backlight = of_find_backlight(dev);

Any reason you don't use the devm_ version here?
You do remove error_free_backlight...

With the devm_ version remember to drop the put_device in
panel_dpi_remove().

Noralf.


-   if (!ddata->backlight)
-   return -EPROBE_DEFER;
-   }
+   if (IS_ERR(ddata->backlight))
+   return PTR_ERR(ddata->backlight);
r = of_get_display_timing(node, "panel-timing", );
if (r) {
-   dev_err(>dev, "failed to get video timing\n");
-   goto error_free_backlight;
+   dev_err(dev, "failed to get video timing\n");
+   return r;
}
videomode_from_timing(, >vm);
in = omapdss_of_find_source_for_first_ep(node);
if (IS_ERR(in)) {
-   dev_err(>dev, "failed to find video source\n");
-   r = PTR_ERR(in);
-   goto error_free_backlight;
+   dev_err(dev, "failed to find video source\n");
+   return PTR_ERR(in);
}
ddata->in = in;
return 0;
-
-error_free_backlight:
-   if (ddata->backlight)
-   put_device(>backlight->dev);
-
-   return r;
  }
  static int panel_dpi_probe(struct platform_device *pdev)

Re: [PATCH v18 10/10] drm/omapdrm: Use of_find_backlight helper

[Noralf Trønnes]

Den 23.01.2018 17.55, skrev Meghana Madhyastha:

On Tue, Jan 23, 2018 at 03:37:38PM +0100, Noralf Trønnes wrote:

Den 22.01.2018 15.56, skrev Meghana Madhyastha:

Replace of_find_backlight_by_node and of the code around it
with of_find_backlight helper to avoid repetition of code.

Signed-off-by: Meghana Madhyastha 
---
Changes in v18:
-Fixed warnings resulting from passing device_node* to of_find_backlight.
  Fixed it by passing struct device* to of_find_backlight

  drivers/gpu/drm/omapdrm/displays/panel-dpi.c | 33 ++--
  1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c 
b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
index ac9596251..93b7a176d 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
@@ -156,14 +156,14 @@ static struct omap_dss_driver panel_dpi_ops = {
  static int panel_dpi_probe_of(struct platform_device *pdev)
  {
struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+   struct device *dev = >dev;
struct device_node *node = pdev->dev.of_node;
-   struct device_node *bl_node;
struct omap_dss_device *in;
int r;
struct display_timing timing;
struct gpio_desc *gpio;
-   gpio = devm_gpiod_get_optional(>dev, "enable", GPIOD_OUT_LOW);
+   gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);

Please don't make unrelated changes like this. It clutters the patch.
You can just as well use >dev when getting the backlight also.

I had made the change in order to be more consistent with how the other
drivers were doing this. Most of them had a variable struct device *dev.
However, I can undo this if necessary.


It's best to be consistent with the coding style in the driver you're
changing. If you make an extra dev variable or not isn't that important,
unless the driver maintainer have a strict coding style for their driver.

I try to stay on the safe side, change as little as possible and do thing
the way it's done in the driver to increase the change of getting the
patch accepted as-is the first time around.

The important feedback from me is to remove the unrelated changes.

Noralf.




if (IS_ERR(gpio))
return PTR_ERR(gpio);
@@ -175,47 +175,36 @@ static int panel_dpi_probe_of(struct platform_device 
*pdev)
 * timing and order relative to the enable gpio. So for now it's just
 * ensured that the reset line isn't active.
 */
-   gpio = devm_gpiod_get_optional(>dev, "reset", GPIOD_OUT_LOW);
+   gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(gpio))
return PTR_ERR(gpio);
-   ddata->vcc_supply = devm_regulator_get(>dev, "vcc");
+   ddata->vcc_supply = devm_regulator_get(dev, "vcc");
if (IS_ERR(ddata->vcc_supply))
return PTR_ERR(ddata->vcc_supply);
-   bl_node = of_parse_phandle(node, "backlight", 0);
-   if (bl_node) {
-   ddata->backlight = of_find_backlight_by_node(bl_node);
-   of_node_put(bl_node);
+   ddata->backlight = of_find_backlight(dev);

Any reason you don't use the devm_ version here?
You do remove error_free_backlight...

With the devm_ version remember to drop the put_device in
panel_dpi_remove().

Noralf.


-   if (!ddata->backlight)
-   return -EPROBE_DEFER;
-   }
+   if (IS_ERR(ddata->backlight))
+   return PTR_ERR(ddata->backlight);
r = of_get_display_timing(node, "panel-timing", );
if (r) {
-   dev_err(>dev, "failed to get video timing\n");
-   goto error_free_backlight;
+   dev_err(dev, "failed to get video timing\n");
+   return r;
}
videomode_from_timing(, >vm);
in = omapdss_of_find_source_for_first_ep(node);
if (IS_ERR(in)) {
-   dev_err(>dev, "failed to find video source\n");
-   r = PTR_ERR(in);
-   goto error_free_backlight;
+   dev_err(dev, "failed to find video source\n");
+   return PTR_ERR(in);
}
ddata->in = in;
return 0;
-
-error_free_backlight:
-   if (ddata->backlight)
-   put_device(>backlight->dev);
-
-   return r;
  }
  static int panel_dpi_probe(struct platform_device *pdev)

Re: [PATCH v18 01/10] video: backlight: Add helpers to enable and disable backlight

[Noralf Trønnes]

Den 23.01.2018 11.00, skrev Daniel Thompson:

On Mon, Jan 22, 2018 at 02:49:28PM +, Meghana Madhyastha wrote:

Add helper functions backlight_enable and backlight_disable to
enable/disable a backlight device. These helper functions can
then be used by different drm and tinydrm drivers to avoid
repetition of code and also to enforce a uniform and consistent
way to enable/disable a backlight device.

Signed-off-by: Meghana Madhyastha <meghana.madhyas...@gmail.com>
---
Acked-by: Daniel Thompson <daniel.thomp...@linaro.org>
Reviewed-by: Noralf Trønnes <nor...@tronnes.org>
Reviewed-by: Sean Paul<seanp...@chromium.org>

Just in case there's another spin...  the additional tags are normally
presented above the --- (otherwise they are unlikely to be copied into
the git history when they are applied).


Indeed, they should be part of the commit message.

Noralf.



Daniel.



  include/linux/backlight.h | 32 
  1 file changed, 32 insertions(+)

diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index af7003548..ace825e2c 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -130,6 +130,38 @@ static inline int backlight_update_status(struct 
backlight_device *bd)
return ret;
  }
  
+/**

+ * backlight_enable - Enable backlight
+ * @bd: the backlight device to enable
+ */
+static inline int backlight_enable(struct backlight_device *bd)
+{
+   if (!bd)
+   return 0;
+
+   bd->props.power = FB_BLANK_UNBLANK;
+   bd->props.fb_blank = FB_BLANK_UNBLANK;
+   bd->props.state &= ~BL_CORE_FBBLANK;
+
+   return backlight_update_status(bd);
+}
+
+/**
+ * backlight_disable - Disable backlight
+ * @bd: the backlight device to disable
+ */
+static inline int backlight_disable(struct backlight_device *bd)
+{
+   if (!bd)
+   return 0;
+
+   bd->props.power = FB_BLANK_POWERDOWN;
+   bd->props.fb_blank = FB_BLANK_POWERDOWN;
+   bd->props.state |= BL_CORE_FBBLANK;
+
+   return backlight_update_status(bd);
+}
+
  extern struct backlight_device *backlight_device_register(const char *name,
struct device *dev, void *devdata, const struct backlight_ops *ops,
const struct backlight_properties *props);
--
2.11.0

Re: [PATCH v18 01/10] video: backlight: Add helpers to enable and disable backlight

[Noralf Trønnes]

Den 23.01.2018 11.00, skrev Daniel Thompson:

On Mon, Jan 22, 2018 at 02:49:28PM +, Meghana Madhyastha wrote:

Add helper functions backlight_enable and backlight_disable to
enable/disable a backlight device. These helper functions can
then be used by different drm and tinydrm drivers to avoid
repetition of code and also to enforce a uniform and consistent
way to enable/disable a backlight device.

Signed-off-by: Meghana Madhyastha 
---
Acked-by: Daniel Thompson 
Reviewed-by: Noralf Trønnes 
Reviewed-by: Sean Paul

Just in case there's another spin...  the additional tags are normally
presented above the --- (otherwise they are unlikely to be copied into
the git history when they are applied).


Indeed, they should be part of the commit message.

Noralf.



Daniel.



  include/linux/backlight.h | 32 
  1 file changed, 32 insertions(+)

diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index af7003548..ace825e2c 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -130,6 +130,38 @@ static inline int backlight_update_status(struct 
backlight_device *bd)
return ret;
  }
  
+/**

+ * backlight_enable - Enable backlight
+ * @bd: the backlight device to enable
+ */
+static inline int backlight_enable(struct backlight_device *bd)
+{
+   if (!bd)
+   return 0;
+
+   bd->props.power = FB_BLANK_UNBLANK;
+   bd->props.fb_blank = FB_BLANK_UNBLANK;
+   bd->props.state &= ~BL_CORE_FBBLANK;
+
+   return backlight_update_status(bd);
+}
+
+/**
+ * backlight_disable - Disable backlight
+ * @bd: the backlight device to disable
+ */
+static inline int backlight_disable(struct backlight_device *bd)
+{
+   if (!bd)
+   return 0;
+
+   bd->props.power = FB_BLANK_POWERDOWN;
+   bd->props.fb_blank = FB_BLANK_POWERDOWN;
+   bd->props.state |= BL_CORE_FBBLANK;
+
+   return backlight_update_status(bd);
+}
+
  extern struct backlight_device *backlight_device_register(const char *name,
struct device *dev, void *devdata, const struct backlight_ops *ops,
const struct backlight_properties *props);
--
2.11.0

Re: [PATCH v18 10/10] drm/omapdrm: Use of_find_backlight helper

[Noralf Trønnes]

Den 22.01.2018 15.56, skrev Meghana Madhyastha:

Replace of_find_backlight_by_node and of the code around it
with of_find_backlight helper to avoid repetition of code.

Signed-off-by: Meghana Madhyastha 
---
Changes in v18:
-Fixed warnings resulting from passing device_node* to of_find_backlight.
  Fixed it by passing struct device* to of_find_backlight

  drivers/gpu/drm/omapdrm/displays/panel-dpi.c | 33 ++--
  1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c 
b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
index ac9596251..93b7a176d 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
@@ -156,14 +156,14 @@ static struct omap_dss_driver panel_dpi_ops = {
  static int panel_dpi_probe_of(struct platform_device *pdev)
  {
struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+   struct device *dev = >dev;
struct device_node *node = pdev->dev.of_node;
-   struct device_node *bl_node;
struct omap_dss_device *in;
int r;
struct display_timing timing;
struct gpio_desc *gpio;
  
-	gpio = devm_gpiod_get_optional(>dev, "enable", GPIOD_OUT_LOW);

+   gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);


Please don't make unrelated changes like this. It clutters the patch.
You can just as well use >dev when getting the backlight also.


if (IS_ERR(gpio))
return PTR_ERR(gpio);
  
@@ -175,47 +175,36 @@ static int panel_dpi_probe_of(struct platform_device *pdev)

 * timing and order relative to the enable gpio. So for now it's just
 * ensured that the reset line isn't active.
 */
-   gpio = devm_gpiod_get_optional(>dev, "reset", GPIOD_OUT_LOW);
+   gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(gpio))
return PTR_ERR(gpio);
  
-	ddata->vcc_supply = devm_regulator_get(>dev, "vcc");

+   ddata->vcc_supply = devm_regulator_get(dev, "vcc");
if (IS_ERR(ddata->vcc_supply))
return PTR_ERR(ddata->vcc_supply);
  
-	bl_node = of_parse_phandle(node, "backlight", 0);

-   if (bl_node) {
-   ddata->backlight = of_find_backlight_by_node(bl_node);
-   of_node_put(bl_node);
+   ddata->backlight = of_find_backlight(dev);


Any reason you don't use the devm_ version here?
You do remove error_free_backlight...

With the devm_ version remember to drop the put_device in 
panel_dpi_remove().


Noralf.

  
-		if (!ddata->backlight)

-   return -EPROBE_DEFER;
-   }
+   if (IS_ERR(ddata->backlight))
+   return PTR_ERR(ddata->backlight);
  
  	r = of_get_display_timing(node, "panel-timing", );

if (r) {
-   dev_err(>dev, "failed to get video timing\n");
-   goto error_free_backlight;
+   dev_err(dev, "failed to get video timing\n");
+   return r;
}
  
  	videomode_from_timing(, >vm);
  
  	in = omapdss_of_find_source_for_first_ep(node);

if (IS_ERR(in)) {
-   dev_err(>dev, "failed to find video source\n");
-   r = PTR_ERR(in);
-   goto error_free_backlight;
+   dev_err(dev, "failed to find video source\n");
+   return PTR_ERR(in);
}
  
  	ddata->in = in;
  
  	return 0;

-
-error_free_backlight:
-   if (ddata->backlight)
-   put_device(>backlight->dev);
-
-   return r;
  }
  
  static int panel_dpi_probe(struct platform_device *pdev)

Re: [PATCH v18 10/10] drm/omapdrm: Use of_find_backlight helper

[Noralf Trønnes]

Den 22.01.2018 15.56, skrev Meghana Madhyastha:

Replace of_find_backlight_by_node and of the code around it
with of_find_backlight helper to avoid repetition of code.

Signed-off-by: Meghana Madhyastha 
---
Changes in v18:
-Fixed warnings resulting from passing device_node* to of_find_backlight.
  Fixed it by passing struct device* to of_find_backlight

  drivers/gpu/drm/omapdrm/displays/panel-dpi.c | 33 ++--
  1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c 
b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
index ac9596251..93b7a176d 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
@@ -156,14 +156,14 @@ static struct omap_dss_driver panel_dpi_ops = {
  static int panel_dpi_probe_of(struct platform_device *pdev)
  {
struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+   struct device *dev = >dev;
struct device_node *node = pdev->dev.of_node;
-   struct device_node *bl_node;
struct omap_dss_device *in;
int r;
struct display_timing timing;
struct gpio_desc *gpio;
  
-	gpio = devm_gpiod_get_optional(>dev, "enable", GPIOD_OUT_LOW);

+   gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);


Please don't make unrelated changes like this. It clutters the patch.
You can just as well use >dev when getting the backlight also.


if (IS_ERR(gpio))
return PTR_ERR(gpio);
  
@@ -175,47 +175,36 @@ static int panel_dpi_probe_of(struct platform_device *pdev)

 * timing and order relative to the enable gpio. So for now it's just
 * ensured that the reset line isn't active.
 */
-   gpio = devm_gpiod_get_optional(>dev, "reset", GPIOD_OUT_LOW);
+   gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(gpio))
return PTR_ERR(gpio);
  
-	ddata->vcc_supply = devm_regulator_get(>dev, "vcc");

+   ddata->vcc_supply = devm_regulator_get(dev, "vcc");
if (IS_ERR(ddata->vcc_supply))
return PTR_ERR(ddata->vcc_supply);
  
-	bl_node = of_parse_phandle(node, "backlight", 0);

-   if (bl_node) {
-   ddata->backlight = of_find_backlight_by_node(bl_node);
-   of_node_put(bl_node);
+   ddata->backlight = of_find_backlight(dev);


Any reason you don't use the devm_ version here?
You do remove error_free_backlight...

With the devm_ version remember to drop the put_device in 
panel_dpi_remove().


Noralf.

  
-		if (!ddata->backlight)

-   return -EPROBE_DEFER;
-   }
+   if (IS_ERR(ddata->backlight))
+   return PTR_ERR(ddata->backlight);
  
  	r = of_get_display_timing(node, "panel-timing", );

if (r) {
-   dev_err(>dev, "failed to get video timing\n");
-   goto error_free_backlight;
+   dev_err(dev, "failed to get video timing\n");
+   return r;
}
  
  	videomode_from_timing(, >vm);
  
  	in = omapdss_of_find_source_for_first_ep(node);

if (IS_ERR(in)) {
-   dev_err(>dev, "failed to find video source\n");
-   r = PTR_ERR(in);
-   goto error_free_backlight;
+   dev_err(dev, "failed to find video source\n");
+   return PTR_ERR(in);
}
  
  	ddata->in = in;
  
  	return 0;

-
-error_free_backlight:
-   if (ddata->backlight)
-   put_device(>backlight->dev);
-
-   return r;
  }
  
  static int panel_dpi_probe(struct platform_device *pdev)

Re: [PATCH v18 09/10] drm/panel: Use of_find_backlight helper

[Noralf Trønnes]

Den 22.01.2018 15.55, skrev Meghana Madhyastha:

Replace of_find_backlight_by_node and of the code around it
with of_find_backlight helper to avoid repetition of code.

Signed-off-by: Meghana Madhyastha <meghana.madhyas...@gmail.com>
---


Reviewed-by: Noralf Trønnes<nor...@tronnes.org>



Changes in v18:
-Fixed warnings resulting from passing device_node* to of_find_backlight.
  Fixed it by passing struct device* to of_find_backlight

  drivers/gpu/drm/panel/panel-innolux-p079zca.c   | 24 -
  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 28 +
  drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 27 
  3 files changed, 13 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c 
b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
index 4c1b29eec..57df39b5c 100644
--- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
+++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
@@ -215,7 +215,6 @@ MODULE_DEVICE_TABLE(of, innolux_of_match);
  static int innolux_panel_add(struct innolux_panel *innolux)
  {
struct device *dev = >link->dev;
-   struct device_node *np;
int err;
  
  	innolux->supply = devm_regulator_get(dev, "power");

@@ -230,37 +229,22 @@ static int innolux_panel_add(struct innolux_panel 
*innolux)
innolux->enable_gpio = NULL;
}
  
-	np = of_parse_phandle(dev->of_node, "backlight", 0);

-   if (np) {
-   innolux->backlight = of_find_backlight_by_node(np);
-   of_node_put(np);
+   innolux->backlight = devm_of_find_backlight(dev);
  
-		if (!innolux->backlight)

-   return -EPROBE_DEFER;
-   }
+   if (IS_ERR(innolux->backlight))
+   return PTR_ERR(innolux->backlight);
  
  	drm_panel_init(>base);

innolux->base.funcs = _panel_funcs;
innolux->base.dev = >link->dev;
  
-	err = drm_panel_add(>base);

-   if (err < 0)
-   goto put_backlight;
-
-   return 0;
-
-put_backlight:
-   put_device(>backlight->dev);
-
-   return err;
+   return drm_panel_add(>base);
  }
  
  static void innolux_panel_del(struct innolux_panel *innolux)

  {
if (innolux->base.dev)
drm_panel_remove(>base);
-
-   put_device(>backlight->dev);
  }
  
  static int innolux_panel_probe(struct mipi_dsi_device *dsi)

diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c 
b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index 072c0fc79..6bf8730f1 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -318,8 +318,7 @@ MODULE_DEVICE_TABLE(of, sharp_of_match);
  
  static int sharp_panel_add(struct sharp_panel *sharp)

  {
-   struct device_node *np;
-   int err;
+   struct device *dev = >link1->dev;
  
  	sharp->mode = _mode;
  
@@ -327,30 +326,16 @@ static int sharp_panel_add(struct sharp_panel *sharp)

if (IS_ERR(sharp->supply))
return PTR_ERR(sharp->supply);
  
-	np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0);

-   if (np) {
-   sharp->backlight = of_find_backlight_by_node(np);
-   of_node_put(np);
+   sharp->backlight = devm_of_find_backlight(dev);
  
-		if (!sharp->backlight)

-   return -EPROBE_DEFER;
-   }
+   if (IS_ERR(sharp->backlight))
+   return PTR_ERR(sharp->backlight);
  
  	drm_panel_init(>base);

sharp->base.funcs = _panel_funcs;
sharp->base.dev = >link1->dev;
  
-	err = drm_panel_add(>base);

-   if (err < 0)
-   goto put_backlight;
-
-   return 0;
-
-put_backlight:
-   if (sharp->backlight)
-   put_device(>backlight->dev);
-
-   return err;
+   return drm_panel_add(>base);
  }
  
  static void sharp_panel_del(struct sharp_panel *sharp)

@@ -358,9 +343,6 @@ static void sharp_panel_del(struct sharp_panel *sharp)
if (sharp->base.dev)
drm_panel_remove(>base);
  
-	if (sharp->backlight)

-   put_device(>backlight->dev);
-
if (sharp->link2)
put_device(>link2->dev);
  }
diff --git a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c 
b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
index 8a5137963..494aa9b16 100644
--- a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
@@ -253,8 +253,6 @@ static const struct drm_panel_funcs sharp_nt_panel_funcs = {
  static int sharp_nt_panel_add(struct sharp_nt_panel *sharp_nt)
  {
struct device *dev = _nt->dsi->dev;
-   struct device_node *np;
-   int ret;
  
  	sharp_nt->mode = _mode;
  
@@ -271,39 

Re: [PATCH v18 09/10] drm/panel: Use of_find_backlight helper

[Noralf Trønnes]

Den 22.01.2018 15.55, skrev Meghana Madhyastha:

Replace of_find_backlight_by_node and of the code around it
with of_find_backlight helper to avoid repetition of code.

Signed-off-by: Meghana Madhyastha 
---


Reviewed-by: Noralf Trønnes



Changes in v18:
-Fixed warnings resulting from passing device_node* to of_find_backlight.
  Fixed it by passing struct device* to of_find_backlight

  drivers/gpu/drm/panel/panel-innolux-p079zca.c   | 24 -
  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 28 +
  drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 27 
  3 files changed, 13 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c 
b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
index 4c1b29eec..57df39b5c 100644
--- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
+++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
@@ -215,7 +215,6 @@ MODULE_DEVICE_TABLE(of, innolux_of_match);
  static int innolux_panel_add(struct innolux_panel *innolux)
  {
struct device *dev = >link->dev;
-   struct device_node *np;
int err;
  
  	innolux->supply = devm_regulator_get(dev, "power");

@@ -230,37 +229,22 @@ static int innolux_panel_add(struct innolux_panel 
*innolux)
innolux->enable_gpio = NULL;
}
  
-	np = of_parse_phandle(dev->of_node, "backlight", 0);

-   if (np) {
-   innolux->backlight = of_find_backlight_by_node(np);
-   of_node_put(np);
+   innolux->backlight = devm_of_find_backlight(dev);
  
-		if (!innolux->backlight)

-   return -EPROBE_DEFER;
-   }
+   if (IS_ERR(innolux->backlight))
+   return PTR_ERR(innolux->backlight);
  
  	drm_panel_init(>base);

innolux->base.funcs = _panel_funcs;
innolux->base.dev = >link->dev;
  
-	err = drm_panel_add(>base);

-   if (err < 0)
-   goto put_backlight;
-
-   return 0;
-
-put_backlight:
-   put_device(>backlight->dev);
-
-   return err;
+   return drm_panel_add(>base);
  }
  
  static void innolux_panel_del(struct innolux_panel *innolux)

  {
if (innolux->base.dev)
drm_panel_remove(>base);
-
-   put_device(>backlight->dev);
  }
  
  static int innolux_panel_probe(struct mipi_dsi_device *dsi)

diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c 
b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index 072c0fc79..6bf8730f1 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -318,8 +318,7 @@ MODULE_DEVICE_TABLE(of, sharp_of_match);
  
  static int sharp_panel_add(struct sharp_panel *sharp)

  {
-   struct device_node *np;
-   int err;
+   struct device *dev = >link1->dev;
  
  	sharp->mode = _mode;
  
@@ -327,30 +326,16 @@ static int sharp_panel_add(struct sharp_panel *sharp)

if (IS_ERR(sharp->supply))
return PTR_ERR(sharp->supply);
  
-	np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0);

-   if (np) {
-   sharp->backlight = of_find_backlight_by_node(np);
-   of_node_put(np);
+   sharp->backlight = devm_of_find_backlight(dev);
  
-		if (!sharp->backlight)

-   return -EPROBE_DEFER;
-   }
+   if (IS_ERR(sharp->backlight))
+   return PTR_ERR(sharp->backlight);
  
  	drm_panel_init(>base);

sharp->base.funcs = _panel_funcs;
sharp->base.dev = >link1->dev;
  
-	err = drm_panel_add(>base);

-   if (err < 0)
-   goto put_backlight;
-
-   return 0;
-
-put_backlight:
-   if (sharp->backlight)
-   put_device(>backlight->dev);
-
-   return err;
+   return drm_panel_add(>base);
  }
  
  static void sharp_panel_del(struct sharp_panel *sharp)

@@ -358,9 +343,6 @@ static void sharp_panel_del(struct sharp_panel *sharp)
if (sharp->base.dev)
drm_panel_remove(>base);
  
-	if (sharp->backlight)

-   put_device(>backlight->dev);
-
if (sharp->link2)
put_device(>link2->dev);
  }
diff --git a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c 
b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
index 8a5137963..494aa9b16 100644
--- a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
@@ -253,8 +253,6 @@ static const struct drm_panel_funcs sharp_nt_panel_funcs = {
  static int sharp_nt_panel_add(struct sharp_nt_panel *sharp_nt)
  {
struct device *dev = _nt->dsi->dev;
-   struct device_node *np;
-   int ret;
  
  	sharp_nt->mode = _mode;
  
@@ -271,39 +269,22 @@ static int sharp_nt_panel_add(struct sharp_nt_panel *sharp_nt)

   

Re: [PATCH v18 08/10] drm/omapdrm: Use backlight_enable/disable helpers

[Noralf Trønnes]

Den 22.01.2018 15.54, skrev Meghana Madhyastha:

Use backlight_enable/disable helpers instead of changing
the property and calling backlight_update_status for cleaner
and simpler code and also to avoid repetitions.

Signed-off-by: Meghana Madhyastha <meghana.madhyas...@gmail.com>
---


Reviewed-by: Noralf Trønnes<nor...@tronnes.org>



  drivers/gpu/drm/omapdrm/displays/panel-dpi.c | 11 ++-
  1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c 
b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
index e065f7e10..ac9596251 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
@@ -87,11 +87,7 @@ static int panel_dpi_enable(struct omap_dss_device *dssdev)
}
  
  	gpiod_set_value_cansleep(ddata->enable_gpio, 1);

-
-   if (ddata->backlight) {
-   ddata->backlight->props.power = FB_BLANK_UNBLANK;
-   backlight_update_status(ddata->backlight);
-   }
+   backlight_enable(ddata->backlight);
  
  	dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
  
@@ -106,10 +102,7 @@ static void panel_dpi_disable(struct omap_dss_device *dssdev)

if (!omapdss_device_is_enabled(dssdev))
return;
  
-	if (ddata->backlight) {

-   ddata->backlight->props.power = FB_BLANK_POWERDOWN;
-   backlight_update_status(ddata->backlight);
-   }
+   backlight_disable(ddata->backlight);
  
  	gpiod_set_value_cansleep(ddata->enable_gpio, 0);

regulator_disable(ddata->vcc_supply);

Re: [PATCH v18 08/10] drm/omapdrm: Use backlight_enable/disable helpers

[Noralf Trønnes]

Den 22.01.2018 15.54, skrev Meghana Madhyastha:

Use backlight_enable/disable helpers instead of changing
the property and calling backlight_update_status for cleaner
and simpler code and also to avoid repetitions.

Signed-off-by: Meghana Madhyastha 
---


Reviewed-by: Noralf Trønnes



  drivers/gpu/drm/omapdrm/displays/panel-dpi.c | 11 ++-
  1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c 
b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
index e065f7e10..ac9596251 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dpi.c
@@ -87,11 +87,7 @@ static int panel_dpi_enable(struct omap_dss_device *dssdev)
}
  
  	gpiod_set_value_cansleep(ddata->enable_gpio, 1);

-
-   if (ddata->backlight) {
-   ddata->backlight->props.power = FB_BLANK_UNBLANK;
-   backlight_update_status(ddata->backlight);
-   }
+   backlight_enable(ddata->backlight);
  
  	dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
  
@@ -106,10 +102,7 @@ static void panel_dpi_disable(struct omap_dss_device *dssdev)

if (!omapdss_device_is_enabled(dssdev))
return;
  
-	if (ddata->backlight) {

-   ddata->backlight->props.power = FB_BLANK_POWERDOWN;
-   backlight_update_status(ddata->backlight);
-   }
+   backlight_disable(ddata->backlight);
  
  	gpiod_set_value_cansleep(ddata->enable_gpio, 0);

regulator_disable(ddata->vcc_supply);

Re: [PATCH v18 07/10] drm/panel: Use backlight_enable/disable helpers

[Noralf Trønnes]

Den 22.01.2018 15.54, skrev Meghana Madhyastha:

Use backlight_enable/disable helpers instead of changing
the property and calling backlight_update_status for cleaner
and simpler code and also to avoid repetitions.

Signed-off-by: Meghana Madhyastha <meghana.madhyas...@gmail.com>
---


Reviewed-by: Noralf Trønnes <nor...@tronnes.org>


  drivers/gpu/drm/panel/panel-innolux-p079zca.c   |  6 ++
  drivers/gpu/drm/panel/panel-jdi-lt070me05000.c  |  6 ++
  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 10 ++
  drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 10 ++
  4 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c 
b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
index 6ba93449f..4c1b29eec 100644
--- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
+++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
@@ -45,8 +45,7 @@ static int innolux_panel_disable(struct drm_panel *panel)
if (!innolux->enabled)
return 0;
  
-	innolux->backlight->props.power = FB_BLANK_POWERDOWN;

-   backlight_update_status(innolux->backlight);
+   backlight_disable(innolux->backlight);
  
  	err = mipi_dsi_dcs_set_display_off(innolux->link);

if (err < 0)
@@ -151,8 +150,7 @@ static int innolux_panel_enable(struct drm_panel *panel)
if (innolux->enabled)
return 0;
  
-	innolux->backlight->props.power = FB_BLANK_UNBLANK;

-   ret = backlight_update_status(innolux->backlight);
+   ret = backlight_enable(innolux->backlight);
if (ret) {
DRM_DEV_ERROR(panel->drm->dev,
  "Failed to enable backlight %d\n", ret);
diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c 
b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
index 5b2340ef7..0a94ab79a 100644
--- a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
+++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
@@ -192,8 +192,7 @@ static int jdi_panel_disable(struct drm_panel *panel)
if (!jdi->enabled)
return 0;
  
-	jdi->backlight->props.power = FB_BLANK_POWERDOWN;

-   backlight_update_status(jdi->backlight);
+   backlight_disable(jdi->backlight);
  
  	jdi->enabled = false;
  
@@ -289,8 +288,7 @@ static int jdi_panel_enable(struct drm_panel *panel)

if (jdi->enabled)
return 0;
  
-	jdi->backlight->props.power = FB_BLANK_UNBLANK;

-   backlight_update_status(jdi->backlight);
+   backlight_enable(jdi->backlight);
  
  	jdi->enabled = true;
  
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c

index 3cce3ca19..072c0fc79 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -96,10 +96,7 @@ static int sharp_panel_disable(struct drm_panel *panel)
if (!sharp->enabled)
return 0;
  
-	if (sharp->backlight) {

-   sharp->backlight->props.power = FB_BLANK_POWERDOWN;
-   backlight_update_status(sharp->backlight);
-   }
+   backlight_disable(sharp->backlight);
  
  	sharp->enabled = false;
  
@@ -263,10 +260,7 @@ static int sharp_panel_enable(struct drm_panel *panel)

if (sharp->enabled)
return 0;
  
-	if (sharp->backlight) {

-   sharp->backlight->props.power = FB_BLANK_UNBLANK;
-   backlight_update_status(sharp->backlight);
-   }
+   backlight_enable(sharp->backlight);
  
  	sharp->enabled = true;
  
diff --git a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c

index 3aeb0bda4..8a5137963 100644
--- a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
@@ -117,10 +117,7 @@ static int sharp_nt_panel_disable(struct drm_panel *panel)
if (!sharp_nt->enabled)
return 0;
  
-	if (sharp_nt->backlight) {

-   sharp_nt->backlight->props.power = FB_BLANK_POWERDOWN;
-   backlight_update_status(sharp_nt->backlight);
-   }
+   backlight_disable(sharp_nt->backlight);
  
  	sharp_nt->enabled = false;
  
@@ -203,10 +200,7 @@ static int sharp_nt_panel_enable(struct drm_panel *panel)

if (sharp_nt->enabled)
return 0;
  
-	if (sharp_nt->backlight) {

-   sharp_nt->backlight->props.power = FB_BLANK_UNBLANK;
-   backlight_update_status(sharp_nt->backlight);
-   }
+   backlight_enable(sharp_nt->backlight);
  
  	sharp_nt->enabled = true;
  

Re: [PATCH v18 07/10] drm/panel: Use backlight_enable/disable helpers

[Noralf Trønnes]

Den 22.01.2018 15.54, skrev Meghana Madhyastha:

Use backlight_enable/disable helpers instead of changing
the property and calling backlight_update_status for cleaner
and simpler code and also to avoid repetitions.

Signed-off-by: Meghana Madhyastha 
---


Reviewed-by: Noralf Trønnes 


  drivers/gpu/drm/panel/panel-innolux-p079zca.c   |  6 ++
  drivers/gpu/drm/panel/panel-jdi-lt070me05000.c  |  6 ++
  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 10 ++
  drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 10 ++
  4 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c 
b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
index 6ba93449f..4c1b29eec 100644
--- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
+++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
@@ -45,8 +45,7 @@ static int innolux_panel_disable(struct drm_panel *panel)
if (!innolux->enabled)
return 0;
  
-	innolux->backlight->props.power = FB_BLANK_POWERDOWN;

-   backlight_update_status(innolux->backlight);
+   backlight_disable(innolux->backlight);
  
  	err = mipi_dsi_dcs_set_display_off(innolux->link);

if (err < 0)
@@ -151,8 +150,7 @@ static int innolux_panel_enable(struct drm_panel *panel)
if (innolux->enabled)
return 0;
  
-	innolux->backlight->props.power = FB_BLANK_UNBLANK;

-   ret = backlight_update_status(innolux->backlight);
+   ret = backlight_enable(innolux->backlight);
if (ret) {
DRM_DEV_ERROR(panel->drm->dev,
  "Failed to enable backlight %d\n", ret);
diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c 
b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
index 5b2340ef7..0a94ab79a 100644
--- a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
+++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
@@ -192,8 +192,7 @@ static int jdi_panel_disable(struct drm_panel *panel)
if (!jdi->enabled)
return 0;
  
-	jdi->backlight->props.power = FB_BLANK_POWERDOWN;

-   backlight_update_status(jdi->backlight);
+   backlight_disable(jdi->backlight);
  
  	jdi->enabled = false;
  
@@ -289,8 +288,7 @@ static int jdi_panel_enable(struct drm_panel *panel)

if (jdi->enabled)
return 0;
  
-	jdi->backlight->props.power = FB_BLANK_UNBLANK;

-   backlight_update_status(jdi->backlight);
+   backlight_enable(jdi->backlight);
  
  	jdi->enabled = true;
  
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c

index 3cce3ca19..072c0fc79 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -96,10 +96,7 @@ static int sharp_panel_disable(struct drm_panel *panel)
if (!sharp->enabled)
return 0;
  
-	if (sharp->backlight) {

-   sharp->backlight->props.power = FB_BLANK_POWERDOWN;
-   backlight_update_status(sharp->backlight);
-   }
+   backlight_disable(sharp->backlight);
  
  	sharp->enabled = false;
  
@@ -263,10 +260,7 @@ static int sharp_panel_enable(struct drm_panel *panel)

if (sharp->enabled)
return 0;
  
-	if (sharp->backlight) {

-   sharp->backlight->props.power = FB_BLANK_UNBLANK;
-   backlight_update_status(sharp->backlight);
-   }
+   backlight_enable(sharp->backlight);
  
  	sharp->enabled = true;
  
diff --git a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c

index 3aeb0bda4..8a5137963 100644
--- a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
@@ -117,10 +117,7 @@ static int sharp_nt_panel_disable(struct drm_panel *panel)
if (!sharp_nt->enabled)
return 0;
  
-	if (sharp_nt->backlight) {

-   sharp_nt->backlight->props.power = FB_BLANK_POWERDOWN;
-   backlight_update_status(sharp_nt->backlight);
-   }
+   backlight_disable(sharp_nt->backlight);
  
  	sharp_nt->enabled = false;
  
@@ -203,10 +200,7 @@ static int sharp_nt_panel_enable(struct drm_panel *panel)

if (sharp_nt->enabled)
return 0;
  
-	if (sharp_nt->backlight) {

-   sharp_nt->backlight->props.power = FB_BLANK_UNBLANK;
-   backlight_update_status(sharp_nt->backlight);
-   }
+   backlight_enable(sharp_nt->backlight);
  
  	sharp_nt->enabled = true;
  

Re: [PATCH v17 03/10] video: backlight: Add of_find_backlight helper in backlight.c

[Noralf Trønnes]

Den 19.01.2018 11.42, skrev Meghana Madhyastha:

Add of_find_backlight, a helper function which is a generic version
of tinydrm_of_find_backlight that can be used by other drivers to avoid
repetition of code and simplify things.

Signed-off-by: Meghana Madhyastha <meghana.madhyas...@gmail.com>
---


Reviewed-by: Noralf Trønnes <nor...@tronnes.org>


changes in v17:
-rebase with drm-misc-next
-convert st7735r callers from tinydrm specific helpers
  to new generic backlight helpers
-remove select BACKLIGHT_LCD_SUPPORT
  and select BACKLIGHT_CLASS_DEVICE from
  tinydrm/Kconfig

  drivers/video/backlight/backlight.c | 43 +
  include/linux/backlight.h   | 19 
  2 files changed, 62 insertions(+)

diff --git a/drivers/video/backlight/backlight.c 
b/drivers/video/backlight/backlight.c
index 8049e7656..553bf5c48 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -580,6 +580,49 @@ struct backlight_device *of_find_backlight_by_node(struct 
device_node *node)
  EXPORT_SYMBOL(of_find_backlight_by_node);
  #endif
  
+/**

+ * of_find_backlight - Get backlight device
+ * @dev: Device
+ *
+ * This function looks for a property named 'backlight' on the DT node
+ * connected to @dev and looks up the backlight device.
+ *
+ * Call backlight_put() to drop the reference on the backlight device.
+ *
+ * Returns:
+ * A pointer to the backlight device if found.
+ * Error pointer -EPROBE_DEFER if the DT property is set, but no backlight
+ * device is found.
+ * NULL if there's no backlight property.
+ */
+struct backlight_device *of_find_backlight(struct device *dev)
+{
+   struct backlight_device *bd = NULL;
+   struct device_node *np;
+
+   if (!dev)
+   return NULL;
+
+   if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
+   np = of_parse_phandle(dev->of_node, "backlight", 0);
+   if (np) {
+   bd = of_find_backlight_by_node(np);
+   of_node_put(np);
+   if (!bd)
+   return ERR_PTR(-EPROBE_DEFER);
+   /*
+* Note: gpio_backlight uses brightness as
+* power state during probe
+*/
+   if (!bd->props.brightness)
+   bd->props.brightness = bd->props.max_brightness;
+   }
+   }
+
+   return bd;
+}
+EXPORT_SYMBOL(of_find_backlight);
+
  static void __exit backlight_class_exit(void)
  {
class_destroy(backlight_class);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index ace825e2c..ddc9bade4 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -162,6 +162,16 @@ static inline int backlight_disable(struct 
backlight_device *bd)
return backlight_update_status(bd);
  }
  
+/**

+ * backlight_put - Drop backlight reference
+ * @bd: the backlight device to put
+ */
+static inline void backlight_put(struct backlight_device *bd)
+{
+   if (bd)
+   put_device(>dev);
+}
+
  extern struct backlight_device *backlight_device_register(const char *name,
struct device *dev, void *devdata, const struct backlight_ops *ops,
const struct backlight_properties *props);
@@ -205,4 +215,13 @@ of_find_backlight_by_node(struct device_node *node)
  }
  #endif
  
+#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

+struct backlight_device *of_find_backlight(struct device *dev);
+#else
+static inline struct backlight_device *of_find_backlight(struct device *dev)
+{
+   return NULL;
+}
+#endif
+
  #endif

Re: [PATCH v17 03/10] video: backlight: Add of_find_backlight helper in backlight.c

[Noralf Trønnes]

Den 19.01.2018 11.42, skrev Meghana Madhyastha:

Add of_find_backlight, a helper function which is a generic version
of tinydrm_of_find_backlight that can be used by other drivers to avoid
repetition of code and simplify things.

Signed-off-by: Meghana Madhyastha 
---


Reviewed-by: Noralf Trønnes 


changes in v17:
-rebase with drm-misc-next
-convert st7735r callers from tinydrm specific helpers
  to new generic backlight helpers
-remove select BACKLIGHT_LCD_SUPPORT
  and select BACKLIGHT_CLASS_DEVICE from
  tinydrm/Kconfig

  drivers/video/backlight/backlight.c | 43 +
  include/linux/backlight.h   | 19 
  2 files changed, 62 insertions(+)

diff --git a/drivers/video/backlight/backlight.c 
b/drivers/video/backlight/backlight.c
index 8049e7656..553bf5c48 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -580,6 +580,49 @@ struct backlight_device *of_find_backlight_by_node(struct 
device_node *node)
  EXPORT_SYMBOL(of_find_backlight_by_node);
  #endif
  
+/**

+ * of_find_backlight - Get backlight device
+ * @dev: Device
+ *
+ * This function looks for a property named 'backlight' on the DT node
+ * connected to @dev and looks up the backlight device.
+ *
+ * Call backlight_put() to drop the reference on the backlight device.
+ *
+ * Returns:
+ * A pointer to the backlight device if found.
+ * Error pointer -EPROBE_DEFER if the DT property is set, but no backlight
+ * device is found.
+ * NULL if there's no backlight property.
+ */
+struct backlight_device *of_find_backlight(struct device *dev)
+{
+   struct backlight_device *bd = NULL;
+   struct device_node *np;
+
+   if (!dev)
+   return NULL;
+
+   if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
+   np = of_parse_phandle(dev->of_node, "backlight", 0);
+   if (np) {
+   bd = of_find_backlight_by_node(np);
+   of_node_put(np);
+   if (!bd)
+   return ERR_PTR(-EPROBE_DEFER);
+   /*
+* Note: gpio_backlight uses brightness as
+* power state during probe
+*/
+   if (!bd->props.brightness)
+   bd->props.brightness = bd->props.max_brightness;
+   }
+   }
+
+   return bd;
+}
+EXPORT_SYMBOL(of_find_backlight);
+
  static void __exit backlight_class_exit(void)
  {
class_destroy(backlight_class);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index ace825e2c..ddc9bade4 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -162,6 +162,16 @@ static inline int backlight_disable(struct 
backlight_device *bd)
return backlight_update_status(bd);
  }
  
+/**

+ * backlight_put - Drop backlight reference
+ * @bd: the backlight device to put
+ */
+static inline void backlight_put(struct backlight_device *bd)
+{
+   if (bd)
+   put_device(>dev);
+}
+
  extern struct backlight_device *backlight_device_register(const char *name,
struct device *dev, void *devdata, const struct backlight_ops *ops,
const struct backlight_properties *props);
@@ -205,4 +215,13 @@ of_find_backlight_by_node(struct device_node *node)
  }
  #endif
  
+#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

+struct backlight_device *of_find_backlight(struct device *dev);
+#else
+static inline struct backlight_device *of_find_backlight(struct device *dev)
+{
+   return NULL;
+}
+#endif
+
  #endif

Re: [PATCH v17 06/10] drm/tinydrm: Call devres version of of_find_backlight

[Noralf Trønnes]

Den 19.01.2018 11.46, skrev Meghana Madhyastha:

Call devm_of_find_backlight (the devres version) instead of
of_find_backlight.

Signed-off-by: Meghana Madhyastha <meghana.madhyas...@gmail.com>
---


I already put my r-b on this one in the previous version, but now also:

Tested-by: Noralf Trønnes <nor...@tronnes.org>

I did also test with the backlight subsystem disabled.


changes in v17:
-convert st7735r callers from tinydrm specific helpers
  to new generic backlight helpers

  drivers/gpu/drm/tinydrm/mi0283qt.c | 2 +-
  drivers/gpu/drm/tinydrm/st7735r.c  | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c 
b/drivers/gpu/drm/tinydrm/mi0283qt.c
index a8aafce36..d8ed6e6f8 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -196,7 +196,7 @@ static int mi0283qt_probe(struct spi_device *spi)
if (IS_ERR(mipi->regulator))
return PTR_ERR(mipi->regulator);
  
-	mipi->backlight = of_find_backlight(dev);

+   mipi->backlight = devm_of_find_backlight(dev);
if (IS_ERR(mipi->backlight))
return PTR_ERR(mipi->backlight);
  
diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c

index 2e6b7b8ec..67d197ecf 100644
--- a/drivers/gpu/drm/tinydrm/st7735r.c
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -164,7 +164,7 @@ static int st7735r_probe(struct spi_device *spi)
return PTR_ERR(dc);
}
  
-	mipi->backlight = of_find_backlight(dev);

+   mipi->backlight = devm_of_find_backlight(dev);
if (IS_ERR(mipi->backlight))
return PTR_ERR(mipi->backlight);
  

Re: [PATCH v17 06/10] drm/tinydrm: Call devres version of of_find_backlight

[Noralf Trønnes]

Den 19.01.2018 11.46, skrev Meghana Madhyastha:

Call devm_of_find_backlight (the devres version) instead of
of_find_backlight.

Signed-off-by: Meghana Madhyastha 
---


I already put my r-b on this one in the previous version, but now also:

Tested-by: Noralf Trønnes 

I did also test with the backlight subsystem disabled.


changes in v17:
-convert st7735r callers from tinydrm specific helpers
  to new generic backlight helpers

  drivers/gpu/drm/tinydrm/mi0283qt.c | 2 +-
  drivers/gpu/drm/tinydrm/st7735r.c  | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c 
b/drivers/gpu/drm/tinydrm/mi0283qt.c
index a8aafce36..d8ed6e6f8 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -196,7 +196,7 @@ static int mi0283qt_probe(struct spi_device *spi)
if (IS_ERR(mipi->regulator))
return PTR_ERR(mipi->regulator);
  
-	mipi->backlight = of_find_backlight(dev);

+   mipi->backlight = devm_of_find_backlight(dev);
if (IS_ERR(mipi->backlight))
return PTR_ERR(mipi->backlight);
  
diff --git a/drivers/gpu/drm/tinydrm/st7735r.c b/drivers/gpu/drm/tinydrm/st7735r.c

index 2e6b7b8ec..67d197ecf 100644
--- a/drivers/gpu/drm/tinydrm/st7735r.c
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -164,7 +164,7 @@ static int st7735r_probe(struct spi_device *spi)
return PTR_ERR(dc);
}
  
-	mipi->backlight = of_find_backlight(dev);

+   mipi->backlight = devm_of_find_backlight(dev);
if (IS_ERR(mipi->backlight))
return PTR_ERR(mipi->backlight);