[PATCH v4 0/5] MST DSC support in drm-mst

[PATCH v4 0/5] MST DSC support in drm-mst

[David Francis]

Add necessary support for MST DSC.
(Display Stream COmpression over Multi-Stream Transport)

v4: Split patchset and rebase onto drm-tip

David Francis (5):
  drm/dp-mst: Add PBN calculation for DSC modes
  drm/dp-mst: Parse FEC capability on MST ports
  drm/dp-mst: Add MST support to DP DPCD R/W functions
  drm/dp-mst: Fill branch->num_ports
  drm/dp-mst: Add helpers for querying and enabling MST DSC

 drivers/gpu/drm/drm_dp_aux_dev.c      |  12 +-
 drivers/gpu/drm/drm_dp_helper.c       |  10 +-
 drivers/gpu/drm/drm_dp_mst_topology.c | 243 ++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |   8 +-
 4 files changed, 260 insertions(+), 13 deletions(-)

-- 
2.17.1

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

[PATCH v4 1/5] drm/dp-mst: Add PBN calculation for DSC modes

[David Francis]

With DSC, bpp can be a multiple of 1/16, so
drm_dp_calc_pbn_mode is insufficient.

Add drm_dp_calc_pbn_mode_dsc, a function which is
the same as drm_dp_calc_pbn_mode, but the bpp is
in units of 1/16.

Reviewed-by: Lyude Paul <lyude@redhat.com>
Signed-off-by: David Francis <David.Francis@amd.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 43 +++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |  2 +-
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 82add736e17d..8e2e731c35c5 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3591,6 +3591,49 @@ static int test_calc_pbn_mode(void)
 	return 0;
 }
 
+/**
+ * drm_dp_calc_pbn_mode_dsc() - Calculate the PBN for a mode with DSC enabled.
+ * @clock: dot clock for the mode
+ * @dsc_bpp: dsc bits per pixel x16 (e.g. dsc_bpp = 136 is 8.5 bpp)
+ *
+ * This uses the formula in the spec to calculate the PBN value for a mode,
+ * given that the mode is using DSC
+ * Returns:
+ * PBN required for this mode
+ */
+int drm_dp_calc_pbn_mode_dsc(int clock, int dsc_bpp)
+{
+	u64 kbps;
+	s64 peak_kbps;
+	u32 numerator;
+	u32 denominator;
+
+	kbps = clock * dsc_bpp;
+
+	/*
+	 * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
+	 * The unit of 54/64Mbytes/sec is an arbitrary unit chosen based on
+	 * common multiplier to render an integer PBN for all link rate/lane
+	 * counts combinations
+	 * calculate
+	 * peak_kbps *= (1/16) bppx16 to bpp
+	 * peak_kbps *= (1006/1000)
+	 * peak_kbps *= (64/54)
+	 * peak_kbps *= 8    convert to bytes
+	 *
+	 * Divide numerator and denominator by 16 to avoid overflow
+	 */
+
+	numerator = 64 * 1006 / 16;
+	denominator = 54 * 8 * 1000 * 1000;
+
+	kbps *= numerator;
+	peak_kbps = drm_fixp_from_fraction(kbps, denominator);
+
+	return drm_fixp2int_ceil(peak_kbps);
+}
+EXPORT_SYMBOL(drm_dp_calc_pbn_mode_dsc);
+
 /* we want to kick the TX after we've ack the up/down IRQs. */
 static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr)
 {
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 2ba6253ea6d3..ddb518f2157a 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -611,7 +611,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_
 
 
 int drm_dp_calc_pbn_mode(int clock, int bpp);
-
+int drm_dp_calc_pbn_mode_dsc(int clock, int dsc_bpp);
 
 bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
 			      struct drm_dp_mst_port *port, int pbn, int slots);
-- 
2.17.1

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

Re: [PATCH v4 1/5] drm/dp-mst: Add PBN calculation for DSC modes

[Lyude Paul]

Agh, sorry to go back on this - I'm noticing a lot more changes we should have
now that I've been able apply this locally to my tree. See below:

On Thu, 2019-08-22 at 09:57 -0400, David Francis wrote:
> With DSC, bpp can be a multiple of 1/16, so
> drm_dp_calc_pbn_mode is insufficient.
> 
> Add drm_dp_calc_pbn_mode_dsc, a function which is
> the same as drm_dp_calc_pbn_mode, but the bpp is
> in units of 1/16.
> 
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> Signed-off-by: David Francis <David.Francis@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 43 +++++++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       |  2 +-
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 82add736e17d..8e2e731c35c5 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -3591,6 +3591,49 @@ static int test_calc_pbn_mode(void)
>  	return 0;
>  }
>  
> +/**
> + * drm_dp_calc_pbn_mode_dsc() - Calculate the PBN for a mode with DSC
> enabled.
> + * @clock: dot clock for the mode
> + * @dsc_bpp: dsc bits per pixel x16 (e.g. dsc_bpp = 136 is 8.5 bpp)
> + *
> + * This uses the formula in the spec to calculate the PBN value for a mode,
> + * given that the mode is using DSC
> + * Returns:
> + * PBN required for this mode
> + */
> +int drm_dp_calc_pbn_mode_dsc(int clock, int dsc_bpp)
> +{
> +	u64 kbps;
> +	s64 peak_kbps;
> +	u32 numerator;
> +	u32 denominator;
> +
> +	kbps = clock * dsc_bpp;
> +
> +	/*
> +	 * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
> +	 * The unit of 54/64Mbytes/sec is an arbitrary unit chosen based on
> +	 * common multiplier to render an integer PBN for all link rate/lane
> +	 * counts combinations
> +	 * calculate
> +	 * peak_kbps *= (1/16) bppx16 to bpp
> +	 * peak_kbps *= (1006/1000)
> +	 * peak_kbps *= (64/54)
> +	 * peak_kbps *= 8    convert to bytes
> +	 *
> +	 * Divide numerator and denominator by 16 to avoid overflow
> +	 */
> +
> +	numerator = 64 * 1006 / 16;
> +	denominator = 54 * 8 * 1000 * 1000;
> +
> +	kbps *= numerator;
> +	peak_kbps = drm_fixp_from_fraction(kbps, denominator);
> +
> +	return drm_fixp2int_ceil(peak_kbps);
> +}
> +EXPORT_SYMBOL(drm_dp_calc_pbn_mode_dsc);

I didn't notice until now that the only difference between the two functions
is

numerator = 64 * 1006 / 16;

So it doesn't seem like it's worth having two seperate functions for this, so
let's just get rid of drm_dp_calc_pbn_mode_dsc() and combine it with
drm_dp_calc_pbn_mode() by adding a bool parameter to specify whether or not
we're calculating for dsc

