diff mbox series

[RFC,4/7] drm/i915/dsi: Helper to find dsi encoder in cmd mode

Message ID 20191014110122.31923-5-vandita.kulkarni@intel.com (mailing list archive)
State New, archived
Headers show
Series Add mipi dsi command mode support. | expand

Commit Message

Kulkarni, Vandita Oct. 14, 2019, 11:01 a.m. UTC 
From: Madhav Chauhan <madhav.chauhan@intel.com>

This patch adds a helper function to find encoder
if DSI is operating in command mode. This function
will be used while enabling/disabling TE interrupts
for DSI.

Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
---
 drivers/gpu/drm/i915/display/icl_dsi.c   | 17 +++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dsi.h |  3 +++
 2 files changed, 20 insertions(+)

Comments

Jani Nikula Oct. 15, 2019, 7:20 p.m. UTC | #1 
On Mon, 14 Oct 2019, Vandita Kulkarni <vandita.kulkarni@intel.com> wrote:
> From: Madhav Chauhan <madhav.chauhan@intel.com>
>
> This patch adds a helper function to find encoder
> if DSI is operating in command mode. This function
> will be used while enabling/disabling TE interrupts
> for DSI.
>
> Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> ---
>  drivers/gpu/drm/i915/display/icl_dsi.c   | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dsi.h |  3 +++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> index 5dd9eebab6b1..877746416e52 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -73,6 +73,23 @@ static enum transcoder dsi_port_to_transcoder(enum port port)
>  		return TRANSCODER_DSI_1;
>  }
>  
> +struct intel_encoder *gen11_dsi_find_cmd_mode_encoder(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct intel_encoder *encoder;
> +	struct intel_dsi *intel_dsi;
> +
> +	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
> +		if (encoder->type != INTEL_OUTPUT_DSI)
> +			continue;
> +		intel_dsi = enc_to_intel_dsi(&encoder->base);
> +		if (intel_dsi->operation_mode == INTEL_DSI_COMMAND_MODE)
> +			return encoder;
> +	}
> +
> +	return NULL;
> +}

This may be a bit harsh, but everything that feels wrong about the
following patches pretty much boils down to this function. It may get
the job done, and I don't have a better suggestion on how to accomplish
this right now. But it seems like we shouldn't have to do anything like
this, and makes you feel like there's something wrong with the design.

It would be great to be able to handle this using crtc state, but alas
the vblank enable hook only gets passed a struct drm_crtc *. (Patch 7
could easily switch to using crtc state, but need to also solve patch
5.)

I'll get back to you on this later, but in the mean time - Ville, do you
have any ideas?


BR,
Jani.


> +
>  static void wait_for_cmds_dispatched_to_panel(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h b/drivers/gpu/drm/i915/display/intel_dsi.h
> index b15be5814599..071dad7ee04a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsi.h
> @@ -201,6 +201,9 @@ u32 bxt_dsi_get_pclk(struct intel_encoder *encoder,
>  		     struct intel_crtc_state *config);
>  void bxt_dsi_reset_clocks(struct intel_encoder *encoder, enum port port);
>  
> +/* icl_dsi.c */
> +struct intel_encoder *gen11_dsi_find_cmd_mode_encoder(struct intel_crtc *crtc);
> +
>  /* intel_dsi_vbt.c */
>  bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id);
>  void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,
Kulkarni, Vandita Oct. 16, 2019, 1:27 p.m. UTC | #2 
> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Wednesday, October 16, 2019 12:51 AM
> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com; Shankar, Uma <uma.shankar@intel.com>;
> Chauhan, Madhav <madhav.chauhan@intel.com>; Kulkarni, Vandita
> <vandita.kulkarni@intel.com>
> Subject: Re: [RFC 4/7] drm/i915/dsi: Helper to find dsi encoder in cmd mode
> 
> On Mon, 14 Oct 2019, Vandita Kulkarni <vandita.kulkarni@intel.com> wrote:
> > From: Madhav Chauhan <madhav.chauhan@intel.com>
> >
> > This patch adds a helper function to find encoder if DSI is operating
> > in command mode. This function will be used while enabling/disabling
> > TE interrupts for DSI.
> >
> > Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/icl_dsi.c   | 17 +++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dsi.h |  3 +++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> > b/drivers/gpu/drm/i915/display/icl_dsi.c
> > index 5dd9eebab6b1..877746416e52 100644
> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> > @@ -73,6 +73,23 @@ static enum transcoder
> dsi_port_to_transcoder(enum port port)
> >  		return TRANSCODER_DSI_1;
> >  }
> >
> > +struct intel_encoder *gen11_dsi_find_cmd_mode_encoder(struct
> > +intel_crtc *crtc) {
> > +	struct drm_device *dev = crtc->base.dev;
> > +	struct intel_encoder *encoder;
> > +	struct intel_dsi *intel_dsi;
> > +
> > +	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
> > +		if (encoder->type != INTEL_OUTPUT_DSI)
> > +			continue;
> > +		intel_dsi = enc_to_intel_dsi(&encoder->base);
> > +		if (intel_dsi->operation_mode ==
> INTEL_DSI_COMMAND_MODE)
> > +			return encoder;
> > +	}
> > +
> > +	return NULL;
> > +}
> 
> This may be a bit harsh, but everything that feels wrong about the following
> patches pretty much boils down to this function. It may get the job done,
> and I don't have a better suggestion on how to accomplish this right now.
> But it seems like we shouldn't have to do anything like this, and makes you
> feel like there's something wrong with the design.
> 
> It would be great to be able to handle this using crtc state, but alas the
> vblank enable hook only gets passed a struct drm_crtc *. (Patch 7 could
> easily switch to using crtc state, but need to also solve patch
> 5.)
.
> 
> I'll get back to you on this later, but in the mean time - Ville, do you have
> any ideas?

Other option of adding in crtc_state, did you mean something like this
https://patchwork.freedesktop.org/patch/336178/ 
@Jani and @Ville  Please let me know your comments.

Thanks,
Vandita
> 
> 
> BR,
> Jani.
> 
> 
> > +
> >  static void wait_for_cmds_dispatched_to_panel(struct intel_encoder
> > *encoder)  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); diff
> > --git a/drivers/gpu/drm/i915/display/intel_dsi.h
> > b/drivers/gpu/drm/i915/display/intel_dsi.h
> > index b15be5814599..071dad7ee04a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsi.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dsi.h
> > @@ -201,6 +201,9 @@ u32 bxt_dsi_get_pclk(struct intel_encoder
> *encoder,
> >  		     struct intel_crtc_state *config);  void
> > bxt_dsi_reset_clocks(struct intel_encoder *encoder, enum port port);
> >
> > +/* icl_dsi.c */
> > +struct intel_encoder *gen11_dsi_find_cmd_mode_encoder(struct
> > +intel_crtc *crtc);
> > +
> >  /* intel_dsi_vbt.c */
> >  bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id);
> > void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,
> 
> --
> Jani Nikula, Intel Open Source Graphics Center
Jani Nikula Oct. 24, 2019, 9:07 a.m. UTC | #3 
On Wed, 16 Oct 2019, "Kulkarni, Vandita" <vandita.kulkarni@intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani <jani.nikula@intel.com>
>> Sent: Wednesday, October 16, 2019 12:51 AM
>> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; intel-
>> gfx@lists.freedesktop.org
>> Cc: ville.syrjala@linux.intel.com; Shankar, Uma <uma.shankar@intel.com>;
>> Chauhan, Madhav <madhav.chauhan@intel.com>; Kulkarni, Vandita
>> <vandita.kulkarni@intel.com>
>> Subject: Re: [RFC 4/7] drm/i915/dsi: Helper to find dsi encoder in cmd mode
>> 
>> On Mon, 14 Oct 2019, Vandita Kulkarni <vandita.kulkarni@intel.com> wrote:
>> > From: Madhav Chauhan <madhav.chauhan@intel.com>
>> >
>> > This patch adds a helper function to find encoder if DSI is operating
>> > in command mode. This function will be used while enabling/disabling
>> > TE interrupts for DSI.
>> >
>> > Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
>> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/icl_dsi.c   | 17 +++++++++++++++++
>> >  drivers/gpu/drm/i915/display/intel_dsi.h |  3 +++
>> >  2 files changed, 20 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
>> > b/drivers/gpu/drm/i915/display/icl_dsi.c
>> > index 5dd9eebab6b1..877746416e52 100644
>> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
>> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
>> > @@ -73,6 +73,23 @@ static enum transcoder
>> dsi_port_to_transcoder(enum port port)
>> >  		return TRANSCODER_DSI_1;
>> >  }
>> >
>> > +struct intel_encoder *gen11_dsi_find_cmd_mode_encoder(struct
>> > +intel_crtc *crtc) {
>> > +	struct drm_device *dev = crtc->base.dev;
>> > +	struct intel_encoder *encoder;
>> > +	struct intel_dsi *intel_dsi;
>> > +
>> > +	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
>> > +		if (encoder->type != INTEL_OUTPUT_DSI)
>> > +			continue;
>> > +		intel_dsi = enc_to_intel_dsi(&encoder->base);
>> > +		if (intel_dsi->operation_mode ==
>> INTEL_DSI_COMMAND_MODE)
>> > +			return encoder;
>> > +	}
>> > +
>> > +	return NULL;
>> > +}
>> 
>> This may be a bit harsh, but everything that feels wrong about the following
>> patches pretty much boils down to this function. It may get the job done,
>> and I don't have a better suggestion on how to accomplish this right now.
>> But it seems like we shouldn't have to do anything like this, and makes you
>> feel like there's something wrong with the design.
>> 
>> It would be great to be able to handle this using crtc state, but alas the
>> vblank enable hook only gets passed a struct drm_crtc *. (Patch 7 could
>> easily switch to using crtc state, but need to also solve patch
>> 5.)
> .
>> 
>> I'll get back to you on this later, but in the mean time - Ville, do you have
>> any ideas?
>
> Other option of adding in crtc_state, did you mean something like this
> https://patchwork.freedesktop.org/patch/336178/ 
> @Jani and @Ville  Please let me know your comments.

