diff mbox series

[v5,4/7] drm: sun4i: dsi: Add mode_set function

Message ID 20211122065223.88059-5-jagan@amarulasolutions.com
State New
Headers
Series drm: sun4i: dsi: Convert drm bridge |

Commit Message

Jagan Teki Nov. 22, 2021, 6:52 a.m. UTC 
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(-)

Comments

Maxime Ripard Nov. 22, 2021, 10:07 a.m. UTC | #1 
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
Jagan Teki Nov. 22, 2021, 1:05 p.m. UTC | #2 
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.
Maxime Ripard Nov. 22, 2021, 1:28 p.m. UTC | #3 
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
Jagan Teki Nov. 22, 2021, 1:51 p.m. UTC | #4 
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.
Maxime Ripard Nov. 22, 2021, 2:09 p.m. UTC | #5 
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
Jagan Teki Nov. 22, 2021, 2:31 p.m. UTC | #6 
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.
Maxime Ripard Nov. 22, 2021, 3:06 p.m. UTC | #7 
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
Jagan Teki Nov. 22, 2021, 3:17 p.m. UTC | #8 
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 mbox series

Patch

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;