Re: [v8 4/6] drm/panel-simple: Update validation warnings for eDP panel description

[Laurent Pinchart]

Hi Doug,

On Mon, Jun 28, 2021 at 08:34:04AM -0700, Doug Anderson wrote:
> On Mon, Jun 28, 2021 at 6:33 AM Laurent Pinchart wrote:
> > On Mon, Jun 28, 2021 at 05:46:24PM +0530, rajeevny@xxxxxxxxxxxxxx wrote:
> > > On 27-06-2021 23:48, Laurent Pinchart wrote:
> > > > On Sat, Jun 26, 2021 at 10:21:06PM +0530, Rajeev Nandan wrote:
> > > >> Do not give a warning for the eDP panels if the "bus_format" is
> > > >> not specified, since most eDP panels can support more than one
> > > >> bus formats and this can be auto-detected.
> > > >> Also, update the check to include bpc=10 for the eDP panel.
> > > >>
> > > >> Signed-off-by: Rajeev Nandan <rajeevny@xxxxxxxxxxxxxx>
> > > >> ---
> > > >>
> > > >> Changes in v8:
> > > >> - New patch, to address the review comments of Sam Ravnborg [1]
> > > >>
> > > >> [1]
> > > >> https://lore.kernel.org/dri-devel/20210621184157.GB918146@xxxxxxxxxxxx/
> > > >>
> > > >>  drivers/gpu/drm/panel/panel-simple.c | 6 ++----
> > > >>  1 file changed, 2 insertions(+), 4 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> > > >> b/drivers/gpu/drm/panel/panel-simple.c
> > > >> index 86e5a45..f966b562 100644
> > > >> --- a/drivers/gpu/drm/panel/panel-simple.c
> > > >> +++ b/drivers/gpu/drm/panel/panel-simple.c
> > > >> @@ -772,10 +772,8 @@ static int panel_simple_probe(struct device *dev,
> > > >> const struct panel_desc *desc,
> > > >>                    desc->bpc != 8);
> > > >>            break;
> > > >>    case DRM_MODE_CONNECTOR_eDP:
> > > >> -          if (desc->bus_format == 0)
> > > >> -                  dev_warn(dev, "Specify missing bus_format\n");
> > > >> -          if (desc->bpc != 6 && desc->bpc != 8)
> > > >> -                  dev_warn(dev, "Expected bpc in {6,8} but got: %u\n", desc->bpc);
> > > >> +          if (desc->bpc != 6 && desc->bpc != 8 && desc->bpc != 10)
> > > >> +                  dev_warn(dev, "Expected bpc in {6,8,10} but got: %u\n", desc->bpc);
> > > >
> > > > You'll still get a warning is bpc == 0, is that intentional ?
> > >
> > > This was not intentional, I missed considering bpc=0 case. As we are
> > > removing the warning for bus_format=0 then a similar thing can be done
> > > for the bpc=0 also. The bpc value should be a valid one if it is
> > > specified. Unlike the bus_format, bpc has few possible values that can
> > > be checked here along with 0. Please correct me if I misunderstood the
> > > concept.
> > > I will fix this.
> >
> > What's the point of specifying bpc if it's optional though ? Users of
> > the panel will need to support the case where bpc is set to 0. Have you
> > ensured that they all do ? Can they meaningfully use the bpc value if
> > they need to be ready to support bpc == 0 ?
> 
> I must be missing something, but to me it seems like Rajeev's patch is
> fine as-is. From my reading of the code:
> 
> * Removes the warning if bus_format == 0. This is correct since I
> don't think specifying bus format for eDP panels makes lots of sense.

This is embarassing, I've been reading it as desc->bpc == 0 from the
beginning :-( My bad. The bpc change is correct.

> * Removes the warning if bpc == 10. This is correct since we've seen
> eDP panels with 10bpc.
> 
> * Keeps the warning if bpc == 0. IMO we can/should still require
> panels to specify their BPC. I guess I'm treating this as a "max BPC".
> I know that we use this field in the sn65dsi86 driver, so if it's OK
> for this to be 0 then we'll have to change that driver to handle it.
> 
> Does that sound right to you Laurent? So since I think Rajeev's patch
> is OK, I'm happy with:
> 
> Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> 
> Unless I missed something and this patch needs to change then it feels
> like Rajeev's patch series is in pretty good shape to land. I'm happy
> to commit it but since Sam made comments on the previous version I'd
> plan to wait a bit to make sure he has a chance for another look if he
> wants to. I've also only got 2 days left before I vanish for 1 week of
> vacation. ...so my plan is:
> * If Sam / Laurent come back before tomorrow and say they're happy
> then I'll commit.
> * If I hear nothing then I'll check back after my vacation. If someone
> else has committed then I'll be happy. If not and there has just been
> silence then I'll commit it myself.
> 
> Please yell if that's not OK. :-)

-- 
Regards,

Laurent Pinchart