All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@oracle.com>
To: contact@emersion.fr
Cc: amd-gfx@lists.freedesktop.org
Subject: [bug report] drm/amd/display: use FB pitch to fill dc_cursor_attributes
Date: Tue, 26 Jul 2022 18:20:13 +0300	[thread overview]
Message-ID: <YuAGLYvbtrrgBH0U@kili> (raw)

Hello Simon Ser,

The patch 03a663673063: "drm/amd/display: use FB pitch to fill
dc_cursor_attributes" from Dec 2, 2020, leads to the following Smatch
static checker warning:

	drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_plane.c:1271 handle_cursor_update()
	error: we previously assumed 'afb' could be null (see line 1230)

drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_plane.c
    1222 void handle_cursor_update(struct drm_plane *plane,
    1223                                  struct drm_plane_state *old_plane_state)
    1224 {
    1225         struct amdgpu_device *adev = drm_to_adev(plane->dev);
    1226         struct amdgpu_framebuffer *afb = to_amdgpu_framebuffer(plane->state->fb);
    1227         struct drm_crtc *crtc = afb ? plane->state->crtc : old_plane_state->crtc;

afb is container_of() but it's basically a cast in this case.  I would
always prefer to check "plane->state->fb" for NULL instead of the
container_of(), but that's sort of a style debate, I guess.  Some people
really like checking the returned pointer and add build time asserts to
ensure that the container_of() is a no-op.

    1228         struct dm_crtc_state *crtc_state = crtc ? to_dm_crtc_state(crtc->state) : NULL;
    1229         struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
    1230         uint64_t address = afb ? afb->address : 0;
    1231         struct dc_cursor_position position = {0};
    1232         struct dc_cursor_attributes attributes;
    1233         int ret;
    1234 
    1235         if (!plane->state->fb && !old_plane_state->fb)
    1236                 return;
    1237 
    1238         DC_LOG_CURSOR("%s: crtc_id=%d with size %d to %d\n",
    1239                       __func__,
    1240                       amdgpu_crtc->crtc_id,
    1241                       plane->state->crtc_w,
    1242                       plane->state->crtc_h);
    1243 
    1244         ret = get_cursor_position(plane, crtc, &position);
    1245         if (ret)
    1246                 return;
    1247 
    1248         if (!position.enable) {
    1249                 /* turn off cursor */
    1250                 if (crtc_state && crtc_state->stream) {
    1251                         mutex_lock(&adev->dm.dc_lock);
    1252                         dc_stream_set_cursor_position(crtc_state->stream,
    1253                                                       &position);
    1254                         mutex_unlock(&adev->dm.dc_lock);
    1255                 }
    1256                 return;
    1257         }
    1258 
    1259         amdgpu_crtc->cursor_width = plane->state->crtc_w;
    1260         amdgpu_crtc->cursor_height = plane->state->crtc_h;
    1261 
    1262         memset(&attributes, 0, sizeof(attributes));
    1263         attributes.address.high_part = upper_32_bits(address);
    1264         attributes.address.low_part  = lower_32_bits(address);
    1265         attributes.width             = plane->state->crtc_w;
    1266         attributes.height            = plane->state->crtc_h;
    1267         attributes.color_format      = CURSOR_MODE_COLOR_PRE_MULTIPLIED_ALPHA;
    1268         attributes.rotation_angle    = 0;
    1269         attributes.attribute_flags.value = 0;
    1270 
--> 1271         attributes.pitch = afb->base.pitches[0] / afb->base.format->cpp[0];
                                    ^^^^^                  ^^^^^
Unchecked dereferences.

    1272 
    1273         if (crtc_state->stream) {
    1274                 mutex_lock(&adev->dm.dc_lock);
    1275                 if (!dc_stream_set_cursor_attributes(crtc_state->stream,
    1276                                                          &attributes))
    1277                         DRM_ERROR("DC failed to set cursor attributes\n");
    1278 
    1279                 if (!dc_stream_set_cursor_position(crtc_state->stream,
    1280                                                    &position))
    1281                         DRM_ERROR("DC failed to set cursor position\n");
    1282                 mutex_unlock(&adev->dm.dc_lock);
    1283         }
    1284 }

regards,
dan carpenter

             reply	other threads:[~2022-07-26 15:20 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-26 15:20 Dan Carpenter [this message]
2022-07-26 15:27 ` [bug report] drm/amd/display: use FB pitch to fill dc_cursor_attributes Simon Ser
2022-07-26 15:47   ` Dan Carpenter
2022-07-26 17:16     ` Simon Ser
  -- strict thread matches above, loose matches on Subject: below --
2020-12-07 14:51 Dan Carpenter
2020-12-07 14:53 ` Simon Ser
2020-12-07 17:51   ` Dan Carpenter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YuAGLYvbtrrgBH0U@kili \
    --to=dan.carpenter@oracle.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=contact@emersion.fr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.