Re: [PATCH] drm/i915/bxt: Fix uninitialized variables in intel_check_sprite_plane

From: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915/bxt: Fix uninitialized variables in intel_check_sprite_plane
Date: Thu, 26 Nov 2015 23:33:43 +0530	[thread overview]
Message-ID: <5657497F.6080608@intel.com> (raw)
In-Reply-To: <20151118172655.GP4437@intel.com>


On 11/18/2015 10:56 PM, Ville Syrjälä wrote:
> On Wed, Nov 18, 2015 at 10:33:55PM +0530, Maiti, Nabendu Bikash wrote:
>>
>> On 11/18/2015 7:00 PM, Ville Syrjälä wrote:
>>> On Wed, Nov 18, 2015 at 06:48:37PM +0530, Maiti, Nabendu Bikash wrote:
>>>>
>>>> On 11/18/2015 6:22 PM, Ville Syrjälä wrote:
>>>>> On Wed, Nov 18, 2015 at 12:19:06PM +0000, Chris Wilson wrote:
>>>>>> On Wed, Nov 18, 2015 at 05:43:52PM +0530, Nabendu Maiti wrote:
>>>>>>> Uninitialized variables (width, Height) in intel_check_sprite_plane
>>>>>>> leads to compilererror in O1 level. Initialize all declared variables
>>>>>>> to fix this issue.
>>>>>>>
>>>>>>> Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
>>>>>> Or perhaps:
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>> index 2b96f336589e..8d7b4eb5b5b9 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>> @@ -747,7 +747,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>>>>>>            struct drm_framebuffer *fb = state->base.fb;
>>>>>>            int crtc_x, crtc_y;
>>>>>>            unsigned int crtc_w, crtc_h;
>>>>>> -       uint32_t src_x, src_y, src_w, src_h;
>>>>>>            struct drm_rect *src = &state->src;
>>>>>>            struct drm_rect *dst = &state->dst;
>>>>>>            const struct drm_rect *clip = &state->clip;
>>>>>> @@ -813,6 +812,8 @@ intel_check_sprite_plane(struct drm_plane *plane,
>>>>>>            crtc_h = drm_rect_height(dst);
>>>>>>     
>>>>>>            if (state->visible) {
>>>>>> +               u32 src_x, src_y, src_w, src_h;
>>>>>> +
>>>>>>                    /* check again in case clipping clamped the results */
>>>>>>                    hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale);
>>>>>>                    if (hscale < 0) {
>>>>>> @@ -871,10 +872,9 @@ intel_check_sprite_plane(struct drm_plane *plane,
>>>>>>                            if (crtc_w == 0)
>>>>>>                                    state->visible = false;
>>>>>>                    }
>>>>>> -       }
>>>>>>     
>>>>>>                    /* Check size restrictions when scaling */
>>>>>> -       if (state->visible && (src_w != crtc_w || src_h != crtc_h)) {
>>>>>> +               if (src_w != crtc_w || src_h != crtc_h) {
>>>>> That would change what it does.
>>>> yes, checked the code where inside each if condition loop we may be
>>>> changing the state->visible var itself. Next condition check it may be
>>>> false too.
>>>>
>>>> The place giving compiler error
>>>>                    src->x1 = src_x << 16;
>>>>                    src->x2 = (src_x + src_w) << 16;
>>>>                    src->y1 = src_y << 16;
>>>>                    src->y2 = (src_y + src_h) << 16;
>>>>
>>>> Then just one line change of initializing the variables is better?
>>> Or maybe fix your compiler instead? I don't get any warning/errors from
>>> this. What version of gcc are you using?
>>    I still get the warning and error if -Werror is enabled, And on
>> Makefile O1 optimization is enabled.
> And why exactly are you building with -O1?
I am using it for platform level debug symbol inclusion in elf which are 
excluded in Os/O2 . We need it.Else it will break generic kernel.
>> (note on Android build by default
>> Werror is enabled and gcc-- GCC: (GNU) 4.9.2
>> )
>>
>> on generic build, gcc --gcc (Ubuntu 4.9.3-5ubuntu1~14.04) 4.9.3
>>
>> following are the error..
>>     CC      drivers/staging/android/timed_gpio.o
>> /home/nbikash/PROJ_CODEBASE/MDsrt_WW44.2/kernel/bxt/drivers/gpu/drm/i915/intel_sprite.c:
>> In function 'intel_check_sprite_plane':
>> /home/nbikash/PROJ_CODEBASE/MDsrt_WW44.2/kernel/bxt/drivers/gpu/drm/i915/intel_sprite.c:1241:20:
>> error: 'src_h' may be used uninitialized in this function
>> [-Werror=maybe-uninitialized]
>>      src->y2 = (src_y + src_h) << 16;
>>                       ^
>> /home/nbikash/PROJ_CODEBASE/MDsrt_WW44.2/kernel/bxt/drivers/gpu/drm/i915/intel_sprite.c:1239:20:
>> error: 'src_w' may be used uninitialized in this function
>> [-Werror=maybe-uninitialized]
>>      src->x2 = (src_x + src_w) << 16;
>>                       ^
>> /home/nbikash/PROJ_CODEBASE/MDsrt_WW44.2/kernel/bxt/drivers/gpu/drm/i915/intel_sprite.c:1241:20:
>> error: 'src_y' may be used uninitialized in this function
>> [-Werror=maybe-uninitialized]
>>      src->y2 = (src_y + src_h) << 16;
>>                       ^
>> /home/nbikash/PROJ_CODEBASE/MDsrt_WW44.2/kernel/bxt/drivers/gpu/drm/i915/intel_sprite.c:1238:19:
>> error: 'src_x' may be used uninitialized in this function
>> [-Werror=maybe-uninitialized]
>>      src->x1 = src_x << 16;
>>>>>>                            unsigned int width_bytes;
>>>>>>     
>>>>>>                            WARN_ON(!can_scale);
>>>>>> @@ -898,7 +898,6 @@ intel_check_sprite_plane(struct drm_plane *plane,
>>>>>>                            }
>>>>>>                    }
>>>>>>     
>>>>>> -       if (state->visible) {
>>>>>>                    src->x1 = src_x << 16;
>>>>>>                    src->x2 = (src_x + src_w) << 16;
>>>>>>                    src->y1 = src_y << 16;
>>>>>>
>>>>>> And make both the compiler and reader happier
>>>>>> -Chris
>>>>>>
>>>>>> -- 
>>>>>> Chris Wilson, Intel Open Source Technology Centre
>>>>>> _______________________________________________
>>>>>> Intel-gfx mailing list
>>>>>> Intel-gfx@lists.freedesktop.org
>>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx