Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays
On Mon, Jun 13, 2022 at 11:39:30AM +, Dominik Kierner wrote: > On 5/25/2022 21:46, Javier Martinez Canillas wrote: ... > > Thanks, I looked at the code briefly and think that there are things there > > that we > > could use. I.e the 3-wire SPI support that's in > > panel-solomon-ssd130x-spi-3wire.c. > > When writing my driver code, I wasn't even considering using regmaps, > like You did in Your I2C-Code. If that's applicable for 3-wire-SPI, > it would likely be the better, more generic option. Your SPI-code > reuses these parts to some extent. For that case, > ssd130x_spi_regmap_config.reg_bits would need be changed to 1, > as the "register address" has a length of 1 bit and we are sending > 9 bits over the line and not 16. > Since we still have 2 bytes of host memory, > it should still be compatible with the 4-wire write, right? > Or would 3-wire SPI require a second regmap? I believe the cleanest solution is to have different regmap configurations. -- With Best Regards, Andy Shevchenko
Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays
On Wed, May 25, 2022 at 09:46:24PM +0200, Javier Martinez Canillas wrote: > On 3/10/22 14:11, Dominik Kierner wrote: ... > > # DRM Mode Configuration via Device Tree > > > > In the old fbdev driver, the display modes are hard-coded, which means > > for every new display configuration, a new patch needs to be mainlined, > > which slows down official Kernel support and > > puts burden on the maintainers. > > Additionally, with the DRM-subsystem supporting height and length > > information, for scaling, this opens up a lot of new combinations. > > The SSD1306 for example, is available in multiple resolutions like > > 128x64 and 96x16 and comes in different sizes per resolution as well. > > Just to name a few: > > * 128x64 0.96" (22x11mm) > > * 128x64 1.3" (30x15mm) > > * 96x16 0.69" (18x3mm) > >> Instead of hard-coding, I would suggest something along the lines of > > of_get_drm_display_mode(). > > The displays won't need to support multiple modes at the same time, > > let alone support for switching between them, > > so the one-time invocation of this expensive function might be worth it. > > maybe a new and simpler function that could be named: > > of_get_drm_display_mode_simple() > > This makes sense to me as well. What about non-OF platforms? Please, do not spread OF-only interfaces, and use fwnode instead. > > Providing a mode could later prove useful for a conversion to > > drm_panel, if that is feasible. > > > > But for a function like this, I have to chicken out. -- With Best Regards, Andy Shevchenko
Re: [PATCH] drm: Remove drm_mode_config::fb_base
On Wed, Oct 19, 2022 at 09:32:26AM +0200, Thomas Zimmermann wrote: > Am 18.10.22 um 17:52 schrieb Zack Rusin: > IIRC PSB hardware is only available in 32-bit systems. Dunno about deep details, but IIUC the Intel Tangier and Intel Annioedale are 64-bit SoCs that have Imagination + Intel IPs, the latter from this GMA5xx/GMA6xx family. -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Thu, Feb 27, 2025 at 05:28:33PM +, Aditya Garg wrote: > > On 27 Feb 2025, at 10:24 PM, kernel test robot wrote: > > Hi Aditya, > > > > kernel test robot noticed the following build warnings: > > > > [auto build test WARNING on linus/master] > > [also build test WARNING on v6.14-rc4 next-20250227] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > A version 7 of this patch has already been submitted, not sure why kernel > test robot tested version 4 Because you are too fast to send a new versions. Whenever patch appears in ML it is seconds/minutes to trigger a bot build, which takes hours. I recommend spend more time on thinking and discussing, than issuing versions like from a machine gun. -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] drm/appletbdm: use %p4cl instead of %p4cc
On Wed, Mar 12, 2025 at 12:51:33PM +0100, Thomas Zimmermann wrote: > Am 12.03.25 um 10:06 schrieb Aditya Garg: ... > If you want to print out protocol-header tokens, why not use formatting > macros as we do with DRM mode? There's one for the format string [1] and one > for the argument. [2] It's not as fancy as the conversion code, but you'll > get something for your driver that is easily understandable. The benefit of the specific code formatters as in this patch at least in the stack usage and hence a lot of code generated again and again. I believe you can get rid of dozens of (kilo?) bytes in DRM on top of compensating this in the printf() implementation. This backs us to the discussion on how the best would be to implement custom printf() specifiers... -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/3] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc
On Thu, Feb 20, 2025 at 04:39:23PM +, Aditya Garg wrote:
> From: Hector Martin
>
> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
> it's useful to be able to print generic 4-character codes formatted as
> an integer. Extend it to add format specifiers for printing generic
> 32-bit FOURCCs with various endian semantics:
>
> %p4ch Host-endian
> %p4cl Little-endian
> %p4cb Big-endian
> %p4cr Reverse-endian
>
> The endianness determines how bytes are interpreted as a u32, and the
> FOURCC is then always printed MSByte-first (this is the opposite of
> V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
> allow printing LSByte-first FOURCCs stored in host endian order
> (other than the hex form being in character order, not the integer
> value).
...
> orig = get_unaligned(fourcc);
> - val = orig & ~BIT(31);
> + switch (fmt[2]) {
> + case 'h':
> + val = orig;
> + break;
> + case 'r':
> + orig = swab32(orig);
> + val = orig;
> + break;
> + case 'l':
> + orig = le32_to_cpu(orig);
> + val = orig;
> + break;
> + case 'b':
> + orig = be32_to_cpu(orig);
I do not see that orig is a union of different types. Have you run sparse?
It will definitely complain on this code.
> + val = orig;
> + break;
> + case 'c':
> + /* Pixel formats are printed LSB-first */
> + val = swab32(orig & ~BIT(31));
> + pixel_fmt = true;
> + break;
> + default:
> + return error_string(buf, end, "(%p4?)", spec);
> + }
...
> - *p++ = ' ';
> - strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> - p += strlen(p);
> + if (pixel_fmt) {
Technically we can avoid a boolean by checking fmt[2] again here, but I'm okay
with a temporary holder.
> + *p++ = ' ';
> + strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> + p += strlen(p);
> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 2/3] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc
On Fri, Feb 21, 2025 at 07:37:17PM +, Aditya Garg wrote: > > On 21 Feb 2025, at 8:59 PM, [email protected] wrote: > > On Fri, Feb 21, 2025 at 11:37:13AM +, Aditya Garg wrote: > > First of all, I do not see the cover letter. Is it only an issue on my side? > > These are literally 3 patches that are self explanatory. So what? Anybody will be puzzled with the simple question (and probably not the only one): Why are these in the series? Do they dependent or independent? What's the goal and how they should be routed? (You see, there are already 4). > Is this a hard and fast rule that a cover letter MUST be there? Cover letter SHOULD be there if there are more than one patch. Yes, there are exceptions, but this is the idea for the series. Use your common sense, if there is no documented thingy. ... > > Second, don't issue versions too fast, give at least a few days for the > > reviewers to have a chance on looking into it. > > Sure, I’ll take care of that. Btw, _this_ is very clearly documented. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/3] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc
On Fri, Feb 21, 2025 at 08:06:51PM +, Aditya Garg wrote: > > > Does this look good now? Made orig a union. > > Wait, it's messier. Maybe declare data type of val separately in each case? Yes, this sounds better. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/3] drm/format-helper: Add conversion from XRGB8888 to BGR888
On Fri, Feb 21, 2025 at 05:21:08PM +, Aditya Garg wrote: > > On 21 Feb 2025, at 9:21 PM, [email protected] wrote: > > On Fri, Feb 21, 2025 at 11:36:00AM +, Aditya Garg wrote: ... > >> + for (x = 0; x < pixels; x++) { > >> + pix = le32_to_cpu(sbuf32[x]); > >> + /* write red-green-blue to output in little endianness */ > >> + *dbuf8++ = (pix & 0x00ff) >> 16; > >> + *dbuf8++ = (pix & 0xff00) >> 8; > >> + *dbuf8++ = (pix & 0x00ff) >> 0; > > > > put_unaligned_be24() (1) > >> + } ... > >> + static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = { > >> + 3, > >> + }; > > > > One line? > > > > static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = { 3 }; > > Wrt all the above respective changes, the formatting has been done exactly > like what other emulation helps do in the upstream patch. > > I doubt Thomas would want these changes to be done, or would want these > changes to be done for the upstream emulation helpers as well. > > For reference: > https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_format_helper.c I'm not sure how this is applicable to (1) above. Otherwise it's fine if there is a documented style or due to consistency with the existing style. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 2/3] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc
On Fri, Feb 21, 2025 at 10:18:16PM +0200, [email protected] wrote: > On Fri, Feb 21, 2025 at 07:37:17PM +, Aditya Garg wrote: > > > On 21 Feb 2025, at 8:59 PM, [email protected] wrote: > > > On Fri, Feb 21, 2025 at 11:37:13AM +, Aditya Garg wrote: > > > > First of all, I do not see the cover letter. Is it only an issue on my > > > side? > > > > These are literally 3 patches that are self explanatory. > > So what? Anybody will be puzzled with the simple question (and probably not > the > only one): Why are these in the series? Do they dependent or independent? > What's > the goal and how they should be routed? (You see, there are already 4). > > > Is this a hard and fast rule that a cover letter MUST be there? > > Cover letter SHOULD be there if there are more than one patch. > Yes, there are exceptions, but this is the idea for the series. FWIW, two more points: 1) yes, it's a MUST for some subsystems (BPF has this even documented); 2) there are tools (`b4`) that rely on the cover letter (shazam feature or multiplying trailers if it/they was/were given against the cover letter). > Use your common sense, if there is no documented thingy. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 3/3] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Fri, Feb 21, 2025 at 07:13:06PM +, Aditya Garg wrote:
> > On Fri, Feb 21, 2025 at 11:37:57AM +, Aditya Garg wrote:
...
> >> +} __packed;
> >
> > Why __packed? Please explain and justify for all the data types that are
> > marked
> > with this attribute.
>
> Just following the original Windows driver here (#pragma pack) :
>
> https://github.com/imbushuo/DFRDisplayKm/blob/master/src/DFRDisplayKm/include/DFRHostIo.h
>
> IMO these structures are used for communication with the Touch Bar over USB.
> The hardware expects a very specific layout for the data it receives and
> sends. If the compiler were to insert padding for alignment, it would break
> the communication protocol because the fields would not be in the expected
> positions.
What padding, please? Why TCP UAPI headers do not have these attributes?
Think about it, and think about what actually __packed does and how it affects
(badly) the code generation. Otherwise it looks like a cargo cult.
> I tried removing __packed btw and driver no longer works.
So, you need to find a justification why. But definitely not due to padding in
many of them. They can go without __packed as they are naturally aligned.
...
> >> + if (response->msg == APPLETBDRM_MSG_SIGNAL_READINESS) {
> >> + if (!readiness_signal_received) {
> >> + readiness_signal_received = true;
> >> + goto retry;
> >> + }
> >> +
> >> + drm_err(drm, "Encountered unexpected readiness signal\n");
> >> + return -EIO;
> >> + }
> >> +
> >> + if (actual_size != size) {
> >> + drm_err(drm, "Actual size (%d) doesn't match expected size (%lu)\n",
> >> + actual_size, size);
> >> + return -EIO;
> >> + }
> >> +
> >> + if (response->msg != expected_response) {
> >> + drm_err(drm, "Unexpected response from device (expected %p4ch found
> >> %p4ch)\n",
> >> + &expected_response, &response->msg);
> >> + return -EIO;
> >
> > For three different cases the same error code, can it be adjusted more to
> > the
> > situation?
>
> All these are I/O errors, you got any suggestion?
Your email client mangled the code so badly that it's hard to read. But I would
suggest to use -EINTR in the first case, and -EBADMSG. But also you may consider
-EPROTO.
> >> + }
...
> >> + if (ret)
> >> + return ret;
> >
> >> + else if (!new_plane_state->visible)
> >
> > Why 'else'? It's redundant.
>
> I’ve just followed what other drm drivers are doing here:
>
> https://elixir.bootlin.com/linux/v6.13.3/source/drivers/gpu/drm/tiny/bochs.c#L436
> https://elixir.bootlin.com/linux/v6.13.3/source/drivers/gpu/drm/tiny/cirrus.c#L363
>
> And plenty more
A bad example is still a bad example. 'else' is simply redundant in this
case and add a noisy to the code.
> I won’t mind removing else. You want that?
Sure.
...
> >> + request_size = ALIGN(sizeof(struct appletbdrm_fb_request) +
> >> +frames_size +
> >> +sizeof(struct appletbdrm_fb_request_footer), 16);
> >
> > Missing header for ALIGN().
> >
> > But have you checked overflow.h for the possibility of using some helper
> > macros
> > from there? This is what should be usually done for k*alloc() in the kernel.
>
> I don’t really think we need a macro here.
Hmm... is frames_size known to be in a guaranteed range to make sure no
potential overflow happens?
> >> + appletbdrm_state->request = kzalloc(request_size, GFP_KERNEL);
> >> +
> >> + if (!appletbdrm_state->request)
> >> + return -ENOMEM;
...
> >> + request->msg_id = timestamp & 0xff;
> >
> > Why ' & 0xff'?
>
> https://github.com/imbushuo/DFRDisplayKm/blob/master/src/DFRDisplayKm/DfrDisplay.c#L147
This is not an answer.
Why do you need this here? Isn't the type of msg_id enough?
...
> >> + adev->mode = (struct drm_display_mode) {
> >
> > Why do you need a compound literal here? Perhaps you want to have that to be
> > done directly in DRM_MODE_INIT()?
>
> I really don’t find this as an issue. You want me to declare another
> structure, basically this?:
Nope, I'm asking if the DRM_MODE_INIT() is done in a way that it only can be
used for the static data. Seems like the case. Have you tried to convert
DRM_MODE_INIT() to be always a compound literal? Does it break things?
> struct drm_display_mode mode = {
> DRM_MODE_INIT(60, adev->height, adev->width,
> DRM_MODE_RES_MM(adev->height, 218),
> DRM_MODE_RES_MM(adev->width, 218))
> };
> adev->mode = mode;
>
> >> + DRM_MODE_INIT(60, adev->height, adev->width,
> >> + DRM_MODE_RES_MM(adev->height, 218),
> >> + DRM_MODE_RES_MM(adev->width, 218))
> >> + };
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 2/3] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc
On Fri, Feb 21, 2025 at 11:37:13AM +, Aditya Garg wrote: > From: Hector Martin First of all, I do not see the cover letter. Is it only an issue on my side? > %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but > it's useful to be able to print generic 4-character codes formatted as > an integer. Extend it to add format specifiers for printing generic > 32-bit FOURCCs with various endian semantics: > > %p4ch Host-endian > %p4cl Little-endian > %p4cb Big-endian > %p4cr Reverse-endian > > The endianness determines how bytes are interpreted as a u32, and the > FOURCC is then always printed MSByte-first (this is the opposite of > V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would > allow printing LSByte-first FOURCCs stored in host endian order > (other than the hex form being in character order, not the integer > value). Second, don't issue versions too fast, give at least a few days for the reviewers to have a chance on looking into it. Due to above this inherits the same issues as v2, please refer to my comments there. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 3/3] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Fri, Feb 21, 2025 at 11:37:57AM +, Aditya Garg wrote:
> From: Kerem Karabay
>
> The Touch Bars found on x86 Macs support two USB configurations: one
> where the device presents itself as a HID keyboard and can display
> predefined sets of keys, and one where the operating system has full
> control over what is displayed.
>
> This commit adds support for the display functionality of the second
> configuration. Functionality for the first configuration has been
> merged in the HID tree.
>
> Note that this driver has only been tested on T2 Macs, and only includes
> the USB device ID for these devices. Testing on T1 Macs would be
> appreciated.
>
> Credit goes to Ben (Bingxing) Wang on GitHub [1] for reverse engineering
> most of the protocol.
>
> [1]: https://github.com/imbushuo/DFRDisplayKm
Use Link tag for this.
> +config DRM_APPLETBDRM
> + tristate "DRM support for Apple Touch Bars"
> + depends on DRM && USB && MMU
I dunno if tiny is not only about SPI panels, would be nice to hear somebody
from DRM to confirm that USB ones are okay to have.
> + select DRM_GEM_SHMEM_HELPER
> + select DRM_KMS_HELPER
> + select HID_APPLETB_BL
> + select HID_MULTITOUCH
> + help
> + Say Y here if you want support for the display of Touch Bars on x86
> + MacBook Pros.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called appletbdrm.
...
> +/*
> + * Apple Touch Bar DRM Driver
> + *
> + * Copyright (c) 2023 Kerem Karabay
No changes in 2024/2025?
> + */
...
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Why? Don't you have a struct device available?
...
> +#include
> +#include
> +#include
This is way too little list of what you are actually using, please consider
IWYU principle in place.
...
> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << 24) |
> (str4[1] << 16) | (str4[2] << 8) | str4[3]))
Reinventing a wheel from get_unaligned_be32() AFAICS.
...
> +#define drm_to_adev(_drm)container_of(_drm, struct
> appletbdrm_device, drm)
+ container_of.h
...
> +struct appletbdrm_msg_request_header {
> + __le16 unk_00;
+ types.h
> + __le16 unk_02;
> + __le32 unk_04;
> + __le32 unk_08;
> + __le32 size;
> +} __packed;
Why __packed? Please explain and justify for all the data types that are marked
with this attribute.
...
> +static int appletbdrm_send_request(struct appletbdrm_device *adev,
> +struct appletbdrm_msg_request_header
> *request, size_t size)
> +{
> + struct usb_device *udev = adev_to_udev(adev);
> + struct drm_device *drm = &adev->drm;
> + int ret, actual_size;
> +
> + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, adev->out_ep),
> +request, size, &actual_size,
> APPLETBDRM_BULK_MSG_TIMEOUT);
> + if (ret) {
> + drm_err(drm, "Failed to send message (%d)\n", ret);
> + return ret;
> + }
> +
> + if (actual_size != size) {
> + drm_err(drm, "Actual size (%d) doesn't match expected size
> (%lu)\n",
> + actual_size, size);
> + return -EIO;
> + }
> +
> + return ret;
return 0;
Or you are expecting something else here?
> +}
...
> +static int appletbdrm_read_response(struct appletbdrm_device *adev,
> + struct appletbdrm_msg_response_header
> *response,
> + size_t size, u32 expected_response)
> +{
> + struct usb_device *udev = adev_to_udev(adev);
> + struct drm_device *drm = &adev->drm;
> + int ret, actual_size;
> + bool readiness_signal_received = false;
> +
> +retry:
> + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, adev->in_ep),
> +response, size, &actual_size,
> APPLETBDRM_BULK_MSG_TIMEOUT);
> + if (ret) {
> + drm_err(drm, "Failed to read response (%d)\n", ret);
> + return ret;
> + }
> +
> + /*
> + * The device responds to the first request sent in a particular
> + * timeframe after the USB device configuration is set with a readiness
> + * signal, in which case the response should be read again
> + */
> + if (response->msg == APPLETBDRM_MSG_SIGNAL_READINESS) {
> + if (!readiness_signal_received) {
> + readiness_signal_received = true;
> + goto retry;
> + }
> +
> + drm_err(drm, "Encountered unexpected readiness signal\n");
> + return -EIO;
> + }
> +
> + if (actual_size != size) {
> + drm_err(drm, "Actual size (%d) doesn't match expected size
> (%lu)\n",
> + actual_size, size);
> + return -EIO;
> + }
> +
> + if (response->msg != expected_response) {
> + drm_err(drm, "Unexpected response from device (expected %p4ch
> found %p4ch)\n",
> + &exp
Re: [PATCH v3 1/3] drm/format-helper: Add conversion from XRGB8888 to BGR888
On Fri, Feb 21, 2025 at 11:36:00AM +, Aditya Garg wrote:
> From: Kerem Karabay
>
> Add XRGB emulation helper for devices that only support BGR888.
...
> + for (x = 0; x < pixels; x++) {
> + pix = le32_to_cpu(sbuf32[x]);
> + /* write red-green-blue to output in little endianness */
> + *dbuf8++ = (pix & 0x00ff) >> 16;
> + *dbuf8++ = (pix & 0xff00) >> 8;
> + *dbuf8++ = (pix & 0x00ff) >> 0;
put_unaligned_be24()
> + }
...
> + static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
> + 3,
> + };
One line?
static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = { 3 };
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v5 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Tue, Feb 25, 2025 at 10:36:03AM +, Aditya Garg wrote: > > On 25 Feb 2025, at 4:03 PM, [email protected] wrote: > > On Tue, Feb 25, 2025 at 10:09:42AM +, Aditya Garg wrote: ... > >> +static int appletbdrm_probe(struct usb_interface *intf, > >> +const struct usb_device_id *id) > >> +{ > >> +struct usb_endpoint_descriptor *bulk_in, *bulk_out; > >> +struct device *dev = &intf->dev; > >> +struct appletbdrm_device *adev; > >> +struct drm_device *drm; > >> +int ret; > >> + > >> +ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, > >> &bulk_out, NULL, NULL); > >> +if (ret) { > >> +drm_err(drm, "Failed to find bulk endpoints\n"); > > > > This is simply wrong (and in this case even lead to crash in some > > circumstances). > > drm_err() may not be used here. That's my point in previous discussions. > > Independently on the subsystem the ->probe() for the sake of consistency and > > being informative should only rely on struct device *dev, > > I'm not sure how drm_err works, It's a macro. > but struct drm_device does have a struct device *dev as well. Yes, but only when it's initialized. > Anyways, this is something I'll leave for Thomas to reply. The code above is wrong independently on his reply :-) -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Tue, Feb 25, 2025 at 09:48:11AM +0100, Thomas Zimmermann wrote:
> Am 25.02.25 um 09:00 schrieb Aditya Garg:
> > > On 25 Feb 2025, at 1:22 PM, Thomas Zimmermann wrote:
> > > > Am 24.02.25 um 14:40 schrieb Aditya Garg:
...
> > > > +struct appletbdrm_device {
> > > > +struct device *dev;
> > > This field should go away, please. There's drm.dev, which contains the
> > > same address.
> > >
> > > So seems to have remove the dmadev field instead, which you'll need for
> > > dma-buf sharing. Was that a misunderstanding from the last review?
> > Yeah that was a misunderstanding. I though you meant to remove dmadev.
> > > The rest of the driver looks good.
> > Maybe you missed the left over dev_err_probe left in this patch? I'll fix
> > them.
>
> Sure.
But can you comment on my reply where I'm asking for a clarification how
drm_err() can be used on the physical device ->probe() to begin with?
> > > > +unsigned int in_ep;
> > > > +unsigned int out_ep;
> > > > +
> > > > +unsigned int width;
> > > > +unsigned int height;
> > > > +
> > > > +struct drm_device drm;
> > > > +struct drm_display_mode mode;
> > > > +struct drm_connector connector;
> > > > +struct drm_plane primary_plane;
> > > > +struct drm_crtc crtc;
> > > > +struct drm_encoder encoder;
> > > > +};
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 1/2] drm/format-helper: Add conversion from XRGB8888 to BGR888
On Tue, Feb 25, 2025 at 08:37:32AM +0100, Thomas Zimmermann wrote: > Am 24.02.25 um 15:29 schrieb [email protected]: > > On Mon, Feb 24, 2025 at 01:38:32PM +, Aditya Garg wrote: ... > > > +static void drm_fb_xrgb_to_bgr888_line(void *dbuf, const void *sbuf, > > > unsigned int pixels) > > Okay the xrgb is the actual pixel format independently on > > the CPU endianess. > > > > > +{ > > > + u8 *dbuf8 = dbuf; > > > + const __le32 *sbuf32 = sbuf; > > But here we assume that sbuf is __le32. > > And I think we may benefit from the __be32 there. > > No, please. XRGB is the logical order. The raw physical byte order for DRM > formats is always* little endian, hence reversed from the logical one. sbuf > points to raw memory and is therefore __le32. DRM-format byte order is > impossible to understand, I know. But that code is correct. Okay, so it's only about the colour (top-level) layout, the input and output data is always in little endian? > *) White lie: there's a DRM format flag signalling physical big endianess. > That isn't the case here. So nothing here should ever indicate big > endianess. But should it indicate the little? To me sounds like neither... > > > + unsigned int x; > > > + u32 pix; > > > + > > > + for (x = 0; x < pixels; x++) { > > > + pix = le32_to_cpu(sbuf32[x]); > > > + /* write red-green-blue to output in little endianness */ > > > + *dbuf8++ = (pix & 0x00ff) >> 16; > > > + *dbuf8++ = (pix & 0xff00) >> 8; > > > + *dbuf8++ = (pix & 0x00ff) >> 0; > > pix = be32_to_cpu(sbuf[4 * x]) >> 8; > > put_unaligned_le24(pix, &dbuf[3 * x]); > > > > > + } > > Or, after all, this __le32 magic might be not needed at all. Wouldn't the > > below > > be the equivalent > > > > static void drm_fb_xrgb_to_bgr888_line(void *dbuf, const void *sbuf, > > unsigned int pixels) > > { > > unsigned int x; > > u32 pix; > > > > for (x = 0; x < pixels; x++) { > > /* Read red-green-blue from input in big endianess and... */ > > pix = get_unaligned_be24(sbuf + x * 4 + 1); > > /* ...write it to output in little endianness. */ > > put_unaligned_le24(pix, dbuf + x * 3); > > } > > } > > > > The comments can even be dropped as the code quite clear about what's going > > on. > > > > > +} > > But it's up to you. I don't know which solution gives better code generation > > either. -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Tue, Feb 25, 2025 at 10:09:42AM +, Aditya Garg wrote:
> From: Kerem Karabay
>
> The Touch Bars found on x86 Macs support two USB configurations: one
> where the device presents itself as a HID keyboard and can display
> predefined sets of keys, and one where the operating system has full
> control over what is displayed.
>
> This commit adds support for the display functionality of the second
> configuration. Functionality for the first configuration has been
> merged in the HID tree.
>
> Note that this driver has only been tested on T2 Macs, and only includes
> the USB device ID for these devices. Testing on T1 Macs would be
> appreciated.
>
> Credit goes to Ben (Bingxing) Wang on GitHub for reverse engineering
> most of the protocol.
>
> Also, as requested by Andy, I would like to clarify the use of __packed
> structs in this driver:
>
> - All the packed structs are aligned except for appletbdrm_msg_information.
> - We have to pack appletbdrm_msg_information since it is requirement of
> the protocol.
> - We compared binaries compiled by keeping the rest structs __packed and
> not __packed using bloat-o-meter, and __packed was not affecting code
> generation.
> - To maintain consistency, rest structs have been kept __packed.
>
> I would also like to point out that since the driver was reverse-engineered
> the actual data types of the protocol might be different, including, but
> not limited to, endianness.
...
> +static int appletbdrm_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
> + struct device *dev = &intf->dev;
> + struct appletbdrm_device *adev;
> + struct drm_device *drm;
> + int ret;
> +
> + ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in,
> &bulk_out, NULL, NULL);
> + if (ret) {
> + drm_err(drm, "Failed to find bulk endpoints\n");
This is simply wrong (and in this case even lead to crash in some
circumstances).
drm_err() may not be used here. That's my point in previous discussions.
Independently on the subsystem the ->probe() for the sake of consistency and
being informative should only rely on struct device *dev,
> + return ret;
> + }
> +
> + adev = devm_drm_dev_alloc(dev, &appletbdrm_drm_driver, struct
> appletbdrm_device, drm);
> + if (IS_ERR(adev))
> + return PTR_ERR(adev);
> +
> + adev->in_ep = bulk_in->bEndpointAddress;
> + adev->out_ep = bulk_out->bEndpointAddress;
> + adev->dmadev = dev;
> +
> + drm = &adev->drm;
> +
> + usb_set_intfdata(intf, adev);
> +
> + ret = appletbdrm_get_information(adev);
> + if (ret) {
> + drm_err(drm, "Failed to get display information\n");
> + return ret;
> + }
> +
> + ret = appletbdrm_signal_readiness(adev);
> + if (ret) {
> + drm_err(drm, "Failed to signal readiness\n");
> + return ret;
> + }
> +
> + ret = appletbdrm_setup_mode_config(adev);
> + if (ret) {
> + drm_err(drm, "Failed to setup mode config\n");
> + return ret;
> + }
> +
> + ret = drm_dev_register(drm, 0);
> + if (ret) {
> + drm_err(drm, "Failed to register DRM device\n");
> + return ret;
> + }
> +
> + ret = appletbdrm_clear_display(adev);
> + if (ret) {
> + drm_err(drm, "Failed to clear display\n");
> + return ret;
> + }
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Mon, Feb 24, 2025 at 01:40:20PM +, Aditya Garg wrote:
> From: Kerem Karabay
>
> The Touch Bars found on x86 Macs support two USB configurations: one
> where the device presents itself as a HID keyboard and can display
> predefined sets of keys, and one where the operating system has full
> control over what is displayed.
>
> This commit adds support for the display functionality of the second
> configuration. Functionality for the first configuration has been
> merged in the HID tree.
>
> Note that this driver has only been tested on T2 Macs, and only includes
> the USB device ID for these devices. Testing on T1 Macs would be
> appreciated.
>
> Credit goes to Ben (Bingxing) Wang on GitHub for reverse engineering
> most of the protocol.
>
> Also, as requested by Andy, I would like to clarify the use of __packed
> structs in this driver:
>
> - All the packed structs are aligned except for appletbdrm_msg_information.
> - We have to pack appletbdrm_msg_information since it is requirement of
> the protocol.
> - We compared binaries compiled by keeping the rest structs __packed and
> not __packed using bloat-o-meter, and __packed was not affecting code
> generation.
> - To maintain consistency, rest structs have been kept __packed.
...
> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << 24) |
> (str4[1] << 16) | (str4[2] << 8) | str4[3]))
As commented previously this is quite strange what's going on with endianess in
this driver. Especially the above weirdness when get_unaligned_be32() is being
open coded and force-cast to __le32.
...
> +struct appletbdrm_msg_information {
> + struct appletbdrm_msg_response_header header;
> + u8 unk_14[12];
> + __le32 width;
> + __le32 height;
> + u8 bits_per_pixel;
> + __le32 bytes_per_row;
> + __le32 orientation;
> + __le32 bitmap_info;
> + __le32 pixel_format;
> + __le32 width_inches;/* floating point */
> + __le32 height_inches; /* floating point */
> +} __packed;
Haven't looked deeply into the protocol, but still makes me think that
the above (since it's the only __packed data type required) might be simply
depicted wrongly w.r.t. endianess / data types in use. It might be that
the data types have something combined and / or different types.
Do I understand correctly that the protocol was basically reverse-engineered?
...
> + /*
> + * The coordinate system used by the device is different from the
> + * coordinate system of the framebuffer in that the x and y axes are
> + * swapped, and that the y axis is inverted; so what the device reports
> + * as the height is actually the width of the framebuffer and vice
> + * versa
Missing period.
> + */
...
Otherwise it's nice tiny driver.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 2/3] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc
On Mon, Feb 24, 2025 at 10:18:48AM +, Aditya Garg wrote: > > > > On 24 Feb 2025, at 3:28 PM, [email protected] wrote: > > > > On Sat, Feb 22, 2025 at 03:46:03PM +, Aditya Garg wrote: > >>>> On 20 Feb 2025, at 10:09 PM, Aditya Garg wrote: > >>> > >>> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but > >>> it's useful to be able to print generic 4-character codes formatted as > >>> an integer. Extend it to add format specifiers for printing generic > >>> 32-bit FOURCCs with various endian semantics: > >>> > >>> %p4ch Host-endian > >>> %p4cl Little-endian > >>> %p4cb Big-endian > >>> %p4cr Reverse-endian > >>> > >>> The endianness determines how bytes are interpreted as a u32, and the > >>> FOURCC is then always printed MSByte-first (this is the opposite of > >>> V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would > >>> allow printing LSByte-first FOURCCs stored in host endian order > >>> (other than the hex form being in character order, not the integer > >>> value). > > > > ... > > > >> BTW, after looking at the comments by Martin [1], its actually better to > >> use > >> existing specifiers for the appletbdrm driver. The driver needs the host > >> endian as proposed by this patch, so instead of that, we can use %.4s > > > > Do you mean this patch will not be needed? If this a case, that would be the > > best solution. > > I tested with %4pE, and the results are different from expected. So this > would be preferred. Kindly see my latest email with a proposed workaround for > the sparse warnings. %.4s sounded okay, but %4pE is always about escaping and the result may occupy %4x memory (octal escaping of non-printable characters). Of course, you may vary the escaping classes, but IIRC the octal or hex escaping is unconditional. > >> [1]: > >> https://lore.kernel.org/asahi/[email protected]/ > >> > >> Alternatively we could add a host endian only. Other endians are not really > >> used by any driver AFAIK. The host endian is being used by appletbdrm and > >> Asahi Linux’ SMC driver only. -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 1/2] drm/format-helper: Add conversion from XRGB8888 to BGR888
On Mon, Feb 24, 2025 at 01:38:32PM +, Aditya Garg wrote:
> From: Kerem Karabay
>
> Add XRGB emulation helper for devices that only support BGR888.
...
> +static void drm_fb_xrgb_to_bgr888_line(void *dbuf, const void *sbuf,
> unsigned int pixels)
Okay the xrgb is the actual pixel format independently on
the CPU endianess.
> +{
> + u8 *dbuf8 = dbuf;
> + const __le32 *sbuf32 = sbuf;
But here we assume that sbuf is __le32.
And I think we may benefit from the __be32 there.
> + unsigned int x;
> + u32 pix;
> +
> + for (x = 0; x < pixels; x++) {
> + pix = le32_to_cpu(sbuf32[x]);
> + /* write red-green-blue to output in little endianness */
> + *dbuf8++ = (pix & 0x00ff) >> 16;
> + *dbuf8++ = (pix & 0xff00) >> 8;
> + *dbuf8++ = (pix & 0x00ff) >> 0;
pix = be32_to_cpu(sbuf[4 * x]) >> 8;
put_unaligned_le24(pix, &dbuf[3 * x]);
> + }
Or, after all, this __le32 magic might be not needed at all. Wouldn't the below
be the equivalent
static void drm_fb_xrgb_to_bgr888_line(void *dbuf, const void *sbuf,
unsigned int pixels)
{
unsigned int x;
u32 pix;
for (x = 0; x < pixels; x++) {
/* Read red-green-blue from input in big endianess and... */
pix = get_unaligned_be24(sbuf + x * 4 + 1);
/* ...write it to output in little endianness. */
put_unaligned_le24(pix, dbuf + x * 3);
}
}
The comments can even be dropped as the code quite clear about what's going on.
> +}
But it's up to you. I don't know which solution gives better code generation
either.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Mon, Feb 24, 2025 at 02:32:37PM +, Aditya Garg wrote: > > On 24 Feb 2025, at 7:30 PM, [email protected] wrote: > > On Mon, Feb 24, 2025 at 01:40:20PM +, Aditya Garg wrote: ... > >> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << 24) | > >> (str4[1] << 16) | (str4[2] << 8) | str4[3])) > > > > As commented previously this is quite strange what's going on with > > endianess in > > this driver. Especially the above weirdness when get_unaligned_be32() is > > being > > open coded and force-cast to __le32. > > I would assume it was also mimicked from the Windows driver, though I haven't > really tried exploring this there. > > I’d rather be happy if you give me code change suggestions and let me review > and test them For the starter I would do the following for all related constants and drop that weird and ugly macros at the top (it also has an issue with the str4 length as it is 5 bytes long, not 4, btw): #define APPLETBDRM_MSG_CLEAR_DISPLAY cpu_to_le32(0x434c5244) /* CLRD */ ... (assuming we stick with __leXX for now). This will be much less confusing. ... > >> +struct appletbdrm_msg_information { > >> + struct appletbdrm_msg_response_header header; > >> + u8 unk_14[12]; > >> + __le32 width; > >> + __le32 height; > >> + u8 bits_per_pixel; > >> + __le32 bytes_per_row; > >> + __le32 orientation; > >> + __le32 bitmap_info; > >> + __le32 pixel_format; > >> + __le32 width_inches; /* floating point */ > >> + __le32 height_inches; /* floating point */ > >> +} __packed; > > > > Haven't looked deeply into the protocol, but still makes me think that > > the above (since it's the only __packed data type required) might be simply > > depicted wrongly w.r.t. endianess / data types in use. It might be that > > the data types have something combined and / or different types. > > > > Do I understand correctly that the protocol was basically > > reverse-engineered? > > Yes. Although it was reverse engineered by the person who wrote the Windows > driver. The author has just made a Linux port. > So, as far as how is was reverse engineered, it not really possible for me to > explain. I don't even have any contact with the person who wrote the Windows > driver. The only point here would be to myself RE the hardware again, which > tbh isn't very motivating, considering that we have a working driver. Right. I agree that is better to have something working than something good looking, but wrong. Can you add a summary to the commit message that since the driver was reverse-engineered the actual data types of the protocol might be different (including, but not limited to, endianess)? ... > >> + /* > >> + * The coordinate system used by the device is different from the > >> + * coordinate system of the framebuffer in that the x and y axes are > >> + * swapped, and that the y axis is inverted; so what the device reports > >> + * as the height is actually the width of the framebuffer and vice > >> + * versa > > > > Missing period. > > Alright. For some reason (a mistake on my part), some dev_err_probe were also > still left in this version. But those are seems to me in the correct locations, no? How do we even know the DRM device before its creation? So, dev_err_probe() calls in ->probe() seem logical to me. Somebody from DRM should clarify this. > >> + */ ... > I’ll send a v5. Please, wait a bit. it's too fast to send one version quicker than 24h... -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 1/2] drm/format-helper: Add conversion from XRGB8888 to BGR888
On Mon, Feb 24, 2025 at 02:54:07PM +, Aditya Garg wrote: > This conversion helper mimics the existing drm_fb_xrgb_to_rgb888 helper Not really. See below. > > On 24 Feb 2025, at 7:59 PM, [email protected] wrote: > > On Mon, Feb 24, 2025 at 01:38:32PM +, Aditya Garg wrote: ... > >> +static void drm_fb_xrgb_to_bgr888_line(void *dbuf, const void *sbuf, > >> unsigned int pixels) > > > > Okay the xrgb is the actual pixel format independently on > > the CPU endianess. > > > >> +{ > >> + u8 *dbuf8 = dbuf; > >> + const __le32 *sbuf32 = sbuf; > > > > But here we assume that sbuf is __le32. > > And I think we may benefit from the __be32 there. > > So, like drm_fb_xrgb_to_rgb888, we are using __le32 > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_format_helper.c?h=v6.14-rc4#n657 The rgb888 != bgr888, that's where the byte swapping happens. So, one should use __be32 if the other has already been using __le32. > >> + unsigned int x; > >> + u32 pix; > >> + > >> + for (x = 0; x < pixels; x++) { > >> + pix = le32_to_cpu(sbuf32[x]); > >> + /* write red-green-blue to output in little endianness */ > >> + *dbuf8++ = (pix & 0x00ff) >> 16; > >> + *dbuf8++ = (pix & 0xff00) >> 8; > >> + *dbuf8++ = (pix & 0x00ff) >> 0; > > > > pix = be32_to_cpu(sbuf[4 * x]) >> 8; > > put_unaligned_le24(pix, &dbuf[3 * x]); > > Again, > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_format_helper.c?h=v6.14-rc4#n664 As per above. > >> + } > > > > Or, after all, this __le32 magic might be not needed at all. Wouldn't the > > below > > be the equivalent > > > > static void drm_fb_xrgb_to_bgr888_line(void *dbuf, const void *sbuf, > > unsigned int pixels) > > { > > unsigned int x; > > u32 pix; > > > > for (x = 0; x < pixels; x++) { > > /* Read red-green-blue from input in big endianess and... */ > > pix = get_unaligned_be24(sbuf + x * 4 + 1); > > /* ...write it to output in little endianness. */ > > put_unaligned_le24(pix, dbuf + x * 3); > > } > > } > > > > The comments can even be dropped as the code quite clear about what's going > > on. > > These comments are literally rewritten : > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_format_helper.c?h=v6.14-rc4#n663 > > >> +} > > > > But it's up to you. I don't know which solution gives better code generation > > either. > > I don't really mind any code change tbh, but I’d prefer that as an > improvement to existing code, and not a part of this patchset. Right, but see my argumentation above. -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 1/2] drm/format-helper: Add conversion from XRGB8888 to BGR888
On Mon, Feb 24, 2025 at 05:00:50PM +0200, [email protected] wrote: > On Mon, Feb 24, 2025 at 02:54:07PM +, Aditya Garg wrote: > > This conversion helper mimics the existing drm_fb_xrgb_to_rgb888 helper > > Not really. See below. > > > > On 24 Feb 2025, at 7:59 PM, [email protected] wrote: > > > On Mon, Feb 24, 2025 at 01:38:32PM +, Aditya Garg wrote: ... > > >> +static void drm_fb_xrgb_to_bgr888_line(void *dbuf, const void > > >> *sbuf, unsigned int pixels) > > > > > > Okay the xrgb is the actual pixel format independently on > > > the CPU endianess. > > > > > >> +{ > > >> + u8 *dbuf8 = dbuf; > > >> + const __le32 *sbuf32 = sbuf; > > > > > > But here we assume that sbuf is __le32. > > > And I think we may benefit from the __be32 there. > > > > So, like drm_fb_xrgb_to_rgb888, we are using __le32 > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_format_helper.c?h=v6.14-rc4#n657 > > The rgb888 != bgr888, that's where the byte swapping happens. So, one should > use __be32 if the other has already been using __le32. But in both cases it's actually the 24-bit format in 4-byte packets. I would rewrite both to remove these types as they are just confusing. But it's probably not your call. So, if you want to stick with __le32, fine, not my call either :-) > > >> + unsigned int x; > > >> + u32 pix; > > >> + > > >> + for (x = 0; x < pixels; x++) { > > >> + pix = le32_to_cpu(sbuf32[x]); > > >> + /* write red-green-blue to output in little endianness */ > > >> + *dbuf8++ = (pix & 0x00ff) >> 16; > > >> + *dbuf8++ = (pix & 0xff00) >> 8; > > >> + *dbuf8++ = (pix & 0x00ff) >> 0; > > > > > > pix = be32_to_cpu(sbuf[4 * x]) >> 8; > > > put_unaligned_le24(pix, &dbuf[3 * x]); > > > > Again, > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_format_helper.c?h=v6.14-rc4#n664 > > As per above. > > > >> + } > > > > > > Or, after all, this __le32 magic might be not needed at all. Wouldn't the > > > below > > > be the equivalent > > > > > > static void drm_fb_xrgb_to_bgr888_line(void *dbuf, const void *sbuf, > > > unsigned int pixels) > > > { > > > unsigned int x; > > > u32 pix; > > > > > > for (x = 0; x < pixels; x++) { > > > /* Read red-green-blue from input in big endianess and... */ > > > pix = get_unaligned_be24(sbuf + x * 4 + 1); > > > /* ...write it to output in little endianness. */ > > > put_unaligned_le24(pix, dbuf + x * 3); > > > } > > > } > > > > > > The comments can even be dropped as the code quite clear about what's > > > going on. -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Mon, Feb 24, 2025 at 03:03:40PM +, Aditya Garg wrote: > > On 24 Feb 2025, at 8:27 PM, [email protected] wrote: > > On Mon, Feb 24, 2025 at 02:32:37PM +, Aditya Garg wrote: > >>> On 24 Feb 2025, at 7:30 PM, [email protected] wrote: > >>> On Mon, Feb 24, 2025 at 01:40:20PM +, Aditya Garg wrote: ... > >>>> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << 24) | > >>>> (str4[1] << 16) | (str4[2] << 8) | str4[3])) > >>> > >>> As commented previously this is quite strange what's going on with > >>> endianess in > >>> this driver. Especially the above weirdness when get_unaligned_be32() is > >>> being > >>> open coded and force-cast to __le32. > >> > >> I would assume it was also mimicked from the Windows driver, though I > >> haven't > >> really tried exploring this there. > >> > >> I’d rather be happy if you give me code change suggestions and let me > >> review > >> and test them > > > > For the starter I would do the following for all related constants and > > drop that weird and ugly macros at the top (it also has an issue with > > the str4 length as it is 5 bytes long, not 4, btw): > > > > #define APPLETBDRM_MSG_CLEAR_DISPLAY cpu_to_le32(0x434c5244) /* CLRD */ > > Lemme test this. Just in case it won't work, reverse bytes in the integer. Because I was lost in this conversion. > > (assuming we stick with __leXX for now). This will be much less confusing. ... > >> Alright. For some reason (a mistake on my part), some dev_err_probe were > >> also > >> still left in this version. > > > > But those are seems to me in the correct locations, no? How do we even know > > the DRM device before its creation? So, dev_err_probe() calls in ->probe() > > seem logical to me. Somebody from DRM should clarify this. > > Thomas asked me to do this change. Maybe you didn’t see his reply. I saw, maybe I took it wrong, but I really don't understand how on earth drm_err() or whatever can be used in real ->probe() of the physical device. Imagine the hypotetical case probe(strict device *dev) { mydrm; foo; ... foo = devm_gpiod_get(dev, ...); if (IS_ERR(foo)) return dev_err_probe(dev, ...); // how?! ... mydrm = ...DRM alloc...; ... } I don't even believe it will be possible to create drm_err_probe() as it most likely will require to have an allocation to be always the first op (that may fail) in the ->probe() which might be not the case for some device drivers. > >>>> + */ -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Mon, Feb 24, 2025 at 03:20:13PM +, Aditya Garg wrote: > > On 24 Feb 2025, at 8:41 PM, [email protected] wrote: > > On Mon, Feb 24, 2025 at 03:03:40PM +, Aditya Garg wrote: > >>>> On 24 Feb 2025, at 8:27 PM, [email protected] wrote: > >>> On Mon, Feb 24, 2025 at 02:32:37PM +, Aditya Garg wrote: > >>>>> On 24 Feb 2025, at 7:30 PM, [email protected] wrote: > >>>>> On Mon, Feb 24, 2025 at 01:40:20PM +, Aditya Garg wrote: ... > >>>>>> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << 24) > >>>>>> | (str4[1] << 16) | (str4[2] << 8) | str4[3])) > >>>>> > >>>>> As commented previously this is quite strange what's going on with > >>>>> endianess in > >>>>> this driver. Especially the above weirdness when get_unaligned_be32() > >>>>> is being > >>>>> open coded and force-cast to __le32. > >>>> > >>>> I would assume it was also mimicked from the Windows driver, though I > >>>> haven't > >>>> really tried exploring this there. > >>>> > >>>> I’d rather be happy if you give me code change suggestions and let me > >>>> review > >>>> and test them > >>> > >>> For the starter I would do the following for all related constants and > >>> drop that weird and ugly macros at the top (it also has an issue with > >>> the str4 length as it is 5 bytes long, not 4, btw): > >>> > >>> #define APPLETBDRM_MSG_CLEAR_DISPLAY cpu_to_le32(0x434c5244) /* CLRD */ > >> > >> Lemme test this. > > > > Just in case it won't work, reverse bytes in the integer. Because I was > > lost in > > this conversion. > > It works. What I understand is that you used the macro to get the final hex > and converted it into little endian, which on the x86 macs would technically > remain the same. Right, the problem is the macro itself which does really weird things altogether. Using integer + comment much clearer in my opinion. > >>> (assuming we stick with __leXX for now). This will be much less confusing. -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Mon, Feb 24, 2025 at 03:32:56PM +, Aditya Garg wrote: > > On 24 Feb 2025, at 8:50 PM, Aditya Garg wrote: > >> On 24 Feb 2025, at 8:41 PM, [email protected] wrote: > >> On Mon, Feb 24, 2025 at 03:03:40PM +, Aditya Garg wrote: > >>>>> On 24 Feb 2025, at 8:27 PM, [email protected] wrote: > >>>> On Mon, Feb 24, 2025 at 02:32:37PM +, Aditya Garg wrote: > >>>>>> On 24 Feb 2025, at 7:30 PM, [email protected] wrote: > >>>>>>> On Mon, Feb 24, 2025 at 01:40:20PM +, Aditya Garg wrote: ... > >>>>>>>> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << > >>>>>>>> 24) | (str4[1] << 16) | (str4[2] << 8) | str4[3])) > >>>>>>> > >>>>>>> As commented previously this is quite strange what's going on with > >>>>>>> endianess in > >>>>>>> this driver. Especially the above weirdness when get_unaligned_be32() > >>>>>>> is being > >>>>>>> open coded and force-cast to __le32. > >>>>>> > >>>>>> I would assume it was also mimicked from the Windows driver, though I > >>>>>> haven't > >>>>>> really tried exploring this there. > >>>>>> > >>>>>> I’d rather be happy if you give me code change suggestions and let me > >>>>>> review > >>>>>> and test them > >>>>> > >>>>> For the starter I would do the following for all related constants and > >>>>> drop that weird and ugly macros at the top (it also has an issue with > >>>>> the str4 length as it is 5 bytes long, not 4, btw): > >>>>> > >>>>> #define APPLETBDRM_MSG_CLEAR_DISPLAY cpu_to_le32(0x434c5244) /* CLRD */ > >>> > >>> Lemme test this. > >> > >> Just in case it won't work, reverse bytes in the integer. Because I was > >> lost in > >> this conversion. > > > > It works. What I understand is that you used the macro to get the final hex > > and converted it into little endian, which on the x86 macs would > > technically remain the same. > > And now that I oberved again, %p4cc is actually printing these CLRD, REDY etc > in reverse order, probably the reason %p4ch was chosen. And I am unable to > find what macro upstream can be used. %.4s should work as it technically not DRM 4cc, but specifics of the protocol (that reminds me about ACPI that uses 4cc a lot). -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Mon, Feb 24, 2025 at 03:39:56PM +, Aditya Garg wrote: > > On 24 Feb 2025, at 9:07 PM, [email protected] wrote: > > On Mon, Feb 24, 2025 at 03:20:13PM +, Aditya Garg wrote: > >>>> On 24 Feb 2025, at 8:41 PM, [email protected] wrote: > >>> On Mon, Feb 24, 2025 at 03:03:40PM +, Aditya Garg wrote: > >>>>>> On 24 Feb 2025, at 8:27 PM, [email protected] wrote: > >>>>> On Mon, Feb 24, 2025 at 02:32:37PM +0000, Aditya Garg wrote: > >>>>>>> On 24 Feb 2025, at 7:30 PM, [email protected] wrote: > >>>>>>> On Mon, Feb 24, 2025 at 01:40:20PM +, Aditya Garg wrote: ... > >>>>>>>> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << > >>>>>>>> 24) | (str4[1] << 16) | (str4[2] << 8) | str4[3])) > >>>>>>> > >>>>>>> As commented previously this is quite strange what's going on with > >>>>>>> endianess in > >>>>>>> this driver. Especially the above weirdness when get_unaligned_be32() > >>>>>>> is being > >>>>>>> open coded and force-cast to __le32. > >>>>>> > >>>>>> I would assume it was also mimicked from the Windows driver, though I > >>>>>> haven't > >>>>>> really tried exploring this there. > >>>>>> > >>>>>> I’d rather be happy if you give me code change suggestions and let me > >>>>>> review > >>>>>> and test them > >>>>> > >>>>> For the starter I would do the following for all related constants and > >>>>> drop that weird and ugly macros at the top (it also has an issue with > >>>>> the str4 length as it is 5 bytes long, not 4, btw): > >>>>> > >>>>> #define APPLETBDRM_MSG_CLEAR_DISPLAY cpu_to_le32(0x434c5244) /* CLRD */ > >>>> > >>>> Lemme test this. > >>> > >>> Just in case it won't work, reverse bytes in the integer. Because I was > >>> lost in > >>> this conversion. > >> > >> It works. What I understand is that you used the macro to get the final hex > >> and converted it into little endian, which on the x86 macs would > >> technically > >> remain the same. > > The Macro is just converting the letters into their hex form, but simply > calculating them and putting the letters in comments is equally good. Again, it does it in most confusing and weird way. Just kill it. > > Right, the problem is the macro itself which does really weird things > > altogether. > > Using integer + comment much clearer in my opinion. > > > >>>>> (assuming we stick with __leXX for now). This will be much less > >>>>> confusing. -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Mon, Feb 24, 2025 at 03:40:29PM +, Aditya Garg wrote: > > On 24 Feb 2025, at 9:08 PM, [email protected] wrote: > > On Mon, Feb 24, 2025 at 03:32:56PM +, Aditya Garg wrote: > >>> On 24 Feb 2025, at 8:50 PM, Aditya Garg wrote: > >>>> On 24 Feb 2025, at 8:41 PM, [email protected] wrote: > >>>> On Mon, Feb 24, 2025 at 03:03:40PM +, Aditya Garg wrote: > >>>>>>> On 24 Feb 2025, at 8:27 PM, [email protected] wrote: > >>>>>> On Mon, Feb 24, 2025 at 02:32:37PM +, Aditya Garg wrote: > >>>>>>>> On 24 Feb 2025, at 7:30 PM, [email protected] wrote: > >>>>>>>>>> On Mon, Feb 24, 2025 at 01:40:20PM +, Aditya Garg wrote: ... > >>>>>>>>>>> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] > >>>>>>>>>>> << 24) | (str4[1] << 16) | (str4[2] << 8) | str4[3])) > >>>>>>>>>> > >>>>>>>>>> As commented previously this is quite strange what's going on with > >>>>>>>>>> endianess in > >>>>>>>>>> this driver. Especially the above weirdness when > >>>>>>>>>> get_unaligned_be32() is being > >>>>>>>>>> open coded and force-cast to __le32. > >>>>>>>>> > >>>>>>>>> I would assume it was also mimicked from the Windows driver, though > >>>>>>>>> I haven't > >>>>>>>>> really tried exploring this there. > >>>>>>>>> > >>>>>>>>> I’d rather be happy if you give me code change suggestions and let > >>>>>>>>> me review > >>>>>>>>> and test them > >>>>>>>> > >>>>>>>> For the starter I would do the following for all related constants > >>>>>>>> and > >>>>>>>> drop that weird and ugly macros at the top (it also has an issue with > >>>>>>>> the str4 length as it is 5 bytes long, not 4, btw): > >>>>>>>> > >>>>>>>> #define APPLETBDRM_MSG_CLEAR_DISPLAY cpu_to_le32(0x434c5244) /* CLRD > >>>>>>>> */ > >>>>>> > >>>>>> Lemme test this. > >>>>> > >>>>> Just in case it won't work, reverse bytes in the integer. Because I was > >>>>> lost in > >>>>> this conversion. > >>> > >>> It works. What I understand is that you used the macro to get the final > >>> hex and converted it into little endian, which on the x86 macs would > >>> technically remain the same. > >> > >> And now that I oberved again, %p4cc is actually printing these CLRD, REDY > >> etc > >> in reverse order, probably the reason %p4ch was chosen. And I am unable to > >> find what macro upstream can be used. > > > > %.4s should work as it technically not DRM 4cc, but specifics of the > > protocol > > (that reminds me about ACPI that uses 4cc a lot). > > I still get reverse order in that. Ah, right, it will give you the first letter as LSB, indeed. At the end of the day if it's so important, there are ways how to solve that without using %p4cc. But if others (and esp. PRINTK maintainers) want to have / don't object having that extension, why not? -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Mon, Feb 24, 2025 at 03:52:03PM +, Aditya Garg wrote: > > On 24 Feb 2025, at 9:19 PM, [email protected] wrote: > > On Mon, Feb 24, 2025 at 03:39:56PM +, Aditya Garg wrote: > >>>> On 24 Feb 2025, at 9:07 PM, [email protected] wrote: > >>> On Mon, Feb 24, 2025 at 03:20:13PM +, Aditya Garg wrote: > >>>>>> On 24 Feb 2025, at 8:41 PM, [email protected] wrote: > >>>>> On Mon, Feb 24, 2025 at 03:03:40PM +, Aditya Garg wrote: > >>>>>>>> On 24 Feb 2025, at 8:27 PM, [email protected] wrote: > >>>>>>> On Mon, Feb 24, 2025 at 02:32:37PM +, Aditya Garg wrote: > >>>>>>>>> On 24 Feb 2025, at 7:30 PM, [email protected] wrote: > >>>>>>>>> On Mon, Feb 24, 2025 at 01:40:20PM +, Aditya Garg wrote: ... > >>>>>>>>>> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << > >>>>>>>>>> 24) | (str4[1] << 16) | (str4[2] << 8) | str4[3])) > >>>>>>>>> > >>>>>>>>> As commented previously this is quite strange what's going on with > >>>>>>>>> endianess in > >>>>>>>>> this driver. Especially the above weirdness when > >>>>>>>>> get_unaligned_be32() is being > >>>>>>>>> open coded and force-cast to __le32. > >>>>>>>> > >>>>>>>> I would assume it was also mimicked from the Windows driver, though > >>>>>>>> I haven't > >>>>>>>> really tried exploring this there. > >>>>>>>> > >>>>>>>> I’d rather be happy if you give me code change suggestions and let > >>>>>>>> me review > >>>>>>>> and test them > >>>>>>> > >>>>>>> For the starter I would do the following for all related constants and > >>>>>>> drop that weird and ugly macros at the top (it also has an issue with > >>>>>>> the str4 length as it is 5 bytes long, not 4, btw): > >>>>>>> > >>>>>>> #define APPLETBDRM_MSG_CLEAR_DISPLAY cpu_to_le32(0x434c5244) /* CLRD > >>>>>>> */ > >>>>>> > >>>>>> Lemme test this. > >>>>> > >>>>> Just in case it won't work, reverse bytes in the integer. Because I was > >>>>> lost in > >>>>> this conversion. > >>>> > >>>> It works. What I understand is that you used the macro to get the final > >>>> hex > >>>> and converted it into little endian, which on the x86 macs would > >>>> technically > >>>> remain the same. > >> > >> The Macro is just converting the letters into their hex form, but simply > >> calculating them and putting the letters in comments is equally good. > > > > Again, it does it in most confusing and weird way. Just kill it. > > #define APPLETBDRM_PIXEL_FORMAT cpu_to_le32(0x52474241) /* > RGBA, the actual format is BGR888 */ > #define APPLETBDRM_BITS_PER_PIXEL 24 > > #define APPLETBDRM_MSG_CLEAR_DISPLAY cpu_to_le32(0x434c5244) /* CLRD */ > #define APPLETBDRM_MSG_GET_INFORMATIONcpu_to_le32(0x47494e46) /* GINF > */ > #define APPLETBDRM_MSG_UPDATE_COMPLETEcpu_to_le32(0x5544434c) /* UDCL > */ > #define APPLETBDRM_MSG_SIGNAL_READINESS cpu_to_le32(0x52454459) /* REDY > */ > > This should be good. Yep, way better than the original code! -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Mon, Feb 24, 2025 at 03:56:36PM +, Aditya Garg wrote: > > On 24 Feb 2025, at 9:23 PM, [email protected] wrote: > > On Mon, Feb 24, 2025 at 03:40:29PM +, Aditya Garg wrote: > >>>> On 24 Feb 2025, at 9:08 PM, [email protected] wrote: > >>> On Mon, Feb 24, 2025 at 03:32:56PM +, Aditya Garg wrote: > >>>>> On 24 Feb 2025, at 8:50 PM, Aditya Garg wrote: > >>>>>> On 24 Feb 2025, at 8:41 PM, [email protected] wrote: > >>>>>> On Mon, Feb 24, 2025 at 03:03:40PM +, Aditya Garg wrote: > >>>>>>>>> On 24 Feb 2025, at 8:27 PM, [email protected] wrote: > >>>>>>>> On Mon, Feb 24, 2025 at 02:32:37PM +, Aditya Garg wrote: > >>>>>>>>>> On 24 Feb 2025, at 7:30 PM, [email protected] > >>>>>>>>>> wrote: > >>>>>>>>>>>> On Mon, Feb 24, 2025 at 01:40:20PM +, Aditya Garg wrote: ... > >>>>>>>>>>>>> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] > >>>>>>>>>>>>> << 24) | (str4[1] << 16) | (str4[2] << 8) | str4[3])) > >>>>>>>>>>>> > >>>>>>>>>>>> As commented previously this is quite strange what's going on > >>>>>>>>>>>> with endianess in > >>>>>>>>>>>> this driver. Especially the above weirdness when > >>>>>>>>>>>> get_unaligned_be32() is being > >>>>>>>>>>>> open coded and force-cast to __le32. > >>>>>>>>>>> > >>>>>>>>>>> I would assume it was also mimicked from the Windows driver, > >>>>>>>>>>> though I haven't > >>>>>>>>>>> really tried exploring this there. > >>>>>>>>>>> > >>>>>>>>>>> I’d rather be happy if you give me code change suggestions and > >>>>>>>>>>> let me review > >>>>>>>>>>> and test them > >>>>>>>>>> > >>>>>>>>>> For the starter I would do the following for all related constants > >>>>>>>>>> and > >>>>>>>>>> drop that weird and ugly macros at the top (it also has an issue > >>>>>>>>>> with > >>>>>>>>>> the str4 length as it is 5 bytes long, not 4, btw): > >>>>>>>>>> > >>>>>>>>>> #define APPLETBDRM_MSG_CLEAR_DISPLAY cpu_to_le32(0x434c5244) /* > >>>>>>>>>> CLRD */ > >>>>>>>> > >>>>>>>> Lemme test this. > >>>>>>> > >>>>>>> Just in case it won't work, reverse bytes in the integer. Because I > >>>>>>> was lost in > >>>>>>> this conversion. > >>>>> > >>>>> It works. What I understand is that you used the macro to get the final > >>>>> hex and converted it into little endian, which on the x86 macs would > >>>>> technically remain the same. > >>>> > >>>> And now that I oberved again, %p4cc is actually printing these CLRD, > >>>> REDY etc > >>>> in reverse order, probably the reason %p4ch was chosen. And I am unable > >>>> to > >>>> find what macro upstream can be used. > >>> > >>> %.4s should work as it technically not DRM 4cc, but specifics of the > >>> protocol > >>> (that reminds me about ACPI that uses 4cc a lot). > >> > >> I still get reverse order in that. > > > > Ah, right, it will give you the first letter as LSB, indeed. At the end of > > the > > day if it's so important, there are ways how to solve that without using > > %p4cc. > > But if others (and esp. PRINTK maintainers) want to have / don't object > > having > > that extension, why not? > > Right, but what to do about the case of little endian and host endian? I > remember the statement "for the sake of completeness" for them. Do you think > just host endian and reverse endian should be just fine? Or you got any "no > sparse warning" way to get it done? The macros to convert to le32/be32 expect > a u32 value, but in those cases we actually are passing a le32/be32 value. For now I think we better save the energy and wait for PRINTK people to tell if they are okay in general. Otherwise it makes no sense to develop and review something that will go to the trash bin. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 3/3] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Mon, Feb 24, 2025 at 09:41:43AM +0100, Thomas Zimmermann wrote: > Am 22.02.25 um 10:07 schrieb Aditya Garg: ... > > > What padding, please? Why TCP UAPI headers do not have these attributes? > > > Think about it, and think about what actually __packed does and how it > > > affects > > > (badly) the code generation. Otherwise it looks like a cargo cult. > > > > > > > I tried removing __packed btw and driver no longer works. > > > So, you need to find a justification why. But definitely not due to > > > padding in > > > many of them. They can go without __packed as they are naturally aligned. > > Alright, I did some debugging, basically printk sizeof(struct). Did it for > > both packed and unpacked with the following results: > > > > Feb 22 13:02:03 MacBook kernel: size of struct > > appletbdrm_msg_request_header is 16 > > Feb 22 13:02:03 MacBook kernel: size of struct > > appletbdrm_msg_request_header_unpacked is 16 > > > > Feb 22 13:02:03 MacBook kernel: size of struct > > appletbdrm_msg_response_header is 20 > > Feb 22 13:02:03 MacBook kernel: size of struct > > appletbdrm_msg_response_header_unpacked is 20 > > > > Feb 22 13:02:03 MacBook kernel: size of struct > > appletbdrm_msg_simple_request is 32 > > Feb 22 13:02:03 MacBook kernel: size of struct > > appletbdrm_msg_simple_request_unpacked is 32 > > > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_information > > is 65 > > Feb 22 13:02:03 MacBook kernel: size of struct > > appletbdrm_msg_information_unpacked is 68 > > In the unpacked version, there is a 3-byte gap after the 'bits_per_pixel' to > align the next field. Using __packed removes those gaps at the expense of > runtime overhead. > > > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_frame is 12 > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_frame_unpacked is > > 12 > > > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_fb_request_footer > > is 80 > > Feb 22 13:02:03 MacBook kernel: size of struct > > appletbdrm_fb_request_footer_unpacked is 80 > > > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_fb_request is 48 > > Feb 22 13:02:03 MacBook kernel: size of struct > > appletbdrm_fb_request_unpacked is 48 > > > > Feb 22 13:02:03 MacBook kernel: size of struct > > appletbdrm_fb_request_response is 40 > > Feb 22 13:02:04 MacBook kernel: size of struct > > appletbdrm_fb_request_response_unpacked is 40 > > > > So, the difference in sizeof in unpacked and packed is only in > > appletbdrm_msg_information. So, I kept this packed, and removed it from > > others. The Touch Bar still works. > > > > So maybe keep just this packed? > > The fields in the TCP header are aligned by design. > Unfortunately, this hardware's protocol is not. And there's no way of fixing > this now. Just keep all of them packed if you want. It would be nice to see the difference in the code generation for the all __packed vs. only those that require it. > At least it's clear then > what happens. And if your hardware requires this, you can't do much anyway. One aspect (member level alignment) is clear but the other is not (object level alignment). I dunno if it makes sense to be pedantic about this, but would like to see the binary outcome asked for. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/3] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc
On Sat, Feb 22, 2025 at 03:46:03PM +, Aditya Garg wrote: > > On 20 Feb 2025, at 10:09 PM, Aditya Garg wrote: > > > > %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but > > it's useful to be able to print generic 4-character codes formatted as > > an integer. Extend it to add format specifiers for printing generic > > 32-bit FOURCCs with various endian semantics: > > > > %p4ch Host-endian > > %p4cl Little-endian > > %p4cb Big-endian > > %p4cr Reverse-endian > > > > The endianness determines how bytes are interpreted as a u32, and the > > FOURCC is then always printed MSByte-first (this is the opposite of > > V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would > > allow printing LSByte-first FOURCCs stored in host endian order > > (other than the hex form being in character order, not the integer > > value). ... > BTW, after looking at the comments by Martin [1], its actually better to use > existing specifiers for the appletbdrm driver. The driver needs the host > endian as proposed by this patch, so instead of that, we can use %.4s Do you mean this patch will not be needed? If this a case, that would be the best solution. > [1]: > https://lore.kernel.org/asahi/[email protected]/ > > Alternatively we could add a host endian only. Other endians are not really > used by any driver AFAIK. The host endian is being used by appletbdrm and > Asahi Linux’ SMC driver only. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/3] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc
On Mon, Feb 24, 2025 at 10:43:54AM +, Aditya Garg wrote: > > On 24 Feb 2025, at 4:11 PM, [email protected] wrote: > > On Mon, Feb 24, 2025 at 10:32:27AM +, Aditya Garg wrote: > >>>> On 24 Feb 2025, at 3:54 PM, [email protected] wrote: > >>> On Mon, Feb 24, 2025 at 10:18:48AM +, Aditya Garg wrote: > >>>>>> On 24 Feb 2025, at 3:28 PM, [email protected] wrote: > >>>>> On Sat, Feb 22, 2025 at 03:46:03PM +, Aditya Garg wrote: > >>>>>>>> On 20 Feb 2025, at 10:09 PM, Aditya Garg > >>>>>>>> wrote: ... > >>>>>>> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but > >>>>>>> it's useful to be able to print generic 4-character codes formatted as > >>>>>>> an integer. Extend it to add format specifiers for printing generic > >>>>>>> 32-bit FOURCCs with various endian semantics: > >>>>>>> > >>>>>>> %p4ch Host-endian > >>>>>>> %p4cl Little-endian > >>>>>>> %p4cb Big-endian > >>>>>>> %p4cr Reverse-endian > >>>>>>> > >>>>>>> The endianness determines how bytes are interpreted as a u32, and the > >>>>>>> FOURCC is then always printed MSByte-first (this is the opposite of > >>>>>>> V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would > >>>>>>> allow printing LSByte-first FOURCCs stored in host endian order > >>>>>>> (other than the hex form being in character order, not the integer > >>>>>>> value). > >>>>> > >>>>> ... > >>>>> > >>>>>> BTW, after looking at the comments by Martin [1], its actually better > >>>>>> to use > >>>>>> existing specifiers for the appletbdrm driver. The driver needs the > >>>>>> host > >>>>>> endian as proposed by this patch, so instead of that, we can use %.4s > >>>>> > >>>>> Do you mean this patch will not be needed? If this a case, that would > >>>>> be the > >>>>> best solution. > >>>> > >>>> I tested with %4pE, and the results are different from expected. So this > >>>> would be preferred. Kindly see my latest email with a proposed > >>>> workaround for > >>>> the sparse warnings. > >>> > >>> %.4s sounded okay, but %4pE is always about escaping and the result may > >>> occupy > >>> %4x memory (octal escaping of non-printable characters). Of course, you > >>> may vary > >>> the escaping classes, but IIRC the octal or hex escaping is unconditional. > >> > >> %.4s is used for unsigned int iirc, here it's __le32. > > > > No, it's used to 'char *'. in case one may guarantee that it at least is > > four characters long. > Still the argument here is __le32. %p4h is showing reverse values than what > %4pE as well as %.4s shows. For __le32 the %.4s will print from LSB to MSB and otherwise for __be32. You can provide any conversion you want to have it stable between the endianesses. In any case looking at the DRM patch it might be that the entire driver got the endianess wrong. Have you checked my comment there? > >>>>>> [1]: > >>>>>> https://lore.kernel.org/asahi/[email protected]/ > >>>>>> > >>>>>> Alternatively we could add a host endian only. Other endians are not > >>>>>> really > >>>>>> used by any driver AFAIK. The host endian is being used by appletbdrm > >>>>>> and > >>>>>> Asahi Linux’ SMC driver only. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 3/3] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Mon, Feb 24, 2025 at 11:20:12AM +, Aditya Garg wrote: > > > > > It would be nice to see the difference in the code generation for the all > > __packed vs. only those that require it. > > > >> At least it's clear then > >> what happens. And if your hardware requires this, you can't do much anyway. > > > > One aspect (member level alignment) is clear but the other is not > > (object level alignment). I dunno if it makes sense to be pedantic about > > this, > > but would like to see the binary outcome asked for. > > Hex dump of the compiled binary: Oh, sorry I wasn't clear. We have a script called bloat-o-meter for these purposes. Please, run it with old and new binaries as parameters and share the output. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 3/3] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Mon, Feb 24, 2025 at 11:57:47AM +, Aditya Garg wrote: > > On 24 Feb 2025, at 5:13 PM, [email protected] wrote: > > On Mon, Feb 24, 2025 at 11:20:12AM +, Aditya Garg wrote: > >> > >>> It would be nice to see the difference in the code generation for the all > >>> __packed vs. only those that require it. > >>> > >>>> At least it's clear then > >>>> what happens. And if your hardware requires this, you can't do much > >>>> anyway. > >>> > >>> One aspect (member level alignment) is clear but the other is not > >>> (object level alignment). I dunno if it makes sense to be pedantic about > >>> this, > >>> but would like to see the binary outcome asked for. > >> > >> Hex dump of the compiled binary: > > > > Oh, sorry I wasn't clear. We have a script called bloat-o-meter for these > > purposes. Please, run it with old and new binaries as parameters and share > > the > > output. > aditya@MacBook:~/linux$ ./scripts/bloat-o-meter $PACKED $UNPACKED > add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) > Function old new delta > Total: Before=13286, After=13286, chg +0.00% Thanks! That shows that __packed can be used for all protocol related data types. Just mention in the commit message that the __packed, even if not needed, is: a) for the consistency, b) not affecting code generation in accordance with bloat-o-meter. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/3] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc
On Sun, Feb 23, 2025 at 06:39:15AM +, Aditya Garg wrote: > > On 22 Feb 2025, at 5:41 PM, Aditya Garg wrote: > >> On 21 Feb 2025, at 8:57 PM, [email protected] wrote: > >>> On Thu, Feb 20, 2025 at 04:39:23PM +, Aditya Garg wrote: ... > >>> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but > >>> it's useful to be able to print generic 4-character codes formatted as > >>> an integer. Extend it to add format specifiers for printing generic > >>> 32-bit FOURCCs with various endian semantics: > >>> %p4ch Host-endian > >>> %p4cl Little-endian > >>> %p4cb Big-endian > >>> %p4cr Reverse-endian > >>> The endianness determines how bytes are interpreted as a u32, and the > >>> FOURCC is then always printed MSByte-first (this is the opposite of > >>> V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would > >>> allow printing LSByte-first FOURCCs stored in host endian order > >>> (other than the hex form being in character order, not the integer > >>> value). > >> ... > >>> orig = get_unaligned(fourcc); > >>> - val = orig & ~BIT(31); > >>> + switch (fmt[2]) { > >>> + case 'h': > >>> + val = orig; > >>> + break; > >>> + case 'r': > >>> + orig = swab32(orig); > >>> + val = orig; > >>> + break; > >>> + case 'l': > >>> + orig = le32_to_cpu(orig); > >>> + val = orig; > >>> + break; > >>> + case 'b': > >>> + orig = be32_to_cpu(orig); > >> I do not see that orig is a union of different types. Have you run sparse? > >> It will definitely complain on this code. > > > > After messing around with this, what I’ve noticed is that orig and val used > > in this struct should be u32. Now in case of little endian and big endian, > > that things are messy. The original code by Hector was using le32_to_cpu on > > orig, which itself is declared as a u32 here (maybe was done with the > > intention to convert le32 orig to u32 orig?). > > > > Anyways, what I have done is that: > > > > 1. Declare new variable, orig_le which is __le32. > > 2. Instead of doing orig = le32_to_cpu(orig); , we can do orig_le = > > cpu_to_le32(orig). This fixes the sparse warning: cast to restricted __le32 > > 3. Now the original code was intending to use val=orig=le32_to_cpu(orig) at > > the bottom part of this struct. Those parts also require val and orig to be > > u32. For that, we are now using le32_to_cpu(orig_le). Since val is same as > > orig, in case these cases, instead of making a val_le, I’ve simply used > > orig_le there as well. > > > > Similar changes done for big endian. > > > > So, the struct looks like this now: > > > > static noinline_for_stack > > char *fourcc_string(char *buf, char *end, const u32 *fourcc, > > struct printf_spec spec, const char *fmt) > > { > > char output[sizeof("0123 little-endian (0x01234567)")]; > > char *p = output; > > unsigned int i; > > unsigned char c; > > bool pixel_fmt = false; > > u32 orig, val; > > __le32 orig_le; > > __be32 orig_be; > > > > if (fmt[1] != 'c') > > return error_string(buf, end, "(%p4?)", spec); > > > > if (check_pointer(&buf, end, fourcc, spec)) > > return buf; > > > > orig = get_unaligned(fourcc); > > switch (fmt[2]) { > > case 'h': > > val = orig; > > break; > > case 'r': > > orig = swab32(orig); > > val = orig; > > break; > > case 'l': > > orig_le = cpu_to_le32(orig); > > break; > > case 'b': > > orig_be = cpu_to_be32(orig); > > break; > > case 'c': > > /* Pixel formats are printed LSB-first */ > > val = swab32(orig & ~BIT(31)); > > pixel_fmt = true; > > break; > > default: > > return error_string(buf, end, "(%p4?)", spec); > > } > > > > for (i = 0; i < sizeof(u32); i++) { > > switch (fmt[2]) { > > case 'h': > > case 'r': > > case 'c': > > c = val >> ((3 - i) * 8); > > break; > > case 'l': > >
Re: [PATCH v3 1/3] drm/format-helper: Add conversion from XRGB8888 to BGR888
On Mon, Feb 24, 2025 at 10:19:13AM +0100, Thomas Zimmermann wrote: > Am 21.02.25 um 16:51 schrieb [email protected]: > > On Fri, Feb 21, 2025 at 11:36:00AM +, Aditya Garg wrote: ... > > > + for (x = 0; x < pixels; x++) { > > > + pix = le32_to_cpu(sbuf32[x]); > > > + /* write red-green-blue to output in little endianness */ > > > + *dbuf8++ = (pix & 0x00ff) >> 16; > > > + *dbuf8++ = (pix & 0xff00) >> 8; > > > + *dbuf8++ = (pix & 0x00ff) >> 0; > > put_unaligned_be24() > > I'm all for sharing helper code, but maybe not here. > > - DRM pixel formats are always little endian. > - CPU encoding is LE or BE. > - Pixel-component order can be anything: RGB/BGR/etc. > > So the code has a comment to explain what happens here. Adding that call > with the _be24 postfix into the mix would make it more confusing. I'm not objecting the comment, the code has definite meaning and with the proposed helper it makes clearer (in my opinion). Comment can be adjusted to explain better this conversion. Or, perhaps pix should be be32? That's probably where confusion starts. pix = be32_to_cpu(sbuf32[x]); /* write red-green-blue to output in little endianness */ put_unaligned_le24(...); ? > > > + } ... > > > + static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = { > > > + 3, > > > + }; > > One line? > > > > static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = { 3 }; > > I'd be ok, if there's a string preference in the kernel to use thins style. Just a common sense to avoid more LoCs when it's not needed / redundant. > Most of DRM doesn't AFAIK. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/3] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc
On Mon, Feb 24, 2025 at 10:32:27AM +, Aditya Garg wrote: > > On 24 Feb 2025, at 3:54 PM, [email protected] wrote: > > On Mon, Feb 24, 2025 at 10:18:48AM +, Aditya Garg wrote: > >>>> On 24 Feb 2025, at 3:28 PM, [email protected] wrote: > >>> On Sat, Feb 22, 2025 at 03:46:03PM +, Aditya Garg wrote: > >>>>>> On 20 Feb 2025, at 10:09 PM, Aditya Garg wrote: ... > >>>>> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but > >>>>> it's useful to be able to print generic 4-character codes formatted as > >>>>> an integer. Extend it to add format specifiers for printing generic > >>>>> 32-bit FOURCCs with various endian semantics: > >>>>> > >>>>> %p4ch Host-endian > >>>>> %p4cl Little-endian > >>>>> %p4cb Big-endian > >>>>> %p4cr Reverse-endian > >>>>> > >>>>> The endianness determines how bytes are interpreted as a u32, and the > >>>>> FOURCC is then always printed MSByte-first (this is the opposite of > >>>>> V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would > >>>>> allow printing LSByte-first FOURCCs stored in host endian order > >>>>> (other than the hex form being in character order, not the integer > >>>>> value). > >>> > >>> ... > >>> > >>>> BTW, after looking at the comments by Martin [1], its actually better to > >>>> use > >>>> existing specifiers for the appletbdrm driver. The driver needs the host > >>>> endian as proposed by this patch, so instead of that, we can use %.4s > >>> > >>> Do you mean this patch will not be needed? If this a case, that would be > >>> the > >>> best solution. > >> > >> I tested with %4pE, and the results are different from expected. So this > >> would be preferred. Kindly see my latest email with a proposed workaround > >> for > >> the sparse warnings. > > > > %.4s sounded okay, but %4pE is always about escaping and the result may > > occupy > > %4x memory (octal escaping of non-printable characters). Of course, you may > > vary > > the escaping classes, but IIRC the octal or hex escaping is unconditional. > > %.4s is used for unsigned int iirc, here it's __le32. No, it's used to 'char *'. in case one may guarantee that it at least is four characters long. > >>>> [1]: > >>>> https://lore.kernel.org/asahi/[email protected]/ > >>>> > >>>> Alternatively we could add a host endian only. Other endians are not > >>>> really > >>>> used by any driver AFAIK. The host endian is being used by appletbdrm and > >>>> Asahi Linux’ SMC driver only. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/3] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc
On Sun, Feb 23, 2025 at 03:16:28PM +, Aditya Garg wrote: > > Looking at the header files, it looks like doing cpu_to_le32 on that > > variable and doing le32_to_cpu will actually reverse the order twice, on > > big endian systems, thus technically all way would not swap the order at > > all. > > > > I'm not really sure how to manage the sparse warnings here. > > Not sure whether the maintainers would like it, but we can do something like > this: This is not what we want, I believe. And this looks like a reinventing a wheel of cpu_to_*() and *_to_cpu() or similar macros. > case 'l’: > #ifdef __LITTLE_ENDIAN > val = orig; > #else > orig = swab32(orig); > val = orig; > #endif > break; > > case 'b’: > #ifdef __LITTLE_ENDIAN > orig = swab32(orig); > val = orig; > #else > val = orig; > #endif > break; -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Tue, Feb 25, 2025 at 02:53:15PM +0100, Thomas Zimmermann wrote: > Am 25.02.25 um 14:27 schrieb [email protected]: > > On Tue, Feb 25, 2025 at 12:59:43PM +0100, Thomas Zimmermann wrote: > > > Am 25.02.25 um 12:01 schrieb [email protected]: > > > > On Tue, Feb 25, 2025 at 10:48:53AM +, Aditya Garg wrote: > > > > > > On 25 Feb 2025, at 4:17 PM, [email protected] wrote: > > > > > > On Tue, Feb 25, 2025 at 10:36:03AM +, Aditya Garg wrote: > > > > > > > > > On 25 Feb 2025, at 4:03 PM, [email protected] > > > > > > > > > wrote: > > > > > > > > On Tue, Feb 25, 2025 at 10:09:42AM +, Aditya Garg wrote: ... > > > > > > > > > +static int appletbdrm_probe(struct usb_interface *intf, > > > > > > > > > +const struct usb_device_id *id) > > > > > > > > > +{ > > > > > > > > > +struct usb_endpoint_descriptor *bulk_in, *bulk_out; > > > > > > > > > +struct device *dev = &intf->dev; > > > > > > > > > +struct appletbdrm_device *adev; > > > > > > > > > +struct drm_device *drm; > > > > > > > > > +int ret; > > > > > > > > > + > > > > > > > > > +ret = usb_find_common_endpoints(intf->cur_altsetting, > > > > > > > > > &bulk_in, &bulk_out, NULL, NULL); > > > > > > > > > +if (ret) { > > > > > > > > > +drm_err(drm, "Failed to find bulk endpoints\n"); > > > > > > > > This is simply wrong (and in this case even lead to crash in > > > > > > > > some circumstances). > > > > > > > > drm_err() may not be used here. That's my point in previous > > > > > > > > discussions. > > > > > > > > Independently on the subsystem the ->probe() for the sake of > > > > > > > > consistency and > > > > > > > > being informative should only rely on struct device *dev, > > > > > > > I'm not sure how drm_err works, > > > > > > It's a macro. > > > > > > > > > > > > > but struct drm_device does have a struct device *dev as well. > > > > > > Yes, but only when it's initialized. > > > > > > > > > > > > > Anyways, this is something I'll leave for Thomas to reply. > > > > > > The code above is wrong independently on his reply :-) > > > > > I'm kinda stuck between contrasting views of 2 kernel maintainers lol, > > > > > so I said let Thomas reply. > > > > Sure. I also want him to clarify my question about potential > > > > drm_err_probe(). > > > These threads get a little lengthy. What is the question? > > How drm_err_probe() can be (consistently) implemented as there are and will > > be > > cases when we want to return an error code with the message and having DRM > > devce > > not being available, please? > > The DRM logging works with a DRM device pointer of NULL. It'll simply leave > out device infos. Right and that's what makes it less informative than pure dev_*() macros. For the probe it should really take the struct device instead of struct drm in my opinion. Otherwise we may end up with the code like above, which has hidden bugs. > > Also, drm_err() has a downside of not checking for deferred probe and > > potentially leads to the noisy log floods. > > I think it should be possible to export __dev_probe_failed() [1] from the > core and write drm_err_probe() and drm_warn_probe() around this. Yep, we can do that > The output then looks like a DRM logging, but behaves like dev-based logging. > Note that DRM logging already is an elaborate wrapper around the dev-based > logging, so it will be more of the same. Okay, this sounds promising. My only worries are the possibilities of misuse of the API and/or leaving it non- or less informative (in comparison to the existing helpers). > [1] https://elixir.bootlin.com/linux/v6.13.4/source/drivers/base/core.c#L5008 -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Tue, Feb 25, 2025 at 10:48:53AM +, Aditya Garg wrote: > > On 25 Feb 2025, at 4:17 PM, [email protected] wrote: > > On Tue, Feb 25, 2025 at 10:36:03AM +, Aditya Garg wrote: > >>>> On 25 Feb 2025, at 4:03 PM, [email protected] wrote: > >>> On Tue, Feb 25, 2025 at 10:09:42AM +, Aditya Garg wrote: ... > >>>> +static int appletbdrm_probe(struct usb_interface *intf, > >>>> +const struct usb_device_id *id) > >>>> +{ > >>>> +struct usb_endpoint_descriptor *bulk_in, *bulk_out; > >>>> +struct device *dev = &intf->dev; > >>>> +struct appletbdrm_device *adev; > >>>> +struct drm_device *drm; > >>>> +int ret; > >>>> + > >>>> +ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, > >>>> &bulk_out, NULL, NULL); > >>>> +if (ret) { > >>>> +drm_err(drm, "Failed to find bulk endpoints\n"); > >>> > >>> This is simply wrong (and in this case even lead to crash in some > >>> circumstances). > >>> drm_err() may not be used here. That's my point in previous discussions. > >>> Independently on the subsystem the ->probe() for the sake of consistency > >>> and > >>> being informative should only rely on struct device *dev, > >> > >> I'm not sure how drm_err works, > > > > It's a macro. > > > >> but struct drm_device does have a struct device *dev as well. > > > > Yes, but only when it's initialized. > > > >> Anyways, this is something I'll leave for Thomas to reply. > > > > The code above is wrong independently on his reply :-) > > I'm kinda stuck between contrasting views of 2 kernel maintainers lol, > so I said let Thomas reply. Sure. I also want him to clarify my question about potential drm_err_probe(). -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Tue, Feb 25, 2025 at 12:58:17PM +0100, Thomas Zimmermann wrote: > Am 25.02.25 um 11:33 schrieb [email protected]: > > On Tue, Feb 25, 2025 at 10:09:42AM +, Aditya Garg wrote: ... > > > +static int appletbdrm_probe(struct usb_interface *intf, > > > + const struct usb_device_id *id) > > > +{ > > > + struct usb_endpoint_descriptor *bulk_in, *bulk_out; > > > + struct device *dev = &intf->dev; > > > + struct appletbdrm_device *adev; > > > + struct drm_device *drm; > > > + int ret; > > > + > > > + ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, > > > &bulk_out, NULL, NULL); > > > + if (ret) { > > > + drm_err(drm, "Failed to find bulk endpoints\n"); > > This is simply wrong (and in this case even lead to crash in some > > circumstances). > > drm_err() may not be used here. That's my point in previous discussions. > > Independently on the subsystem the ->probe() for the sake of consistency and > > being informative should only rely on struct device *dev, > > That's never going to work with DRM. There's so much code in a DRM probe > function that uses the DRM error functions. > This specific instance here is wrong, as the drm pointer hasn't been > initialized. But as soon as it is, it's much better to use drm_err() and > friends. It will do the right thing and give consistent output across > drivers. Okay and my question was how is it possible to create drm_err_probe() for such cases? > > > + return ret; > > > + } -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
On Tue, Feb 25, 2025 at 12:59:43PM +0100, Thomas Zimmermann wrote: > Am 25.02.25 um 12:01 schrieb [email protected]: > > On Tue, Feb 25, 2025 at 10:48:53AM +, Aditya Garg wrote: > > > > On 25 Feb 2025, at 4:17 PM, [email protected] wrote: > > > > On Tue, Feb 25, 2025 at 10:36:03AM +, Aditya Garg wrote: > > > > > > > On 25 Feb 2025, at 4:03 PM, [email protected] > > > > > > > wrote: > > > > > > On Tue, Feb 25, 2025 at 10:09:42AM +, Aditya Garg wrote: ... > > > > > > > +static int appletbdrm_probe(struct usb_interface *intf, > > > > > > > +const struct usb_device_id *id) > > > > > > > +{ > > > > > > > +struct usb_endpoint_descriptor *bulk_in, *bulk_out; > > > > > > > +struct device *dev = &intf->dev; > > > > > > > +struct appletbdrm_device *adev; > > > > > > > +struct drm_device *drm; > > > > > > > +int ret; > > > > > > > + > > > > > > > +ret = usb_find_common_endpoints(intf->cur_altsetting, > > > > > > > &bulk_in, &bulk_out, NULL, NULL); > > > > > > > +if (ret) { > > > > > > > +drm_err(drm, "Failed to find bulk endpoints\n"); > > > > > > This is simply wrong (and in this case even lead to crash in some > > > > > > circumstances). > > > > > > drm_err() may not be used here. That's my point in previous > > > > > > discussions. > > > > > > Independently on the subsystem the ->probe() for the sake of > > > > > > consistency and > > > > > > being informative should only rely on struct device *dev, > > > > > I'm not sure how drm_err works, > > > > It's a macro. > > > > > > > > > but struct drm_device does have a struct device *dev as well. > > > > Yes, but only when it's initialized. > > > > > > > > > Anyways, this is something I'll leave for Thomas to reply. > > > > The code above is wrong independently on his reply :-) > > > I'm kinda stuck between contrasting views of 2 kernel maintainers lol, > > > so I said let Thomas reply. > > Sure. I also want him to clarify my question about potential > > drm_err_probe(). > > These threads get a little lengthy. What is the question? How drm_err_probe() can be (consistently) implemented as there are and will be cases when we want to return an error code with the message and having DRM devce not being available, please? Also, drm_err() has a downside of not checking for deferred probe and potentially leads to the noisy log floods. -- With Best Regards, Andy Shevchenko