> +
>  /* we want to kick the TX after we've ack the up/down IRQs. */
>  static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr)
>  {
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index 2ba6253ea6d3..ddb518f2157a 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -611,7 +611,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector
> *connector, struct drm_dp_
>  
>  
>  int drm_dp_calc_pbn_mode(int clock, int bpp);
> -
> +int drm_dp_calc_pbn_mode_dsc(int clock, int dsc_bpp);
>  
>  bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr,
>  			      struct drm_dp_mst_port *port, int pbn, int
> slots);
-- 
Cheers,
	Lyude Paul

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

[PATCH v4 2/5] drm/dp-mst: Parse FEC capability on MST ports

[David Francis]

As of DP1.4, ENUM_PATH_RESOURCES returns a bit indicating
if FEC can be supported up to that point in the MST network.

The bit is the first byte of the ENUM_PATH_RESOURCES ack reply,
bottom-most bit (refer to section 2.11.9.4 of DP standard,
v1.4)

That value is needed for FEC and DSC support

Store it on drm_dp_mst_port

Signed-off-by: David Francis <David.Francis@amd.com>
Reviewed-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 2 ++
 include/drm/drm_dp_mst_helper.h       | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 8e2e731c35c5..50a044718439 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -553,6 +553,7 @@ static bool drm_dp_sideband_parse_enum_path_resources_ack(struct drm_dp_sideband
 {
 	int idx = 1;
 	repmsg->u.path_resources.port_number = (raw->msg[idx] >> 4) & 0xf;
+	repmsg->u.path_resources.fec_capable = raw->msg[idx] & 0x1;
 	idx++;
 	if (idx > raw->curlen)
 		goto fail_len;
@@ -2183,6 +2184,7 @@ static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
 			DRM_DEBUG_KMS("enum path resources %d: %d %d\n", txmsg->reply.u.path_resources.port_number, txmsg->reply.u.path_resources.full_payload_bw_number,
 			       txmsg->reply.u.path_resources.avail_payload_bw_number);
 			port->available_pbn = txmsg->reply.u.path_resources.avail_payload_bw_number;
+			port->fec_capable = txmsg->reply.u.path_resources.fec_capable;
 		}
 	}
 
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index ddb518f2157a..fa973773a4a7 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -108,6 +108,8 @@ struct drm_dp_mst_port {
 	 * audio-capable.
 	 */
 	bool has_audio;
+
+	bool fec_capable;
 };
 
 /**
@@ -312,6 +314,7 @@ struct drm_dp_port_number_req {
 
 struct drm_dp_enum_path_resources_ack_reply {
 	u8 port_number;
+	bool fec_capable;
 	u16 full_payload_bw_number;
 	u16 avail_payload_bw_number;
 };
-- 
2.17.1

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

[PATCH v4 3/5] drm/dp-mst: Add MST support to DP DPCD R/W functions

[David Francis]

Instead of having drm_dp_dpcd_read/write and
drm_dp_mst_dpcd_read/write as entry points into the
aux code, have drm_dp_dpcd_read/write handle both.

This means that DRM drivers can make MST DPCD read/writes.

v2: Fix spacing

Cc: Leo Li <sunpeng.li@amd.com>
Cc: Lyude Paul <lyude@redhat.com>
Signed-off-by: David Francis <David.Francis@amd.com>
---
 drivers/gpu/drm/drm_dp_aux_dev.c | 12 ++----------
 drivers/gpu/drm/drm_dp_helper.c  | 10 ++++++++--
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
index 0cfb386754c3..418cad4f649a 100644
--- a/drivers/gpu/drm/drm_dp_aux_dev.c
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -163,11 +163,7 @@ static ssize_t auxdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 			break;
 		}
 
-		if (aux_dev->aux->is_remote)
-			res = drm_dp_mst_dpcd_read(aux_dev->aux, pos, buf,
-						   todo);
-		else
-			res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
+		res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
 
 		if (res <= 0)
 			break;
@@ -215,11 +211,7 @@ static ssize_t auxdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 			break;
 		}
 
-		if (aux_dev->aux->is_remote)
-			res = drm_dp_mst_dpcd_write(aux_dev->aux, pos, buf,
-						    todo);
-		else
-			res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
+		res = drm_dp_mst_dpcd_write(aux_dev->aux, pos, buf, todo);
 
 		if (res <= 0)
 			break;
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index ffc68d305afe..b1599760b6a3 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -30,6 +30,7 @@
 #include <linux/seq_file.h>
 
 #include <drm/drm_dp_helper.h>
+#include <drm/drm_dp_mst_helper.h>
 #include <drm/drm_print.h>
 #include <drm/drm_vblank.h>
 
@@ -251,7 +252,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 
 /**
  * drm_dp_dpcd_read() - read a series of bytes from the DPCD
- * @aux: DisplayPort AUX channel
+ * @aux: DisplayPort AUX channel (SST or MST)
  * @offset: address of the (first) register to read
  * @buffer: buffer to store the register values
  * @size: number of bytes in @buffer
@@ -268,6 +269,8 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
 {
 	int ret;
 
+	if (aux->is_remote)
+		return drm_dp_mst_dpcd_read(aux, offset, buffer, size);
 	/*
 	 * HP ZR24w corrupts the first DPCD access after entering power save
 	 * mode. Eg. on a read, the entire buffer will be filled with the same
@@ -296,7 +299,7 @@ EXPORT_SYMBOL(drm_dp_dpcd_read);
 
 /**
  * drm_dp_dpcd_write() - write a series of bytes to the DPCD
- * @aux: DisplayPort AUX channel
+ * @aux: DisplayPort AUX channel (SST or MST)
  * @offset: address of the (first) register to write
  * @buffer: buffer containing the values to write
  * @size: number of bytes in @buffer
@@ -313,6 +316,9 @@ ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
 {
 	int ret;
 
+	if (aux->is_remote)
+		return drm_dp_mst_dpcd_write(aux, offset, buffer, size);
+
 	ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer,
 				 size);
 	drm_dp_dump_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer, ret);
-- 
2.17.1

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

Re: [PATCH v4 3/5] drm/dp-mst: Add MST support to DP DPCD R/W functions

[Lyude Paul]

Mhhh, one last nitpick

On Thu, 2019-08-22 at 09:57 -0400, David Francis wrote:
> Instead of having drm_dp_dpcd_read/write and
> drm_dp_mst_dpcd_read/write as entry points into the
> aux code, have drm_dp_dpcd_read/write handle both.
> 
> This means that DRM drivers can make MST DPCD read/writes.
> 
> v2: Fix spacing
> 
> Cc: Leo Li <sunpeng.li@amd.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: David Francis <David.Francis@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_aux_dev.c | 12 ++----------
>  drivers/gpu/drm/drm_dp_helper.c  | 10 ++++++++--
>  2 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
> b/drivers/gpu/drm/drm_dp_aux_dev.c
> index 0cfb386754c3..418cad4f649a 100644
> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> @@ -163,11 +163,7 @@ static ssize_t auxdev_read_iter(struct kiocb *iocb,
> struct iov_iter *to)
>  			break;
>  		}
>  
> -		if (aux_dev->aux->is_remote)
> -			res = drm_dp_mst_dpcd_read(aux_dev->aux, pos, buf,
> -						   todo);
> -		else
> -			res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
> +		res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
>  
>  		if (res <= 0)
>  			break;
> @@ -215,11 +211,7 @@ static ssize_t auxdev_write_iter(struct kiocb *iocb,
> struct iov_iter *from)
>  			break;
>  		}
>  
> -		if (aux_dev->aux->is_remote)
> -			res = drm_dp_mst_dpcd_write(aux_dev->aux, pos, buf,
> -						    todo);
> -		else
> -			res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
> +		res = drm_dp_mst_dpcd_write(aux_dev->aux, pos, buf, todo);
>  
>  		if (res <= 0)
>  			break;
> diff --git a/drivers/gpu/drm/drm_dp_helper.c
> b/drivers/gpu/drm/drm_dp_helper.c
> index ffc68d305afe..b1599760b6a3 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -30,6 +30,7 @@
>  #include <linux/seq_file.h>
>  
>  #include <drm/drm_dp_helper.h>
> +#include <drm/drm_dp_mst_helper.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_vblank.h>
>  
> @@ -251,7 +252,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8
> request,
>  
>  /**
>   * drm_dp_dpcd_read() - read a series of bytes from the DPCD
> - * @aux: DisplayPort AUX channel
> + * @aux: DisplayPort AUX channel (SST or MST)
>   * @offset: address of the (first) register to read
>   * @buffer: buffer to store the register values
>   * @size: number of bytes in @buffer
> @@ -268,6 +269,8 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux,
> unsigned int offset,
>  {
>  	int ret;
>  
> +	if (aux->is_remote)
> +		return drm_dp_mst_dpcd_read(aux, offset, buffer, size);

Let's actually replace this with

if (aux->is_remote)
	ret = drm_dp_mst_dpcd_read(aux, offset, buffer, size);
else
	ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer,
	                         size);

That way we still call drm_dp_dump_access() so that we can log the DPCD read
to the kernel log.

Let's also add a space down here ⬇...
>  	/*
>  	 * HP ZR24w corrupts the first DPCD access after entering power save
>  	 * mode. Eg. on a read, the entire buffer will be filled with the same
> @@ -296,7 +299,7 @@ EXPORT_SYMBOL(drm_dp_dpcd_read);
>  
>  /**
>   * drm_dp_dpcd_write() - write a series of bytes to the DPCD
> - * @aux: DisplayPort AUX channel
> + * @aux: DisplayPort AUX channel (SST or MST)
>   * @offset: address of the (first) register to write
>   * @buffer: buffer containing the values to write
>   * @size: number of bytes in @buffer
> @@ -313,6 +316,9 @@ ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux,
> unsigned int offset,
>  {
>  	int ret;
>  
> +	if (aux->is_remote)
> +		return drm_dp_mst_dpcd_write(aux, offset, buffer, size);
> +

...and do the same thing with drm_dp_dpcd_write() that we did with
drm_dp_dpcd_read()

With those changes, this patch is:

Reviewed-by: Lyude Paul <lyude@redhat.com>
>  	ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer,
>  				 size);
>  	drm_dp_dump_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer, ret);
-- 
Cheers,
	Lyude Paul

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

[PATCH v4 4/5] drm/dp-mst: Fill branch->num_ports

[David Francis]

This field on drm_dp_mst_branch was never filled

Initialize it to zero when the list of ports is created.
When a port is added to the list, increment num_ports,
and when a port is removed from the list, decrement num_ports.

v2: remember to decrement on port removal

Signed-off-by: David Francis <David.Francis@amd.com>
Reviewed-by: Lyude Paul <lyude@redhat.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 50a044718439..af4b5cec7c84 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -919,6 +919,7 @@ static struct drm_dp_mst_branch *drm_dp_add_mst_branch_device(u8 lct, u8 *rad)
 	INIT_LIST_HEAD(&mstb->ports);
 	kref_init(&mstb->topology_kref);
 	kref_init(&mstb->malloc_kref);
+	mstb->num_ports = 0;
 	return mstb;
 }
 
@@ -1669,6 +1670,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
 		mutex_lock(&mstb->mgr->lock);
 		drm_dp_mst_topology_get_port(port);
 		list_add(&port->next, &mstb->ports);
+		mstb->num_ports++;
 		mutex_unlock(&mstb->mgr->lock);
 	}
 
@@ -1703,6 +1705,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
 			/* remove it from the port list */
 			mutex_lock(&mstb->mgr->lock);
 			list_del(&port->next);
