Re: [bug report] drm/panthor: Add the scheduler logical block
On 10/04/2024 15:34, Dan Carpenter wrote:
> On Wed, Apr 10, 2024 at 03:11:52PM +0100, Steven Price wrote:
>> On 08/04/2024 08:35, Dan Carpenter wrote:
>>> Hello Boris Brezillon,
>>>
>>> Commit de8548813824 ("drm/panthor: Add the scheduler logical block")
>>> from Feb 29, 2024 (linux-next), leads to the following Smatch static
>>> checker warning:
>>>
>>> drivers/gpu/drm/panthor/panthor_sched.c:1153
>>> csg_slot_sync_state_locked()
>>> error: uninitialized symbol 'new_state'.
>>>
>>> drivers/gpu/drm/panthor/panthor_sched.c
>>> 1123 static void
>>> 1124 csg_slot_sync_state_locked(struct panthor_device *ptdev, u32
>>> csg_id)
>>> 1125 {
>>> 1126 struct panthor_csg_slot *csg_slot =
>>> &ptdev->scheduler->csg_slots[csg_id];
>>> 1127 struct panthor_fw_csg_iface *csg_iface;
>>> 1128 struct panthor_group *group;
>>> 1129 enum panthor_group_state new_state, old_state;
>>> 1130
>>> 1131 lockdep_assert_held(&ptdev->scheduler->lock);
>>> 1132
>>> 1133 csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
>>> 1134 group = csg_slot->group;
>>> 1135
>>> 1136 if (!group)
>>> 1137 return;
>>> 1138
>>> 1139 old_state = group->state;
>>> 1140 switch (csg_iface->output->ack & CSG_STATE_MASK) {
>>> ^^
>>> This mask is 0x7. Should it be 0x3? That would silence the static
>>> checker warning.
>>
>> The mask is correct - it's effectively a hardware register (well it's
>> read/written by the firmware on the hardware). States 4-7 are officially
>> "reserved" and Should Not Happen™!
>>
>> I guess a "default:" case with a WARN_ON() would be the right solution.
>>
>> Steve
>
> A WARN_ON() won't silence the warning. Plus WARN_ON() is not free in
> terms of memory usage. And they're kind of controversial these days to
> be honest.
Sorry, I didn't mean just a WARN_ON() - there should also be an early
return from the function, ideally with the GPU being reset in the hope
that it starts behaving again.
And you're probably right WARN_ON is a bit too strong, we should
definitely output a warning message though to point out that the
hardware isn't behaving as expected.
> One solution would be to just ignore the static checker warning. These
> are a one time thing, and if people have questions in the future, they
> can just search lore for this thread.
Well the static checker is not wrong - but the situation where this
could happen is if the hardware (or firmware running on the hardware)
breaks the contract of the specification. I'll put it on my todo list to
take a look at handling this more gracefully, but it's likely to take a
while before it bubbles to the top. But thanks for pointing out the issue.
Steve
Re: [bug report] drm/panthor: Add the scheduler logical block
On Wed, Apr 10, 2024 at 03:11:52PM +0100, Steven Price wrote:
> On 08/04/2024 08:35, Dan Carpenter wrote:
> > Hello Boris Brezillon,
> >
> > Commit de8548813824 ("drm/panthor: Add the scheduler logical block")
> > from Feb 29, 2024 (linux-next), leads to the following Smatch static
> > checker warning:
> >
> > drivers/gpu/drm/panthor/panthor_sched.c:1153
> > csg_slot_sync_state_locked()
> > error: uninitialized symbol 'new_state'.
> >
> > drivers/gpu/drm/panthor/panthor_sched.c
> > 1123 static void
> > 1124 csg_slot_sync_state_locked(struct panthor_device *ptdev, u32
> > csg_id)
> > 1125 {
> > 1126 struct panthor_csg_slot *csg_slot =
> > &ptdev->scheduler->csg_slots[csg_id];
> > 1127 struct panthor_fw_csg_iface *csg_iface;
> > 1128 struct panthor_group *group;
> > 1129 enum panthor_group_state new_state, old_state;
> > 1130
> > 1131 lockdep_assert_held(&ptdev->scheduler->lock);
> > 1132
> > 1133 csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> > 1134 group = csg_slot->group;
> > 1135
> > 1136 if (!group)
> > 1137 return;
> > 1138
> > 1139 old_state = group->state;
> > 1140 switch (csg_iface->output->ack & CSG_STATE_MASK) {
> > ^^
> > This mask is 0x7. Should it be 0x3? That would silence the static
> > checker warning.
>
> The mask is correct - it's effectively a hardware register (well it's
> read/written by the firmware on the hardware). States 4-7 are officially
> "reserved" and Should Not Happen™!
>
> I guess a "default:" case with a WARN_ON() would be the right solution.
>
> Steve
A WARN_ON() won't silence the warning. Plus WARN_ON() is not free in
terms of memory usage. And they're kind of controversial these days to
be honest.
One solution would be to just ignore the static checker warning. These
are a one time thing, and if people have questions in the future, they
can just search lore for this thread.
regards,
dan carpenter
Re: [bug report] drm/panthor: Add the scheduler logical block
On 08/04/2024 08:35, Dan Carpenter wrote:
> Hello Boris Brezillon,
>
> Commit de8548813824 ("drm/panthor: Add the scheduler logical block")
> from Feb 29, 2024 (linux-next), leads to the following Smatch static
> checker warning:
>
> drivers/gpu/drm/panthor/panthor_sched.c:1153
> csg_slot_sync_state_locked()
> error: uninitialized symbol 'new_state'.
>
> drivers/gpu/drm/panthor/panthor_sched.c
> 1123 static void
> 1124 csg_slot_sync_state_locked(struct panthor_device *ptdev, u32 csg_id)
> 1125 {
> 1126 struct panthor_csg_slot *csg_slot =
> &ptdev->scheduler->csg_slots[csg_id];
> 1127 struct panthor_fw_csg_iface *csg_iface;
> 1128 struct panthor_group *group;
> 1129 enum panthor_group_state new_state, old_state;
> 1130
> 1131 lockdep_assert_held(&ptdev->scheduler->lock);
> 1132
> 1133 csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> 1134 group = csg_slot->group;
> 1135
> 1136 if (!group)
> 1137 return;
> 1138
> 1139 old_state = group->state;
> 1140 switch (csg_iface->output->ack & CSG_STATE_MASK) {
> ^^
> This mask is 0x7. Should it be 0x3? That would silence the static
> checker warning.
The mask is correct - it's effectively a hardware register (well it's
read/written by the firmware on the hardware). States 4-7 are officially
"reserved" and Should Not Happen™!
I guess a "default:" case with a WARN_ON() would be the right solution.
Steve
> 1141 case CSG_STATE_START:
> 1142 case CSG_STATE_RESUME:
> 1143 new_state = PANTHOR_CS_GROUP_ACTIVE;
> 1144 break;
> 1145 case CSG_STATE_TERMINATE:
> 1146 new_state = PANTHOR_CS_GROUP_TERMINATED;
> 1147 break;
> 1148 case CSG_STATE_SUSPEND:
> 1149 new_state = PANTHOR_CS_GROUP_SUSPENDED;
> 1150 break;
> 1151 }
> 1152
> --> 1153 if (old_state == new_state)
> 1154 return;
> 1155
> 1156 if (new_state == PANTHOR_CS_GROUP_SUSPENDED)
> 1157 csg_slot_sync_queues_state_locked(ptdev, csg_id);
> 1158
> 1159 if (old_state == PANTHOR_CS_GROUP_ACTIVE) {
> 1160 u32 i;
> 1161
> 1162 /* Reset the queue slots so we start from a clean
> 1163 * state when starting/resuming a new group on this
> 1164 * CSG slot. No wait needed here, and no ringbell
> 1165 * either, since the CS slot will only be re-used
> 1166 * on the next CSG start operation.
> 1167 */
> 1168 for (i = 0; i < group->queue_count; i++) {
> 1169 if (group->queues[i])
> 1170 cs_slot_reset_locked(ptdev, csg_id,
> i);
> 1171 }
> 1172 }
> 1173
> 1174 group->state = new_state;
> 1175 }
>
> regards,
> dan carpenter
[bug report] drm/panthor: Add the scheduler logical block
Hello Boris Brezillon,
Commit de8548813824 ("drm/panthor: Add the scheduler logical block")
from Feb 29, 2024 (linux-next), leads to the following Smatch static
checker warning:
drivers/gpu/drm/panthor/panthor_sched.c:1153
csg_slot_sync_state_locked()
error: uninitialized symbol 'new_state'.
drivers/gpu/drm/panthor/panthor_sched.c
1123 static void
1124 csg_slot_sync_state_locked(struct panthor_device *ptdev, u32 csg_id)
1125 {
1126 struct panthor_csg_slot *csg_slot =
&ptdev->scheduler->csg_slots[csg_id];
1127 struct panthor_fw_csg_iface *csg_iface;
1128 struct panthor_group *group;
1129 enum panthor_group_state new_state, old_state;
1130
1131 lockdep_assert_held(&ptdev->scheduler->lock);
1132
1133 csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
1134 group = csg_slot->group;
1135
1136 if (!group)
1137 return;
1138
1139 old_state = group->state;
1140 switch (csg_iface->output->ack & CSG_STATE_MASK) {
^^
This mask is 0x7. Should it be 0x3? That would silence the static
checker warning.
1141 case CSG_STATE_START:
1142 case CSG_STATE_RESUME:
1143 new_state = PANTHOR_CS_GROUP_ACTIVE;
1144 break;
1145 case CSG_STATE_TERMINATE:
1146 new_state = PANTHOR_CS_GROUP_TERMINATED;
1147 break;
1148 case CSG_STATE_SUSPEND:
1149 new_state = PANTHOR_CS_GROUP_SUSPENDED;
1150 break;
1151 }
1152
--> 1153 if (old_state == new_state)
1154 return;
1155
1156 if (new_state == PANTHOR_CS_GROUP_SUSPENDED)
1157 csg_slot_sync_queues_state_locked(ptdev, csg_id);
1158
1159 if (old_state == PANTHOR_CS_GROUP_ACTIVE) {
1160 u32 i;
1161
1162 /* Reset the queue slots so we start from a clean
1163 * state when starting/resuming a new group on this
1164 * CSG slot. No wait needed here, and no ringbell
1165 * either, since the CS slot will only be re-used
1166 * on the next CSG start operation.
1167 */
1168 for (i = 0; i < group->queue_count; i++) {
1169 if (group->queues[i])
1170 cs_slot_reset_locked(ptdev, csg_id, i);
1171 }
1172 }
1173
1174 group->state = new_state;
1175 }
regards,
dan carpenter
