On Wed, 26 Feb 2025, "Andy Yan" <andyshrk@xxxxxxx> wrote: > Hi， > > 在 2025-02-26 16:33:21，"Thomas Zimmermann" <tzimmermann@xxxxxxx> 写道： >>Hi >> >>Am 25.01.25 um 04:53 schrieb Andy Yan: >>> >>> 在 2025-01-24 19:43:07，"Jani Nikula" <jani.nikula@xxxxxxxxx> 写道： >>>> On Fri, 24 Jan 2025, "Andy Yan" <andyshrk@xxxxxxx> wrote: >>>>> Hi， >>>>> >>>>> At 2025-01-23 23:09:09, "Jani Nikula" <jani.nikula@xxxxxxxxx> wrote: >>>>>> The expectation is that the struct drm_device based logging helpers get >>>>>> passed an actual struct drm_device pointer rather than some random >>>>>> struct pointer where you can dereference the ->dev member. >>>>>> >>>>>> Convert drm_err(hdmi, ...) to dev_err(hdmi->dev, ...). This matches >>>>>> current usage, but drops "[drm] *ERROR*" prefix from logging. >>>>> Frankly, I prefer the original version of the log. >>>>> It is a platform driver, so it should use its own device. >>>>> It is a driver that works in drm subsystem, so it's better to use "[drm] *ERROR*" prefix when logging >>>> If you need to do struct device based logging that is not the same >>>> device as the struct drm_device dev member, you need to use dev_err() >>>> and friends. You can't and must not use drm_err() and friends. >>>> >>>> It's as simple as that. >>>> >>>> The current drm_err(hdmi, ...) usage is simply abuse of the macros, and >>>> must stop. >>> Perhaps when you initially designed this macros, you intended it to accept only drm_device, >>> but your code implementation didn't enforce this restriction at the beginning. >> >>C'mon. Are we really arguing about type safety now? >> >>Patch 5 adds a little getter function that does the type check on the >>function call's argument. >> >> >>> If that's truly what you intended, I suggest just reverting this commit that converting to use these macros[0], >>> as neither drm_err nor dev_err can maintain consistency with the original log of this driver. >>> Alternatively, as suggested by Sam in the initial submission of your patch 5 years ago, >>> there should also be a macro similar to drm_dev_info(device *, ..).[1] >> >>DRM code tends to keep a reference to the drm_device somewhere and >>fetches it if necessary. For this patch, it should be possible to fetch >>the DRM device from struct rockchip_hdmi easily. Just do >> >> drm_err(rockchip_hdmi_drm_dev(hdmi), "..."); >> >>This would resolve the problem without new logging functions and keep >>the "[drm]" prefix to the messages. > > Yes, this keeps the "[drm]" prefix to the log messages, but it also changed hdmi > device from drm device in the log messages. > For more efficient debugging, it is preferable for log entries to explicitly specify which device generated them, > especially in DRM systems where multiple devices(bridge/encoder) may be present." > > And I'm ok with this PATCH. However, I would also like to know your and Jani's opinions on whether we can consider > adding an API similar to drm_dev_info，as Sam suggested before. Of course, this could be left for future implementation > by others. I'm not at all thrilled about adding loads of variations of drm specific debug macros. They just multiply. The way around that would be to use _Generic() to allow passing either struct drm_device (which would use drm->dev) or a more specific struct device to be used for dev based logging. I'd be supportive of that. However, this patch series is a dependency for implementing generics at a later time. I'd like to start here instead of piling on more work. Furthermore, there's been talk about drm display object based logging. The generics could later be expanded to allow for example: drm_err(connector, "Fail\n"); to translate to: dev_err(connector->dev->dev, "[CONNECTOR:%d:%s] Fail\n", connector->base.id, connector->name); This would simplify a lot of connector based logging. Similarly for other objects. BR, Jani. -- Jani Nikula, Intel
