Re: [PATCH v2 10/23] drm/dsc: Add helpers for DSC picture parameter set infoframes
On 2018-07-31 05:07 PM, Manasi Navare wrote:
> According to Display Stream compression spec 1.2, the picture
> parameter set metadata is sent from source to sink device
> using the DP Secondary data packet. An infoframe is formed
> for the PPS SDP header and PPS SDP payload bytes.
> This patch adds helpers to fill the PPS SDP header
> and PPS SDP payload according to the DSC 1.2 specification.
>
> v5:
> Do not use bitfields for DRM structs (Jani N)
> v4:
> * Use DSC constants for params that dont change across
> configurations
> v3:
> * Add reference to added kernel-docs in Documentation/gpu/drm-kms-helpers.rst
> (Daniel Vetter)
>
> v2:
> * Add EXPORT_SYMBOL for the drm functions (Manasi)
>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Jani Nikula
> Cc: Ville Syrjala
> Cc: Anusha Srivatsa
> Cc: Harry Wentland
> Signed-off-by: Manasi Navare
Acked-by: Harry Wentland
Harry
> ---
> Documentation/gpu/drm-kms-helpers.rst | 12 ++
> drivers/gpu/drm/Makefile | 2 +-
> drivers/gpu/drm/drm_dsc.c | 221
> ++
> include/drm/drm_dsc.h | 22
> 4 files changed, 256 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/drm_dsc.c
>
> diff --git a/Documentation/gpu/drm-kms-helpers.rst
> b/Documentation/gpu/drm-kms-helpers.rst
> index f9cfcdc..50bb717 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -223,6 +223,18 @@ MIPI DSI Helper Functions Reference
> .. kernel-doc:: drivers/gpu/drm/drm_mipi_dsi.c
> :export:
>
> +Display Stream Compression Helper Functions Reference
> +=
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_dsc.c
> + :doc: dsc helpers
> +
> +.. kernel-doc:: include/drm/drm_dsc.h
> + :internal:
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_dsc.c
> + :export:
> +
> Output Probing Helper Functions Reference
> =
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index a6771ce..961e511 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -31,7 +31,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
> drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
> drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>
> -drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> +drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o
> drm_probe_helper.o \
> drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> drm_simple_kms_helper.o drm_modeset_helper.o \
> diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c
> new file mode 100644
> index 000..99a8794
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_dsc.c
> @@ -0,0 +1,221 @@
> +/*
> + *Copyright © 2018 Intel Corp
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> its
> + * documentation for any purpose is hereby granted without fee, provided that
> + * the above copyright notice appear in all copies and that both that
> copyright
> + * notice and this permission notice appear in supporting documentation, and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission. The copyright holders make no representations
> + * about the suitability of this software for any purpose. It is provided
> "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF
> USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
> PERFORMANCE
> + * OF THIS SOFTWARE.
> + *
> + * Author:
> + * Manasi Navare
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/**
> + * DOC: dsc helpers
> + *
> + * These functions contain some common logic and helpers to deal with VESA
> + * Display Stream Compression standard required for DSC on Display Port/eDP
> or
> + * MIPI display interfaces.
> + */
> +
> +/**
> + * drm_dsc_dp_pps_header_init() - Initializes the PPS Header
> + * for DisplayPort as per the DP 1.4 spec.
> + * @pps_sdp: Secondary data packet for DSC Picture Parameter Set
> + */
> +void drm_dsc_dp_pps_header_init(struct drm_dsc_pps_infoframe *pps_sdp)
> +{
> + memset(_sdp->pps_header, 0, sizeof(pps_sdp->pps_header));
> +
> + pps_sdp->pps_header.HB1 = DP_SDP_PPS;
> + pps_sdp->pps_header.HB2 =
Re: [PATCH v2 10/23] drm/dsc: Add helpers for DSC picture parameter set infoframes
On Fri, Aug 03, 2018 at 08:43:51PM +0100, Chris Wilson wrote: > Quoting Manasi Navare (2018-08-03 20:18:42) > > On Tue, Jul 31, 2018 at 10:16:45PM +0100, Chris Wilson wrote: > > > Quoting Manasi Navare (2018-07-31 22:07:06) > > > > + /* PPS 4 */ > > > > + pps_sdp->pps_payload.pps_4 = (u8)((dsc_cfg->bits_per_pixel & > > > > + DSC_PPS_BPP_HIGH_MASK) >> > > > > + DSC_PPS_MSB_SHIFT) | > > > > > > To avoid overhanging cliffs, insert the newline after the sequence > > > point. Quite a few examples throughout the series that would benefit > > > from more judicial placement of line breaks. > > > > What exactly are you refering to by sequence point here? > > Which ever makes sense visually, here '='. > > pps_sdp->pps_payload.pps_4 = > dsc_cfg->bits_per_pixel >> X << DSC_PPS_MSB_SHIFT | > dsc_cfg->vbr_enable << DSC_PPS_VBR_EN_SHIFT | > dsc_cfg->enable422 << DSC_PPS_SIMPLE422_SHIFT | > dsc_cfg->convert_rgb << DSC_PPS_CONVERT_RGB_SHIFT | > dsc_cfg->block_pred_enable << DSC_PPS_BLOCK_PRED_EN_SHIFT; Ok understood about the sequence point > > If you want to mask with 0xff do it without having to refer to C type > promotion > rules. This point is still not clear. is this not correct: (dsc_cfg->bits_per_pixel & DSC_PPS_BPP_HIGH_MASK) >> DSC_PPS_MSB_SHIFT > > > > Furthermore, you only need the SPDX shorthand rather than full licence > > > text. > > > > Here the header is what all the other .c files in drm have. They all tend > > to use > > complete text. > > And they all need updating, eventually. There's no reason for new file > to use the old style. > > > Are you suggesting just adding SPDX-License-Identifier: GPL-2.0+ at the > > begining of the file? > > Should I remove the entire paragraph about the Copyright? > > No, the code is MIT. You replace the licence with just a SPDX link. Could you give an example here for the correct way of adding SPDX info in the header and what part of the existing header needs to stay? That would be helpful. Manasi > -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 10/23] drm/dsc: Add helpers for DSC picture parameter set infoframes
Quoting Manasi Navare (2018-08-03 20:18:42) > On Tue, Jul 31, 2018 at 10:16:45PM +0100, Chris Wilson wrote: > > Quoting Manasi Navare (2018-07-31 22:07:06) > > > + /* PPS 4 */ > > > + pps_sdp->pps_payload.pps_4 = (u8)((dsc_cfg->bits_per_pixel & > > > + DSC_PPS_BPP_HIGH_MASK) >> > > > + DSC_PPS_MSB_SHIFT) | > > > > To avoid overhanging cliffs, insert the newline after the sequence > > point. Quite a few examples throughout the series that would benefit > > from more judicial placement of line breaks. > > What exactly are you refering to by sequence point here? Which ever makes sense visually, here '='. pps_sdp->pps_payload.pps_4 = dsc_cfg->bits_per_pixel >> X << DSC_PPS_MSB_SHIFT | dsc_cfg->vbr_enable << DSC_PPS_VBR_EN_SHIFT | dsc_cfg->enable422 << DSC_PPS_SIMPLE422_SHIFT | dsc_cfg->convert_rgb << DSC_PPS_CONVERT_RGB_SHIFT | dsc_cfg->block_pred_enable << DSC_PPS_BLOCK_PRED_EN_SHIFT; If you want to mask with 0xff do it without having to refer to C type promotion rules. > > Furthermore, you only need the SPDX shorthand rather than full licence > > text. > > Here the header is what all the other .c files in drm have. They all tend to > use > complete text. And they all need updating, eventually. There's no reason for new file to use the old style. > Are you suggesting just adding SPDX-License-Identifier: GPL-2.0+ at the > begining of the file? > Should I remove the entire paragraph about the Copyright? No, the code is MIT. You replace the licence with just a SPDX link. -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 10/23] drm/dsc: Add helpers for DSC picture parameter set infoframes
On Tue, Jul 31, 2018 at 10:16:45PM +0100, Chris Wilson wrote: > Quoting Manasi Navare (2018-07-31 22:07:06) > > + /* PPS 4 */ > > + pps_sdp->pps_payload.pps_4 = (u8)((dsc_cfg->bits_per_pixel & > > + DSC_PPS_BPP_HIGH_MASK) >> > > + DSC_PPS_MSB_SHIFT) | > > To avoid overhanging cliffs, insert the newline after the sequence > point. Quite a few examples throughout the series that would benefit > from more judicial placement of line breaks. What exactly are you refering to by sequence point here? I am adding newline here after the operator if the next operand exceeds the 80 column limit. > > > + (u8)dsc_cfg->vbr_enable << DSC_PPS_VBR_EN_SHIFT | > > + (u8)dsc_cfg->enable422 << DSC_PPS_SIMPLE422_SHIFT | > > + (u8)dsc_cfg->convert_rgb << DSC_PPS_CONVERT_RGB_SHIFT | > > + (u8)dsc_cfg->block_pred_enable << > > DSC_PPS_BLOCK_PRED_EN_SHIFT; > > Furthermore, you only need the SPDX shorthand rather than full licence > text. Here the header is what all the other .c files in drm have. They all tend to use complete text. Are you suggesting just adding SPDX-License-Identifier: GPL-2.0+ at the begining of the file? Should I remove the entire paragraph about the Copyright? Manasi > -Chris > ___ > 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 v2 10/23] drm/dsc: Add helpers for DSC picture parameter set infoframes
Quoting Manasi Navare (2018-07-31 22:07:06) > + /* PPS 4 */ > + pps_sdp->pps_payload.pps_4 = (u8)((dsc_cfg->bits_per_pixel & > + DSC_PPS_BPP_HIGH_MASK) >> > + DSC_PPS_MSB_SHIFT) | To avoid overhanging cliffs, insert the newline after the sequence point. Quite a few examples throughout the series that would benefit from more judicial placement of line breaks. > + (u8)dsc_cfg->vbr_enable << DSC_PPS_VBR_EN_SHIFT | > + (u8)dsc_cfg->enable422 << DSC_PPS_SIMPLE422_SHIFT | > + (u8)dsc_cfg->convert_rgb << DSC_PPS_CONVERT_RGB_SHIFT | > + (u8)dsc_cfg->block_pred_enable << DSC_PPS_BLOCK_PRED_EN_SHIFT; Furthermore, you only need the SPDX shorthand rather than full licence text. -Chris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 10/23] drm/dsc: Add helpers for DSC picture parameter set infoframes
According to Display Stream compression spec 1.2, the picture
parameter set metadata is sent from source to sink device
using the DP Secondary data packet. An infoframe is formed
for the PPS SDP header and PPS SDP payload bytes.
This patch adds helpers to fill the PPS SDP header
and PPS SDP payload according to the DSC 1.2 specification.
v5:
Do not use bitfields for DRM structs (Jani N)
v4:
* Use DSC constants for params that dont change across
configurations
v3:
* Add reference to added kernel-docs in Documentation/gpu/drm-kms-helpers.rst
(Daniel Vetter)
v2:
* Add EXPORT_SYMBOL for the drm functions (Manasi)
Cc: dri-devel@lists.freedesktop.org
Cc: Jani Nikula
Cc: Ville Syrjala
Cc: Anusha Srivatsa
Cc: Harry Wentland
Signed-off-by: Manasi Navare
---
Documentation/gpu/drm-kms-helpers.rst | 12 ++
drivers/gpu/drm/Makefile | 2 +-
drivers/gpu/drm/drm_dsc.c | 221 ++
include/drm/drm_dsc.h | 22
4 files changed, 256 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/drm_dsc.c
diff --git a/Documentation/gpu/drm-kms-helpers.rst
b/Documentation/gpu/drm-kms-helpers.rst
index f9cfcdc..50bb717 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -223,6 +223,18 @@ MIPI DSI Helper Functions Reference
.. kernel-doc:: drivers/gpu/drm/drm_mipi_dsi.c
:export:
+Display Stream Compression Helper Functions Reference
+=
+
+.. kernel-doc:: drivers/gpu/drm/drm_dsc.c
+ :doc: dsc helpers
+
+.. kernel-doc:: include/drm/drm_dsc.h
+ :internal:
+
+.. kernel-doc:: drivers/gpu/drm/drm_dsc.c
+ :export:
+
Output Probing Helper Functions Reference
=
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index a6771ce..961e511 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -31,7 +31,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
-drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
+drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o
drm_probe_helper.o \
drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
drm_simple_kms_helper.o drm_modeset_helper.o \
diff --git a/drivers/gpu/drm/drm_dsc.c b/drivers/gpu/drm/drm_dsc.c
new file mode 100644
index 000..99a8794
--- /dev/null
+++ b/drivers/gpu/drm/drm_dsc.c
@@ -0,0 +1,221 @@
+/*
+ *Copyright © 2018 Intel Corp
+ *
+ * Permission to use, copy, modify, distribute, and sell this software and its
+ * documentation for any purpose is hereby granted without fee, provided that
+ * the above copyright notice appear in all copies and that both that copyright
+ * notice and this permission notice appear in supporting documentation, and
+ * that the name of the copyright holders not be used in advertising or
+ * publicity pertaining to distribution of the software without specific,
+ * written prior permission. The copyright holders make no representations
+ * about the suitability of this software for any purpose. It is provided "as
+ * is" without express or implied warranty.
+ *
+ * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
+ * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
+ * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+ * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
+ * OF THIS SOFTWARE.
+ *
+ * Author:
+ * Manasi Navare
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+/**
+ * DOC: dsc helpers
+ *
+ * These functions contain some common logic and helpers to deal with VESA
+ * Display Stream Compression standard required for DSC on Display Port/eDP or
+ * MIPI display interfaces.
+ */
+
+/**
+ * drm_dsc_dp_pps_header_init() - Initializes the PPS Header
+ * for DisplayPort as per the DP 1.4 spec.
+ * @pps_sdp: Secondary data packet for DSC Picture Parameter Set
+ */
+void drm_dsc_dp_pps_header_init(struct drm_dsc_pps_infoframe *pps_sdp)
+{
+ memset(_sdp->pps_header, 0, sizeof(pps_sdp->pps_header));
+
+ pps_sdp->pps_header.HB1 = DP_SDP_PPS;
+ pps_sdp->pps_header.HB2 = DP_SDP_PPS_HEADER_PAYLOAD_BYTES_MINUS_1;
+}
+EXPORT_SYMBOL(drm_dsc_dp_pps_header_init);
+
+/**
+ * drm_dsc_pps_infoframe_pack() - Populates the DSC PPS infoframe
+ * using the DSC configuration parameters in the order expected
+ * by the DSC Display Sink device. For the DSC, the sink device
+ * expects the PPS payload in the big endian format for the fields
+ * that span more