+			mstb->num_ports--;
 			mutex_unlock(&mstb->mgr->lock);
 			/* drop port list reference */
 			drm_dp_mst_topology_put_port(port);
-- 
2.17.1

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

Re: [PATCH v4 4/5] drm/dp-mst: Fill branch->num_ports

[Lyude Paul]

On Thu, 2019-08-22 at 09:57 -0400, David Francis wrote:
> This field on drm_dp_mst_branch was never filled
> 
> Initialize it to zero when the list of ports is created.
> When a port is added to the list, increment num_ports,
> and when a port is removed from the list, decrement num_ports.
> 
> v2: remember to decrement on port removal
> 
> Signed-off-by: David Francis <David.Francis@amd.com>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 50a044718439..af4b5cec7c84 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -919,6 +919,7 @@ static struct drm_dp_mst_branch
> *drm_dp_add_mst_branch_device(u8 lct, u8 *rad)
>  	INIT_LIST_HEAD(&mstb->ports);
>  	kref_init(&mstb->topology_kref);
>  	kref_init(&mstb->malloc_kref);
> +	mstb->num_ports = 0;

Just realized as well, we can drop the mstb->num_ports = 0; here, since we're
allocating the struct with kzalloc()

>  	return mstb;
>  }
>  
> @@ -1669,6 +1670,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
> *mstb,
>  		mutex_lock(&mstb->mgr->lock);
>  		drm_dp_mst_topology_get_port(port);
>  		list_add(&port->next, &mstb->ports);
> +		mstb->num_ports++;
>  		mutex_unlock(&mstb->mgr->lock);
>  	}
>  
> @@ -1703,6 +1705,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
> *mstb,
>  			/* remove it from the port list */
>  			mutex_lock(&mstb->mgr->lock);
>  			list_del(&port->next);
> +			mstb->num_ports--;
>  			mutex_unlock(&mstb->mgr->lock);
>  			/* drop port list reference */
>  			drm_dp_mst_topology_put_port(port);
-- 
Cheers,
	Lyude Paul

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

[PATCH v4 5/5] drm/dp-mst: Add helpers for querying and enabling MST DSC

[David Francis]

Add drm_dp_mst_dsc_caps_for_port and drm_dp_mst_dsc_enable,
two helper functions for MST DSC

The former, given a port, returns the raw DPCD DSC caps off
that port.

The latter, given a port, enables or disables DSC on that port.

In both cases, the port given as input should be a leaf of
the MST tree with an attached display.

