| Message ID | 20170308163025.31098-1-Liviu.Dudau@arm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 08, 2017 at 04:30:25PM +0000, Liviu Dudau wrote: > The calculation of the framebuffer's start address was wrongly using > the CRTC's x and y position rather than the one of the source > framebuffer. To fix that we need to update the plane_check code to > call drm_plane_helper_check_state() to clip the src and dst coordinates. > While there so some minor cleanup of redundant freeing of > devm_alloc-ated memory. The following series is what I came up with, although I've had no time to test it.
On Fri, Mar 31, 2017 at 12:41:30PM +0100, Liviu Dudau wrote: > On Fri, Mar 31, 2017 at 11:27:51AM +0100, Russell King - ARM Linux wrote: > > On Fri, Mar 31, 2017 at 11:23:45AM +0100, Liviu Dudau wrote: > > > On Fri, Mar 31, 2017 at 11:20:35AM +0100, Russell King - ARM Linux wrote: > > > > On Fri, Mar 31, 2017 at 11:18:50AM +0100, Liviu Dudau wrote: > > > > > Hi Russell, > > > > > > > > > > You were Cc-ed in a patch from March 8th that did all this: > > > > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2017-March/135172.html > > > > > > > > I'm aware of that (you may notice that this was threaded to that patch.) > > > > > > > > > I have not received any response from you, so I have already pushed the > > > > > patch in my public repo: > > > > > > > > > > git://linux-arm.org/linux-ld.git for-upstream/hdlcd > > > > > > > > > > It has been included into linux-next for at least a couple of weeks now. > > > > > > > > I've not had a chance to test any of this, but I believe that your > > > > patch does not fully address the issue, due to bits missing from > > > > the validation path. > > > > > > Care to point out which bits were missing from my patch that are in yours? > > > > The visible check? > > A plane's ->atomic_check() hook can be called with TEST_ONLY to figure out from > userspace if the given configuration is a valid one that can be accepted by > the hardware. There should be no error if the plane will not be visible, as we > are not programming anything yet. > > I would also argue that the test that you remove and replace with state->visible > is important. We can't do *any* scaling, while with your patch we could accept > src_w != crtc_w as long as it is visible. Hardware is not capable of handling that. That's what the "DRM_PLANE_HELPER_NO_SCALING" arguments to drm_plane_helper_check_state() are doing: drm_plane_helper_check_state() drm_rect_calc_hscale() if (hscale < min_hscale || hscale > max_hscale) return -ERANGE; drm_rect_calc_vscale() if (vscale < min_vscale || vscale > max_vscale) return -ERANGE; where DRM_PLANE_HELPER_NO_SCALING is 1.0 in 16:16 format. So, this ensures that the scaling factor is 1.0, returning -ERANGE if it isn't. If this lets through a scaled source, then there's a bug that needs fixing in the helper.
On Fri, Mar 31, 2017 at 10:49:38AM +0100, Russell King - ARM Linux wrote: > On Wed, Mar 08, 2017 at 04:30:25PM +0000, Liviu Dudau wrote: > > The calculation of the framebuffer's start address was wrongly using > > the CRTC's x and y position rather than the one of the source > > framebuffer. To fix that we need to update the plane_check code to > > call drm_plane_helper_check_state() to clip the src and dst coordinates. > > While there so some minor cleanup of redundant freeing of > > devm_alloc-ated memory. > > The following series is what I came up with, although I've had no time > to test it. I'm afraid I'm going to NAK this series. It would've been nicer for you to comment on the v2 patch that I have sent rather than going around and coming back with effectively the same thing but split into 2 patches. I'm having trouble applying your series to the v4.11-rc4 (specially 2/3). Also 3/3 is superfluous, as we don't expose the DRM_ROTATE property to userspace. Best regards, Liviu > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
On Fri, Mar 31, 2017 at 02:18:31PM +0100, Liviu Dudau wrote: > On Fri, Mar 31, 2017 at 10:49:38AM +0100, Russell King - ARM Linux wrote: > > On Wed, Mar 08, 2017 at 04:30:25PM +0000, Liviu Dudau wrote: > > > The calculation of the framebuffer's start address was wrongly using > > > the CRTC's x and y position rather than the one of the source > > > framebuffer. To fix that we need to update the plane_check code to > > > call drm_plane_helper_check_state() to clip the src and dst coordinates. > > > While there so some minor cleanup of redundant freeing of > > > devm_alloc-ated memory. > > > > The following series is what I came up with, although I've had no time > > to test it. > > I'm afraid I'm going to NAK this series. It would've been nicer for you to > comment on the v2 patch that I have sent rather than going around and coming > back with effectively the same thing but split into 2 patches. I'm having > trouble applying your series to the v4.11-rc4 (specially 2/3). Also 3/3 is > superfluous, as we don't expose the DRM_ROTATE property to userspace. Rather than throwing accusations, let's look at what happened. I reported the bug on 18th November 2016 - I quote: Something I also noticed is this: scanout_start = gem->paddr + plane->state->fb->offsets[0] + plane->state->crtc_y * plane->state->fb->pitches[0] + plane->state->crtc_x * bpp / 8; Surely this should be using src_[xy] (which are the position in the source - iow, memory, and not crtc_[xy] which is the position on the CRTC displayed window. To put it another way, the src_* define the region of the source material that is mapped onto a rectangular area on the display defined by crtc_*. This got ignored, and on 21st November, I came up with an initial patch to solve it at the time, but we were busy discussing the base address issue. I sent a reminder on 20th February about it, and we discussed it, and I said at the time I did not have time to test your patch. Ville commented on your patch, which confused me a little, but having worked it out, I reworked the patch from 21st November to fix that, creating this patch series. I did not post it, because I hadn't tested it (since the Juno requires a long-winded way to update the kernel), so I never got around to testing this. So, this series pre-dates your v2 patch by a good few weeks. You posted your v2 patch on March 8th, and I've not had a chance to test that, nor have I had a chance to test my own three patch series. Today, I noticed that I still had the three patch series, so I thought I ought to get it out in the wild. Now, due to the amount of patches I carry: $ git lg origin.. | wc -l 491 I work against Linus' tree _only_, so all patches I post are based on Linus' kernel, and not random other git trees. I do not randomly fetch other git trees to base patches on them, because that would cause me insane merging issues so that I can test half the stuff I'm carrying. Now, it's true that they're not against -rc, but are currently against 4.10 - it seems that I missed rebasing _this_ particular branch to 4.11-rc yet, although most of my other branches are. What was I so busy with when you posted your patch on the 8th March? Oh yes, that was the week _after_ the merge window closed, and was the week I was doing the mass rebase of about 500 patches. Sorry I didn't get around to testing your patch, and sorry for eventually getting around to posting my patches. Obviously, I should just fuck off and do something else.
On Fri, Mar 31, 2017 at 02:48:10PM +0100, Russell King - ARM Linux wrote: > On Fri, Mar 31, 2017 at 02:18:31PM +0100, Liviu Dudau wrote: > > On Fri, Mar 31, 2017 at 10:49:38AM +0100, Russell King - ARM Linux wrote: > > > On Wed, Mar 08, 2017 at 04:30:25PM +0000, Liviu Dudau wrote: > > > > The calculation of the framebuffer's start address was wrongly using > > > > the CRTC's x and y position rather than the one of the source > > > > framebuffer. To fix that we need to update the plane_check code to > > > > call drm_plane_helper_check_state() to clip the src and dst coordinates. > > > > While there so some minor cleanup of redundant freeing of > > > > devm_alloc-ated memory. > > > > > > The following series is what I came up with, although I've had no time > > > to test it. > > > > I'm afraid I'm going to NAK this series. It would've been nicer for you to > > comment on the v2 patch that I have sent rather than going around and coming > > back with effectively the same thing but split into 2 patches. I'm having > > trouble applying your series to the v4.11-rc4 (specially 2/3). Also 3/3 is > > superfluous, as we don't expose the DRM_ROTATE property to userspace. > > Rather than throwing accusations, let's look at what happened. No accusations were thrown, just explanations of my decision. > > I reported the bug on 18th November 2016 - I quote: > > Something I also noticed is this: > > scanout_start = gem->paddr + plane->state->fb->offsets[0] + > plane->state->crtc_y * plane->state->fb->pitches[0] + > plane->state->crtc_x * bpp / 8; > > Surely this should be using src_[xy] (which are the position in the > source - iow, memory, and not crtc_[xy] which is the position on the > CRTC displayed window. To put it another way, the src_* define the > region of the source material that is mapped onto a rectangular area > on the display defined by crtc_*. > > This got ignored, and on 21st November, I came up with an initial patch > to solve it at the time, but we were busy discussing the base address > issue. As I've replied to the 20th February email, I did not ignore it, just lost track of it. > > I sent a reminder on 20th February about it, and we discussed it, and I > said at the time I did not have time to test your patch. Ville commented > on your patch, which confused me a little, but having worked it out, I > reworked the patch from 21st November to fix that, creating this patch > series. > > I did not post it, because I hadn't tested it (since the Juno requires > a long-winded way to update the kernel), so I never got around to > testing this. So, this series pre-dates your v2 patch by a good few > weeks. That information was privy to you and would've been nice to share with me. > > You posted your v2 patch on March 8th, and I've not had a chance to > test that, nor have I had a chance to test my own three patch series. > > Today, I noticed that I still had the three patch series, so I thought > I ought to get it out in the wild. Hey, look, a clasic case of comedy of errors when people don't talk to each other!!! Sorry, Russell, but I'm not inside your brain or your computer, so I don't know intimately the history of things. Thanks for enlightening me but I also read your story as a clear example why a small paragraph after the commit log explaining why this series has been submitted would have gone a long way clearing the fog. > > Now, due to the amount of patches I carry: > > $ git lg origin.. | wc -l > 491 > > I work against Linus' tree _only_, so all patches I post are based on > Linus' kernel, and not random other git trees. I do not randomly fetch > other git trees to base patches on them, because that would cause me > insane merging issues so that I can test half the stuff I'm carrying. I understand (to the best of my abilities) your position and the fact that as a maintainer of a large subsystem you have a specific workflow that you don't want to polute with minor exceptions. I would also ask you to understand that not everyone works in the same way as you and that other maintainers and other subsystems have different workflows and requirements. Having my tree as part of the DRM subtree means that we work mostly on the recent Linus' -rc up until about -rc4 and the quickly switch to linux-next. So one way of approaching this is to drop the arch/arm frame of thought when contributing to other trees and adopt their workflow (you know, the "when in Rome, do what romans do" attitude). The other way is to go to different maintainers and ask for special status and tell them that their puny drivers and tree don't matter that much compared to your mighty workflow and they should all bend to your greatness (the "all your bases belong to us" mindset). > > Now, it's true that they're not against -rc, but are currently against > 4.10 - it seems that I missed rebasing _this_ particular branch to > 4.11-rc yet, although most of my other branches are. And how would you have handled this situation? Another maintainer sending you a patchset based on an older tree that doesn't match your currently published one? Would you have gone to the trouble of rebasing their work, or ask them do get back to you with a better series? > > What was I so busy with when you posted your patch on the 8th March? > Oh yes, that was the week _after_ the merge window closed, and was the > week I was doing the mass rebase of about 500 patches. I've seen people sending emails (not while they are busy, 8th March was 3 weeks prior to this whole thread) saying "I'm still interested in this, I will test it when I get a chance". Or something more personal, like "I already had something in my tree, if I don't get around testing your patch then I will publish mine based on an old tree, so be prepared" > > Sorry I didn't get around to testing your patch, and sorry for eventually > getting around to posting my patches. Obviously, I should just fuck off > and do something else. Please don't! I am happy that you show interest in this driver and you help me improve it, and I do not wish to repeat the current conflict in the future. I'm just hoping that we can improve the flow of information between us and give you more peace of mind. Best regards, Liviu > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
On Mon, Apr 03, 2017 at 11:31:34AM +0100, Liviu Dudau wrote: > On Fri, Mar 31, 2017 at 02:48:10PM +0100, Russell King - ARM Linux wrote: > > I sent a reminder on 20th February about it, and we discussed it, and I > > said at the time I did not have time to test your patch. Ville commented > > on your patch, which confused me a little, but having worked it out, I > > reworked the patch from 21st November to fix that, creating this patch > > series. > > > > I did not post it, because I hadn't tested it (since the Juno requires > > a long-winded way to update the kernel), so I never got around to > > testing this. So, this series pre-dates your v2 patch by a good few > > weeks. > > That information was privy to you and would've been nice to share with me. So what the _hell_ do you think _this_ sentence means? It _was_ shared with you. The following series is what I came up with, although I've had no time to test it. which was in the mail which was the parent to the series? Does it mean I'm bouncing a ball around the back yard maybe? No, the information was there, you chose not to read it, and *you* fucked up. Notice that it is PAST TENSE, which means it HAPPENED IN THE PAST. NOT PRESENT. This is basic english comprehension. > > Now, due to the amount of patches I carry: > > > > $ git lg origin.. | wc -l > > 491 > > > > I work against Linus' tree _only_, so all patches I post are based on > > Linus' kernel, and not random other git trees. I do not randomly fetch > > other git trees to base patches on them, because that would cause me > > insane merging issues so that I can test half the stuff I'm carrying. > > I understand (to the best of my abilities) your position and the fact that > as a maintainer of a large subsystem you have a specific workflow that you > don't want to polute with minor exceptions. I would also ask you to understand > that not everyone works in the same way as you and that other maintainers > and other subsystems have different workflows and requirements. Having my > tree as part of the DRM subtree means that we work mostly on the recent > Linus' -rc up until about -rc4 and the quickly switch to linux-next. So one > way of approaching this is to drop the arch/arm frame of thought when > contributing to other trees and adopt their workflow (you know, the "when > in Rome, do what romans do" attitude). The other way is to go to different > maintainers and ask for special status and tell them that their puny drivers > and tree don't matter that much compared to your mighty workflow and they > should all bend to your greatness (the "all your bases belong to us" mindset). For christ sake, I sent the patches out because I thought it would be useful to show what I had come up with, because I believed it to be of use. I won't make that mistake again, I'll just delete such work, because it's obviously far too much hassle to work with other people, because they don't READ. > > Now, it's true that they're not against -rc, but are currently against > > 4.10 - it seems that I missed rebasing _this_ particular branch to > > 4.11-rc yet, although most of my other branches are. > > And how would you have handled this situation? Another maintainer sending > you a patchset based on an older tree that doesn't match your currently > published one? Would you have gone to the trouble of rebasing their work, > or ask them do get back to you with a better series? If the other work is better, then I would have replaced what I had already merged with the better version, or ask for an update against the current version. I doubt that I'm going to get any time what so ever to test either version, so I'm going to delete my version and wait for your version to trickle through - which I guess will be in about two months time, after the next merge window.
On Mon, Apr 03, 2017 at 02:13:48PM +0100, Russell King - ARM Linux wrote: > On Mon, Apr 03, 2017 at 11:31:34AM +0100, Liviu Dudau wrote: > > On Fri, Mar 31, 2017 at 02:48:10PM +0100, Russell King - ARM Linux wrote: > > > I sent a reminder on 20th February about it, and we discussed it, and I > > > said at the time I did not have time to test your patch. Ville commented > > > on your patch, which confused me a little, but having worked it out, I > > > reworked the patch from 21st November to fix that, creating this patch > > > series. > > > > > > I did not post it, because I hadn't tested it (since the Juno requires > > > a long-winded way to update the kernel), so I never got around to > > > testing this. So, this series pre-dates your v2 patch by a good few > > > weeks. > > > > That information was privy to you and would've been nice to share with me. > > So what the _hell_ do you think _this_ sentence means? It _was_ shared > with you. > > The following series is what I came up with, although I've had no time > to test it. > > which was in the mail which was the parent to the series? Does it > mean I'm bouncing a ball around the back yard maybe? > > No, the information was there, you chose not to read it, and *you* > fucked up. Notice that it is PAST TENSE, which means it HAPPENED IN > THE PAST. NOT PRESENT. This is basic english comprehension. Hey, you fail basic english comprehension too! My sentence was referring to your last sentence: "So, this series pre-dates your v2 patch by a good few weeks." That was the information that you have failed to share. > > > > Now, due to the amount of patches I carry: > > > > > > $ git lg origin.. | wc -l > > > 491 > > > > > > I work against Linus' tree _only_, so all patches I post are based on > > > Linus' kernel, and not random other git trees. I do not randomly fetch > > > other git trees to base patches on them, because that would cause me > > > insane merging issues so that I can test half the stuff I'm carrying. > > > > I understand (to the best of my abilities) your position and the fact that > > as a maintainer of a large subsystem you have a specific workflow that you > > don't want to polute with minor exceptions. I would also ask you to understand > > that not everyone works in the same way as you and that other maintainers > > and other subsystems have different workflows and requirements. Having my > > tree as part of the DRM subtree means that we work mostly on the recent > > Linus' -rc up until about -rc4 and the quickly switch to linux-next. So one > > way of approaching this is to drop the arch/arm frame of thought when > > contributing to other trees and adopt their workflow (you know, the "when > > in Rome, do what romans do" attitude). The other way is to go to different > > maintainers and ask for special status and tell them that their puny drivers > > and tree don't matter that much compared to your mighty workflow and they > > should all bend to your greatness (the "all your bases belong to us" mindset). > > For christ sake, I sent the patches out because I thought it would be > useful to show what I had come up with, because I believed it to be of > use. > > I won't make that mistake again, I'll just delete such work, because > it's obviously far too much hassle to work with other people, because > they don't READ. > > > > Now, it's true that they're not against -rc, but are currently against > > > 4.10 - it seems that I missed rebasing _this_ particular branch to > > > 4.11-rc yet, although most of my other branches are. > > > > And how would you have handled this situation? Another maintainer sending > > you a patchset based on an older tree that doesn't match your currently > > published one? Would you have gone to the trouble of rebasing their work, > > or ask them do get back to you with a better series? > > If the other work is better, then I would have replaced what I had > already merged with the better version, or ask for an update against > the current version. > > I doubt that I'm going to get any time what so ever to test either > version, so I'm going to delete my version and wait for your version > to trickle through - which I guess will be in about two months time, > after the next merge window. Good. Thanks! Liviu > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
On Mon, 03 Apr 2017, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > So what the _hell_ do you think _this_ sentence means? It _was_ shared > with you. > > The following series is what I came up with, although I've had no time > to test it. > > which was in the mail which was the parent to the series? Does it > mean I'm bouncing a ball around the back yard maybe? > > No, the information was there, you chose not to read it, and *you* > fucked up. Notice that it is PAST TENSE, which means it HAPPENED IN > THE PAST. NOT PRESENT. This is basic english comprehension. [...] > For christ sake, I sent the patches out because I thought it would be > useful to show what I had come up with, because I believed it to be of > use. > > I won't make that mistake again, I'll just delete such work, because > it's obviously far too much hassle to work with other people, because > they don't READ. Please keep the discussion civil. We try to maintain a friendly collaborative atmosphere on dri-devel, and this kind of interaction is not welcome in our community. Thanks for your understanding. BR, Jani.
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c index 20ebfb4fbdfa..c65116348281 100644 --- a/drivers/gpu/drm/arm/hdlcd_crtc.c +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c @@ -10,6 +10,7 @@ */ #include <drm/drmP.h> +#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc.h> #include <drm/drm_crtc_helper.h> @@ -206,16 +207,33 @@ static const struct drm_crtc_helper_funcs hdlcd_crtc_helper_funcs = { static int hdlcd_plane_atomic_check(struct drm_plane *plane, struct drm_plane_state *state) { - u32 src_w, src_h; + struct drm_rect clip = { 0 }; + struct drm_crtc_state *crtc_state; + u32 src_h = state->src_h >> 16; - src_w = state->src_w >> 16; - src_h = state->src_h >> 16; + /* only the HDLCD_REG_FB_LINE_COUNT register has a limit */ + if (src_h >= HDLCD_MAX_YRES) { + DRM_DEBUG_KMS("Invalid source width: %d\n", src_h); + return -EINVAL; + } + + if (!state->fb || !state->crtc) + return 0; - /* we can't do any scaling of the plane source */ - if ((src_w != state->crtc_w) || (src_h != state->crtc_h)) + crtc_state = drm_atomic_get_existing_crtc_state(state->state, + state->crtc); + if (!crtc_state) { + DRM_DEBUG_KMS("Invalid crtc state\n"); return -EINVAL; + } - return 0; + clip.x2 = crtc_state->adjusted_mode.hdisplay; + clip.y2 = crtc_state->adjusted_mode.vdisplay; + + return drm_plane_helper_check_state(state, &clip, + DRM_PLANE_HELPER_NO_SCALING, + DRM_PLANE_HELPER_NO_SCALING, + false, true); } static void hdlcd_plane_atomic_update(struct drm_plane *plane, @@ -224,21 +242,20 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane, struct drm_framebuffer *fb = plane->state->fb; struct hdlcd_drm_private *hdlcd; struct drm_gem_cma_object *gem; - u32 src_w, src_h, dest_w, dest_h; + u32 src_x, src_y, dest_h; dma_addr_t scanout_start; if (!fb) return; - src_w = plane->state->src_w >> 16; - src_h = plane->state->src_h >> 16; - dest_w = plane->state->crtc_w; - dest_h = plane->state->crtc_h; + src_x = plane->state->src.x1 >> 16; + src_y = plane->state->src.y1 >> 16; + dest_h = drm_rect_height(&plane->state->dst); gem = drm_fb_cma_get_gem_obj(fb, 0); + scanout_start = gem->paddr + fb->offsets[0] + - plane->state->crtc_y * fb->pitches[0] + - plane->state->crtc_x * - fb->format->cpp[0]; + src_y * fb->pitches[0] + + src_x * fb->format->cpp[0]; hdlcd = plane->dev->dev_private; hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_LENGTH, fb->pitches[0]); @@ -285,7 +302,6 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device *drm) formats, ARRAY_SIZE(formats), DRM_PLANE_TYPE_PRIMARY, NULL); if (ret) { - devm_kfree(drm->dev, plane); return ERR_PTR(ret); } @@ -309,7 +325,6 @@ int hdlcd_setup_crtc(struct drm_device *drm) &hdlcd_crtc_funcs, NULL); if (ret) { hdlcd_plane_destroy(primary); - devm_kfree(drm->dev, primary); return ret; }
The calculation of the framebuffer's start address was wrongly using the CRTC's x and y position rather than the one of the source framebuffer. To fix that we need to update the plane_check code to call drm_plane_helper_check_state() to clip the src and dst coordinates. While there so some minor cleanup of redundant freeing of devm_alloc-ated memory. Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> --- v1 was discussed here: https://lists.freedesktop.org/archives/dri-devel/2017-February/133379.html drivers/gpu/drm/arm/hdlcd_crtc.c | 47 ++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 16 deletions(-)