Re: [PATCH 5/6] drm/i915: Extract VESA DSC bpp alignment to separate function

[Navare, Manasi]

On Wed, Nov 23, 2022 at 12:05:51PM +0200, Stanislav Lisovskiy wrote:
> We might to use that function separately from intel_dp_dsc_compute_config
> for DP DSC over MST case, because allocating bandwidth in that
> case can be a bit more tricky. So in order to avoid code copy-pasta
> lets extract this to separate function and reuse it for both SST
> and MST cases.
> 
> v2: Removed multiple blank lines
> v3: Rename intel_dp_dsc_nearest_vesa_bpp to intel_dp_dsc_nearest_valid_bpp
> to reflect its meaning more properly.
> (Manasi Navare)
> 
> Signed-off-by: Stanislav Lisovskiy 

Looks good now,

Reviewed-by: Manasi Navare 

Manasi

> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 50 +
>  drivers/gpu/drm/i915/display/intel_dp.h |  1 +
>  drivers/gpu/drm/i915/display/intel_dp_mst.c |  1 -
>  3 files changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 44e2424a54c0..d78216fba0a2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -672,6 +672,36 @@ small_joiner_ram_size_bits(struct drm_i915_private *i915)
>   return 6144 * 8;
>  }
>  
> +u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, 
> u32 pipe_bpp)
> +{
> + u32 bits_per_pixel = bpp;
> + int i;
> +
> + /* Error out if the max bpp is less than smallest allowed valid bpp */
> + if (bits_per_pixel < valid_dsc_bpp[0]) {
> + drm_dbg_kms(>drm, "Unsupported BPP %u, min %u\n",
> + bits_per_pixel, valid_dsc_bpp[0]);
> + return 0;
> + }
> +
> + /* From XE_LPD onwards we support from bpc upto uncompressed bpp-1 BPPs 
> */
> + if (DISPLAY_VER(i915) >= 13) {
> + bits_per_pixel = min(bits_per_pixel, pipe_bpp - 1);
> + } else {
> + /* Find the nearest match in the array of known BPPs from VESA 
> */
> + for (i = 0; i < ARRAY_SIZE(valid_dsc_bpp) - 1; i++) {
> + if (bits_per_pixel < valid_dsc_bpp[i + 1])
> + break;
> + }
> + drm_dbg_kms(>drm, "Set dsc bpp from %d to VESA %d\n",
> + bits_per_pixel, valid_dsc_bpp[i]);
> +
> + bits_per_pixel = valid_dsc_bpp[i];
> + }
> +
> + return bits_per_pixel;
> +}
> +
>  u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>   u32 link_clock, u32 lane_count,
>   u32 mode_clock, u32 mode_hdisplay,
> @@ -680,7 +710,6 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private 
> *i915,
>   u32 timeslots)
>  {
>   u32 bits_per_pixel, max_bpp_small_joiner_ram;
> - int i;
>  
>   /*
>* Available Link Bandwidth(Kbits/sec) = (NumberOfLanes)*
> @@ -713,24 +742,7 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private 
> *i915,
>   bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
>   }
>  
> - /* Error out if the max bpp is less than smallest allowed valid bpp */
> - if (bits_per_pixel < valid_dsc_bpp[0]) {
> - drm_dbg_kms(>drm, "Unsupported BPP %u, min %u\n",
> - bits_per_pixel, valid_dsc_bpp[0]);
> - return 0;
> - }
> -
> - /* From XE_LPD onwards we support from bpc upto uncompressed bpp-1 BPPs 
> */
> - if (DISPLAY_VER(i915) >= 13) {
> - bits_per_pixel = min(bits_per_pixel, pipe_bpp - 1);
> - } else {
> - /* Find the nearest match in the array of known BPPs from VESA 
> */
> - for (i = 0; i < ARRAY_SIZE(valid_dsc_bpp) - 1; i++) {
> - if (bits_per_pixel < valid_dsc_bpp[i + 1])
> - break;
> - }
> - bits_per_pixel = valid_dsc_bpp[i];
> - }
> + bits_per_pixel = intel_dp_dsc_nearest_valid_bpp(i915, bits_per_pixel, 
> pipe_bpp);
>  
>   /*
>* Compressed BPP in U6.4 format so multiply by 16, for Gen 11,
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h 
> b/drivers/gpu/drm/i915/display/intel_dp.h
> index c6539a6915e9..e4faccf87370 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -120,6 +120,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int 
> lane_count)
>  }
>  
>  u32 intel_dp_mode_to_fec_clock(u32 mode_clock);
> +u32 intel_dp_dsc_nearest_valid_bpp(struct drm_i915_private *i915, u32 bpp, 
> u

RE: [PATCH 6/6] drm/i915: Bpp/timeslot calculation fixes for DP MST DSC

[Navare, Manasi D]

Thanks Stan for the explanation,
With that

Reviewed-by: Manasi Navare 

Manasi


-Original Message-
From: Lisovskiy, Stanislav  
Sent: Tuesday, November 22, 2022 2:40 AM
To: Navare, Manasi D 
Cc: intel-...@lists.freedesktop.org; Saarinen, Jani ; 
Nikula, Jani ; dri-devel@lists.freedesktop.org; 
Govindapillai, Vinod 
Subject: Re: [PATCH 6/6] drm/i915: Bpp/timeslot calculation fixes for DP MST DSC

On Thu, Nov 10, 2022 at 02:23:53PM -0800, Navare, Manasi wrote:
> On Thu, Nov 03, 2022 at 03:23:00PM +0200, Stanislav Lisovskiy wrote:
> > Fix intel_dp_dsc_compute_config, previously timeslots parameter was 
> > used in fact not as a timeslots, but more like a ratio timeslots/64, 
> > which of course didn't have any effect for SST DSC, but causes now 
> > issues for MST DSC.
> > Secondly we need to calculate pipe_bpp using 
> > intel_dp_dsc_compute_bpp only for SST DSC case, while for MST case 
> > it has been calculated earlier already with 
> > intel_dp_dsc_mst_compute_link_config.
> > Third we also were wrongly determining sink min bpp/max bpp, those 
> > limites should be intersected with our limits to find common 
> > acceptable bpp's, plus on top of that we should align those with 
> > VESA bpps and only then calculate required timeslots amount.
> > Some MST hubs started to work only after third change was made.
> > 
> > v2: Make kernel test robot happy(claimed there was unitialzed use,
> > while there is none)
> > 
> > Signed-off-by: Stanislav Lisovskiy 
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 69 ++---
> >  drivers/gpu/drm/i915/display/intel_dp.h |  3 +-
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 69 
> > +
> >  3 files changed, 106 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 8288a30dbd51..82752b696498 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -716,9 +716,14 @@ u16 intel_dp_dsc_get_output_bpp(struct 
> > drm_i915_private *i915,
> >  * for SST -> TimeSlotsPerMTP is 1,
> >  * for MST -> TimeSlotsPerMTP has to be calculated
> >  */
> > -   bits_per_pixel = (link_clock * lane_count * 8) * timeslots /
> > -intel_dp_mode_to_fec_clock(mode_clock);
> > -   drm_dbg_kms(>drm, "Max link bpp: %u\n", bits_per_pixel);
> > +   bits_per_pixel = DIV_ROUND_UP((link_clock * lane_count) * timeslots,
> > + intel_dp_mode_to_fec_clock(mode_clock) * 
> > 8);
> 
> Why did we remove the *8 in the numerator for the total bandwidth 
> link_clock * lane_count * 8 ?
> 
> Other than this clarification, all changes look good
> 
> Manasi

Hi Manasi,

Because previously this function was actually confusing the ratio timeslots/64, 
with the timeslots number.

It was actually expecting a ratio timeslots/64, rather than the timeslots 
number.

For SST it didn't matter as timeslots were always 1, but for MST case if we 
multiply that by number number of timeslots, this formula will return some big 
bogus bits_per_pixel number(checked that). 
Of course we can pass a ratio timeslots/64 here, but it isn't very convenient 
and intuitive to manipulate.
So I made it to use a "timeslots" parameter as timeslots number, so that the 
ratio is calculated as part of the formula i.e:

((link_clock * lane_count * 8) * (timeslots / 64)) /  
intel_dp_mode_to_fec_clock(mode_clock);

which can be simplified as

((link_clock * lane_count * timeslots) / 8) / 
intel_dp_mode_to_fec_clock(mode_clock);

the whole formula comes from that
pipe_bpp * crtc_clock should be equal to link_total_bw * (timeslots / 64), i.e
timeslots/64 ratio defines, how much of the link_total_bw(link_clock * 
lane_count * 8) we have for those pipe_bpp * crtc_clock, which we want to 
accomodate there.

Obviously if we just multiplied link_total_bw by timeslots, we would get a 
situation that the more timeslots we allocate, the more total bw we get, which 
is wrong and will result in some bogus huge pipe_bpp numbers.

Stan

> 
> > +
> > +   drm_dbg_kms(>drm, "Max link bpp is %u for %u timeslots "
> > +   "total bw %u pixel clock %u\n",
> > +   bits_per_pixel, timeslots,
> > +   (link_clock * lane_count * 8),
> > +   intel_dp_mode_to_fec_clock(mode_clock));
> >  
> > /* Small Joiner Check: output bpp <= joiner RAM (bits) / Horiz. width */
> > max_bpp_small_joiner_ram = small_joiner_ram_size_bits(i915)

Re: [PATCH 6/6] drm/i915: Bpp/timeslot calculation fixes for DP MST DSC

[Navare, Manasi]

On Thu, Nov 03, 2022 at 03:23:00PM +0200, Stanislav Lisovskiy wrote:
> Fix intel_dp_dsc_compute_config, previously timeslots parameter
> was used in fact not as a timeslots, but more like a ratio
> timeslots/64, which of course didn't have any effect for SST DSC,
> but causes now issues for MST DSC.
> Secondly we need to calculate pipe_bpp using intel_dp_dsc_compute_bpp
> only for SST DSC case, while for MST case it has been calculated
> earlier already with intel_dp_dsc_mst_compute_link_config.
> Third we also were wrongly determining sink min bpp/max bpp, those
> limites should be intersected with our limits to find common
> acceptable bpp's, plus on top of that we should align those with
> VESA bpps and only then calculate required timeslots amount.
> Some MST hubs started to work only after third change was made.
> 
> v2: Make kernel test robot happy(claimed there was unitialzed use,
> while there is none)
> 
> Signed-off-by: Stanislav Lisovskiy 
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 69 ++---
>  drivers/gpu/drm/i915/display/intel_dp.h |  3 +-
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 69 +
>  3 files changed, 106 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 8288a30dbd51..82752b696498 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -716,9 +716,14 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private 
> *i915,
>* for SST -> TimeSlotsPerMTP is 1,
>* for MST -> TimeSlotsPerMTP has to be calculated
>*/
> - bits_per_pixel = (link_clock * lane_count * 8) * timeslots /
> -  intel_dp_mode_to_fec_clock(mode_clock);
> - drm_dbg_kms(>drm, "Max link bpp: %u\n", bits_per_pixel);
> + bits_per_pixel = DIV_ROUND_UP((link_clock * lane_count) * timeslots,
> +   intel_dp_mode_to_fec_clock(mode_clock) * 
> 8);

Why did we remove the *8 in the numerator for the total bandwidth
link_clock * lane_count * 8 ? 

Other than this clarification, all changes look good

Manasi

> +
> + drm_dbg_kms(>drm, "Max link bpp is %u for %u timeslots "
> + "total bw %u pixel clock %u\n",
> + bits_per_pixel, timeslots,
> + (link_clock * lane_count * 8),
> + intel_dp_mode_to_fec_clock(mode_clock));
>  
>   /* Small Joiner Check: output bpp <= joiner RAM (bits) / Horiz. width */
>   max_bpp_small_joiner_ram = small_joiner_ram_size_bits(i915) /
> @@ -1047,7 +1052,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>   target_clock,
>   mode->hdisplay,
>   bigjoiner,
> - pipe_bpp, 1) >> 4;
> + pipe_bpp, 64) >> 4;
>   dsc_slice_count =
>   intel_dp_dsc_get_slice_count(intel_dp,
>target_clock,
> @@ -1481,7 +1486,8 @@ int intel_dp_dsc_compute_config(struct intel_dp 
> *intel_dp,
>   struct intel_crtc_state *pipe_config,
>   struct drm_connector_state *conn_state,
>   struct link_config_limits *limits,
> - int timeslots)
> + int timeslots,
> + bool compute_pipe_bpp)
>  {
>   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>   struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> @@ -1496,7 +1502,10 @@ int intel_dp_dsc_compute_config(struct intel_dp 
> *intel_dp,
>   if (!intel_dp_supports_dsc(intel_dp, pipe_config))
>   return -EINVAL;
>  
> - pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, 
> conn_state->max_requested_bpc);
> + if (compute_pipe_bpp)
> + pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, 
> conn_state->max_requested_bpc);
> + else
> + pipe_bpp = pipe_config->pipe_bpp;
>  
>   if (intel_dp->force_dsc_bpc) {
>   pipe_bpp = intel_dp->force_dsc_bpc * 3;
> @@ -1527,31 +1536,47 @@ int intel_dp_dsc_compute_config(struct intel_dp 
> *intel_dp,
>   drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
>   true);
>   } else {
> - u16 dsc_max_output_bpp;
> + u16 dsc_max_output_bpp = 0;
>   u8 dsc_dp_slice_count;
>  
> - dsc_max_output_bpp =
> - intel_dp_dsc_get_output_bpp(dev_priv,
> - 

Re: [PATCH 5/6] drm/i915: Extract VESA DSC bpp alignment to separate function

[Navare, Manasi]

On Thu, Nov 03, 2022 at 03:21:46PM +0200, Stanislav Lisovskiy wrote:
> We might to use that function separately from intel_dp_dsc_compute_config
> for DP DSC over MST case, because allocating bandwidth in that
> case can be a bit more tricky. So in order to avoid code copy-pasta
> lets extract this to separate function and reuse it for both SST
> and MST cases.
> 
> v2: Removed multiple blank lines
> 
> Signed-off-by: Stanislav Lisovskiy 
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 50 +
>  drivers/gpu/drm/i915/display/intel_dp.h |  1 +
>  drivers/gpu/drm/i915/display/intel_dp_mst.c |  1 -
>  3 files changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 70f4d6422795..8288a30dbd51 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -671,6 +671,36 @@ small_joiner_ram_size_bits(struct drm_i915_private *i915)
>   return 6144 * 8;
>  }
>  
> +u32 intel_dp_dsc_nearest_vesa_bpp(struct drm_i915_private *i915, u32 bpp, 
> u32 pipe_bpp)

It makes sense to pull this out into a separate function.
For the function name, we have never had vesa in any of the function
names even though most of these come from vesa spec. So I think we
should remove vesa IMO, just name it as intel_dp_dsc_nearest_valid_bpp
or something?

Manasi

> +{
> + u32 bits_per_pixel = bpp;
> + int i;
> +
> + /* Error out if the max bpp is less than smallest allowed valid bpp */
> + if (bits_per_pixel < valid_dsc_bpp[0]) {
> + drm_dbg_kms(>drm, "Unsupported BPP %u, min %u\n",
> + bits_per_pixel, valid_dsc_bpp[0]);
> + return 0;
> + }
> +
> + /* From XE_LPD onwards we support from bpc upto uncompressed bpp-1 BPPs 
> */
> + if (DISPLAY_VER(i915) >= 13) {
> + bits_per_pixel = min(bits_per_pixel, pipe_bpp - 1);
> + } else {
> + /* Find the nearest match in the array of known BPPs from VESA 
> */
> + for (i = 0; i < ARRAY_SIZE(valid_dsc_bpp) - 1; i++) {
> + if (bits_per_pixel < valid_dsc_bpp[i + 1])
> + break;
> + }
> + drm_dbg_kms(>drm, "Set dsc bpp from %d to VESA %d\n",
> + bits_per_pixel, valid_dsc_bpp[i]);
> +
> + bits_per_pixel = valid_dsc_bpp[i];
> + }
> +
> + return bits_per_pixel;
> +}
> +
>  u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915,
>   u32 link_clock, u32 lane_count,
>   u32 mode_clock, u32 mode_hdisplay,
> @@ -679,7 +709,6 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private 
> *i915,
>   u32 timeslots)
>  {
>   u32 bits_per_pixel, max_bpp_small_joiner_ram;
> - int i;
>  
>   /*
>* Available Link Bandwidth(Kbits/sec) = (NumberOfLanes)*
> @@ -712,24 +741,7 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private 
> *i915,
>   bits_per_pixel = min(bits_per_pixel, max_bpp_bigjoiner);
>   }
>  
> - /* Error out if the max bpp is less than smallest allowed valid bpp */
> - if (bits_per_pixel < valid_dsc_bpp[0]) {
> - drm_dbg_kms(>drm, "Unsupported BPP %u, min %u\n",
> - bits_per_pixel, valid_dsc_bpp[0]);
> - return 0;
> - }
> -
> - /* From XE_LPD onwards we support from bpc upto uncompressed bpp-1 BPPs 
> */
> - if (DISPLAY_VER(i915) >= 13) {
> - bits_per_pixel = min(bits_per_pixel, pipe_bpp - 1);
> - } else {
> - /* Find the nearest match in the array of known BPPs from VESA 
> */
> - for (i = 0; i < ARRAY_SIZE(valid_dsc_bpp) - 1; i++) {
> - if (bits_per_pixel < valid_dsc_bpp[i + 1])
> - break;
> - }
> - bits_per_pixel = valid_dsc_bpp[i];
> - }
> + bits_per_pixel = intel_dp_dsc_nearest_vesa_bpp(i915, bits_per_pixel, 
> pipe_bpp);
>  
>   /*
>* Compressed BPP in U6.4 format so multiply by 16, for Gen 11,
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h 
> b/drivers/gpu/drm/i915/display/intel_dp.h
> index c6539a6915e9..0fe10d93b75c 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -120,6 +120,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int 
> lane_count)
>  }
>  
>  u32 intel_dp_mode_to_fec_clock(u32 mode_clock);
> +u32 intel_dp_dsc_nearest_vesa_bpp(struct drm_i915_private *i915, u32 bpp, 
> u32 pipe_bpp);
>  
>  void intel_ddi_update_pipe(struct intel_atomic_state *state,
>  struct intel_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c 
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 61b2bd504e80..8442eea27a57 100644
> --- 

Re: [PATCH] drm/edid: Parse VRR cap fields from HFVSDB block

[Navare, Manasi]

On Tue, Oct 18, 2022 at 11:01:48AM +0530, Ankit Nautiyal wrote:
> This patch parses HFVSDB fields for VRR capabilities of an
> HDMI2.1 sink and stores the VRR caps in a new structure in
> drm_hdmi_info.
> 
> Signed-off-by: Ankit Nautiyal 

Makes sense to add this VRR info to drm_hdmi_info struct and not combine
with DP VRR info struct in display info since hdmi info has other HDMI
cap info stored here.

Reviewed-by: Manasi Navare 

Manasi

> ---
>  drivers/gpu/drm/drm_edid.c  | 26 --
>  include/drm/drm_connector.h | 27 +++
>  2 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 47465b9765f1..bb1f7d899580 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5823,6 +5823,21 @@ static void drm_parse_ycbcr420_deep_color_info(struct 
> drm_connector *connector,
>   hdmi->y420_dc_modes = dc_mask;
>  }
>  
> +static void drm_parse_vrr_info(struct drm_hdmi_vrr_cap *hdmi_vrr,
> +const u8 *hf_scds)
> +{
> + if (hf_scds[8] & DRM_EDID_CNMVRR)
> + hdmi_vrr->cnm_vrr = true;
> + if (hf_scds[8] & DRM_EDID_CINEMA_VRR)
> + hdmi_vrr->cinema_vrr = true;
> + if (hf_scds[8] & DRM_EDID_MDELTA)
> + hdmi_vrr->m_delta = true;
> +
> + hdmi_vrr->vrr_min = hf_scds[9] & DRM_EDID_VRR_MIN_MASK;
> + hdmi_vrr->vrr_max = (hf_scds[9] & DRM_EDID_VRR_MAX_UPPER_MASK) << 2;
> + hdmi_vrr->vrr_max |= hf_scds[10] & DRM_EDID_VRR_MAX_LOWER_MASK;
> +}
> +
>  static void drm_parse_dsc_info(struct drm_hdmi_dsc_cap *hdmi_dsc,
>  const u8 *hf_scds)
>  {
> @@ -5901,9 +5916,11 @@ static void drm_parse_hdmi_forum_scds(struct 
> drm_connector *connector,
>   struct drm_display_info *display = >display_info;
>   struct drm_hdmi_info *hdmi = >hdmi;
>   struct drm_hdmi_dsc_cap *hdmi_dsc = >dsc_cap;
> + struct drm_hdmi_vrr_cap *hdmi_vrr = >vrr_cap;
>   int max_tmds_clock = 0;
>   u8 max_frl_rate = 0;
>   bool dsc_support = false;
> + bool vrr_support = false;
>  
>   display->has_hdmi_infoframe = true;
>  
> @@ -5949,14 +5966,19 @@ static void drm_parse_hdmi_forum_scds(struct 
> drm_connector *connector,
>  
>   drm_parse_ycbcr420_deep_color_info(connector, hf_scds);
>  
> + if (cea_db_payload_len(hf_scds) >= 8 && hf_scds[8]) {
> + drm_parse_vrr_info(hdmi_vrr, hf_scds);
> + vrr_support = true;
> + }
> +
>   if (cea_db_payload_len(hf_scds) >= 11 && hf_scds[11]) {
>   drm_parse_dsc_info(hdmi_dsc, hf_scds);
>   dsc_support = true;
>   }
>  
>   drm_dbg_kms(connector->dev,
> - "HF-VSDB: max TMDS clock: %d KHz, HDMI 2.1 support: %s, DSC 
> 1.2 support: %s\n",
> - max_tmds_clock, str_yes_no(max_frl_rate), 
> str_yes_no(dsc_support));
> + "HF-VSDB: max TMDS clock: %d KHz, HDMI 2.1 support: %s, VRR 
> support: %s, DSC 1.2 support: %s\n",
> + max_tmds_clock, str_yes_no(max_frl_rate), 
> str_yes_no(vrr_support), str_yes_no(dsc_support));
>  }
>  
>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index b1b2df48d42c..ec6ef71ab5cd 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -219,6 +219,30 @@ struct drm_hdmi_dsc_cap {
>   u8 total_chunk_kbytes;
>  };
>  
> +
> +/**
> + * struct drm_hdmi_vrr_cap - VRR capabilities of HDMI sink
> + * Describes the VRR support provided by HDMI 2.1 sink.
> + * The information is fetched fom additional HFVSDB blocks defined
> + * for HDMI 2.1.
> + */
> +struct drm_hdmi_vrr_cap {
> + /** @cnm_vrr: sink supports negative Mvrr values*/
> + bool cnm_vrr;
> +
> + /** @cinema_vrr: sink supports fractional and integer media rates < 
> VRRmin*/
> + bool cinema_vrr;
> +
> + /** @m_delta: sink can anticipate and compensate for frame-to-frame 
> variation in Mvrr */
> + bool m_delta;
> +
> + /** @vrr_min: VRRmin - lowest framerate in Hz that sink can support in 
> VRR */
> + u8 vrr_min;
> +
> + /** @vrr_max: VRRmax - highest framerate in Hz that sink can support in 
> VRR */
> + u16 vrr_max;
> +};
> +
>  /**
>   * struct drm_hdmi_info - runtime information about the connected HDMI sink
>   *
> @@ -259,6 +283,9 @@ struct drm_hdmi_info {
>  
>   /** @dsc_cap: DSC capabilities of the sink */
>   struct drm_hdmi_dsc_cap dsc_cap;
> +
> + /** @vrr_cap: VRR capabilities of the sink */
> + struct drm_hdmi_vrr_cap vrr_cap;
>  };
>  
>  /**
> -- 
> 2.25.1
> 

Re: [PATCH 03/11] drm/edid: s/monitor_rage/vrr_range/

[Navare, Manasi]

On Sat, Aug 27, 2022 at 12:34:53AM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Rename info->monitor_range to info->vrr_range to actually
> reflect its usage.

Okay makes sense.

Reviewed-by: Manasi Navare 

Manasi

> 
> Cc: Manasi Navare 
> Cc: Nicholas Kazlauskas 
> Cc: Harry Wentland 
> Cc: Leo Li 
> Cc: Rodrigo Siqueira 
> Cc: amd-...@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä 
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 12 -
>  drivers/gpu/drm/drm_debugfs.c |  4 +--
>  drivers/gpu/drm/drm_edid.c| 26 +--
>  drivers/gpu/drm/i915/display/intel_vrr.c  |  6 ++---
>  include/drm/drm_connector.h   |  4 +--
>  5 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index e702f0d72d53..928b5b6541db 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -9921,8 +9921,8 @@ void amdgpu_dm_update_freesync_caps(struct 
> drm_connector *connector,
>   amdgpu_dm_connector->min_vfreq = 0;
>   amdgpu_dm_connector->max_vfreq = 0;
>   amdgpu_dm_connector->pixel_clock_mhz = 0;
> - connector->display_info.monitor_range.min_vfreq = 0;
> - connector->display_info.monitor_range.max_vfreq = 0;
> + connector->display_info.vrr_range.min_vfreq = 0;
> + connector->display_info.vrr_range.max_vfreq = 0;
>   freesync_capable = false;
>  
>   goto update;
> @@ -9970,8 +9970,8 @@ void amdgpu_dm_update_freesync_caps(struct 
> drm_connector *connector,
>   amdgpu_dm_connector->pixel_clock_mhz =
>   range->pixel_clock_mhz * 10;
>  
> - connector->display_info.monitor_range.min_vfreq 
> = range->min_vfreq;
> - connector->display_info.monitor_range.max_vfreq 
> = range->max_vfreq;
> + connector->display_info.vrr_range.min_vfreq = 
> range->min_vfreq;
> + connector->display_info.vrr_range.max_vfreq = 
> range->max_vfreq;
>  
>   break;
>   }
> @@ -9993,8 +9993,8 @@ void amdgpu_dm_update_freesync_caps(struct 
> drm_connector *connector,
>   if (amdgpu_dm_connector->max_vfreq - 
> amdgpu_dm_connector->min_vfreq > 10)
>   freesync_capable = true;
>  
> - connector->display_info.monitor_range.min_vfreq = 
> vsdb_info.min_refresh_rate_hz;
> - connector->display_info.monitor_range.max_vfreq = 
> vsdb_info.max_refresh_rate_hz;
> + connector->display_info.vrr_range.min_vfreq = 
> vsdb_info.min_refresh_rate_hz;
> + connector->display_info.vrr_range.max_vfreq = 
> vsdb_info.max_refresh_rate_hz;
>   }
>   }
>  
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 01ee3febb813..1437c798b122 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -377,8 +377,8 @@ static int vrr_range_show(struct seq_file *m, void *data)
>   if (connector->status != connector_status_connected)
>   return -ENODEV;
>  
> - seq_printf(m, "Min: %u\n", 
> connector->display_info.monitor_range.min_vfreq);
> - seq_printf(m, "Max: %u\n", 
> connector->display_info.monitor_range.max_vfreq);
> + seq_printf(m, "Min: %u\n", connector->display_info.vrr_range.min_vfreq);
> + seq_printf(m, "Max: %u\n", connector->display_info.vrr_range.max_vfreq);
>  
>   return 0;
>  }
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index ac662495635c..4355d73632c3 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6020,11 +6020,11 @@ static void drm_parse_cea_ext(struct drm_connector 
> *connector,
>  }
>  
>  static
> -void get_monitor_range(const struct detailed_timing *timing, void *c)
> +void get_vrr_range(const struct detailed_timing *timing, void *c)
>  {
>   struct detailed_mode_closure *closure = c;
>   struct drm_display_info *info = >connector->display_info;
> - struct drm_monitor_range_info *monitor_range = >monitor_range;
> + struct drm_monitor_range_info *vrr_range = >vrr_range;
>   const struct

Re: [PATCH 02/11] drm/edid: Clarify why we only accept the "range limits only" descriptor

[Navare, Manasi]

On Sat, Aug 27, 2022 at 12:34:52AM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> The current comment fails to clarify why we only accept
> the "range limits only" variant of the range descriptor.
> Reword it to make some actual sense.
>

Thanks Ville for adding this description for monitor_range

Reviewed-by: Manasi Navare 

Manasi

> Cc: Manasi Navare 
> Cc: Nicholas Kazlauskas 
> Cc: Harry Wentland 
> Cc: Leo Li 
> Cc: Rodrigo Siqueira 
> Cc: amd-...@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/drm_edid.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 4005dab6147d..ac662495635c 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6033,10 +6033,13 @@ void get_monitor_range(const struct detailed_timing 
> *timing, void *c)
>   return;
>  
>   /*
> -  * Check for flag range limits only. If flag == 1 then
> -  * no additional timing information provided.
> -  * Default GTF, GTF Secondary curve and CVT are not
> -  * supported
> +  * These limits are used to determine the VRR refresh
> +  * rate range. Only the "range limits only" variant
> +  * of the range descriptor seems to guarantee that
> +  * any and all timings are accepted by the sink, as
> +  * opposed to just timings conforming to the indicated
> +  * formula (GTF/GTF2/CVT). Thus other variants of the
> +  * range descriptor are not accepted here.
>*/
>   if (range->flags != DRM_EDID_RANGE_LIMITS_ONLY_FLAG)
>   return;
> -- 
> 2.35.1
> 

