Thanks for reviewing, just back from vacation and send out V3 for review.
I split it to 2 patches now and I removed lazy unmap since it's unnecessary.
On Wed, Feb 21, 2018 at 7:42 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> HI Lepton,
>
> It seems that this has fallen through the cracks.
>
> There's one important functionality change which should be split out
> and documented.
> The rest is just style nitpicks.
>
> On 28 December 2017 at 07:35, Lepton Wu <lep...@chromium.org> wrote:
>> v2: address comments from Tomasz Figa
>> a) Add more check for plane size.
>> b) Avoid duplicated mapping and leaked mapping.
>> c) Other minor changes.
> Normally I omit saying "other minor changes" since it don't mean anything.
>
>
>> @@ -105,6 +114,42 @@ kms_sw_is_displaytarget_format_supported( struct
>> sw_winsys *ws,
>> return TRUE;
>> }
>>
>> +static struct kms_sw_plane *get_plane(struct kms_sw_displaytarget
>> *kms_sw_dt,
>> + enum pipe_format format,
>> + unsigned width, unsigned height,
>> + unsigned stride, unsigned offset) {
> Opening bracket should be at the start of next line.
>
>> + struct kms_sw_plane * tmp, * plane = NULL;
> Through the patch: no space between * and variable name - example: s/*
> tmp/*tmp/
>
> Add empty line between declarations and code.
>
>> + if (offset + util_format_get_2d_size(format, stride, height) >
>> + kms_sw_dt->size) {
>> + DEBUG_PRINT("KMS-DEBUG: plane too big. format: %d stride: %d height:
>> %d "
>> + "offset: %d size:%d\n", format, stride, height, offset,
>> + kms_sw_dt->size);
>> + return NULL;
>> + }
>> + LIST_FOR_EACH_ENTRY(tmp, &kms_sw_dt->planes, link) {
>> + if (tmp->offset == offset) {
>> + plane = tmp;
>> + break;
>> + }
>> + }
>> + if (plane) {
>> + assert(plane->width == width);
>> + assert(plane->height == height);
>> + assert(plane->stride == stride);
>> + assert(plane->dt == kms_sw_dt);
> The only way to hit this if there's serious corruption happening. I'd
> just drop it and "return tmp;" in the loop above.
>
>> + } else {
>> + plane = CALLOC_STRUCT(kms_sw_plane);
>> + if (plane == NULL) return NULL;
> The return statement should be on separate line.
>
> Thorough the patch: swap "foo == NULL" with "!foo"
>
>> @@ -138,17 +182,19 @@ kms_sw_displaytarget_create(struct sw_winsys *ws,
>
>> - *stride = kms_sw_dt->stride;
>> - return (struct sw_displaytarget *)kms_sw_dt;
>> -
>> + *stride = create_req.pitch;
>> + return (struct sw_displaytarget *) plane;
> Swap the cast with an inline wrapper analogous to the kms_sw_plane() one.
>
>
>> @@ -163,13 +209,19 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
>> struct sw_displaytarget *dt)
>> {
>> struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
>> - struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
>> + struct kms_sw_plane *plane = kms_sw_plane(dt);
>> + struct kms_sw_displaytarget *kms_sw_dt = plane->dt;
>> struct drm_mode_destroy_dumb destroy_req;
>>
>> kms_sw_dt->ref_count --;
>> if (kms_sw_dt->ref_count > 0)
>> return;
>>
>> + if (kms_sw_dt->ro_mapped)
>> + munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
>> + if (kms_sw_dt->mapped)
>> + munmap(kms_sw_dt->mapped, kms_sw_dt->size);
>> +
> This hunk moves the munmap from dt_unmap to dt_destroy.
> It should be safe, although it is a subtle functionality change that
> should be split out.
> A commit message should explain why it's needed, etc.
>
> Ideally, the ro_mapped introduction will be another patch.
> Alternatively the commit message should mention it.
>
>
>> @@ -198,16 +255,20 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
>> return NULL;
>>
>> prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ |
>> PROT_WRITE);
>> - kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
>> - kms_sw->fd, map_req.offset);
>> -
>> - if (kms_sw_dt->mapped == MAP_FAILED)
>> - return NULL;
>> + void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped :
>> &kms_sw_dt->mapped;
>> + if (*ptr == NULL) {
> To prevent interment breakage, this NULL check must be part of the
> delayer munmap patch.
>
> HTH
> Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