The logic for this is somewhat complicated, as DSC can be
enabled in 4 different ways.

Case 1: DP-to-DP peer device
if the branch immediately upstream has
 - PDT = DP_PEER_DEVICE_DP_MST_BRANCHING (2)
 - DPCD rev. >= DP 1.4
 - Exactly one input and one output
 - The output has PDT = DP_PEER_DEVICE_SST_SINK (3)

In this case, DSC could be possible either on the endpoint
or the peer device. Prefer the endpoint, which is possible if
 - The endpoint has DP_DSC_DECOMPRESSION_IS_SUPPORTED bit set
 - The endpoint has DP_FEC_CAPABLE bit set
 - The peer device has DSC_PASSTHROUGH_CAPABILITY bit set (from DP v2.0)

Otherwise, use the peer device

Case 2: DP-to-HDMI peer device
If the output port has
 - PDT = DP_PEER_DEVICE_DP_LEGACY_CONV (4)
 - DPCD rev. >= DP 1.4
 - LDPS = true
 - MCS = false

In this case, DSC can only be attempted on the peer device
(the output port)

Case 3: Virtual DP Sink (Internal Display Panel)
If the output port has
 - DPCD rev. >= DP 1.4
 - port_num >= 8

In this case, DSC can only be attempted on the peer device
(the output port)

Case 4: Synaptix Workaround
If the output has
 - link DPCD rev. >= DP 1.4
 - link branch_dev_id = 0x90CC24 (Synaptix)
 - There is exactly one branch device between the link and output

In this case, DSC can be attempted, but only using the *link*
aux device's caps. This is a quirk.

Cc: Lyude Paul <lyude@redhat.com>
Cc: Wenjing Liu <Wenjing.Liu@amd.com>
Cc: Nikola Cornij <Nikola.Cornij@amd.com>
Signed-off-by: David Francis <David.Francis@amd.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 195 ++++++++++++++++++++++++++
 include/drm/drm_dp_mst_helper.h       |   3 +
 2 files changed, 198 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index af4b5cec7c84..00ddc54af65b 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -4186,3 +4186,198 @@ static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_aux *aux)
 {
 	i2c_del_adapter(&aux->ddc);
 }
+
+/**
+ * drm_dp_mst_is_virtual_dpcd() - Is the given port a virtual DPCD device?
+ * @port: The port to check
+ *
+ * Returns:
+ * true if the port is a virtual DPCD peer device, false otherwise
+ */
+static bool drm_dp_mst_is_virtual_dpcd(struct drm_dp_mst_port *port)
+{
+	struct drm_dp_mst_port *downstream_port;
+
+	if (!port)
+		return false;
+
+	/* Virtual DP Sink (Internal Display Panel) */
+	if (port->port_num >= 8 && port->dpcd_rev >= DP_DPCD_REV_14)
+		return true;
+
+	/* DP-to-HDMI Protocol Converter */
+	if (port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV &&
+			!port->mcs &&
+			port->ldps &&
+			port->dpcd_rev >= DP_DPCD_REV_14)
+		return true;
+
+	/* DP-to-DP */
+	if (port->pdt == DP_PEER_DEVICE_MST_BRANCHING &&
+			port->mstb &&
+			port->dpcd_rev >= DP_DPCD_REV_14 &&
+			port->mstb->num_ports == 2) {
+		list_for_each_entry(downstream_port, &port->mstb->ports, next) {
+			if (!downstream_port->input &&
+				downstream_port->pdt == DP_PEER_DEVICE_SST_SINK)
+				return true;
+		}
+	}
+
+	return false;
+}
+
+/**
+ * drm_dp_mst_is_virtual_dpcd() - Test for Synaptix DSC quirk
+ * @port: The port to check
+ *
+ * Some Synaptix MST hubs support DSC even though they do not support virtual
+ * DPCD. This is a quirk.
+ *
+ * Returns:
+ * true if the Synaptix workaround is required, false otherwise
+ */
+static bool drm_dp_mst_dsc_synaptix_workaround(struct drm_dp_mst_port *port)
+{
+	u8 data[3] = { 0 };
+	u32 dev_id;
+	struct drm_dp_aux *phys_aux;
+
+	/* The hub must be directly connected to the connector */
+	if (port->mgr->mst_primary != port->parent)
+		return false;
+
+	phys_aux = port->mgr->aux;
+	if (drm_dp_dpcd_read(phys_aux, DP_BRANCH_OUI, data, 3) < 0)
+		return false;
+	dev_id = (data[0] << 16) & (data[1] << 8) & data[3];
+	/* Synaptix device ID */
+	if (dev_id != 0x90CC24)
+		return false;
+
+	if (drm_dp_dpcd_read(phys_aux, DP_DPCD_REV, data, 1) < 0)
+		return false;
+	/* Must be DPCD rev. 1.4 or later */
+	if (data[0] < DP_DPCD_REV_14)
+		return false;
+
+	if (drm_dp_dpcd_read(&port->aux,
+			DP_DOWNSTREAMPORT_PRESENT, data, 1) < 0)
+		return false;
+	/* Must not be a VGA converter */
+	if ((data[0] & 7) == 3)
+		return false;
+
+	return true;
+}
+
+/**
+ * drm_dp_mst_dsc_aux_for_port() - Find the correct aux for DSC
+ * @port: The port to check. A leaf of the MST tree with an attached display.
+ *
+ * Depending on the situation, DSC may be enabled via the endpoint aux,
+ * the immediately upstream aux, or the connector's physical aux.
+ *
+ * Returns:
+ * NULL if DSC cannot be enabled on this port, otherwise the aux device
+ */
+struct drm_dp_aux *drm_dp_mst_dsc_aux_for_port(struct drm_dp_mst_port *port)
+{
+	u8 upstream_dsc = 0;
+	u8 endpoint_dsc = 0;
+	u8 endpoint_fec = 0;
+	struct drm_dp_mst_port *immediate_upstream_port;
+	struct drm_dp_mst_port *fec_port;
+
+	if (port && port->parent)
+		immediate_upstream_port = port->parent->port_parent;
+	else
+		immediate_upstream_port = NULL;
+
+	fec_port = immediate_upstream_port;
+	while (fec_port) {
+		if (!fec_port->fec_capable)
+			return NULL;
+
+		fec_port = fec_port->parent->port_parent;
+	}
+
+	if (immediate_upstream_port) {
+		if (drm_dp_dpcd_read(&immediate_upstream_port->aux,
+				DP_DSC_SUPPORT, &upstream_dsc, 1) < 0)
+			return NULL;
+	}
+
+	if (drm_dp_dpcd_read(&port->aux,
+			DP_DSC_SUPPORT, &endpoint_dsc, 1) < 0)
+		return NULL;
+	if (drm_dp_dpcd_read(&port->aux,
+			DP_FEC_CAPABILITY, &endpoint_fec, 1) < 0)
+		return NULL;
+
+	/* Enpoint decompression with DP-to-DP peer device */
+	if (drm_dp_mst_is_virtual_dpcd(immediate_upstream_port)
+			&& (upstream_dsc & 0x2) /* DSC passthrough */
+			&& (endpoint_fec & DP_FEC_CAPABLE)
+			&& (endpoint_dsc & DP_DSC_DECOMPRESSION_IS_SUPPORTED))
+		return &port->aux;
+
+	/* Virtual DPCD decompression with DP-to-DP peer device */
+	if (drm_dp_mst_is_virtual_dpcd(immediate_upstream_port))
+		return &immediate_upstream_port->aux;
+
+	/* Virtual DPCD decompression with DP-to-HDMI or Virtual DP Sink */
+	if (drm_dp_mst_is_virtual_dpcd(port))
+		return &port->aux;
+
+	/* Synaptix workaround */
+	if (drm_dp_mst_dsc_synaptix_workaround(port))
+		return port->mgr->aux;
+
+	return NULL;
+}
+
+/**
+ * drm_dp_mst_dsc_aux_for_port() - Retrieve the DSC capability registers
+ * @port: The port to check. A leaf of the MST tree with an attached display.
+ * @caps: Output.  A pointer to an array at least 16 bytes long
+ *
+ * Reads the DSC capability registers (DSC_SUPPORT through
+ * BITS_PER_PIXEL_INCREMENT) and store them in the given pointer. Use
+ * the correct aux for DSC on the given port.
+ *
+ * Returns:
+ * The number of bytes read on success, or a negative error code on failure
+ */
+int drm_dp_mst_dsc_caps_for_port(struct drm_dp_mst_port *port, u8 *caps)
+{
+	struct drm_dp_aux *aux = drm_dp_mst_dsc_aux_for_port(port);
+
+	if (!aux)
+		return -EINVAL;
+
+	return drm_dp_dpcd_read(aux, DP_DSC_SUPPORT, caps, 16);
+}
+EXPORT_SYMBOL(drm_dp_mst_dsc_caps_for_port);
+
+/**
+ * drm_dp_mst_dsc_aux_for_port() - Enable DSC on an MST endpoint
+ * @port: The port to check. A leaf of the MST tree with an attached display.
+ * @enable: true for turn on DSC, false for turn off DSC
+ *
+ * Writes DP_DSC_ENABLE on the correct aux for the given port.
+ *
+ * Returns:
+ * The number of bytes written on success, or a negative error code on failure
+ */
+int drm_dp_mst_dsc_enable(struct drm_dp_mst_port *port, bool enable)
+{
+	struct drm_dp_aux *aux = drm_dp_mst_dsc_aux_for_port(port);
+	u8 enable_dsc = enable ? 1 : 0;
+
+	if (!aux)
+		return -EINVAL;
+
+	return drm_dp_dpcd_write(aux, DP_DSC_ENABLE, &enable_dsc, 1);
+}
+EXPORT_SYMBOL(drm_dp_mst_dsc_enable);
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index fa973773a4a7..0f70dc8dfbeb 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -674,6 +674,9 @@ int __must_check drm_dp_mst_atomic_check(struct drm_atomic_state *state);
 void drm_dp_mst_get_port_malloc(struct drm_dp_mst_port *port);
 void drm_dp_mst_put_port_malloc(struct drm_dp_mst_port *port);
 
