[PATCH 0/7] Consider G2D hardware restrictions.

[Inki Dae]

Hi Rob,


2013/3/19 Rob Clark <robdclark at gmail.com>:
> On Mon, Mar 18, 2013 at 9:32 PM, YoungJun Cho <yj44.cho at samsung.com> wrote:
>>
>> On Mar 19, 2013 9:55 AM, "Rob Clark" <robdclark at gmail.com> wrote:
>>>
>>> On Mon, Mar 18, 2013 at 8:00 PM, YoungJun Cho <yj44.cho at samsung.com>
>>> wrote:
>>> >
>>> > On Mar 19, 2013 3:01 AM, "Rob Clark" <robdclark at gmail.com> wrote:
>>> >>
>>> >> Btw, what is the hw response to invalid input (ie. bottom>top, invalid
>>> >> size, etc)?
>>> >>
>>> >
>>> > Unfortunately the IOMMU page fault is happened. So we need some codes
>>> > for
>>> > protecting kernel.
>>>
>>> hmm, a page fault is not necessarily a problem.. do you have some way
>>> to know which userspace client the the fault is associated with (I
>>> assume so, unless you have some way to have multiple contexts active
>>> at one time), and some sane way to recover?
>>>
>>
>> When IOMMU page fault is occured, kernel oops is generated now because it is
>> unrecoverable.
>
> oh, ouch.. maybe there is someway to reset the 2d core, and move on?
> If no, then I guess cmdstream checking is required.  If there is some
> way to recover, that seems like a generally good thing to implement..
>

Right, the g2d core could be reset so I think it's possible to recover
the g2d core at page fault handler. But I think we need invalid
command checking also because with this checking, it could avoid page
fault by invalid commands in advance(g2d dma could access invalid
memory region by invalid commands such as x/y position and
width/height and so on). And the invalid command checking is done in
one pass and has no overhead(maybe so much little??). Anyway now g2d
driver needs its own page fault handler to recover g2d core on the
unpredictable situation(not by invalid commands) and this is a TODO
work. And I know that the unrecoverable issue is not because of
invalid commands and that is a pair issue to HOLD CMD and GCF
interrupt.

Thanks,
Inki Dae

> BR,
> -R
>
>
>>> I only ask this because, for an xorg/exa perspective, you can have a
>>> large # of blits that effect a small # of pixels, so minimizing
>>> per-blit overhead can be important for performance.  (otoh, if you
>>> already have to do some cmdstream checking in the kernel to ensure
>>> security, maybe it doesn't add much overhead for a few extra checks.
>>> Depends on whether it can all be done in one pass and without
>>> additional load (LDR) instructions from the cmdstream buffer (which is
>>> presumably not in a cached buffer)
>>>
>>
>> Right, performance is important.
>> We already have checked all command lists for buffer addresses security and
>> it is done in one pass.
>> And this patch gathers additional data for HW restriction during previous
>> checking routine.
>>
>> Thank you
>> Best regards YJ
>>
>>> BR,
>>> -R
>>>
>>> > Thank you~
>>> > Best regards YJ
>>> >
>>> >> Ie. if it will just ignore the blit or raise an error irq which can be
>>> >> handled sanely, it could be ok to avoid the overhead of the cmdstream
>>> >> checking in the kernel.  The kernel part really just needs to ensure
>>> >> that userspace can't cause security problems (read/write access to
>>> >> non-gfx-buffers, or lock up the system, that sort of thing).  It
>>> >> doesn't need to guarantee that the results are sensible.
>>> >>
>>> >> BR,
>>> >> -R
>>> >>
>>> >> On Wed, Mar 13, 2013 at 5:03 AM, Inki Dae <inki.dae at samsung.com> wrote:
>>> >> > This patch set checks the contents of g2d command list from user
>>> >> > is valid or not according to G2D hardware restrictions. For now,
>>> >> > G2D driver wasn't considered for them properly.
>>> >> >
>>> >> > For this, this patch set includes relevant code cleaups, fixups
>>> >> > and adds a new function to get buffer size to the gem to be
>>> >> > accessed by G2D dma.
>>> >> >
>>> >> > Inki Dae (1):
>>> >> >   drm/exynos: Add a new function to get gem buffer size
>>> >> >
>>> >> > YoungJun Cho (6):
>>> >> >   drm/exynos: Fix error routine to getting dma addr.
>>> >> >   drm/exynos: clear node object type at gem unmap
>>> >> >   drm/exynos: Fix G2D core mulfunctioning issue
>>> >> >   drm/exynos: Clean up some G2D codes for readability
>>> >> >   drm/exynos: Deal with g2d buffer info more efficiently
>>> >> >   drm/exynos: Check g2d cmd list for g2d restrictions
>>> >> >
>>> >> >  drivers/gpu/drm/exynos/exynos_drm_g2d.c |  381
>>> >> > ++++++++++++++++++++++++++-----
>>> >> >  drivers/gpu/drm/exynos/exynos_drm_gem.c |   21 ++
>>> >> >  drivers/gpu/drm/exynos/exynos_drm_gem.h |    5 +
>>> >> >  3 files changed, 349 insertions(+), 58 deletions(-)
>>> >> >
>>> >> > --
>>> >> > 1.7.4.1
>>> >> >
>>> >> > _______________________________________________
>>> >> > dri-devel mailing list
>>> >> > dri-devel at lists.freedesktop.org
>>> >> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> >> _______________________________________________
>>> >> dri-devel mailing list
>>> >> dri-devel at lists.freedesktop.org
>>> >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel