lkml.org 
[lkml]   [2019]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] drm/modes: remove unused variables
    On Thu, Dec 05, 2019 at 10:23:51AM +0000, Benjamin GAIGNARD wrote:
    > + Zhao Yakui
    >
    > On 12/5/19 10:55 AM, Jani Nikula wrote:
    > > On Wed, 04 Dec 2019, Benjamin GAIGNARD <benjamin.gaignard@st.com> wrote:
    > >> On 12/4/19 10:35 AM, Thomas Zimmermann wrote:
    > >>> Hi
    > >>>
    > >>> Am 19.11.19 um 14:47 schrieb Benjamin Gaignard:
    > >>>> When compiling with W=1 few warnings about unused variables show up.
    > >>>> This patch removes all the involved variables.
    > >>>>
    > >>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
    > >>>> ---
    > >>>> drivers/gpu/drm/drm_modes.c | 22 +++-------------------
    > >>>> 1 file changed, 3 insertions(+), 19 deletions(-)
    > >>>>
    > >>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
    > >>>> index 88232698d7a0..aca901aff042 100644
    > >>>> --- a/drivers/gpu/drm/drm_modes.c
    > >>>> +++ b/drivers/gpu/drm/drm_modes.c
    > >>>> @@ -233,7 +233,7 @@ struct drm_display_mode *drm_cvt_mode(struct drm_device *dev, int hdisplay,
    > >>>> /* 3) Nominal HSync width (% of line period) - default 8 */
    > >>>> #define CVT_HSYNC_PERCENTAGE 8
    > >>>> unsigned int hblank_percentage;
    > >>>> - int vsyncandback_porch, vback_porch, hblank;
    > >>>> + int vsyncandback_porch, hblank;
    > >>>>
    > >>>> /* estimated the horizontal period */
    > >>>> tmp1 = HV_FACTOR * 1000000 -
    > >>>> @@ -249,7 +249,6 @@ struct drm_display_mode *drm_cvt_mode(struct drm_device *dev, int hdisplay,
    > >>>> else
    > >>>> vsyncandback_porch = tmp1;
    > >>>> /* 10. Find number of lines in back porch */
    > >>>> - vback_porch = vsyncandback_porch - vsync;
    > >>>> drm_mode->vtotal = vdisplay_rnd + 2 * vmargin +
    > >>>> vsyncandback_porch + CVT_MIN_V_PORCH;
    > >>>> /* 5) Definition of Horizontal blanking time limitation */
    > >>>> @@ -386,9 +385,8 @@ drm_gtf_mode_complex(struct drm_device *dev, int hdisplay, int vdisplay,
    > >>>> int top_margin, bottom_margin;
    > >>>> int interlace;
    > >>>> unsigned int hfreq_est;
    > >>>> - int vsync_plus_bp, vback_porch;
    > >>>> - unsigned int vtotal_lines, vfieldrate_est, hperiod;
    > >>>> - unsigned int vfield_rate, vframe_rate;
    > >>>> + int vsync_plus_bp;
    > >>>> + unsigned int vtotal_lines;
    > >>>> int left_margin, right_margin;
    > >>>> unsigned int total_active_pixels, ideal_duty_cycle;
    > >>>> unsigned int hblank, total_pixels, pixel_freq;
    > >>>> @@ -451,23 +449,9 @@ drm_gtf_mode_complex(struct drm_device *dev, int hdisplay, int vdisplay,
    > >>>> /* [V SYNC+BP] = RINT(([MIN VSYNC+BP] * hfreq_est / 1000000)) */
    > >>>> vsync_plus_bp = MIN_VSYNC_PLUS_BP * hfreq_est / 1000;
    > >>>> vsync_plus_bp = (vsync_plus_bp + 500) / 1000;
    > >>>> - /* 9. Find the number of lines in V back porch alone: */
    > >>>> - vback_porch = vsync_plus_bp - V_SYNC_RQD;
    > >>>> /* 10. Find the total number of lines in Vertical field period: */
    > >>>> vtotal_lines = vdisplay_rnd + top_margin + bottom_margin +
    > >>>> vsync_plus_bp + GTF_MIN_V_PORCH;
    > >>>> - /* 11. Estimate the Vertical field frequency: */
    > >>>> - vfieldrate_est = hfreq_est / vtotal_lines;
    > >>>> - /* 12. Find the actual horizontal period: */
    > >>>> - hperiod = 1000000 / (vfieldrate_rqd * vtotal_lines);
    > >>>> -
    > >>>> - /* 13. Find the actual Vertical field frequency: */
    > >>>> - vfield_rate = hfreq_est / vtotal_lines;
    > >>>> - /* 14. Find the Vertical frame frequency: */
    > >>>> - if (interlaced)
    > >>>> - vframe_rate = vfield_rate / 2;
    > >>>> - else
    > >>>> - vframe_rate = vfield_rate;
    > >>> The amount of unused code is quite large, which makes me wonder if
    > >>> there's something missing below where these variables are supposed to be
    > >>> used.
    > >>>
    > >>> If these variables can be removed, comments should mention that steps 9
    > >>> and 11 to 14 are being left out. After all, the function is fairly
    > >>> explicit about implementing the GTF algorithm step by step.
    > >>>
    > >>> Best regards
    > >>> Thomas
    > >> If the goal is to keep all the steps then I could prefix all problematic
    > >> variables with __maybe_unused macro.
    > > The effect is the same; it hides a potential bug that should be analyzed
    > > and fixed. If you have the time, please look at the code and figure out
    > > what it's supposed to do, and why isn't it using the information. Look
    > > at git blame and log, was it always so, or did something change?
    > >
    > > The warnings are about potential bugs. The objective or end goal is to
    > > fix the bugs, not to silence the warnings.
    > This code haven't change since it has been added by commit:
    > 26bbdadad356e ("drm/mode: add the GTF algorithm in kernel space")
    > The variables that I'm removing are not used anywhere else.
    > The algorithm is copy from xserver/hw/xfree86/modes/xf86gtf.c where
    > vframe_rate and v_back_porch are used with (void) calls:
    > (void) v_back_porch;
    > (void) v_frame_rate;
    > It is another way avoid the warnings.
    > Note that if you start removing v_frame_rate then vfield_rate becomes
    > unused, etc...

    I'd say we should do the same thing then as xf86 and just mark them up as
    unused. Might be good to double-check with the spec at least whether
    they're really not unused, or whether we have some issue. But given that
    it seems to have worked for ages, I suspect it's all good.

    Removing the code otoh feels a bit icky.
    -Daniel


    >
    > Benjamin
    >
    > >
    > > BR,
    > > Jani.
    > >
    > >
    > >> Benjamin
    > >>
    > >>>> /* 15. Find number of pixels in left margin: */
    > >>>> if (margins)
    > >>>> left_margin = (hdisplay_rnd * GTF_MARGIN_PERCENTAGE + 500) /
    > >>>>
    > >> _______________________________________________
    > >> dri-devel mailing list
    > >> dri-devel@lists.freedesktop.org
    > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel

    --
    Daniel Vetter
    Software Engineer, Intel Corporation
    http://blog.ffwll.ch

    \
     
     \ /
      Last update: 2019-12-10 10:53    [W:4.548 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site