| Message ID | 20231201220843.2023117-1-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] drm/atomic-helper: rename drm_atomic_helper_check_wb_encoder_state | expand |
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@linaro.org> >> --- >> >> 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. > >> { >> struct drm_writeback_job *wb_job = conn_state->writeback_job; >> struct drm_property_blob *pixel_format_blob; >> @@ -827,11 +825,11 @@ drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder, >> if (fb->format->format == formats[i]) >> return 0; >> >> - drm_dbg_kms(encoder->dev, "Invalid pixel format %p4cc\n", &fb->format->format); >> + drm_dbg_kms(conn_state->connector->dev, "Invalid pixel format %p4cc\n", &fb->format->format); > > Which would also avoid the checkpatch warning there. > > Maxime
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@linaro.org> > > > --- > > > > > > 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. Maxime
On Tue, 5 Dec 2023 at 13:48, Maxime Ripard <mripard@kernel.org> 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@linaro.org> > > > > --- > > > > > > > > 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?
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) { struct drm_writeback_job *wb_job = conn_state->writeback_job; struct drm_property_blob *pixel_format_blob; @@ -827,11 +825,11 @@ drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder, if (fb->format->format == formats[i]) return 0; - drm_dbg_kms(encoder->dev, "Invalid pixel format %p4cc\n", &fb->format->format); + drm_dbg_kms(conn_state->connector->dev, "Invalid pixel format %p4cc\n", &fb->format->format); return -EINVAL; } -EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state); +EXPORT_SYMBOL(drm_atomic_helper_check_wb_connector_state); /** * drm_atomic_helper_check_plane_state() - Check plane state for validity diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c index d7e63aa14663..56edec6f1634 100644 --- a/drivers/gpu/drm/vkms/vkms_writeback.c +++ b/drivers/gpu/drm/vkms/vkms_writeback.c @@ -48,7 +48,7 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder, return -EINVAL; } - ret = drm_atomic_helper_check_wb_encoder_state(encoder, conn_state); + ret = drm_atomic_helper_check_wb_connector_state(conn_state); if (ret < 0) return ret; diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 536a0b0091c3..742ccbcd7809 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -50,8 +50,7 @@ struct drm_private_state; int drm_atomic_helper_check_modeset(struct drm_device *dev, struct drm_atomic_state *state); 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); int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, const struct drm_crtc_state *crtc_state, int min_scale,
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@linaro.org> --- 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(-)