/snip
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index d16158deacdc..f3587a54b8ac 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -141,6 +141,56 @@ struct drm_panel { */ const struct drm_panel_funcs *funcs;
All these new added members seems dupliated with drm_display_info, So I think, can we add a new drm_plane_funcs func:
int (*set_display_info)(struct drm_panel *panel, struct drm_display_info *info);
I don't disagree personally, since I originally wrote it this way, but 2 maintainers, Daniel Vetter and Thierry Reding, requested that it be changed. Unless that decision is reversed, I won't be changing this.
Reading back the feedback on v1, I don't think anyone said they were against storing a drm_display_info struct in drm_panel (no one really weighed in on it one way or another). IMO duplicating a bunch of fields feels pretty icky.
Thanks for the review. Should we duplicate the entire struct, or just create a sub-struct, say, physical_properties that will be part of drm_display_info and drm_panel?
That's a good idea, but I think you can use the entire struct. Everything has reasonable default values and I think it might be difficult to draw a line in the sand as to which fields will or won't be useful for panels going forward. Best for drivers to just treat the info in drm_display_info as best effort and have good defaults IMO.
Sean
I think most fields in drm_display_info have pretty reasonable defaults, so I'd recommend just storing a copy in drm_panel. As Thierry suggests, we can populate the dt parts in probe (orientation) and then copy over all or a subset of the struct to connector on attach.
Sean
Then in drm_panel_attach(), via this interface the specific panel driver can directly set connector->display_info. like
... if (panel->funcs && panel->funcs->set_display_info) panel->funcs->unprepare(panel, connector->display_info); ...
Thanks James
/*** @width_mm:** Physical width in mm.*/unsigned int width_mm;/*** @height_mm:** Physical height in mm.*/unsigned int height_mm;/*** @bpc:** Maximum bits per color channel. Used by HDMI and DP outputs.*/unsigned int bpc;/*** @orientation** Installation orientation of the panel with respect to the chassis.*/int orientation;/*** @bus_formats** Pixel data format on the wire.*/const u32 *bus_formats;/*** @num_bus_formats:** Number of elements pointed to by @bus_formats*/unsigned int num_bus_formats;/*** @bus_flags:** Additional information (like pixel signal polarity) for the pixel* data on the bus.*/u32 bus_flags;/** * @list: *Thanks for the review
-- Sean Paul, Software Engineer, Google / Chromium OS