Re: [Intel-gfx] [PATCH 1/3] drm/i915: Extract intel_dp_output_format()

2020-09-18 Thread Navare, Manasi 
On Fri, Sep 18, 2020 at 01:39:45PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 17, 2020 at 04:57:09PM -0700, Navare, Manasi wrote:
> > On Fri, Sep 18, 2020 at 12:43:33AM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä 
> > > 
> > > Refactor the output_format calculation into a helper so that
> > > we can reuse it for mode validation as well.
> > > 
> > > Signed-off-by: Ville Syrjälä 
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c | 32 +++--
> > >  1 file changed, 20 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index bf1e9cf1c0f3..ad9b8b16fadb 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -592,6 +592,22 @@ static u8 intel_dp_dsc_get_slice_count(struct 
> > > intel_dp *intel_dp,
> > >   return 0;
> > >  }
> > >  
> > > +static enum intel_output_format
> > > +intel_dp_output_format(struct drm_connector *connector,
> > > +const struct drm_display_mode *mode)
> > > +{
> > > + struct intel_dp *intel_dp = 
> > > intel_attached_dp(to_intel_connector(connector));
> > > + const struct drm_display_info *info = >display_info;
> > > +
> > > + if (!drm_mode_is_420_only(info, mode))
> > > + return INTEL_OUTPUT_FORMAT_RGB;
> > > +
> > > + if (intel_dp->dfp.ycbcr_444_to_420)
> > > + return INTEL_OUTPUT_FORMAT_YCBCR444;
> > > + else
> > > + return INTEL_OUTPUT_FORMAT_YCBCR420;
> > > +}
> > > +
> > >  static bool intel_dp_hdisplay_bad(struct drm_i915_private *dev_priv,
> > > int hdisplay)
> > >  {
> > > @@ -2430,27 +2446,20 @@ intel_dp_compute_link_config(struct intel_encoder 
> > > *encoder,
> > >  }
> > >  
> > >  static int
> > > -intel_dp_ycbcr420_config(struct intel_dp *intel_dp,
> > > -  struct intel_crtc_state *crtc_state,
> > > +intel_dp_ycbcr420_config(struct intel_crtc_state *crtc_state,
> > >const struct drm_connector_state *conn_state)
> > >  {
> > >   struct drm_connector *connector = conn_state->connector;
> > > - const struct drm_display_info *info = >display_info;
> > >   const struct drm_display_mode *adjusted_mode =
> > >   _state->hw.adjusted_mode;
> > >  
> > >   if (!connector->ycbcr_420_allowed)
> > >   return 0;
> > >  
> > > - if (!drm_mode_is_420_only(info, adjusted_mode))
> > > - return 0;
> > > + crtc_state->output_format = intel_dp_output_format(connector, 
> > > adjusted_mode);
> > 
> > So by default if its not 420_only then we set it to RGB?
> 
> Yes. The code is still a bit messy because we have three places where
> we set this. Probably will try to unify it a bit more, and try to get
> the lspcon stuff looking more like any other protocol converter.
> 
> Actually IIRC I noticed that some lspcon chips seem to have some
> of the 1.3 protocol converter registers even though they only
> advertise DPCD 1.2. Not yet sure how to handle that in the
> cleanest way possible...

But if DPCD says only 1.2 then we shouldnt be reading 1.3 specific registers
and support it as 1.2 IMO

Manasi
> 
> -- 
> Ville Syrjälä
> Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm/i915: Extract intel_dp_output_format()

2020-09-18 Thread Ville Syrjälä 
On Thu, Sep 17, 2020 at 04:57:09PM -0700, Navare, Manasi wrote:
> On Fri, Sep 18, 2020 at 12:43:33AM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > Refactor the output_format calculation into a helper so that
> > we can reuse it for mode validation as well.
> > 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 32 +++--
> >  1 file changed, 20 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index bf1e9cf1c0f3..ad9b8b16fadb 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -592,6 +592,22 @@ static u8 intel_dp_dsc_get_slice_count(struct intel_dp 
> > *intel_dp,
> > return 0;
> >  }
> >  
> > +static enum intel_output_format
> > +intel_dp_output_format(struct drm_connector *connector,
> > +  const struct drm_display_mode *mode)
> > +{
> > +   struct intel_dp *intel_dp = 
> > intel_attached_dp(to_intel_connector(connector));
> > +   const struct drm_display_info *info = >display_info;
> > +
> > +   if (!drm_mode_is_420_only(info, mode))
> > +   return INTEL_OUTPUT_FORMAT_RGB;
> > +
> > +   if (intel_dp->dfp.ycbcr_444_to_420)
> > +   return INTEL_OUTPUT_FORMAT_YCBCR444;
> > +   else
> > +   return INTEL_OUTPUT_FORMAT_YCBCR420;
> > +}
> > +
> >  static bool intel_dp_hdisplay_bad(struct drm_i915_private *dev_priv,
> >   int hdisplay)
> >  {
> > @@ -2430,27 +2446,20 @@ intel_dp_compute_link_config(struct intel_encoder 
> > *encoder,
> >  }
> >  
> >  static int
> > -intel_dp_ycbcr420_config(struct intel_dp *intel_dp,
> > -struct intel_crtc_state *crtc_state,
> > +intel_dp_ycbcr420_config(struct intel_crtc_state *crtc_state,
> >  const struct drm_connector_state *conn_state)
> >  {
> > struct drm_connector *connector = conn_state->connector;
> > -   const struct drm_display_info *info = >display_info;
> > const struct drm_display_mode *adjusted_mode =
> > _state->hw.adjusted_mode;
> >  
> > if (!connector->ycbcr_420_allowed)
> > return 0;
> >  
> > -   if (!drm_mode_is_420_only(info, adjusted_mode))
> > -   return 0;
> > +   crtc_state->output_format = intel_dp_output_format(connector, 
> > adjusted_mode);
> 
> So by default if its not 420_only then we set it to RGB?

Yes. The code is still a bit messy because we have three places where
we set this. Probably will try to unify it a bit more, and try to get
the lspcon stuff looking more like any other protocol converter.

Actually IIRC I noticed that some lspcon chips seem to have some
of the 1.3 protocol converter registers even though they only
advertise DPCD 1.2. Not yet sure how to handle that in the
cleanest way possible...

-- 
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm/i915: Extract intel_dp_output_format()

2020-09-17 Thread Kulkarni, Vandita 
> -Original Message-
> From: Intel-gfx  On Behalf Of Ville
> Syrjala
> Sent: Friday, September 18, 2020 3:14 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 1/3] drm/i915: Extract intel_dp_output_format()
> 
> From: Ville Syrjälä 
> 
> Refactor the output_format calculation into a helper so that we can reuse it
> for mode validation as well.
> 
> Signed-off-by: Ville Syrjälä 

Looks good to me.
Reviewed-by: Vandita Kulkarni 

Thanks,
Vandita
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 32 +++--
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index bf1e9cf1c0f3..ad9b8b16fadb 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -592,6 +592,22 @@ static u8 intel_dp_dsc_get_slice_count(struct
> intel_dp *intel_dp,
>   return 0;
>  }
> 
> +static enum intel_output_format
> +intel_dp_output_format(struct drm_connector *connector,
> +const struct drm_display_mode *mode) {
> + struct intel_dp *intel_dp =
> intel_attached_dp(to_intel_connector(connector));
> + const struct drm_display_info *info = >display_info;
> +
> + if (!drm_mode_is_420_only(info, mode))
> + return INTEL_OUTPUT_FORMAT_RGB;
> +
> + if (intel_dp->dfp.ycbcr_444_to_420)
> + return INTEL_OUTPUT_FORMAT_YCBCR444;
> + else
> + return INTEL_OUTPUT_FORMAT_YCBCR420;
> +}
> +
>  static bool intel_dp_hdisplay_bad(struct drm_i915_private *dev_priv,
> int hdisplay)
>  {
> @@ -2430,27 +2446,20 @@ intel_dp_compute_link_config(struct
> intel_encoder *encoder,  }
> 
>  static int
> -intel_dp_ycbcr420_config(struct intel_dp *intel_dp,
> -  struct intel_crtc_state *crtc_state,
> +intel_dp_ycbcr420_config(struct intel_crtc_state *crtc_state,
>const struct drm_connector_state *conn_state)  {
>   struct drm_connector *connector = conn_state->connector;
> - const struct drm_display_info *info = >display_info;
>   const struct drm_display_mode *adjusted_mode =
>   _state->hw.adjusted_mode;
> 
>   if (!connector->ycbcr_420_allowed)
>   return 0;
> 
> - if (!drm_mode_is_420_only(info, adjusted_mode))
> - return 0;
> + crtc_state->output_format = intel_dp_output_format(connector,
> +adjusted_mode);
> 
> - if (intel_dp->dfp.ycbcr_444_to_420) {
> - crtc_state->output_format =
> INTEL_OUTPUT_FORMAT_YCBCR444;
> + if (crtc_state->output_format !=
> INTEL_OUTPUT_FORMAT_YCBCR420)
>   return 0;
> - }
> -
> - crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
> 
>   return intel_pch_panel_fitting(crtc_state, conn_state);  } @@ -
> 2710,8 +2719,7 @@ intel_dp_compute_config(struct intel_encoder
> *encoder,
>   if (lspcon->active)
>   lspcon_ycbcr420_config(_connector->base,
> pipe_config);
>   else
> - ret = intel_dp_ycbcr420_config(intel_dp, pipe_config,
> -conn_state);
> + ret = intel_dp_ycbcr420_config(pipe_config, conn_state);
>   if (ret)
>   return ret;
> 
> --
> 2.26.2
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm/i915: Extract intel_dp_output_format()

2020-09-17 Thread Navare, Manasi 
On Fri, Sep 18, 2020 at 12:43:33AM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Refactor the output_format calculation into a helper so that
> we can reuse it for mode validation as well.
> 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 32 +++--
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index bf1e9cf1c0f3..ad9b8b16fadb 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -592,6 +592,22 @@ static u8 intel_dp_dsc_get_slice_count(struct intel_dp 
> *intel_dp,
>   return 0;
>  }
>  
> +static enum intel_output_format
> +intel_dp_output_format(struct drm_connector *connector,
> +const struct drm_display_mode *mode)
> +{
> + struct intel_dp *intel_dp = 
> intel_attached_dp(to_intel_connector(connector));
> + const struct drm_display_info *info = >display_info;
> +
> + if (!drm_mode_is_420_only(info, mode))
> + return INTEL_OUTPUT_FORMAT_RGB;
> +
> + if (intel_dp->dfp.ycbcr_444_to_420)
> + return INTEL_OUTPUT_FORMAT_YCBCR444;
> + else
> + return INTEL_OUTPUT_FORMAT_YCBCR420;
> +}
> +
>  static bool intel_dp_hdisplay_bad(struct drm_i915_private *dev_priv,
> int hdisplay)
>  {
> @@ -2430,27 +2446,20 @@ intel_dp_compute_link_config(struct intel_encoder 
> *encoder,
>  }
>  
>  static int
> -intel_dp_ycbcr420_config(struct intel_dp *intel_dp,
> -  struct intel_crtc_state *crtc_state,
> +intel_dp_ycbcr420_config(struct intel_crtc_state *crtc_state,
>const struct drm_connector_state *conn_state)
>  {
>   struct drm_connector *connector = conn_state->connector;
> - const struct drm_display_info *info = >display_info;
>   const struct drm_display_mode *adjusted_mode =
>   _state->hw.adjusted_mode;
>  
>   if (!connector->ycbcr_420_allowed)
>   return 0;
>  
> - if (!drm_mode_is_420_only(info, adjusted_mode))
> - return 0;
> + crtc_state->output_format = intel_dp_output_format(connector, 
> adjusted_mode);

So by default if its not 420_only then we set it to RGB?

Manasi

>  
> - if (intel_dp->dfp.ycbcr_444_to_420) {
> - crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR444;
> + if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_YCBCR420)
>   return 0;
> - }
> -
> - crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
>  
>   return intel_pch_panel_fitting(crtc_state, conn_state);
>  }
> @@ -2710,8 +2719,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>   if (lspcon->active)
>   lspcon_ycbcr420_config(_connector->base, pipe_config);
>   else
> - ret = intel_dp_ycbcr420_config(intel_dp, pipe_config,
> -conn_state);
> + ret = intel_dp_ycbcr420_config(pipe_config, conn_state);
>   if (ret)
>   return ret;
>  
> -- 
> 2.26.2
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/3] drm/i915: Extract intel_dp_output_format()

2020-09-17 Thread Ville Syrjala 
From: Ville Syrjälä 

Refactor the output_format calculation into a helper so that
we can reuse it for mode validation as well.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_dp.c | 32 +++--
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
b/drivers/gpu/drm/i915/display/intel_dp.c
index bf1e9cf1c0f3..ad9b8b16fadb 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -592,6 +592,22 @@ static u8 intel_dp_dsc_get_slice_count(struct intel_dp 
*intel_dp,
return 0;
 }
 
+static enum intel_output_format
+intel_dp_output_format(struct drm_connector *connector,
+  const struct drm_display_mode *mode)
+{
+   struct intel_dp *intel_dp = 
intel_attached_dp(to_intel_connector(connector));
+   const struct drm_display_info *info = >display_info;
+
+   if (!drm_mode_is_420_only(info, mode))
+   return INTEL_OUTPUT_FORMAT_RGB;
+
+   if (intel_dp->dfp.ycbcr_444_to_420)
+   return INTEL_OUTPUT_FORMAT_YCBCR444;
+   else
+   return INTEL_OUTPUT_FORMAT_YCBCR420;
+}
+
 static bool intel_dp_hdisplay_bad(struct drm_i915_private *dev_priv,
  int hdisplay)
 {
@@ -2430,27 +2446,20 @@ intel_dp_compute_link_config(struct intel_encoder 
*encoder,
 }
 
 static int
-intel_dp_ycbcr420_config(struct intel_dp *intel_dp,
-struct intel_crtc_state *crtc_state,
+intel_dp_ycbcr420_config(struct intel_crtc_state *crtc_state,
 const struct drm_connector_state *conn_state)
 {
struct drm_connector *connector = conn_state->connector;
-   const struct drm_display_info *info = >display_info;
const struct drm_display_mode *adjusted_mode =
_state->hw.adjusted_mode;
 
if (!connector->ycbcr_420_allowed)
return 0;
 
-   if (!drm_mode_is_420_only(info, adjusted_mode))
-   return 0;
+   crtc_state->output_format = intel_dp_output_format(connector, 
adjusted_mode);
 
-   if (intel_dp->dfp.ycbcr_444_to_420) {
-   crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR444;
+   if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_YCBCR420)
return 0;
-   }
-
-   crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
 
return intel_pch_panel_fitting(crtc_state, conn_state);
 }
@@ -2710,8 +2719,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
if (lspcon->active)
lspcon_ycbcr420_config(_connector->base, pipe_config);
else
-   ret = intel_dp_ycbcr420_config(intel_dp, pipe_config,
-  conn_state);
+   ret = intel_dp_ycbcr420_config(pipe_config, conn_state);
if (ret)
return ret;
 
-- 
2.26.2

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx