Re: [PATCH v2] docs/gpu: ci: update flake tests requirements
On 9/26/2024 12:06 AM, Vignesh Raman wrote:
Update the documentation to require linking to a relevant GitLab
issue for each new flake entry instead of an email report. Added
specific GitLab issue URLs for i915, xe and other drivers.
Signed-off-by: Vignesh Raman
---
v2:
- Add gitlab issue link for msm driver.
---
Documentation/gpu/automated_testing.rst | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/Documentation/gpu/automated_testing.rst
b/Documentation/gpu/automated_testing.rst
index 2d5a28866afe..f918fe56f2b0 100644
--- a/Documentation/gpu/automated_testing.rst
+++ b/Documentation/gpu/automated_testing.rst
@@ -67,20 +67,26 @@ Lists the tests that for a given driver on a specific
hardware revision are
known to behave unreliably. These tests won't cause a job to fail regardless
of
the result. They will still be run.
-Each new flake entry must be associated with a link to the email reporting the
-bug to the author of the affected driver, the board name or Device Tree name of
-the board, the first kernel version affected, the IGT version used for tests,
-and an approximation of the failure rate.
+Each new flake entry must include a link to the relevant GitLab issue, the
board
+name or Device Tree name, the first kernel version affected, the IGT version
used
+for tests and an approximation of the failure rate.
They should be provided under the following format::
- # Bug Report: $LORE_OR_PATCHWORK_URL
+ # Bug Report: $GITLAB_ISSUE
# Board Name: broken-board.dtb
# Linux Version: 6.6-rc1
# IGT Version: 1.28-gd2af13d9f
# Failure Rate: 100
flaky-test
+The GitLab issue must include the logs and the pipeline link. Use the appropriate
+link below to create an issue.
+https://gitlab.freedesktop.org/drm/i915/kernel/-/issues for i915 driver
+https://gitlab.freedesktop.org/drm/xe/kernel/-/issues for xe driver
+https://gitlab.freedesktop.org/drm/msm/-/issues for msm driver
+https://gitlab.freedesktop.org/drm/misc/kernel/-/issues for other drivers
+
Acked for MSM.
Thanks
Abhinav
drivers/gpu/drm/ci/${DRIVER_NAME}-${HW_REVISION}-skips.txt
---
Re: [PATCH v3 4/6] drm/ci: uprev IGT
On 5/29/2024 2:48 AM, Vignesh Raman wrote:
Hi Dmitry,
On 29/05/24 13:39, Dmitry Baryshkov wrote:
On Wed, May 29, 2024 at 08:10:47AM +0530, Vignesh Raman wrote:
test-list.txt and test-list-full.txt are not generated for
cross-builds and they are required by drm-ci for testing
arm32 targets. This is fixed in igt-gpu-tools. So uprev
IGT to include the commit which fixes this issue. Also
disable building xe driver tests for non-intel platforms.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Vignesh Raman
---
v2:
- Split IGT uprev to seperate patch.
v3:
- No changes.
---
drivers/gpu/drm/ci/build-igt.sh | 4
drivers/gpu/drm/ci/gitlab-ci.yml | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ci/build-igt.sh
b/drivers/gpu/drm/ci/build-igt.sh
index b7d2a49a6db3..eddb5f782a5e 100644
--- a/drivers/gpu/drm/ci/build-igt.sh
+++ b/drivers/gpu/drm/ci/build-igt.sh
@@ -45,6 +45,10 @@
MESON_OPTIONS="-Doverlay=disabled \
-Dlibunwind=enabled \
-Dprefix=/igt"
+if [[ "$KERNEL_ARCH" = "arm64" ]] || [[ "$KERNEL_ARCH" = "arm" ]]; then
+ MESON_OPTIONS="$MESON_OPTIONS -Dxe_driver=disabled"
+fi
+
mkdir -p /igt
meson build $MESON_OPTIONS $EXTRA_MESON_ARGS
ninja -C build -j${FDO_CI_CONCURRENT:-4} || ninja -C build -j 1
diff --git a/drivers/gpu/drm/ci/gitlab-ci.yml
b/drivers/gpu/drm/ci/gitlab-ci.yml
index 8f32de63d92e..1b29c3b6406b 100644
--- a/drivers/gpu/drm/ci/gitlab-ci.yml
+++ b/drivers/gpu/drm/ci/gitlab-ci.yml
@@ -5,7 +5,7 @@ variables:
UPSTREAM_REPO: git://anongit.freedesktop.org/drm/drm
TARGET_BRANCH: drm-next
- IGT_VERSION: d2af13d9f5be5ce23d996e4afd3e45990f5ab977
+ IGT_VERSION: 0df7b9b97f9da0e364f5ee30fe331004b8c86b56
Let's land this, then I'll ask to uprev to
dc2d7fb4f978048b87707ea9ec32da748b01b378, which fixes an issue with the
writeback tests on MSM devices.
Sure. Once this is merged, we can uprev to the latest IGT.
Regards,
Vignesh
Thanks, yes moving to latest IGT after this is merged will be great.
DEQP_RUNNER_GIT_URL:
https://gitlab.freedesktop.org/anholt/deqp-runner.git
DEQP_RUNNER_GIT_TAG: v0.15.0
--
2.40.1
Re: i915 build error on drm-misc-next
On 2/23/2024 11:35 AM, Rodrigo Vivi wrote:
On Fri, Feb 23, 2024 at 09:47:11AM -0800, Abhinav Kumar wrote:
CC Dmitry
Hi Rodrigo
On 2/23/2024 9:00 AM, Rodrigo Vivi wrote:
On Fri, Feb 23, 2024 at 08:50:06AM -0700, Jeffrey Hugo wrote:
With the x86_64_defconfig I see the following when building drm-misc-next:
CC drivers/gpu/drm/i915/display/intel_crt.o
CC drivers/gpu/drm/i915/display/intel_cx0_phy.o
CC drivers/gpu/drm/i915/display/intel_ddi.o
CC drivers/gpu/drm/i915/display/intel_ddi_buf_trans.o
CC drivers/gpu/drm/i915/display/intel_display_device.o
CC drivers/gpu/drm/i915/display/intel_display_trace.o
CC drivers/gpu/drm/i915/display/intel_dkl_phy.o
CC drivers/gpu/drm/i915/display/intel_dp.o
CC drivers/gpu/drm/i915/display/intel_dp_aux.o
CC drivers/gpu/drm/i915/display/intel_dp_aux_backlight.o
CC drivers/gpu/drm/i915/display/intel_dp_hdcp.o
CC drivers/gpu/drm/i915/display/intel_dp_link_training.o
CC drivers/gpu/drm/i915/display/intel_dp_mst.o
CC drivers/gpu/drm/i915/display/intel_dsi.o
CC drivers/gpu/drm/i915/display/intel_dsi_dcs_backlight.o
CC drivers/gpu/drm/i915/display/intel_dsi_vbt.o
CC drivers/gpu/drm/i915/display/intel_dvo.o
CC drivers/gpu/drm/i915/display/intel_gmbus.o
CC drivers/gpu/drm/i915/display/intel_hdmi.o
CC drivers/gpu/drm/i915/display/intel_lspcon.o
CC drivers/gpu/drm/i915/display/intel_lvds.o
CC drivers/gpu/drm/i915/display/intel_panel.o
CC drivers/gpu/drm/i915/display/intel_pps.o
drivers/gpu/drm/i915/display/intel_dp.c: In function
‘intel_write_dp_vsc_sdp’:
drivers/gpu/drm/i915/display/intel_dp.c:4232:15: error: implicit declaration
of function ‘intel_dp_vsc_sdp_pack’; did you mean ‘drm_dp_vsc_sdp_pack’?
[-Werror=implicit-function-declaration]
4232 | len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
| ^
| drm_dp_vsc_sdp_pack
Is this a known issue?
o.O - what a mistery!
it looks that drm-misc-next has only part of the patch:
31a5b6ed88c7 ("drm/i915/display: Unify VSC SPD preparation")
without the patch itself...
I couldn't even trace back to understand how the declaration is
gone from the drm-misc-next...
Looks like the issue here is that the below patch which landed in
drm-misc-next
https://patchwork.freedesktop.org/patch/579128/?series=130145&rev=1
was based on top of drm-tip because the intel CI runs on drm-tip and not
drm-misc-next.
But, https://patchwork.freedesktop.org/patch/572622/ is not present in
drm-misc-next.
Hence this broke the compilation.
How would you prefer to fix this? We revert
https://patchwork.freedesktop.org/series/130145/ from drm-misc and land it
through i915 tree and can you provide us a tag from the i915 tree to rebase
our msm-next tree on?
The revert from drm-misc is a possibility, then you squash
https://lore.kernel.org/all/20240223191548.392185-1-rodrigo.v...@intel.com/
in and merge it again.
or if drm-misc and drm maintainers are okay we can simply add
https://lore.kernel.org/all/20240223191548.392185-1-rodrigo.v...@intel.com/
on top of drm-misc-next
I am totally fine with this second option. Have given my R-b.
and on any conflict later the resolution is simply deleting this line
anyway.
-Jeff
Re: i915 build error on drm-misc-next
CC Dmitry
Hi Rodrigo
On 2/23/2024 9:00 AM, Rodrigo Vivi wrote:
On Fri, Feb 23, 2024 at 08:50:06AM -0700, Jeffrey Hugo wrote:
With the x86_64_defconfig I see the following when building drm-misc-next:
CC drivers/gpu/drm/i915/display/intel_crt.o
CC drivers/gpu/drm/i915/display/intel_cx0_phy.o
CC drivers/gpu/drm/i915/display/intel_ddi.o
CC drivers/gpu/drm/i915/display/intel_ddi_buf_trans.o
CC drivers/gpu/drm/i915/display/intel_display_device.o
CC drivers/gpu/drm/i915/display/intel_display_trace.o
CC drivers/gpu/drm/i915/display/intel_dkl_phy.o
CC drivers/gpu/drm/i915/display/intel_dp.o
CC drivers/gpu/drm/i915/display/intel_dp_aux.o
CC drivers/gpu/drm/i915/display/intel_dp_aux_backlight.o
CC drivers/gpu/drm/i915/display/intel_dp_hdcp.o
CC drivers/gpu/drm/i915/display/intel_dp_link_training.o
CC drivers/gpu/drm/i915/display/intel_dp_mst.o
CC drivers/gpu/drm/i915/display/intel_dsi.o
CC drivers/gpu/drm/i915/display/intel_dsi_dcs_backlight.o
CC drivers/gpu/drm/i915/display/intel_dsi_vbt.o
CC drivers/gpu/drm/i915/display/intel_dvo.o
CC drivers/gpu/drm/i915/display/intel_gmbus.o
CC drivers/gpu/drm/i915/display/intel_hdmi.o
CC drivers/gpu/drm/i915/display/intel_lspcon.o
CC drivers/gpu/drm/i915/display/intel_lvds.o
CC drivers/gpu/drm/i915/display/intel_panel.o
CC drivers/gpu/drm/i915/display/intel_pps.o
drivers/gpu/drm/i915/display/intel_dp.c: In function
‘intel_write_dp_vsc_sdp’:
drivers/gpu/drm/i915/display/intel_dp.c:4232:15: error: implicit declaration
of function ‘intel_dp_vsc_sdp_pack’; did you mean ‘drm_dp_vsc_sdp_pack’?
[-Werror=implicit-function-declaration]
4232 | len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
| ^
| drm_dp_vsc_sdp_pack
Is this a known issue?
o.O - what a mistery!
it looks that drm-misc-next has only part of the patch:
31a5b6ed88c7 ("drm/i915/display: Unify VSC SPD preparation")
without the patch itself...
I couldn't even trace back to understand how the declaration is
gone from the drm-misc-next...
Looks like the issue here is that the below patch which landed in
drm-misc-next
https://patchwork.freedesktop.org/patch/579128/?series=130145&rev=1
was based on top of drm-tip because the intel CI runs on drm-tip and not
drm-misc-next.
But, https://patchwork.freedesktop.org/patch/572622/ is not present in
drm-misc-next.
Hence this broke the compilation.
How would you prefer to fix this? We revert
https://patchwork.freedesktop.org/series/130145/ from drm-misc and land
it through i915 tree and can you provide us a tag from the i915 tree to
rebase our msm-next tree on?
-Jeff
[PATCH v3 1/2] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
Lets move this to drm_dp_helper to achieve this.
changes in v2:
- rebased on top of drm-tip
Acked-by: Dmitry Baryshkov
Signed-off-by: Abhinav Kumar
Acked-by: Jani Nikula
---
drivers/gpu/drm/display/drm_dp_helper.c | 78 +
drivers/gpu/drm/i915/display/intel_dp.c | 71 +-
include/drm/display/drm_dp_helper.h | 3 +
3 files changed, 83 insertions(+), 69 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
b/drivers/gpu/drm/display/drm_dp_helper.c
index 8d6ce46471ae..6c91f400ecb1 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2913,6 +2913,84 @@ void drm_dp_vsc_sdp_log(struct drm_printer *p, const
struct drm_dp_vsc_sdp *vsc)
}
EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
+/**
+ * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
+ * @vsc: vsc sdp initialized according to its purpose as defined in
+ * table 2-118 - table 2-120 in DP 1.4a specification
+ * @sdp: valid handle to the generic dp_sdp which will be packed
+ * @size: valid size of the passed sdp handle
+ *
+ * Returns length of sdp on success and error code on failure
+ */
+ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
+ struct dp_sdp *sdp, size_t size)
+{
+ size_t length = sizeof(struct dp_sdp);
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(sdp, 0, size);
+
+ /*
+* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
+* VSC SDP Header Bytes
+*/
+ sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
+ sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
+ sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
+ sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
+
+ if (vsc->revision == 0x6) {
+ sdp->db[0] = 1;
+ sdp->db[3] = 1;
+ }
+
+ /*
+* Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
+* Format as per DP 1.4a spec and DP 2.0 respectively.
+*/
+ if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
+ goto out;
+
+ /* VSC SDP Payload for DB16 through DB18 */
+ /* Pixel Encoding and Colorimetry Formats */
+ sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
+ sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
+
+ switch (vsc->bpc) {
+ case 6:
+ /* 6bpc: 0x0 */
+ break;
+ case 8:
+ sdp->db[17] = 0x1; /* DB17[3:0] */
+ break;
+ case 10:
+ sdp->db[17] = 0x2;
+ break;
+ case 12:
+ sdp->db[17] = 0x3;
+ break;
+ case 16:
+ sdp->db[17] = 0x4;
+ break;
+ default:
+ WARN(1, "Missing case %d\n", vsc->bpc);
+ return -EINVAL;
+ }
+
+ /* Dynamic Range and Component Bit Depth */
+ if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
+ sdp->db[17] |= 0x80; /* DB17[7] */
+
+ /* Content Type */
+ sdp->db[18] = vsc->content_type & 0x7;
+
+out:
+ return length;
+}
+EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
+
/**
* drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
* @dpcd: DisplayPort configuration data
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 217196196e50..a9458df475e2 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4089,73 +4089,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state
*crtc_state,
return false;
}
-static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
-struct dp_sdp *sdp, size_t size)
-{
- size_t length = sizeof(struct dp_sdp);
-
- if (size < length)
- return -ENOSPC;
-
- memset(sdp, 0, size);
-
- /*
-* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
-* VSC SDP Header Bytes
-*/
- sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
- sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
- sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
- sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
-
- if (vsc->revision == 0x6) {
- sdp->db[0] = 1;
- sdp->db[3] = 1;
- }
-
- /*
-* Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
-* Format as per DP 1.4a spec and DP 2.0 respectively.
-*/
- if (!(vsc->revisi
[PATCH v3 2/2] drm/dp: drop the size parameter from drm_dp_vsc_sdp_pack()
Currently the size parameter of drm_dp_vsc_sdp_pack() is always
the size of struct dp_sdp. Hence lets drop this parameter and
use sizeof() directly.
Suggested-by: Dmitry Baryshkov
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/display/drm_dp_helper.c | 8 ++--
drivers/gpu/drm/i915/display/intel_dp.c | 3 +--
include/drm/display/drm_dp_helper.h | 3 +--
3 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
b/drivers/gpu/drm/display/drm_dp_helper.c
index 6c91f400ecb1..10ee82e34de7 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2918,19 +2918,15 @@ EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
* @vsc: vsc sdp initialized according to its purpose as defined in
* table 2-118 - table 2-120 in DP 1.4a specification
* @sdp: valid handle to the generic dp_sdp which will be packed
- * @size: valid size of the passed sdp handle
*
* Returns length of sdp on success and error code on failure
*/
ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
- struct dp_sdp *sdp, size_t size)
+ struct dp_sdp *sdp)
{
size_t length = sizeof(struct dp_sdp);
- if (size < length)
- return -ENOSPC;
-
- memset(sdp, 0, size);
+ memset(sdp, 0, sizeof(struct dp_sdp));
/*
* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index a9458df475e2..e13121dc3a03 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4181,8 +4181,7 @@ static void intel_write_dp_sdp(struct intel_encoder
*encoder,
switch (type) {
case DP_SDP_VSC:
- len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp,
- sizeof(sdp));
+ len = drm_dp_vsc_sdp_pack(&crtc_state->infoframes.vsc, &sdp);
break;
case HDMI_PACKET_TYPE_GAMUT_METADATA:
len = intel_dp_hdr_metadata_infoframe_sdp_pack(dev_priv,
diff --git a/include/drm/display/drm_dp_helper.h
b/include/drm/display/drm_dp_helper.h
index 8474504d4c88..1f41994796d3 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -812,7 +812,6 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
int bpp_x16, unsigned long flags);
int drm_dp_bw_channel_coding_efficiency(bool is_uhbr);
-ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
- struct dp_sdp *sdp, size_t size);
+ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, struct dp_sdp
*sdp);
#endif /* _DRM_DP_HELPER_H_ */
--
2.34.1
Re: [PATCH v2] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
On 2/20/2024 11:41 AM, Ville Syrjälä wrote: On Tue, Feb 20, 2024 at 11:27:18AM -0800, Abhinav Kumar wrote: On 2/20/2024 11:20 AM, Dmitry Baryshkov wrote: On Tue, 20 Feb 2024 at 21:05, Dmitry Baryshkov wrote: On Tue, 20 Feb 2024 at 20:53, Abhinav Kumar wrote: On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote: On Thu, 15 Feb 2024 at 21:08, Abhinav Kumar wrote: intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well. Lets move this to drm_dp_helper to achieve this. changes in v2: - rebased on top of drm-tip Acked-by: Dmitry Baryshkov v1 had an explicit comment before the ack: Yes, I remember the comment. I did not make any changes to v2 other than just rebasing it on drm-tip to get the ack from i915 folks. From my side, with the promise of the size fixup. However I observe neither a second patch removing the size argument nor it being dropped as a part of this patch. Yes, now that in v2 we got the ack for this patch, I can spin a v3 with the addition of the next patch to remove the size OR as another series so as to not block the main series which needs this patch. I would prefer the latter. It doesn't work this way. The comment should have been fixed for v2. This probably deserves some explanation. Currently there is only one user of this function. So it is easy to fix it. Once there are several users, you have to fix all of them at the same time, patching different drm subtrees. That complicates the life of maintainers. Yes, understood. Its easier to fix it now if its really needed. Actually, I think the reason the size was passed was to make sure a valid struct dp_sdp *sdp was being passed. The size is supposed to be the size of *hardware* buffer where this gets written into. But looks like this wasn't done correctly when the code was copy-pasted from the HDMI inforframe code. Alright, in that case, let me post a patch to drop this and let me know if that works for you.
Re: [PATCH v2] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
On 2/20/2024 11:20 AM, Dmitry Baryshkov wrote: On Tue, 20 Feb 2024 at 21:05, Dmitry Baryshkov wrote: On Tue, 20 Feb 2024 at 20:53, Abhinav Kumar wrote: On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote: On Thu, 15 Feb 2024 at 21:08, Abhinav Kumar wrote: intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well. Lets move this to drm_dp_helper to achieve this. changes in v2: - rebased on top of drm-tip Acked-by: Dmitry Baryshkov v1 had an explicit comment before the ack: Yes, I remember the comment. I did not make any changes to v2 other than just rebasing it on drm-tip to get the ack from i915 folks. From my side, with the promise of the size fixup. However I observe neither a second patch removing the size argument nor it being dropped as a part of this patch. Yes, now that in v2 we got the ack for this patch, I can spin a v3 with the addition of the next patch to remove the size OR as another series so as to not block the main series which needs this patch. I would prefer the latter. It doesn't work this way. The comment should have been fixed for v2. This probably deserves some explanation. Currently there is only one user of this function. So it is easy to fix it. Once there are several users, you have to fix all of them at the same time, patching different drm subtrees. That complicates the life of maintainers. Yes, understood. Its easier to fix it now if its really needed. Actually, I think the reason the size was passed was to make sure a valid struct dp_sdp *sdp was being passed. If we drop the size, we need to have a if (!sdp) check as there is a memset followed by dereference. So maybe, if we want to keep this API protected, its not too bad to have?
Re: [PATCH v2] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
On 2/20/2024 11:05 AM, Dmitry Baryshkov wrote: On Tue, 20 Feb 2024 at 20:53, Abhinav Kumar wrote: On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote: On Thu, 15 Feb 2024 at 21:08, Abhinav Kumar wrote: intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well. Lets move this to drm_dp_helper to achieve this. changes in v2: - rebased on top of drm-tip Acked-by: Dmitry Baryshkov v1 had an explicit comment before the ack: Yes, I remember the comment. I did not make any changes to v2 other than just rebasing it on drm-tip to get the ack from i915 folks. From my side, with the promise of the size fixup. However I observe neither a second patch removing the size argument nor it being dropped as a part of this patch. Yes, now that in v2 we got the ack for this patch, I can spin a v3 with the addition of the next patch to remove the size OR as another series so as to not block the main series which needs this patch. I would prefer the latter. It doesn't work this way. The comment should have been fixed for v2. Ack, I can post a v3 now by adding the removal in patch 2 of this series.
Re: [PATCH v2] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
On 2/20/2024 10:49 AM, Dmitry Baryshkov wrote:
On Thu, 15 Feb 2024 at 21:08, Abhinav Kumar wrote:
intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
Lets move this to drm_dp_helper to achieve this.
changes in v2:
- rebased on top of drm-tip
Acked-by: Dmitry Baryshkov
v1 had an explicit comment before the ack:
Yes, I remember the comment. I did not make any changes to v2 other than
just rebasing it on drm-tip to get the ack from i915 folks.
From my side, with the promise of the size fixup.
However I observe neither a second patch removing the size argument
nor it being dropped as a part of this patch.
Yes, now that in v2 we got the ack for this patch, I can spin a v3 with
the addition of the next patch to remove the size OR as another series
so as to not block the main series which needs this patch.
I would prefer the latter.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/display/drm_dp_helper.c | 78 +
drivers/gpu/drm/i915/display/intel_dp.c | 71 +-
include/drm/display/drm_dp_helper.h | 3 +
3 files changed, 83 insertions(+), 69 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
b/drivers/gpu/drm/display/drm_dp_helper.c
index 8d6ce46471ae..6c91f400ecb1 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2913,6 +2913,84 @@ void drm_dp_vsc_sdp_log(struct drm_printer *p, const
struct drm_dp_vsc_sdp *vsc)
}
EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
+/**
+ * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
+ * @vsc: vsc sdp initialized according to its purpose as defined in
+ * table 2-118 - table 2-120 in DP 1.4a specification
+ * @sdp: valid handle to the generic dp_sdp which will be packed
+ * @size: valid size of the passed sdp handle
+ *
+ * Returns length of sdp on success and error code on failure
+ */
+ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
+ struct dp_sdp *sdp, size_t size)
+{
+ size_t length = sizeof(struct dp_sdp);
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(sdp, 0, size);
+
+ /*
+* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
+* VSC SDP Header Bytes
+*/
+ sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
+ sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
+ sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
+ sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
+
+ if (vsc->revision == 0x6) {
+ sdp->db[0] = 1;
+ sdp->db[3] = 1;
+ }
+
+ /*
+* Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
+* Format as per DP 1.4a spec and DP 2.0 respectively.
+*/
+ if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
+ goto out;
+
+ /* VSC SDP Payload for DB16 through DB18 */
+ /* Pixel Encoding and Colorimetry Formats */
+ sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
+ sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
+
+ switch (vsc->bpc) {
+ case 6:
+ /* 6bpc: 0x0 */
+ break;
+ case 8:
+ sdp->db[17] = 0x1; /* DB17[3:0] */
+ break;
+ case 10:
+ sdp->db[17] = 0x2;
+ break;
+ case 12:
+ sdp->db[17] = 0x3;
+ break;
+ case 16:
+ sdp->db[17] = 0x4;
+ break;
+ default:
+ WARN(1, "Missing case %d\n", vsc->bpc);
+ return -EINVAL;
+ }
+
+ /* Dynamic Range and Component Bit Depth */
+ if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
+ sdp->db[17] |= 0x80; /* DB17[7] */
+
+ /* Content Type */
+ sdp->db[18] = vsc->content_type & 0x7;
+
+out:
+ return length;
+}
+EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
+
/**
* drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
* @dpcd: DisplayPort configuration data
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 217196196e50..a9458df475e2 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4089,73 +4089,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state
*crtc_state,
return false;
}
-static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
-struct dp_sdp *sdp, size_t size)
-{
- size_t length = sizeof(struct dp_sdp);
-
- if (size < length)
- return -ENOSPC;
-
- memset(sdp, 0, size);
-
- /*
-* Prepare VSC Header for SU as per DP 1.4a
Re: [PATCH v5] drm/dp: add an API to indicate if sink supports VSC SDP
Hi DRM maintainers
Gentle ping for reviews on this one.
Since the dependent series is mostly complete, would like to get your
reviews on this one to land it.
Thanks
Abhinav
On 2/15/2024 11:15 AM, Abhinav Kumar wrote:
From: Paloma Arellano
YUV420 format is supported only in the VSC SDP packet and not through
MSA. Hence add an API which indicates the sink support which can be used
by the rest of the DP programming.
changes in v5:
- rebased on top of drm-tip
changes in v4:
- bail out early if dpcd rev check fails
changes in v3:
- fix the commit title prefix to drm/dp
- get rid of redundant !!
- break out this change from series [1] to get acks from drm core
maintainers
Changes in v2:
- Move VSC SDP support check API from dp_panel.c to
drm_dp_helper.c
[1]: https://patchwork.freedesktop.org/series/129180/
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Paloma Arellano
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/display/drm_dp_helper.c | 23 +++
include/drm/display/drm_dp_helper.h | 2 ++
2 files changed, 25 insertions(+)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
b/drivers/gpu/drm/display/drm_dp_helper.c
index 8d6ce46471ae..61b11cb45245 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2913,6 +2913,29 @@ void drm_dp_vsc_sdp_log(struct drm_printer *p, const
struct drm_dp_vsc_sdp *vsc)
}
EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
+/**
+ * drm_dp_vsc_sdp_supported() - check if vsc sdp is supported
+ * @aux: DisplayPort AUX channel
+ * @dpcd: DisplayPort configuration data
+ *
+ * Returns true if vsc sdp is supported, else returns false
+ */
+bool drm_dp_vsc_sdp_supported(struct drm_dp_aux *aux, const u8
dpcd[DP_RECEIVER_CAP_SIZE])
+{
+ u8 rx_feature;
+
+ if (dpcd[DP_DPCD_REV] < DP_DPCD_REV_13)
+ return false;
+
+ if (drm_dp_dpcd_readb(aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
&rx_feature) != 1) {
+ drm_dbg_dp(aux->drm_dev, "failed to read
DP_DPRX_FEATURE_ENUMERATION_LIST\n");
+ return false;
+ }
+
+ return (rx_feature & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED);
+}
+EXPORT_SYMBOL(drm_dp_vsc_sdp_supported);
+
/**
* drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
* @dpcd: DisplayPort configuration data
diff --git a/include/drm/display/drm_dp_helper.h
b/include/drm/display/drm_dp_helper.h
index d02014a87f12..36351f3cdba9 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -100,6 +100,8 @@ struct drm_dp_vsc_sdp {
void drm_dp_vsc_sdp_log(struct drm_printer *p, const struct drm_dp_vsc_sdp *vsc);
+bool drm_dp_vsc_sdp_supported(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
+
int drm_dp_psr_setup_time(const u8 psr_cap[EDP_PSR_RECEIVER_CAP_SIZE]);
static inline int
[PATCH v5] drm/dp: add an API to indicate if sink supports VSC SDP
From: Paloma Arellano
YUV420 format is supported only in the VSC SDP packet and not through
MSA. Hence add an API which indicates the sink support which can be used
by the rest of the DP programming.
changes in v5:
- rebased on top of drm-tip
changes in v4:
- bail out early if dpcd rev check fails
changes in v3:
- fix the commit title prefix to drm/dp
- get rid of redundant !!
- break out this change from series [1] to get acks from drm core
maintainers
Changes in v2:
- Move VSC SDP support check API from dp_panel.c to
drm_dp_helper.c
[1]: https://patchwork.freedesktop.org/series/129180/
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Paloma Arellano
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/display/drm_dp_helper.c | 23 +++
include/drm/display/drm_dp_helper.h | 2 ++
2 files changed, 25 insertions(+)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
b/drivers/gpu/drm/display/drm_dp_helper.c
index 8d6ce46471ae..61b11cb45245 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2913,6 +2913,29 @@ void drm_dp_vsc_sdp_log(struct drm_printer *p, const
struct drm_dp_vsc_sdp *vsc)
}
EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
+/**
+ * drm_dp_vsc_sdp_supported() - check if vsc sdp is supported
+ * @aux: DisplayPort AUX channel
+ * @dpcd: DisplayPort configuration data
+ *
+ * Returns true if vsc sdp is supported, else returns false
+ */
+bool drm_dp_vsc_sdp_supported(struct drm_dp_aux *aux, const u8
dpcd[DP_RECEIVER_CAP_SIZE])
+{
+ u8 rx_feature;
+
+ if (dpcd[DP_DPCD_REV] < DP_DPCD_REV_13)
+ return false;
+
+ if (drm_dp_dpcd_readb(aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
&rx_feature) != 1) {
+ drm_dbg_dp(aux->drm_dev, "failed to read
DP_DPRX_FEATURE_ENUMERATION_LIST\n");
+ return false;
+ }
+
+ return (rx_feature & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED);
+}
+EXPORT_SYMBOL(drm_dp_vsc_sdp_supported);
+
/**
* drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
* @dpcd: DisplayPort configuration data
diff --git a/include/drm/display/drm_dp_helper.h
b/include/drm/display/drm_dp_helper.h
index d02014a87f12..36351f3cdba9 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -100,6 +100,8 @@ struct drm_dp_vsc_sdp {
void drm_dp_vsc_sdp_log(struct drm_printer *p, const struct drm_dp_vsc_sdp
*vsc);
+bool drm_dp_vsc_sdp_supported(struct drm_dp_aux *aux, const u8
dpcd[DP_RECEIVER_CAP_SIZE]);
+
int drm_dp_psr_setup_time(const u8 psr_cap[EDP_PSR_RECEIVER_CAP_SIZE]);
static inline int
--
2.34.1
[PATCH v2] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
Lets move this to drm_dp_helper to achieve this.
changes in v2:
- rebased on top of drm-tip
Acked-by: Dmitry Baryshkov
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/display/drm_dp_helper.c | 78 +
drivers/gpu/drm/i915/display/intel_dp.c | 71 +-
include/drm/display/drm_dp_helper.h | 3 +
3 files changed, 83 insertions(+), 69 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
b/drivers/gpu/drm/display/drm_dp_helper.c
index 8d6ce46471ae..6c91f400ecb1 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2913,6 +2913,84 @@ void drm_dp_vsc_sdp_log(struct drm_printer *p, const
struct drm_dp_vsc_sdp *vsc)
}
EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
+/**
+ * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
+ * @vsc: vsc sdp initialized according to its purpose as defined in
+ * table 2-118 - table 2-120 in DP 1.4a specification
+ * @sdp: valid handle to the generic dp_sdp which will be packed
+ * @size: valid size of the passed sdp handle
+ *
+ * Returns length of sdp on success and error code on failure
+ */
+ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
+ struct dp_sdp *sdp, size_t size)
+{
+ size_t length = sizeof(struct dp_sdp);
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(sdp, 0, size);
+
+ /*
+* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
+* VSC SDP Header Bytes
+*/
+ sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
+ sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
+ sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
+ sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
+
+ if (vsc->revision == 0x6) {
+ sdp->db[0] = 1;
+ sdp->db[3] = 1;
+ }
+
+ /*
+* Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
+* Format as per DP 1.4a spec and DP 2.0 respectively.
+*/
+ if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
+ goto out;
+
+ /* VSC SDP Payload for DB16 through DB18 */
+ /* Pixel Encoding and Colorimetry Formats */
+ sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
+ sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
+
+ switch (vsc->bpc) {
+ case 6:
+ /* 6bpc: 0x0 */
+ break;
+ case 8:
+ sdp->db[17] = 0x1; /* DB17[3:0] */
+ break;
+ case 10:
+ sdp->db[17] = 0x2;
+ break;
+ case 12:
+ sdp->db[17] = 0x3;
+ break;
+ case 16:
+ sdp->db[17] = 0x4;
+ break;
+ default:
+ WARN(1, "Missing case %d\n", vsc->bpc);
+ return -EINVAL;
+ }
+
+ /* Dynamic Range and Component Bit Depth */
+ if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
+ sdp->db[17] |= 0x80; /* DB17[7] */
+
+ /* Content Type */
+ sdp->db[18] = vsc->content_type & 0x7;
+
+out:
+ return length;
+}
+EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
+
/**
* drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
* @dpcd: DisplayPort configuration data
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 217196196e50..a9458df475e2 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4089,73 +4089,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state
*crtc_state,
return false;
}
-static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
-struct dp_sdp *sdp, size_t size)
-{
- size_t length = sizeof(struct dp_sdp);
-
- if (size < length)
- return -ENOSPC;
-
- memset(sdp, 0, size);
-
- /*
-* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
-* VSC SDP Header Bytes
-*/
- sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
- sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
- sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
- sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
-
- if (vsc->revision == 0x6) {
- sdp->db[0] = 1;
- sdp->db[3] = 1;
- }
-
- /*
-* Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
-* Format as per DP 1.4a spec and DP 2.0 respectively.
-*/
- if (!(vsc->revision == 0x5 || vsc->
Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
On 2/14/2024 10:02 AM, Ville Syrjälä wrote:
On Wed, Feb 14, 2024 at 09:17:34AM -0800, Abhinav Kumar wrote:
On 2/14/2024 12:15 AM, Dmitry Baryshkov wrote:
On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar wrote:
intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
Lets move this to drm_dp_helper to achieve this.
Signed-off-by: Abhinav Kumar
My preference would be to have packing functions in
drivers/video/hdmi.c, as we already have
hdmi_audio_infoframe_pack_for_dp() there.
My preference is drm_dp_helper because it already has some VSC SDP stuff
and after discussion with Ville on IRC, I decided to post it this way.
hdmi_audio_infoframe_pack_for_dp() is an exception from my PoV as the
hdmi audio infoframe fields were re-used and packed into a DP SDP
thereby re-using the existing struct hdmi_audio_infoframe .
This is not like that. Here we pack from struct drm_dp_vsc_sdp to struct
dp_sdp both of which had prior usages already in this file.
So it all adds up and makes sense to me to be in this file.
I will let the other DRM core maintainers comment on this.
Ville, Jani?
Yeah, I'm not sure bloating the (poorly named) hdmi.c with all
SDP stuff is a great idea. Since other related stuff already
lives in the drm_dp_helper.c that seems reasonable to me at this
time. And if we get a decent amount of this then probably all
DP SDP stuff should be extracted into its own file.
Yes, thanks.
There are of course a few overlaps here andthere (the audio SDP
I guess, and the CTA infoframe SDP). But I'm not sure that actually
needs any SDP specific stuff in hdmi.c, or could we just let hdmi.c
deal with the actual CTA-861 stuff and then have the DP SDP code
wrap that up in its own thing externally? Dunno, haven't really
looked at the details.
Thats a good way to look at it. this packing is from DP spec and not CTA
so makes more sense to be in this file.
In that case, R-b?
---
drivers/gpu/drm/display/drm_dp_helper.c | 78 +
drivers/gpu/drm/i915/display/intel_dp.c | 73 +--
include/drm/display/drm_dp_helper.h | 3 +
3 files changed, 84 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
b/drivers/gpu/drm/display/drm_dp_helper.c
index b1ca3a1100da..066cfbbf7a91 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device
*dev,
}
EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
+/**
+ * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
+ * @vsc: vsc sdp initialized according to its purpose as defined in
+ * table 2-118 - table 2-120 in DP 1.4a specification
+ * @sdp: valid handle to the generic dp_sdp which will be packed
+ * @size: valid size of the passed sdp handle
+ *
+ * Returns length of sdp on success and error code on failure
+ */
+ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
+ struct dp_sdp *sdp, size_t size)
I know that you are just moving the function. Maybe there can be
patch#2, which drops the size argument? The struct dp_sdp already has
a defined size. The i915 driver just passes sizeof(sdp), which is more
or less useless.
Yes this is a valid point, I also noticed this. I can post it on top of
this once we get an agreement and ack on this patch first.
+{
+ size_t length = sizeof(struct dp_sdp);
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(sdp, 0, size);
+
+ /*
+* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
+* VSC SDP Header Bytes
+*/
+ sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
+ sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
+ sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
+ sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
+
+ if (vsc->revision == 0x6) {
+ sdp->db[0] = 1;
+ sdp->db[3] = 1;
+ }
+
+ /*
+* Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
+* Format as per DP 1.4a spec and DP 2.0 respectively.
+*/
+ if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
+ goto out;
+
+ /* VSC SDP Payload for DB16 through DB18 */
+ /* Pixel Encoding and Colorimetry Formats */
+ sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
+ sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
+
+ switch (vsc->bpc) {
+ case 6:
+ /* 6bpc: 0x0 */
+ break;
+ case 8:
+ sdp->db[17] = 0x1; /* DB17[3:0] */
+ break;
+ case 10:
+ sdp->db[17] = 0x2;
+ break;
+ case 12:
+ sdp->db[17] = 0x3;
+
Re: [PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
On 2/14/2024 12:15 AM, Dmitry Baryshkov wrote:
On Wed, 14 Feb 2024 at 01:45, Abhinav Kumar wrote:
intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
Lets move this to drm_dp_helper to achieve this.
Signed-off-by: Abhinav Kumar
My preference would be to have packing functions in
drivers/video/hdmi.c, as we already have
hdmi_audio_infoframe_pack_for_dp() there.
My preference is drm_dp_helper because it already has some VSC SDP stuff
and after discussion with Ville on IRC, I decided to post it this way.
hdmi_audio_infoframe_pack_for_dp() is an exception from my PoV as the
hdmi audio infoframe fields were re-used and packed into a DP SDP
thereby re-using the existing struct hdmi_audio_infoframe .
This is not like that. Here we pack from struct drm_dp_vsc_sdp to struct
dp_sdp both of which had prior usages already in this file.
So it all adds up and makes sense to me to be in this file.
I will let the other DRM core maintainers comment on this.
Ville, Jani?
---
drivers/gpu/drm/display/drm_dp_helper.c | 78 +
drivers/gpu/drm/i915/display/intel_dp.c | 73 +--
include/drm/display/drm_dp_helper.h | 3 +
3 files changed, 84 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
b/drivers/gpu/drm/display/drm_dp_helper.c
index b1ca3a1100da..066cfbbf7a91 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device
*dev,
}
EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
+/**
+ * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
+ * @vsc: vsc sdp initialized according to its purpose as defined in
+ * table 2-118 - table 2-120 in DP 1.4a specification
+ * @sdp: valid handle to the generic dp_sdp which will be packed
+ * @size: valid size of the passed sdp handle
+ *
+ * Returns length of sdp on success and error code on failure
+ */
+ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
+ struct dp_sdp *sdp, size_t size)
I know that you are just moving the function. Maybe there can be
patch#2, which drops the size argument? The struct dp_sdp already has
a defined size. The i915 driver just passes sizeof(sdp), which is more
or less useless.
Yes this is a valid point, I also noticed this. I can post it on top of
this once we get an agreement and ack on this patch first.
+{
+ size_t length = sizeof(struct dp_sdp);
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(sdp, 0, size);
+
+ /*
+* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
+* VSC SDP Header Bytes
+*/
+ sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
+ sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
+ sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
+ sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
+
+ if (vsc->revision == 0x6) {
+ sdp->db[0] = 1;
+ sdp->db[3] = 1;
+ }
+
+ /*
+* Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
+* Format as per DP 1.4a spec and DP 2.0 respectively.
+*/
+ if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
+ goto out;
+
+ /* VSC SDP Payload for DB16 through DB18 */
+ /* Pixel Encoding and Colorimetry Formats */
+ sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
+ sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
+
+ switch (vsc->bpc) {
+ case 6:
+ /* 6bpc: 0x0 */
+ break;
+ case 8:
+ sdp->db[17] = 0x1; /* DB17[3:0] */
+ break;
+ case 10:
+ sdp->db[17] = 0x2;
+ break;
+ case 12:
+ sdp->db[17] = 0x3;
+ break;
+ case 16:
+ sdp->db[17] = 0x4;
+ break;
+ default:
+ WARN(1, "Missing case %d\n", vsc->bpc);
+ return -EINVAL;
+ }
+
+ /* Dynamic Range and Component Bit Depth */
+ if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
+ sdp->db[17] |= 0x80; /* DB17[7] */
+
+ /* Content Type */
+ sdp->db[18] = vsc->content_type & 0x7;
+
+out:
+ return length;
+}
+EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
+
/**
* drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
* @dpcd: DisplayPort configuration data
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index f5ef95da5534..e94db51aeeb7 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4110,73 +4110,6 @@ i
[PATCH] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
Lets move this to drm_dp_helper to achieve this.
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/display/drm_dp_helper.c | 78 +
drivers/gpu/drm/i915/display/intel_dp.c | 73 +--
include/drm/display/drm_dp_helper.h | 3 +
3 files changed, 84 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
b/drivers/gpu/drm/display/drm_dp_helper.c
index b1ca3a1100da..066cfbbf7a91 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2916,6 +2916,84 @@ void drm_dp_vsc_sdp_log(const char *level, struct device
*dev,
}
EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
+/**
+ * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
+ * @vsc: vsc sdp initialized according to its purpose as defined in
+ * table 2-118 - table 2-120 in DP 1.4a specification
+ * @sdp: valid handle to the generic dp_sdp which will be packed
+ * @size: valid size of the passed sdp handle
+ *
+ * Returns length of sdp on success and error code on failure
+ */
+ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
+ struct dp_sdp *sdp, size_t size)
+{
+ size_t length = sizeof(struct dp_sdp);
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(sdp, 0, size);
+
+ /*
+* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
+* VSC SDP Header Bytes
+*/
+ sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
+ sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
+ sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
+ sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
+
+ if (vsc->revision == 0x6) {
+ sdp->db[0] = 1;
+ sdp->db[3] = 1;
+ }
+
+ /*
+* Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
+* Format as per DP 1.4a spec and DP 2.0 respectively.
+*/
+ if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
+ goto out;
+
+ /* VSC SDP Payload for DB16 through DB18 */
+ /* Pixel Encoding and Colorimetry Formats */
+ sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
+ sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
+
+ switch (vsc->bpc) {
+ case 6:
+ /* 6bpc: 0x0 */
+ break;
+ case 8:
+ sdp->db[17] = 0x1; /* DB17[3:0] */
+ break;
+ case 10:
+ sdp->db[17] = 0x2;
+ break;
+ case 12:
+ sdp->db[17] = 0x3;
+ break;
+ case 16:
+ sdp->db[17] = 0x4;
+ break;
+ default:
+ WARN(1, "Missing case %d\n", vsc->bpc);
+ return -EINVAL;
+ }
+
+ /* Dynamic Range and Component Bit Depth */
+ if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
+ sdp->db[17] |= 0x80; /* DB17[7] */
+
+ /* Content Type */
+ sdp->db[18] = vsc->content_type & 0x7;
+
+out:
+ return length;
+}
+EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
+
/**
* drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
* @dpcd: DisplayPort configuration data
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index f5ef95da5534..e94db51aeeb7 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4110,73 +4110,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state
*crtc_state,
return false;
}
-static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
-struct dp_sdp *sdp, size_t size)
-{
- size_t length = sizeof(struct dp_sdp);
-
- if (size < length)
- return -ENOSPC;
-
- memset(sdp, 0, size);
-
- /*
-* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
-* VSC SDP Header Bytes
-*/
- sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
- sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
- sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
- sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
-
- if (vsc->revision == 0x6) {
- sdp->db[0] = 1;
- sdp->db[3] = 1;
- }
-
- /*
-* Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
-* Format as per DP 1.4a spec and DP 2.0 respectively.
-*/
- if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
- goto out;
-
- /* VSC SDP Payload for DB16 through DB18 */
-
[PATCH v4] drm/dp: add an API to indicate if sink supports VSC SDP
From: Paloma Arellano
YUV420 format is supported only in the VSC SDP packet and not through
MSA. Hence add an API which indicates the sink support which can be used
by the rest of the DP programming.
changes in v4:
- bail out early if dpcd rev check fails
changes in v3:
- fix the commit title prefix to drm/dp
- get rid of redundant !!
- break out this change from series [1] to get acks from drm core
maintainers
Changes in v2:
- Move VSC SDP support check API from dp_panel.c to
drm_dp_helper.c
[1]: https://patchwork.freedesktop.org/series/129180/
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Paloma Arellano
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/display/drm_dp_helper.c | 23 +++
include/drm/display/drm_dp_helper.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
b/drivers/gpu/drm/display/drm_dp_helper.c
index b1ca3a1100da..b10fb2be837e 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2916,6 +2916,29 @@ void drm_dp_vsc_sdp_log(const char *level, struct device
*dev,
}
EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
+/**
+ * drm_dp_vsc_sdp_supported() - check if vsc sdp is supported
+ * @aux: DisplayPort AUX channel
+ * @dpcd: DisplayPort configuration data
+ *
+ * Returns true if vsc sdp is supported, else returns false
+ */
+bool drm_dp_vsc_sdp_supported(struct drm_dp_aux *aux, const u8
dpcd[DP_RECEIVER_CAP_SIZE])
+{
+ u8 rx_feature;
+
+ if (dpcd[DP_DPCD_REV] < DP_DPCD_REV_13)
+ return false;
+
+ if (drm_dp_dpcd_readb(aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
&rx_feature) != 1) {
+ drm_dbg_dp(aux->drm_dev, "failed to read
DP_DPRX_FEATURE_ENUMERATION_LIST\n");
+ return false;
+ }
+
+ return (rx_feature & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED);
+}
+EXPORT_SYMBOL(drm_dp_vsc_sdp_supported);
+
/**
* drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
* @dpcd: DisplayPort configuration data
diff --git a/include/drm/display/drm_dp_helper.h
b/include/drm/display/drm_dp_helper.h
index 863b2e7add29..948381b2b0b1 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -100,6 +100,7 @@ struct drm_dp_vsc_sdp {
void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
const struct drm_dp_vsc_sdp *vsc);
+bool drm_dp_vsc_sdp_supported(struct drm_dp_aux *aux, const u8
dpcd[DP_RECEIVER_CAP_SIZE]);
int drm_dp_psr_setup_time(const u8 psr_cap[EDP_PSR_RECEIVER_CAP_SIZE]);
--
2.34.1
[PATCH v3] drm/dp: add an API to indicate if sink supports VSC SDP
From: Paloma Arellano
YUV420 format is supported only in the VSC SDP packet and not through
MSA. Hence add an API which indicates the sink support which can be used
by the rest of the DP programming.
changes in v3:
- fix the commit title prefix to drm/dp
- get rid of redundant !!
- break out this change from series [1] to get acks from drm core
maintainers
Changes in v2:
- Move VSC SDP support check API from dp_panel.c to
drm_dp_helper.c
[1]: https://patchwork.freedesktop.org/series/129180/
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Paloma Arellano
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/display/drm_dp_helper.c | 21 +
include/drm/display/drm_dp_helper.h | 1 +
2 files changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
b/drivers/gpu/drm/display/drm_dp_helper.c
index b1ca3a1100da..7a851f92b249 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2916,6 +2916,27 @@ void drm_dp_vsc_sdp_log(const char *level, struct device
*dev,
}
EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
+/**
+ * drm_dp_vsc_sdp_supported() - check if vsc sdp is supported
+ * @aux: DisplayPort AUX channel
+ * @dpcd: DisplayPort configuration data
+ *
+ * Returns true if vsc sdp is supported, else returns false
+ */
+bool drm_dp_vsc_sdp_supported(struct drm_dp_aux *aux, const u8
dpcd[DP_RECEIVER_CAP_SIZE])
+{
+ u8 rx_feature;
+
+ if (drm_dp_dpcd_readb(aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
&rx_feature) != 1) {
+ drm_dbg_dp(aux->drm_dev, "failed to read
DP_DPRX_FEATURE_ENUMERATION_LIST\n");
+ return false;
+ }
+
+ return (dpcd[DP_DPCD_REV] >= DP_DPCD_REV_13) &&
+ (rx_feature & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED);
+}
+EXPORT_SYMBOL(drm_dp_vsc_sdp_supported);
+
/**
* drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
* @dpcd: DisplayPort configuration data
diff --git a/include/drm/display/drm_dp_helper.h
b/include/drm/display/drm_dp_helper.h
index 863b2e7add29..948381b2b0b1 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -100,6 +100,7 @@ struct drm_dp_vsc_sdp {
void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
const struct drm_dp_vsc_sdp *vsc);
+bool drm_dp_vsc_sdp_supported(struct drm_dp_aux *aux, const u8
dpcd[DP_RECEIVER_CAP_SIZE]);
int drm_dp_psr_setup_time(const u8 psr_cap[EDP_PSR_RECEIVER_CAP_SIZE]);
--
2.34.1
Re: [Intel-gfx] [PATCH v3 3/5] drm/msm: add trailing newlines to drm_dbg msgs
Hi Jim On 9/6/2023 12:02 PM, Jim Cromie wrote: By at least strong convention, a print-buffer's trailing newline says "message complete, send it". The exception (no TNL, followed by a call to pr_cont) proves the general rule. Most DRM.debug calls already comport with this: 207 DRM_DEV_DEBUG, 1288 drm_dbg. Clean up the remainders, in maintainer sized chunks. May I know what 207, 1288 mean here? Is it the number of callers already having \n? If so, this might be a big confusing as its subjective to the code-base you are referring to. So I will just stop with "Most DRM.debug calls already comport with this". No functional changes. Signed-off-by: Jim Cromie --- drivers/gpu/drm/msm/msm_fb.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) The change itself LGTM, hence Reviewed-by: Abhinav Kumar
Re: [Intel-gfx] [PATCH v5 11/13] drm/msm: Use regular fbdev I/O helpers
On 5/30/2023 8:02 AM, Thomas Zimmermann wrote: Use the regular fbdev helpers for framebuffer I/O instead of DRM's helpers. Msm does not use damage handling, so DRM's fbdev helpers are mere wrappers around the fbdev code. By using fbdev helpers directly within each DRM fbdev emulation, we can eventually remove DRM's wrapper functions entirely. Msm's fbdev emulation has been incomplete as it didn't implement damage handling. Partilly fix this by implementing damage handling for write and draw operation. It is still missing for mmaped pages. v4: * use initializer macros for struct fb_ops * partially support damage handling v2: * use FB_SYS_HELPERS option Signed-off-by: Thomas Zimmermann Reviewed-by: Dmitry Baryshkov Acked-by: Sam Ravnborg Cc: Rob Clark Cc: Abhinav Kumar Cc: Dmitry Baryshkov Cc: Sean Paul --- Reviewed-by: Abhinav Kumar
Re: [Intel-gfx] [Freedreno] [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions
On 3/16/2023 9:36 AM, Abhinav Kumar wrote:
On 3/16/2023 9:23 AM, Dmitry Baryshkov wrote:
On 16/03/2023 18:13, Abhinav Kumar wrote:
On 3/16/2023 9:03 AM, Dmitry Baryshkov wrote:
Hi,
[removed previous conversation]
Hi Dmitry and Abhinav,
Just wanted to follow up on this thread. I've gone over the
MSM-specific
DSC params for DP and DSI and have found a few shared calculations and
variables between both DSI and DP paths:
- (as mentioned earlier in the thread) almost all the calculations in
dpu_dsc_populate_dsc_config() match dsi_populate_dsc_params() [1]. The
only difference in the math I'm seeing is initial_scale_value.
The value in dsi code is valid for initial_offset = 6144. Please use
the formula from the standard (= sde_dsc_populate_dsc_config) and add
it to drm_dsc_helper.c
Yes, I agree with this part. for rc_model_size we can use
DSC_RC_MODEL_SIZE_CONST.
initial_offset is already handled in
https://patchwork.freedesktop.org/patch/525424/?series=114472&rev=2
Then we can use this math:
rc_model_size / (rc_model_size -
initial_offset), keeping in mind that initial_scale_value has three
fractional bits.
So this would be 8192 / (8192 - 6144) = 4
Then << 3 for 3 fractional bits = 32.
If I remember correctly the last remaining item in
dsi_populate_dsc_params() (except mentioned initial_offset) was
line_buf_depth, see [3]. I'm not sure about setting it to bpc+1.
According to the standard it should come from a DSC decoder spec,
which means it should be set by the DSI panel driver or via
drm_dp_dsc_sink_line_buf_depth() in the case of DP output.
- dsc_extra_pclk_cycle_cnt and dce_bytes_per_line, which were
introduced
in Kuogee's v1 DSC series [2], are used for DSI, DP, and the DPU
timing
engine. dsc_extra_pclk_cycle_cnt is calculated based on pclk_per_line
(which is calculated differently between DP and DSI), but
dce_bytes_per_line is calculated the same way between DP and DSI.
To avoid having to duplicate math in 2 different places, I think it
would help to have these calculations in some msm_dsc_helper.c
file. Any
thoughts on this?
dsc_extra_pclk_cycle_cnt and dce_bytes_per_line are used only in DPU
code, so they can stay in DPU driver.
They can stay in the dpu driver is fine but where?
Like Jessica wrote, this is computed and used in 3 places today :
1) DSI video engine computation
2) DP controller computation
3) timing engine programming
Please excuse me if I'm wrong. I checked both vendor techpack and the
Kuogee's patches. I see them being used only in the SDE / DPU driver
code. Could you please point me to the code path that we are discussing?
DSI code :
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/dsi/dsi_host.c#L868
DP code:
Refer to dp_panel_dsc_pclk_param_calc in
https://patchwork.freedesktop.org/patch/519837/?series=113240&rev=1
Timing engine:
refer to
https://patchwork.freedesktop.org/patch/519838/?series=113240&rev=1
Probably confusion is due to the naming. bytes_per_line is nothing but
bytes_per_pkt * pkt_per_line but the concept is common for DP and DSI.
+ if (phys->comp_type == MSM_DISPLAY_COMPRESSION_DSC) {
+ phys->dsc_extra_pclk_cycle_cnt = dsc_info->pclk_per_line;
+ phys->dsc_extra_disp_width = dsc_info->extra_width;
+ phys->dce_bytes_per_line =
+ dsc_info->bytes_per_pkt * dsc_info->pkt_per_line;
So either we have a helper in a common location somewhere so that
these 3 modules can call that helper and use it OR each module
duplicates the computation code.
What should be the common location is the discussion here.
It cannot be dpu_encoder.c as the DSI/DP dont call into the encoder
methods.
Thanks,
Jessica Zhang
[1]
https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/gpu/drm/msm/dsi/dsi_host.c#L1756
[2]
https://patchwork.freedesktop.org/patch/519845/?series=113240&rev=1
[3] https://patchwork.freedesktop.org/patch/525441/?series=114472&rev=2
Re: [Intel-gfx] [Freedreno] [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions
On 3/16/2023 9:23 AM, Dmitry Baryshkov wrote:
On 16/03/2023 18:13, Abhinav Kumar wrote:
On 3/16/2023 9:03 AM, Dmitry Baryshkov wrote:
Hi,
[removed previous conversation]
Hi Dmitry and Abhinav,
Just wanted to follow up on this thread. I've gone over the
MSM-specific
DSC params for DP and DSI and have found a few shared calculations and
variables between both DSI and DP paths:
- (as mentioned earlier in the thread) almost all the calculations in
dpu_dsc_populate_dsc_config() match dsi_populate_dsc_params() [1]. The
only difference in the math I'm seeing is initial_scale_value.
The value in dsi code is valid for initial_offset = 6144. Please use
the formula from the standard (= sde_dsc_populate_dsc_config) and add
it to drm_dsc_helper.c
If I remember correctly the last remaining item in
dsi_populate_dsc_params() (except mentioned initial_offset) was
line_buf_depth, see [3]. I'm not sure about setting it to bpc+1.
According to the standard it should come from a DSC decoder spec,
which means it should be set by the DSI panel driver or via
drm_dp_dsc_sink_line_buf_depth() in the case of DP output.
- dsc_extra_pclk_cycle_cnt and dce_bytes_per_line, which were
introduced
in Kuogee's v1 DSC series [2], are used for DSI, DP, and the DPU timing
engine. dsc_extra_pclk_cycle_cnt is calculated based on pclk_per_line
(which is calculated differently between DP and DSI), but
dce_bytes_per_line is calculated the same way between DP and DSI.
To avoid having to duplicate math in 2 different places, I think it
would help to have these calculations in some msm_dsc_helper.c file.
Any
thoughts on this?
dsc_extra_pclk_cycle_cnt and dce_bytes_per_line are used only in DPU
code, so they can stay in DPU driver.
They can stay in the dpu driver is fine but where?
Like Jessica wrote, this is computed and used in 3 places today :
1) DSI video engine computation
2) DP controller computation
3) timing engine programming
Please excuse me if I'm wrong. I checked both vendor techpack and the
Kuogee's patches. I see them being used only in the SDE / DPU driver
code. Could you please point me to the code path that we are discussing?
DSI code :
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/dsi/dsi_host.c#L868
DP code:
Refer to dp_panel_dsc_pclk_param_calc in
https://patchwork.freedesktop.org/patch/519837/?series=113240&rev=1
Timing engine:
refer to https://patchwork.freedesktop.org/patch/519838/?series=113240&rev=1
Probably confusion is due to the naming. bytes_per_line is nothing but
bytes_per_pkt * pkt_per_line but the concept is common for DP and DSI.
+ if (phys->comp_type == MSM_DISPLAY_COMPRESSION_DSC) {
+ phys->dsc_extra_pclk_cycle_cnt =
dsc_info->pclk_per_line;
+ phys->dsc_extra_disp_width = dsc_info->extra_width;
+ phys->dce_bytes_per_line =
+ dsc_info->bytes_per_pkt *
dsc_info->pkt_per_line;
So either we have a helper in a common location somewhere so that
these 3 modules can call that helper and use it OR each module
duplicates the computation code.
What should be the common location is the discussion here.
It cannot be dpu_encoder.c as the DSI/DP dont call into the encoder
methods.
Thanks,
Jessica Zhang
[1]
https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/gpu/drm/msm/dsi/dsi_host.c#L1756
[2] https://patchwork.freedesktop.org/patch/519845/?series=113240&rev=1
[3] https://patchwork.freedesktop.org/patch/525441/?series=114472&rev=2
Re: [Intel-gfx] [Freedreno] [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions
On 3/16/2023 9:03 AM, Dmitry Baryshkov wrote: Hi, [removed previous conversation] Hi Dmitry and Abhinav, Just wanted to follow up on this thread. I've gone over the MSM-specific DSC params for DP and DSI and have found a few shared calculations and variables between both DSI and DP paths: - (as mentioned earlier in the thread) almost all the calculations in dpu_dsc_populate_dsc_config() match dsi_populate_dsc_params() [1]. The only difference in the math I'm seeing is initial_scale_value. The value in dsi code is valid for initial_offset = 6144. Please use the formula from the standard (= sde_dsc_populate_dsc_config) and add it to drm_dsc_helper.c If I remember correctly the last remaining item in dsi_populate_dsc_params() (except mentioned initial_offset) was line_buf_depth, see [3]. I'm not sure about setting it to bpc+1. According to the standard it should come from a DSC decoder spec, which means it should be set by the DSI panel driver or via drm_dp_dsc_sink_line_buf_depth() in the case of DP output. - dsc_extra_pclk_cycle_cnt and dce_bytes_per_line, which were introduced in Kuogee's v1 DSC series [2], are used for DSI, DP, and the DPU timing engine. dsc_extra_pclk_cycle_cnt is calculated based on pclk_per_line (which is calculated differently between DP and DSI), but dce_bytes_per_line is calculated the same way between DP and DSI. To avoid having to duplicate math in 2 different places, I think it would help to have these calculations in some msm_dsc_helper.c file. Any thoughts on this? dsc_extra_pclk_cycle_cnt and dce_bytes_per_line are used only in DPU code, so they can stay in DPU driver. They can stay in the dpu driver is fine but where? Like Jessica wrote, this is computed and used in 3 places today : 1) DSI video engine computation 2) DP controller computation 3) timing engine programming So either we have a helper in a common location somewhere so that these 3 modules can call that helper and use it OR each module duplicates the computation code. What should be the common location is the discussion here. It cannot be dpu_encoder.c as the DSI/DP dont call into the encoder methods. Thanks, Jessica Zhang [1] https://elixir.bootlin.com/linux/v6.3-rc2/source/drivers/gpu/drm/msm/dsi/dsi_host.c#L1756 [2] https://patchwork.freedesktop.org/patch/519845/?series=113240&rev=1 [3] https://patchwork.freedesktop.org/patch/525441/?series=114472&rev=2
Re: [Intel-gfx] [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions
On 2/27/2023 11:25 AM, Dmitry Baryshkov wrote:
27 февраля 2023 г. 19:59:35 GMT+02:00, Abhinav Kumar
пишет:
On 2/27/2023 4:45 AM, Dmitry Baryshkov wrote:
On Mon, 27 Feb 2023 at 01:49, Abhinav Kumar wrote:
On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:
On 26/02/2023 02:47, Abhinav Kumar wrote:
Hi Dmitry
On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
On 25/02/2023 02:36, Abhinav Kumar wrote:
On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar
wrote:
On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar
пишет:
On 2/24/2023 1:13 PM, Dmitry Baryshkov wrote:
On 24/02/2023 21:40, Kuogee Hsieh wrote:
Add DSC helper functions based on DSC configuration profiles
to produce
DSC related runtime parameters through both table look up and
runtime
calculation to support DSC on DPU.
There are 6 different DSC configuration profiles are supported
currently.
DSC configuration profiles are differiented by 5 keys, DSC
version (V1.1),
chroma (444/422/420), colorspace (RGB/YUV), bpc(8/10),
bpp (6/7/7.5/8/9/10/12/15) and SCR (0/1).
Only DSC version V1.1 added and V1.2 will be added later.
These helpers should go to
drivers/gpu/drm/display/drm_dsc_helper.c
Also please check that they can be used for i915 or for amdgpu
(ideally for both of them).
No, it cannot. So each DSC encoder parameter is calculated based
on the HW core which is being used.
They all get packed to the same DSC structure which is the
struct drm_dsc_config but the way the parameters are computed is
specific to the HW.
This DPU file helper still uses the drm_dsc_helper's
drm_dsc_compute_rc_parameters() like all other vendors do but
the parameters themselves are very HW specific and belong to
each vendor's dir.
This is not unique to MSM.
Lets take a few other examples:
AMD:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
i915:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
I checked several values here. Intel driver defines more bpc/bpp
combinations, but the ones which are defined in intel_vdsc and in
this patch seem to match. If there are major differences there,
please point me to the exact case.
I remember that AMD driver might have different values.
Some values in the rc_params table do match. But the
rc_buf_thresh[] doesnt.
Because later they do:
vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
Vs
+static u16 dpu_dsc_rc_buf_thresh[DSC_NUM_BUF_RANGES - 1] = {
+ 0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54,
+ 0x62, 0x69, 0x70, 0x77, 0x79, 0x7b, 0x7d, 0x7e
+};
I'd prefer to have 896, 1792, etc. here, as those values come from the
standard. As it's done in the Intel driver.
Got it, thanks
I dont know the AMD calculation very well to say that moving this
to the
helper is going to help.
Those calculations correspond (more or less) at the first glance to
what intel does for their newer generations. I think that's not our
problem for now.
Well, we have to figure out if each value matches and if each of
them come from the spec for us and i915 and from which section. So
it is unfortunately our problem.
Otherwise it will have to be handled by Marijn, me or anybody else
wanting to hack up the DSC code. Or by anybody adding DSC support to
the next platform and having to figure out the difference between
i915, msm and their platform.
Yes, I wonder why the same doubt didn't arise when the other vendors
added their support both from other maintainers and others.
Which makes me think that like I wrote in my previous response, these
are "recommended" values in the spec but its not mandatory.
I think, it is because there were no other drivers to compare. In other
words, for a first driver it is pretty logical to have everything
handled on its own. As soon as we start getting other implementations of
a feature, it becomes logical to think if the code can be generalized.
This is what we see we with the HDCP series or with the code being moved
to DP helpers.
We were not the second, MSM was/is the third to add support for DSC afer
i915 and AMD. Thats what made me think why whoever was the second didnt
end up generalizing. Was it just missed out or was it intentionally left
in the vendor driver.
I didn't count AMD here, since it calculates some of the params rather
than using the fixed ones from the model.
Moving this to the drm_dsc_helper is generalizing the tables and not
giving room for the vendors to customize even if they want to (which
the spec does allow).
That depends on the API you select. For example, in
intel_dsc_compute_params() I see customization being applied to
rc_buf_thresh in 6bpp case. I'd leave th
Re: [Intel-gfx] [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions
On 2/27/2023 4:45 AM, Dmitry Baryshkov wrote:
On Mon, 27 Feb 2023 at 01:49, Abhinav Kumar wrote:
On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:
On 26/02/2023 02:47, Abhinav Kumar wrote:
Hi Dmitry
On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
On 25/02/2023 02:36, Abhinav Kumar wrote:
On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar
wrote:
On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar
пишет:
On 2/24/2023 1:13 PM, Dmitry Baryshkov wrote:
On 24/02/2023 21:40, Kuogee Hsieh wrote:
Add DSC helper functions based on DSC configuration profiles
to produce
DSC related runtime parameters through both table look up and
runtime
calculation to support DSC on DPU.
There are 6 different DSC configuration profiles are supported
currently.
DSC configuration profiles are differiented by 5 keys, DSC
version (V1.1),
chroma (444/422/420), colorspace (RGB/YUV), bpc(8/10),
bpp (6/7/7.5/8/9/10/12/15) and SCR (0/1).
Only DSC version V1.1 added and V1.2 will be added later.
These helpers should go to
drivers/gpu/drm/display/drm_dsc_helper.c
Also please check that they can be used for i915 or for amdgpu
(ideally for both of them).
No, it cannot. So each DSC encoder parameter is calculated based
on the HW core which is being used.
They all get packed to the same DSC structure which is the
struct drm_dsc_config but the way the parameters are computed is
specific to the HW.
This DPU file helper still uses the drm_dsc_helper's
drm_dsc_compute_rc_parameters() like all other vendors do but
the parameters themselves are very HW specific and belong to
each vendor's dir.
This is not unique to MSM.
Lets take a few other examples:
AMD:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
i915:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
I checked several values here. Intel driver defines more bpc/bpp
combinations, but the ones which are defined in intel_vdsc and in
this patch seem to match. If there are major differences there,
please point me to the exact case.
I remember that AMD driver might have different values.
Some values in the rc_params table do match. But the
rc_buf_thresh[] doesnt.
Because later they do:
vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
Vs
+static u16 dpu_dsc_rc_buf_thresh[DSC_NUM_BUF_RANGES - 1] = {
+ 0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54,
+ 0x62, 0x69, 0x70, 0x77, 0x79, 0x7b, 0x7d, 0x7e
+};
I'd prefer to have 896, 1792, etc. here, as those values come from the
standard. As it's done in the Intel driver.
Got it, thanks
I dont know the AMD calculation very well to say that moving this
to the
helper is going to help.
Those calculations correspond (more or less) at the first glance to
what intel does for their newer generations. I think that's not our
problem for now.
Well, we have to figure out if each value matches and if each of
them come from the spec for us and i915 and from which section. So
it is unfortunately our problem.
Otherwise it will have to be handled by Marijn, me or anybody else
wanting to hack up the DSC code. Or by anybody adding DSC support to
the next platform and having to figure out the difference between
i915, msm and their platform.
Yes, I wonder why the same doubt didn't arise when the other vendors
added their support both from other maintainers and others.
Which makes me think that like I wrote in my previous response, these
are "recommended" values in the spec but its not mandatory.
I think, it is because there were no other drivers to compare. In other
words, for a first driver it is pretty logical to have everything
handled on its own. As soon as we start getting other implementations of
a feature, it becomes logical to think if the code can be generalized.
This is what we see we with the HDCP series or with the code being moved
to DP helpers.
We were not the second, MSM was/is the third to add support for DSC afer
i915 and AMD. Thats what made me think why whoever was the second didnt
end up generalizing. Was it just missed out or was it intentionally left
in the vendor driver.
I didn't count AMD here, since it calculates some of the params rather
than using the fixed ones from the model.
Moving this to the drm_dsc_helper is generalizing the tables and not
giving room for the vendors to customize even if they want to (which
the spec does allow).
That depends on the API you select. For example, in
intel_dsc_compute_params() I see customization being applied to
rc_buf_thresh in 6bpp case. I'd leave that to the i915 driver.
Thanks for going through the i915 to figure out that the 6bpp is handled
in a customized
Re: [Intel-gfx] [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions
On 2/26/2023 5:09 AM, Dmitry Baryshkov wrote:
On 26/02/2023 02:47, Abhinav Kumar wrote:
Hi Dmitry
On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
On 25/02/2023 02:36, Abhinav Kumar wrote:
On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar
wrote:
On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar
пишет:
On 2/24/2023 1:13 PM, Dmitry Baryshkov wrote:
On 24/02/2023 21:40, Kuogee Hsieh wrote:
Add DSC helper functions based on DSC configuration profiles
to produce
DSC related runtime parameters through both table look up and
runtime
calculation to support DSC on DPU.
There are 6 different DSC configuration profiles are supported
currently.
DSC configuration profiles are differiented by 5 keys, DSC
version (V1.1),
chroma (444/422/420), colorspace (RGB/YUV), bpc(8/10),
bpp (6/7/7.5/8/9/10/12/15) and SCR (0/1).
Only DSC version V1.1 added and V1.2 will be added later.
These helpers should go to
drivers/gpu/drm/display/drm_dsc_helper.c
Also please check that they can be used for i915 or for amdgpu
(ideally for both of them).
No, it cannot. So each DSC encoder parameter is calculated based
on the HW core which is being used.
They all get packed to the same DSC structure which is the
struct drm_dsc_config but the way the parameters are computed is
specific to the HW.
This DPU file helper still uses the drm_dsc_helper's
drm_dsc_compute_rc_parameters() like all other vendors do but
the parameters themselves are very HW specific and belong to
each vendor's dir.
This is not unique to MSM.
Lets take a few other examples:
AMD:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
i915:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
I checked several values here. Intel driver defines more bpc/bpp
combinations, but the ones which are defined in intel_vdsc and in
this patch seem to match. If there are major differences there,
please point me to the exact case.
I remember that AMD driver might have different values.
Some values in the rc_params table do match. But the
rc_buf_thresh[] doesnt.
Because later they do:
vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
Vs
+static u16 dpu_dsc_rc_buf_thresh[DSC_NUM_BUF_RANGES - 1] = {
+ 0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54,
+ 0x62, 0x69, 0x70, 0x77, 0x79, 0x7b, 0x7d, 0x7e
+};
I'd prefer to have 896, 1792, etc. here, as those values come from the
standard. As it's done in the Intel driver.
Got it, thanks
I dont know the AMD calculation very well to say that moving this
to the
helper is going to help.
Those calculations correspond (more or less) at the first glance to
what intel does for their newer generations. I think that's not our
problem for now.
Well, we have to figure out if each value matches and if each of
them come from the spec for us and i915 and from which section. So
it is unfortunately our problem.
Otherwise it will have to be handled by Marijn, me or anybody else
wanting to hack up the DSC code. Or by anybody adding DSC support to
the next platform and having to figure out the difference between
i915, msm and their platform.
Yes, I wonder why the same doubt didn't arise when the other vendors
added their support both from other maintainers and others.
Which makes me think that like I wrote in my previous response, these
are "recommended" values in the spec but its not mandatory.
I think, it is because there were no other drivers to compare. In other
words, for a first driver it is pretty logical to have everything
handled on its own. As soon as we start getting other implementations of
a feature, it becomes logical to think if the code can be generalized.
This is what we see we with the HDCP series or with the code being moved
to DP helpers.
We were not the second, MSM was/is the third to add support for DSC afer
i915 and AMD. Thats what made me think why whoever was the second didnt
end up generalizing. Was it just missed out or was it intentionally left
in the vendor driver.
Moving this to the drm_dsc_helper is generalizing the tables and not
giving room for the vendors to customize even if they want to (which
the spec does allow).
That depends on the API you select. For example, in
intel_dsc_compute_params() I see customization being applied to
rc_buf_thresh in 6bpp case. I'd leave that to the i915 driver.
Thanks for going through the i915 to figure out that the 6bpp is handled
in a customized way. So what you are saying is let the helper first fill
up the recommended values of the spec, whatever is changed from that let
the vendor driver override that.
Th
Re: [Intel-gfx] [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions
Hi Dmitry
On 2/25/2023 7:23 AM, Dmitry Baryshkov wrote:
On 25/02/2023 02:36, Abhinav Kumar wrote:
On 2/24/2023 3:53 PM, Dmitry Baryshkov wrote:
On Sat, 25 Feb 2023 at 00:26, Abhinav Kumar
wrote:
On 2/24/2023 1:36 PM, Dmitry Baryshkov wrote:
24 февраля 2023 г. 23:23:03 GMT+02:00, Abhinav Kumar
пишет:
On 2/24/2023 1:13 PM, Dmitry Baryshkov wrote:
On 24/02/2023 21:40, Kuogee Hsieh wrote:
Add DSC helper functions based on DSC configuration profiles to
produce
DSC related runtime parameters through both table look up and
runtime
calculation to support DSC on DPU.
There are 6 different DSC configuration profiles are supported
currently.
DSC configuration profiles are differiented by 5 keys, DSC
version (V1.1),
chroma (444/422/420), colorspace (RGB/YUV), bpc(8/10),
bpp (6/7/7.5/8/9/10/12/15) and SCR (0/1).
Only DSC version V1.1 added and V1.2 will be added later.
These helpers should go to drivers/gpu/drm/display/drm_dsc_helper.c
Also please check that they can be used for i915 or for amdgpu
(ideally for both of them).
No, it cannot. So each DSC encoder parameter is calculated based
on the HW core which is being used.
They all get packed to the same DSC structure which is the struct
drm_dsc_config but the way the parameters are computed is specific
to the HW.
This DPU file helper still uses the drm_dsc_helper's
drm_dsc_compute_rc_parameters() like all other vendors do but the
parameters themselves are very HW specific and belong to each
vendor's dir.
This is not unique to MSM.
Lets take a few other examples:
AMD:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dml/dsc/rc_calc_fpu.c#L165
i915:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L379
I checked several values here. Intel driver defines more bpc/bpp
combinations, but the ones which are defined in intel_vdsc and in
this patch seem to match. If there are major differences there,
please point me to the exact case.
I remember that AMD driver might have different values.
Some values in the rc_params table do match. But the rc_buf_thresh[]
doesnt.
Because later they do:
vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L40
Vs
+static u16 dpu_dsc_rc_buf_thresh[DSC_NUM_BUF_RANGES - 1] = {
+ 0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54,
+ 0x62, 0x69, 0x70, 0x77, 0x79, 0x7b, 0x7d, 0x7e
+};
I'd prefer to have 896, 1792, etc. here, as those values come from the
standard. As it's done in the Intel driver.
Got it, thanks
I dont know the AMD calculation very well to say that moving this to
the
helper is going to help.
Those calculations correspond (more or less) at the first glance to
what intel does for their newer generations. I think that's not our
problem for now.
Well, we have to figure out if each value matches and if each of them
come from the spec for us and i915 and from which section. So it is
unfortunately our problem.
Otherwise it will have to be handled by Marijn, me or anybody else
wanting to hack up the DSC code. Or by anybody adding DSC support to the
next platform and having to figure out the difference between i915, msm
and their platform.
Yes, I wonder why the same doubt didn't arise when the other vendors
added their support both from other maintainers and others.
Which makes me think that like I wrote in my previous response, these
are "recommended" values in the spec but its not mandatory.
Moving this to the drm_dsc_helper is generalizing the tables and not
giving room for the vendors to customize even if they want to (which the
spec does allow).
So if this has any merit and if you or Marijn would like to take it up,
go for it. We would do the same thing as either of you would have to in
terms of figuring out the difference between msm and the i915 code.
This is not a generic API we are trying to put in a helper, these are
hard-coded tables so there is a difference between looking at these Vs
looking at some common code which can move to the core.
Also, i think its too risky to change other drivers to use whatever
math
we put in the drm_dsc_helper to compute thr RC params because their
code
might be computing and using this tables differently.
Its too much ownership for MSM developers to move this to
drm_dsc_helper
and own that as it might cause breakage of basic DSC even if some
values
are repeated.
It's time to stop thinking about ownership and start thinking about
shared code. We already have two instances of DSC tables. I don't
think having a third instance, which is a subset of an existing
dataset, would be beneficial to anybody.
AMD has complicated code which supports half-bit bpp and calculates
some of the parameters. But sharing data with the i915 driver i
Re: [Intel-gfx] [PATCH v6 09/10] arm64: dts: qcom: sc7180: Add support for HDCP in dp-controller
Hi Mark
On 1/18/2023 11:30 AM, Mark Yacoub wrote:
From: Sean Paul
This patch adds the register ranges required for HDCP key injection and
HDCP TrustZone interaction as described in the dt-bindings for the
sc7180 dp controller. Now that these are supported, change the
compatible string to "dp-hdcp".
Signed-off-by: Sean Paul
Signed-off-by: Mark Yacoub
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-15-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-14-s...@poorly.run
#v2
Link:
https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-14-s...@poorly.run
#v3
Link:
https://patchwork.freedesktop.org/patch/msgid/20211105030434.2828845-14-s...@poorly.run
#v4
Link:
https://patchwork.freedesktop.org/patch/msgid/20220411204741.1074308-10-s...@poorly.run
#v5
Changes in v3:
-Split off into a new patch containing just the dts change (Stephen)
-Add hdcp compatible string (Stephen)
Changes in v4:
-Rebase on Bjorn's multi-dp patchset
Changes in v5:
-Put the tz register offsets in trogdor dtsi (Rob C)
Changes in v6:
-Rebased: Removed modifications in sc7180.dtsi as it's already upstream
---
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 178efaaa89ec..6f6fe5cb6563 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -817,6 +817,14 @@ &mdss_dp {
pinctrl-names = "default";
pinctrl-0 = <&dp_hot_plug_det>;
data-lanes = <0 1>;
+
+ reg = <0 0x0ae9 0 0x200>,
+ <0 0x0ae90200 0 0x200>,
+ <0 0x0ae90400 0 0xc00>,
+ <0 0x0ae91000 0 0x400>,
+ <0 0x0ae91400 0 0x400>,
+ <0 0x0aed1000 0 0x175>,
+ <0 0x0aee1000 0 0x2c>;
};
Can you pls point me to which tree you rebased this on top of?
The mdss_dp node looks different here:
https://gitlab.com/linux-kernel/linux-next/-/blob/master/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi#L815
For the TZ regs the second entry is fine but any reason for the size of
the first register space to be 0x175 instead of 0x174?
&pm6150_adc {
Re: [Intel-gfx] [PATCH] drm/atomic-helpers: remove legacy_cursor_update hacks
Hi Daniel
On 4/8/2022 9:04 PM, Abhinav Kumar wrote:
On 4/7/2022 4:12 PM, Rob Clark wrote:
On Thu, Apr 7, 2022 at 3:59 PM Abhinav Kumar
wrote:
Hi Rob and Daniel
On 4/7/2022 3:51 PM, Rob Clark wrote:
On Wed, Apr 6, 2022 at 6:27 PM Jessica Zhang
wrote:
On 3/31/2022 8:20 AM, Daniel Vetter wrote:
The stuff never really worked, and leads to lots of fun because it
out-of-order frees atomic states. Which upsets KASAN, among other
things.
For async updates we now have a more solid solution with the
->atomic_async_check and ->atomic_async_commit hooks. Support for
that
for msm and vc4 landed. nouveau and i915 have their own commit
routines, doing something similar.
For everyone else it's probably better to remove the use-after-free
bug, and encourage folks to use the async support instead. The
affected drivers which register a legacy cursor plane and don't
either
use the new async stuff or their own commit routine are: amdgpu,
atmel, mediatek, qxl, rockchip, sti, sun4i, tegra, virtio, and
vmwgfx.
Inspired by an amdgpu bug report.
v2: Drop RFC, I think with amdgpu converted over to use
atomic_async_check/commit done in
commit 674e78acae0dfb4beb56132e41cbae5b60f7d662
Author: Nicholas Kazlauskas
Date: Wed Dec 5 14:59:07 2018 -0500
drm/amd/display: Add fast path for cursor plane updates
we don't have any driver anymore where we have userspace expecting
solid legacy cursor support _and_ they are using the atomic
helpers in
their fully glory. So we can retire this.
v3: Paper over msm and i915 regression. The complete_all is the only
thing missing afaict.
v4: Fixup i915 fixup ...
References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
References:
https://lore.kernel.org/all/20220221134155.125447-9-max...@cerno.tech/
References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
Cc: Maxime Ripard
Tested-by: Maxime Ripard
Cc: mikita.lip...@amd.com
Cc: Michel Dänzer
Cc: harry.wentl...@amd.com
Cc: Rob Clark
Hey Rob,
I saw your tested-by and reviewed-by tags on Patchwork. Just curious,
what device did you test on?
I was testing on strongbad.. v5.18-rc1 + patches (notably, revert
80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
I think the display setup shouldn't be significantly different than
limozeen (ie. it's an eDP panel). But I didn't do much start/stop
ui.. I was mostly looking to make sure cursor movements weren't
causing fps drops ;-)
BR,
-R
start ui/ stop ui is a basic operation for us to use IGT on msm-next.
So we cannot let that break.
I think we need to check whats causing this splat.
Can we hold back this change till then?
Can you reproduce on v5.18-rc1 (plus 80253168dbfd)? I'm running a
loop of stop ui / start ui and hasn't triggered a splat yet.
Otherwise maybe you can addr2line to figure out where it crashed?
BR,
-R
So this is not a crash. Its a warning splat coming from
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c#L785
Looks like the complete_commit() which should signal the event has not
happened before the next cursor commit.
Somehow this change is affecting the flow to miss the event signaling
that the event is done.
We tried a couple of approaches but couldnt still fix the warning.
Will continue to check further next week.
Thanks
Abhinav
After checking this more this week, I think the current patch needs to
be changed a bit.
So, here you are removing the complete_all part and leaving that to the
individual drivers, which is fine.
But, you are also removing the continue part which I think seems
incorrect and causing these warnings for MSM driver.
@@ -2135,12 +2128,6 @@ int drm_atomic_helper_setup_commit(struct
drm_atomic_state *state,
continue;
}
- /* Legacy cursor updates are fully unsynced. */
- if (state->legacy_cursor_update) {
- complete_all(&commit->flip_done);
- continue;
- }
-
Thats because MSM driver thinks that if the previous crtc_state->event
was not consumed, then something went wrong and throws a warning.
if (!new_crtc_state->event) {
commit->event = kzalloc(sizeof(*commit->event),
GFP_KERNEL);
if (!commit->event)
return -ENOMEM;
new_crtc_state->event = commit->event;
}
But for a cursor update, we should not or need not populate the event at
all because it is async.
So i think we should still keep the continue, rest of the patch is fine.
@@ -2128,6 +2128,9 @@ int drm_atomic_helper_setup_commit(struct
drm_atomic_state *state,
continue;
}
+ if (state->legacy_cursor_update)
+ continue;
+
Let me know your comments.
Thanks
Abhinav
I'm hitting several instances of this error wh
Re: [Intel-gfx] [PATCH] drm/atomic-helpers: remove legacy_cursor_update hacks
On 4/7/2022 4:12 PM, Rob Clark wrote:
On Thu, Apr 7, 2022 at 3:59 PM Abhinav Kumar wrote:
Hi Rob and Daniel
On 4/7/2022 3:51 PM, Rob Clark wrote:
On Wed, Apr 6, 2022 at 6:27 PM Jessica Zhang wrote:
On 3/31/2022 8:20 AM, Daniel Vetter wrote:
The stuff never really worked, and leads to lots of fun because it
out-of-order frees atomic states. Which upsets KASAN, among other
things.
For async updates we now have a more solid solution with the
->atomic_async_check and ->atomic_async_commit hooks. Support for that
for msm and vc4 landed. nouveau and i915 have their own commit
routines, doing something similar.
For everyone else it's probably better to remove the use-after-free
bug, and encourage folks to use the async support instead. The
affected drivers which register a legacy cursor plane and don't either
use the new async stuff or their own commit routine are: amdgpu,
atmel, mediatek, qxl, rockchip, sti, sun4i, tegra, virtio, and vmwgfx.
Inspired by an amdgpu bug report.
v2: Drop RFC, I think with amdgpu converted over to use
atomic_async_check/commit done in
commit 674e78acae0dfb4beb56132e41cbae5b60f7d662
Author: Nicholas Kazlauskas
Date: Wed Dec 5 14:59:07 2018 -0500
drm/amd/display: Add fast path for cursor plane updates
we don't have any driver anymore where we have userspace expecting
solid legacy cursor support _and_ they are using the atomic helpers in
their fully glory. So we can retire this.
v3: Paper over msm and i915 regression. The complete_all is the only
thing missing afaict.
v4: Fixup i915 fixup ...
References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
References:
https://lore.kernel.org/all/20220221134155.125447-9-max...@cerno.tech/
References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
Cc: Maxime Ripard
Tested-by: Maxime Ripard
Cc: mikita.lip...@amd.com
Cc: Michel Dänzer
Cc: harry.wentl...@amd.com
Cc: Rob Clark
Hey Rob,
I saw your tested-by and reviewed-by tags on Patchwork. Just curious,
what device did you test on?
I was testing on strongbad.. v5.18-rc1 + patches (notably, revert
80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
I think the display setup shouldn't be significantly different than
limozeen (ie. it's an eDP panel). But I didn't do much start/stop
ui.. I was mostly looking to make sure cursor movements weren't
causing fps drops ;-)
BR,
-R
start ui/ stop ui is a basic operation for us to use IGT on msm-next.
So we cannot let that break.
I think we need to check whats causing this splat.
Can we hold back this change till then?
Can you reproduce on v5.18-rc1 (plus 80253168dbfd)? I'm running a
loop of stop ui / start ui and hasn't triggered a splat yet.
Otherwise maybe you can addr2line to figure out where it crashed?
BR,
-R
So this is not a crash. Its a warning splat coming from
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c#L785
Looks like the complete_commit() which should signal the event has not
happened before the next cursor commit.
Somehow this change is affecting the flow to miss the event signaling
that the event is done.
We tried a couple of approaches but couldnt still fix the warning.
Will continue to check further next week.
Thanks
Abhinav
I'm hitting several instances of this error when doing a start/stop ui
on Lazor Chromebook with this patch:
[ 3092.608322] CPU: 2 PID: 18579 Comm: DrmThread Tainted: GW
5.17.0-rc2-lockdep-00089-g7f17ab7bf567 #155
e5912cd286513b064a82a38938b3fdef86b079aa
[ 3092.622880] Hardware name: Google Lazor Limozeen without Touchscreen
(rev4) (DT)
[ 3092.630492] pstate: 8049 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS
BTYPE=--)
[ 3092.637664] pc : dpu_crtc_atomic_flush+0x9c/0x144
[ 3092.642523] lr : dpu_crtc_atomic_flush+0x60/0x144
[ 3092.647379] sp : ffc00c1e3760
[ 3092.650805] x29: ffc00c1e3760 x28: ff80985dd800 x27:
0425
[ 3092.658164] x26: ff80985dc500 x25: ff80985ddc00 x24:
ffdf8ae3b6f0
[ 3092.665522] x23: x22: x21:
ff809b82da00
[ 3092.672890] x20: ff80840e1000 x19: ff80840e2000 x18:
1000
[ 3092.680255] x17: 0400 x16: 0100 x15:
003b
[ 3092.687622] x14: x13: 0002 x12:
0003
[ 3092.694979] x11: ff8084009000 x10: 0040 x9 :
0040
[ 3092.702340] x8 : 0300 x7 : 000c x6 :
0004
[ 3092.709698] x5 : 0320 x4 : 0018 x3 :
[ 3092.717056] x2 : x1 : 7bfb38b2a3a89800 x0 :
ff809a1eb300
[ 3092.724424] Call trace:
[ 3092.726958] dpu_crtc_atomic_flush+0x9c/0x144
[ 3092.731463] drm_atomic_helper_commit_planes+0x1bc/0x1c4
[ 3092.736944] msm_atomic_commit_tail+0x23c/0x3e0
[ 3092.741627] commit_tail+0x7c/0xfc
[ 3092.745145] drm_
Re: [Intel-gfx] [PATCH] drm/atomic-helpers: remove legacy_cursor_update hacks
Hi Rob and Daniel
On 4/7/2022 3:51 PM, Rob Clark wrote:
On Wed, Apr 6, 2022 at 6:27 PM Jessica Zhang wrote:
On 3/31/2022 8:20 AM, Daniel Vetter wrote:
The stuff never really worked, and leads to lots of fun because it
out-of-order frees atomic states. Which upsets KASAN, among other
things.
For async updates we now have a more solid solution with the
->atomic_async_check and ->atomic_async_commit hooks. Support for that
for msm and vc4 landed. nouveau and i915 have their own commit
routines, doing something similar.
For everyone else it's probably better to remove the use-after-free
bug, and encourage folks to use the async support instead. The
affected drivers which register a legacy cursor plane and don't either
use the new async stuff or their own commit routine are: amdgpu,
atmel, mediatek, qxl, rockchip, sti, sun4i, tegra, virtio, and vmwgfx.
Inspired by an amdgpu bug report.
v2: Drop RFC, I think with amdgpu converted over to use
atomic_async_check/commit done in
commit 674e78acae0dfb4beb56132e41cbae5b60f7d662
Author: Nicholas Kazlauskas
Date: Wed Dec 5 14:59:07 2018 -0500
drm/amd/display: Add fast path for cursor plane updates
we don't have any driver anymore where we have userspace expecting
solid legacy cursor support _and_ they are using the atomic helpers in
their fully glory. So we can retire this.
v3: Paper over msm and i915 regression. The complete_all is the only
thing missing afaict.
v4: Fixup i915 fixup ...
References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
References:
https://lore.kernel.org/all/20220221134155.125447-9-max...@cerno.tech/
References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
Cc: Maxime Ripard
Tested-by: Maxime Ripard
Cc: mikita.lip...@amd.com
Cc: Michel Dänzer
Cc: harry.wentl...@amd.com
Cc: Rob Clark
Hey Rob,
I saw your tested-by and reviewed-by tags on Patchwork. Just curious,
what device did you test on?
I was testing on strongbad.. v5.18-rc1 + patches (notably, revert
80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
I think the display setup shouldn't be significantly different than
limozeen (ie. it's an eDP panel). But I didn't do much start/stop
ui.. I was mostly looking to make sure cursor movements weren't
causing fps drops ;-)
BR,
-R
start ui/ stop ui is a basic operation for us to use IGT on msm-next.
So we cannot let that break.
I think we need to check whats causing this splat.
Can we hold back this change till then?
Thanks
Abhinav
I'm hitting several instances of this error when doing a start/stop ui
on Lazor Chromebook with this patch:
[ 3092.608322] CPU: 2 PID: 18579 Comm: DrmThread Tainted: GW
5.17.0-rc2-lockdep-00089-g7f17ab7bf567 #155
e5912cd286513b064a82a38938b3fdef86b079aa
[ 3092.622880] Hardware name: Google Lazor Limozeen without Touchscreen
(rev4) (DT)
[ 3092.630492] pstate: 8049 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS
BTYPE=--)
[ 3092.637664] pc : dpu_crtc_atomic_flush+0x9c/0x144
[ 3092.642523] lr : dpu_crtc_atomic_flush+0x60/0x144
[ 3092.647379] sp : ffc00c1e3760
[ 3092.650805] x29: ffc00c1e3760 x28: ff80985dd800 x27:
0425
[ 3092.658164] x26: ff80985dc500 x25: ff80985ddc00 x24:
ffdf8ae3b6f0
[ 3092.665522] x23: x22: x21:
ff809b82da00
[ 3092.672890] x20: ff80840e1000 x19: ff80840e2000 x18:
1000
[ 3092.680255] x17: 0400 x16: 0100 x15:
003b
[ 3092.687622] x14: x13: 0002 x12:
0003
[ 3092.694979] x11: ff8084009000 x10: 0040 x9 :
0040
[ 3092.702340] x8 : 0300 x7 : 000c x6 :
0004
[ 3092.709698] x5 : 0320 x4 : 0018 x3 :
[ 3092.717056] x2 : x1 : 7bfb38b2a3a89800 x0 :
ff809a1eb300
[ 3092.724424] Call trace:
[ 3092.726958] dpu_crtc_atomic_flush+0x9c/0x144
[ 3092.731463] drm_atomic_helper_commit_planes+0x1bc/0x1c4
[ 3092.736944] msm_atomic_commit_tail+0x23c/0x3e0
[ 3092.741627] commit_tail+0x7c/0xfc
[ 3092.745145] drm_atomic_helper_commit+0x158/0x15c
[ 3092.749998] drm_atomic_commit+0x60/0x74
[ 3092.754055] drm_atomic_helper_update_plane+0x100/0x110
[ 3092.759449] __setplane_atomic+0x11c/0x120
[ 3092.763685] drm_mode_cursor_universal+0x188/0x22c
[ 3092.768633] drm_mode_cursor_common+0x120/0x1f8
[ 3092.773310] drm_mode_cursor_ioctl+0x68/0x8c
[ 3092.21] drm_ioctl_kernel+0xe8/0x168
[ 3092.781770] drm_ioctl+0x320/0x370
[ 3092.785289] drm_compat_ioctl+0x40/0xdc
[ 3092.789257] __arm64_compat_sys_ioctl+0xe0/0x150
[ 3092.794030] invoke_syscall+0x80/0x114
[ 3092.797905] el0_svc_common.constprop.3+0xc4/0xf8
[ 3092.802765] do_el0_svc_compat+0x2c/0x54
[ 3092.806811] el0_svc_compat+0x4c/0xe4
[ 3092.810598] el0t_32_sync_handler+0xc4/0xf4
[ 3092.814914] el0t_32_sync+0x174/0x178
[ 3092.818701] irq event stamp: 55940
[ 3092.822217] hardirqs last enabled at (
Re: [Intel-gfx] [PATCH 00/22] drm: Review of mode copies
On 3/23/2022 3:39 AM, Dmitry Baryshkov wrote: On 22/03/2022 01:37, Ville Syrjälä wrote: On Tue, Mar 15, 2022 at 02:52:38PM -0400, Alex Deucher wrote: On Mon, Mar 14, 2022 at 6:12 PM Ville Syrjälä wrote: On Fri, Feb 18, 2022 at 12:03:41PM +0200, Ville Syrjala wrote: drm: Add drm_mode_init() drm/bridge: Use drm_mode_copy() drm/imx: Use drm_mode_duplicate() drm/panel: Use drm_mode_duplicate() drm/vc4: Use drm_mode_copy() These have been pushed to drm-misc-next. drm/amdgpu: Remove pointless on stack mode copies drm/amdgpu: Use drm_mode_init() for on-stack modes drm/amdgpu: Use drm_mode_copy() amdgpu ones are reviewed, but I'll leave them for the AMD folks to push to whichever tree they prefer. I pulled patches 2, 4, 5 into my tree. Thanks. For 3, I'm happy to have it land via drm-misc with the rest of the mode_init changes if you'd prefer. Either way works for me. I don't yet have reviews yet for the other drivers, so I'll proably hold off for a bit more at least. And the i915 patch I'll be merging via drm-intel. drm/radeon: Use drm_mode_copy() drm/gma500: Use drm_mode_copy() drm/tilcdc: Use drm_mode_copy() drm/i915: Use drm_mode_copy() Those are now all in. Which leaves us with these stragglers: drm/hisilicon: Use drm_mode_init() for on-stack modes drm/msm: Nuke weird on stack mode copy drm/msm: Use drm_mode_init() for on-stack modes drm/msm: Use drm_mode_copy() These three patches went beneath my radar for some reason. I have just sent a replacement for the first patch ([1]). Other two patches can be pulled in msm/msm-next (or msm/msm-fixes) during the next development cycle if that works for you. [1] https://patchwork.freedesktop.org/series/101682/ Agree with this approach. We can replace the first patch with https://patchwork.freedesktop.org/series/101682/. Thanks Abhinav drm/mtk: Use drm_mode_init() for on-stack modes drm/rockchip: Use drm_mode_copy() drm/sti: Use drm_mode_copy() drm: Use drm_mode_init() for on-stack modes drm: Use drm_mode_copy()
Re: [Intel-gfx] [PATCH 12/22] drm/msm: Use drm_mode_copy()
On 2/18/2022 2:03 AM, Ville Syrjala wrote:
From: Ville Syrjälä
struct drm_display_mode embeds a list head, so overwriting
the full struct with another one will corrupt the list
(if the destination mode is on a list). Use drm_mode_copy()
instead which explicitly preserves the list head of
the destination mode.
Even if we know the destination mode is not on any list
using drm_mode_copy() seems decent as it sets a good
example. Bad examples of not using it might eventually
get copied into code where preserving the list head
actually matters.
Obviously one case not covered here is when the mode
itself is embedded in a larger structure and the whole
structure is copied. But if we are careful when copying
into modes embedded in structures I think we can be a
little more reassured that bogus list heads haven't been
propagated in.
@is_mode_copy@
@@
drm_mode_copy(...)
{
...
}
@depends on !is_mode_copy@
struct drm_display_mode *mode;
expression E, S;
@@
(
- *mode = E
+ drm_mode_copy(mode, &E)
|
- memcpy(mode, E, S)
+ drm_mode_copy(mode, E)
)
@depends on !is_mode_copy@
struct drm_display_mode mode;
expression E;
@@
(
- mode = E
+ drm_mode_copy(&mode, &E)
|
- memcpy(&mode, E, S)
+ drm_mode_copy(&mode, E)
)
@@
struct drm_display_mode *mode;
@@
- &*mode
+ mode
Cc: Rob Clark
Cc: Sean Paul
Cc: Abhinav Kumar
Cc: linux-arm-...@vger.kernel.org
Cc: freedr...@lists.freedesktop.org
Signed-off-by: Ville Syrjälä
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 34a6940d12c5..57592052af23 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -157,7 +157,7 @@ static void dpu_encoder_phys_cmd_mode_set(
DPU_ERROR("invalid args\n");
return;
}
- phys_enc->cached_mode = *adj_mode;
+ drm_mode_copy(&phys_enc->cached_mode, adj_mode);
DPU_DEBUG_CMDENC(cmd_enc, "caching mode:\n");
drm_mode_debug_printmodeline(adj_mode);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index e7813c6f7bd9..d5deca07b65a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -370,7 +370,7 @@ static void dpu_encoder_phys_vid_mode_set(
struct dpu_encoder_irq *irq;
if (adj_mode) {
- phys_enc->cached_mode = *adj_mode;
+ drm_mode_copy(&phys_enc->cached_mode, adj_mode);
drm_mode_debug_printmodeline(adj_mode);
DPU_DEBUG_VIDENC(phys_enc, "caching mode:\n");
}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 7cc4d21f2091..2ed6028ca8d6 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -825,7 +825,7 @@ static int dp_display_set_mode(struct msm_dp *dp_display,
dp = container_of(dp_display, struct dp_display_private, dp_display);
- dp->panel->dp_mode.drm_mode = mode->drm_mode;
+ drm_mode_copy(&dp->panel->dp_mode.drm_mode, &mode->drm_mode);
dp->panel->dp_mode.bpp = mode->bpp;
dp->panel->dp_mode.capabilities = mode->capabilities;
dp_panel_init_panel_info(dp->panel);
Re: [Intel-gfx] [PATCH 11/22] drm/msm: Use drm_mode_init() for on-stack modes
On 2/18/2022 2:03 AM, Ville Syrjala wrote:
From: Ville Syrjälä
Initialize on-stack modes with drm_mode_init() to guarantee
no stack garbage in the list head, or that we aren't copying
over another mode's list head.
Based on the following cocci script, with manual fixups:
@decl@
identifier M;
expression E;
@@
- struct drm_display_mode M = E;
+ struct drm_display_mode M;
@@
identifier decl.M;
expression decl.E;
statement S, S1;
@@
struct drm_display_mode M;
... when != S
+ drm_mode_init(&M, &E);
+
S1
@@
expression decl.E;
@@
- &*E
+ E
Cc: Rob Clark
Cc: Sean Paul
Cc: Abhinav Kumar
Cc: linux-arm-...@vger.kernel.org
Cc: freedr...@lists.freedesktop.org
Signed-off-by: Ville Syrjälä
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index ddd9d89cd456..e7813c6f7bd9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -248,12 +248,13 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
unsigned long lock_flags;
struct dpu_hw_intf_cfg intf_cfg = { 0 };
+ drm_mode_init(&mode, &phys_enc->cached_mode);
+
if (!phys_enc->hw_ctl->ops.setup_intf_cfg) {
DPU_ERROR("invalid encoder %d\n", phys_enc != NULL);
return;
}
- mode = phys_enc->cached_mode;
if (!phys_enc->hw_intf->ops.setup_timing_gen) {
DPU_ERROR("timing engine setup is not supported\n");
return;
@@ -652,7 +653,9 @@ static int dpu_encoder_phys_vid_get_frame_count(
{
struct intf_status s = {0};
u32 fetch_start = 0;
- struct drm_display_mode mode = phys_enc->cached_mode;
+ struct drm_display_mode mode;
+
+ drm_mode_init(&mode, &phys_enc->cached_mode);
if (!dpu_encoder_phys_vid_is_master(phys_enc))
return -EINVAL;
Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes
Hi Suraj On 3/8/2022 6:30 AM, Kandpal, Suraj wrote: Hi Abhinav, Hi, Hi, Hi Abhinav, Hi Laurent Ok sure, I can take this up but I need to understand the proposal a little bit more before proceeding on this. So we will discuss this in another email where we specifically talk about the connector changes. Also, I am willing to take this up once the encoder part is done and merged so that atleast we keep moving on that as MSM WB implementation can proceed with that first. Hi Jani and Suraj Any concerns with splitting up the series into encoder and connector OR re- arranging them? Let me know if you can do this OR I can also split this up keeping Suraj's name in the Co-developed by tag. I was actually thinking of floating a new patch series with both encoder and connector pointer changes but with a change in initialization functions wherein we allocate a connector and encoder incase the driver doesn’t have its own this should minimize the effect on other drivers already using current drm writeback framework and also allow i915 to create its own connector. I was proposing to split up the encoder and connector because the comments from Laurent meant we were in agreement about the encoder but not about the connector. I am afraid another attempt with the similar idea is only going to stall the series again and hence i gave this option. Seems like this patch series currently won't be able to move forward with the connector pointer change so I guess you can split the series to encoder pointer change where we optionally create encoder if required ,keeping my name in co-developed tag so that msm can move forward with its implementation and then we can work to see how the connector issue can be bypassed. Best Regards, Suraj Kandpal Thanks, let me re-spin this and will keep your name as co-developer. Should be able to get it on the list in a week. Thanks Abhinav Eventually its upto Laurent and the other maintainers to guide us forward on this as this series has been stalled for weeks now. I thought that Laurent was quite strict about the connector. So I'd suggest targeting drm_writeback_connector with an optionally created encoder and the builtin connector In that case even if we target that i915 will not be able to move forward with its implementation of writeback as builtin connector does not work for us right now as explained earlier, optionally creating encoder and connector will help everyone move forward and once done we can actually sit and work on how to side step this issue using Laurent's suggestion This is up to Laurent & other DRM maintainers to decide whether this approach would be accepted or not. So far unfortunately I have mostly seen the pushback and a strong suggestion to stop treating each drm_connector as i915_connector. I haven't checked the exact details, but IMO adding a branch if the connector's type is DRM_CONNECTOR_VIRTUAL should be doable. Bringing in the change where we don’t treat each drm_connector as an intel_connector or even adding a branch which checks if drm_connector is DRM_CONNECTOR_VIRTUAL and conditionally cast it to intel_connector is quite a lot of work for i915. That's why I was suggesting if for now we could move forward with optionally creating both encoder and connector minimizing affects on other drivers as well as allowing us to move forward. What do you think, Launrent? We can work on Laurent's suggestion but that would first require the initial RFC patches and then a rework from both of our drivers side to see if they gel well with our current code which will take a considerable amount of time. We can for now however float the patch series up which minimally affects the current drivers and also allows i915 and msm to move forward with its writeback implementation off course with a proper documentation stating new drivers shouldn't try to make their own connectors and encoders and that drm_writeback will do that for them. Once that's done and merged we can work with Laurent on his proposal to improve the drm writeback framework so that this issue can be side stepped entirely in future. For now I would like to keep the encoder and connector pointer changes together.
Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes
Hi Suraj On 3/4/2022 6:16 AM, Kandpal, Suraj wrote: Hi, Hi, Hi Abhinav, Hi Laurent Ok sure, I can take this up but I need to understand the proposal a little bit more before proceeding on this. So we will discuss this in another email where we specifically talk about the connector changes. Also, I am willing to take this up once the encoder part is done and merged so that atleast we keep moving on that as MSM WB implementation can proceed with that first. Hi Jani and Suraj Any concerns with splitting up the series into encoder and connector OR re- arranging them? Let me know if you can do this OR I can also split this up keeping Suraj's name in the Co-developed by tag. I was actually thinking of floating a new patch series with both encoder and connector pointer changes but with a change in initialization functions wherein we allocate a connector and encoder incase the driver doesn’t have its own this should minimize the effect on other drivers already using current drm writeback framework and also allow i915 to create its own connector. I was proposing to split up the encoder and connector because the comments from Laurent meant we were in agreement about the encoder but not about the connector. I am afraid another attempt with the similar idea is only going to stall the series again and hence i gave this option. Eventually its upto Laurent and the other maintainers to guide us forward on this as this series has been stalled for weeks now. I thought that Laurent was quite strict about the connector. So I'd suggest targeting drm_writeback_connector with an optionally created encoder and the builtin connector In that case even if we target that i915 will not be able to move forward with its implementation of writeback as builtin connector does not work for us right now as explained earlier, optionally creating encoder and connector will help everyone move forward and once done we can actually sit and work on how to side step this issue using Laurent's suggestion This is up to Laurent & other DRM maintainers to decide whether this approach would be accepted or not. So far unfortunately I have mostly seen the pushback and a strong suggestion to stop treating each drm_connector as i915_connector. I haven't checked the exact details, but IMO adding a branch if the connector's type is DRM_CONNECTOR_VIRTUAL should be doable. Bringing in the change where we don’t treat each drm_connector as an intel_connector or even adding a branch which checks if drm_connector is DRM_CONNECTOR_VIRTUAL and conditionally cast it to intel_connector is quite a lot of work for i915. That's why I was suggesting if for now we could move forward with optionally creating both encoder and connector minimizing affects on other drivers as well as allowing us to move forward. What do you think, Launrent? We can work on Laurent's suggestion but that would first require the initial RFC patches and then a rework from both of our drivers side to see if they gel well with our current code which will take a considerable amount of time. We can for now however float the patch series up which minimally affects the current drivers and also allows i915 and msm to move forward with its writeback implementation off course with a proper documentation stating new drivers shouldn't try to make their own connectors and encoders and that drm_writeback will do that for them. Once that's done and merged we can work with Laurent on his proposal to improve the drm writeback framework so that this issue can be side stepped entirely in future. For now I would like to keep the encoder and connector pointer changes together. -- With best wishes Dmitry Best Regards, Suraj Kandpal
Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes
On 3/2/2022 10:31 AM, Laurent Pinchart wrote:
Hi Abhinav,
On Wed, Mar 02, 2022 at 10:28:03AM -0800, Abhinav Kumar wrote:
On 2/28/2022 5:42 AM, Laurent Pinchart wrote:
On Mon, Feb 28, 2022 at 02:28:27PM +0200, Laurent Pinchart wrote:
On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote:
On Mon, 28 Feb 2022, Laurent Pinchart wrote:
On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote:
On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote:
On Wed, 02 Feb 2022, Laurent Pinchart wrote:
On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
On Wed, 02 Feb 2022, Laurent Pinchart wrote:
On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
Changing rcar_du driver to accomadate the change of
drm_writeback_connector.base and drm_writeback_connector.encoder
to a pointer the reason for which is explained in the
Patch(drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal Suraj
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++
drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 66e8839db708..68f387a04502 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -72,6 +72,8 @@ struct rcar_du_crtc {
const char *const *sources;
unsigned int sources_count;
+ struct drm_connector connector;
+ struct drm_encoder encoder;
Those fields are, at best, poorly named. Furthermore, there's no need in
this driver or in other drivers using drm_writeback_connector to create
an encoder or connector manually. Let's not polute all drivers because
i915 doesn't have its abstractions right.
i915 uses the quite common model for struct inheritance:
struct intel_connector {
struct drm_connector base;
/* ... */
}
Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
radeon, tilcdc, and vboxvideo.
We could argue about the relative merits of that abstraction, but I
think the bottom line is that it's popular and the drivers using it are
not going to be persuaded to move away from it.
Nobody said inheritance is bad.
It's no coincidence that the drivers who've implemented writeback so far
(komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
because the drm_writeback_connector midlayer does, forcing the issue.
Are you sure it's not a coincidence ? :-)
The encoder and especially connector created by drm_writeback_connector
are there only because KMS requires a drm_encoder and a drm_connector to
be exposed to userspace (and I could argue that using a connector for
writeback is a hack, but that won't change). The connector is "virtual",
I still fail to see why i915 or any other driver would need to wrap it
into something else. The whole point of the drm_writeback_connector
abstraction is that drivers do not have to manage the writeback
drm_connector manually, they shouldn't touch it at all.
The thing is, drm_writeback_connector_init() calling
drm_connector_init() on the drm_connector embedded in
drm_writeback_connector leads to that connector being added to the
drm_device's list of connectors. Ditto for the encoder.
All the driver code that handles drm_connectors would need to take into
account they might not be embedded in intel_connector. Throughout the
driver. Ditto for the encoders.
The assumption that a connector is embedded in intel_connector doesn't
really play that well with how bridge and panel connectors work.. so
in general this seems like a good thing to unwind.
But as a point of practicality, i915 is a large driver covering a lot
of generations of hw with a lot of users. So I can understand
changing this design isn't something that can happen quickly or
easily. IMO we should allow i915 to create it's own connector for
writeback, and just document clearly that this isn't the approach new
drivers should take. I mean, I understand idealism, but sometimes a
dose of pragmatism is needed. :-)
i915 is big, but so is Intel. It's not fair to treat everybody else as a
second class citizen and let Intel get away without doing its homework.
Laurent, as you accuse us of not doing our homework, I'll point out that
we've been embedding drm crtc, encoder and connector ever since
modesetting support was added to i915 in 2008, since before *any* of the
things you now use as a rationale for asking us to do a massive rewrite
of the driver existed.
It's been ok to embed those structures for well over ten years. It's a
common pattern, basically throughout the kernel. Other drivers do it
too, not just i915. There hasn't been the slightest hint this should not
be done until this very conversation.
I want to see this refactoring effort moving forward in i915 (and moving
to
Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes
On 2/28/2022 5:42 AM, Laurent Pinchart wrote:
Hello,
On Mon, Feb 28, 2022 at 02:28:27PM +0200, Laurent Pinchart wrote:
On Mon, Feb 28, 2022 at 02:09:15PM +0200, Jani Nikula wrote:
On Mon, 28 Feb 2022, Laurent Pinchart wrote:
On Sat, Feb 26, 2022 at 10:27:59AM -0800, Rob Clark wrote:
On Wed, Feb 2, 2022 at 7:41 AM Jani Nikula wrote:
On Wed, 02 Feb 2022, Laurent Pinchart wrote:
On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
On Wed, 02 Feb 2022, Laurent Pinchart wrote:
On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
Changing rcar_du driver to accomadate the change of
drm_writeback_connector.base and drm_writeback_connector.encoder
to a pointer the reason for which is explained in the
Patch(drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal Suraj
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++
drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 66e8839db708..68f387a04502 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -72,6 +72,8 @@ struct rcar_du_crtc {
const char *const *sources;
unsigned int sources_count;
+ struct drm_connector connector;
+ struct drm_encoder encoder;
Those fields are, at best, poorly named. Furthermore, there's no need in
this driver or in other drivers using drm_writeback_connector to create
an encoder or connector manually. Let's not polute all drivers because
i915 doesn't have its abstractions right.
i915 uses the quite common model for struct inheritance:
struct intel_connector {
struct drm_connector base;
/* ... */
}
Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
radeon, tilcdc, and vboxvideo.
We could argue about the relative merits of that abstraction, but I
think the bottom line is that it's popular and the drivers using it are
not going to be persuaded to move away from it.
Nobody said inheritance is bad.
It's no coincidence that the drivers who've implemented writeback so far
(komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
because the drm_writeback_connector midlayer does, forcing the issue.
Are you sure it's not a coincidence ? :-)
The encoder and especially connector created by drm_writeback_connector
are there only because KMS requires a drm_encoder and a drm_connector to
be exposed to userspace (and I could argue that using a connector for
writeback is a hack, but that won't change). The connector is "virtual",
I still fail to see why i915 or any other driver would need to wrap it
into something else. The whole point of the drm_writeback_connector
abstraction is that drivers do not have to manage the writeback
drm_connector manually, they shouldn't touch it at all.
The thing is, drm_writeback_connector_init() calling
drm_connector_init() on the drm_connector embedded in
drm_writeback_connector leads to that connector being added to the
drm_device's list of connectors. Ditto for the encoder.
All the driver code that handles drm_connectors would need to take into
account they might not be embedded in intel_connector. Throughout the
driver. Ditto for the encoders.
The assumption that a connector is embedded in intel_connector doesn't
really play that well with how bridge and panel connectors work.. so
in general this seems like a good thing to unwind.
But as a point of practicality, i915 is a large driver covering a lot
of generations of hw with a lot of users. So I can understand
changing this design isn't something that can happen quickly or
easily. IMO we should allow i915 to create it's own connector for
writeback, and just document clearly that this isn't the approach new
drivers should take. I mean, I understand idealism, but sometimes a
dose of pragmatism is needed. :-)
i915 is big, but so is Intel. It's not fair to treat everybody else as a
second class citizen and let Intel get away without doing its homework.
Laurent, as you accuse us of not doing our homework, I'll point out that
we've been embedding drm crtc, encoder and connector ever since
modesetting support was added to i915 in 2008, since before *any* of the
things you now use as a rationale for asking us to do a massive rewrite
of the driver existed.
It's been ok to embed those structures for well over ten years. It's a
common pattern, basically throughout the kernel. Other drivers do it
too, not just i915. There hasn't been the slightest hint this should not
be done until this very conversation.
I want to see this refactoring effort moving forward in i915 (and moving
to drm_bridge would then be a good idea too). If writeback support in
i915 urgent, then we can discuss *temporary* pragmatic stopgap measures,
but not without a real effort to fix the core issue.
I think the onus is on you to first convince everyo
Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes
Hi Suraj
On 2/22/2022 10:17 PM, Kandpal, Suraj wrote:
Hey,
The connector/encoder funcs you do have to pass to
drm_writeback_connector_init() can't use any of the shared driver
infrastructure that assume a certain inheritance.
See also my reply to Laurent [1].
It well might be that we all misunderstand your design. Do you have a
complete intel drm_writeback implementation based on this patchset? It
would be easier to judge if the approach is correct seeing your
target.
That would be up to Suraj Kandpal.
I have floated the intel drm_writeback implementation.
Please refer to [1] to get a better understanding of how we are implementing
writeback functionality.
[1] https://patchwork.freedesktop.org/series/100617/
Thanks,
Suraj Kandpal
Based on the discussion in this thread [1] , it seems like having
drm_encoder as a pointer seems to have merits for both of us and also in
agreement with the folks on this thread and has a better chance of an ack.
However drm_connector is not.
I had a brief look at your implementation. Any reason you need to go
through intel_connector here and not drm_writeback_connector directly?
The reason I ask is that I see you are not using prepare_writeback_job
hook. If you use that, you can use the drm_writeback_connector passed on
from there instead of looping through your list like you are doing in
intel_find_writeback_connector.
Also, none of the other entries of struct intel_connector are being used
for the writeback implementation. So does the drm_writeback_connector in
your implementation need to be an intel_connector when its anyway not
using other fields? Why cant it be just stored as a
drm_writeback_connector itself in your struct intel_wd.
@@ -539,6 +540,8 @@ struct intel_connector {
struct work_struct modeset_retry_work;
struct intel_hdcp hdcp;
+
+ struct drm_writeback_connector wb_conn;
};
[1]
https://patchwork.kernel.org/project/dri-devel/patch/20220202085429.22261-6-suraj.kand...@intel.com/#24747889
If you are in agreement with this, do you think you can re-spin the
series only with the drm_encoder as a pointer without the drm_connector
part.
Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes
Hi Laurent
Thanks for responding.
On 2/21/2022 11:34 PM, Laurent Pinchart wrote:
Hi Dmitry,
On Tue, Feb 22, 2022 at 06:32:50AM +0300, Dmitry Baryshkov wrote:
On Thu, 10 Feb 2022 at 07:59, Laurent Pinchart wrote:
On Wed, Feb 09, 2022 at 05:40:29PM -0800, Abhinav Kumar wrote:
Hi Laurent
Gentle reminder on this.
I won't have time before next week I'm afraid.
Laurent, another gentle ping.
I'm really late on this so I probably deserve a bit of a rougher ping,
but thanks for being gentle :-)
On 2/6/2022 11:20 PM, Abhinav Kumar wrote:
On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote:
On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart wrote:
On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
On Wed, 02 Feb 2022, Laurent Pinchart wrote:
On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
Changing rcar_du driver to accomadate the change of
drm_writeback_connector.base and drm_writeback_connector.encoder
to a pointer the reason for which is explained in the
Patch(drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal Suraj
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++
drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 66e8839db708..68f387a04502 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -72,6 +72,8 @@ struct rcar_du_crtc {
const char *const *sources;
unsigned int sources_count;
+ struct drm_connector connector;
+ struct drm_encoder encoder;
Those fields are, at best, poorly named. Furthermore, there's no need in
this driver or in other drivers using drm_writeback_connector to create
an encoder or connector manually. Let's not polute all drivers because
i915 doesn't have its abstractions right.
i915 uses the quite common model for struct inheritance:
struct intel_connector {
struct drm_connector base;
/* ... */
}
Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
radeon, tilcdc, and vboxvideo.
We could argue about the relative merits of that abstraction, but I
think the bottom line is that it's popular and the drivers using it are
not going to be persuaded to move away from it.
Nobody said inheritance is bad.
It's no coincidence that the drivers who've implemented writeback so far
(komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
because the drm_writeback_connector midlayer does, forcing the issue.
Are you sure it's not a coincidence ? :-)
The encoder and especially connector created by drm_writeback_connector
are there only because KMS requires a drm_encoder and a drm_connector to
be exposed to userspace (and I could argue that using a connector for
writeback is a hack, but that won't change). The connector is "virtual",
I still fail to see why i915 or any other driver would need to wrap it
into something else. The whole point of the drm_writeback_connector
abstraction is that drivers do not have to manage the writeback
drm_connector manually, they shouldn't touch it at all.
Laurent, I wanted to shift a bit from the question of drm_connector to
the question of drm_encoder being embedded in the drm_writeback_connector.
In case of the msm driver the drm_encoder is not a lightweight entity,
but a full-featured driver part. Significant part of it can be shared
with the writeback implementation, if we allow using a pointer to the
external drm_encoder with the drm_writeback_connector.
Does the following patch set stand a chance to receive your ack?
- Switch drm_writeback_connector to point to drm_encoder rather than
embedding it?
- Create drm_encoder for the drm_writeback_connector when one is not
specified, so the current drivers can be left unchanged.
The situation is a bit different for the encoder indeed.
The encoder concept is loosely defined nowadays, with more and more of
the "real" encoders being implemented as a drm_bridge. That's what I
usually recommend when reviewing new drivers. drm_encoder is slowly
becoming an empty shell (see for instance [1]), although that transition
is not enforced globally and will thus take a long time to complete (if
ever).
This being said, lots of drivers have "featureful" encoder
implementations, and that won't go away any time soon. In those cases, I
could be OK with drivers optionally passing an encoder fo the writeback
helper if the hardware really shares resources between writeback and a
real encoder. I would however be careful there, as in many cases I would
expect the need to pass a custom encoder to originate from an old
software design decision rather than from the hardware architecture. In
those cases it would be best, I think, to move towards cleaning up the
software
Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes
Hi Laurent
Gentle reminder on this.
Thanks
Abhinav
On 2/6/2022 11:20 PM, Abhinav Kumar wrote:
Hi Laurent
On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote:
On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart
wrote:
Hi Jani,
On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
On Wed, 02 Feb 2022, Laurent Pinchart wrote:
On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
Changing rcar_du driver to accomadate the change of
drm_writeback_connector.base and drm_writeback_connector.encoder
to a pointer the reason for which is explained in the
Patch(drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal Suraj
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++
drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 66e8839db708..68f387a04502 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -72,6 +72,8 @@ struct rcar_du_crtc {
const char *const *sources;
unsigned int sources_count;
+ struct drm_connector connector;
+ struct drm_encoder encoder;
Those fields are, at best, poorly named. Furthermore, there's no
need in
this driver or in other drivers using drm_writeback_connector to
create
an encoder or connector manually. Let's not polute all drivers because
i915 doesn't have its abstractions right.
i915 uses the quite common model for struct inheritance:
struct intel_connector {
struct drm_connector base;
/* ... */
}
Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
radeon, tilcdc, and vboxvideo.
We could argue about the relative merits of that abstraction, but I
think the bottom line is that it's popular and the drivers using it are
not going to be persuaded to move away from it.
Nobody said inheritance is bad.
It's no coincidence that the drivers who've implemented writeback so
far
(komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
because the drm_writeback_connector midlayer does, forcing the issue.
Are you sure it's not a coincidence ? :-)
The encoder and especially connector created by drm_writeback_connector
are there only because KMS requires a drm_encoder and a drm_connector to
be exposed to userspace (and I could argue that using a connector for
writeback is a hack, but that won't change). The connector is "virtual",
I still fail to see why i915 or any other driver would need to wrap it
into something else. The whole point of the drm_writeback_connector
abstraction is that drivers do not have to manage the writeback
drm_connector manually, they shouldn't touch it at all.
Laurent, I wanted to shift a bit from the question of drm_connector to
the question of drm_encoder being embedded in the
drm_writeback_connector.
In case of the msm driver the drm_encoder is not a lightweight entity,
but a full-featured driver part. Significant part of it can be shared
with the writeback implementation, if we allow using a pointer to the
external drm_encoder with the drm_writeback_connector.
Does the following patch set stand a chance to receive your ack?
- Switch drm_writeback_connector to point to drm_encoder rather than
embedding it?
- Create drm_encoder for the drm_writeback_connector when one is not
specified, so the current drivers can be left unchanged.
I second Dmitry's request here. For the reasons he has mentioned along
with the possibility of the writeback encoder being shared across
display pipelines, strengthens our request of the drm encoder being a
pointer inside the drm_writeback_connector instead of embedding it.
Like I had shown in my RFC, in case the other drivers dont specify one,
we can allocate one:
https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhin...@quicinc.com/
We think this should be a reasonable accomodation to the existing
drm_writeback driver.
Thanks
Abhinav
So I think drm_writeback_connector should *not* use the inheritance
abstraction because it's a midlayer that should leave that option to
the
drivers. I think drm_writeback_connector needs to be changed to
accommodate that, and, unfortunately, it means current writeback users
need to be changed as well.
I am not sure, however, if the series at hand is the right
approach. Perhaps writeback can be modified to allocate the stuff for
you if you prefer it that way, as long as the drm_connector is not
embedded in struct drm_writeback_connector.
Nack.
struct drm_writeback_connector writeback;
};
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
index c79d1259e49b..5b1e83380c47 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
@@ -2
Re: [Intel-gfx] [PATCH 5/6] drm/rcar_du: changes to rcar-du driver resulting from drm_writeback_connector structure changes
Hi Laurent
On 2/6/2022 3:32 PM, Dmitry Baryshkov wrote:
On Wed, 2 Feb 2022 at 16:26, Laurent Pinchart
wrote:
Hi Jani,
On Wed, Feb 02, 2022 at 03:15:03PM +0200, Jani Nikula wrote:
On Wed, 02 Feb 2022, Laurent Pinchart wrote:
On Wed, Feb 02, 2022 at 02:24:28PM +0530, Kandpal Suraj wrote:
Changing rcar_du driver to accomadate the change of
drm_writeback_connector.base and drm_writeback_connector.encoder
to a pointer the reason for which is explained in the
Patch(drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal Suraj
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++
drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 8 +---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 66e8839db708..68f387a04502 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -72,6 +72,8 @@ struct rcar_du_crtc {
const char *const *sources;
unsigned int sources_count;
+ struct drm_connector connector;
+ struct drm_encoder encoder;
Those fields are, at best, poorly named. Furthermore, there's no need in
this driver or in other drivers using drm_writeback_connector to create
an encoder or connector manually. Let's not polute all drivers because
i915 doesn't have its abstractions right.
i915 uses the quite common model for struct inheritance:
struct intel_connector {
struct drm_connector base;
/* ... */
}
Same with at least amd, ast, fsl-dcu, hisilicon, mga200, msm, nouveau,
radeon, tilcdc, and vboxvideo.
We could argue about the relative merits of that abstraction, but I
think the bottom line is that it's popular and the drivers using it are
not going to be persuaded to move away from it.
Nobody said inheritance is bad.
It's no coincidence that the drivers who've implemented writeback so far
(komeda, mali, rcar-du, vc4, and vkms) do not use the abstraction,
because the drm_writeback_connector midlayer does, forcing the issue.
Are you sure it's not a coincidence ? :-)
The encoder and especially connector created by drm_writeback_connector
are there only because KMS requires a drm_encoder and a drm_connector to
be exposed to userspace (and I could argue that using a connector for
writeback is a hack, but that won't change). The connector is "virtual",
I still fail to see why i915 or any other driver would need to wrap it
into something else. The whole point of the drm_writeback_connector
abstraction is that drivers do not have to manage the writeback
drm_connector manually, they shouldn't touch it at all.
Laurent, I wanted to shift a bit from the question of drm_connector to
the question of drm_encoder being embedded in the
drm_writeback_connector.
In case of the msm driver the drm_encoder is not a lightweight entity,
but a full-featured driver part. Significant part of it can be shared
with the writeback implementation, if we allow using a pointer to the
external drm_encoder with the drm_writeback_connector.
Does the following patch set stand a chance to receive your ack?
- Switch drm_writeback_connector to point to drm_encoder rather than
embedding it?
- Create drm_encoder for the drm_writeback_connector when one is not
specified, so the current drivers can be left unchanged.
I second Dmitry's request here. For the reasons he has mentioned along
with the possibility of the writeback encoder being shared across
display pipelines, strengthens our request of the drm encoder being a
pointer inside the drm_writeback_connector instead of embedding it.
Like I had shown in my RFC, in case the other drivers dont specify one,
we can allocate one:
https://patchwork.kernel.org/project/dri-devel/patch/1642732195-25349-1-git-send-email-quic_abhin...@quicinc.com/
We think this should be a reasonable accomodation to the existing
drm_writeback driver.
Thanks
Abhinav
So I think drm_writeback_connector should *not* use the inheritance
abstraction because it's a midlayer that should leave that option to the
drivers. I think drm_writeback_connector needs to be changed to
accommodate that, and, unfortunately, it means current writeback users
need to be changed as well.
I am not sure, however, if the series at hand is the right
approach. Perhaps writeback can be modified to allocate the stuff for
you if you prefer it that way, as long as the drm_connector is not
embedded in struct drm_writeback_connector.
Nack.
struct drm_writeback_connector writeback;
};
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
index c79d1259e49b..5b1e83380c47 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
@@ -200,8 +200,10 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
{
struct drm_writeback_connector *wb_conn = &rcrtc->writeback;
- wb_conn->encoder.possible_crtcs
Re: [Intel-gfx] [PATCH 1/3] drm: add writeback pointers to drm_connector
Hi Suraj
I think there are more places affected with this change. I can get below
compilation issues while trying to compile my branch:
drivers/gpu/drm/vc4/vc4_txp.c: In function ‘encoder_to_vc4_txp’:
./include/linux/build_bug.h:78:41: error: static assertion failed:
"pointer type mismatch in container_of()"
#define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
^
./include/linux/build_bug.h:77:34: note: in expansion of macro
‘__static_assert’
#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__,
#expr)
^
./include/linux/container_of.h:19:2: note: in expansion of macro
‘static_assert’
static_assert(__same_type(*(ptr), ((type *)0)->member) || \
^
drivers/gpu/drm/vc4/vc4_txp.c:162:9: note: in expansion of macro
‘container_of’
return container_of(encoder, struct vc4_txp, connector.encoder);
^
drivers/gpu/drm/vc4/vc4_txp.c: In function ‘connector_to_vc4_txp’:
./include/linux/build_bug.h:78:41: error: static assertion failed:
"pointer type mismatch in container_of()"
#define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
^
./include/linux/build_bug.h:77:34: note: in expansion of macro
‘__static_assert’
#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__,
#expr)
^
./include/linux/container_of.h:19:2: note: in expansion of macro
‘static_assert’
static_assert(__same_type(*(ptr), ((type *)0)->member) || \
^
drivers/gpu/drm/vc4/vc4_txp.c:167:9: note: in expansion of macro
‘container_of’
return container_of(conn, struct vc4_txp, connector.base);
^
drivers/gpu/drm/vc4/vc4_txp.c: In function ‘vc4_txp_bind’:
drivers/gpu/drm/vc4/vc4_txp.c:495:27: error: passing argument 1 of
‘drm_connector_helper_add’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
drm_connector_helper_add(&txp->connector.base,
^
In file included from ./include/drm/drm_atomic_helper.h:32:0,
from drivers/gpu/drm/vc4/vc4_txp.c:17:
./include/drm/drm_modeset_helper_vtables.h:1153:20: note: expected
‘struct drm_connector *’ but argument is of type ‘struct drm_connector **’
static inline void drm_connector_helper_add(struct drm_connector
*connector,
^
drivers/gpu/drm/vc4/vc4_txp.c:509:10: error: assignment from
incompatible pointer type [-Werror=incompatible-pointer-types]
encoder = &txp->connector.encoder;
^
drivers/gpu/drm/vc4/vc4_txp.c: In function ‘vc4_txp_unbind’:
drivers/gpu/drm/vc4/vc4_txp.c:532:28: error: passing argument 1 of
‘vc4_txp_connector_destroy’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
vc4_txp_connector_destroy(&txp->connector.base);
^
drivers/gpu/drm/vc4/vc4_txp.c:333:13: note: expected ‘struct
drm_connector *’ but argument is of type ‘struct drm_connector **’
static void vc4_txp_connector_destroy(struct drm_connector *connector)
Looks like vc4 doesnt have its own encoder so we might have to move it
to have its own?
struct vc4_txp {
struct vc4_crtc base;
struct platform_device *pdev;
struct drm_writeback_connector connector;
void __iomem *regs;
struct debugfs_regset32 regset;
};
static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder
*encoder)
{
return container_of(encoder, struct vc4_txp, connector.encoder);
}
static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector
*conn)
{
return container_of(conn, struct vc4_txp, connector.base);
}
One more thing, it looks like to me, we need to do one more thing.
Like intel does and what MSM will do, the encoder pointer of the wb
connector has to point to the encoder struct .
>> wb_conn = &intel_connector->wb_conn;
>> wb_conn->base = &intel_connector->base;
>> wb_conn->encoder = &intel_wd->base.base;
Thanks
Abhinav
On 1/27/2022 10:17 AM, Abhinav Kumar wrote:
Hi Suraj
Thanks for the response.
I was not too worried about the intel driver as I am sure you must have
validated this change with that :)
My question was more for the other vendor writeback drivers.
Thanks for looking into the others and providing the snippets.
After looking at those, yes I also think it should work.
So, if others do not have any concern with this change,
Reviewed-by: Abhinav Kumar
On 1/27/2022 1:33 AM, Kandpal, Suraj wrote:
+ laurent on this
Hi Suraj
Jani pointed me to this thread as i had posted something similar here :
https://patchwork.freedesktop.org/patch/470296/ but since this thread
was
posted earlier, we can discuss further here.
Overall, its similar to what I had posted in the RFC and your commit
text also
covers my concerns too.
One question I have about your change is since you have changed
wb_connector:
Re: [Intel-gfx] [PATCH 1/3] drm: add writeback pointers to drm_connector
Hi Suraj
Thanks for the response.
I was not too worried about the intel driver as I am sure you must have
validated this change with that :)
My question was more for the other vendor writeback drivers.
Thanks for looking into the others and providing the snippets.
After looking at those, yes I also think it should work.
So, if others do not have any concern with this change,
Reviewed-by: Abhinav Kumar
On 1/27/2022 1:33 AM, Kandpal, Suraj wrote:
+ laurent on this
Hi Suraj
Jani pointed me to this thread as i had posted something similar here :
https://patchwork.freedesktop.org/patch/470296/ but since this thread was
posted earlier, we can discuss further here.
Overall, its similar to what I had posted in the RFC and your commit text also
covers my concerns too.
One question I have about your change is since you have changed
wb_connector::encoder to be a pointer, i saw the other changes in the series
but they do not allocate an encoder. Would this not affect the other drivers
which are assuming that the encoder in wb_connector is struct drm_encoder
encoder and not struct drm_encoder* encoder.
Your changes fix the compilation issue but wouldnt this crash as encoder
wasnt allocated for other drivers.
Hi Abhinav,
That shouldn't be an issue as normally drivers tend to have their own output
structure which has drm_connector and drm_encoder embedded in it depending
on the level of binding they have decided to give the connector and encoder in
their respective output and the addresses of these are passed to the
drm_connector* and drm_encoder* in drm_writeback_connector structure
which then gets initialized in drm_writeback_connector_init().
In our i915 code we have intel_connector structure with drm_connector base
field and intel_wd with a intel_encoder base which in turn has drm_encoder
field and both intel_connector and intel_wd are initialized not requiring
explicit
alloc and dealloc for drm_encoder
intel_wd = kzalloc(sizeof(*intel_wd), GFP_KERNEL);
intel_connector = intel_connector_alloc();
wb_conn = &intel_connector->wb_conn;
wb_conn->base = &intel_connector->base;
wb_conn->encoder = &intel_wd->base.base;
Similary for komeda driver has
struct komeda_wb_connector {
struct drm_connector conn;
/** @base: &drm_writeback_connector */
struct drm_writeback_connector base;
/** @wb_layer: represents associated writeback pipeline of komeda */
struct komeda_layer *wb_layer;
};
static int komeda_wb_connector_add(struct komeda_kms_dev *kms,
struct komeda_crtc *kcrtc)
{
struct komeda_wb_connector *kwb_conn;
struct drm_writeback_connector *wb_conn;
kwb_conn = kzalloc(sizeof(*kwb_conn), GFP_KERNEL);
wb_conn = &kwb_conn->base;
wb_conn->base = &kwb_conn->conn;
and they do not depend on the encoder binding so changes will work for them
Also in vkms driver we have the
struct vkms_output {
struct drm_crtc crtc;
struct drm_encoder encoder;
struct drm_connector connector;
struct drm_writeback_connector wb_connector;
struct hrtimer vblank_hrtimer;
ktime_t period_ns;
struct drm_pending_vblank_event *event;
/* ordered wq for composer_work */
struct workqueue_struct *composer_workq;
/* protects concurrent access to composer */
spinlock_t lock;
/* protected by @lock */
bool composer_enabled;
struct vkms_crtc_state *composer_state;
spinlock_t composer_lock;
};
Which gets allocated covering for the drm_encoder alloc and dealloc
Regards,
Suraj Kandpal
Re: [Intel-gfx] [PATCH 1/3] drm: add writeback pointers to drm_connector
+ laurent on this
Hi Suraj
On 1/11/2022 2:17 AM, Kandpal, Suraj wrote:
Changing drm_connector and drm_encoder feilds to pointers in
drm_writeback_connector as the elements of struct
drm_writeback_connector are:
struct drm_writeback_connector {
struct drm_connector base;
struct drm_encoder encoder;
Similarly the elements of intel_encoder and intel_connector
are:
struct intel_encoder {
struct drm_encoder base;
struct intel_connector {
struct drm_connector base;
The function drm_writeback_connector_init() will initialize the
drm_connector and drm_encoder and attach them as well.
Since the drm_connector/encoder are both struct in
drm_writeback_connector and intel_connector/encoder, we need
one of them to be a pointer so we can reference them or else we
will be pointing to 2 seprate instances.
Usually the struct defined in drm framework pointing to any
struct will be pointer and allocating them and initialization
will be done with the users.
Like struct drm_connector and drm_encoder are part of drm
framework and the users of these such as i915 have included them
in their struct intel_connector and intel_encoder. Likewise
struct drm_writeback_connector is a special connector and hence
is not a user of drm_connector and hence this should be pointers.
Adding drm_writeback_connector to drm_connector so that
writeback_connector can be fetched from drm_connector as the previous
container_of method won't work due to change in the feilds of
drm_connector and drm_encoder in drm_writeback_connector.
Note:The corresponding ripple effect due to the above changes namely in
two drivers as I can see it komeda and vkms have been dealt with in the
upcoming patches of this series.
Signed-off-by: Kandpal, Suraj
Jani pointed me to this thread as i had posted something similar here :
https://patchwork.freedesktop.org/patch/470296/ but since this thread
was posted earlier, we can discuss further here.
Overall, its similar to what I had posted in the RFC and your commit
text also covers my concerns too.
One question I have about your change is since you have changed
wb_connector::encoder to be a pointer, i saw the other changes in the
series but they do not allocate an encoder. Would this not affect the
other drivers which are assuming that the encoder in wb_connector is
struct drm_encoder encoder and not struct drm_encoder* encoder.
Your changes fix the compilation issue but wouldnt this crash as encoder
wasnt allocated for other drivers.
---
drivers/gpu/drm/drm_writeback.c | 19 ++-
include/drm/drm_connector.h | 3 +++
include/drm/drm_writeback.h | 6 +++---
3 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dccf4504f1bb..47238db42363 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -87,7 +87,7 @@ static const char *drm_writeback_fence_get_driver_name(struct
dma_fence *fence)
struct drm_writeback_connector *wb_connector =
fence_to_wb_connector(fence);
- return wb_connector->base.dev->driver->name;
+ return wb_connector->base->dev->driver->name;
}
static const char *
@@ -177,7 +177,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
const u32 *formats, int n_formats)
{
struct drm_property_blob *blob;
- struct drm_connector *connector = &wb_connector->base;
+ struct drm_connector *connector = wb_connector->base;
struct drm_mode_config *config = &dev->mode_config;
int ret = create_writeback_properties(dev);
@@ -189,14 +189,15 @@ int drm_writeback_connector_init(struct drm_device *dev,
if (IS_ERR(blob))
return PTR_ERR(blob);
- drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
- ret = drm_encoder_init(dev, &wb_connector->encoder,
+ drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
+ ret = drm_encoder_init(dev, wb_connector->encoder,
&drm_writeback_encoder_funcs,
DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
goto fail;
connector->interlace_allowed = 0;
+ connector->wb_connector = wb_connector;
ret = drm_connector_init(dev, connector, con_funcs,
DRM_MODE_CONNECTOR_WRITEBACK);
@@ -204,7 +205,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
goto connector_fail;
ret = drm_connector_attach_encoder(connector,
- &wb_connector->encoder);
+ wb_connector->encoder);
if (ret)
goto attach_fail;
@@ -233,7 +234,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
attach_fail:
drm_connector_cleanup(connector);
connector_fail:
- drm