+int drm_dp_mst_dsc_caps_for_port(struct drm_dp_mst_port *port, u8 *caps);
+int drm_dp_mst_dsc_enable(struct drm_dp_mst_port *port, bool enable);
+
 extern const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs;
 
 /**
-- 
2.17.1

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

Re: [PATCH v4 5/5] drm/dp-mst: Add helpers for querying and enabling MST DSC

[Lyude Paul]

On Thu, 2019-08-22 at 09:57 -0400, David Francis wrote:
> Add drm_dp_mst_dsc_caps_for_port and drm_dp_mst_dsc_enable,
> two helper functions for MST DSC
> 
> The former, given a port, returns the raw DPCD DSC caps off
> that port.
> 
> The latter, given a port, enables or disables DSC on that port.
> 
> In both cases, the port given as input should be a leaf of
> the MST tree with an attached display.
> 
> The logic for this is somewhat complicated, as DSC can be
> enabled in 4 different ways.
> 
> Case 1: DP-to-DP peer device
> if the branch immediately upstream has
>  - PDT = DP_PEER_DEVICE_DP_MST_BRANCHING (2)
>  - DPCD rev. >= DP 1.4
>  - Exactly one input and one output
>  - The output has PDT = DP_PEER_DEVICE_SST_SINK (3)
> 
> In this case, DSC could be possible either on the endpoint
> or the peer device. Prefer the endpoint, which is possible if
>  - The endpoint has DP_DSC_DECOMPRESSION_IS_SUPPORTED bit set
>  - The endpoint has DP_FEC_CAPABLE bit set
>  - The peer device has DSC_PASSTHROUGH_CAPABILITY bit set (from DP v2.0)
> 
> Otherwise, use the peer device
> 
> Case 2: DP-to-HDMI peer device
> If the output port has
>  - PDT = DP_PEER_DEVICE_DP_LEGACY_CONV (4)
>  - DPCD rev. >= DP 1.4
>  - LDPS = true
>  - MCS = false
> 
> In this case, DSC can only be attempted on the peer device
> (the output port)
> 
> Case 3: Virtual DP Sink (Internal Display Panel)
> If the output port has
>  - DPCD rev. >= DP 1.4
>  - port_num >= 8
> 
> In this case, DSC can only be attempted on the peer device
> (the output port)
> 
> Case 4: Synaptix Workaround
> If the output has
>  - link DPCD rev. >= DP 1.4
>  - link branch_dev_id = 0x90CC24 (Synaptix)
>  - There is exactly one branch device between the link and output
> 
> In this case, DSC can be attempted, but only using the *link*
> aux device's caps. This is a quirk.
> 
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Wenjing Liu <Wenjing.Liu@amd.com>
> Cc: Nikola Cornij <Nikola.Cornij@amd.com>
> Signed-off-by: David Francis <David.Francis@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 195 ++++++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       |   3 +
>  2 files changed, 198 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index af4b5cec7c84..00ddc54af65b 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -4186,3 +4186,198 @@ static void drm_dp_mst_unregister_i2c_bus(struct
> drm_dp_aux *aux)
>  {
>  	i2c_del_adapter(&aux->ddc);
>  }
> +
> +/**
> + * drm_dp_mst_is_virtual_dpcd() - Is the given port a virtual DPCD device?
> + * @port: The port to check
> + *

Mind elaborating a bit here in the kernel documentation on what a virtual DPCD
device is?

> + * Returns:
> + * true if the port is a virtual DPCD peer device, false otherwise
> + */
> +static bool drm_dp_mst_is_virtual_dpcd(struct drm_dp_mst_port *port)
> +{
> +	struct drm_dp_mst_port *downstream_port;
> +
> +	if (!port)
> +		return false;

I'd drop this, and just move the if (!port) check into
drm_dp_mst_dsc_aux_for_port(). Makes things a little more explicit.

> +
> +	/* Virtual DP Sink (Internal Display Panel) */
> +	if (port->port_num >= 8 && port->dpcd_rev >= DP_DPCD_REV_14)
> +		return true;
> +
> +	/* DP-to-HDMI Protocol Converter */
> +	if (port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV &&
> +			!port->mcs &&
> +			port->ldps &&
> +			port->dpcd_rev >= DP_DPCD_REV_14)
> +		return true;

Please fix the indenting here, most DRM code (I may have mistakenly pointed
you at the kernel style guidelines, but I realized they don't explicitly
mention this other then the rule of "your code should look like the code
around it") written with line continuations starting on the same column as the
beginning paranthesis

if (some_very_long_variable == another_very_long_variable_wow ||
    every_identifier_here_is_long > but_what_if_we_have_a_sub_conditional ||
    (then_we_end_up_formatting_it_like_this &&
     starting_at_the_beginning_paranthesis)) {
	same_for_function_calls("Which also require very long lines\n",
	                        foo, bar, baz);
}

> +
> +	/* DP-to-DP */
> +	if (port->pdt == DP_PEER_DEVICE_MST_BRANCHING &&
> +			port->mstb &&
> +			port->dpcd_rev >= DP_DPCD_REV_14 &&
> +			port->mstb->num_ports == 2) {
> +		list_for_each_entry(downstream_port, &port->mstb->ports, next)
> {
> +			if (!downstream_port->input &&
> +				downstream_port->pdt ==
> DP_PEER_DEVICE_SST_SINK)
> +				return true;
> +		}
This isn't safe, anything that iterates downwards (so either mstb->ports, or
port->mstb) in the MST topology needs to happen under &mgr->lock, since
otherwise the port list can change under us. Note there's probably some places
this isn't followed, but I'm in the process of fixing that:

https://patchwork.freedesktop.org/patch/318683/?series=63847&rev=1
> +	}
> +
> +	return false;
> +}
> +
> +/**
> + * drm_dp_mst_is_virtual_dpcd() - Test for Synaptix DSC quirk

I think you messed up a bunch of your kerneldoc comments by accident ^
> + * @port: The port to check
> + *
> + * Some Synaptix MST hubs support DSC even though they do not support
> virtual
> + * DPCD. This is a quirk.
> + *
> + * Returns:
> + * true if the Synaptix workaround is required, false otherwise
> + */
> +static bool drm_dp_mst_dsc_synaptix_workaround(struct drm_dp_mst_port
> *port)
> +{
> +	u8 data[3] = { 0 };
> +	u32 dev_id;
> +	struct drm_dp_aux *phys_aux;
> +
> +	/* The hub must be directly connected to the connector */
> +	if (port->mgr->mst_primary != port->parent)
> +		return false;
> +
> +	phys_aux = port->mgr->aux;
> +	if (drm_dp_dpcd_read(phys_aux, DP_BRANCH_OUI, data, 3) < 0)
> +		return false;
> +	dev_id = (data[0] << 16) & (data[1] << 8) & data[3];
> +	/* Synaptix device ID */
> +	if (dev_id != 0x90CC24)
> +		return false;

More things I'm noticing now that I've got this applied locally. I originally
suggested using only the dpcd_quirks list here, but it appears I didn't read
this function closely enough and missed the fact you have to read from
multiple aux ports here. Huh.

So, first off we don't need to read the ident by hand here, we should just use
the helper in drm_dp_read_desc() and add a quirk for this into
dpcd_quirk_list[]. Maybe something like
DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD. This also ives us the benefit of
having this information printed out into the kernel debug log.

Plus, you accidentally go out of bounds when you check data[3] anyway ;)

