> On 24 Feb 2025, at 8:27 PM, andriy.shevchenko@xxxxxxxxxxxxxxx wrote:
>
> On Mon, Feb 24, 2025 at 02:32:37PM +0000, Aditya Garg wrote:
>>> On 24 Feb 2025, at 7:30 PM, andriy.shevchenko@xxxxxxxxxxxxxxx wrote:
>>> On Mon, Feb 24, 2025 at 01:40:20PM +0000, 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.
> ...
>
> (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)?
Ok
>
> ...
>
>>>> + /*
>>>> + * 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.
Thomas asked me to do this change. Maybe you didn’t see his reply.
>
>>>> + */
>
> ...
>
>> 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
>
>
