This fixes an issue where the DRM planes do not support the same pixel formats. The current implementation selects a DRM plane without checking whether the pixel format is supported or not. As a consequence modetest may try to set up a plane not supporting the user request-format, which fails. Modetest has to check the supported formats accross the plane list before selecting a candidate.
Signed-off-by: Fabien Dessenne fabien.dessenne@st.com --- tests/modetest/modetest.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c index 4761c60..866ea82 100644 --- a/tests/modetest/modetest.c +++ b/tests/modetest/modetest.c @@ -951,7 +951,7 @@ static int set_plane(struct device *dev, struct plane_arg *p) int crtc_x, crtc_y, crtc_w, crtc_h; struct crtc *crtc = NULL; unsigned int pipe; - unsigned int i; + unsigned int i, j;
/* Find an unused plane which can be connected to our CRTC. Find the * CRTC index first, then iterate over available planes. @@ -974,8 +974,11 @@ static int set_plane(struct device *dev, struct plane_arg *p) if (!ovr) continue;
- if ((ovr->possible_crtcs & (1 << pipe)) && !ovr->crtc_id) - plane_id = ovr->plane_id; + if ((ovr->possible_crtcs & (1 << pipe)) && !ovr->crtc_id) { + for (j = 0; j < ovr->count_formats; j++) + if (!strncmp(p->format_str, (char *) &ovr->formats[j], 4)) + plane_id = ovr->plane_id; + } }
if (!plane_id) {
Can anyone review this patch ? Thanks for your time. Fabien
-----Original Message----- From: Fabien DESSENNE [mailto:fabien.dessenne@st.com] Sent: vendredi 28 mars 2014 11:16 To: dri-devel@lists.freedesktop.org Cc: Benjamin Gaignard; Vincent Abriou; Fabien DESSENNE Subject: [PATCH] modetest: consider supported formats before selecting a DRM plane
This fixes an issue where the DRM planes do not support the same pixel formats. The current implementation selects a DRM plane without checking whether the pixel format is supported or not. As a consequence modetest may try to set up a plane not supporting the user request-format, which fails. Modetest has to check the supported formats accross the plane list before selecting a candidate.
Signed-off-by: Fabien Dessenne fabien.dessenne@st.com
tests/modetest/modetest.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c index 4761c60..866ea82 100644 --- a/tests/modetest/modetest.c +++ b/tests/modetest/modetest.c @@ -951,7 +951,7 @@ static int set_plane(struct device *dev, struct plane_arg *p) int crtc_x, crtc_y, crtc_w, crtc_h; struct crtc *crtc = NULL; unsigned int pipe;
- unsigned int i;
unsigned int i, j;
/* Find an unused plane which can be connected to our CRTC. Find
the * CRTC index first, then iterate over available planes. @@ -974,8 +974,11 @@ static int set_plane(struct device *dev, struct plane_arg *p) if (!ovr) continue;
if ((ovr->possible_crtcs & (1 << pipe)) && !ovr->crtc_id)plane_id = ovr->plane_id;
if ((ovr->possible_crtcs & (1 << pipe)) && !ovr->crtc_id) {for (j = 0; j < ovr->count_formats; j++)if (!strncmp(p->format_str, (char *) &ovr-formats[j], 4))
plane_id = ovr->plane_id;}}
if (!plane_id) {
-- 1.9.0
On Fri, Mar 28, 2014 at 6:15 AM, Fabien DESSENNE fabien.dessenne@st.com wrote:
This fixes an issue where the DRM planes do not support the same pixel formats. The current implementation selects a DRM plane without checking whether the pixel format is supported or not. As a consequence modetest may try to set up a plane not supporting the user request-format, which fails. Modetest has to check the supported formats accross the plane list before selecting a candidate.
Signed-off-by: Fabien Dessenne fabien.dessenne@st.com
Looks reasonable. I suppose one drawback is that you could previously use modetest to test an error condition. I'm not sure if anyone cares about that.. having a proper collection of tests for generic KMS APIs would of course be a better solution to that problem ;-)
It could be a useful thing for linaro to look into intel-gpu-tools and see if it could be possible to refactor some of that into some cross-driver tests ;-)
BR, -R
tests/modetest/modetest.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c index 4761c60..866ea82 100644 --- a/tests/modetest/modetest.c +++ b/tests/modetest/modetest.c @@ -951,7 +951,7 @@ static int set_plane(struct device *dev, struct plane_arg *p) int crtc_x, crtc_y, crtc_w, crtc_h; struct crtc *crtc = NULL; unsigned int pipe;
unsigned int i;
unsigned int i, j; /* Find an unused plane which can be connected to our CRTC. Find the * CRTC index first, then iterate over available planes.@@ -974,8 +974,11 @@ static int set_plane(struct device *dev, struct plane_arg *p) if (!ovr) continue;
if ((ovr->possible_crtcs & (1 << pipe)) && !ovr->crtc_id)plane_id = ovr->plane_id;
if ((ovr->possible_crtcs & (1 << pipe)) && !ovr->crtc_id) {for (j = 0; j < ovr->count_formats; j++)if (!strncmp(p->format_str, (char *) &ovr->formats[j], 4))plane_id = ovr->plane_id;} } if (!plane_id) {-- 1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi,
Do we have something more to do get this patch merge ? Or do you see issue ?
Regards, Benjamin
2014-04-04 16:36 GMT+02:00 Rob Clark robdclark@gmail.com:
On Fri, Mar 28, 2014 at 6:15 AM, Fabien DESSENNE fabien.dessenne@st.com wrote:
This fixes an issue where the DRM planes do not support the same pixel formats. The current implementation selects a DRM plane without checking whether the pixel format is supported or not. As a consequence modetest may try to set up a plane not supporting the user request-format, which fails. Modetest has to check the supported formats accross the plane list before selecting a candidate.
Signed-off-by: Fabien Dessenne fabien.dessenne@st.com
Looks reasonable. I suppose one drawback is that you could previously use modetest to test an error condition. I'm not sure if anyone cares about that.. having a proper collection of tests for generic KMS APIs would of course be a better solution to that problem ;-)
It could be a useful thing for linaro to look into intel-gpu-tools and see if it could be possible to refactor some of that into some cross-driver tests ;-)
BR, -R
tests/modetest/modetest.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c index 4761c60..866ea82 100644 --- a/tests/modetest/modetest.c +++ b/tests/modetest/modetest.c @@ -951,7 +951,7 @@ static int set_plane(struct device *dev, struct plane_arg *p) int crtc_x, crtc_y, crtc_w, crtc_h; struct crtc *crtc = NULL; unsigned int pipe;
unsigned int i;
unsigned int i, j; /* Find an unused plane which can be connected to our CRTC. Find the * CRTC index first, then iterate over available planes.@@ -974,8 +974,11 @@ static int set_plane(struct device *dev, struct plane_arg *p) if (!ovr) continue;
if ((ovr->possible_crtcs & (1 << pipe)) && !ovr->crtc_id)plane_id = ovr->plane_id;
if ((ovr->possible_crtcs & (1 << pipe)) && !ovr->crtc_id) {for (j = 0; j < ovr->count_formats; j++)if (!strncmp(p->format_str, (char *) &ovr->formats[j], 4))plane_id = ovr->plane_id;} } if (!plane_id) {-- 1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Mar 28, 2014 at 6:15 PM, Fabien DESSENNE fabien.dessenne@st.comwrote:
This fixes an issue where the DRM planes do not support the same pixel formats. The current implementation selects a DRM plane without checking whether the pixel format is supported or not. As a consequence modetest may try to set up a plane not supporting the user request-format, which fails. Modetest has to check the supported formats accross the plane list before selecting a candidate.
Signed-off-by: Fabien Dessenne fabien.dessenne@st.com
tests/modetest/modetest.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c index 4761c60..866ea82 100644 --- a/tests/modetest/modetest.c +++ b/tests/modetest/modetest.c @@ -951,7 +951,7 @@ static int set_plane(struct device *dev, struct plane_arg *p) int crtc_x, crtc_y, crtc_w, crtc_h; struct crtc *crtc = NULL; unsigned int pipe;
unsigned int i;
unsigned int i, j; /* Find an unused plane which can be connected to our CRTC. Findthe * CRTC index first, then iterate over available planes. @@ -974,8 +974,11 @@ static int set_plane(struct device *dev, struct plane_arg *p) if (!ovr) continue;
if ((ovr->possible_crtcs & (1 << pipe)) && !ovr->crtc_id)plane_id = ovr->plane_id;
if ((ovr->possible_crtcs & (1 << pipe)) && !ovr->crtc_id) {for (j = 0; j < ovr->count_formats; j++)if (!strncmp(p->format_str, (char *)&ovr->formats[j], 4))
plane_id = ovr->plane_id;}
This loop continues to cycle through the list even after a positive match. A micro-optimization would be to add a "break;" after setting plane_id. Either way, this patch is:
Reviewed-by: Daniel Kurtz djkurtz@chromium.org
But, I can't help you get the patch actually committed to the DRM repository... I'm still trying to get someone to help me get my own patches in.
Best Regards, -djk
}
if (!plane_id) {-- 1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Benjamin Gaignard -
Daniel Kurtz -
Fabien DESSENNE -
Rob Clark