| Message ID | 20211122065223.88059-5-jagan@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Mon, Nov 22, 2021 at 12:22:20PM +0530, Jagan Teki wrote: > Get the display mode settings via mode_set bridge function > instead of explicitly de-reference. What's wrong with dereferencing the mode? Maxime
On Mon, Nov 22, 2021 at 3:38 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Mon, Nov 22, 2021 at 12:22:20PM +0530, Jagan Teki wrote: > > Get the display mode settings via mode_set bridge function > > instead of explicitly de-reference. > > What's wrong with dereferencing the mode? Nothing wrong with dereferencing, however we have built-in API to that job. Jagan.
On Mon, Nov 22, 2021 at 06:35:58PM +0530, Jagan Teki wrote: > On Mon, Nov 22, 2021 at 3:38 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Mon, Nov 22, 2021 at 12:22:20PM +0530, Jagan Teki wrote: > > > Get the display mode settings via mode_set bridge function > > > instead of explicitly de-reference. > > > > What's wrong with dereferencing the mode? > > Nothing wrong with dereferencing, however we have built-in API to that job. That's not an API though? It's perfectly valid to dereference the pointer in atomic_enable, and that patch would consume memory for no particular reason. Maxime
Hi Maxime, On Mon, Nov 22, 2021 at 6:58 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Mon, Nov 22, 2021 at 06:35:58PM +0530, Jagan Teki wrote: > > On Mon, Nov 22, 2021 at 3:38 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Mon, Nov 22, 2021 at 12:22:20PM +0530, Jagan Teki wrote: > > > > Get the display mode settings via mode_set bridge function > > > > instead of explicitly de-reference. > > > > > > What's wrong with dereferencing the mode? > > > > Nothing wrong with dereferencing, however we have built-in API to that job. > > That's not an API though? May be we can call it bridge or encoding function, I usually call these ops are API's. > > It's perfectly valid to dereference the pointer in atomic_enable, and > that patch would consume memory for no particular reason. Again, I'm not pointing any mistake in dereference and certainly not understand about what memory consumption issue here. I'm doing it here since I'm doing it via mode_set in other drivers. No problem for me either way. Thanks, Jagan.
On Mon, Nov 22, 2021 at 07:21:57PM +0530, Jagan Teki wrote: > > It's perfectly valid to dereference the pointer in atomic_enable, and > > that patch would consume memory for no particular reason. > > Again, I'm not pointing any mistake in dereference and certainly not > understand about what memory consumption issue here. You add a struct drm_display_mode field to struct sun6i_dsi. It increases the size of struct sun6i_dsi of sizeof(struct drm_display_mode). > I'm doing it here since I'm doing it via mode_set in other drivers. No > problem for me either way. But *why* are you doing so? There might be a valid reason in other drivers, but there's none here (that you mentioned at least). Maxime
On Mon, Nov 22, 2021 at 7:39 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Mon, Nov 22, 2021 at 07:21:57PM +0530, Jagan Teki wrote: > > > It's perfectly valid to dereference the pointer in atomic_enable, and > > > that patch would consume memory for no particular reason. > > > > Again, I'm not pointing any mistake in dereference and certainly not > > understand about what memory consumption issue here. > > You add a struct drm_display_mode field to struct sun6i_dsi. It > increases the size of struct sun6i_dsi of sizeof(struct > drm_display_mode). > > > I'm doing it here since I'm doing it via mode_set in other drivers. No > > problem for me either way. > > But *why* are you doing so? > > There might be a valid reason in other drivers, but there's none here > (that you mentioned at least). The reason is to use existing bridge function instead of dereference ie what I've mentioned. I don't have any other reasons. Jagan.
On Mon, Nov 22, 2021 at 08:01:47PM +0530, Jagan Teki wrote: > On Mon, Nov 22, 2021 at 7:39 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Mon, Nov 22, 2021 at 07:21:57PM +0530, Jagan Teki wrote: > > > > It's perfectly valid to dereference the pointer in atomic_enable, and > > > > that patch would consume memory for no particular reason. > > > > > > Again, I'm not pointing any mistake in dereference and certainly not > > > understand about what memory consumption issue here. > > > > You add a struct drm_display_mode field to struct sun6i_dsi. It > > increases the size of struct sun6i_dsi of sizeof(struct > > drm_display_mode). > > > > > I'm doing it here since I'm doing it via mode_set in other drivers. No > > > problem for me either way. > > > > But *why* are you doing so? > > > > There might be a valid reason in other drivers, but there's none here > > (that you mentioned at least). > > The reason is to use existing bridge function instead of dereference > ie what I've mentioned. I don't have any other reasons. This discussion is going in circles. Unless you have a reason other than "because we can", NAK for the reasons already stated above. Maxime
On Mon, Nov 22, 2021 at 8:36 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Mon, Nov 22, 2021 at 08:01:47PM +0530, Jagan Teki wrote: > > On Mon, Nov 22, 2021 at 7:39 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Mon, Nov 22, 2021 at 07:21:57PM +0530, Jagan Teki wrote: > > > > > It's perfectly valid to dereference the pointer in atomic_enable, and > > > > > that patch would consume memory for no particular reason. > > > > > > > > Again, I'm not pointing any mistake in dereference and certainly not > > > > understand about what memory consumption issue here. > > > > > > You add a struct drm_display_mode field to struct sun6i_dsi. It > > > increases the size of struct sun6i_dsi of sizeof(struct > > > drm_display_mode). > > > > > > > I'm doing it here since I'm doing it via mode_set in other drivers. No > > > > problem for me either way. > > > > > > But *why* are you doing so? > > > > > > There might be a valid reason in other drivers, but there's none here > > > (that you mentioned at least). > > > > The reason is to use existing bridge function instead of dereference > > ie what I've mentioned. I don't have any other reasons. > > This discussion is going in circles. Unless you have a reason other than > "because we can", NAK for the reasons already stated above. Agreed your point. Thanks, Jagan.
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index a6a272b55f77..731af31e2bde 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -718,7 +718,7 @@ static void sun6i_dsi_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge); - struct drm_display_mode *mode = &bridge->encoder->crtc->state->adjusted_mode; + struct drm_display_mode *mode = &dsi->mode; struct mipi_dsi_device *device = dsi->device; union phy_configure_opts opts = { }; struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; @@ -854,6 +854,15 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, }; +static void sun6i_dsi_bridge_mode_set(struct drm_bridge *bridge, + const struct drm_display_mode *mode, + const struct drm_display_mode *adjusted_mode) +{ + struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge); + + drm_mode_copy(&dsi->mode, adjusted_mode); +} + static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { @@ -872,6 +881,7 @@ static const struct drm_bridge_funcs sun6i_dsi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset, .atomic_enable = sun6i_dsi_bridge_atomic_enable, .atomic_disable = sun6i_dsi_bridge_atomic_disable, + .mode_set = sun6i_dsi_bridge_mode_set, .attach = sun6i_dsi_bridge_attach, }; diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h index d269304691c9..acdd586a4157 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h @@ -20,6 +20,7 @@ struct sun6i_dsi { struct drm_connector connector; struct drm_encoder encoder; struct mipi_dsi_host host; + struct drm_display_mode mode; struct clk *bus_clk; struct clk *mod_clk;
Get the display mode settings via mode_set bridge function instead of explicitly de-reference. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- Changes for v5: - new patch drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 12 +++++++++++- drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 1 + 2 files changed, 12 insertions(+), 1 deletion(-)