From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH 02/10] drm/radeon: UVD bringup v7 Date: Wed, 03 Apr 2013 17:53:55 +0200 Message-ID: <515C5093.9030709@vodafone.de> References: <1364944719-5175-1-git-send-email-deathsimple@vodafone.de> <1364944719-5175-3-git-send-email-deathsimple@vodafone.de> <20130403145323.GC2010@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from outgoing.email.vodafone.de (outgoing.email.vodafone.de [139.7.28.128]) by gabe.freedesktop.org (Postfix) with ESMTP id F0C85E5D32 for ; Wed, 3 Apr 2013 08:53:58 -0700 (PDT) In-Reply-To: <20130403145323.GC2010@gmail.com> 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: Jerome Glisse Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org Am 03.04.2013 16:53, schrieb Jerome Glisse: > On Wed, Apr 03, 2013 at 01:18:31AM +0200, Christian K=F6nig wrote: >> [SNIP] >> >> /* hardcode those limit for now */ >> #define RADEON_VA_IB_OFFSET (1 << 20) >> #define RADEON_VA_RESERVED_SIZE (8 << 20) >> @@ -357,8 +360,9 @@ struct radeon_bo_list { >> struct ttm_validate_buffer tv; >> struct radeon_bo *bo; >> uint64_t gpu_offset; >> - unsigned rdomain; >> - unsigned wdomain; >> + bool written; >> + unsigned domain; >> + unsigned alt_domain; >> u32 tiling_flags; >> }; > I think that the change to the rdomain/wdomain should be in a patch > of its own. I think the change is fine but we had issue with change > that touched that part previously, would make bisecting and > understanding the change implication easier. Agree, I actually planed to do so, but for the whole IP review stuff we = needed to maintain a more or less stable patch base. Long story, but I'm = going to change it. >> = >> @@ -826,7 +830,6 @@ struct radeon_cs_reloc { >> struct radeon_bo *robj; >> struct radeon_bo_list lobj; >> uint32_t handle; >> - uint32_t flags; >> }; > Why removing the flags ? iirc it's not really use right now but i > remember plan to use it. Ups, just a rebasing artifact. But when it's unused we should remove it, = probably just not in this patch. >> = >> [SNIP] >> >> +static int radeon_uvd_cs_reloc(struct radeon_cs_parser *p, int data0, i= nt data1) >> +{ >> + struct radeon_cs_chunk *relocs_chunk; >> + struct radeon_cs_reloc *reloc; >> + unsigned idx, cmd; >> + uint64_t start, end; >> + >> + relocs_chunk =3D &p->chunks[p->chunk_relocs_idx]; >> + idx =3D radeon_get_ib_value(p, data1); >> + if (idx >=3D relocs_chunk->length_dw) { >> + DRM_ERROR("Relocs at %d after relocations chunk end %d !\n", >> + idx, relocs_chunk->length_dw); >> + return -EINVAL; >> + } >> + >> + reloc =3D p->relocs_ptr[(idx / 4)]; >> + start =3D reloc->lobj.gpu_offset; >> + end =3D start + radeon_bo_size(reloc->robj); >> + start +=3D radeon_get_ib_value(p, data0); > I am assuming there is no way for you to know the size that the uvd engin= e will write to ? > You are not checking anything on uvd possibly overwritting after the bo e= nd. Yeah that gave me headache for a quite long time, too. The problem is to = figure out how much is actually written you need to keep track of the = whole lot of informations including the UVD session, = create/decode/destroy messages and allot of fiddling with the codec = specific parameters. And if I understand the UVD internals correctly even if we check = everything there is no guarantee that a special crafted bitstream could = not let UVD to write over the end of the buffer.... Is it ok if we but a big TODO on it for the initial patch? Cheers, Christian.