On 2/10/2024 10:57 PM, Dmitry Baryshkov wrote:
On Sun, 11 Feb 2024 at 06:06, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 2/10/2024 1:46 PM, Dmitry Baryshkov wrote:
On Sat, 10 Feb 2024 at 20:50, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 2/10/2024 10:14 AM, Abhinav Kumar wrote:
On 2/10/2024 2:09 AM, Dmitry Baryshkov wrote:
On Sat, 10 Feb 2024 at 03:52, Paloma Arellano
<quic_parellan@xxxxxxxxxxx> wrote:
Add support to pack and send the VSC SDP packet for DP. This therefore
allows the transmision of format information to the sinks which is
needed for YUV420 support over DP.
Changes in v2:
- Rename GENERIC0_SDPSIZE macro to GENERIC0_SDPSIZE_VALID
- Remove dp_sdp from the dp_catalog struct since this data is
being allocated at the point used
- Create a new function in dp_utils to pack the VSC SDP data
into a buffer
- Create a new function that packs the SDP header bytes into a
buffer. This function is made generic so that it can be
utilized by dp_audio
header bytes into a buffer
- Create a new function in dp_utils that takes the packed
buffer
and writes to the DP_GENERIC0_* registers
- Split the dp_catalog_panel_config_vsc_sdp() function into two
to disable/enable sending VSC SDP packets
- Check the DP HW version using the original useage of
dp_catalog_hw_revision() and correct the version checking
logic
- Rename dp_panel_setup_vsc_sdp() to
dp_panel_setup_vsc_sdp_yuv_420() to explicitly state that
currently VSC SDP is only being set up to support YUV420
modes
Signed-off-by: Paloma Arellano <quic_parellan@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/dp/dp_catalog.c | 105 ++++++++++++++++++++++++++++
drivers/gpu/drm/msm/dp/dp_catalog.h | 6 ++
drivers/gpu/drm/msm/dp/dp_ctrl.c | 4 ++
drivers/gpu/drm/msm/dp/dp_panel.c | 59 ++++++++++++++++
drivers/gpu/drm/msm/dp/dp_reg.h | 3 +
drivers/gpu/drm/msm/dp/dp_utils.c | 80 +++++++++++++++++++++
drivers/gpu/drm/msm/dp/dp_utils.h | 3 +
7 files changed, 260 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 5d84c089e520a..0f28a4897b7b7 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -901,6 +901,111 @@ int dp_catalog_panel_timing_cfg(struct
dp_catalog *dp_catalog)
return 0;
}
<Snip>
+static int dp_panel_setup_vsc_sdp_yuv_420(struct dp_panel *dp_panel)
+{
+ struct dp_catalog *catalog;
+ struct dp_panel_private *panel;
+ struct dp_display_mode *dp_mode;
+ struct dp_sdp_header sdp_header;
+ struct drm_dp_vsc_sdp vsc_sdp_data;
+ size_t buff_size;
+ u32 gen_buffer[10];
+ int rc = 0;
+
+ if (!dp_panel) {
+ DRM_ERROR("invalid input\n");
+ rc = -EINVAL;
+ return rc;
+ }
+
+ panel = container_of(dp_panel, struct dp_panel_private,
dp_panel);
+ catalog = panel->catalog;
+ dp_mode = &dp_panel->dp_mode;
+ buff_size = sizeof(gen_buffer);
+
+ memset(&sdp_header, 0, sizeof(sdp_header));
+ memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data));
+
+ /* VSC SDP header as per table 2-118 of DP 1.4 specification */
+ sdp_header.HB0 = 0x00;
+ sdp_header.HB1 = 0x07;
+ sdp_header.HB2 = 0x05;
+ sdp_header.HB3 = 0x13;
This should go to the packing function.
We can .... but ....
the header bytes can change based on the format. These values are
specific to YUV420. Thats why this part was kept outside of the generic
vsc sdp packing. Today we support only YUV420 VSC SDP so we can move it
but this was the reason.
These values can be set from the sdp_type, revision and length fields
of struct drm_dp_vsc_sdp.
The function intel_dp_vsc_sdp_pack() is pretty much close to what I had in mind.
+
+ /* VSC SDP Payload for DB16 */
Comments are useless more or less. The code fills generic information
structure which might or might not be packed to the data buffer.
+ vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420;
+ vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT;
+
+ /* VSC SDP Payload for DB17 */
+ vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA;
+
+ /* VSC SDP Payload for DB18 */
+ vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS;
+
+ vsc_sdp_data.bpc = dp_mode->bpp / 3;
Consider extracting intel_dp_compute_vsc_colorimetry() and using it.
intel_dp_compute_vsc_colorimetry() uses colorspace property to pick
YUV420, we do not.
Intel function also uses output_format, but it's true, it is full of
Intel specifics.
Right now, its a pure driver decision to use YUV420
when the mode is supported only in that format.
Also, many params are to be used within this function cached inside
intel_crtc_state . We will first need to make that API more generic to
be re-usable by others.
I think overall, if we want to have a generic packing across vendors, it
needs more work. I think one of us can take that up as a separate effort.
Ack, I agree here. I did only a quick glance over
intel_dp_compute_vsc_colorimetry function() beforehand.
+
+ rc = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &sdp_header,
gen_buffer, buff_size);
+ if (rc) {
+ DRM_ERROR("unable to pack vsc sdp\n");
+ return rc;
+ }
+
+ dp_catalog_panel_enable_vsc_sdp(catalog, gen_buffer);
+
+ return rc;
+}
+
void dp_panel_dump_regs(struct dp_panel *dp_panel)
{
struct dp_catalog *catalog;
@@ -344,6 +399,10 @@ int dp_panel_timing_cfg(struct dp_panel *dp_panel)
catalog->dp_active = data;
dp_catalog_panel_timing_cfg(catalog);
+
+ if (dp_panel->dp_mode.out_fmt_is_yuv_420)
+ dp_panel_setup_vsc_sdp_yuv_420(dp_panel);
+
panel->panel_on = true;
return 0;
diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h
b/drivers/gpu/drm/msm/dp/dp_reg.h
index ea85a691e72b5..2983756c125cd 100644
--- a/drivers/gpu/drm/msm/dp/dp_reg.h
+++ b/drivers/gpu/drm/msm/dp/dp_reg.h
@@ -142,6 +142,7 @@
#define DP_MISC0_SYNCHRONOUS_CLK (0x00000001)
#define DP_MISC0_COLORIMETRY_CFG_SHIFT (0x00000001)
#define DP_MISC0_TEST_BITS_DEPTH_SHIFT (0x00000005)
+#define DP_MISC1_VSC_SDP (0x00004000)
#define REG_DP_VALID_BOUNDARY (0x00000030)
#define REG_DP_VALID_BOUNDARY_2 (0x00000034)
@@ -201,9 +202,11 @@
#define MMSS_DP_AUDIO_CTRL_RESET (0x00000214)
#define MMSS_DP_SDP_CFG (0x00000228)
+#define GEN0_SDP_EN (0x00020000)
#define MMSS_DP_SDP_CFG2 (0x0000022C)
#define MMSS_DP_AUDIO_TIMESTAMP_0 (0x00000230)
#define MMSS_DP_AUDIO_TIMESTAMP_1 (0x00000234)
+#define GENERIC0_SDPSIZE_VALID (0x00010000)
#define MMSS_DP_AUDIO_STREAM_0 (0x00000240)
#define MMSS_DP_AUDIO_STREAM_1 (0x00000244)
diff --git a/drivers/gpu/drm/msm/dp/dp_utils.c
b/drivers/gpu/drm/msm/dp/dp_utils.c
index 176d839906cec..05e0133eb50f3 100644
--- a/drivers/gpu/drm/msm/dp/dp_utils.c
+++ b/drivers/gpu/drm/msm/dp/dp_utils.c
@@ -4,6 +4,16 @@
*/
#include <linux/types.h>
+#include <drm/display/drm_dp_helper.h>
+#include <drm/drm_print.h>
+
+#include "dp_utils.h"
+
+#define DP_GENERIC0_6_YUV_8_BPC BIT(0)
+#define DP_GENERIC0_6_YUV_10_BPC BIT(1)
+
+#define DP_SDP_HEADER_SIZE 8
+#define DP_VSC_SDP_BUFFER_SIZE 40
u8 dp_utils_get_g0_value(u8 data)
{
@@ -69,3 +79,73 @@ u8 dp_utils_calculate_parity(u32 data)
return parity_byte;
}
+
+int dp_utils_pack_sdp_header(struct dp_sdp_header *sdp_header, u32
*buffer, size_t buff_size)
+{
+ u32 header, parity;
+
+ if (buff_size < DP_SDP_HEADER_SIZE)
+ return -ENOSPC;
+
+ memset(buffer, 0, sizeof(buffer));
+
+ /* HEADER BYTE 0 */
+ header = sdp_header->HB0;
+ parity = dp_utils_calculate_parity(header);
+ buffer[0] = ((header << HEADER_BYTE_0_BIT) | (parity <<
PARITY_BYTE_0_BIT));
+
+ /* HEADER BYTE 1 */
+ header = sdp_header->HB1;
+ parity = dp_utils_calculate_parity(header);
+ buffer[0] |= ((header << HEADER_BYTE_1_BIT) | (parity <<
PARITY_BYTE_1_BIT));
+
+ /* HEADER BYTE 2 */
+ header = sdp_header->HB2;
+ parity = dp_utils_calculate_parity(header);
+ buffer[1] = ((header << HEADER_BYTE_2_BIT) | (parity <<
PARITY_BYTE_2_BIT));
+
+ /* HEADER BYTE 3 */
+ header = sdp_header->HB3;
+ parity = dp_utils_calculate_parity(header);
+ buffer[1] |= ((header << HEADER_BYTE_3_BIT) | (parity <<
PARITY_BYTE_3_BIT));
+
+ return 0;
+}
+
+int dp_utils_pack_vsc_sdp(struct drm_dp_vsc_sdp *vsc_sdp_data,
struct dp_sdp_header *sdp_header,
+ u32 *buffer, size_t buff_size)
+{
No. This function should pack data into struct dp_sdp and it should go
to drivers/video/hdmi.c
What is the difference between struct drm_dp_vsc_sdp and struct dp_sdp?
I think you were referring to the fact that instead of a custom buffer,
we should use struct dp_sdp to pack elements from struct drm_dp_vsc_sdp.
If yes, I agree with this part.
Yes.
It seems like struct drm_dp_vsc_sdp is more appropriate for this.
Regarding hdmi.c, I think the packing needs to be more generic to move
it there.
I am not seeing parity bytes packing with other vendors. So there will
have to be some packing there and then some packing here.
Yes. Writing the HB + PB seems specific to Qualcomm hardware. At least
Intel and AMD seem to write header bytes without parity.
Also, like you already noticed, to make the VSC SDP packing more generic
to move to hdmi.c, it needs more work to make it more usable like
supporting all pixel formats and all colorimetry values.
We do not have that much utility yet for that.
I think you are mixing the filling of drm_dp_vsc_sdp and packing of
that struct. I suppose intel_dp_vsc_sdp_pack() can be extracted more
or less as is.
Once somebody needs to support 3D (AMD does), they can extend the function.
Yes once I corrected my understanding of using the function to just pack
struct dp_sdp instead of the buffer, I understood the intention.
We can try it. I have written up a RFC to move the
intel_dp_vsc_sdp_pack() to drm_dp_helper as that seems more appropriate
now for it and not hdmi.c as we already have some vsc sdp utils in
drm_dp_helper.
Yes, we have. However all structure manipulation functions, even for
DP, are usually a part of the hdmi.c. If I understand correctly, we
can still touch this file from drm-misc (or from other drm trees).
hmm ... I think the only reason we have
hdmi_audio_infoframe_pack_for_dp() is because of re-using
hdmi_audio_infoframe to pack into a struct dp_sdp.
We will not be doing that here. It will be from a drm_dp_vsc_sdp to
dp_sdp. Thats why I thought drm_dp_helper is more appropriate.
But before I post, we will do some cleanup and see if things look
reasonable in this patch too because we will still need to write a
common utility to pack the parity bytes for the sdp header which we need
to utilize for audio and vsc sdp in our DP case.
I am thinking of a
struct msm_dp_sdp_pb {
u8 PB0;
u8 PB1;
u8 PB2;
u8 PB3;
} __packed;
struct msm_dp_sdp_with_parity {
struct dp_sdp vsc_sdp;
struct msm_dp_sdp_pb pb;
};
intel_dp_vsc_sdp_pack() will pack the struct dp_sdp but we will have to
do some additional parity byte packing after that. That part will remain
common for audio and vsc for msm.
I think you can do it in a much easier way:
struct dp_sdp sdp
u32 header[2];
hdmi_dp_vsc_sdp_pack(vsc_sdp, &sdp);
msm_dp_pack_header(&sdp, header);
dp_write_register(catalog, MMSS_DP_FOO_HEADER0, header[0]);
dp_write_register(catalog, MMSS_DP_FOO_HEADER1, header[1]);
// write sdp.db
Sure. Just a difference of packing it together Vs a local header. We
will see which one looks better and post it. I felt that my way makes it
clear that msm_dp needs extra parity specific to msm.