Re: [PATCH 01/11] drm/edid: Handle EDID 1.4 range descriptor h/vfreq offsets

[Navare, Manasi]

On Sat, Aug 27, 2022 at 12:34:51AM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> EDID 1.4 introduced some extra flags in the range
> descriptor to support min/max h/vfreq >= 255. Consult them
> to correctly parse the vfreq limits.
> 
> Note that some combinations of the flags are documented
> as "reserved" (as are some other values in the descriptor)
> but explicitly checking for those doesn't seem particularly
> worthwile since we end up with bogus results whether we
> decode them or not.
> 
> v2: Increase the storage to u16 to make it work (Jani)
> Note the "reserved" values situation (Jani)
> v3: Document the EDID version number in the defines
> Drop some bogus (u8) casts
> 
> Cc: sta...@vger.kernel.org
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6519
> Reviewed-by: Jani Nikula 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/drm_debugfs.c |  4 ++--
>  drivers/gpu/drm/drm_edid.c| 24 ++--
>  include/drm/drm_connector.h   |  4 ++--
>  include/drm/drm_edid.h|  5 +
>  4 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 493922069c90..01ee3febb813 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -377,8 +377,8 @@ static int vrr_range_show(struct seq_file *m, void *data)
>   if (connector->status != connector_status_connected)
>   return -ENODEV;
>  
> - seq_printf(m, "Min: %u\n", 
> (u8)connector->display_info.monitor_range.min_vfreq);
> - seq_printf(m, "Max: %u\n", 
> (u8)connector->display_info.monitor_range.max_vfreq);
> + seq_printf(m, "Min: %u\n", 
> connector->display_info.monitor_range.min_vfreq);
> + seq_printf(m, "Max: %u\n", 
> connector->display_info.monitor_range.max_vfreq);
>  
>   return 0;
>  }
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 90a5e26eafa8..4005dab6147d 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6020,12 +6020,14 @@ static void drm_parse_cea_ext(struct drm_connector 
> *connector,
>  }
>  
>  static
> -void get_monitor_range(const struct detailed_timing *timing,
> -void *info_monitor_range)
> +void get_monitor_range(const struct detailed_timing *timing, void *c)
>  {
> - struct drm_monitor_range_info *monitor_range = info_monitor_range;
> + struct detailed_mode_closure *closure = c;
> + struct drm_display_info *info = >connector->display_info;
> + struct drm_monitor_range_info *monitor_range = >monitor_range;
>   const struct detailed_non_pixel *data = >data.other_data;
>   const struct detailed_data_monitor_range *range = >data.range;
> + const struct edid *edid = closure->drm_edid->edid;
>  
>   if (!is_display_descriptor(timing, EDID_DETAIL_MONITOR_RANGE))
>   return;
> @@ -6041,18 +6043,28 @@ void get_monitor_range(const struct detailed_timing 
> *timing,
>  
>   monitor_range->min_vfreq = range->min_vfreq;
>   monitor_range->max_vfreq = range->max_vfreq;
> +
> + if (edid->revision >= 4) {
> + if (data->pad2 & DRM_EDID_RANGE_OFFSET_MIN_VFREQ)
> + monitor_range->min_vfreq += 255;
> +         if (data->pad2 & DRM_EDID_RANGE_OFFSET_MAX_VFREQ)
> + monitor_range->max_vfreq += 255;

Yes this makes sense. This looks like added for supporting HRR (high
refresh rate) panels.
Do you think we should mention that in the commit message or in the
comment in the code itself?

Other than that looks good to me:

Reviewed-by: Manasi Navare 

Manasi

> + }
>  }
>  
>  static void drm_get_monitor_range(struct drm_connector *connector,
> const struct drm_edid *drm_edid)
>  {
> - struct drm_display_info *info = >display_info;
> + const struct drm_display_info *info = >display_info;
> + struct detailed_mode_closure closure = {
> + .connector = connector,
> + .drm_edid = drm_edid,
> + };
>  
>   if (!version_greater(drm_edid, 1, 1))
>   return;
>  
> - drm_for_each_detailed_block(drm_edid, get_monitor_range,
> - >monitor_range);
> + drm_for_each_detailed_block(drm_edid, get_monitor_range, );
>  
>   DRM_DEBUG_KMS("Supported Monitor Refresh rate range is %d Hz - %d Hz\n",
> info->monitor_range.min_vfreq,
> diff --git a/include/drm/drm_connector.h b/in

Re: [PATCH 0/2] Add DP MST DSC support to i915

[Navare, Manasi]

On Thu, Aug 11, 2022 at 10:33:51AM +0300, Lisovskiy, Stanislav wrote:
> On Wed, Aug 10, 2022 at 04:02:08PM -0400, Lyude Paul wrote:
> > Btw, what's the plan for this? Figured I'd ask since I noticed this on the 
> > ML,
> > nd I'm now finishing up getting the atomic only MST patches I've been 
> > working
> > on merged :)
> 
> Current plan is that I need to fix this, as current implementation doesn't
> seem to work because of my wrong assumption that drm_dp_mst_find_vcpi_slots
> will fail if no slots are available and then we can fallback to DSC.
> 
> In reality that function can return whatever bogus value it wants, like
> 71 slots, while you have only 63 available. The real check is done in
> drm_dp_mst_atomic_check, which would of course reject that configuration,
> however by that moment its going to be too late for swithcing to DSC.
> 
> So looke like I will have to move that check at least partly to where DSC/no 
> DSC decision is done. However if there are multiple displays we get
> another problem, lets say we have 2 displays requiring 40 vcpi slots each in 
> DSC
> mode with certain input bpp.
> We have now either option to reject the whole config or go back and try with
> another bpp to check if we can reduce amount of slots.
> Because by default we choose the first one which fits, however by the time 
> when 
> compute_config is called, we still don't have all config computed, which might
> lead to that last crtc can either run our of vcpi slots or we will have to 
> go back and try recalculating with higher compression ratio.
> 
> My other question was that DSC was supposed to be "visually" lossless, 
> wondering
> why we are still trying with different bpps? Could have just set highest
> compression ratio right away.

We do want to still maintain as high pic quality as possible. So we try
to find out the min compression ration that will make the mode fit into
the available link BW.

Manasi

> 
> So need to sort this out first before floating new series.
> 
> Stan
> 
> > 
> > On Wed, 2022-08-10 at 11:17 +0300, Stanislav Lisovskiy wrote:
> > > Currently we have only DSC support for DP SST.
> > > 
> > > Stanislav Lisovskiy (2):
> > >   drm: Add missing DP DSC extended capability definitions.
> > >   drm/i915: Add DSC support to MST path
> > > 
> > >  drivers/gpu/drm/i915/display/intel_dp.c |  76 +-
> > >  drivers/gpu/drm/i915/display/intel_dp.h |  17 +++
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 145 
> > >  include/drm/display/drm_dp.h|  10 +-
> > >  4 files changed, 203 insertions(+), 45 deletions(-)
> > > 
> > 
> > -- 
> > Cheers,
> >  Lyude Paul (she/her)
> >  Software Engineer at Red Hat
> > 

Re: [RFC V3 2/2] drm/i915/vrr: Set drm crtc vrr_enabled property

[Navare, Manasi]

On Tue, May 17, 2022 at 12:56:36PM +0530, Bhanuprakash Modem wrote:
> This function sets the vrr_enabled property for crtc based
> on the platform support and the request from userspace.
> 
> V2: Check for platform support before updating the prop.
> V3: Don't attach vrr_enabled prop, as it is alreay attached.
> 
> Cc: Ville Syrjälä 
> Cc: Manasi Navare 
> Signed-off-by: Bhanuprakash Modem 
> ---
>  drivers/gpu/drm/i915/display/intel_vrr.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c 
> b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 396f2f994fa0..7263b35550de 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -160,6 +160,10 @@ void intel_vrr_enable(struct intel_encoder *encoder,
>   enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>   u32 trans_vrr_ctl;
>  
> + if (HAS_VRR(dev_priv))
> + drm_mode_crtc_set_vrr_enabled_property(crtc_state->uapi.crtc,
> +crtc_state->vrr.enable);

But here if userspace has accidently set the CRTC vrr enabled prop to
true without cheking VRR capabile prop, this will be true and here the
driver will still not
reset it. If that is the purpose of adding this then we should infact
set this to false if !HAS_VRR

Manasi

> +
>   if (!crtc_state->vrr.enable)
>   return;
>  
> @@ -211,6 +215,10 @@ void intel_vrr_disable(const struct intel_crtc_state 
> *old_crtc_state)
>   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>   enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
>  
> + if (HAS_VRR(dev_priv))
> + 
> drm_mode_crtc_set_vrr_enabled_property(old_crtc_state->uapi.crtc,
> +false);
> +
>   if (!old_crtc_state->vrr.enable)
>   return;
>  
> -- 
> 2.35.1
> 

Re: [RFC V3 1/2] drm/vrr: Attach vrr_enabled property to the drm crtc

[Navare, Manasi]

On Tue, May 17, 2022 at 12:56:35PM +0530, Bhanuprakash Modem wrote:
> Modern display hardware is capable of supporting variable refresh rates.
> This patch introduces helpers to attach and set "vrr_enabled" property
> on the crtc to allow userspace to query VRR enabled status on that crtc.
> 
> Atomic drivers should attach this property to crtcs those are capable of
> driving variable refresh rates using
> drm_mode_crtc_attach_vrr_enabled_property().

We are not attaching the prop anymore, please remove this from the
commit message.

> 
> The value should be updated based on driver and hardware capability
> by using drm_mode_crtc_set_vrr_enabled_property().
> 
> V2: Use property flag as atomic
> V3: Drop helper to attach vrr_enabled prop, since it is already
> attached (Manasi)
> 
> Cc: Ville Syrjälä 
> Cc: Nicholas Kazlauskas 
> Cc: Manasi Navare 
> Cc: Harry Wentland 
> Signed-off-by: Bhanuprakash Modem 
> ---
>  drivers/gpu/drm/drm_crtc.c| 26 ++
>  drivers/gpu/drm/drm_mode_config.c |  2 +-
>  include/drm/drm_crtc.h|  3 +++
>  3 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 26a77a735905..8bb8b4bf4199 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -239,6 +239,9 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc 
> *crtc)
>   *   Driver's default scaling filter
>   *   Nearest Neighbor:
>   *   Nearest Neighbor scaling filter
> + * VRR_ENABLED:
> + *   Atomic property for setting the VRR state of the CRTC.
> + *   To enable the VRR on CRTC, user-space must set this property to 1.

This prop was primarily a userspace Write only and driver read only
property which would be used only by the userspace to request VRR on
that CRTC,

Are we now modifying this to be used as a bidirectional property to also
indicate the status of VRR on that CRTC which will be updated by the
driver?

We need to add this accordingly and update the DRM documentation and
also get acks from other vendors since AMD and other folks mght be using
this as a write only prop.

Manasi

>   */
>  
>  __printf(6, 0)
> @@ -883,3 +886,26 @@ int drm_crtc_create_scaling_filter_property(struct 
> drm_crtc *crtc,
>   return 0;
>  }
>  EXPORT_SYMBOL(drm_crtc_create_scaling_filter_property);
> +
> +/**
> + * drm_mode_crtc_set_vrr_enabled_property - sets the vrr enabled property for
> + * a crtc.
> + * @crtc: drm CRTC
> + * @vrr_enabled: True to enable the VRR on CRTC
> + *
> + * Should be used by atomic drivers to update the VRR enabled status on a 
> CRTC
> + */
> +void drm_mode_crtc_set_vrr_enabled_property(struct drm_crtc *crtc,
> + bool vrr_enabled)
> +{
> + struct drm_device *dev = crtc->dev;
> + struct drm_mode_config *config = >mode_config;
> +
> + if (!config->prop_vrr_enabled)
> + return;
> +
> + drm_object_property_set_value(>base,
> +   config->prop_vrr_enabled,
> +   vrr_enabled);
> +}
> +EXPORT_SYMBOL(drm_mode_crtc_set_vrr_enabled_property);
> diff --git a/drivers/gpu/drm/drm_mode_config.c 
> b/drivers/gpu/drm/drm_mode_config.c
> index 37b4b9f0e468..b7cde73d5586 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -323,7 +323,7 @@ static int drm_mode_create_standard_properties(struct 
> drm_device *dev)
>   return -ENOMEM;
>   dev->mode_config.prop_mode_id = prop;
>  
> - prop = drm_property_create_bool(dev, 0,
> + prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
>   "VRR_ENABLED");
>   if (!prop)
>   return -ENOMEM;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index a70baea0636c..906787398f40 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1333,4 +1333,7 @@ static inline struct drm_crtc *drm_crtc_find(struct 
> drm_device *dev,
>  int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
>   unsigned int supported_filters);
>  
> +void drm_mode_crtc_set_vrr_enabled_property(struct drm_crtc *crtc,
> + bool vrr_enabled);
> +
>  #endif /* __DRM_CRTC_H__ */
> -- 
> 2.35.1
> 

Re: [RFC v2 1/2] drm/vrr: Attach vrr_enabled property to the drm crtc

[Navare, Manasi]

On Mon, Apr 25, 2022 at 12:16:11PM +0530, Bhanuprakash Modem wrote:
> Modern display hardware is capable of supporting variable refresh rates.
> This patch introduces helpers to attach and set "vrr_enabled" property
> on the crtc to allow userspace to query VRR enabled status on that crtc.
> 
> Atomic drivers should attach this property to crtcs those are capable of
> driving variable refresh rates using
> drm_mode_crtc_attach_vrr_enabled_property().
> 
> The value should be updated based on driver and hardware capability
> by using drm_mode_crtc_set_vrr_enabled_property().
> 
> V2: Use property flag as atomic

We already have userspace making us of the CRTC vrr_enabled property to
enable VRR for the CRTC like in case of full screen gaming.

This can already be done through:
drm_atomic_crtc_set_property call. Why do we need additonal helpers
for setting the same per CRTC property?

This is a default CRTC property so it will be created annd attached for
CRTC as per the DRM doc:
"VRR_ENABLED":
 *  Default _crtc boolean property that notifies the driver that the
 *  content on the CRTC is suitable for variable refresh rate presentation.
 *  The driver will take this property as a hint to enable variable
 *  refresh rate support if the receiver supports it, ie. if the
 *  "vrr_capable" property is true on the _connector object. The
 *  vertical front porch duration will be extended until page-flip or
 *  timeout when enabled.

Then we can use the atomic_crtc_set/get_property helpers to set it
Am I missing some other use case here?

Manasi

> 
> Cc: Ville Syrjälä 
> Cc: Nicholas Kazlauskas 
> Cc: Manasi Navare 
> Cc: Harry Wentland 
> Signed-off-by: Bhanuprakash Modem 
> ---
>  drivers/gpu/drm/drm_crtc.c| 44 +++
>  drivers/gpu/drm/drm_mode_config.c |  2 +-
>  include/drm/drm_crtc.h|  4 +++
>  3 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 26a77a735905..95b4a0c7ecb3 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -883,3 +883,47 @@ int drm_crtc_create_scaling_filter_property(struct 
> drm_crtc *crtc,
>   return 0;
>  }
>  EXPORT_SYMBOL(drm_crtc_create_scaling_filter_property);
> +
> +/**
> + * drm_mode_crtc_attach_vrr_enabled_property - attaches the vrr_enabled 
> property
> + * @crtc: drm CRTC to attach the vrr_enabled property on.
> + *
> + * This is used by atomic drivers to add support for querying
> + * VRR enabled status for a crtc.
> + */
> +void drm_mode_crtc_attach_vrr_enabled_property(struct drm_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->dev;
> + struct drm_mode_config *config = >mode_config;
> +
> + if (!config->prop_vrr_enabled)
> + return;
> +
> + drm_object_attach_property(>base,
> +config->prop_vrr_enabled,
> +0);
> +}
> +EXPORT_SYMBOL(drm_mode_crtc_attach_vrr_enabled_property);
> +
> +/**
> + * drm_mode_crtc_set_vrr_enabled_property - sets the vrr enabled property for
> + * a crtc.
> + * @crtc: drm CRTC
> + * @vrr_enabled: True to enable the VRR on CRTC
> + *
> + * Should be used by atomic drivers to update the VRR enabled status on a 
> CRTC
> + */
> +void drm_mode_crtc_set_vrr_enabled_property(struct drm_crtc *crtc,
> + bool vrr_enabled)
> +{
> + struct drm_device *dev = crtc->dev;
> + struct drm_mode_config *config = >mode_config;
> +
> + if (!config->prop_vrr_enabled)
> + return;
> +
> + drm_object_property_set_value(>base,
> +   config->prop_vrr_enabled,
> +   vrr_enabled);
> +}
> +EXPORT_SYMBOL(drm_mode_crtc_set_vrr_enabled_property);
> diff --git a/drivers/gpu/drm/drm_mode_config.c 
> b/drivers/gpu/drm/drm_mode_config.c
> index 37b4b9f0e468..b7cde73d5586 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -323,7 +323,7 @@ static int drm_mode_create_standard_properties(struct 
> drm_device *dev)
>   return -ENOMEM;
>   dev->mode_config.prop_mode_id = prop;
>  
> - prop = drm_property_create_bool(dev, 0,
> + prop = drm_property_create_bool(dev, DRM_MODE_PROP_ATOMIC,
>   "VRR_ENABLED");
>   if (!prop)
>   return -ENOMEM;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index a70baea0636c..bde657cb0f9e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1333,4 +1333,8 @@ static inline struct drm_crtc *drm_crtc_find(struct 
> drm_device *dev,
>  int drm_crtc_create_scaling_filter_property(struct drm_crtc *crtc,
>   unsigned int supported_filters);
>  
> +void drm_mode_crtc_attach_vrr_enabled_property(struct drm_crtc *crtc);
> +void drm_mode_crtc_set_vrr_enabled_property(struct drm_crtc *crtc,

RE: [PATCH 1/3] drm: New function to get luminance range based on static hdr metadata

[Navare, Manasi D]

Yes I agree that this data parsed from EDID/Display ID should be stored in 
structs defined in drm_display_info.
Like for the VRR range that we parse from EDID, we store that in a struct 
monitor_range in drm_display_info.

Manasi

-Original Message-
From: dri-devel  On Behalf Of Jani 
Nikula
Sent: Tuesday, April 26, 2022 6:38 AM
To: Hogander, Jouni ; dri-devel@lists.freedesktop.org
Cc: Hogander, Jouni ; Rodrigo Siqueira 
; Kahola, Mika 
Subject: Re: [PATCH 1/3] drm: New function to get luminance range based on 
static hdr metadata

On Tue, 26 Apr 2022, Jouni Högander  wrote:
> Split luminance min/max calculation using static hdr metadata from 
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:update_connector_ext
> _caps
> into drm/drm_edid.c.

IMO all of this should be computed at EDID parsing time once and stored in the 
metadata. Maybe in drm_parse_hdr_metadata_block().

Over time we've added a bunch of functions to do this type of stuff, and all 
drivers call them at slightly different times and different ways, and it just 
grows complicated.

(Also, I think basically everything that comes out of the EDID or DisplayID 
should be stored in connector->display_info instead of being spread around like 
we currently do. But that's for another patch series, another time.)

BR,
Jani.

>
> Cc: Rodrigo Siqueira 
> Cc: Harry Wentland 
> Cc: Lyude Paul 
> Cc: Mika Kahola 
> Cc: Jani Nikula 
> Signed-off-by: Jouni Högander 
> ---
>  drivers/gpu/drm/drm_edid.c | 55 ++
>  include/drm/drm_edid.h |  4 +++
>  2 files changed, 59 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c 
> index 7a8482b75071..1cb886debbbe 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4005,6 +4005,61 @@ drm_display_mode_from_cea_vic(struct drm_device 
> *dev,  }  EXPORT_SYMBOL(drm_display_mode_from_cea_vic);
>  
> +/**
> + * drm_luminance_range_from_static_hdr_metadata() - luminance range 
> +from static hdr
> + * metadata
> + * @connector: connector we're calculating for
> + * @min: Calculated min value
> + * @max: Calculated max value
> + *
> + * Calculates backlight min and max as described in CTA-861-G
> + *
> + * Returns: True when calculation succeeds.
> + */
> +bool
> +drm_luminance_range_from_static_hdr_metadata(struct drm_connector *connector,
> +  u32 *min, u32 *max)
> +{
> + struct hdr_sink_metadata *hdr_metadata = >hdr_sink_metadata;
> + static const u8 pre_computed_values[] = {
> + 50, 51, 52, 53, 55, 56, 57, 58, 59, 61, 62, 63, 65, 66, 68, 69,
> + 71, 72, 74, 75, 77, 79, 81, 82, 84, 86, 88, 90, 92, 94, 96, 98};
> + u32 min_cll, max_cll, q, r;
> +
> + if (!(hdr_metadata->hdmi_type1.metadata_type &
> +   BIT(HDMI_STATIC_METADATA_TYPE1)))
> + return false;
> +
> + max_cll = hdr_metadata->hdmi_type1.max_cll;
> + min_cll = hdr_metadata->hdmi_type1.min_cll;
> +
> + /* From the specification (CTA-861-G), for calculating the maximum
> +  * luminance we need to use:
> +  *  Luminance = 50*2**(CV/32)
> +  * Where CV is a one-byte value.
> +  * For calculating this expression we may need float point precision;
> +  * to avoid this complexity level, we take advantage that CV is divided
> +  * by a constant. From the Euclids division algorithm, we know that CV
> +  * can be written as: CV = 32*q + r. Next, we replace CV in the
> +  * Luminance expression and get 50*(2**q)*(2**(r/32)), hence we just
> +  * need to pre-compute the value of r/32. For pre-computing the values
> +  * We just used the following Ruby line:
> +  *  (0...32).each {|cv| puts (50*2**(cv/32.0)).round}
> +  * The results of the above expressions can be verified at
> +  * pre_computed_values.
> +  */
> + q = max_cll >> 5;
> + r = max_cll % 32;
> + *max = (1 << q) * pre_computed_values[r];
> +
> + /* min luminance: maxLum * (CV/255)^2 / 100 */
> + q = DIV_ROUND_CLOSEST(min_cll, 255);
> + *min = *max * DIV_ROUND_CLOSEST((q * q), 100);
> +
> + return true;
> +}
> +EXPORT_SYMBOL(drm_luminance_range_from_static_hdr_metadata);
> +
>  static int
>  do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)  
> { diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 
> c3204a58fb09..63e441c84d72 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -406,6 +406,10 @@ drm_hdmi_avi_infoframe_quant_range(struct 
> hdmi_avi_infoframe *frame,
>  const struct drm_display_mode *mode,
>  enum hdmi_quantization_range 
> rgb_quant_range);
>  
> +bool
> +drm_luminance_range_from_static_hdr_metadata(struct drm_connector *connector,
> +  u32 *min, u32 *max);
> +
>  /**
>   * drm_eld_mnl - Get ELD monitor name length in bytes.
>   

Re: [PATCH v3] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true

[Navare, Manasi]

On Tue, Jan 18, 2022 at 06:34:20PM +0200, Ville Syrjälä wrote:
> On Fri, Oct 22, 2021 at 12:51:12PM -0700, Navare, Manasi wrote:
> > 
> > Hi Ville,
> > 
> > Could you take a look at this, this addresses teh review comments from prev 
> > version
> 
> I don't think I ever got an answer to my question as to whether this
> was tested with all the interesting scenarios:
> 1) just with the master crtc added by userspace into the commit
> 2) just with the slave crtc added by userspace into the commit
> 3) both crtcs added by userspace into the commit
> 
> I guess 1) has been tested since that happens all the time, but the other
> two scanarios would likely need to be done with a synthetic test to make
> sure we're actually hitting them.
> 
> I think it *should* work, but I'd like to have real proof of that.
> Reviewed-by: Ville Syrjälä 

Thank you for your review here Ville.
I have triggered a separate email thread to understand all the above testing 
scenarios and get them tested with bigjoiner IGT.

Manasi

> 
> > 
> > Manasi
> > 
> > On Mon, Oct 04, 2021 at 04:59:13AM -0700, Manasi Navare wrote:
> > > In case of a modeset where a mode gets split across mutiple CRTCs
> > > in the driver specific implementation (bigjoiner in i915) we wrongly count
> > > the affected CRTCs based on the drm_crtc_mask and indicate the stolen 
> > > CRTC as
> > > an affected CRTC in atomic_check_only().
> > > This triggers a warning since affected CRTCs doent match requested CRTC.
> > > 
> > > To fix this in such bigjoiner configurations, we should only
> > > increment affected crtcs if that CRTC is enabled in UAPI not
> > > if it is just used internally in the driver to split the mode.
> > > 
> > > v3: Add the same uapi crtc_state->enable check in requested
> > > crtc calc (Ville)
> > > 
> > > Cc: Ville Syrjälä 
> > > Cc: Simon Ser 
> > > Cc: Pekka Paalanen 
> > > Cc: Daniel Stone 
> > > Cc: Daniel Vetter 
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: Manasi Navare 
> > > ---
> > >  drivers/gpu/drm/drm_atomic.c | 12 
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index ff1416cd609a..a1e4c7905ebb 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1310,8 +1310,10 @@ int drm_atomic_check_only(struct drm_atomic_state 
> > > *state)
> > >  
> > >   DRM_DEBUG_ATOMIC("checking %p\n", state);
> > >  
> > > - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > > - requested_crtc |= drm_crtc_mask(crtc);
> > > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > + if (new_crtc_state->enable)
> > > + requested_crtc |= drm_crtc_mask(crtc);
> > > + }
> > >  
> > >   for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
> > > new_plane_state, i) {
> > >   ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
> > > @@ -1360,8 +1362,10 @@ int drm_atomic_check_only(struct drm_atomic_state 
> > > *state)
> > >   }
> > >   }
> > >  
> > > - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > > - affected_crtc |= drm_crtc_mask(crtc);
> > > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > + if (new_crtc_state->enable)
> > > + affected_crtc |= drm_crtc_mask(crtc);
> > > + }
> > >  
> > >   /*
> > >* For commits that allow modesets drivers can add other CRTCs to the
> > > -- 
> > > 2.19.1
> > > 
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH v3] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true

[Navare, Manasi]

Hi Ville,

Could you take a look at this, this addresses teh review comments from prev 
version

Manasi

On Mon, Oct 04, 2021 at 04:59:13AM -0700, Manasi Navare wrote:
> In case of a modeset where a mode gets split across mutiple CRTCs
> in the driver specific implementation (bigjoiner in i915) we wrongly count
> the affected CRTCs based on the drm_crtc_mask and indicate the stolen CRTC as
> an affected CRTC in atomic_check_only().
> This triggers a warning since affected CRTCs doent match requested CRTC.
> 
> To fix this in such bigjoiner configurations, we should only
> increment affected crtcs if that CRTC is enabled in UAPI not
> if it is just used internally in the driver to split the mode.
> 
> v3: Add the same uapi crtc_state->enable check in requested
> crtc calc (Ville)
> 
> Cc: Ville Syrjälä 
> Cc: Simon Ser 
> Cc: Pekka Paalanen 
> Cc: Daniel Stone 
> Cc: Daniel Vetter 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Manasi Navare 
> ---
>  drivers/gpu/drm/drm_atomic.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index ff1416cd609a..a1e4c7905ebb 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1310,8 +1310,10 @@ int drm_atomic_check_only(struct drm_atomic_state 
> *state)
>  
>   DRM_DEBUG_ATOMIC("checking %p\n", state);
>  
> - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> - requested_crtc |= drm_crtc_mask(crtc);
> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> + if (new_crtc_state->enable)
> + requested_crtc |= drm_crtc_mask(crtc);
> + }
>  
>   for_each_oldnew_plane_in_state(state, plane, old_plane_state, 
> new_plane_state, i) {
>   ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
> @@ -1360,8 +1362,10 @@ int drm_atomic_check_only(struct drm_atomic_state 
> *state)
>   }
>   }
>  
> - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> - affected_crtc |= drm_crtc_mask(crtc);
> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> + if (new_crtc_state->enable)
> + affected_crtc |= drm_crtc_mask(crtc);
> + }
>  
>   /*
>* For commits that allow modesets drivers can add other CRTCs to the
> -- 
> 2.19.1
> 

RE: [PATCH v2] drm/amd/display: Only define DP 2.0 symbols if not already defined

[Navare, Manasi D]

We have merged such DRM definition dependencies previously through a topic 
branch in order to avoid redefining inside the driver.
But yes guarding this with ifdef is good.

Reviewed-by: Manasi Navare 

Manasi

-Original Message-
From: Zuo, Jerry  
Sent: Tuesday, September 28, 2021 11:11 PM
To: Wentland, Harry ; Deucher, Alexander 
; amd-...@lists.freedesktop.org
Cc: Nikula, Jani ; Li, Sun peng (Leo) 
; nat...@kernel.org; intel-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org; ville.syrj...@linux.intel.com; Navare, Manasi 
D ; Koenig, Christian ; 
Pan, Xinhui ; s...@canb.auug.org.au; 
linux-n...@vger.kernel.org; airl...@gmail.com; daniel.vet...@ffwll.ch; 
Wentland, Harry 
Subject: RE: [PATCH v2] drm/amd/display: Only define DP 2.0 symbols if not 
already defined

[AMD Official Use Only]

> -Original Message-
> From: Harry Wentland 
> Sent: September 28, 2021 1:08 PM
> To: Deucher, Alexander ; amd- 
> g...@lists.freedesktop.org; Zuo, Jerry 
> Cc: jani.nik...@intel.com; Li, Sun peng (Leo) ; 
> nat...@kernel.org; intel-...@lists.freedesktop.org; dri- 
> de...@lists.freedesktop.org; ville.syrj...@linux.intel.com; 
> manasi.d.nav...@intel.com; Koenig, Christian 
> ; Pan, Xinhui ; 
> s...@canb.auug.org.au; linux- n...@vger.kernel.org; airl...@gmail.com; 
> daniel.vet...@ffwll.ch; Wentland, Harry 
> Subject: [PATCH v2] drm/amd/display: Only define DP 2.0 symbols if not 
> already defined
>
> [Why]
> For some reason we're defining DP 2.0 definitions inside our driver. 
> Now that patches to introduce relevant definitions are slated to be 
> merged into drm- next this is causing conflicts.
>
> In file included from drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c:33:
> In file included
> from ./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu.h:70:
> In file included
> from ./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu_mode.h:36:
> ./include/drm/drm_dp_helper.h:1322:9: error:
> 'DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER' macro redefined [- 
> Werror,-Wmacro-redefined]
> ^
> ./drivers/gpu/drm/amd/amdgpu/../display/dc/dc_dp_types.h:881:9: note:
> previous definition is here
> ^
> 1 error generated.
>
> v2: Add one missing endif
>
> [How]
> Guard all display driver defines with #ifndef for now. Once we pull in 
> the new definitions into amd-staging-drm-next we will follow up and 
> drop definitions from our driver and provide follow-up header updates 
> for any addition DP
> 2.0 definitions required by our driver.
>
> Signed-off-by: Harry Wentland 

Reviewed-by: Fangzhi Zuo 

> ---
>  drivers/gpu/drm/amd/display/dc/dc_dp_types.h | 54
> ++--
>  1 file changed, 49 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_dp_types.h
> b/drivers/gpu/drm/amd/display/dc/dc_dp_types.h
> index a5e798b5da79..9de86ff5ef1b 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_dp_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_dp_types.h
> @@ -860,28 +860,72 @@ struct psr_caps {  };
>
>  #if defined(CONFIG_DRM_AMD_DC_DCN)
> +#ifndef DP_MAIN_LINK_CHANNEL_CODING_CAP
>  #define DP_MAIN_LINK_CHANNEL_CODING_CAP  0x006
> +#endif
> +#ifndef DP_SINK_VIDEO_FALLBACK_FORMATS
>  #define DP_SINK_VIDEO_FALLBACK_FORMATS   0x020
> +#endif
> +#ifndef DP_FEC_CAPABILITY_1
>  #define DP_FEC_CAPABILITY_1  0x091
> +#endif
> +#ifndef DP_DFP_CAPABILITY_EXTENSION_SUPPORT
>  #define DP_DFP_CAPABILITY_EXTENSION_SUPPORT  0x0A3
> +#endif
> +#ifndef DP_DSC_CONFIGURATION
>  #define DP_DSC_CONFIGURATION 0x161
> +#endif
> +#ifndef DP_PHY_SQUARE_PATTERN
>  #define DP_PHY_SQUARE_PATTERN0x249
> +#endif
> +#ifndef DP_128b_132b_SUPPORTED_LINK_RATES
>  #define DP_128b_132b_SUPPORTED_LINK_RATES0x2215
> +#endif
> +#ifndef DP_128b_132b_TRAINING_AUX_RD_INTERVAL
>  #define DP_128b_132b_TRAINING_AUX_RD_INTERVAL
>   0x2216
> +#endif
> +#ifndef DP_TEST_264BIT_CUSTOM_PATTERN_7_0
>  #define DP_TEST_264BIT_CUSTOM_PATTERN_7_00X2230
> +#endif
> +#ifndef DP_TEST_264BIT_CUSTOM_PATTERN_263_256
>  #define DP_TEST_264BIT_CUSTOM_PATTERN_263_256
>   0X2250
> +#endif
> +#ifndef DP_DSC_SUPPORT_AND_DECODER_COUNT
>  #define DP_DSC_SUPPORT_AND_DECODER_COUNT 0x2260
> +#endif
> +#ifndef DP_DSC_MAX_SLICE_COUNT_AND_AGGREGATION_0
>  #define DP_DSC_MAX_SLICE_COUNT_AND_AGGREGATION_0
>   0x2270
> -# define DP_DSC_DECODER_0_MAXIMUM_SLICE_COUNT_MASK   (1 <<
> 0)
> -# define DP_DSC_DECODER_0_AGGREGATION_SUPPORT_MASK
>   (0b111 << 1)
> -# define DP_DSC_DECODER_0_AGGREGATION_SUPPORT_SHIFT  1
> -# define DP_DSC_DECODER_COUNT_MASK   (

Re: [PATCH 52/53] drm/i915/dg2: Update to bigjoiner path

[Navare, Manasi]

On Thu, Jul 01, 2021 at 01:24:26PM -0700, Matt Roper wrote:
> From: Animesh Manna 
> 
> In verify_mpllb_state() encoder is retrieved from best_encoder
> of connector_state. As there will be only one connector_state
> for bigjoiner and checking encoder may not be needed for
> bigjoiner-slave. This code path related to mpll is done on dg2
> and need this fix to avoid null pointer dereference issue.
> 
> Cc: Manasi Navare 
> Signed-off-by: Animesh Manna 
> Signed-off-by: Matt Roper 

Reviewed-by: Manasi Navare 

Manasi

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 9655f1b1b41b..3f4e811145b6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -9153,6 +9153,9 @@ verify_mpllb_state(struct intel_atomic_state *state,
>   if (!new_crtc_state->hw.active)
>   return;
>  
> + if (new_crtc_state->bigjoiner_slave)
> + return;
> +
>   encoder = intel_get_crtc_new_encoder(state, new_crtc_state);
>   intel_mpllb_readout_hw_state(encoder, _hw_state);
>  
> -- 
> 2.25.4
> 

Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true

[Navare, Manasi]

On Fri, Mar 26, 2021 at 06:15:22PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 25, 2021 at 03:01:29PM -0700, Navare, Manasi wrote:
> > On Fri, Mar 19, 2021 at 11:27:59PM +0200, Ville Syrjälä wrote:
> > > On Fri, Mar 19, 2021 at 02:26:24PM -0700, Navare, Manasi wrote:
> > > > On Fri, Mar 19, 2021 at 11:12:41PM +0200, Ville Syrjälä wrote:
> > > > > On Fri, Mar 19, 2021 at 01:54:13PM -0700, Navare, Manasi wrote:
> > > > > > On Fri, Mar 19, 2021 at 04:56:24PM +0200, Ville Syrjälä wrote:
> > > > > > > On Thu, Mar 18, 2021 at 04:01:26PM -0700, Navare, Manasi wrote:
> > > > > > > > So basically we see this warning only in case of bigjoiner when
> > > > > > > > drm_atomic_check gets called without setting the 
> > > > > > > > state->allow_modeset flag.
> > > > > > > 
> > > > > > > Considering the code is 'WARN(!state->allow_modeset, ...' that
> > > > > > > fact should be rather obvious.
> > > > > > > 
> > > > > > > > 
> > > > > > > > So do you think that in i915, in intel_atomic_check_bigjoiner() 
> > > > > > > > we should only
> > > > > > > > steal the crtc when allow_modeset flag is set in state?
> > > > > > > 
> > > > > > > No. If you fully read drm_atomic_check_only() you will observe
> > > > > > > that it will reject any commit w/ allow_modeset==false which 
> > > > > > > needs a modeset. And it does that before the WARN.
> > > > > > > 
> > > > > > > So you're barking up the wrong tree here. The problem I think
> > > > > > > is that you're just computing requested_crtcs wrong.
> > > > > > 
> > > > > > So here in this case, requested CRTC = 0x1 since it requests 
> > > > > > modeset on CRTC 0
> > > > > > Now in teh atomic check, it steals the slave CRTC 1 and hence 
> > > > > > affected CRTC comes out
> > > > > > as 0x3 and hence the mismatch.
> > > > > 
> > > > > Hmm. How can it be 0x3 if we filtered out the uapi.enable==false case?
> > > > > 
> > > > 
> > > > Yes if I add that condition like in this patch then it correctly 
> > > > calculates
> > > > the affected crtc bitmask as only 0x1 since it doesnt include the slave 
> > > > crtc.
> > > > So with this patch, requested crtc = 0x 1, affected crtc = 0x1
> > > > 
> > > > If this looks good then this fixes our bigjoiner warnings.
> > > > Does this patch look good to you as is then?
> > > 
> > > I think you still need to fix the requested_crtcs calculation.
> > 
> > We calculate requested crtc at the beginning :
> > for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > requested_crtc |= drm_crtc_mask(crtc);
> > 
> > Are you suggesting adding this to after:
> >  if (config->funcs->atomic_check) {
> > ret = config->funcs->atomic_check(state->dev, state);
> > 
> > if (ret) {
> > DRM_DEBUG_ATOMIC("atomic driver check for %p 
> > failed: %d\n",
> >  state, ret);
> > return ret;
> > }
> > requested_crtc |= drm_crtc_mask(crtc);// Here it will have 
> > requested crtc = 0x11
> > }
> > 
> > in this case here the state should already have master crtc 0 and slave 
> > crtc 1
> > and that requested crtc should already be 0x11
> > 
> > Then in that case we dont need any special check for calculating affected 
> > crtc, that also will be 0x11
> 
> All I'm saying is that you're currently calculating requested_crtcs and
> affected_crtcs differently. So I'm not at all surprised that they might
> not match.
>

I dont get your point yet.
requested crtc is calculated before the atomic check call and we dont check for 
crtc uapi.enable to be true.
And hence requested crtc  = CRTC 0 = 0x2
After I added the check in this patch where affected crtc will include only the 
crtcs that have uapi.enable = true
then  it perfectly matches the requested crtc and return 0x2 but without this 
check when the calculation of
requested and affected crtc is the same is where we see the affected crtc = 
CRTC 0 and 1 = 0x3

So when the calculation is different infcat we dont see the mismatch

What is your point here?

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

Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true

[Navare, Manasi]

On Fri, Mar 19, 2021 at 11:27:59PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 19, 2021 at 02:26:24PM -0700, Navare, Manasi wrote:
> > On Fri, Mar 19, 2021 at 11:12:41PM +0200, Ville Syrjälä wrote:
> > > On Fri, Mar 19, 2021 at 01:54:13PM -0700, Navare, Manasi wrote:
> > > > On Fri, Mar 19, 2021 at 04:56:24PM +0200, Ville Syrjälä wrote:
> > > > > On Thu, Mar 18, 2021 at 04:01:26PM -0700, Navare, Manasi wrote:
> > > > > > So basically we see this warning only in case of bigjoiner when
> > > > > > drm_atomic_check gets called without setting the 
> > > > > > state->allow_modeset flag.
> > > > > 
> > > > > Considering the code is 'WARN(!state->allow_modeset, ...' that
> > > > > fact should be rather obvious.
> > > > > 
> > > > > > 
> > > > > > So do you think that in i915, in intel_atomic_check_bigjoiner() we 
> > > > > > should only
> > > > > > steal the crtc when allow_modeset flag is set in state?
> > > > > 
> > > > > No. If you fully read drm_atomic_check_only() you will observe
> > > > > that it will reject any commit w/ allow_modeset==false which 
> > > > > needs a modeset. And it does that before the WARN.
> > > > > 
> > > > > So you're barking up the wrong tree here. The problem I think
> > > > > is that you're just computing requested_crtcs wrong.
> > > > 
> > > > So here in this case, requested CRTC = 0x1 since it requests modeset on 
> > > > CRTC 0
> > > > Now in teh atomic check, it steals the slave CRTC 1 and hence affected 
> > > > CRTC comes out
> > > > as 0x3 and hence the mismatch.
> > > 
> > > Hmm. How can it be 0x3 if we filtered out the uapi.enable==false case?
> > > 
> > 
> > Yes if I add that condition like in this patch then it correctly calculates
> > the affected crtc bitmask as only 0x1 since it doesnt include the slave 
> > crtc.
> > So with this patch, requested crtc = 0x 1, affected crtc = 0x1
> > 
> > If this looks good then this fixes our bigjoiner warnings.
> > Does this patch look good to you as is then?
> 
> I think you still need to fix the requested_crtcs calculation.

We calculate requested crtc at the beginning :
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
requested_crtc |= drm_crtc_mask(crtc);

Are you suggesting adding this to after:
 if (config->funcs->atomic_check) {
ret = config->funcs->atomic_check(state->dev, state);

if (ret) {
DRM_DEBUG_ATOMIC("atomic driver check for %p failed: 
%d\n",
 state, ret);
return ret;
}
requested_crtc |= drm_crtc_mask(crtc);// Here it will have 
requested crtc = 0x11
}

in this case here the state should already have master crtc 0 and slave crtc 1
and that requested crtc should already be 0x11

Then in that case we dont need any special check for calculating affected crtc, 
that also will be 0x11

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

Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true

[Navare, Manasi]

On Fri, Mar 19, 2021 at 11:12:41PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 19, 2021 at 01:54:13PM -0700, Navare, Manasi wrote:
> > On Fri, Mar 19, 2021 at 04:56:24PM +0200, Ville Syrjälä wrote:
> > > On Thu, Mar 18, 2021 at 04:01:26PM -0700, Navare, Manasi wrote:
> > > > So basically we see this warning only in case of bigjoiner when
> > > > drm_atomic_check gets called without setting the state->allow_modeset 
> > > > flag.
> > > 
> > > Considering the code is 'WARN(!state->allow_modeset, ...' that
> > > fact should be rather obvious.
> > > 
> > > > 
> > > > So do you think that in i915, in intel_atomic_check_bigjoiner() we 
> > > > should only
> > > > steal the crtc when allow_modeset flag is set in state?
> > > 
> > > No. If you fully read drm_atomic_check_only() you will observe
> > > that it will reject any commit w/ allow_modeset==false which 
> > > needs a modeset. And it does that before the WARN.
> > > 
> > > So you're barking up the wrong tree here. The problem I think
> > > is that you're just computing requested_crtcs wrong.
> > 
> > So here in this case, requested CRTC = 0x1 since it requests modeset on 
> > CRTC 0
> > Now in teh atomic check, it steals the slave CRTC 1 and hence affected CRTC 
> > comes out
> > as 0x3 and hence the mismatch.
> 
> Hmm. How can it be 0x3 if we filtered out the uapi.enable==false case?
> 

Yes if I add that condition like in this patch then it correctly calculates
the affected crtc bitmask as only 0x1 since it doesnt include the slave crtc.
So with this patch, requested crtc = 0x 1, affected crtc = 0x1

If this looks good then this fixes our bigjoiner warnings.
Does this patch look good to you as is then?

Manasi

> > Now what is not clear to me is that if a full modeset was not required
> > why did i915 still steal that slave CRTC?
> > 
> > Manasi
> > 
> > > 
> > > > If we add this check there then the affected crtc wont count the slave 
> > > > crtc
> > > > and we wont get this warning.
> > > > 
> > > > Ville, Danvet?
> > > > 
> > > > Manasi
> > > > 
> > > > 
> > > > On Tue, Mar 16, 2021 at 10:35:09PM +0100, Daniel Vetter wrote:
> > > > > On Tue, Mar 9, 2021 at 10:14 AM Pekka Paalanen  
> > > > > wrote:
> > > > > >
> > > > > > On Mon, 8 Mar 2021 16:52:58 -0800
> > > > > > "Navare, Manasi"  wrote:
> > > > > >
> > > > > > > On Thu, Mar 04, 2021 at 10:42:23AM +0200, Pekka Paalanen wrote:
> > > > > > > > On Wed, 3 Mar 2021 12:44:33 -0800
> > > > > > > > "Navare, Manasi"  wrote:
> > > > > > > >
> > > > > > > > > On Wed, Mar 03, 2021 at 10:47:44AM +0200, Pekka Paalanen 
> > > > > > > > > wrote:
> > > > > > > > > > On Tue,  2 Mar 2021 12:41:32 -0800
> > > > > > > > > > Manasi Navare  wrote:
> > > > > > > > > >
> > > > > > > > > > > In case of a modeset where a mode gets split across 
> > > > > > > > > > > mutiple CRTCs
> > > > > > > > > > > in the driver specific implementation (bigjoiner in i915) 
> > > > > > > > > > > we wrongly count
> > > > > > > > > > > the affected CRTCs based on the drm_crtc_mask and 
> > > > > > > > > > > indicate the stolen CRTC as
> > > > > > > > > > > an affected CRTC in atomic_check_only().
> > > > > > > > > > > This triggers a warning since affected CRTCs doent match 
> > > > > > > > > > > requested CRTC.
> > > > > > > > > > >
> > > > > > > > > > > To fix this in such bigjoiner configurations, we should 
> > > > > > > > > > > only
> > > > > > > > > > > increment affected crtcs if that CRTC is enabled in UAPI 
> > > > > > > > > > > not
> > > > > > > > > > > if it is just used internally in the driver to split the 
> > > > > > > > > > > mode.
> > > > > > > > > >
> > > > > > > > > > Hi,
> > &

Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true

[Navare, Manasi]

On Fri, Mar 19, 2021 at 04:56:24PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 18, 2021 at 04:01:26PM -0700, Navare, Manasi wrote:
> > So basically we see this warning only in case of bigjoiner when
> > drm_atomic_check gets called without setting the state->allow_modeset flag.
> 
> Considering the code is 'WARN(!state->allow_modeset, ...' that
> fact should be rather obvious.
> 
> > 
> > So do you think that in i915, in intel_atomic_check_bigjoiner() we should 
> > only
> > steal the crtc when allow_modeset flag is set in state?
> 
> No. If you fully read drm_atomic_check_only() you will observe
> that it will reject any commit w/ allow_modeset==false which 
> needs a modeset. And it does that before the WARN.
> 
> So you're barking up the wrong tree here. The problem I think
> is that you're just computing requested_crtcs wrong.

So here in this case, requested CRTC = 0x1 since it requests modeset on CRTC 0
Now in teh atomic check, it steals the slave CRTC 1 and hence affected CRTC 
comes out
as 0x3 and hence the mismatch.
Now what is not clear to me is that if a full modeset was not required
why did i915 still steal that slave CRTC?

Manasi

> 
> > If we add this check there then the affected crtc wont count the slave crtc
> > and we wont get this warning.
> > 
> > Ville, Danvet?
> > 
> > Manasi
> > 
> > 
> > On Tue, Mar 16, 2021 at 10:35:09PM +0100, Daniel Vetter wrote:
> > > On Tue, Mar 9, 2021 at 10:14 AM Pekka Paalanen  
> > > wrote:
> > > >
> > > > On Mon, 8 Mar 2021 16:52:58 -0800
> > > > "Navare, Manasi"  wrote:
> > > >
> > > > > On Thu, Mar 04, 2021 at 10:42:23AM +0200, Pekka Paalanen wrote:
> > > > > > On Wed, 3 Mar 2021 12:44:33 -0800
> > > > > > "Navare, Manasi"  wrote:
> > > > > >
> > > > > > > On Wed, Mar 03, 2021 at 10:47:44AM +0200, Pekka Paalanen wrote:
> > > > > > > > On Tue,  2 Mar 2021 12:41:32 -0800
> > > > > > > > Manasi Navare  wrote:
> > > > > > > >
> > > > > > > > > In case of a modeset where a mode gets split across mutiple 
> > > > > > > > > CRTCs
> > > > > > > > > in the driver specific implementation (bigjoiner in i915) we 
> > > > > > > > > wrongly count
> > > > > > > > > the affected CRTCs based on the drm_crtc_mask and indicate 
> > > > > > > > > the stolen CRTC as
> > > > > > > > > an affected CRTC in atomic_check_only().
> > > > > > > > > This triggers a warning since affected CRTCs doent match 
> > > > > > > > > requested CRTC.
> > > > > > > > >
> > > > > > > > > To fix this in such bigjoiner configurations, we should only
> > > > > > > > > increment affected crtcs if that CRTC is enabled in UAPI not
> > > > > > > > > if it is just used internally in the driver to split the mode.
> > > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > I think that makes sense to me. Stealing CRTCs that are not 
> > > > > > > > currently
> > > > > > > > used by the userspace (display server) should be ok, as long as 
> > > > > > > > that
> > > > > > > > is completely invisible to userspace: meaning that it does not 
> > > > > > > > cause
> > > > > > > > userspace to unexpectedly e.g. receive or miss per-crtc atomic
> > > > > > > > completion events.
> > > > > > >
> > > > > > > Yes since we are only doing atomic_check_only() here, the stolen
> > > > > >
> > > > > > But the real not-test-only commit will follow if this test-only 
> > > > > > commit
> > > > > > succeeds, and keeping the guarantees for the real commit are 
> > > > > > important.
> > > > >
> > > > > Hmm well after the actual real commit, since the second crtc is stolen
> > > > > even though it is not being used for the display output, it is
> > > > > used for joiner so the uapi.enable will be true after the real commit.
> > > > >
> > > > > so actually the assertion would fail in this c

Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true

[Navare, Manasi]

So basically we see this warning only in case of bigjoiner when
drm_atomic_check gets called without setting the state->allow_modeset flag.

So do you think that in i915, in intel_atomic_check_bigjoiner() we should only
steal the crtc when allow_modeset flag is set in state?
If we add this check there then the affected crtc wont count the slave crtc
and we wont get this warning.

Ville, Danvet?

Manasi


On Tue, Mar 16, 2021 at 10:35:09PM +0100, Daniel Vetter wrote:
> On Tue, Mar 9, 2021 at 10:14 AM Pekka Paalanen  wrote:
> >
> > On Mon, 8 Mar 2021 16:52:58 -0800
> > "Navare, Manasi"  wrote:
> >
> > > On Thu, Mar 04, 2021 at 10:42:23AM +0200, Pekka Paalanen wrote:
> > > > On Wed, 3 Mar 2021 12:44:33 -0800
> > > > "Navare, Manasi"  wrote:
> > > >
> > > > > On Wed, Mar 03, 2021 at 10:47:44AM +0200, Pekka Paalanen wrote:
> > > > > > On Tue,  2 Mar 2021 12:41:32 -0800
> > > > > > Manasi Navare  wrote:
> > > > > >
> > > > > > > In case of a modeset where a mode gets split across mutiple CRTCs
> > > > > > > in the driver specific implementation (bigjoiner in i915) we 
> > > > > > > wrongly count
> > > > > > > the affected CRTCs based on the drm_crtc_mask and indicate the 
> > > > > > > stolen CRTC as
> > > > > > > an affected CRTC in atomic_check_only().
> > > > > > > This triggers a warning since affected CRTCs doent match 
> > > > > > > requested CRTC.
> > > > > > >
> > > > > > > To fix this in such bigjoiner configurations, we should only
> > > > > > > increment affected crtcs if that CRTC is enabled in UAPI not
> > > > > > > if it is just used internally in the driver to split the mode.
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > I think that makes sense to me. Stealing CRTCs that are not 
> > > > > > currently
> > > > > > used by the userspace (display server) should be ok, as long as that
> > > > > > is completely invisible to userspace: meaning that it does not cause
> > > > > > userspace to unexpectedly e.g. receive or miss per-crtc atomic
> > > > > > completion events.
> > > > >
> > > > > Yes since we are only doing atomic_check_only() here, the stolen
> > > >
> > > > But the real not-test-only commit will follow if this test-only commit
> > > > succeeds, and keeping the guarantees for the real commit are important.
> > >
> > > Hmm well after the actual real commit, since the second crtc is stolen
> > > even though it is not being used for the display output, it is
> > > used for joiner so the uapi.enable will be true after the real commit.
> > >
> > > so actually the assertion would fail in this case.
> > >
> > > @Ville @Danvet any suggestions here in that case?
> 
> That is very bad. We can't frob uapi state like that. I think that
> calls for even more checks to make sure kms drivers who try to play
> clever games don't get it wrong, so we probably need to check uapi
> enable and active state in another mask before/after ->atomic_check
> too. Or something like that.
> 
> > Hi,
> >
> > that is not what I was talking about, but sounds like you found a
> > different problem. It seems like the problem you are talking about
> > would be guaranteed to be hit if bigjoiner was used. Have you not
> > tested this?
> >
> > However, I was talking about the real commit itself, not what happens
> > on commits after it, see below.
> >
> > > > > crtc is completely invisible to the userspace and hence that is
> > > > > indicated by uapi.enable which is not true for this stolen
> > > > > crtc. However if allow modeset flag set, then it will do a full
> > > > > modeset and indicate the uapi.enable for this stolen crtc as well
> > > > > since that cannot be used for other modeset requested by userspace.
> > > > >
> > > > > >
> > > > > > Can that also be asserted somehow, or does this already do that?
> > > > >
> > > > > Not clear what you want the assertion for? Could you elaborate
> > > >
> > > > As assertion that when the real atomic commit happens and then
> > > > completion events are fired, they match exactly the affected 

Re: [Intel-gfx] [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true

[Navare, Manasi]

On Tue, Mar 16, 2021 at 11:46:38PM +, Daniel Stone wrote:
> On Tue, 16 Mar 2021 at 21:35, Daniel Vetter  wrote:
> 
> > On Tue, Mar 9, 2021 at 10:14 AM Pekka Paalanen 
> > wrote:
> > > On Mon, 8 Mar 2021 16:52:58 -0800
> > > "Navare, Manasi"  wrote:
> > > > Hmm well after the actual real commit, since the second crtc is stolen
> > > > even though it is not being used for the display output, it is
> > > > used for joiner so the uapi.enable will be true after the real commit.
> > > >
> > > > so actually the assertion would fail in this case.
> > > >
> > > > @Ville @Danvet any suggestions here in that case?
> >
> > That is very bad. We can't frob uapi state like that. I think that
> > calls for even more checks to make sure kms drivers who try to play
> > clever games don't get it wrong, so we probably need to check uapi
> > enable and active state in another mask before/after ->atomic_check
> > too. Or something like that.
> >
> 
> Yeah. We can _never_ generate externally-visible completion events. We can
> later fail to enable the stolen CRTC - because trying to enable new things
> can fail for any reason whatsoever - but we can't generate spurious
> completion events, as doing so falls into the uncanny valley.
> 
> If the kernel is doing clever things behind userspace's back - such as
> stealing planes or CRTCs - then userspace can never know about it, apart
> from failing to enable those resources later. The kernel can either never
> do anything clever (and make userspace bind them both together), or be
> extremely clever (by hiding the entire details from userspace), but it
> cannot choose the halfway house of doing clever things behind userspace's
> back (such as stealing new CRTCs) whilst also exposing all those details to
> userspace (such as delivering spurious completion events for resources
> userspace never requested to be programmed).
> 
> Cheers,
> Daniel

Yes I agree, in this case there will not be any completion events associated 
with
the stolen slave CRTC since that does not get used for the output.
The completion events will only occur on the bigjoiner master crtc.

But I guess like Danvet suggested we need a separate mask for keeping track of 
active and
enabled crtcs before and after atomic check. But need to look at how this will 
fix
the affected crtc not matching warning.

Manasi

> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true

[Navare, Manasi]

On Thu, Mar 04, 2021 at 10:42:23AM +0200, Pekka Paalanen wrote:
> On Wed, 3 Mar 2021 12:44:33 -0800
> "Navare, Manasi"  wrote:
> 
> > On Wed, Mar 03, 2021 at 10:47:44AM +0200, Pekka Paalanen wrote:
> > > On Tue,  2 Mar 2021 12:41:32 -0800
> > > Manasi Navare  wrote:
> > >   
> > > > In case of a modeset where a mode gets split across mutiple CRTCs
> > > > in the driver specific implementation (bigjoiner in i915) we wrongly 
> > > > count
> > > > the affected CRTCs based on the drm_crtc_mask and indicate the stolen 
> > > > CRTC as
> > > > an affected CRTC in atomic_check_only().
> > > > This triggers a warning since affected CRTCs doent match requested CRTC.
> > > > 
> > > > To fix this in such bigjoiner configurations, we should only
> > > > increment affected crtcs if that CRTC is enabled in UAPI not
> > > > if it is just used internally in the driver to split the mode.  
> > > 
> > > Hi,
> > > 
> > > I think that makes sense to me. Stealing CRTCs that are not currently
> > > used by the userspace (display server) should be ok, as long as that
> > > is completely invisible to userspace: meaning that it does not cause
> > > userspace to unexpectedly e.g. receive or miss per-crtc atomic
> > > completion events.  
> > 
> > Yes since we are only doing atomic_check_only() here, the stolen
> 
> But the real not-test-only commit will follow if this test-only commit
> succeeds, and keeping the guarantees for the real commit are important.

Hmm well after the actual real commit, since the second crtc is stolen
even though it is not being used for the display output, it is
used for joiner so the uapi.enable will be true after the real commit.

so actually the assertion would fail in this case.

@Ville @Danvet any suggestions here in that case?

Manasi

> 
> > crtc is completely invisible to the userspace and hence that is 
> > indicated by uapi.enable which is not true for this stolen
> > crtc. However if allow modeset flag set, then it will do a full
> > modeset and indicate the uapi.enable for this stolen crtc as well
> > since that cannot be used for other modeset requested by userspace.
> > 
> > > 
> > > Can that also be asserted somehow, or does this already do that?  
> > 
> > Not clear what you want the assertion for? Could you elaborate
> 
> As assertion that when the real atomic commit happens and then
> completion events are fired, they match exactly the affected crtcs mask.
> 
> I understand this may be off-topic for this particular patch, but since
> we are discussing the topic, such checks would be really nice. I'm
> curious if such checks already exist.
> 
> 
> Thanks,
> pq
> 
> > 
> > Manasi
> > 
> > > 
> > > 
> > > Thanks,
> > > pq
> > >   
> > > > Cc: Ville Syrjälä 
> > > > Cc: Simon Ser 
> > > > Cc: Pekka Paalanen 
> > > > Cc: Daniel Stone 
> > > > Cc: Daniel Vetter 
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Signed-off-by: Manasi Navare 
> > > > ---
> > > >  drivers/gpu/drm/drm_atomic.c | 6 --
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > > index 5b4547e0f775..d7acd6bbd97e 100644
> > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > @@ -1358,8 +1358,10 @@ int drm_atomic_check_only(struct 
> > > > drm_atomic_state *state)
> > > > }
> > > > }
> > > >  
> > > > -   for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > > > -   affected_crtc |= drm_crtc_mask(crtc);
> > > > +   for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > > +   if (new_crtc_state->enable)
> > > > +   affected_crtc |= drm_crtc_mask(crtc);
> > > > +   }
> > > >  
> > > > /*
> > > >  * For commits that allow modesets drivers can add other CRTCs 
> > > > to the  
> > >   
> > 
> > 
> 


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true

[Navare, Manasi]

On Wed, Mar 03, 2021 at 10:47:44AM +0200, Pekka Paalanen wrote:
> On Tue,  2 Mar 2021 12:41:32 -0800
> Manasi Navare  wrote:
> 
> > In case of a modeset where a mode gets split across mutiple CRTCs
> > in the driver specific implementation (bigjoiner in i915) we wrongly count
> > the affected CRTCs based on the drm_crtc_mask and indicate the stolen CRTC 
> > as
> > an affected CRTC in atomic_check_only().
> > This triggers a warning since affected CRTCs doent match requested CRTC.
> > 
> > To fix this in such bigjoiner configurations, we should only
> > increment affected crtcs if that CRTC is enabled in UAPI not
> > if it is just used internally in the driver to split the mode.
> 
> Hi,
> 
> I think that makes sense to me. Stealing CRTCs that are not currently
> used by the userspace (display server) should be ok, as long as that
> is completely invisible to userspace: meaning that it does not cause
> userspace to unexpectedly e.g. receive or miss per-crtc atomic
> completion events.

Yes since we are only doing atomic_check_only() here, the stolen
crtc is completely invisible to the userspace and hence that is 
indicated by uapi.enable which is not true for this stolen
crtc. However if allow modeset flag set, then it will do a full
modeset and indicate the uapi.enable for this stolen crtc as well
since that cannot be used for other modeset requested by userspace.

> 
> Can that also be asserted somehow, or does this already do that?

Not clear what you want the assertion for? Could you elaborate

Manasi

> 
> 
> Thanks,
> pq
> 
> > Cc: Ville Syrjälä 
> > Cc: Simon Ser 
> > Cc: Pekka Paalanen 
> > Cc: Daniel Stone 
> > Cc: Daniel Vetter 
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Manasi Navare 
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 5b4547e0f775..d7acd6bbd97e 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1358,8 +1358,10 @@ int drm_atomic_check_only(struct drm_atomic_state 
> > *state)
> > }
> > }
> >  
> > -   for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > -   affected_crtc |= drm_crtc_mask(crtc);
> > +   for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > +   if (new_crtc_state->enable)
> > +   affected_crtc |= drm_crtc_mask(crtc);
> > +   }
> >  
> > /*
> >  * For commits that allow modesets drivers can add other CRTCs to the
> 


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] Revert "drm/atomic: document and enforce rules around "spurious" EBUSY"

[Navare, Manasi]

On Wed, Feb 10, 2021 at 05:07:03PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 10, 2021 at 01:38:45PM +, Simon Ser wrote:
> > On Wednesday, February 10th, 2021 at 2:16 PM, Daniel Vetter 
> >  wrote:
> > 
> > > On Tue, Feb 09, 2021 at 04:14:01PM -0800, Manasi Navare wrote:
> > >
> > > > These additional checks added to avoid EBUSY give unnecessary WARN_ON
> > > > in case of big joiner used in i915 in which case even if the modeset
> > > > is requested on a single pipe, internally another consecutive
> > > > pipe is stolen and used to drive half of the transcoder timings.
> > > > So in this case it is expected that requested crtc and affected crtcs
> > > > do not match. Hence the added WARN ON becomes irrelevant.
> > 
> > The WARN_ON only happens if allow_modeset == false. If allow_modeset == 
> > true,
> > then the driver is allowed to steal an unrelated pipe.
> > 
> > Maybe i915 is stealing a pipe without allow_modeset?
> 
> No. All page flips etc. will have to get split up internally
> between multiple crtcs.
> 
> So I think there's basically three options:
> a) massive rewrite of i915 to bypass even more of drm_atomic stuff
> b) allow i915 to silence that warning, which opens up the question
>whether the warn is doing any good if it can just be bypassed
> c) nuke the warning entirely
> 
> a) is not going to happen, and it would any way allow i915 to
> do things any which way it wants without tripping the warn,
> rendering the warn entirely toothless.
> 
> Hmm. Maybe there is a d) which would be to ignore all crtcs
> that are not logically enabled in the warn? Not sure if that
> could allow something to slit through that people want it to
> catch?

So as per the offline IRC discussions,
- We can check for crtc_state->enable and only use the enabled crtcs
in the affected crtc calculation. And this enable would only
be set when modeset is done. So in case of bigjoiner no modeset on Pipe A,
even if Pipe B is stolen, since no modeset and because that pipe doesnt
get enabled the affected crtcs would still be 0x1.

This should solve the problem. 
Ville, Danvet - I will make this change?

Manasi

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

Re: [Intel-gfx] [PATCH 1/3] i915/display/intel_dp: Read PCON DSC ENC caps only for DPCD rev >= 1.4

[Navare, Manasi]

On Fri, Feb 05, 2021 at 10:06:48PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 05, 2021 at 12:07:41PM -0800, Navare, Manasi wrote:
> > On Fri, Feb 05, 2021 at 09:58:07PM +0200, Ville Syrjälä wrote:
> > > On Thu, Feb 04, 2021 at 12:18:40PM +0530, Ankit Nautiyal wrote:
> > > > DP-HDMI2.1 PCON has DSC encoder caps defined in registers 0x92-0x9E.
> > > > Do not read the registers if DPCD rev < 1.4.
> > > > 
> > > > Fixes: https://gitlab.freedesktop.org/drm/intel/-/issues/2868
> > > > Signed-off-by: Ankit Nautiyal 
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index 8c12d5375607..2b83f0f433a2 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -2489,9 +2489,11 @@ static void intel_dp_get_pcon_dsc_cap(struct 
> > > > intel_dp *intel_dp)
> > > > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > >  
> > > > /* Clear the cached register set to avoid using stale values */
> > > > -
> > > > memset(intel_dp->pcon_dsc_dpcd, 0, 
> > > > sizeof(intel_dp->pcon_dsc_dpcd));
> > > >  
> > > > +   if (intel_dp->dpcd[DP_DPCD_REV] < 0x14)
> > > > +   return;
> > > > +
> > > 
> > > Can't check the spec, but makes sense that this stuff is only valid
> > > for recent DCPD revisions.
> > > 
> > > Acked-by: Ville Syrjälä 
> > 
> > Yes checked the DP 1.4 spec and this is correct
> 
> I didn't think this is in the DP spec, but rather some special extra
> spec which I do not have.

Yes I meant just double checked that the DSC support itself from DP 1.4 and 
hence
makes sense that the PCON DSC regs also from >= 1.4

Manasi

> 
> > 
> > Reviewed-by: Manasi Navare 
> > 
> > Manasi
> > 
> > > 
> > > > if (drm_dp_dpcd_read(_dp->aux, DP_PCON_DSC_ENCODER,
> > > >  intel_dp->pcon_dsc_dpcd,
> > > >  sizeof(intel_dp->pcon_dsc_dpcd)) < 0)
> > > > -- 
> > > > 2.29.2
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> > > ___
> > > Intel-gfx mailing list
> > > intel-...@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/3] i915/display/intel_dp: Read PCON DSC ENC caps only for DPCD rev >= 1.4

[Navare, Manasi]

On Fri, Feb 05, 2021 at 09:58:07PM +0200, Ville Syrjälä wrote:
> On Thu, Feb 04, 2021 at 12:18:40PM +0530, Ankit Nautiyal wrote:
> > DP-HDMI2.1 PCON has DSC encoder caps defined in registers 0x92-0x9E.
> > Do not read the registers if DPCD rev < 1.4.
> > 
> > Fixes: https://gitlab.freedesktop.org/drm/intel/-/issues/2868
> > Signed-off-by: Ankit Nautiyal 
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 8c12d5375607..2b83f0f433a2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2489,9 +2489,11 @@ static void intel_dp_get_pcon_dsc_cap(struct 
> > intel_dp *intel_dp)
> > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> >  
> > /* Clear the cached register set to avoid using stale values */
> > -
> > memset(intel_dp->pcon_dsc_dpcd, 0, sizeof(intel_dp->pcon_dsc_dpcd));
> >  
> > +   if (intel_dp->dpcd[DP_DPCD_REV] < 0x14)
> > +   return;
> > +
> 
> Can't check the spec, but makes sense that this stuff is only valid
> for recent DCPD revisions.
> 
> Acked-by: Ville Syrjälä 

Yes checked the DP 1.4 spec and this is correct

Reviewed-by: Manasi Navare 

Manasi

> 
> > if (drm_dp_dpcd_read(_dp->aux, DP_PCON_DSC_ENCODER,
> >  intel_dp->pcon_dsc_dpcd,
> >  sizeof(intel_dp->pcon_dsc_dpcd)) < 0)
> > -- 
> > 2.29.2
> 
> -- 
> Ville Syrjälä
> Intel
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm/i915/bios: fill in DSC rc_model_size from VBT

[Navare, Manasi]

On Tue, Dec 08, 2020 at 02:33:54PM +0200, Jani Nikula wrote:
> The VBT fields match the DPCD data, so use the same helper.
> 
> Cc: Manasi Navare 
> Cc: Vandita Kulkarni 
> Signed-off-by: Jani Nikula 

Only for DSI so far right?

In that case looks good

Reviewed-by: Manasi Navare 

Manasi

> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 11 +++
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c 
> b/drivers/gpu/drm/i915/display/intel_bios.c
> index 4cc949b228f2..06c3310446a2 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -2555,16 +2555,11 @@ static void fill_dsc(struct intel_crtc_state 
> *crtc_state,
> crtc_state->dsc.slice_count);
>  
>   /*
> -  * FIXME: Use VBT rc_buffer_block_size and rc_buffer_size for the
> -  * implementation specific physical rate buffer size. Currently we use
> -  * the required rate buffer model size calculated in
> -  * drm_dsc_compute_rc_parameters() according to VESA DSC Annex E.
> -  *
>* The VBT rc_buffer_block_size and rc_buffer_size definitions
> -  * correspond to DP 1.4 DPCD offsets 0x62 and 0x63. The DP DSC
> -  * implementation should also use the DPCD (or perhaps VBT for eDP)
> -  * provided value for the buffer size.
> +  * correspond to DP 1.4 DPCD offsets 0x62 and 0x63.
>*/
> + vdsc_cfg->rc_model_size = 
> drm_dsc_dp_rc_buffer_size(dsc->rc_buffer_block_size,
> + 
> dsc->rc_buffer_size);
>  
>   /* FIXME: DSI spec says bpc + 1 for this one */
>   vdsc_cfg->line_buf_depth = 
> VBT_DSC_LINE_BUFFER_DEPTH(dsc->line_buffer_depth);
> -- 
> 2.20.1
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm/i915/dsc: make rc_model_size an encoder defined value

[Navare, Manasi]

On Tue, Dec 08, 2020 at 02:33:52PM +0200, Jani Nikula wrote:
> Move the intialization of the rc_model_size from the common code into
> encoder code, allowing different encoders to specify the size according
> to their needs. Keep using the hard coded value in the encoders for now
> to make this a non-functional change.
> 
> Cc: Manasi Navare 
> Cc: Vandita Kulkarni 
> Signed-off-by: Jani Nikula 

So still using the hardcoded value since thats in the DSC C model, Looks good 
to me

Reviewed-by: Manasi Navare 

Manasi

> ---
>  drivers/gpu/drm/i915/display/icl_dsi.c| 3 +++
>  drivers/gpu/drm/i915/display/intel_dp.c   | 8 
>  drivers/gpu/drm/i915/display/intel_vdsc.c | 2 --
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c 
> b/drivers/gpu/drm/i915/display/icl_dsi.c
> index a9439b415603..676e40172fe9 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -1535,6 +1535,9 @@ static int gen11_dsi_dsc_compute_config(struct 
> intel_encoder *encoder,
>  
>   vdsc_cfg->convert_rgb = true;
>  
> + /* FIXME: initialize from VBT */
> + vdsc_cfg->rc_model_size = DSC_RC_MODEL_SIZE_CONST;
> +
>   ret = intel_dsc_compute_params(encoder, crtc_state);
>   if (ret)
>   return ret;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index cb5e42c3ecd5..b2bc0c8c39c7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2289,6 +2289,14 @@ static int intel_dp_dsc_compute_params(struct 
> intel_encoder *encoder,
>   u8 line_buf_depth;
>   int ret;
>  
> + /*
> +  * RC_MODEL_SIZE is currently a constant across all configurations.
> +  *
> +  * FIXME: Look into using sink defined DPCD DP_DSC_RC_BUF_BLK_SIZE and
> +  * DP_DSC_RC_BUF_SIZE for this.
> +  */
> + vdsc_cfg->rc_model_size = DSC_RC_MODEL_SIZE_CONST;
> +
>   ret = intel_dsc_compute_params(encoder, crtc_state);
>   if (ret)
>   return ret;
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c 
> b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 22d08679844f..f58cc5700784 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -454,8 +454,6 @@ int intel_dsc_compute_params(struct intel_encoder 
> *encoder,
>   else if (vdsc_cfg->bits_per_component == 12)
>   vdsc_cfg->mux_word_size = DSC_MUX_WORD_SIZE_12_BPC;
>  
> - /* RC_MODEL_SIZE is a constant across all configurations */
> - vdsc_cfg->rc_model_size = DSC_RC_MODEL_SIZE_CONST;
>   /* InitialScaleValue is a 6 bit value with 3 fractional bits (U3.3) */
>   vdsc_cfg->initial_scale_value = (vdsc_cfg->rc_model_size << 3) /
>   (vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset);
> -- 
> 2.20.1
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/dp: add a number of DP 2.0 DPCD definitions

[Navare, Manasi]

On Fri, Sep 18, 2020 at 02:40:17PM +0300, Jani Nikula wrote:
> Prepare for future with DP 2.0 DPCD definitions, with a couple of
> related drive-by cleanups. No functional changes.
> 
> v2: Send the version that actually builds.
> 
> Signed-off-by: Jani Nikula 

Verified the below DP 2.0 DPCD registers from the DP 2.0 spec

Reviewed-by: Manasi Navare 

Manasi

> ---
>  include/drm/drm_dp_helper.h | 52 -
>  1 file changed, 45 insertions(+), 7 deletions(-)
> 
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 388083b4716b..e144b4b9d79a 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -125,6 +125,7 @@ struct drm_device;
>  
>  #define DP_MAX_DOWNSPREAD   0x003
>  # define DP_MAX_DOWNSPREAD_0_5   (1 << 0)
> +# define DP_STREAM_REGENERATION_STATUS_CAP  (1 << 1) /* 2.0 */
>  # define DP_NO_AUX_HANDSHAKE_LINK_TRAINING  (1 << 6)
>  # define DP_TPS4_SUPPORTED  (1 << 7)
>  
> @@ -142,6 +143,7 @@ struct drm_device;
>  
>  #define DP_MAIN_LINK_CHANNEL_CODING 0x006
>  # define DP_CAP_ANSI_8B10B   (1 << 0)
> +# define DP_CAP_ANSI_128B132B   (1 << 1) /* 2.0 */
>  
>  #define DP_DOWN_STREAM_PORT_COUNT0x007
>  # define DP_PORT_COUNT_MASK  0x0f
> @@ -185,8 +187,14 @@ struct drm_device;
>  #define DP_FAUX_CAP  0x020   /* 1.2 */
>  # define DP_FAUX_CAP_1   (1 << 0)
>  
> +#define DP_SINK_VIDEO_FALLBACK_FORMATS  0x020   /* 2.0 */
> +# define DP_FALLBACK_1024x768_60HZ_24BPP(1 << 0)
> +# define DP_FALLBACK_1280x720_60HZ_24BPP(1 << 1)
> +# define DP_FALLBACK_1920x1080_60HZ_24BPP   (1 << 2)
> +
>  #define DP_MSTM_CAP  0x021   /* 1.2 */
>  # define DP_MST_CAP  (1 << 0)
> +# define DP_SINGLE_STREAM_SIDEBAND_MSG  (1 << 1) /* 2.0 */
>  
>  #define DP_NUMBER_OF_AUDIO_ENDPOINTS 0x022   /* 1.2 */
>  
> @@ -434,6 +442,9 @@ struct drm_device;
>  # define DP_LINK_BW_2_7  0x0a
>  # define DP_LINK_BW_5_4  0x14/* 1.2 */
>  # define DP_LINK_BW_8_1  0x1e/* 1.4 */
> +# define DP_LINK_BW_10  0x01/* 2.0 128b/132b Link 
> Layer */
> +# define DP_LINK_BW_13_50x04/* 2.0 128b/132b Link 
> Layer */
> +# define DP_LINK_BW_20  0x02/* 2.0 128b/132b Link 
> Layer */
>  
>  #define DP_LANE_COUNT_SET0x101
>  # define DP_LANE_COUNT_MASK  0x0f
> @@ -485,12 +496,15 @@ struct drm_device;
>  # define DP_TRAIN_PRE_EMPHASIS_SHIFT 3
>  # define DP_TRAIN_MAX_PRE_EMPHASIS_REACHED  (1 << 5)
>  
> +# define DP_TX_FFE_PRESET_VALUE_MASK(0xf << 0) /* 2.0 128b/132b Link 
> Layer */
> +
>  #define DP_DOWNSPREAD_CTRL   0x107
>  # define DP_SPREAD_AMP_0_5   (1 << 4)
>  # define DP_MSA_TIMING_PAR_IGNORE_EN (1 << 7) /* eDP */
>  
>  #define DP_MAIN_LINK_CHANNEL_CODING_SET  0x108
>  # define DP_SET_ANSI_8B10B   (1 << 0)
> +# define DP_SET_ANSI_128B132B   (1 << 1)
>  
>  #define DP_I2C_SPEED_CONTROL_STATUS  0x109   /* DPI */
>  /* bitmask as for DP_I2C_SPEED_CAP */
> @@ -509,8 +523,19 @@ struct drm_device;
>  # define DP_LINK_QUAL_PATTERN_ERROR_RATE2
>  # define DP_LINK_QUAL_PATTERN_PRBS7  3
>  # define DP_LINK_QUAL_PATTERN_80BIT_CUSTOM  4
> -# define DP_LINK_QUAL_PATTERN_HBR2_EYE  5
> -# define DP_LINK_QUAL_PATTERN_MASK   7
> +# define DP_LINK_QUAL_PATTERN_CP2520_PAT_1  5
> +# define DP_LINK_QUAL_PATTERN_CP2520_PAT_2  6
> +# define DP_LINK_QUAL_PATTERN_CP2520_PAT_3  7
> +/* DP 2.0 UHBR10, UHBR13.5, UHBR20 */
> +# define DP_LINK_QUAL_PATTERN_128B132B_TPS1 0x08
> +# define DP_LINK_QUAL_PATTERN_128B132B_TPS2 0x10
> +# define DP_LINK_QUAL_PATTERN_PRSBS90x18
> +# define DP_LINK_QUAL_PATTERN_PRSBS11   0x20
> +# define DP_LINK_QUAL_PATTERN_PRSBS15   0x28
> +# define DP_LINK_QUAL_PATTERN_PRSBS23   0x30
> +# define DP_LINK_QUAL_PATTERN_PRSBS31   0x38
> +# define DP_LINK_QUAL_PATTERN_CUSTOM0x40
> +# define DP_LINK_QUAL_PATTERN_SQUARE0x48
>  
>  #define DP_TRAINING_LANE0_1_SET2 0x10f
>  #define DP_TRAINING_LANE2_3_SET2 0x110
> @@ -613,9 +638,9 @@ struct drm_device;
>  #define DP_LINK_STATUS_UPDATED   (1 << 7)
>  
>  #define DP_SINK_STATUS   0x205
> -
> -#define DP_RECEIVE_PORT_0_STATUS (1 << 0)
> -

Re: [PATCH 1/2] drm/dp: add subheadings to DPCD address definitions

[Navare, Manasi]

On Fri, Sep 18, 2020 at 02:40:16PM +0300, Jani Nikula wrote:
> Add the subheadings from the DP spec. No functional changes.
> 
> Signed-off-by: Jani Nikula 

Looks good to me

Reviewed-by: Manasi Navare 

Manasi

> ---
>  include/drm/drm_dp_helper.h | 22 ++
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index c9f2851904d0..388083b4716b 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -106,8 +106,9 @@ struct drm_device;
>  #define DP_AUX_I2C_REPLY_DEFER   (0x2 << 2)
>  #define DP_AUX_I2C_REPLY_MASK(0x3 << 2)
>  
> -/* AUX CH addresses */
> -/* DPCD */
> +/* DPCD Field Address Mapping */
> +
> +/* Receiver Capability */
>  #define DP_DPCD_REV 0x000
>  # define DP_DPCD_REV_10 0x10
>  # define DP_DPCD_REV_11 0x11
> @@ -426,7 +427,7 @@ struct drm_device;
>  #define DP_DSC_BRANCH_OVERALL_THROUGHPUT_1  0x0a1
>  #define DP_DSC_BRANCH_MAX_LINE_WIDTH0x0a2
>  
> -/* link configuration */
> +/* Link Configuration */
>  #define  DP_LINK_BW_SET  0x100
>  # define DP_LINK_RATE_TABLE  0x00/* eDP 1.4 */
>  # define DP_LINK_BW_1_62 0x06
> @@ -580,6 +581,7 @@ struct drm_device;
>  #define DP_PAYLOAD_ALLOCATE_START_TIME_SLOT 0x1c1
>  #define DP_PAYLOAD_ALLOCATE_TIME_SLOT_COUNT 0x1c2
>  
> +/* Link/Sink Device Status */
>  #define DP_SINK_COUNT0x200
>  /* prior to 1.2 bit 7 was reserved mbz */
>  # define DP_GET_SINK_COUNT(x)x) & 0x80) >> 1) | ((x) 
> & 0x3f))
> @@ -779,20 +781,27 @@ struct drm_device;
>  #define DP_VC_PAYLOAD_ID_SLOT_1 0x2c1   /* 1.2 MST */
>  /* up to ID_SLOT_63 at 0x2ff */
>  
> +/* Source Device-specific */
>  #define DP_SOURCE_OUI0x300
> +
> +/* Sink Device-specific */
>  #define DP_SINK_OUI  0x400
> +
> +/* Branch Device-specific */
>  #define DP_BRANCH_OUI0x500
>  #define DP_BRANCH_ID0x503
>  #define DP_BRANCH_REVISION_START0x509
>  #define DP_BRANCH_HW_REV0x509
>  #define DP_BRANCH_SW_REV0x50A
>  
> +/* Link/Sink Device Power Control */
>  #define DP_SET_POWER0x600
>  # define DP_SET_POWER_D00x1
>  # define DP_SET_POWER_D30x2
>  # define DP_SET_POWER_MASK  0x3
>  # define DP_SET_POWER_D3_AUX_ON 0x5
>  
> +/* eDP-specific */
>  #define DP_EDP_DPCD_REV  0x700/* eDP 1.2 */
>  # define DP_EDP_11   0x00
>  # define DP_EDP_12   0x01
> @@ -876,11 +885,13 @@ struct drm_device;
>  #define DP_EDP_REGIONAL_BACKLIGHT_BASE  0x740/* eDP 1.4 */
>  #define DP_EDP_REGIONAL_BACKLIGHT_0  0x741/* eDP 1.4 */
>  
> +/* Sideband MSG Buffers */
>  #define DP_SIDEBAND_MSG_DOWN_REQ_BASE0x1000   /* 1.2 MST */
>  #define DP_SIDEBAND_MSG_UP_REP_BASE  0x1200   /* 1.2 MST */
>  #define DP_SIDEBAND_MSG_DOWN_REP_BASE0x1400   /* 1.2 MST */
>  #define DP_SIDEBAND_MSG_UP_REQ_BASE  0x1600   /* 1.2 MST */
>  
> +/* DPRX Event Status Indicator */
>  #define DP_SINK_COUNT_ESI0x2002   /* 1.2 */
>  /* 0-5 sink count */
>  # define DP_SINK_COUNT_CP_READY (1 << 6)
> @@ -934,6 +945,7 @@ struct drm_device;
>  #define DP_LANE_ALIGN_STATUS_UPDATED_ESI   0x200e /* status same as 
> 0x204 */
>  #define DP_SINK_STATUS_ESI 0x200f /* status same as 
> 0x205 */
>  
> +/* Extended Receiver Capability */
>  #define DP_DP13_DPCD_REV0x2200
>  #define DP_DP13_MAX_LINK_RATE   0x2201
>  
> @@ -947,6 +959,7 @@ struct drm_device;
>  # define DP_VSC_EXT_CEA_SDP_SUPPORTED(1 << 6)  /* DP 
> 1.4 */
>  # define DP_VSC_EXT_CEA_SDP_CHAINING_SUPPORTED   (1 << 7)  /* DP 
> 1.4 */
>  
> +/* Protocol Converter Extension */
>  /* HDMI CEC tunneling over AUX DP 1.3 section 5.3.3.3.1 DPCD 1.4+ */
>  #define DP_CEC_TUNNELING_CAPABILITY0x3000
>  # define DP_CEC_TUNNELING_CAPABLE   (1 << 0)
> @@ -1013,6 +1026,7 @@ struct drm_device;
>  #define DP_PROTOCOL_CONVERTER_CONTROL_2  0x3052 /* DP 1.3 */
>  # define DP_CONVERSION_TO_YCBCR422_ENABLE(1 << 0) /* DP 1.3 */
>  
> +/* HDCP 1.3 and HDCP 2.2 */
>  #define DP_AUX_HDCP_BKSV  

Re: [RFC 1/5] drm/i915/dp: Program source OUI on eDP panels

[Navare, Manasi]

On Tue, Sep 15, 2020 at 03:47:01PM -0400, Lyude Paul wrote:
> On Tue, 2020-09-15 at 15:06 -0400, Rodrigo Vivi wrote:
> > On Tue, Sep 15, 2020 at 01:29:35PM -0400, Lyude Paul wrote:
> > > Since we're about to start adding support for Intel's magic HDR
> > > backlight interface over DPCD, we need to ensure we're properly
> > > programming this field so that Intel specific sink services are exposed.
> > > Otherwise, 0x300-0x3ff will just read zeroes.
> > > 
> > > We also take care not to reprogram the source OUI if it already matches
> > > what we expect. This is just to be careful so that we don't accidentally
> > > take the panel out of any backlight control modes we found it in.
> > > 
> > > Signed-off-by: Lyude Paul 
> > > Cc: thay...@noraisin.net
> > > Cc: Vasily Khoruzhick 
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c | 32 +
> > >  1 file changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 4bd10456ad188..b591672ec4eab 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -3428,6 +3428,7 @@ void intel_dp_sink_set_decompression_state(struct
> > > intel_dp *intel_dp,
> > >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> > >  {
> > >   struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > + u8 edp_oui[] = { 0x00, 0xaa, 0x01 };
> > 
> > what are these values?

We in i915 typically use the OUI number for adding any eDP specific
quirks. I have seen these getting spit out in the dmesg log at thebeginning.
AFAIK It is some kind of OEM identification number for a display panel.

Manasi
> 
> I wish I knew, my assumption is this is the OUI that Intel's GPU driver uses 
> on
> other platforms, but I don't have any documentation mentioning this (in fact,
> the few documents I do have on this backlight interface don't seem to make any
> mention of it). I only managed to find this when looking through the last
> attempt someone did at adding support for this backlight interface:
> 
> https://patchwork.freedesktop.org/patch/334989/
> 
> I think it should be fairly safe to write, as I know nouveau always programs a
> source OUI (we don't do it from our driver, but nvidia hardware seems to do it
> automatically) and I don't believe I've seen it ever change any behavior 
> besides
> making things appear in the 0x300-0x3ff register range.
> 
> AFAICT though, the backlight interface won't advertise itself without this 
> being
> set early on. If you could find anyone from Intel who knows more about it 
> though
> I'd definitely appreciate it (and just in general for the rest of the patch
> series as well)
> 
> > 
> > >   int ret, i;
> > >  
> > >   /* Should have a valid DPCD by this point */
> > > @@ -3443,6 +3444,14 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp,
> > > int mode)
> > >   } else {
> > >   struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> > >  
> > > + /* Write the source OUI as early as possible */
> > > + if (intel_dp_is_edp(intel_dp)) {
> > > + ret = drm_dp_dpcd_write(_dp->aux, DP_SOURCE_OUI,
> > > edp_oui,
> > > + sizeof(edp_oui));
> > > + if (ret < 0)
> > > + drm_err(>drm, "Failed to write eDP source
> > > OUI\n");
> > > + }
> > > +
> > >   /*
> > >* When turning on, we need to retry for 1ms to give the sink
> > >* time to wake up.
> > > @@ -4530,6 +4539,23 @@ static void intel_dp_get_dsc_sink_cap(struct 
> > > intel_dp
> > > *intel_dp)
> > >   }
> > >  }
> > >  
> > > +static void
> > > +intel_edp_init_source_oui(struct intel_dp *intel_dp)
> > > +{
> > > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > + u8 oui[] = { 0x00, 0xaa, 0x01 };
> > > + u8 buf[3] = { 0 };
> > > +
> > > + if (drm_dp_dpcd_read(_dp->aux, DP_SOURCE_OUI, buf, sizeof(buf)) <
> > > 0)
> > > + drm_err(>drm, "Failed to read source OUI\n");
> > > +
> > > + if (memcmp(oui, buf, sizeof(oui)) == 0)
> > > + return;
> > > +
> > > + if (drm_dp_dpcd_write(_dp->aux, DP_SOURCE_OUI, oui, sizeof(oui)) <
> > > 0)
> > > + drm_err(>drm, "Failed to write source OUI\n");
> > > +}
> > > +
> > >  static bool
> > >  intel_edp_init_dpcd(struct intel_dp *intel_dp)
> > >  {
> > > @@ -4607,6 +4633,12 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> > >   if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > >   intel_dp_get_dsc_sink_cap(intel_dp);
> > >  
> > > + /*
> > > +  * Program our source OUI so we can make various Intel-specific AUX
> > > +  * services available (such as HDR backlight controls)
> > > +  */
> > > + intel_edp_init_source_oui(intel_dp);
> > 
> > I believe we should restrict this to the supported platforms: cfl, whl, cml,
> > icl, tgl
> > no?
> > 
> > > +
> > >   return true;
> > >  }
> > >  
> > 

Re: [PATCH v2 2/2] drm/i915/dp: TPS4 PHY test pattern compliance support

[Navare, Manasi]

On Wed, Jul 22, 2020 at 05:36:27PM -0700, Khaled Almahallawy wrote:
> Adding support for TPS4 (CP2520 Pattern 3) PHY pattern source tests.
> 
> v2: uniform bit names TP4a/b/c (Manasi)
> 
> Signed-off-by: Khaled Almahallawy 

Looks good to me,

Reviewed-by: Manasi Navare 

Khaled, could you also give a tested by here?

Manasi

> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 14 --
>  drivers/gpu/drm/i915/i915_reg.h |  4 
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index d6295eb20b63..4b74b2ec5665 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5371,7 +5371,7 @@ static void intel_dp_phy_pattern_update(struct intel_dp 
> *intel_dp)
>   _dp->compliance.test_data.phytest;
>   struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
>   enum pipe pipe = crtc->pipe;
> - u32 pattern_val;
> + u32 pattern_val, dp_tp_ctl;
>  
>   switch (data->phy_pattern) {
>   case DP_PHY_TEST_PATTERN_NONE:
> @@ -5411,7 +5411,7 @@ static void intel_dp_phy_pattern_update(struct intel_dp 
> *intel_dp)
>  DDI_DP_COMP_CTL_ENABLE |
>  DDI_DP_COMP_CTL_CUSTOM80);
>   break;
> - case DP_PHY_TEST_PATTERN_CP2520:
> + case DP_PHY_TEST_PATTERN_CP2520_PAT1:
>   /*
>* FIXME: Ideally pattern should come from DPCD 0x24A. As
>* current firmware of DPR-100 could not set it, so hardcoding
> @@ -5423,6 +5423,16 @@ static void intel_dp_phy_pattern_update(struct 
> intel_dp *intel_dp)
>  DDI_DP_COMP_CTL_ENABLE | DDI_DP_COMP_CTL_HBR2 |
>  pattern_val);
>   break;
> + case DP_PHY_TEST_PATTERN_CP2520_PAT3:
> + DRM_DEBUG_KMS("Set TPS4 Phy Test Pattern\n");
> + intel_de_write(dev_priv, DDI_DP_COMP_CTL(pipe), 0x0);
> + dp_tp_ctl = intel_de_read(dev_priv, 
> TGL_DP_TP_CTL(pipe));
> + dp_tp_ctl &= ~DP_TP_CTL_TRAIN_PAT4_SEL_MASK;
> + dp_tp_ctl |= DP_TP_CTL_TRAIN_PAT4_SEL_TP4a;
> + dp_tp_ctl &= ~DP_TP_CTL_LINK_TRAIN_MASK;
> + dp_tp_ctl |= DP_TP_CTL_LINK_TRAIN_PAT4;
> + intel_de_write(dev_priv, TGL_DP_TP_CTL(pipe), 
> dp_tp_ctl);
> + break;
>   default:
>   WARN(1, "Invalid Phy Test Pattern\n");
>   }
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a0d31f3bf634..c586595b9e76 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9982,6 +9982,10 @@ enum skl_power_gate {
>  #define  DP_TP_CTL_MODE_SST  (0 << 27)
>  #define  DP_TP_CTL_MODE_MST  (1 << 27)
>  #define  DP_TP_CTL_FORCE_ACT (1 << 25)
> +#define  DP_TP_CTL_TRAIN_PAT4_SEL_MASK   (3 << 19)
> +#define  DP_TP_CTL_TRAIN_PAT4_SEL_TP4a   (0 << 19)
> +#define  DP_TP_CTL_TRAIN_PAT4_SEL_TP4b   (1 << 19)
> +#define  DP_TP_CTL_TRAIN_PAT4_SEL_TP4c   (2 << 19)
>  #define  DP_TP_CTL_ENHANCED_FRAME_ENABLE (1 << 18)
>  #define  DP_TP_CTL_FDI_AUTOTRAIN (1 << 15)
>  #define  DP_TP_CTL_LINK_TRAIN_MASK   (7 << 8)
> -- 
> 2.17.1
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [Outreachy][VKMS] Apply to VKMS

[Navare, Manasi D]

Hi Lorrany,

Its great to hear that you are interested in VKMS project. 
Is this your first time in the DRM subsystem or the Linux kernel? First of all,
welcome!

In order to improve your experience, we selected a set of tutorials that may
help you to get into the DRM subsystem:

1. This project is about VKMS and IGT, which means that you don't need real
hardware for work in this project. You need to work with a virtual machine; in
this sense, we recommend the following tutorial for preparing your work
environment:

https://flusp.ime.usp.br/others/2019/02/15/use-qemu-to-play-with-linux/

2. If you had success in setup your VM, you could start the most exciting part:
compile and install a kernel. Take a look at this tutorial:

https://flusp.ime.usp.br/others/2019/02/16/Kernel-compilation-and-installation/

3.
Get the IGT tools and compile and run some tests on VKMS:
https://gitlab.freedesktop.org/drm/igt-gpu-tools

4. Now, it is time to work with a single module. Take a look at this tutorial:

https://flusp.ime.usp.br/others/2019/04/07/play_with_modules/

5. Finally, prepare yourself to send a patch. Take a look at:

https://kernelnewbies.org/FirstKernelPatch

Please let me or Rodrigo Siqueira know if you have any questions.

Regards
Manasi Navare 
(mdnavare@Freenode)






Hi guys, 



I'm Lorrany Azevedo, and I'm participating in the application phase for the 
outreachy internships. I'm interested in making a contribution to VKMS, and I 
would like to ask for tips on how I can do this,  I'm new to the community and 
I never
 made any contribution to the FOSS. 




___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: 2019 Xorg Election Results

[Navare, Manasi D]

Thank you so much for giving me this opportunity to be on the X.org board. I 
look forward to active participation in the upcoming conferences as well as 
mentorship for Google SOC.
I am currently on vacation till May 16th with limited email access and will 
resume attending the board meetings after that.

Regards
Manasi

Total membership 84; total votes 65; which makes for a 77.4% voter turnout. 
Here are the results:

Board Members:
   Name   1   2   3   4   5   6  Score
   Daniel Vetter 27  10  14   6   2   6   296
   Samuel Iglesias Gonsálvez 11  10  13  15  10   6   239
   Lyude Paul10  12  13  12  12   6   238
   Manasi Navare  8  13   7  16  18   3   228
   Trevor Woerner 4  14  10  10   8  19   199
   Arkadiusz Hiler5   6   8   6  15  25   165

So that means our new board members are:

   Daniel Vetter
   Samuel Iglesias Gonsálvez
   Lyude Paul
   Manasi Navare

Please welcome our new members to the board!

In the interest of transparency I should mention that one person accidentally 
signed up twice and voted twice. Luckily this doesn't change the results of the 
election since there is more than a 6-point gap between 4th and 5th place. 
We'll take this as a reminder to have a better look at membership sign-ups.

It should also be noted that even though our election process as outlined at 
https://www.x.org/wiki/BoardOfDirectors/Elections/ allows denying candidates 
any points our website didn't take this into account and forced voters to rank 
every candidate. Due to the lack of controversy about the candidates we don't 
expect this to have had any impact on the final results.

Thanks,
Harry Wentland,
On behalf of the Xorg Elections Committee   
___
memb...@foundation.x.org: X.Org Foundation Members
Archives: https://foundation.x.org/cgi-bin/mailman/private/members
Info: https://foundation.x.org/cgi-bin/mailman/listinfo/members
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: FOSDEM 2018: Graphics DevRoom: Call for speaker.

[Navare, Manasi D]

Hi Luc,

I have submitted a proposal for a talk in Graphics Dev Room, but haven’t heard 
anything regarding the selection. Is there a timeline by which the 
notifications will be sent so that the travel can be planned accordingly?

Has anyone else heard about the submissions?

Regards
Manasi


Hi,

At FOSDEM on saturday the 3rd of february 2018, there will be another graphics 
DevRoom. URL: https://fosdem.org/2018/

The focus of this DevRoom is of course the same as the previous editions, 
namely:
* Graphics drivers: from display to media to 3d drivers, both in kernel or 
userspace. Be it part of DRM, KMS, (direct)FB, V4L, Xorg, Mesa...
* Input drivers: kernel and userspace.
* Windowing systems: X, Wayland, Mir, directFB, ...
* Even colour management, low level toolkit stuff, and other areas which i 
might have overlooked above are accepted.

Slots will be handed out on a first come, first serve basis. The best slots 
will go to those who apply the earliest. We have the devroom from
10:30 til 19:00, giving us 8h30, so eight 50 minute talkes and one 20 minute 
talk are available.

Talk Submission:


Like the last few years, the pentabarf system will be used for talk submission.

https://penta.fosdem.org/submission/FOSDEM18

Remember that FOSDEM is not like XDC, it's not some 50 odd people meeting with 
a sliding schedule which only gets filled out on the last day. Upwards of 1 
people are visiting this event, and most of them get a printed booklet or use 
the schedule on the FOSDEM website or an app for their phone to figure out what 
to watch or participate in next. 
So please put some effort in your talk submission and details.

Since this an open source community event, please refrain from turning in a 
talk that is a pure corporate or product commercial. Also, if you are unsure on 
whether you can come or not (this is FOSDEM, why are you not there anyway?), 
please wait with submitting your talk. Submitting a talk and then not turning 
up because you could not be bothered is a sure-fire way to get larted and then 
to never be allowed to talk again.

When in pentabarf, please give the abstract and description, for both the event 
and the speaker, some thought. The abstract should be a shortened description, 
and the event abstract will sometimes even be printed directly in the booklet. 
BUT, on the website the abstract is immediately followed by the full 
description. If your abstract is fully descriptive, while terse, you might get 
away with just the abstract.

All talks will be recorded, and will be streamed out live, and will later be 
made available as CC-BY after a few days.

As for deadlines, the fosdem organizers want to have a finished schedule by the 
15th of december. Don't count on this deadline: first come first serve! The 
worst slots will be assigned to those who come last, which could be pretty dire 
given that there is the traditional FOSDEM beer event the night before ;)

Please try to re-use your accounts from the previous years, i hope that this 
year you can actually recycle your data. If you have forgotten your password, 
then you can reset it here: 
https://penta.fosdem.org/user/forgot_password If there are any issues, just 
poke me here or on IRC.

Necessary information:
--

Below is a list of what i need to see filled in in pentabarf when you apply for 
a devroom before i consider it a valid submission. Remember: 
first come, first serve. The best slots (which are on saturday
afternoon) are for the earliest submissions.

On your personal page:
* General:
  * First and last name
  * Nickname
  * Image
* Contact:
  * email address
  * mobile number (this is a very hard requirement as there will be no
   other reliable form of emergency communication on the day)
* Description:
  * Abstract
  * Description

Create an event:
* On the General page:
  * Event title
  * Event subtitle.
  * Track: Graphics Devroom
  * Event type: Lecture (talk) or Meeting (BoF)
* Persons:
  * Add yourself as speaker.
* Description:
  * Abstract:
  * Full Description
* Links:
  * Add relevant links.

Everything else can be ignored or will be filled in by me or the FOSDEM 
organizers. Remember, i will only schedule your talk after the basics are 
somewhat filled in (you still can change them until december 15th).

I will be keeping a keen eye on your submissions and will come back with 
further questions or make small fixes as needed. Feel free to poke me with any 
questions or anything, both on irc (libv@freenode) and on email.

That's about it. Hope to see you all at FOSDEM :)

Luc Verhaegen.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [RFC PATCH xserver] modesetting: re-set the crtc's mode when link-status goes BAD

[Navare, Manasi D]

On Fri, Feb 24, 2017 at 12:09:51PM -0800, Manasi Navare wrote:
> Hi Daniel,
> 
> We have ACKs on the userspace design from both Adams and Eric.
> Is this enough to merge the kernel patches?
> 
> I spoke to Eric briefly about this and he gave me a verbal r-b as well.
> He said the userspace patches cant get merged unless DRM patches are merged.
> 
> So what should be some of our next steps here?

Still needs review on the kernel patches themselves, iirc Jani still had some 
opens on that. But I was out of the loop for 2  weeks :-) -Daniel

Thanks for merging the 1st patch in the kernel series. Are there any opens on 
the 2nd patch (the i915 patch)?
I wanted to actually respin the 2nd patch to add reset for intel_dp->lane_count 
 on the link training failure so that it doesn't accidently retrain the link 
with the stale/failed values. 

Regards
Manasi

> 
> Regards
> Manasi
> 
> 
> On Mon, Feb 13, 2017 at 01:05:17PM -0800, Eric Anholt wrote:
> > Martin Peres  writes:
> > 
> > > On 06/02/17 17:50, Martin Peres wrote:
> > >> On 03/02/17 10:04, Daniel Vetter wrote:
> > >>> On Fri, Feb 03, 2017 at 01:30:14AM +0200, Martin Peres wrote:
> >  On 01/02/17 22:05, Manasi Navare wrote:
> > > On Wed, Feb 01, 2017 at 11:58:16AM -0800, Eric Anholt wrote:
> > >> Jani Nikula  writes:
> > >>
> > >>> On Tue, 31 Jan 2017, Eric Anholt  wrote:
> >  Martin Peres  writes:
> > 
> > > Despite all the careful planing of the kernel, a link may 
> > > become insufficient to handle the currently-set mode. At 
> > > this point, the kernel should mark this particular 
> > > configuration as being broken and potentially prune the 
> > > mode before setting the offending connector's link-status 
> > > to BAD and send the userspace a hotplug event. This may 
> > > happen right after a modeset or later on.
> > >
> > > When available, we should use the link-status information 
> > > to reset the wanted mode.
> > >
> > > Signed-off-by: Martin Peres 
> >  If I understand this right, there are two failure modes 
> >  being
> >  handled:
> > 
> >  1) A mode that won't actually work because the link isn't 
> >  good enough.
> > 
> >  2) A mode that should work, but link parameters were too 
> >  optimistic and if we just ask the kernel to set the mode 
> >  again it'll use more conservative parameters that work.
> > 
> >  This patch seems good for 2).  For 1), the 
> >  drmmode_set_mode_major is going to set our old mode back.  
> >  Won't the modeset then fail to link train again, and bring 
> >  us back into this loop?  The only escape that I see would 
> >  be some other userspace responding to the advertised mode 
> >  list changing, and then asking X to modeset to something 
> >  new.
> > 
> >  To avoid that failure busy loop, should we re-fetching 
> >  modes at this point, and only re-setting if our mode still exists?
> > >>> Disclaimer: I don't know anything about the internals of the 
> > >>> modesetting driver.
> > >>>
> > >>> Perhaps we can identify the two cases now, but I'd put this 
> > >>> more
> > >>> generally: if the link status has gone bad, it's an 
> > >>> indicator to userspace that the circumstances may have 
> > >>> changed, and userspace should query the kernel for the list 
> > >>> of available modes again. It should no longer trust 
> > >>> information obtained prior to getting the bad link status, 
> > >>> including the current mode.
> > >>>
> > >>> But specifically, I think you're right, and AFAICT asking 
> > >>> for the list of modes again is the only way for the 
> > >>> userspace to distinguish between the two cases. I don't 
> > >>> think there's a shortcut for deciding the current mode is 
> > >>> still valid.
> > >> To avoid the busy-loop problem, I think I'd like this patch 
> > >> to
> > >> re-query
> > >> the kernel to ask about the current mode list, and only try to re-set
> > >> the mode if our mode is still there.
> > >>
> > >> If the mode isn't there, then it's up to the DE to take action in
> > >> response to the notification of new modes.  If you don't have a DE to
> > >> take appropriate action, you're kind of out of luck.
> > >>
> > >> As far as the ABI goes, this seems fine to me.  The only concern I 
> > >> had
> > >> about ABI was having to walk all the connectors on every uevent to 
> > >> see
> > >> if any had gone bad -- couldn't we have a flag of some sort about 
> > >> what
> > >> the uevent indicates?  But uevents should be super 

RE: [PATCH v2] drm: Add DPCD definitions for DP 1.4 DSC feature

[Navare, Manasi D]

On Fri, 17 Feb 2017, Manasi Navare  wrote:
> Display stream compression is supported on DP 1.4 DP devices. This 
> patch adds the corersponding DPCD register definitions for DSC.
>
> v2:
> * Rebased on drm-tip
>
> Signed-off-by: Manasi Navare 
> Cc: Jani Nikula 
> Cc: Paulo Zanoni 
> Cc: dri-devel@lists.freedesktop.org

Tedious work to cross check this stuff against the spec... found one real 
issue; while at it please fix a few nitpicks that I would otherwise have 
ignored.

BR,
Jani.


> ---
>  include/drm/drm_dp_helper.h | 102 
> 
>  1 file changed, 102 insertions(+)
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h 
> index ba89295..4c1fc26 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -179,6 +179,108 @@
>  
>  #define DP_GUID  0x030   /* 1.2 */
>  
> +#define DP_DSC_SUPPORT  0x060   /* DP 1.4 */
> +# define DP_DSC_DECOMPRESSION_IS_SUPPORTED  (1 << 0)
> +
> +#define DP_DSC_REV  0x061
> +# define DP_DSC_MAJOR_MASK  (15 << 0)
> +# define DP_DSC_MINOR_MASK  (15 << 4)
> +# define DP_DSC_MINOR_SHIFT 4

Nitpick: Hex is preferred for masks. Same for all masks below. MAJOR_SHIFT for 
completeness.

So should I add the MAJOR_SHIFT as well even though it will just 0?


> +
> +#define DP_DSC_RC_BUF_BLK_SIZE  0x062
> +# define DP_DSC_RC_BUF_BLK_SIZE_1   0x0
> +# define DP_DSC_RC_BUF_BLK_SIZE_4   0x1
> +# define DP_DSC_RC_BUF_BLK_SIZE_16  0x2
> +# define DP_DSC_RC_BUF_BLK_SIZE_64  0x3
> +
> +#define DP_DSC_RC_BUF_SIZE  0x063
> +
> +#define DP_DSC_SLICE_CAP_1  0x064
> +# define DP_DSC_1_PER_DP_DSC_SINK   (1 << 0)
> +# define DP_DSC_2_PER_DP_DSC_SINK   (1 << 1)
> +# define DP_DSC_4_PER_DP_DSC_SINK   (1 << 3)
> +# define DP_DSC_6_PER_DP_DSC_SINK   (1 << 4)
> +# define DP_DSC_8_PER_DP_DSC_SINK   (1 << 5)
> +# define DP_DSC_10_PER_DP_DSC_SINK  (1 << 6)
> +# define DP_DSC_12_PER_DP_DSC_SINK  (1 << 7)
> +
> +#define DP_DSC_LINE_BUF_BIT_DEPTH   0x065
> +# define DP_DSC_LINE_BUF_BIT_DEPTH_MASK (15 << 0)
> +# define DP_DSC_LINE_BUF_BIT_DEPTH_90x0
> +# define DP_DSC_LINE_BUF_BIT_DEPTH_10   0x1
> +# define DP_DSC_LINE_BUF_BIT_DEPTH_11   0x2
> +# define DP_DSC_LINE_BUF_BIT_DEPTH_12   0x3
> +# define DP_DSC_LINE_BUF_BIT_DEPTH_13   0x4
> +# define DP_DSC_LINE_BUF_BIT_DEPTH_14   0x5
> +# define DP_DSC_LINE_BUF_BIT_DEPTH_15   0x6
> +# define DP_DSC_LINE_BUF_BIT_DEPTH_16   0x7
> +# define DP_DSC_LINE_BUF_BIT_DEPTH_80x8
> +
> +#define DP_DSC_BLK_PREDICTION_SUPPORT   0x066
> +# define DP_DSC_BLK_PREDICTION_IS_SUPPORTED (1 << 0)
> +
> +#define DP_DSC_MAX_BITS_PER_PIXEL_LOW   0x067   /* eDP 1.4 */
> +
> +#define DP_DSC_MAX_BITS_PER_PIXEL_HI0x068   /* eDP 1.4 */
> +
> +#define DP_DSC_DEC_COLOR_FORMAT_CAP 0x069
> +# define DP_DSC_RGB (1 << 0)
> +# define DP_DSC_YCbCr444(1 << 1)
> +# define DP_DSC_YCbCr422_Simple (1 << 2)
> +# define DP_DSC_YCbCr422_Native (1 << 3)
> +# define DP_DSC_YCbCr420_Native (1 << 4)
> +
> +#define DP_DSC_DEC_COLOR_DEPTH_CAP  0x06A
> +# define DP_DSC_8_BPC   (1 << 1)
> +# define DP_DSC_10_BPC  (1 << 2)
> +# define DP_DSC_12_BPC  (1 >> 3)

Oops, shifting to wrong direction!

Oops, yes that is my mistake, I will fix the shift direction.

> +
> +#define DP_DSC_PEAK_THROUGHPUT  0x06B
> +# define DP_DSC_THROUGHPUT_MODE_0_340   0x1
> +# define DP_DSC_THROUGHPUT_MODE_0_400   0x2
> +# define DP_DSC_THROUGHPUT_MODE_0_450   0x3
> +# define DP_DSC_THROUGHPUT_MODE_0_500   0x4
> +# define DP_DSC_THROUGHPUT_MODE_0_550   0x5
> +# define DP_DSC_THROUGHPUT_MODE_0_600   0x6
> +# define DP_DSC_THROUGHPUT_MODE_0_650   0x7
> +# define DP_DSC_THROUGHPUT_MODE_0_700   0x8
> +# define DP_DSC_THROUGHPUT_MODE_0_750   0x9
> +# define DP_DSC_THROUGHPUT_MODE_0_800   0xA
> +# define DP_DSC_THROUGHPUT_MODE_0_850   0xB
> +# define DP_DSC_THROUGHPUT_MODE_0_900   0xC
> +# define DP_DSC_THROUGHPUT_MODE_0_950   0xD
> +# define DP_DSC_THROUGHPUT_MODE_0_1000  0xE

Nitpick: MODE_0_MASK and MODE_0_SHIFT for completeness. Seems inconsistent to 
use hex for MODE_0 values and dec for MODE_1 values.

For Mode 0, it would be all these values shifted by 0, should I add the shifts 
by 0 for consistency with 
MODE_1?
And yes I will add MODE_0 MASK and SHIFT.

Regards
Manasi

> +# define DP_DSC_THROUGHPUT_MODE_1_MASK  (15 << 4)
> +# define DP_DSC_THROUGHPUT_MODE_1_SHIFT 4
> +# define