| Message ID | 1396001748-8147-1-git-send-email-fabien.dessenne@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.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> > --- > 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; > + } > 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 >
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) {
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(-)