The problem is that we don't have access to the crtc state in patch 5
gen11_dsi_configure_te() because the call path is via drm_crtc_funcs and
bdw_enable_vblank.

BR,
Jani.


>
> Thanks,
> Vandita
>> 
>> 
>> BR,
>> Jani.
>> 
>> 
>> > +
>> >  static void wait_for_cmds_dispatched_to_panel(struct intel_encoder
>> > *encoder)  {
>> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); diff
>> > --git a/drivers/gpu/drm/i915/display/intel_dsi.h
>> > b/drivers/gpu/drm/i915/display/intel_dsi.h
>> > index b15be5814599..071dad7ee04a 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dsi.h
>> > +++ b/drivers/gpu/drm/i915/display/intel_dsi.h
>> > @@ -201,6 +201,9 @@ u32 bxt_dsi_get_pclk(struct intel_encoder
>> *encoder,
>> >  		     struct intel_crtc_state *config);  void
>> > bxt_dsi_reset_clocks(struct intel_encoder *encoder, enum port port);
>> >
>> > +/* icl_dsi.c */
>> > +struct intel_encoder *gen11_dsi_find_cmd_mode_encoder(struct
>> > +intel_crtc *crtc);
>> > +
>> >  /* intel_dsi_vbt.c */
>> >  bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id);
>> > void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center
Jani Nikula Oct. 24, 2019, 9:11 a.m. UTC | #4 
On Thu, 24 Oct 2019, Jani Nikula <jani.nikula@intel.com> wrote:
> On Wed, 16 Oct 2019, "Kulkarni, Vandita" <vandita.kulkarni@intel.com> wrote:
>>> -----Original Message-----
>>> From: Nikula, Jani <jani.nikula@intel.com>
>>> Sent: Wednesday, October 16, 2019 12:51 AM
>>> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; intel-
>>> gfx@lists.freedesktop.org
>>> Cc: ville.syrjala@linux.intel.com; Shankar, Uma <uma.shankar@intel.com>;
>>> Chauhan, Madhav <madhav.chauhan@intel.com>; Kulkarni, Vandita
>>> <vandita.kulkarni@intel.com>
>>> Subject: Re: [RFC 4/7] drm/i915/dsi: Helper to find dsi encoder in cmd mode
>>> 
>>> On Mon, 14 Oct 2019, Vandita Kulkarni <vandita.kulkarni@intel.com> wrote:
>>> > From: Madhav Chauhan <madhav.chauhan@intel.com>
>>> >
>>> > This patch adds a helper function to find encoder if DSI is operating
>>> > in command mode. This function will be used while enabling/disabling
>>> > TE interrupts for DSI.
>>> >
>>> > Signed-off-by: Madhav Chauhan <madhav.chauhan@intel.com>
>>> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni@intel.com>
>>> > ---
>>> >  drivers/gpu/drm/i915/display/icl_dsi.c   | 17 +++++++++++++++++
>>> >  drivers/gpu/drm/i915/display/intel_dsi.h |  3 +++
>>> >  2 files changed, 20 insertions(+)
>>> >
>>> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
>>> > b/drivers/gpu/drm/i915/display/icl_dsi.c
>>> > index 5dd9eebab6b1..877746416e52 100644
>>> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
>>> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
>>> > @@ -73,6 +73,23 @@ static enum transcoder
>>> dsi_port_to_transcoder(enum port port)
>>> >  		return TRANSCODER_DSI_1;
>>> >  }
>>> >
>>> > +struct intel_encoder *gen11_dsi_find_cmd_mode_encoder(struct
>>> > +intel_crtc *crtc) {
>>> > +	struct drm_device *dev = crtc->base.dev;
>>> > +	struct intel_encoder *encoder;
>>> > +	struct intel_dsi *intel_dsi;
>>> > +
>>> > +	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
>>> > +		if (encoder->type != INTEL_OUTPUT_DSI)
>>> > +			continue;
>>> > +		intel_dsi = enc_to_intel_dsi(&encoder->base);
>>> > +		if (intel_dsi->operation_mode ==
>>> INTEL_DSI_COMMAND_MODE)
>>> > +			return encoder;
>>> > +	}
>>> > +
>>> > +	return NULL;
>>> > +}
>>> 
>>> This may be a bit harsh, but everything that feels wrong about the following
>>> patches pretty much boils down to this function. It may get the job done,
>>> and I don't have a better suggestion on how to accomplish this right now.
>>> But it seems like we shouldn't have to do anything like this, and makes you
>>> feel like there's something wrong with the design.
>>> 
>>> It would be great to be able to handle this using crtc state, but alas the
>>> vblank enable hook only gets passed a struct drm_crtc *. (Patch 7 could
>>> easily switch to using crtc state, but need to also solve patch
>>> 5.)
>> .
>>> 
>>> I'll get back to you on this later, but in the mean time - Ville, do you have
>>> any ideas?
>>
>> Other option of adding in crtc_state, did you mean something like this
>> https://patchwork.freedesktop.org/patch/336178/ 
>> @Jani and @Ville  Please let me know your comments.
>
> The problem is that we don't have access to the crtc state in patch 5
> gen11_dsi_configure_te() because the call path is via drm_crtc_funcs and
> bdw_enable_vblank.

