Re: [PATCH 2/5] drm/tidss: Set bus_format correctly from bridge/connector

2020-11-09 Thread Nikhil Devshatwar 
On 00:57-20201030, Laurent Pinchart wrote:
> Hi Nikhil,
> 
> Thank you for the patch.
> 
> On Fri, Oct 16, 2020 at 04:09:14PM +0530, Nikhil Devshatwar wrote:
> > When there is a chain of bridges attached to the encoder,
> > the bus_format should be ideally set from the input format of the
> > first bridge in the chain.
> > 
> > Use the bridge state to get the negotiated bus_format.
> > If the bridge does not support format negotiation, error out
> > and fail.
> > 
> > Signed-off-by: Nikhil Devshatwar 
> > ---
> >  drivers/gpu/drm/tidss/tidss_encoder.c | 16 +++-
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c 
> > b/drivers/gpu/drm/tidss/tidss_encoder.c
> > index e278a9c89476..ae7f134754b7 100644
> > --- a/drivers/gpu/drm/tidss/tidss_encoder.c
> > +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
> > @@ -22,6 +22,7 @@ static int tidss_encoder_atomic_check(struct drm_encoder 
> > *encoder,
> > struct drm_device *ddev = encoder->dev;
> > struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc_state);
> > struct drm_display_info *di = _state->connector->display_info;
> > +   struct drm_bridge_state *bstate;
> > struct drm_bridge *bridge;
> > bool bus_flags_set = false;
> >  
> > @@ -41,14 +42,19 @@ static int tidss_encoder_atomic_check(struct 
> > drm_encoder *encoder,
> > break;
> > }
> >  
> > -   if (!di->bus_formats || di->num_bus_formats == 0)  {
> > -   dev_err(ddev->dev, "%s: No bus_formats in connected display\n",
> > -   __func__);
> > +   /* Copy the bus_format from the input_bus_format of first bridge */
> > +   bridge = drm_bridge_chain_get_first_bridge(encoder);
> > +   bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge);
> > +   if (bstate)
> > +   tcrtc_state->bus_format = bstate->input_bus_cfg.format;
> > +
> > +   if (tcrtc_state->bus_format == 0 ||
> > +   tcrtc_state->bus_format == MEDIA_BUS_FMT_FIXED) {
> > +
> > +   dev_err(ddev->dev, "Bridge connected to the encoder did not 
> > specify media bus format\n");
> > return -EINVAL;
> > }
> >  
> > -   // XXX any cleaner way to set bus format and flags?
> > -   tcrtc_state->bus_format = di->bus_formats[0];
> > if (!bus_flags_set)
> > tcrtc_state->bus_flags = di->bus_flags;
> 
> Shouldn't the flags also be retrieved from the bridge state ?

Yes, the code does that above, not covered in the diff context.
When no bridges have reported the timings,
it uses the display_info as fallback (when bus_flags_set is false)

Nikhil D

> 
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm/tidss: Set bus_format correctly from bridge/connector

2020-10-29 Thread Laurent Pinchart 
Hi Nikhil,

Thank you for the patch.

On Fri, Oct 16, 2020 at 04:09:14PM +0530, Nikhil Devshatwar wrote:
> When there is a chain of bridges attached to the encoder,
> the bus_format should be ideally set from the input format of the
> first bridge in the chain.
> 
> Use the bridge state to get the negotiated bus_format.
> If the bridge does not support format negotiation, error out
> and fail.
> 
> Signed-off-by: Nikhil Devshatwar 
> ---
>  drivers/gpu/drm/tidss/tidss_encoder.c | 16 +++-
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c 
> b/drivers/gpu/drm/tidss/tidss_encoder.c
> index e278a9c89476..ae7f134754b7 100644
> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
> @@ -22,6 +22,7 @@ static int tidss_encoder_atomic_check(struct drm_encoder 
> *encoder,
>   struct drm_device *ddev = encoder->dev;
>   struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc_state);
>   struct drm_display_info *di = _state->connector->display_info;
> + struct drm_bridge_state *bstate;
>   struct drm_bridge *bridge;
>   bool bus_flags_set = false;
>  
> @@ -41,14 +42,19 @@ static int tidss_encoder_atomic_check(struct drm_encoder 
> *encoder,
>   break;
>   }
>  
> - if (!di->bus_formats || di->num_bus_formats == 0)  {
> - dev_err(ddev->dev, "%s: No bus_formats in connected display\n",
> - __func__);
> + /* Copy the bus_format from the input_bus_format of first bridge */
> + bridge = drm_bridge_chain_get_first_bridge(encoder);
> + bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge);
> + if (bstate)
> + tcrtc_state->bus_format = bstate->input_bus_cfg.format;
> +
> + if (tcrtc_state->bus_format == 0 ||
> + tcrtc_state->bus_format == MEDIA_BUS_FMT_FIXED) {
> +
> + dev_err(ddev->dev, "Bridge connected to the encoder did not 
> specify media bus format\n");
>   return -EINVAL;
>   }
>  
> - // XXX any cleaner way to set bus format and flags?
> - tcrtc_state->bus_format = di->bus_formats[0];
>   if (!bus_flags_set)
>   tcrtc_state->bus_flags = di->bus_flags;

Shouldn't the flags also be retrieved from the bridge state ?

>  

-- 
Regards,

Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] drm/tidss: Set bus_format correctly from bridge/connector

2020-10-16 Thread Nikhil Devshatwar 
When there is a chain of bridges attached to the encoder,
the bus_format should be ideally set from the input format of the
first bridge in the chain.

Use the bridge state to get the negotiated bus_format.
If the bridge does not support format negotiation, error out
and fail.

Signed-off-by: Nikhil Devshatwar 
---
 drivers/gpu/drm/tidss/tidss_encoder.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c 
b/drivers/gpu/drm/tidss/tidss_encoder.c
index e278a9c89476..ae7f134754b7 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.c
+++ b/drivers/gpu/drm/tidss/tidss_encoder.c
@@ -22,6 +22,7 @@ static int tidss_encoder_atomic_check(struct drm_encoder 
*encoder,
struct drm_device *ddev = encoder->dev;
struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc_state);
struct drm_display_info *di = _state->connector->display_info;
+   struct drm_bridge_state *bstate;
struct drm_bridge *bridge;
bool bus_flags_set = false;
 
@@ -41,14 +42,19 @@ static int tidss_encoder_atomic_check(struct drm_encoder 
*encoder,
break;
}
 
-   if (!di->bus_formats || di->num_bus_formats == 0)  {
-   dev_err(ddev->dev, "%s: No bus_formats in connected display\n",
-   __func__);
+   /* Copy the bus_format from the input_bus_format of first bridge */
+   bridge = drm_bridge_chain_get_first_bridge(encoder);
+   bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge);
+   if (bstate)
+   tcrtc_state->bus_format = bstate->input_bus_cfg.format;
+
+   if (tcrtc_state->bus_format == 0 ||
+   tcrtc_state->bus_format == MEDIA_BUS_FMT_FIXED) {
+
+   dev_err(ddev->dev, "Bridge connected to the encoder did not 
specify media bus format\n");
return -EINVAL;
}
 
-   // XXX any cleaner way to set bus format and flags?
-   tcrtc_state->bus_format = di->bus_formats[0];
if (!bus_flags_set)
tcrtc_state->bus_flags = di->bus_flags;
 
-- 
2.17.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel