On Tue, 5 Dec 2023 at 13:48, Maxime Ripard <mripard@xxxxxxxxxx> wrote: > > On Tue, Dec 05, 2023 at 03:37:15AM +0200, Dmitry Baryshkov wrote: > > On 04/12/2023 10:38, Maxime Ripard wrote: > > > On Sat, Dec 02, 2023 at 12:07:49AM +0200, Dmitry Baryshkov wrote: > > > > The drm_atomic_helper_check_wb_encoder_state() function doesn't use > > > > encoder for anything other than getting the drm_device instance. The > > > > function's description talks about checking the writeback connector > > > > state, not the encoder state. Moreover, there is no such thing as an > > > > encoder state, encoders generally do not have a state on their own. > > > > > > > > Drop the first argument and rename the function to > > > > drm_atomic_helper_check_wb_connector_state(). > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > > > > --- > > > > > > > > Resending, no reaction for two months > > > > > > > > --- > > > > drivers/gpu/drm/drm_atomic_helper.c | 10 ++++------ > > > > drivers/gpu/drm/vkms/vkms_writeback.c | 2 +- > > > > include/drm/drm_atomic_helper.h | 3 +-- > > > > 3 files changed, 6 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > > index 2444fc33dd7c..d69591381f00 100644 > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > > @@ -795,8 +795,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > > > > EXPORT_SYMBOL(drm_atomic_helper_check_modeset); > > > > /** > > > > - * drm_atomic_helper_check_wb_encoder_state() - Check writeback encoder state > > > > - * @encoder: encoder state to check > > > > + * drm_atomic_helper_check_wb_connector_state() - Check writeback connector state > > > > * @conn_state: connector state to check > > > > * > > > > * Checks if the writeback connector state is valid, and returns an error if it > > > > @@ -806,8 +805,7 @@ EXPORT_SYMBOL(drm_atomic_helper_check_modeset); > > > > * Zero for success or -errno > > > > */ > > > > int > > > > -drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder, > > > > - struct drm_connector_state *conn_state) > > > > +drm_atomic_helper_check_wb_connector_state(struct drm_connector_state *conn_state) > > > > > > AFAIK, all the helpers take the object as first argument, so I'm fine > > > with the name change but it should take a drm_connector too. And ideally > > > a drm_atomic_state pointer instead of drm_connector_state too. > > > > I think we then might take even further step and pass > > drm_writeback_connector to this function. I'll send this as a part of v2. > > ... Which is still not the usual function prototype for atomic_check > helpers. On one hand, you are correct. On the other hand, don't we want to emphasise that this function is to be called only for drm_writeback_connector objects? -- With best wishes Dmitry
