On Tue, 20 Feb 2024 at 19:55, Paloma Arellano <quic_parel...@quicinc.com> wrote: > > > On 2/17/2024 12:56 AM, Dmitry Baryshkov wrote: > > On Sat, 17 Feb 2024 at 01:03, Paloma Arellano <quic_parel...@quicinc.com> > > wrote:
> >> + } > >> + > >> + panel = container_of(dp_panel, struct dp_panel_private, dp_panel); > >> + catalog = panel->catalog; > >> + dp_mode = &dp_panel->dp_mode; > >> + > >> + memset(&vsc_sdp_data, 0, sizeof(vsc_sdp_data)); > >> + > >> + /* VSC SDP header as per table 2-118 of DP 1.4 specification */ > >> + vsc_sdp_data.sdp_type = DP_SDP_VSC; > >> + vsc_sdp_data.revision = 0x05; > >> + vsc_sdp_data.length = 0x13; > >> + > >> + /* VSC SDP Payload for DB16 */ > >> + vsc_sdp_data.pixelformat = DP_PIXELFORMAT_YUV420; > >> + vsc_sdp_data.colorimetry = DP_COLORIMETRY_DEFAULT; > >> + > >> + /* VSC SDP Payload for DB17 */ > >> + vsc_sdp_data.bpc = dp_mode->bpp / 3; > >> + vsc_sdp_data.dynamic_range = DP_DYNAMIC_RANGE_CTA; > >> + > >> + /* VSC SDP Payload for DB18 */ > >> + vsc_sdp_data.content_type = DP_CONTENT_TYPE_GRAPHICS; > >> + > >> + len = dp_utils_pack_vsc_sdp(&vsc_sdp_data, &vsc_sdp, header); > >> + if (len < 0) { > >> + DRM_ERROR("unable to pack vsc sdp\n"); > >> + return len; > >> + } > > So, at this point we have the header data both in vsc_sdp.sdp_header > > and in the packed header. The relationship between them becomes less > > obvious. Could you please pack dp_sdp_header into u32[2] just before > > writing it? It really makes little sense to pass both at the same > > time. > > > Just want to clear some stuff up. Do you want to call the > dp_utils_pack_sdp_header() function right before > dp_catalog_panel_send_vsc_sdp()? The point of putting the > dp_utils_pack_sdp_header() function inside dp_utils_pack_vsc_sdp() is so > that all of the packing could be in the same location. Although I agree > that we are passing the same values twice, I believe that having it the > way it is currently is better since it shows that the > drm_dp_vsc_sdp_pack() and dp_utils_pack_sdp_header() are related since > they're packing the data to the set of GENERIC0 registers. I'm perfectly fine with dp_utils_pack_sdp_header() being called from within dp_catalog_panel_send_vsc_sdp(). This way you are not passing extra data and it is perfectly clear how the SDP header is handled before being written to the hardware. -- With best wishes Dmitry