Re: [PATCH v2 2/2] drm/panel: boe-th101mb31ig002 : using drm_connector_helper_get_modes_fixed()

[Doug Anderson]

Hi,

On Tue, Jul 16, 2024 at 5:11 AM Zhaoxiong Lv
<lvzhaoxiong@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> Use public functions(drm_connector_helper_get_modes_fixed()) to
> get porch parameters.
>
> Signed-off-by: Zhaoxiong Lv <lvzhaoxiong@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> ---
>  .../drm/panel/panel-boe-th101mb31ig002-28a.c  | 26 ++-----------------
>  1 file changed, 2 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-boe-th101mb31ig002-28a.c b/drivers/gpu/drm/panel/panel-boe-th101mb31ig002-28a.c
> index d4e4abd103bb..4a61a11099b6 100644
> --- a/drivers/gpu/drm/panel/panel-boe-th101mb31ig002-28a.c
> +++ b/drivers/gpu/drm/panel/panel-boe-th101mb31ig002-28a.c
> @@ -16,6 +16,7 @@
>  #include <drm/drm_mipi_dsi.h>
>  #include <drm/drm_modes.h>
>  #include <drm/drm_panel.h>
> +#include <drm/drm_probe_helper.h>
>
>  struct boe_th101mb31ig002;
>
> @@ -313,31 +314,8 @@ static int boe_th101mb31ig002_get_modes(struct drm_panel *panel,
>                                                       struct boe_th101mb31ig002,
>                                                       panel);
>         const struct drm_display_mode *desc_mode = ctx->desc->modes;
> -       struct drm_display_mode *mode;
>
> -       mode = drm_mode_duplicate(connector->dev, desc_mode);
> -       if (!mode) {
> -               dev_err(panel->dev, "Failed to add mode %ux%u@%u\n",
> -                       desc_mode->hdisplay, desc_mode->vdisplay,
> -                       drm_mode_vrefresh(desc_mode));
> -               return -ENOMEM;
> -       }
> -
> -       drm_mode_set_name(mode);
> -
> -       connector->display_info.bpc = 8;

I notice that drm_connector_helper_get_modes_fixed() doesn't seem to
set bpc. Unless I'm mistaken and that gets set automatically somewhere
else then you should keep that, right?

> -       connector->display_info.width_mm = mode->width_mm;
> -       connector->display_info.height_mm = mode->height_mm;
> -
> -       /*
> -        * TODO: Remove once all drm drivers call
> -        * drm_connector_set_orientation_from_panel()
> -        */
> -       drm_connector_set_panel_orientation(connector, ctx->orientation);

Are we confident that all the other users of this panel are properly
getting the orientation and we can remove the above bit of code? It
looks like one other user is 'rk3566-pinetab2'.

>From what I recall, the relevant commits are commit 15b9ca1641f0
("drm: Config orientation property if panel provides it") and commit
e3ea1806e4ad ("drm/bridge: panel: Set orientation on panel_bridge
connector"). I think in all cases the assumption was that, to get the
right functionality we need to switch to "panel_bridge". That happens
when we use drmm_of_get_bridge() or devm_drm_of_get_bridge(). ...but
it looks like Rockchip DRM is directly using
drm_of_find_panel_or_bridge() and thus hasn't switched to panel
bridge.

...so, unless I'm mistaken, the other users of this panel driver still
need the drm_connector_set_panel_orientation() call here and you
shouldn't remove it. Perhaps Alexander Warnecke could comment about
whether this is still needed. ...or perhaps someone who maintains
Rockchip DRM can say whether they have any plans around this area?

If, for some reason, you do remove it then it should at least be
called out in the description since this is a functionality change.

-Doug