Just a buch of drive-by comments while browsing the code. In general code looks good, especialyl for a v1.
There is a few places that triggers warnings with checkpatch --strict Most looks like things that should be fixed.
Thanks Sam for the review. Will take care of the suggestions in next iteration.
Response inlined below:
+struct pipe_msg_hdr {
- u32 type;
- u32 size; /* size of message after this field */
+} __packed;
+struct hvd_screen_info {
- u16 width;
- u16 height;
+} __packed;
+struct synthvid_msg_hdr {
- u32 type;
- u32 size; /* size of this header + payload after this field*/
Add space before closing "*/"
I wonder what is the difference between what is considered a message and what is considered payload in the above comments. Maybe that just because I do not know this stuff at all and the comment can be ignored.
message = struct pipe_msg_hdr + struct synthvid_msg_hdr + payload
Will try to make it more clear.
+} __packed;
+struct synthvid_version_req {
- u32 version;
+} __packed;
+struct synthvid_version_resp {
- u32 version;
- u8 is_accepted;
- u8 max_video_outputs;
+} __packed;
+struct synthvid_vram_location {
- u64 user_ctx;
- u8 is_vram_gpa_specified;
- u64 vram_gpa;
+} __packed;
Not an alignmnet friendly layout - but I guess the layout is fixed. Same goes in otther places.
Yes nothing can be done for this.
+static int synthvid_update_situation(struct hv_device *hdev, u8 active, u32 bpp,
u32 w, u32 h, u32 pitch)+{
- struct synthvid_msg msg;
Sometimes synthvid_msg is hv->init_buf. Sometimes a local variable. I wonder why there is a difference.
When a reply is expected, hv->init_buf should be used, though I haven't verified this. Just kept the same logic as in framebuffer driver.