| Message ID | 20211026113409.7242-6-igormtorrente@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add new formats support to vkms | expand |
Hi, On 10/26/21 08:34, Igor Torrente wrote: > Add a helper function to validate the connector configuration receive in > the encoder atomic_check by the drivers. > > So the drivers don't need do these common validations themselves. > > Signed-off-by: Igor Torrente <igormtorrente@gmail.com> > --- > V2: Move the format verification to a new helper at the drm_atomic_helper.c > (Thomas Zimmermann). > --- > drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++++++++++++++ > drivers/gpu/drm/vkms/vkms_writeback.c | 9 +++-- > include/drm/drm_atomic_helper.h | 3 ++ > 3 files changed, 54 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 2c0c6ec92820..c2653b9824b5 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_atomic_helper_check_modeset); > > +/** > + * drm_atomic_helper_check_wb_connector_state() - Check writeback encoder state > + * @encoder: encoder state to check > + * @conn_state: connector state to check > + * > + * Checks if the wriback connector state is valid, and returns a erros if it > + * isn't. > + * > + * RETURNS: > + * Zero for success or -errno > + */ > +int > +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder, > + struct drm_connector_state *conn_state) > +{ > + struct drm_writeback_job *wb_job = conn_state->writeback_job; > + struct drm_property_blob *pixel_format_blob; > + bool format_supported = false; > + struct drm_framebuffer *fb; > + int i, n_formats; > + u32 *formats; > + > + if (!wb_job || !wb_job->fb) > + return 0; I think that this should be removed and that this functions should assume that (wb_job && wb_job->fb) == true. Actually, it's weird to have conn_state as argument and only use it to get the wb_job. Instead, this function could receive wb_job directly. Of course, its name/description would have to change. > + > + pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr; > + n_formats = pixel_format_blob->length / sizeof(u32); > + formats = pixel_format_blob->data; > + fb = wb_job->fb; > + > + for (i = 0; i < n_formats; i++) { > + if (fb->format->format == formats[i]) { > + format_supported = true; > + break; > + } > + } > + > + if (!format_supported) { > + DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", > + &fb->format->format); > + return -EINVAL; > + } > + > + return 0; If you do this, you can get rid of the format_supported flag: for(...) { if (fb->format->format == formats[i]) return 0; } DRM_DEBUG_KMS(...); return -EINVAL; Thanks, Leandro Ribeiro > +} > +EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state); > + > /** > * drm_atomic_helper_check_plane_state() - Check plane state for validity > * @plane_state: plane state to check > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c > index 32734cdbf6c2..42f3396c523a 100644 > --- a/drivers/gpu/drm/vkms/vkms_writeback.c > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c > @@ -30,6 +30,7 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder, > { > struct drm_framebuffer *fb; > const struct drm_display_mode *mode = &crtc_state->mode; > + int ret; > > if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > return 0; > @@ -41,11 +42,9 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder, > return -EINVAL; > } > > - if (fb->format->format != vkms_wb_formats[0]) { > - DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", > - &fb->format->format); > - return -EINVAL; > - } > + ret = drm_atomic_helper_check_wb_encoder_state(encoder, conn_state); > + if (ret < 0) > + return ret; > > return 0; > } > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 4045e2507e11..3fbf695da60f 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -40,6 +40,9 @@ 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); > int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, > const struct drm_crtc_state *crtc_state, > int min_scale, >
Hi Leandro, On 10/28/21 6:38 PM, Leandro Ribeiro wrote: > Hi, > > On 10/26/21 08:34, Igor Torrente wrote: >> Add a helper function to validate the connector configuration receive in >> the encoder atomic_check by the drivers. >> >> So the drivers don't need do these common validations themselves. >> >> Signed-off-by: Igor Torrente <igormtorrente@gmail.com> >> --- >> V2: Move the format verification to a new helper at the drm_atomic_helper.c >> (Thomas Zimmermann). >> --- >> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++++++++++++++ >> drivers/gpu/drm/vkms/vkms_writeback.c | 9 +++-- >> include/drm/drm_atomic_helper.h | 3 ++ >> 3 files changed, 54 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index 2c0c6ec92820..c2653b9824b5 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, >> } >> EXPORT_SYMBOL(drm_atomic_helper_check_modeset); >> >> +/** >> + * drm_atomic_helper_check_wb_connector_state() - Check writeback encoder state >> + * @encoder: encoder state to check >> + * @conn_state: connector state to check >> + * >> + * Checks if the wriback connector state is valid, and returns a erros if it >> + * isn't. >> + * >> + * RETURNS: >> + * Zero for success or -errno >> + */ >> +int >> +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder, >> + struct drm_connector_state *conn_state) >> +{ >> + struct drm_writeback_job *wb_job = conn_state->writeback_job; >> + struct drm_property_blob *pixel_format_blob; >> + bool format_supported = false; >> + struct drm_framebuffer *fb; >> + int i, n_formats; >> + u32 *formats; >> + >> + if (!wb_job || !wb_job->fb) >> + return 0; > > I think that this should be removed and that this functions should > assume that (wb_job && wb_job->fb) == true. Ok. > > Actually, it's weird to have conn_state as argument and only use it to > get the wb_job. Instead, this function could receive wb_job directly. In the Thomas review of v1, he said that maybe other things could be tested in this helper. I'm not sure what these additional checks could be, so I tried to design the function signature expecting more things to be added after his review. As you can see, the helper is receiving the `drm_encoder` and doing nothing with it. If we, eventually, don't find anything else that this helper can do, I will revert to something very similar (if not equal) to your proposal. I just want to wait for Thomas's review first. > > Of course, its name/description would have to change. > >> + >> + pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr; >> + n_formats = pixel_format_blob->length / sizeof(u32); >> + formats = pixel_format_blob->data; >> + fb = wb_job->fb; >> + >> + for (i = 0; i < n_formats; i++) { >> + if (fb->format->format == formats[i]) { >> + format_supported = true; >> + break; >> + } >> + } >> + >> + if (!format_supported) { >> + DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", >> + &fb->format->format); >> + return -EINVAL; >> + } >> + >> + return 0; > > If you do this, you can get rid of the format_supported flag: > > for(...) { > if (fb->format->format == formats[i]) > return 0; > } > > > DRM_DEBUG_KMS(...); > return -EINVAL; > Indeed. Thanks! > Thanks, > Leandro Ribeiro > >> +} >> +EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state); >> + >> /** >> * drm_atomic_helper_check_plane_state() - Check plane state for validity >> * @plane_state: plane state to check >> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c >> index 32734cdbf6c2..42f3396c523a 100644 >> --- a/drivers/gpu/drm/vkms/vkms_writeback.c >> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c >> @@ -30,6 +30,7 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder, >> { >> struct drm_framebuffer *fb; >> const struct drm_display_mode *mode = &crtc_state->mode; >> + int ret; >> >> if (!conn_state->writeback_job || !conn_state->writeback_job->fb) >> return 0; >> @@ -41,11 +42,9 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder, >> return -EINVAL; >> } >> >> - if (fb->format->format != vkms_wb_formats[0]) { >> - DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", >> - &fb->format->format); >> - return -EINVAL; >> - } >> + ret = drm_atomic_helper_check_wb_encoder_state(encoder, conn_state); >> + if (ret < 0) >> + return ret; >> >> return 0; >> } >> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h >> index 4045e2507e11..3fbf695da60f 100644 >> --- a/include/drm/drm_atomic_helper.h >> +++ b/include/drm/drm_atomic_helper.h >> @@ -40,6 +40,9 @@ 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); >> int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, >> const struct drm_crtc_state *crtc_state, >> int min_scale, >> Thanks, --- Igor M. A. Torrente
Hi, On 11/3/21 12:03, Igor Torrente wrote: > Hi Leandro, > > On 10/28/21 6:38 PM, Leandro Ribeiro wrote: >> Hi, >> >> On 10/26/21 08:34, Igor Torrente wrote: >>> Add a helper function to validate the connector configuration receive in >>> the encoder atomic_check by the drivers. >>> >>> So the drivers don't need do these common validations themselves. >>> >>> Signed-off-by: Igor Torrente <igormtorrente@gmail.com> >>> --- >>> V2: Move the format verification to a new helper at the >>> drm_atomic_helper.c >>> (Thomas Zimmermann). >>> --- >>> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++++++++++++++ >>> drivers/gpu/drm/vkms/vkms_writeback.c | 9 +++-- >>> include/drm/drm_atomic_helper.h | 3 ++ >>> 3 files changed, 54 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c >>> b/drivers/gpu/drm/drm_atomic_helper.c >>> index 2c0c6ec92820..c2653b9824b5 100644 >>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>> @@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct >>> drm_device *dev, >>> } >>> EXPORT_SYMBOL(drm_atomic_helper_check_modeset); >>> +/** >>> + * drm_atomic_helper_check_wb_connector_state() - Check writeback >>> encoder state >>> + * @encoder: encoder state to check >>> + * @conn_state: connector state to check >>> + * >>> + * Checks if the wriback connector state is valid, and returns a >>> erros if it >>> + * isn't. >>> + * >>> + * RETURNS: >>> + * Zero for success or -errno >>> + */ >>> +int >>> +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder, >>> + struct drm_connector_state *conn_state) >>> +{ >>> + struct drm_writeback_job *wb_job = conn_state->writeback_job; >>> + struct drm_property_blob *pixel_format_blob; >>> + bool format_supported = false; >>> + struct drm_framebuffer *fb; >>> + int i, n_formats; >>> + u32 *formats; >>> + >>> + if (!wb_job || !wb_job->fb) >>> + return 0; >> >> I think that this should be removed and that this functions should >> assume that (wb_job && wb_job->fb) == true. > > Ok. > >> >> Actually, it's weird to have conn_state as argument and only use it to >> get the wb_job. Instead, this function could receive wb_job directly. > > In the Thomas review of v1, he said that maybe other things could be > tested in this helper. I'm not sure what these additional checks could > be, so I tried to design the function signature expecting more things > to be added after his review. > > As you can see, the helper is receiving the `drm_encoder` and doing > nothing with it. > > If we, eventually, don't find anything else that this helper can do, I > will revert to something very similar (if not equal) to your proposal. > I just want to wait for Thomas's review first. > Sure, that makes sense. Thanks, Leandro Ribeiro >> >> Of course, its name/description would have to change. >> >>> + >>> + pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr; >>> + n_formats = pixel_format_blob->length / sizeof(u32); >>> + formats = pixel_format_blob->data; >>> + fb = wb_job->fb; >>> + >>> + for (i = 0; i < n_formats; i++) { >>> + if (fb->format->format == formats[i]) { >>> + format_supported = true; >>> + break; >>> + } >>> + } >>> + >>> + if (!format_supported) { >>> + DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", >>> + &fb->format->format); >>> + return -EINVAL; >>> + } >>> + >>> + return 0; >> >> If you do this, you can get rid of the format_supported flag: >> >> for(...) { >> if (fb->format->format == formats[i]) >> return 0; >> } >> >> >> DRM_DEBUG_KMS(...); >> return -EINVAL; >> > > Indeed. Thanks! > >> Thanks, >> Leandro Ribeiro >> >>> +} >>> +EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state); >>> + >>> /** >>> * drm_atomic_helper_check_plane_state() - Check plane state for >>> validity >>> * @plane_state: plane state to check >>> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c >>> b/drivers/gpu/drm/vkms/vkms_writeback.c >>> index 32734cdbf6c2..42f3396c523a 100644 >>> --- a/drivers/gpu/drm/vkms/vkms_writeback.c >>> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c >>> @@ -30,6 +30,7 @@ static int vkms_wb_encoder_atomic_check(struct >>> drm_encoder *encoder, >>> { >>> struct drm_framebuffer *fb; >>> const struct drm_display_mode *mode = &crtc_state->mode; >>> + int ret; >>> if (!conn_state->writeback_job || >>> !conn_state->writeback_job->fb) >>> return 0; >>> @@ -41,11 +42,9 @@ static int vkms_wb_encoder_atomic_check(struct >>> drm_encoder *encoder, >>> return -EINVAL; >>> } >>> - if (fb->format->format != vkms_wb_formats[0]) { >>> - DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", >>> - &fb->format->format); >>> - return -EINVAL; >>> - } >>> + ret = drm_atomic_helper_check_wb_encoder_state(encoder, >>> conn_state); >>> + if (ret < 0) >>> + return ret; >>> return 0; >>> } >>> diff --git a/include/drm/drm_atomic_helper.h >>> b/include/drm/drm_atomic_helper.h >>> index 4045e2507e11..3fbf695da60f 100644 >>> --- a/include/drm/drm_atomic_helper.h >>> +++ b/include/drm/drm_atomic_helper.h >>> @@ -40,6 +40,9 @@ 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); >>> int drm_atomic_helper_check_plane_state(struct drm_plane_state >>> *plane_state, >>> const struct drm_crtc_state *crtc_state, >>> int min_scale, >>> > > Thanks, > --- > Igor M. A. Torrente
Hi Am 03.11.21 um 16:11 schrieb Leandro Ribeiro: > Hi, > > On 11/3/21 12:03, Igor Torrente wrote: >> Hi Leandro, >> >> On 10/28/21 6:38 PM, Leandro Ribeiro wrote: >>> Hi, >>> >>> On 10/26/21 08:34, Igor Torrente wrote: >>>> Add a helper function to validate the connector configuration receive in >>>> the encoder atomic_check by the drivers. >>>> >>>> So the drivers don't need do these common validations themselves. >>>> >>>> Signed-off-by: Igor Torrente <igormtorrente@gmail.com> >>>> --- >>>> V2: Move the format verification to a new helper at the >>>> drm_atomic_helper.c >>>> (Thomas Zimmermann). >>>> --- >>>> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++++++++++++++ >>>> drivers/gpu/drm/vkms/vkms_writeback.c | 9 +++-- >>>> include/drm/drm_atomic_helper.h | 3 ++ >>>> 3 files changed, 54 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c >>>> b/drivers/gpu/drm/drm_atomic_helper.c >>>> index 2c0c6ec92820..c2653b9824b5 100644 >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>> @@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct >>>> drm_device *dev, >>>> } >>>> EXPORT_SYMBOL(drm_atomic_helper_check_modeset); >>>> +/** >>>> + * drm_atomic_helper_check_wb_connector_state() - Check writeback >>>> encoder state >>>> + * @encoder: encoder state to check >>>> + * @conn_state: connector state to check >>>> + * >>>> + * Checks if the wriback connector state is valid, and returns a 'writeback' 'an error' >>>> erros if it 'error' >>>> + * isn't. >>>> + * >>>> + * RETURNS: >>>> + * Zero for success or -errno >>>> + */ >>>> +int >>>> +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder, >>>> + struct drm_connector_state *conn_state) >>>> +{ >>>> + struct drm_writeback_job *wb_job = conn_state->writeback_job; >>>> + struct drm_property_blob *pixel_format_blob; >>>> + bool format_supported = false; >>>> + struct drm_framebuffer *fb; >>>> + int i, n_formats; Just 'nformats'. Please make both variables 'size_t'. >>>> + u32 *formats; >>>> + >>>> + if (!wb_job || !wb_job->fb) >>>> + return 0; >>> >>> I think that this should be removed and that this functions should >>> assume that (wb_job && wb_job->fb) == true. >> >> Ok. In regular atomic check for planes, there can be planes with no attached framebuffer. Helpers handle this situation. [1] I don't know if this is possible in writeback code, but for consistency, it would make sense to keep this test here. Not sure though. >> >>> >>> Actually, it's weird to have conn_state as argument and only use it to >>> get the wb_job. Instead, this function could receive wb_job directly. >> >> In the Thomas review of v1, he said that maybe other things could be >> tested in this helper. I'm not sure what these additional checks could >> be, so I tried to design the function signature expecting more things >> to be added after his review. >> >> As you can see, the helper is receiving the `drm_encoder` and doing >> nothing with it. >> >> If we, eventually, don't find anything else that this helper can do, I >> will revert to something very similar (if not equal) to your proposal. >> I just want to wait for Thomas's review first. >> > > Sure, that makes sense. We had many helper functions for atomic modesetting that took various arguments for whatever they required. Extending such a function with new functionality/arguments required required touching many drivers and made the parameter list hard to read. At some point, Maxime went through most of the code and unified it all to pass full state to the helpers. So please keep the connector state. I think it's how we do things ATM. > > Thanks, > Leandro Ribeiro > >>> >>> Of course, its name/description would have to change. >>> >>>> + >>>> + pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr; >>>> + n_formats = pixel_format_blob->length / sizeof(u32); >>>> + formats = pixel_format_blob->data; >>>> + fb = wb_job->fb; >>>> + >>>> + for (i = 0; i < n_formats; i++) { >>>> + if (fb->format->format == formats[i]) { >>>> + format_supported = true; >>>> + break; >>>> + } >>>> + } >>>> + >>>> + if (!format_supported) { >>>> + DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", >>>> + &fb->format->format); Please use drm_dgb_kms() instead. There's a 100-character-per-line limit. The comment probably fits onto a single line.(?) >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return 0; >>> >>> If you do this, you can get rid of the format_supported flag: >>> >>> for(...) { >>> if (fb->format->format == formats[i]) >>> return 0; >>> } >>> >>> >>> DRM_DEBUG_KMS(...); >>> return -EINVAL; >>> >> >> Indeed. Thanks! Yes, that looks nicer. >> >>> Thanks, >>> Leandro Ribeiro >>> >>>> +} >>>> +EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state); >>>> + >>>> /** >>>> * drm_atomic_helper_check_plane_state() - Check plane state for >>>> validity >>>> * @plane_state: plane state to check >>>> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c >>>> b/drivers/gpu/drm/vkms/vkms_writeback.c >>>> index 32734cdbf6c2..42f3396c523a 100644 >>>> --- a/drivers/gpu/drm/vkms/vkms_writeback.c >>>> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c >>>> @@ -30,6 +30,7 @@ static int vkms_wb_encoder_atomic_check(struct >>>> drm_encoder *encoder, >>>> { >>>> struct drm_framebuffer *fb; >>>> const struct drm_display_mode *mode = &crtc_state->mode; >>>> + int ret; >>>> if (!conn_state->writeback_job || >>>> !conn_state->writeback_job->fb) >>>> return 0; >>>> @@ -41,11 +42,9 @@ static int vkms_wb_encoder_atomic_check(struct >>>> drm_encoder *encoder, >>>> return -EINVAL; >>>> } >>>> - if (fb->format->format != vkms_wb_formats[0]) { >>>> - DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", >>>> - &fb->format->format); >>>> - return -EINVAL; >>>> - } >>>> + ret = drm_atomic_helper_check_wb_encoder_state(encoder, >>>> conn_state); >>>> + if (ret < 0) >>>> + return ret; We usually use just 'if (ret)' for such test. No need for a less-than. Best regards Thomas [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L809 >>>> return 0; >>>> } >>>> diff --git a/include/drm/drm_atomic_helper.h >>>> b/include/drm/drm_atomic_helper.h >>>> index 4045e2507e11..3fbf695da60f 100644 >>>> --- a/include/drm/drm_atomic_helper.h >>>> +++ b/include/drm/drm_atomic_helper.h >>>> @@ -40,6 +40,9 @@ 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); >>>> int drm_atomic_helper_check_plane_state(struct drm_plane_state >>>> *plane_state, >>>> const struct drm_crtc_state *crtc_state, >>>> int min_scale, >>>> >> >> Thanks, >> --- >> Igor M. A. Torrente
Hi Thomas, On 11/3/21 12:37 PM, Thomas Zimmermann wrote: > Hi > > Am 03.11.21 um 16:11 schrieb Leandro Ribeiro: >> Hi, >> >> On 11/3/21 12:03, Igor Torrente wrote: >>> Hi Leandro, >>> >>> On 10/28/21 6:38 PM, Leandro Ribeiro wrote: >>>> Hi, >>>> >>>> On 10/26/21 08:34, Igor Torrente wrote: >>>>> Add a helper function to validate the connector configuration receive in >>>>> the encoder atomic_check by the drivers. >>>>> >>>>> So the drivers don't need do these common validations themselves. >>>>> >>>>> Signed-off-by: Igor Torrente <igormtorrente@gmail.com> >>>>> --- >>>>> V2: Move the format verification to a new helper at the >>>>> drm_atomic_helper.c >>>>> (Thomas Zimmermann). >>>>> --- >>>>> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++++++++++++++ >>>>> drivers/gpu/drm/vkms/vkms_writeback.c | 9 +++-- >>>>> include/drm/drm_atomic_helper.h | 3 ++ >>>>> 3 files changed, 54 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c >>>>> b/drivers/gpu/drm/drm_atomic_helper.c >>>>> index 2c0c6ec92820..c2653b9824b5 100644 >>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>>> @@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct >>>>> drm_device *dev, >>>>> } >>>>> EXPORT_SYMBOL(drm_atomic_helper_check_modeset); >>>>> +/** >>>>> + * drm_atomic_helper_check_wb_connector_state() - Check writeback >>>>> encoder state >>>>> + * @encoder: encoder state to check >>>>> + * @conn_state: connector state to check >>>>> + * >>>>> + * Checks if the wriback connector state is valid, and returns a > > 'writeback' > > 'an error' > >>>>> erros if it > > 'error' > >>>>> + * isn't. >>>>> + * >>>>> + * RETURNS: >>>>> + * Zero for success or -errno >>>>> + */ >>>>> +int >>>>> +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder, >>>>> + struct drm_connector_state *conn_state) >>>>> +{ >>>>> + struct drm_writeback_job *wb_job = conn_state->writeback_job; >>>>> + struct drm_property_blob *pixel_format_blob; >>>>> + bool format_supported = false; >>>>> + struct drm_framebuffer *fb; >>>>> + int i, n_formats; > > Just 'nformats'. > > Please make both variables 'size_t'. I Will correct all these minor issues. > > >>>>> + u32 *formats; >>>>> + >>>>> + if (!wb_job || !wb_job->fb) >>>>> + return 0; >>>> >>>> I think that this should be removed and that this functions should >>>> assume that (wb_job && wb_job->fb) == true. >>> >>> Ok. > > In regular atomic check for planes, there can be planes with no attached > framebuffer. Helpers handle this situation. [1] I don't know if this is > possible in writeback code, but for consistency, it would make sense to > keep this test here. Not sure though. @Leandro, do you know if it is possible to have a wb_job without a fb attached? > >>> >>>> >>>> Actually, it's weird to have conn_state as argument and only use it to >>>> get the wb_job. Instead, this function could receive wb_job directly. >>> >>> In the Thomas review of v1, he said that maybe other things could be >>> tested in this helper. I'm not sure what these additional checks could >>> be, so I tried to design the function signature expecting more things >>> to be added after his review. >>> >>> As you can see, the helper is receiving the `drm_encoder` and doing >>> nothing with it. >>> >>> If we, eventually, don't find anything else that this helper can do, I >>> will revert to something very similar (if not equal) to your proposal. >>> I just want to wait for Thomas's review first. >>> >> >> Sure, that makes sense. > > We had many helper functions for atomic modesetting that took various > arguments for whatever they required. Extending such a function with new > functionality/arguments required required touching many drivers and made > the parameter list hard to read. At some point, Maxime went through most > of the code and unified it all to pass full state > So please keep the connector state. I think it's how we do things ATM.to the helpers. > > So please keep the connector state. I think it's how we do things ATM. OK, I will keep then. > >> >> Thanks, >> Leandro Ribeiro >> >>>> >>>> Of course, its name/description would have to change. >>>> >>>>> + >>>>> + pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr; >>>>> + n_formats = pixel_format_blob->length / sizeof(u32); >>>>> + formats = pixel_format_blob->data; >>>>> + fb = wb_job->fb; >>>>> + >>>>> + for (i = 0; i < n_formats; i++) { >>>>> + if (fb->format->format == formats[i]) { >>>>> + format_supported = true; >>>>> + break; >>>>> + } >>>>> + } >>>>> + >>>>> + if (!format_supported) { >>>>> + DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", >>>>> + &fb->format->format); > > Please use drm_dgb_kms() instead. There's a 100-character-per-line > limit. The comment probably fits onto a single line.(?) I will improve that. This code came from the vkms, which follows the 80 chars limit. If I'm not mistaken. > >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + return 0; >>>> >>>> If you do this, you can get rid of the format_supported flag: >>>> >>>> for(...) { >>>> if (fb->format->format == formats[i]) >>>> return 0; >>>> } >>>> >>>> >>>> DRM_DEBUG_KMS(...); >>>> return -EINVAL; >>>> >>> >>> Indeed. Thanks! > > Yes, that looks nicer. > >>> >>>> Thanks, >>>> Leandro Ribeiro >>>> >>>>> +} >>>>> +EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state); >>>>> + >>>>> /** >>>>> * drm_atomic_helper_check_plane_state() - Check plane state for >>>>> validity >>>>> * @plane_state: plane state to check >>>>> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c >>>>> b/drivers/gpu/drm/vkms/vkms_writeback.c >>>>> index 32734cdbf6c2..42f3396c523a 100644 >>>>> --- a/drivers/gpu/drm/vkms/vkms_writeback.c >>>>> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c >>>>> @@ -30,6 +30,7 @@ static int vkms_wb_encoder_atomic_check(struct >>>>> drm_encoder *encoder, >>>>> { >>>>> struct drm_framebuffer *fb; >>>>> const struct drm_display_mode *mode = &crtc_state->mode; >>>>> + int ret; >>>>> if (!conn_state->writeback_job || >>>>> !conn_state->writeback_job->fb) >>>>> return 0; >>>>> @@ -41,11 +42,9 @@ static int vkms_wb_encoder_atomic_check(struct >>>>> drm_encoder *encoder, >>>>> return -EINVAL; >>>>> } >>>>> - if (fb->format->format != vkms_wb_formats[0]) { >>>>> - DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", >>>>> - &fb->format->format); >>>>> - return -EINVAL; >>>>> - } >>>>> + ret = drm_atomic_helper_check_wb_encoder_state(encoder, >>>>> conn_state); >>>>> + if (ret < 0) >>>>> + return ret; > > We usually use just 'if (ret)' for such test. No need for a less-than. I will change that. > > Best regards > Thomas > > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_helper.c#L809 > >>>>> return 0; >>>>> } >>>>> diff --git a/include/drm/drm_atomic_helper.h >>>>> b/include/drm/drm_atomic_helper.h >>>>> index 4045e2507e11..3fbf695da60f 100644 >>>>> --- a/include/drm/drm_atomic_helper.h >>>>> +++ b/include/drm/drm_atomic_helper.h >>>>> @@ -40,6 +40,9 @@ 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); >>>>> int drm_atomic_helper_check_plane_state(struct drm_plane_state >>>>> *plane_state, >>>>> const struct drm_crtc_state *crtc_state, >>>>> int min_scale, >>>>> >>> >>> Thanks, >>> --- >>> Igor M. A. Torrente > Thanks, --- Igor M. A. Torrente
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 2c0c6ec92820..c2653b9824b5 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -766,6 +766,53 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, } EXPORT_SYMBOL(drm_atomic_helper_check_modeset); +/** + * drm_atomic_helper_check_wb_connector_state() - Check writeback encoder state + * @encoder: encoder state to check + * @conn_state: connector state to check + * + * Checks if the wriback connector state is valid, and returns a erros if it + * isn't. + * + * RETURNS: + * Zero for success or -errno + */ +int +drm_atomic_helper_check_wb_encoder_state(struct drm_encoder *encoder, + struct drm_connector_state *conn_state) +{ + struct drm_writeback_job *wb_job = conn_state->writeback_job; + struct drm_property_blob *pixel_format_blob; + bool format_supported = false; + struct drm_framebuffer *fb; + int i, n_formats; + u32 *formats; + + if (!wb_job || !wb_job->fb) + return 0; + + pixel_format_blob = wb_job->connector->pixel_formats_blob_ptr; + n_formats = pixel_format_blob->length / sizeof(u32); + formats = pixel_format_blob->data; + fb = wb_job->fb; + + for (i = 0; i < n_formats; i++) { + if (fb->format->format == formats[i]) { + format_supported = true; + break; + } + } + + if (!format_supported) { + DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", + &fb->format->format); + return -EINVAL; + } + + return 0; +} +EXPORT_SYMBOL(drm_atomic_helper_check_wb_encoder_state); + /** * drm_atomic_helper_check_plane_state() - Check plane state for validity * @plane_state: plane state to check diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c index 32734cdbf6c2..42f3396c523a 100644 --- a/drivers/gpu/drm/vkms/vkms_writeback.c +++ b/drivers/gpu/drm/vkms/vkms_writeback.c @@ -30,6 +30,7 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder, { struct drm_framebuffer *fb; const struct drm_display_mode *mode = &crtc_state->mode; + int ret; if (!conn_state->writeback_job || !conn_state->writeback_job->fb) return 0; @@ -41,11 +42,9 @@ static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder, return -EINVAL; } - if (fb->format->format != vkms_wb_formats[0]) { - DRM_DEBUG_KMS("Invalid pixel format %p4cc\n", - &fb->format->format); - return -EINVAL; - } + ret = drm_atomic_helper_check_wb_encoder_state(encoder, conn_state); + if (ret < 0) + return ret; return 0; } diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 4045e2507e11..3fbf695da60f 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -40,6 +40,9 @@ 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); int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, const struct drm_crtc_state *crtc_state, int min_scale,
Add a helper function to validate the connector configuration receive in the encoder atomic_check by the drivers. So the drivers don't need do these common validations themselves. Signed-off-by: Igor Torrente <igormtorrente@gmail.com> --- V2: Move the format verification to a new helper at the drm_atomic_helper.c (Thomas Zimmermann). --- drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++++++++++++++ drivers/gpu/drm/vkms/vkms_writeback.c | 9 +++-- include/drm/drm_atomic_helper.h | 3 ++ 3 files changed, 54 insertions(+), 5 deletions(-)