From mboxrd@z Thu Jan 1 00:00:00 1970 From: robdclark@gmail.com (Rob Clark) Date: Mon, 10 Jun 2013 18:49:06 -0400 Subject: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver In-Reply-To: <20130610211516.GZ18614@n2100.arm.linux.org.uk> References: <20130609190612.GM18614@n2100.arm.linux.org.uk> <20130610170648.GX18614@n2100.arm.linux.org.uk> <20130610200839.GY18614@n2100.arm.linux.org.uk> <20130610211516.GZ18614@n2100.arm.linux.org.uk> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Jun 10, 2013 at 5:15 PM, Russell King - ARM Linux wrote: > On Mon, Jun 10, 2013 at 05:01:18PM -0400, Rob Clark wrote: >> I would like to get at least some of the driver upstream. I'd hate >> for it to have to live completely out of tree. I can think of a >> couple possibilities. >> >> 1) the best would be, if there was some way for the drm driver to know >> the upper/lower bounds of the carveout, then it could bounds-check >> against this. And then in worst case, userspace can just screw up >> other "graphics" buffers > > The bounds check does what? Check that the buffer object's physical > address is within another driver's range? Fine, but all that it's > doing is preventing it being associated with a buffer object which can > _only_ be used for _read_ access by the LCD controller. There is no > write access to the GEM objects by anything that this DRM driver talks > to. I was assuming there was a global carveout pool, so it would be just one upper/lower check. But from what you said and looking at the allocation code in your driver, I guess this is not actually the case. > So all it'll do is prevent you passing rogue addresses to the LCD > controller for scanout. > > And how do we get that information into the driver from other random > drivers? Hack in random inter-driver specific APIs to do it? Come > up with yet another different way to try and identify whether a > particular resource is a block of reserved memory for this driver's > usage as a linear region or one of the others? How. Really, please > think about the problem. > > The benefit vs the complexity involved really isn't worth it. yeah, that suggestion was based on a wrong assumption about how the phys contig buffers are allocated and passed. So disregard it. I guess in the short term, the best I can think is keep those phys ioctls as a small patch on top of the upstream driver. It is ok to leave place-holder ioctl #'s in the upstream driver so that the ioctl #'s don't shift when you rebase. And then try to get the vendor to add support for dmabuf so that the patch on top of upstream can eventually be dropped. Maybe someone else has a better suggestion, but I don't think we can merge those phys ioctls upstream, and I'd really hate to 'throw the baby out with the bathwater' in this case and not at least get the modesetting part of the driver merged. BR, -R From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rob Clark Subject: Re: [PATCH RFC 2/8] DRM: Armada: Add Armada DRM driver Date: Mon, 10 Jun 2013 18:49:06 -0400 Message-ID: References: <20130609190612.GM18614@n2100.arm.linux.org.uk> <20130610170648.GX18614@n2100.arm.linux.org.uk> <20130610200839.GY18614@n2100.arm.linux.org.uk> <20130610211516.GZ18614@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ie0-f176.google.com (mail-ie0-f176.google.com [209.85.223.176]) by gabe.freedesktop.org (Postfix) with ESMTP id 8408CE5F08 for ; Mon, 10 Jun 2013 15:49:06 -0700 (PDT) Received: by mail-ie0-f176.google.com with SMTP id ar20so15476227iec.21 for ; Mon, 10 Jun 2013 15:49:06 -0700 (PDT) In-Reply-To: <20130610211516.GZ18614@n2100.arm.linux.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Russell King - ARM Linux Cc: Jason Cooper , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, Sebastian Hesselbarth List-Id: dri-devel@lists.freedesktop.org On Mon, Jun 10, 2013 at 5:15 PM, Russell King - ARM Linux wrote: > On Mon, Jun 10, 2013 at 05:01:18PM -0400, Rob Clark wrote: >> I would like to get at least some of the driver upstream. I'd hate >> for it to have to live completely out of tree. I can think of a >> couple possibilities. >> >> 1) the best would be, if there was some way for the drm driver to know >> the upper/lower bounds of the carveout, then it could bounds-check >> against this. And then in worst case, userspace can just screw up >> other "graphics" buffers > > The bounds check does what? Check that the buffer object's physical > address is within another driver's range? Fine, but all that it's > doing is preventing it being associated with a buffer object which can > _only_ be used for _read_ access by the LCD controller. There is no > write access to the GEM objects by anything that this DRM driver talks > to. I was assuming there was a global carveout pool, so it would be just one upper/lower check. But from what you said and looking at the allocation code in your driver, I guess this is not actually the case. > So all it'll do is prevent you passing rogue addresses to the LCD > controller for scanout. > > And how do we get that information into the driver from other random > drivers? Hack in random inter-driver specific APIs to do it? Come > up with yet another different way to try and identify whether a > particular resource is a block of reserved memory for this driver's > usage as a linear region or one of the others? How. Really, please > think about the problem. > > The benefit vs the complexity involved really isn't worth it. yeah, that suggestion was based on a wrong assumption about how the phys contig buffers are allocated and passed. So disregard it. I guess in the short term, the best I can think is keep those phys ioctls as a small patch on top of the upstream driver. It is ok to leave place-holder ioctl #'s in the upstream driver so that the ioctl #'s don't shift when you rebase. And then try to get the vendor to add support for dmabuf so that the patch on top of upstream can eventually be dropped. Maybe someone else has a better suggestion, but I don't think we can merge those phys ioctls upstream, and I'd really hate to 'throw the baby out with the bathwater' in this case and not at least get the modesetting part of the driver merged. BR, -R