> +
> +	if (drm_dp_dpcd_read(phys_aux, DP_DPCD_REV, data, 1) < 0)
> +		return false;
> +	/* Must be DPCD rev. 1.4 or later */
> +	if (data[0] < DP_DPCD_REV_14)
> +		return false;

No need to reread the DPCD here, just use the cached dpcd in port->mgr->edid

> +
> +	if (drm_dp_dpcd_read(&port->aux,
> +			DP_DOWNSTREAMPORT_PRESENT, data, 1) < 0)
More indenting to fix

> +		return false;
> +	/* Must not be a VGA converter */
> +	if ((data[0] & 7) == 3)
Let's just use the proper macros from include/drm/drm_dp_helper.h here

> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * drm_dp_mst_dsc_aux_for_port() - Find the correct aux for DSC
> + * @port: The port to check. A leaf of the MST tree with an attached
> display.
> + *
> + * Depending on the situation, DSC may be enabled via the endpoint aux,
> + * the immediately upstream aux, or the connector's physical aux.
> + *
> + * Returns:
> + * NULL if DSC cannot be enabled on this port, otherwise the aux device
> + */
> +struct drm_dp_aux *drm_dp_mst_dsc_aux_for_port(struct drm_dp_mst_port
> *port)
> +{
> +	u8 upstream_dsc = 0;
> +	u8 endpoint_dsc = 0;
> +	u8 endpoint_fec = 0;
> +	struct drm_dp_mst_port *immediate_upstream_port;
> +	struct drm_dp_mst_port *fec_port;
> +
> +	if (port && port->parent)
> +		immediate_upstream_port = port->parent->port_parent;
> +	else
> +		immediate_upstream_port = NULL;
> +
> +	fec_port = immediate_upstream_port;
> +	while (fec_port) {
> +		if (!fec_port->fec_capable)
> +			return NULL;
> +
> +		fec_port = fec_port->parent->port_parent;
> +	}
> +
> +	if (immediate_upstream_port) {
> +		if (drm_dp_dpcd_read(&immediate_upstream_port->aux,
> +				DP_DSC_SUPPORT, &upstream_dsc, 1) < 0)
> +			return NULL;
> +	}
> +
> +	if (drm_dp_dpcd_read(&port->aux,
> +			DP_DSC_SUPPORT, &endpoint_dsc, 1) < 0)
> +		return NULL;
> +	if (drm_dp_dpcd_read(&port->aux,
> +			DP_FEC_CAPABILITY, &endpoint_fec, 1) < 0)
> +		return NULL;
> +
> +	/* Enpoint decompression with DP-to-DP peer device */
> +	if (drm_dp_mst_is_virtual_dpcd(immediate_upstream_port)
> +			&& (upstream_dsc & 0x2) /* DSC passthrough */
> +			&& (endpoint_fec & DP_FEC_CAPABLE)
> +			&& (endpoint_dsc & DP_DSC_DECOMPRESSION_IS_SUPPORTED))
> +		return &port->aux;
> +
> +	/* Virtual DPCD decompression with DP-to-DP peer device */
> +	if (drm_dp_mst_is_virtual_dpcd(immediate_upstream_port))
> +		return &immediate_upstream_port->aux;
> +
> +	/* Virtual DPCD decompression with DP-to-HDMI or Virtual DP Sink */
> +	if (drm_dp_mst_is_virtual_dpcd(port))
> +		return &port->aux;
> +
> +	/* Synaptix workaround */
> +	if (drm_dp_mst_dsc_synaptix_workaround(port))
> +		return port->mgr->aux;
> +
> +	return NULL;
> +}
> +
> +/**
> + * drm_dp_mst_dsc_aux_for_port() - Retrieve the DSC capability registers
> + * @port: The port to check. A leaf of the MST tree with an attached
> display.
> + * @caps: Output.  A pointer to an array at least 16 bytes long
> + *
> + * Reads the DSC capability registers (DSC_SUPPORT through
> + * BITS_PER_PIXEL_INCREMENT) and store them in the given pointer. Use
> + * the correct aux for DSC on the given port.
> + *
> + * Returns:
> + * The number of bytes read on success, or a negative error code on failure
> + */
> +int drm_dp_mst_dsc_caps_for_port(struct drm_dp_mst_port *port, u8 *caps)
> +{
> +	struct drm_dp_aux *aux = drm_dp_mst_dsc_aux_for_port(port);
> +
> +	if (!aux)
> +		return -EINVAL;
> +
> +	return drm_dp_dpcd_read(aux, DP_DSC_SUPPORT, caps, 16);
> +}
> +EXPORT_SYMBOL(drm_dp_mst_dsc_caps_for_port);
> +
> +/**
> + * drm_dp_mst_dsc_aux_for_port() - Enable DSC on an MST endpoint
> + * @port: The port to check. A leaf of the MST tree with an attached
> display.
> + * @enable: true for turn on DSC, false for turn off DSC
> + *
> + * Writes DP_DSC_ENABLE on the correct aux for the given port.
> + *
> + * Returns:
> + * The number of bytes written on success, or a negative error code on
> failure
> + */
> +int drm_dp_mst_dsc_enable(struct drm_dp_mst_port *port, bool enable)
> +{
> +	struct drm_dp_aux *aux = drm_dp_mst_dsc_aux_for_port(port);
> +	u8 enable_dsc = enable ? 1 : 0;
> +
> +	if (!aux)
> +		return -EINVAL;
> +
> +	return drm_dp_dpcd_write(aux, DP_DSC_ENABLE, &enable_dsc, 1);
> +}

