[RFC PATCH 1/2] drm/hyperv: Add DRM driver for hyperv synthetic video device

[Deepak Rawat]

> 
> 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.