PS. I think moving some of this stuff to the crtc state is probably a
good thing no matter what. It's just that we *also* need some solution
to the problem in patch 5.


>
> BR,
> Jani.
>
>
>>
>> Thanks,
>> Vandita
>>> 
>>> 
>>> BR,
>>> Jani.
>>> 
>>> 
>>> > +
>>> >  static void wait_for_cmds_dispatched_to_panel(struct intel_encoder
>>> > *encoder)  {
>>> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); diff
>>> > --git a/drivers/gpu/drm/i915/display/intel_dsi.h
>>> > b/drivers/gpu/drm/i915/display/intel_dsi.h
>>> > index b15be5814599..071dad7ee04a 100644
>>> > --- a/drivers/gpu/drm/i915/display/intel_dsi.h
>>> > +++ b/drivers/gpu/drm/i915/display/intel_dsi.h
>>> > @@ -201,6 +201,9 @@ u32 bxt_dsi_get_pclk(struct intel_encoder
>>> *encoder,
>>> >  		     struct intel_crtc_state *config);  void
>>> > bxt_dsi_reset_clocks(struct intel_encoder *encoder, enum port port);
>>> >
>>> > +/* icl_dsi.c */
>>> > +struct intel_encoder *gen11_dsi_find_cmd_mode_encoder(struct
>>> > +intel_crtc *crtc);
>>> > +
>>> >  /* intel_dsi_vbt.c */
>>> >  bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id);
>>> > void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,
>>> 
>>> --
>>> Jani Nikula, Intel Open Source Graphics Center
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
index 5dd9eebab6b1..877746416e52 100644
--- a/drivers/gpu/drm/i915/display/icl_dsi.c
+++ b/drivers/gpu/drm/i915/display/icl_dsi.c
@@ -73,6 +73,23 @@  static enum transcoder dsi_port_to_transcoder(enum port port)
 		return TRANSCODER_DSI_1;
 }
 
+struct intel_encoder *gen11_dsi_find_cmd_mode_encoder(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct intel_encoder *encoder;
+	struct intel_dsi *intel_dsi;
+
+	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
+		if (encoder->type != INTEL_OUTPUT_DSI)
+			continue;
+		intel_dsi = enc_to_intel_dsi(&encoder->base);
+		if (intel_dsi->operation_mode == INTEL_DSI_COMMAND_MODE)
+			return encoder;
+	}
+
+	return NULL;
+}
+
 static void wait_for_cmds_dispatched_to_panel(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
diff --git a/drivers/gpu/drm/i915/display/intel_dsi.h b/drivers/gpu/drm/i915/display/intel_dsi.h
index b15be5814599..071dad7ee04a 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi.h
+++ b/drivers/gpu/drm/i915/display/intel_dsi.h
@@ -201,6 +201,9 @@  u32 bxt_dsi_get_pclk(struct intel_encoder *encoder,
 		     struct intel_crtc_state *config);
 void bxt_dsi_reset_clocks(struct intel_encoder *encoder, enum port port);
 
+/* icl_dsi.c */
+struct intel_encoder *gen11_dsi_find_cmd_mode_encoder(struct intel_crtc *crtc);
+
 /* intel_dsi_vbt.c */
 bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id);
 void intel_dsi_vbt_exec_sequence(struct intel_dsi *intel_dsi,