Why not just make it so that drm_dp_mst_dsc_caps() also returns the
appropriate drm_dp_aux device to use, and then pass the pointer to
drm_dp_mst_dsc_enable() instead of passing it the drm_dp_mst_port*? That way
we won't have to probe for these caps twice.

> +EXPORT_SYMBOL(drm_dp_mst_dsc_enable);
> diff --git a/include/drm/drm_dp_mst_helper.h
> b/include/drm/drm_dp_mst_helper.h
> index fa973773a4a7..0f70dc8dfbeb 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -674,6 +674,9 @@ int __must_check drm_dp_mst_atomic_check(struct
> drm_atomic_state *state);
>  void drm_dp_mst_get_port_malloc(struct drm_dp_mst_port *port);
>  void drm_dp_mst_put_port_malloc(struct drm_dp_mst_port *port);
>  
> +int drm_dp_mst_dsc_caps_for_port(struct drm_dp_mst_port *port, u8 *caps);
> +int drm_dp_mst_dsc_enable(struct drm_dp_mst_port *port, bool enable);
> +
>  extern const struct drm_private_state_funcs
> drm_dp_mst_topology_state_funcs;
>  
>  /**
-- 
Cheers,
	Lyude Paul

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

Re: [PATCH v4 0/5] MST DSC support in drm-mst

[Lyude Paul]

Still in the process of reviewing this, but one minor change that should be
done on all of the patches (which I didn't notice before, whoops):

s:drm/dp-mst:drm/dp_mst:g

On Thu, 2019-08-22 at 09:57 -0400, David Francis wrote:
> Add necessary support for MST DSC.
> (Display Stream COmpression over Multi-Stream Transport)
> 
> v4: Split patchset and rebase onto drm-tip
> 
> David Francis (5):
>   drm/dp-mst: Add PBN calculation for DSC modes
>   drm/dp-mst: Parse FEC capability on MST ports
>   drm/dp-mst: Add MST support to DP DPCD R/W functions
>   drm/dp-mst: Fill branch->num_ports
>   drm/dp-mst: Add helpers for querying and enabling MST DSC
> 
>  drivers/gpu/drm/drm_dp_aux_dev.c      |  12 +-
>  drivers/gpu/drm/drm_dp_helper.c       |  10 +-
>  drivers/gpu/drm/drm_dp_mst_topology.c | 243 ++++++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       |   8 +-
>  4 files changed, 260 insertions(+), 13 deletions(-)
> 
-- 
Cheers,
	Lyude Paul

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

Re: [PATCH v4 0/5] MST DSC support in drm-mst

[Lyude Paul]

OK-done reviewing, but there's some stuff missing here. The PBN bandwidth
checks in https://patchwork.freedesktop.org/patch/325604/?series=65423&rev=3
need to be moved into drm_dp_mst_atomic_check(), along with moving amdgpu over to using drm_dp_mst_atomic_check(). Doing so will also give us PBN bandwidth checks in both i915 and nouveau as well, and keeps the bandwidth calculation where it should be.

Additionally, you still need to move the code here into an MST atomic helper
or drm_dp_mst_atomic_check() as well:

https://patchwork.freedesktop.org/patch/325611/?series=65423&rev=3

Unless I'm mistaken, adding each CRTC which has a connector whose PBN requires
recalculation into the atomic state is something every DRM driver is going to
need to do. And, you can do this more easily by adding PBN information into
drm_dp_mst_topology_state. Yes-it's a lot of locks, but since every connector
in an MST topology is sharing the bandwidth on the same link it's kind of
expected. I already know I'm going to have to do basically the same thing with
every driver once I've got the time to actually implement fallback link rate
retraining with MST topologies.

If you need help figuring out how to structure this in a way that works for
all drivers, I'm willing to help and I'm sure Manasi is as well.

On Thu, 2019-08-22 at 09:57 -0400, David Francis wrote:
> Add necessary support for MST DSC.
> (Display Stream COmpression over Multi-Stream Transport)
> 
> v4: Split patchset and rebase onto drm-tip
> 
> David Francis (5):
>   drm/dp-mst: Add PBN calculation for DSC modes
>   drm/dp-mst: Parse FEC capability on MST ports
>   drm/dp-mst: Add MST support to DP DPCD R/W functions
>   drm/dp-mst: Fill branch->num_ports
>   drm/dp-mst: Add helpers for querying and enabling MST DSC
> 
>  drivers/gpu/drm/drm_dp_aux_dev.c      |  12 +-
>  drivers/gpu/drm/drm_dp_helper.c       |  10 +-
>  drivers/gpu/drm/drm_dp_mst_topology.c | 243 ++++++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       |   8 +-
>  4 files changed, 260 insertions(+), 13 deletions(-)
> 
-- 
Cheers,
	Lyude Paul

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

Re: [PATCH v4 0/5] MST DSC support in drm-mst

[Francis, David]

Adding DSC functionality to drm_dp_mst_atomic_check() is a good idea.
However, until amdgpu switches over to that system, I wouldn't be able
to test those changes. Making that switch is on our TODO list, and it would
fix a number of problems with our current MST implementation, but
it's going to be a major rewrite.

MST DSC hardware is already on the market. It would be expedient to
merge the patches we need for Navi support sooner and update
drm_dp_mst_atomic_check when we're able to test it.

David Francis

________________________________________
From: Lyude Paul <lyude@redhat.com>
Sent: August 22, 2019 8:03 PM
To: Francis, David; dri-devel@lists.freedesktop.org; Manasi Navare
Subject: Re: [PATCH v4 0/5] MST DSC support in drm-mst

OK-done reviewing, but there's some stuff missing here. The PBN bandwidth
checks in https://patchwork.freedesktop.org/patch/325604/?series=65423&rev=3
need to be moved into drm_dp_mst_atomic_check(), along with moving amdgpu over to using drm_dp_mst_atomic_check(). Doing so will also give us PBN bandwidth checks in both i915 and nouveau as well, and keeps the bandwidth calculation where it should be.

Additionally, you still need to move the code here into an MST atomic helper
or drm_dp_mst_atomic_check() as well:

https://patchwork.freedesktop.org/patch/325611/?series=65423&rev=3

Unless I'm mistaken, adding each CRTC which has a connector whose PBN requires
recalculation into the atomic state is something every DRM driver is going to
need to do. And, you can do this more easily by adding PBN information into
drm_dp_mst_topology_state. Yes-it's a lot of locks, but since every connector
in an MST topology is sharing the bandwidth on the same link it's kind of
expected. I already know I'm going to have to do basically the same thing with
every driver once I've got the time to actually implement fallback link rate
retraining with MST topologies.

If you need help figuring out how to structure this in a way that works for
all drivers, I'm willing to help and I'm sure Manasi is as well.

On Thu, 2019-08-22 at 09:57 -0400, David Francis wrote:
> Add necessary support for MST DSC.
> (Display Stream COmpression over Multi-Stream Transport)
>
> v4: Split patchset and rebase onto drm-tip
>
> David Francis (5):
>   drm/dp-mst: Add PBN calculation for DSC modes
>   drm/dp-mst: Parse FEC capability on MST ports
>   drm/dp-mst: Add MST support to DP DPCD R/W functions
>   drm/dp-mst: Fill branch->num_ports
>   drm/dp-mst: Add helpers for querying and enabling MST DSC
>
>  drivers/gpu/drm/drm_dp_aux_dev.c      |  12 +-
>  drivers/gpu/drm/drm_dp_helper.c       |  10 +-
>  drivers/gpu/drm/drm_dp_mst_topology.c | 243 ++++++++++++++++++++++++++
>  include/drm/drm_dp_mst_helper.h       |   8 +-
>  4 files changed, 260 insertions(+), 13 deletions(-)
>
--
Cheers,
        Lyude Paul

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

Re: [PATCH v4 0/5] MST DSC support in drm-mst

[Dave Airlie]

On Sat, 24 Aug 2019 at 06:24, Francis, David <David.Francis@amd.com> wrote:
>
> Adding DSC functionality to drm_dp_mst_atomic_check() is a good idea.
> However, until amdgpu switches over to that system, I wouldn't be able
> to test those changes. Making that switch is on our TODO list, and it would
> fix a number of problems with our current MST implementation, but
> it's going to be a major rewrite.
>
> MST DSC hardware is already on the market. It would be expedient to
> merge the patches we need for Navi support sooner and update
> drm_dp_mst_atomic_check when we're able to test it.

Is there any commitment to rewriting it, a timeline or anything?

The problem with this situation is there is always new hardware coming
onto the market, and there is always pressure to support all the
features of that new hardware, and the pressure always comes like this
and being expedient. However I've found that a lot of the time the
required refactor or work is never done, because the time is being
allocated now to the next GPU that is coming on the market, and nobody
ever cares enough to clean up their technical debt.

How come the needs for MST DSC support wasn't identified earlier,
blocked on refactoring of the code to use common code, and then that
task made a higher priority?

I'm sorta inclined to say no we shouldn't be merging any driver
specific code here, because this is the only point we can push
pressure on to refactor the MST implementation, which I guess
otherwise we'll just keep avoiding until Lyude ends up doing it for
you.

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

Re: [PATCH v4 0/5] MST DSC support in drm-mst

[Harry Wentland]

On 2019-08-26 3:50 p.m., Dave Airlie wrote:
> On Sat, 24 Aug 2019 at 06:24, Francis, David <David.Francis@amd.com> wrote:
>>
>> Adding DSC functionality to drm_dp_mst_atomic_check() is a good idea.
>> However, until amdgpu switches over to that system, I wouldn't be able
>> to test those changes. Making that switch is on our TODO list, and it would
>> fix a number of problems with our current MST implementation, but
>> it's going to be a major rewrite.
>>
>> MST DSC hardware is already on the market. It would be expedient to
>> merge the patches we need for Navi support sooner and update
>> drm_dp_mst_atomic_check when we're able to test it.
> 
> Is there any commitment to rewriting it, a timeline or anything?
> 
> The problem with this situation is there is always new hardware coming
> onto the market, and there is always pressure to support all the
> features of that new hardware, and the pressure always comes like this
> and being expedient. However I've found that a lot of the time the
> required refactor or work is never done, because the time is being
> allocated now to the next GPU that is coming on the market, and nobody
> ever cares enough to clean up their technical debt.
> 
> How come the needs for MST DSC support wasn't identified earlier,
> blocked on refactoring of the code to use common code, and then that
> task made a higher priority?
> 

drm_dp_mst_atomic_check was introduced by Lyude back in January with
https://patchwork.freedesktop.org/patch/276405/ as part of
https://patchwork.freedesktop.org/series/54031/

At the time Lyude updated i915 and nouveau to use these helpers. amdgpu
wasn't updated.

> I'm sorta inclined to say no we shouldn't be merging any driver
> specific code here, because this is the only point we can push
> pressure on to refactor the MST implementation, which I guess
> otherwise we'll just keep avoiding until Lyude ends up doing it for
> you.
> 

That's fair. I agree that the refactor should be done and I understand
where you're coming from. Since David is heading back to school in less
than a week I was inclined to see if we can push back a little so he can
get his change in. Other than that I don't mind holding off on the merge
unless the refactor is done.

Adding Mikita who'll pick up DSC stuff from David and will iterate on
these patches if necessary and look at the MST refactor.

Thanks for keeping us honest.

Harry

> Dave.
> _______________________________________________
> 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